fix: reject BlobTxs larger than 2 MiB #4084
Conversation
ninabarbakadze
requested review from
staheri14 and
rach-id
and removed request for
a team
ninabarbakadze
added
the
backport:v3.x
📝 WalkthroughWalkthroughThe pull request introduces a new validation step in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (5)
app/check_tx.go (1)
19-25: Simplify the transaction size validationThe size check implementation is correct but can be improved:
- The
maxTxSizeis calculated twice- Consider using a standardized error type for consistency
// all txs should be under the max tx size limit -maxTxSize := appconsts.MaxTxSize(app.AppVersion()) currentTxSize := len(tx) -if currentTxSize > appconsts.MaxTxSize(app.AppVersion()) { - err := fmt.Errorf("tx size %d bytes is larger than the application's configured threshold of %d bytes", currentTxSize, maxTxSize) +maxTxSize := appconsts.MaxTxSize(app.AppVersion()) +if currentTxSize > maxTxSize { + err := sdkerrors.Wrapf( + blobtypes.ErrTxTooLarge, + "size %d bytes exceeds limit of %d bytes", + currentTxSize, + maxTxSize, + ) return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, []abci.Event{}, false) }app/test/big_blob_test.go (1)
70-71: Enhance test coverage with additional edge casesThe current test case verifies a single size scenario. Consider adding more test cases:
- Exactly at the limit
- Just over/under the limit
- Multiple blobs totaling over the limit
Also, consider using a constant or calculated value instead of the magic number
2_000_000.+const ( + justUnderLimit = appconsts.MaxTxSize(1) - 1000 + exactlyAtLimit = appconsts.MaxTxSize(1) + justOverLimit = appconsts.MaxTxSize(1) + 1000 +) + testCases := []testCase{ { - name: "~ 1.9 mebibyte blob", - blob: newBlobWithSize(2_000_000), + name: "exactly at limit", + blob: newBlobWithSize(exactlyAtLimit), want: blobtypes.ErrBlobsTooLarge.ABCICode(), }, + { + name: "just over limit", + blob: newBlobWithSize(justOverLimit), + want: blobtypes.ErrBlobsTooLarge.ABCICode(), + }, + { + name: "multiple blobs over limit", + blobs: []*share.Blob{ + newBlobWithSize(justUnderLimit/2), + newBlobWithSize(justUnderLimit/2), + newBlobWithSize(1000), + }, + want: blobtypes.ErrBlobsTooLarge.ABCICode(), + }, }app/validate_txs.go (1)
41-50: Consider improving error handling and slice allocation.
- Consider structuring the error message in a machine-readable format for easier log parsing
- The slice preallocation could be enabled by calculating the expected capacity
- //nolint:prealloc - var txsBelowLimit [][]byte + txsBelowLimit := make([][]byte, 0, len(txs))app/test/check_tx_test.go (1)
223-250: Consider using constants for blob size limits.The hardcoded value 2097152 (2MiB) should be defined as a constant for better maintainability and clarity.
+const ( + // MaxBlobSize represents the maximum allowed size for blobs (2MiB) + MaxBlobSize = 2 * 1024 * 1024 +) - blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152), signer.Account(accs[11]).Address()) + blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, MaxBlobSize), signer.Account(accs[11]).Address())test/util/blobfactory/payforblob_factory.go (1)
254-256: Consider providing an optional override for DefaultTxOpts.While standardizing on DefaultTxOpts() improves consistency, some test scenarios might need custom options. Consider allowing an optional override while maintaining DefaultTxOpts() as the default.
func ManyMultiBlobTx( t *testing.T, enc client.TxConfig, kr keyring.Keyring, chainid string, accounts []string, accInfos []AccountInfo, blobs [][]*share.Blob, + opts ...user.TxOption, ) [][]byte { t.Helper() txs := make([][]byte, len(accounts)) + txOpts := DefaultTxOpts() + if len(opts) > 0 { + txOpts = opts + } for i, acc := range accounts { signer, err := user.NewSigner(kr, enc, chainid, appconsts.LatestVersion, user.NewAccount(acc, accInfos[i].AccountNum, accInfos[i].Sequence)) require.NoError(t, err) - txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], DefaultTxOpts()...) + txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], txOpts...) require.NoError(t, err) } return txs }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
app/check_tx.go(1 hunks)app/test/big_blob_test.go(1 hunks)app/test/check_tx_test.go(4 hunks)app/test/consistent_apphash_test.go(3 hunks)app/test/prepare_proposal_test.go(3 hunks)app/test/process_proposal_test.go(0 hunks)app/validate_txs.go(2 hunks)test/util/blobfactory/payforblob_factory.go(1 hunks)test/util/blobfactory/test_util.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/test/process_proposal_test.go
🔇 Additional comments (9)
app/check_tx.go (1)
Line range hint 27-65: Consider extracting validation logic into helper functions
The blob transaction validation logic could be more maintainable if split into focused helper functions. However, as noted in a previous review, this refactoring should wait until the AnteHandler decision is finalized.
Consider creating these helper functions once the AnteHandler decision is made:
validateBlobTxStructure- for basic blob tx validationvalidateNormalTx- for non-blob tx validationvalidateBlobTxContent- for detailed blob validation
test/util/blobfactory/test_util.go (1)
14-14: Verify the significant gas limit increase
The gas limit has been increased by 1000x from 10_000_000 to 10_000_000_000. This could impact test execution times and might be more than necessary.
Please clarify:
- Why this specific gas limit was chosen
- If this is the minimum required for the new transaction size limits
- If this affects test execution performance
✅ Verification successful
Let's examine the actual gas limit values being used in these files to understand the context better.
Based on the analysis of the gas limit usage across the codebase, I can now provide a conclusive response.
Gas limit increase appears to be consistent with codebase patterns
The increased gas limit of 10_000_000_000 is actually in line with other high gas limit values used throughout the codebase, particularly for blob-related transactions. Here's why this change is justified:
- Many test files use similarly high gas limits (e.g., 2549760000 in prepare_proposal_test.go)
- Blob transactions specifically require higher gas limits due to their size and complexity
- The value is used in test utilities and doesn't affect production configurations
- The default minimum gas price ensures the actual cost remains reasonable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other gas limit configurations in the codebase
rg -l "SetGasLimit|GasLimit.*10.*000.*000"
Length of output: 725
Script:
#!/bin/bash
# Check the actual gas limit values being used
rg "SetGasLimit|GasLimit.*=|GasLimit.*:" -B 2 -A 2
# Also specifically look for the previous value to understand the change
rg "10_000_000_000|10000000000"
Length of output: 21426
app/validate_txs.go (1)
39-50: LGTM! Verify the MaxTxSize constant across app versions.
The transaction size validation is correctly implemented, filtering out transactions that exceed the maximum size limit.
Let's verify the MaxTxSize constant across different app versions:
✅ Verification successful
MaxTxSize constant is consistently defined and used across the codebase
The verification shows that:
- MaxTxSize is defined as 2 MiB (2097152 bytes) in pkg/appconsts/v3/app_consts.go
- It's properly versioned and only applies to app versions v3 and onwards
- The constant is consistently used in:
- Transaction validation (app/validate_txs.go)
- Ante handler checks (app/ante/max_tx_size.go)
- CheckTx validation (app/check_tx.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check MaxTxSize implementation across app versions
ast-grep --pattern 'func MaxTxSize($version $_) $_'
Length of output: 53
Script:
#!/bin/bash
# Let's try different approaches to find MaxTxSize
# 1. Search for MaxTxSize definition
rg "MaxTxSize" -A 3
# 2. Look specifically in appconsts package
fd "appconsts" --type f --exec cat {}
# 3. Search for any constant definitions related to transaction size
rg "const.*[Tt]x.*[Ss]ize" -A 3
Length of output: 12504
app/test/check_tx_test.go (1)
Line range hint 35-49: LGTM! Test setup is well structured.
The test infrastructure changes appropriately support the new test cases with proper error code handling and logging expectations.
app/test/prepare_proposal_test.go (3)
Line range hint 174-189: LGTM: Well-structured test cases for transaction size limits.
The test cases effectively verify the transaction size limit enforcement by creating transactions that exceed maxTxSize. The approach of using the app version to determine the size limit ensures future compatibility.
190-202: LGTM: Good edge case coverage with mixed-size transactions.
The test effectively verifies that the system can handle a mix of valid and oversized transactions in the same batch, ensuring selective pruning works correctly.
263-269: LGTM: Comprehensive test case for selective transaction pruning.
The test case effectively verifies that only the oversized transaction (third blob) is pruned while retaining the valid ones, ensuring precise size limit enforcement.
app/test/consistent_apphash_test.go (2)
65-67: LGTM: Good refactoring to standardize transaction options.
Centralizing the transaction options in a single function improves maintainability and consistency across tests.
384-384: LGTM: Consistent application of standardized transaction options.
The changes properly integrate the new DefaultTxOpts function across both blob and SDK message transaction creation, ensuring consistent behavior in tests.
Also applies to: 436-436
📝 WalkthroughWalkthroughThe changes in this pull request introduce a new validation step in the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/validate_txs.go (2)
41-42: Consider preallocating 'txsBelowLimit' for performance improvementPreallocating
txsBelowLimitcan enhance performance by reducing memory allocations during the append operations. Since the maximum possible length oftxsBelowLimitis known (i.e.,len(txs)), consider initializing it with a capacity:txsBelowLimit := make([][]byte, 0, len(txs))This change can optimize memory usage and improve execution speed.
44-47: Adjust logging level for oversized transactionsCurrently, transactions exceeding the size limit are logged using
logger.Error. Since oversized transactions may occur frequently and are part of normal operation, consider usinglogger.Warnto prevent log clutter and better reflect the severity.app/check_tx.go (1)
22-22: Use 'maxTxSize' variable to avoid redundant function callIn the condition check,
appconsts.MaxTxSize(app.AppVersion())is called again instead of using the already assignedmaxTxSizevariable. Replace the function call withmaxTxSizeto improve readability and efficiency.Apply this diff:
-if currentTxSize > appconsts.MaxTxSize(app.AppVersion()) { +if currentTxSize > maxTxSize {app/test/check_tx_test.go (1)
35-36: Clarify the use of 'genericErrorCode' in testsThe variable
genericErrorCodeis set to1, representing a generic error code. To enhance test clarity and specificity, consider using a defined error code that corresponds to oversized transactions, if available. This change can help accurately capture and assert the expected error conditions.test/util/blobfactory/payforblob_factory.go (1)
254-254: Consider providing both default and custom transaction options.While using
DefaultTxOpts()promotes consistency, consider allowing optional custom options to be merged with defaults for test flexibility.- txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], DefaultTxOpts()...) + opts := DefaultTxOpts() + if customOpts != nil { + opts = append(opts, customOpts...) + } + txs[i], _, err = signer.CreatePayForBlobs(acc, blobs[i], opts...)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
app/check_tx.go(1 hunks)app/test/big_blob_test.go(1 hunks)app/test/check_tx_test.go(4 hunks)app/test/consistent_apphash_test.go(3 hunks)app/test/prepare_proposal_test.go(3 hunks)app/test/process_proposal_test.go(0 hunks)app/validate_txs.go(2 hunks)test/util/blobfactory/payforblob_factory.go(1 hunks)test/util/blobfactory/test_util.go(1 hunks)
💤 Files with no reviewable changes (1)
- app/test/process_proposal_test.go
🔇 Additional comments (5)
test/util/blobfactory/test_util.go (1)
14-14: Verify the necessity of increasing gas limit to 10,000,000,000
The gas limit in DefaultTxOpts has been significantly increased from 10,000,000 to 10,000,000,000. Please confirm that this high gas limit is required for the test scenarios. An excessively high gas limit may lead to inflated fees and could mask issues related to gas consumption in tests.
app/test/check_tx_test.go (1)
223-250: Ensure specific error codes for oversized blob transactions
In the test cases "v1 blob over 2MiB" and "v0 blob over 2MiB", the expectedABCICode is set to genericErrorCode. Verify if a specific error code exists for transactions exceeding the maximum size (e.g., blobtypes.ErrTxTooLarge.ABCICode()), and use it to improve test accuracy and error handling consistency.
app/test/prepare_proposal_test.go (2)
Line range hint 174-189: LGTM! Good test coverage for oversized transactions.
The test properly uses the app's maximum transaction size to verify that transactions exceeding this limit are filtered out.
190-201: LGTM! Well-structured test for mixed transaction sizes.
The test case effectively verifies that only oversized transactions are filtered while valid ones are retained, which is crucial for partial filtering behavior.
Also applies to: 263-269
app/test/consistent_apphash_test.go (1)
65-67: LGTM! Good centralization of transaction options.
The introduction and consistent usage of DefaultTxOpts() across different test scenarios promotes maintainability and reduces duplication.
Also applies to: 384-384, 436-436
There was a problem hiding this comment.
I think we're okay here but we need to double check a test to make sure. We can't be adding any new rejection cases to processproposal
I'm okay with adding this block in a non-breaking way like this now on the condition that we fix this is a more maintainble way in the near future. meaning, if this is a tx validity rule, then we should use the tools to add tx validity rules (antehandlers). this could involve either #4088 and/or #1166 celestiaorg/cosmos-sdk#417
app/validate_txs.go
Outdated
| maxTxSize := appconsts.MaxTxSize(ctx.BlockHeader().Version.App) | ||
| //nolint:prealloc | ||
| var txsBelowLimit [][]byte | ||
| for idx, tx := range txs { | ||
| if len(tx) > maxTxSize { | ||
| err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize) | ||
| logger.Error(err) | ||
| continue | ||
| } | ||
| txsBelowLimit = append(txsBelowLimit, tx) | ||
| } |
There was a problem hiding this comment.
why not check sizes of PFBs since that's what we care about? This would also allow us to keep logic in antehandlers instead of making a special case and having to remember that we have this logic hidden in this function.
side note: in theory, a consensus breaking version would be handled in the ValidateBasic method since this is a stateless check on PFBs only. We can't do that now without also passing the app version to all validate basic methods, but we need to that eventually anyway ref #4088
There was a problem hiding this comment.
@cmwaters suggested that we should reject encoded transactions directly if the size is over 2MiB. This makes sense to me since we care about transactions not being over limit not only the blobs right? because when this tx is getting gossiped it'd be encoded right?
I'm not sure we can change the AnteHandler in non breaking way. I see it as a temporary fix in place until we can clean this up and break things in v4.
There was a problem hiding this comment.
We could have the check in the antehandler, we would just need to have specific logic for the case of a PFB where we count the the blob sizes as well as the the tx bytes in the context.
This would also allow us to keep logic in antehandlers instead of making a special case and having to remember that we have this logic hidden in this function.
Antehandlers aren't the only source of validation, we do a lot of blob specific validity checks outside the antehandlers
Having it prior to being unmarshalled is the simplest and most likely safest way to implement a max tx size limit. There is no custom logic based on specific messages
There was a problem hiding this comment.
I'm good with putting the logic in umarshaling or an antehandler 👍 I still don't like the idea of not being able to version validate basic logic.
I think its already a block validity that everything must be decodable. do we pass the app version to the encoding and decoding logic?
There was a problem hiding this comment.
My view of validate basic was that it's a stateless (i.e. the function has no arguments). The app version is part of state. Having the app version being an argument to ValidateBasic seems incorrect (to me). The antehandler seems the right place to put version based checks in
I think its already a block validity that everything must be decodable. do we pass the app version to the encoding and decoding logic?
No, the codec currently contains all types, thus types across all versions can be marshalled or unmarshalled.
| { | ||
| name: "10,000,000 byte blob", | ||
| name: "2,000,000 byte blob", | ||
| checkType: abci.CheckTxType_New, | ||
| getTx: func() []byte { | ||
| signer := createSigner(t, kr, accs[9], encCfg.TxConfig, 10) | ||
| _, blobs := blobfactory.RandMsgPayForBlobsWithSigner(tmrand.NewRand(), signer.Account(accs[9]).Address().String(), 10_000_000, 1) | ||
| _, blobs := blobfactory.RandMsgPayForBlobsWithSigner(tmrand.NewRand(), signer.Account(accs[9]).Address().String(), 2_000_000, 1) |
There was a problem hiding this comment.
[question] why does this test case have to change?
Do we need to make the checkTx invocation versioned?
There was a problem hiding this comment.
CheckTx and PrepareProposal doesn't need version gating. please check me on this @evan-forbes
that needs to change because we want to hit ErrBlobsTooLarge ref #4084 (comment)
There was a problem hiding this comment.
we should be good not to version in this case yeah
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/check_tx.go (2)
20-25: LGTM! Consider enhancing error details.The size validation implementation is clean and efficient, occurring before any expensive decoding operations. The error handling is good but could be enhanced with a custom error type for better programmatic handling.
Consider creating a custom error type:
// Define in pkg/errors/errors.go type TxSizeExceededError struct { Current uint64 Max uint64 } func (e *TxSizeExceededError) Error() string { return fmt.Sprintf("tx size %d bytes is larger than the application's configured threshold of %d bytes", e.Current, e.Max) }
Line range hint
26-67: Consider refactoring validation flow for better maintainability.The validation flow is logically structured but could benefit from some refactoring:
- The validation steps are well-ordered (size → blob check → specific validations).
- Error handling is consistent throughout.
- The code successfully implements the PR objective of validating size before decoding.
Consider:
- Extracting the validation logic into separate methods for better maintainability:
validateTxSizevalidateBlobTxvalidateRegularTx- Creating a validation pipeline that can be easily modified when the AnteHandler is removed in v4.
Example structure:
type TxValidator interface { Validate(tx []byte) error } type SizeValidator struct { maxSize uint64 } type BlobValidator struct { txConfig client.TxConfig // ... other dependencies } // Then chain them in CheckTx
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/check_tx.go(1 hunks)
🔇 Additional comments (2)
app/check_tx.go (2)
19-19: Improve comment accuracy for size limit condition.
The current comment implies transactions equal to max size are rejected, which isn't the case.
- // all txs must be less than or equal to the max tx size limit
+ // all txs must be less than the max tx size limitLine range hint 1-67: Verify test coverage for size validation scenarios.
The implementation successfully adds size validation before decoding, but we should ensure comprehensive test coverage.
✅ Verification successful
Let me check the test coverage more thoroughly by examining the actual test implementations.
Let me check the actual test cases in check_tx_test.go to verify if size validation is properly tested.
Size validation test coverage is comprehensive
The test coverage for size validation is well implemented with multiple test cases:
- Direct size validation tests for transactions over 2MiB limit (both v0 and v1 blobs)
- Progressive size tests from 1KB to 2MB to verify behavior at different thresholds
- Tests include proper error messages and error codes for oversized transactions
- Both normal and blob transactions are covered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for size validation scenarios
# Look for size-related test cases
echo "Checking for size validation tests..."
rg -l "MaxTxSize" --type go --glob "*_test.go"
# Look for specific test scenarios
echo "Checking for specific test scenarios..."
rg "TestCheckTx|TestPrepareProposal" --type go -A 10 --glob "*_test.go"
Length of output: 5227
Script:
#!/bin/bash
# Check the implementation of size validation tests
echo "Checking size validation test implementations..."
rg "MaxTxSize" -B 5 -A 10 app/ante/max_tx_size_test.go
echo "\nChecking CheckTx test implementations..."
rg "TestCheckTx" -B 5 -A 20 app/test/check_tx_test.go
# Look for specific size-related test cases
echo "\nChecking for size-related test scenarios in check_tx_test.go..."
rg "currentTxSize|maxTxSize" app/test/check_tx_test.go
Length of output: 2674
Script:
#!/bin/bash
# Get the full content of check_tx_test.go to verify test cases
echo "Checking complete test implementation in check_tx_test.go..."
cat app/test/check_tx_test.go
# Also check if there are any other relevant test files
echo "\nChecking for other relevant test files..."
fd -t f "check.*test\.go$"
Length of output: 9860
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/errors/errors.go (1)
7-12: LGTM! Consider adding error code documentation.The error definition is well-structured and follows cosmos-sdk conventions. Consider adding a comment explaining the error code selection (11142) for future maintainability.
app/check_tx.go (1)
26-26: Consider defining gas constants.The gas values (0, 0) in
ResponseCheckTxWithEventscould be defined as named constants for better maintainability.+ const ( + DefaultGasWanted = 0 + DefaultGasUsed = 0 + )app/test/big_blob_test.go (2)
71-72: Consider using constants for blob sizes.The blob size (2_000_000) could be defined as a named constant for better maintainability and clarity.
+ const ( + LargeBlobSize = 2_000_000 // ~1.9 MiB + )
93-109: LGTM! Consider using constants for magic numbers.The test case is well structured and verifies both error code and message. Consider defining the blob size (2097152) as a named constant, possibly using
1 << 21for clarity that it's 2 MiB.+ const ( + MaxBlobSize = 1 << 21 // 2 MiB + )app/test/check_tx_test.go (2)
36-36: Consider adding documentation for the test struct fieldsThe addition of
expectedLogfield and expansion ofaccsslice are appropriate. Consider adding documentation comments for the test struct fields to improve maintainability.type test struct { + // name is the descriptive name of the test case name string + // checkType specifies whether this is a new transaction check or a recheck checkType abci.CheckTxType + // getTx returns the transaction bytes to be tested getTx func() []byte + // expectedABCICode is the expected response code from CheckTx expectedABCICode uint32 + // expectedLog is the expected error message in the response expectedLog string }Also applies to: 48-48
260-260: Consider using exact matching for error messagesThe current implementation uses
Containswhich might pass even with partial matches. For stricter validation, consider using exact matching.-assert.Contains(t, resp.Log, tt.expectedLog) +assert.Equal(t, tt.expectedLog, resp.Log)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/check_tx.go(2 hunks)app/errors/errors.go(1 hunks)app/test/big_blob_test.go(4 hunks)app/test/check_tx_test.go(8 hunks)
🔇 Additional comments (3)
app/check_tx.go (1)
22-27: LGTM! Efficient placement of size check.
The size check is correctly placed before any expensive unmarshaling operations, which is efficient. The error message is informative, including both the actual and maximum sizes.
app/test/check_tx_test.go (2)
14-14: LGTM: Import addition is appropriate
The addition of the app errors import is necessary for the new transaction size limit error handling.
180-191: Verify consistency of max transaction size constant
The test case now uses 2MB (2,000,000 bytes) as the size limit. Let's verify this value is consistent across the codebase.
✅ Verification successful
Transaction size limit of 2MB is consistently defined and enforced across the codebase
The verification shows that:
- The max transaction size is consistently defined as 2MB (2,097,152 bytes) in
pkg/appconsts/v3/app_consts.go - This limit is correctly enforced through:
MaxTxSizeDecoratorin the ante handler- Test cases across multiple files using the same 2MB limit
- Error handling for blobs and transactions exceeding this size
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for size limit constants and their usages
rg -A 2 "2.*000.*000|2.*MiB|MaxTxSize" --type go
Length of output: 14534
app/test/check_tx_test.go
Outdated
| { | ||
| name: "v1 blob over 2MiB", | ||
| checkType: abci.CheckTxType_New, | ||
| getTx: func() []byte { | ||
| signer := createSigner(t, kr, accs[11], encCfg.TxConfig, 12) | ||
| blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152), signer.Account(accs[11]).Address()) | ||
| require.NoError(t, err) | ||
| blobTx, _, err := signer.CreatePayForBlobs(accs[11], []*share.Blob{blob}, opts...) | ||
| require.NoError(t, err) | ||
| return blobTx | ||
| }, | ||
| expectedLog: apperr.ErrTxExceedsMaxSize.Error(), | ||
| expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(), | ||
| }, | ||
| { | ||
| name: "v0 blob over 2MiB", | ||
| checkType: abci.CheckTxType_New, | ||
| getTx: func() []byte { | ||
| signer := createSigner(t, kr, accs[12], encCfg.TxConfig, 13) | ||
| blob, err := share.NewV0Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152)) | ||
| require.NoError(t, err) | ||
| blobTx, _, err := signer.CreatePayForBlobs(accs[12], []*share.Blob{blob}, opts...) | ||
| require.NoError(t, err) | ||
| return blobTx | ||
| }, | ||
| expectedLog: apperr.ErrTxExceedsMaxSize.Error(), | ||
| expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize the blob size constants
There's an inconsistency in how the 2MB limit is specified:
- Modified test uses 2,000,000 bytes
- New tests use 2097152 bytes (2MiB)
Consider:
- Using a constant for the max size
- Standardizing between decimal (2,000,000) and binary (2097152) representations
+// Define at package level
+const (
+ // MaxBlobSize is 2MiB in bytes
+ MaxBlobSize = 2097152
+)
// Use in test cases
-bytes.Repeat([]byte{1}, 2097152)
+bytes.Repeat([]byte{1}, MaxBlobSize)Committable suggestion skipped: line range outside the PR's diff.
app/validate_txs.go
Outdated
| // all transactions should be below the max tx size | ||
| maxTxSize := appconsts.MaxTxSize(ctx.BlockHeader().Version.App) | ||
| //nolint:prealloc | ||
| var txsBelowLimit [][]byte | ||
| for idx, tx := range txs { | ||
| if len(tx) > maxTxSize { | ||
| err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize) | ||
| logger.Error(err) | ||
| continue | ||
| } | ||
| txsBelowLimit = append(txsBelowLimit, tx) | ||
| } |
There was a problem hiding this comment.
if we checking this in checkTx, we don't need to add it here too, correct?
There was a problem hiding this comment.
Yes I think so. In theory we should be able to remove this and rely just on CheckTx. There might be an edge cases when we change version
app/test/check_tx_test.go
Outdated
| @@ -44,6 +45,7 @@ func TestCheckTx(t *testing.T) { | |||
| checkType abci.CheckTxType | |||
| getTx func() []byte | |||
| expectedABCICode uint32 | |||
| expectedLog string | |||
There was a problem hiding this comment.
nit: Does the expected log really help if we now have error codes we can assert
There was a problem hiding this comment.
i guess it's redundant in check tx
There was a problem hiding this comment.
I'm a little lost on the rationale for this change. Is there a Github issue or document with more context on the bug that this PR is fixing?
I re-read the Slack thread b/c there isn't a GitHub issue for the bug. IMO the Slack thread should be linked in PR description to help reviewers get more context. Better yet a GitHub issue would've really helped. Here's the bug as I understand it:
Context
https://github.com/celestiaorg/CIPs/blob/main/cips/cip-28.md
Problem
The 2 MiB tx limit proposed in CIP-28 doesn't work as expected. Currently it only applies to non-blob txs. Blob txs can be larger than 2 MiB.
Investigation
Our CheckTx wraps the Cosmos SDK base app CheckTx. Both blob txs and non-blob txs have the base app CheckTx invoked on them. BaseApp CheckTx invokes BaseApp runTx which invokes app.anteHandler. The app.anteHandler includes the MaxTxSizeDecorator. Our PrepareProposal constructs an ante handler that includes the MaxTxSizeDecorator. Our ProcessProposal also constructs an ante handler that includes the MaxTxSizeDecorator.
The problem with the current implementation of MaxTxSizeDecorator is that it gets the inner tx and not the blobs attached because of these lines in CheckTx and this line in PrepareProposal. The fix in this PR is to take the implementation of the MaxTxSizeDecorator and move it into CheckTx and PrepareProposal before the blobs are split from the blob tx. This PR doesn't do the same in ProcessProposal because that would be consensus breaking.
Questions
- The
MaxTxSizeDecoratorunit tests don't include a BlobTx so we didn't catch this at development time. Should we add a unit test for BlobTx if we are keeping MaxTxSizeDecorator around? If not we should deprecate MaxTxSizeDecorator because it doesn't work as expected. Can be a follow-up. - Do we need to copy + paste the logic into
PrepareProposal, specificallyFilterTxs? Given this now won't be a consensus rule,CheckTxseems sufficient because validators that are running unmodified code will reject BlobTxs larger than 2 MiB from entering their mempool and therefore won't be included in a block. IMO we can remove it fromFilterTxsbefore merging but not blocking on it. - Can we rename the PR title to something a bit more informative? Perhaps: "fix: reject BlobTxs larger than 2 MiB"
app/validate_txs.go
Outdated
| var txsBelowLimit [][]byte | ||
| for idx, tx := range txs { | ||
| if len(tx) > maxTxSize { | ||
| err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize) |
There was a problem hiding this comment.
[uber nit] in some places we call the max tx size a "limit". In other places we call it a "threshold". It would be nice if we always referred to this with the same vocabulary.
In my opinion it would be sufficient to call it MaxTxSize and not use "limit" or "threshold" because the prefix "max" already conveys that.
ninabarbakadze
changed the title
ninabarbakadze
dismissed stale reviews from rootulp and cmwaters
via
0e99e30
Co-authored-by: Rootul P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/validate_txs.go(2 hunks)
🔇 Additional comments (3)
app/validate_txs.go (3)
4-5: LGTM: Import addition is appropriate
The fmt import is correctly added for error message formatting.
45-45: Use consistent terminology for size limits
As noted in previous reviews, we should maintain consistent terminology. The error message uses "threshold" while the codebase generally refers to this as "MaxTxSize". Consider removing the word "threshold" from the error message.
52-52: Verify impact on transaction processing
The new size validation occurs before transaction separation. Let's verify this doesn't affect any edge cases.
✅ Verification successful
Transaction size validation is properly handled and consistent
The verification shows that the transaction size validation is implemented consistently across multiple layers:
- In
app/validate_txs.go, size validation occurs before transaction separation, filtering out oversized transactions early - The same validation is also enforced in:
app/check_tx.goduring transaction checksapp/ante/max_tx_size.goas part of the ante handler chain
- The
MaxTxSizeconstant is version-controlled (2 MiB for v3) and consistently used across all validation points - Test coverage confirms proper handling of edge cases, including mixed transaction sizes and oversized blob transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any special handling of transaction sizes in tests or related code
# Look for test cases involving transaction sizes
rg -A 5 "MaxTxSize|tx size|transaction size" --type go
# Check for any size-related constants or configurations
ast-grep --pattern 'const MaxTxSize = $_'
Length of output: 11351
app/validate_txs.go
Outdated
| // all transactions should be less than or equal to the max tx size | ||
| maxTxSize := appconsts.MaxTxSize(ctx.BlockHeader().Version.App) | ||
| //nolint:prealloc | ||
| var txsBelowLimit [][]byte | ||
| for idx, tx := range txs { | ||
| if len(tx) > maxTxSize { | ||
| err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize) | ||
| logger.Error(err) | ||
| continue | ||
| } | ||
| txsBelowLimit = append(txsBelowLimit, tx) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed for the transaction size validation
- Error handling should be improved:
- Use structured logging instead of string formatting
- Consider using the standard error type from app/errors package
- Performance can be optimized by preallocating the slice
- Consider extracting the size validation into a separate function for better testability
Here's the suggested implementation:
+ // validateTxSize checks if the transaction size is within the allowed limit
+ func validateTxSize(tx []byte, idx int, maxSize int) error {
+ if len(tx) > maxSize {
+ return ErrTxExceedsMaxSize.Wrapf(
+ "size %d bytes at index %d exceeds max size of %d bytes",
+ len(tx), idx, maxSize,
+ )
+ }
+ return nil
+ }
func FilterTxs(logger log.Logger, ctx sdk.Context, handler sdk.AnteHandler, txConfig client.TxConfig, txs [][]byte) [][]byte {
// all transactions should be less than or equal to the max tx size
maxTxSize := appconsts.MaxTxSize(ctx.BlockHeader().Version.App)
- //nolint:prealloc
- var txsBelowLimit [][]byte
+ txsBelowLimit := make([][]byte, 0, len(txs))
for idx, tx := range txs {
- if len(tx) > maxTxSize {
- err := fmt.Sprintf("tx size %d bytes at index %d exceeds the application's configured threshold of %d bytes", len(tx), idx, maxTxSize)
- logger.Error(err)
+ if err := validateTxSize(tx, idx, maxTxSize); err != nil {
+ logger.Error("transaction exceeds max size",
+ "error", err,
+ "tx_index", idx,
+ "tx_size", len(tx),
+ "max_size", maxTxSize,
+ )
continue
}
txsBelowLimit = append(txsBelowLimit, tx)
}Committable suggestion skipped: line range outside the PR's diff.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/check_tx.go (2)
22-27: LGTM! Clear and efficient size validation.The implementation efficiently validates transaction size before any expensive decoding operations. The error message is comprehensive, including all necessary details for debugging.
Consider rephrasing the error message for clarity:
- return sdkerrors.ResponseCheckTxWithEvents(errors.Wrapf(apperr.ErrTxExceedsMaxSize, "tx size %d bytes is larger than the application's configured MaxTxSize of %d bytes for version %d", currentTxSize, maxTxSize, app.AppVersion()), 0, 0, []abci.Event{}, false) + return sdkerrors.ResponseCheckTxWithEvents(errors.Wrapf(apperr.ErrTxExceedsMaxSize, "tx size of %d bytes exceeds the maximum allowed size of %d bytes for version %d", currentTxSize, maxTxSize, app.AppVersion()), 0, 0, []abci.Event{}, false)
Line range hint
30-45: Consider wrapping errors for better debugging context.While the error handling is correct, adding context to errors would improve debugging experience.
btx, isBlob, err := blobtx.UnmarshalBlobTx(tx) if isBlob && err != nil { - return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, []abci.Event{}, false) + return sdkerrors.ResponseCheckTxWithEvents(errors.Wrap(err, "failed to unmarshal blob transaction"), 0, 0, []abci.Event{}, false) } if !isBlob { // reject transactions that can't be decoded sdkTx, err := app.txConfig.TxDecoder()(tx) if err != nil { - return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, []abci.Event{}, false) + return sdkerrors.ResponseCheckTxWithEvents(errors.Wrap(err, "failed to decode transaction"), 0, 0, []abci.Event{}, false) }app/test/check_tx_test.go (2)
Line range hint
177-186: Fix inconsistent error code in test case.The test case expects
blobtypes.ErrBlobsTooLargebut the implementation returnsapperr.ErrTxExceedsMaxSizefor oversized transactions.- expectedABCICode: blobtypes.ErrBlobsTooLarge.ABCICode(), + expectedABCICode: apperr.ErrTxExceedsMaxSize.ABCICode(),
221-246: Standardize size notation across test cases.While the test cases are comprehensive, they use binary notation (2097152) while other tests use decimal notation (2_000_000). Consider standardizing for consistency.
- blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152), signer.Account(accs[11]).Address()) + blob, err := share.NewV1Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2_097_152), signer.Account(accs[11]).Address()) - blob, err := share.NewV0Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2097152)) + blob, err := share.NewV0Blob(share.RandomBlobNamespace(), bytes.Repeat([]byte{1}, 2_097_152))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/check_tx.go(2 hunks)app/test/check_tx_test.go(4 hunks)app/test/prepare_proposal_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/test/prepare_proposal_test.go
🔇 Additional comments (1)
app/check_tx.go (1)
Line range hint 47-67: LGTM! Robust handling of different CheckTx types.
The implementation correctly handles different CheckTx types and includes appropriate validation for new transactions.
## Overview Directly checks transaction sizes even before they're decoded and removes them if they exceed configured threshold. We should add MaxTxSize constraint in ProcessProposal directly and consider removing the AnteHandler in v4. Issue tracking this work: #4087 ## Testing - Check tx test asserts with logs that the expected error gets hit - Getting logs from prepare proposal was more challenging so i'm inserting a screenshot of application logs when the tests are run. <img width="887" alt="Screenshot 2024-12-06 at 12 27 03" src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27"> <img width="837" alt="Screenshot 2024-12-06 at 12 27 20" src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455"> ## Proposal for improving robustness of our test suites - [ ] Open an issue to assert all logs in our integration tests. --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit 3751aac) # Conflicts: # .github/CODEOWNERS # .github/auto_request_review.yml # .github/mergify.yml # .github/workflows/lint.yml # .github/workflows/pr-review-requester.yml # .github/workflows/test.yml # Makefile # app/app.go # app/benchmarks/README.md # app/benchmarks/results.md # app/check_tx.go # app/square_size.go # app/test/big_blob_test.go # app/test/check_tx_test.go # app/test/consistent_apphash_test.go # app/test/prepare_proposal_test.go # app/test/process_proposal_test.go # app/test/upgrade_test.go # cmd/celestia-appd/cmd/start.go # docs/architecture/adr-011-optimistic-blob-size-independent-inclusion-proofs-and-pfb-fraud-proofs.md # docs/architecture/adr-015-namespace-id-size.md # docs/architecture/adr-021-restricted-block-size.md # docs/maintainers/prebuilt-binaries.md # docs/maintainers/release-guide.md # docs/release-notes/release-notes.md # go.mod # go.sum # pkg/appconsts/chain_ids.go # pkg/appconsts/overrides.go # pkg/appconsts/v1/app_consts.go # pkg/appconsts/v2/app_consts.go # pkg/appconsts/v3/app_consts.go # pkg/appconsts/versioned_consts.go # pkg/appconsts/versioned_consts_test.go # scripts/single-node.sh # specs/src/cat_pool.md # specs/src/data_square_layout.md # specs/src/parameters_v1.md # specs/src/parameters_v2.md # specs/src/parameters_v3.md # test/e2e/benchmark/benchmark.go # test/e2e/benchmark/throughput.go # test/e2e/major_upgrade_v2.go # test/e2e/major_upgrade_v3.go # test/e2e/minor_version_compatibility.go # test/e2e/readme.md # test/e2e/simple.go # test/e2e/testnet/node.go # test/e2e/testnet/setup.go # test/e2e/testnet/testnet.go # test/e2e/testnet/txsimNode.go # test/e2e/testnet/util.go # test/interchain/go.mod # test/interchain/go.sum # test/util/blobfactory/payforblob_factory.go # test/util/blobfactory/test_util.go # test/util/testnode/comet_node_test.go # test/util/testnode/config.go # x/blob/ante/blob_share_decorator.go # x/blob/ante/max_total_blob_size_ante.go # x/tokenfilter/README.md
## Overview Directly checks transaction sizes even before they're decoded and removes them if they exceed configured threshold. We should add MaxTxSize constraint in ProcessProposal directly and consider removing the AnteHandler in v4. Issue tracking this work: #4087 ## Testing - Check tx test asserts with logs that the expected error gets hit - Getting logs from prepare proposal was more challenging so i'm inserting a screenshot of application logs when the tests are run. <img width="887" alt="Screenshot 2024-12-06 at 12 27 03" src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27"> <img width="837" alt="Screenshot 2024-12-06 at 12 27 20" src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455"> ## Proposal for improving robustness of our test suites - [ ] Open an issue to assert all logs in our integration tests. <hr>This is an automatic backport of pull request #4084 done by [Mergify](https://mergify.com). Co-authored-by: Nina Barbakadze <[email protected]>
Overview
Directly checks transaction sizes even before they're decoded and removes them if they exceed configured threshold.
We should add MaxTxSize constraint in ProcessProposal directly and consider removing the AnteHandler in v4.
Issue tracking this work: #4087
Testing
Proposal for improving robustness of our test suites