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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
app/ante/max_tx_size.go
Outdated
|
|
||
| maxTxBytes := appconsts.MaxTxBytes(ctx.BlockHeader().Version.App) | ||
| if len(ctx.TxBytes()) > maxTxBytes { | ||
| return ctx, fmt.Errorf("tx size is larger than the application's configured threshold: %d bytes", maxTxBytes) |
There was a problem hiding this comment.
[nit] provide the current tx bytes in the error message so that a user knows how many bytes they need to trim from their tx.
app/process_proposal.go
Outdated
| // Set the tx size on the context before calling the AnteHandler | ||
| sdkCtx = sdkCtx.WithTxBytes(tx) |
There was a problem hiding this comment.
[question] does this need to be gated behind a version gate? @evan-forbes mentioned that it is consensus breaking
| // Set the tx size on the context before calling the AnteHandler | |
| sdkCtx = sdkCtx.WithTxBytes(tx) | |
| // Set the tx bytes in the context for app version v3 and greater | |
| if sdkCtx.BlockHeader().Version.App >= 3 { | |
| sdkCtx = sdkCtx.WithTxBytes(tx) | |
| } |
There was a problem hiding this comment.
me and @evan-forbes briefly discussed this yesterday but I don't think we reached a firm conclusion. ConsumeTxSizeGasDecorator also uses TxBytes set on the context so I guess it is consensus breaking? I could also reset it back to empty after MaxTxSizeDecorator is executed.
There was a problem hiding this comment.
because even if we version gate it do we want it to be set during the ante handler execution in v3?
There was a problem hiding this comment.
It seems like a bug that it wasn't set before ConsumeTxSizeGasDecorator.
do we want it to be set during the ante handler execution in v3?
IMO yes
There was a problem hiding this comment.
I added this gate in #3960 (which we can close after this)
I think there's a chance it could be consensus breaking because of the ante handle that subtracts gas for the size of the tx. we don't currently set it in process proposal afaik, which would mean that's its possible to reject or accept a block if an account does or doesn't have enough gas. very unlikely, but I think it could happen.
There was a problem hiding this comment.
so we need a gate in processproposal
we don't need a gate in prepare proposal, we can start applying the logic whenever
There was a problem hiding this comment.
makes sense! thanks!
app/process_proposal.go
Outdated
| // Set the tx size on the context before calling the AnteHandler | ||
| sdkCtx = sdkCtx.WithTxBytes(tx) |
There was a problem hiding this comment.
[same question] does this need to be gated behind a version check?
Can this be hoisted up so that it only needs to happen once in this file? Perhaps after this line:
sdkTx, err := app.txConfig.TxDecoder()(tx)
// Set the tx bytes in the context for app version v3 and greater
if sdkCtx.BlockHeader().Version.App >= 3 {
sdkCtx = sdkCtx.WithTxBytes(tx)
}
// rest of fileThere was a problem hiding this comment.
referencing this comment #3909 (comment)
we need a gate for process proposal, we don't need a gate prepare proposal
| // create 2 single blobTxs that include a large memo making the transaction | ||
| // larger than the configured max tx bytes | ||
| largeBlobTxs := blobfactory.ManyMultiBlobTx( | ||
| t, enc, kr, testutil.ChainID, accounts[3:], infos[3:], | ||
| blobfactory.NestedBlobs( | ||
| t, | ||
| testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4), | ||
| [][]int{{100}, {1000}, {420}, {300}}, | ||
| ), | ||
| user.SetMemo(largeMemo)) |
There was a problem hiding this comment.
[question] how does this function create 2 single blobTxs? I don't see 2 used as an argument to ManyMultiBlobTx
There was a problem hiding this comment.
I'll create as many blobs as the accounts you pass into the function
| // Set the tx size on the context before calling the AnteHandler | ||
| ctx = ctx.WithTxBytes(tx) |
There was a problem hiding this comment.
[question] does this need to be version gated? In other words is it consensus breaking?
There was a problem hiding this comment.
nit: this would have been easier to follow if it was done once in PrepareProposal where the context actually is created
| // Set the tx size on the context before calling the AnteHandler | ||
| ctx = ctx.WithTxBytes(tx.Tx) |
There was a problem hiding this comment.
[question] does this need to be version gated? In other words is it consensus breaking?
app/ante/max_tx_size_test.go
Outdated
| testCases := []struct { | ||
| name string | ||
| txSize int | ||
| expectError bool | ||
| appVersion uint64 | ||
| }{ |
There was a problem hiding this comment.
optional, but useful
we can add a bool to determine if this is checkTx here, then add that bool to the test sdk.Context as will with sdkCtx.WithIsCheckTx(true)
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/ante/max_tx_size.go (1)
19-33: LGTM: AnteHandle method is well-implemented with clear error messaging.The method correctly checks the app version and transaction size. The error message now includes all necessary information for users.
Consider using
errors.New()or a custom error type instead offmt.Errorf()for better error handling:return ctx, errors.New(fmt.Sprintf("tx size %d bytes is larger than the application's configured threshold of %d bytes. Please reduce the size by %d bytes", currentTxSize, maxTxBytes, bytesOverLimit))This allows for easier error checking and type assertions in the calling code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/ante/max_tx_size.go (1 hunks)
- app/ante/max_tx_size_test.go (1 hunks)
- app/process_proposal.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/ante/max_tx_size_test.go
- app/process_proposal.go
🧰 Additional context used
🔇 Additional comments (3)
app/ante/max_tx_size.go (3)
1-9: LGTM: Package declaration and imports are appropriate.The package name "ante" is suitable for an ante handler, and the imports are relevant to the implemented functionality.
11-17: LGTM: MaxTxSizeDecorator type and constructor are well-defined.The MaxTxSizeDecorator is correctly implemented as an empty struct, and its constructor follows Go conventions.
1-33: Overall implementation aligns well with PR objectives.The MaxTxSizeDecorator successfully implements the transaction size limit as described in the PR objectives. It correctly applies the limit only to app versions 3 and onwards, addressing the need to cap transaction sizes to alleviate network strain.
Key points:
- The implementation is clean and follows Go best practices.
- It provides clear error messages to guide users when their transactions exceed the size limit.
- The code addresses previous review comments, such as using the correct comparison operator.
This implementation enhances the network's resilience against excessive load and provides clear guidance for transaction batching.
| // create 1 large sendTx that includes a large memo making the | ||
| // transaction over the configured max tx bytes limit | ||
| largeSendTx := testutil.SendTxsWithAccounts( | ||
| t, testApp, enc, kr, 1000, accounts[0], accounts[1:2], testutil.ChainID, user.SetMemo(largeMemo), | ||
| ) |
There was a problem hiding this comment.
I think the memo size is capped (256 bytes IIRC) so this will fail regardless
There was a problem hiding this comment.
do you know where the memo check is? because it was failing with transaction too large error in the logs
There was a problem hiding this comment.
It's called ValidateMemoDecorator
There was a problem hiding this comment.
NewMaxTxSizeDecorator is called before so it'll be rejected for transaction being too large
| // Set the tx size on the context before calling the AnteHandler | ||
| ctx = ctx.WithTxBytes(tx) |
There was a problem hiding this comment.
nit: this would have been easier to follow if it was done once in PrepareProposal where the context actually is created
| // Set the tx bytes in the context for app version v3 and greater | ||
| if sdkCtx.BlockHeader().Version.App >= 3 { | ||
| sdkCtx = sdkCtx.WithTxBytes(tx) | ||
| } |
There was a problem hiding this comment.
same nit here: this would be easier to follow if it was placed right after sdkCtx is initialised on line 54
There was a problem hiding this comment.
it needs to be done while we're iterating over transactions in later lines no?
There was a problem hiding this comment.
so you suggest i iterate over block txs another time after context is created and set WithTxBytes there?
There was a problem hiding this comment.
ignore me. I misunderstood this
| @@ -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 MaxTxBytes. | |||
There was a problem hiding this comment.
[not blocking] we should have considered naming it something else to avoid conflicting with the mempool parameter MaxTxBytes. For example MaxTxSize would've worked
There was a problem hiding this comment.
I can change it in a follow-up since i need to update the ante handler anyway :)
| @@ -51,6 +51,10 @@ func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler | |||
| logger.Error("decoding already checked transaction", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()), "error", err) | |||
| continue | |||
| } | |||
|
|
|||
| // Set the tx size on the context before calling the AnteHandler | |||
| ctx = ctx.WithTxBytes(tx) | |||
There was a problem hiding this comment.
we're setting the tx bytes twice here, but its not super important I don't think
There was a problem hiding this comment.
I think this is due to the merge. I'll update in a follow-up
Overview
Fixes #3686