feat!: limit the max tx size to 2 MiB #3909
Conversation
|
specs/src/ante_handler_v3.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| The AnteHandler chains together several decorators to ensure the following criteria are met for app version 3: | |||
|
|
|||
| - The tx does not contain any messages that are unsupported by the current app version. See `MsgVersioningGateKeeper`. | |||
| - The tx size is not larger than the application's configured versioned constant MaxTxSize. | |||
There was a problem hiding this comment.
I'll add the link to MaxTxSize in appconsts once this pr is merged
ninabarbakadze
requested review from
rootulp and
evan-forbes
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new decorator, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
pkg/appconsts/versioned_consts.go (2)
45-47: Consider implementing the previous review suggestionAs suggested in a previous review, consider removing the parameter name for consistency:
-func MaxTxBytes(_ uint64) int { +func MaxTxBytes(uint64) int { return v3.MaxTxBytes }This change improves code clarity by explicitly showing that the parameter is unused.
45-47: Add a comment explaining the unused parameterConsider adding a comment to explain the purpose of the unused
uint64parameter. If it's for future versioning, make this explicit:+// MaxTxBytes returns the maximum transaction size in bytes. +// The uint64 parameter is reserved for future versioning and is currently unused. func MaxTxBytes(_ uint64) int { return v3.MaxTxBytes }This addition would improve code documentation and clarify the intent behind the function signature.
app/ante/max_tx_size_test.go (3)
20-50: LGTM: Test cases cover important scenarios.The test cases are well-defined and cover crucial scenarios including valid transaction size, oversized transactions, edge cases, and version compatibility. The use of
v3.MaxTxBytesconstant ensures the test adapts to changes in the maximum transaction size.Consider adding an additional test case for a transaction size slightly below the maximum threshold (e.g.,
v3.MaxTxBytes - 1000) to ensure there's no off-by-one error in the implementation.
52-72: LGTM: Test execution logic is robust and well-implemented.The test execution logic is sound, iterating over each test case, setting up the appropriate context, and verifying the results. The use of the
requirepackage ensures clear test failure messages.Consider adding a comment explaining the purpose of
ctx.WithTxBytes(txBytes)to improve code readability. For example:// Set the transaction bytes in the context to simulate a transaction of the specified size ctx = ctx.WithTxBytes(txBytes)
1-72: Great job on implementing comprehensive tests forMaxTxSizeDecorator!This test file effectively validates the functionality of the
MaxTxSizeDecorator, aligning well with the PR objectives of implementing a maximum transaction size. The tests cover various scenarios, including valid and invalid transaction sizes, edge cases, and version compatibility.The code is well-structured, follows Go best practices, and uses appropriate testing utilities. This will help ensure the robustness of the transaction size limitation feature.
As the project evolves, consider expanding these tests to cover more edge cases and potential real-world scenarios. This could include testing with different types of transactions or simulating network conditions that might affect transaction processing.
specs/src/ante_handler_v3.md (1)
6-6: Approved addition with a suggestion for improvementThe new criterion for transaction size validation aligns well with the PR objectives of capping transaction size to reduce network strain. This is a valuable addition to the AnteHandler specifications.
To enhance clarity and maintainability, consider adding a reference or link to the definition of
MaxTxBytes. This would help developers quickly locate the specific value and any related configuration details.Consider adding a reference like this:
- The tx size is not larger than the application's configured versioned constant MaxTxBytes (defined in [appconsts](link-to-appconsts-file)).app/ante/max_tx_size.go (1)
10-11: Improve comment clarity forMaxTxSizeDecorator.Consider rephrasing the comment to form a complete sentence and enhance readability. For example:
-// MaxTxSizeDecorator ensures that a tx can not be larger than -// application's configured versioned constant. +// MaxTxSizeDecorator ensures that a transaction cannot exceed +// the application's configured maximum size.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/ante/ante.go (1 hunks)
- app/ante/max_tx_size.go (1 hunks)
- app/ante/max_tx_size_test.go (1 hunks)
- pkg/appconsts/v3/app_consts.go (1 hunks)
- pkg/appconsts/versioned_consts.go (1 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
- specs/src/ante_handler_v3.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/appconsts/v3/app_consts.go
🔇 Additional comments (6)
pkg/appconsts/versioned_consts.go (2)
45-47: LGTM: Function aligns with PR objectivesThe
MaxTxBytesfunction correctly provides access to the maximum transaction size, which is crucial for implementing the transaction size cap as per the PR objectives.
45-47: Verify the usage of MaxTxBytes in the codebaseTo ensure that this function is being used correctly to implement the transaction size cap, we should verify its usage across the codebase.
✅ Verification successful
MaxTxBytes usage verified across the codebase
The
MaxTxBytesfunction is consistently utilized in all relevant areas to enforce transaction size limits, ensuring correct implementation of the transaction size cap.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of MaxTxBytes function to verify its implementation in capping transaction sizes. # Test: Search for MaxTxBytes usage echo "Searching for MaxTxBytes usage:" rg --type go "MaxTxBytes\(" -A 5 # Test: Search for potential transaction size checks echo "Searching for potential transaction size checks:" rg --type go "tx.*size|size.*tx" -iLength of output: 15509
app/ante/max_tx_size_test.go (2)
1-14: LGTM: Package declaration and imports are well-structured.The package name and imports are appropriate for the test file. The use of aliases for different versions of app constants (v2, v3) enhances readability.
16-19: LGTM: Test function setup is correct.The test function is properly named and the setup correctly initializes the
MaxTxSizeDecoratorand chains it withsdk.ChainAnteDecorators.pkg/appconsts/versioned_consts_test.go (1)
69-74: LGTM! The new test case aligns well with the PR objectives.The addition of this test case for
MaxTxBytesin version 3 is a good implementation. It directly supports the PR's goal of capping transaction sizes by ensuring that the new constant is correctly set and can be retrieved. This test will help maintain the integrity of the transaction size limit as the codebase evolves.app/ante/ante.go (1)
35-36: Approve addition of NewMaxTxSizeDecorator with suggestionsThe addition of
NewMaxTxSizeDecorator()aligns well with the PR objectives of capping transaction size to alleviate network strain. Its placement in the decorator chain is logical, allowing for early rejection of oversized transactions.However, I have a few suggestions to enhance clarity and ensure proper implementation:
- Please provide information about the implementation of
NewMaxTxSizeDecorator(). What specific size limit is being enforced?- Consider adding a comment to explain how this new decorator interacts with the existing
NewConsumeGasForTxSizeDecorator. Are they complementary or potentially redundant?- It would be helpful to see the implementation of
NewMaxTxSizeDecorator()to ensure it correctly enforces the desired transaction size limit.To verify the implementation and usage of
NewMaxTxSizeDecorator(), please run the following script:✅ Verification successful
Approve addition of NewMaxTxSizeDecorator with implementation verified
The
NewMaxTxSizeDecorator()correctly enforces a transaction size limit of 2 MiB, aligning with the PR objectives of capping transaction size to alleviate network strain. Its placement in the decorator chain allows for early rejection of oversized transactions, and the implementation is supported by relevant test cases (TestMaxTxSizeDecorator).Suggestions:
- Add comments to clearly state the enforced transaction size limit.
- Document the interaction between
NewMaxTxSizeDecoratorand existing decorators to ensure clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of NewMaxTxSizeDecorator # Test: Search for the implementation of NewMaxTxSizeDecorator echo "Searching for NewMaxTxSizeDecorator implementation:" rg --type go "func NewMaxTxSizeDecorator\(" -A 10 # Test: Check for any constants or variables related to max transaction size echo "\nSearching for max transaction size constants or variables:" rg --type go "(MaxTxSize|MAX_TX_SIZE)" # Test: Look for tests related to max transaction size echo "\nSearching for related test cases:" rg --type go "TestMaxTxSize"Length of output: 1955
Script:
#!/bin/bash # Description: Locate the definition and value of MaxTxBytes echo "Searching for the definition of MaxTxBytes:" rg --type go "const\s+MaxTxBytes\s*=" echo "\nSearching for configurations or usages of MaxTxBytes:" rg --type go "MaxTxBytes"Length of output: 1785
app/ante/max_tx_size.go
Outdated
| if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) { | ||
| return ctx, sdkerrors.ErrTxTooLarge |
There was a problem hiding this comment.
Correct the transaction size comparison operator.
The current condition rejects transactions that are exactly the maximum allowed size:
if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {If the intention is to allow transactions up to and including the maximum size, consider changing the comparison operator to >:
-if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {
+if len(ctx.TxBytes()) > appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) {This change ensures that transactions equal to the maximum size are accepted, and only those exceeding it are rejected.
There was a problem hiding this comment.
this is good to note, whichever is used, we simply need to document it
using strictly greater than makes sense to me, as we can just use that constant everywhere instead of subtracting 1 everywhere as well
There was a problem hiding this comment.
I second this that it would be better if it were strictly greater than 2MB
|
Another nit: PR title could be more readable to end-users b/c it will show up in release notes. For example: "feat!: limit the max tx size to 2 MiB". Can also add this to release notes. |
ninabarbakadze
changed the title
Thanks @rootulp! Updated :) |
app/ante/max_tx_size.go
Outdated
| // AnteHandle implements the AnteHandler interface. It ensures that tx size is under application's configured threshold. | ||
| func (d MaxTxSizeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { | ||
| // Tx size rule applies to app versions v3 and onwards. | ||
| if ctx.BlockHeader().Version.App >= v3.Version { |
There was a problem hiding this comment.
[very optional]
imo, if we can exit with a negative, then we should. this follows golang's norms
so
if ctx.BlockHeader().Version.App < v3.Version {
next() // or whatever
}
app/ante/max_tx_size.go
Outdated
| if len(ctx.TxBytes()) >= appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) { | ||
| return ctx, sdkerrors.ErrTxTooLarge |
There was a problem hiding this comment.
this is good to note, whichever is used, we simply need to document it
using strictly greater than makes sense to me, as we can just use that constant everywhere instead of subtracting 1 everywhere as well
celestia-bot
requested review from
cmwaters and
rach-id
and removed request for
a team
| } | ||
|
|
||
| if len(ctx.TxBytes()) > appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) { | ||
| return ctx, sdkerrors.ErrTxTooLarge |
There was a problem hiding this comment.
[nit] The current error lacks context for a user to understand how large their transaction is and how large of a tx the system can support.
Proposal to include two more pieces of information in this error: the size of the current tx and the max tx bytes.
There was a problem hiding this comment.
Also for posterity it looks like mempool.ErrTxTooLarge is thrown by Tendermint if the tx exceeds the mempool's max tx bytes parameter.
In BroadcastTx the Cosmos SDK translates the mempool.ErrTxTooLarge into a sdkerrors.ErrTxTooLarge which is the same error used here.
[proposal] I wonder if we should use a different error to differentiate that this error is not because the tx exceeds the mempool's configured max tx bytes but instead is the application's max tx bytes. Since the mempool can be configured locally, user's may or may not observe a mempool.ErrTxTooLarge (and therefore sdkerrors.ErrTxTooLarge) depending on which node they submit transactions to.
There was a problem hiding this comment.
I can address these in a follow-up since I need to link the new versioned constant in ante handler docs anyway
| return next(ctx, tx, simulate) | ||
| } | ||
|
|
||
| if len(ctx.TxBytes()) > appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) { |
There was a problem hiding this comment.
can we also delete the MaxTotalBlobSizeDecorator?
are there any scenarios where this context is not set properly? like in prepare proposal?
There was a problem hiding this comment.
I think we should still keep it because one limits by number of shares and the other number of bytes. I think you pointed out that you could have a PFB that is full of 1 byte blobs which would exceed the share limit but no the byte limit
There was a problem hiding this comment.
MaxTotalBlobSizeDecorator only applies to app version 1
// Ensure that the tx's total blob size is <= the max blob size.
// Only applies to app version == 1.
blobante.NewMaxTotalBlobSizeDecorator(blobKeeper),
and since the ante handler added in this PR only applies to app version >= 3, I think we need to retain MaxTotalBlobSizeDecorator.
The ante handler that limits based on number of shares is NewBlobShareDecorator and that one is applied to app version >= 2.
// Ensure that the blob shares occupied by the tx <= the max shares
// available to blob data in a data square. Only applies to app version
// >= 2.
blobante.NewBlobShareDecorator(blobKeeper),
evan-forbes
dismissed
their stale review
I think we need tests for prepare and process
Overview
Fixes #3686