chore: optimize checkTx #3954
chore: optimize checkTx #3954
Conversation
evan-forbes
requested review from
cmwaters and
staheri14
and removed request for
a team
evan-forbes
requested review from
adlerjohn,
rootulp and
ninabarbakadze
and removed request for
staheri14
| d.Txs = append(d.Txs, tooManyShareBtx) | ||
| }, | ||
| appVersion: v3.Version, | ||
| expectedResult: abci.ResponseProcessProposal_REJECT, |
There was a problem hiding this comment.
this reject isn't new, but it should still be covered by tests
| @@ -45,6 +45,7 @@ func FilterTxs(logger log.Logger, ctx sdk.Context, handler sdk.AnteHandler, txCo | |||
| func filterStdTxs(logger log.Logger, dec sdk.TxDecoder, ctx sdk.Context, handler sdk.AnteHandler, txs [][]byte) ([][]byte, sdk.Context) { | |||
| n := 0 | |||
| for _, tx := range txs { | |||
| ctx = ctx.WithTxBytes(tx) | |||
There was a problem hiding this comment.
this needs to be added for future things anyway, although it has no impact atm
| txBuilder := txConfig.NewTxBuilder() | ||
| require.NoError(t, txBuilder.SetMsgs(tc.pfb)) | ||
| tx := txBuilder.GetTx() | ||
| kr, _ := testnode.NewKeyring(testfactory.TestAccName) | ||
| signer, err := user.NewSigner( | ||
| kr, | ||
| ecfg.TxConfig, | ||
| testfactory.ChainID, | ||
| tc.appVersion, | ||
| user.NewAccount(testfactory.TestAccName, 1, 0), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| blobTx := blobfactory.RandBlobTxs(signer, rand, 1, tc.blobsPerPFB, tc.blobSize) | ||
|
|
||
| btx, isBlob, err := blobtx.UnmarshalBlobTx([]byte(blobTx[0])) | ||
| require.NoError(t, err) | ||
| require.True(t, isBlob) |
There was a problem hiding this comment.
made this a much more meaningful test by using actual signed txs instead of mocking a pfb size
evan-forbes
added
WS: Maintenance 🔧
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to transaction context handling within the Changes
Possibly related PRs
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 (5)
app/validate_txs.go (1)
Line range hint
1-134: Summary: Optimization through context enrichmentThe changes in this file focus on enriching the context with transaction bytes before processing in both
filterStdTxsandfilterBlobTxsfunctions. This optimization aligns with the PR objective of improvingcheckTxperformance.While the modifications are minimal, they could potentially enhance logging, error handling, and overall transaction processing by providing immediate access to raw transaction data within the context.
Consider adding a comment explaining the purpose of these changes to improve code readability and maintainability.
x/blob/ante/blob_share_decorator_test.go (2)
43-132: Comprehensive test cases covering various scenarios and edge cases.The updated test cases provide excellent coverage for different blob sizes, numbers of blobs, and app versions. The inclusion of edge cases, such as blobs that exactly fill or slightly exceed the square capacity, is particularly valuable for ensuring robust behavior.
The explanatory comments for certain test cases are helpful in understanding the expected behavior.
Consider adding a test case for a blob size that is just below the maximum allowed size to ensure there are no off-by-one errors in the size calculation.
136-164: Improved test setup with realistic transaction generation and processing.The updated test setup provides a more authentic testing environment by using actual signers and generated blob transactions. The use of
blobfactory.RandBlobTxsallows for easy creation of test data with specified characteristics, enhancing the flexibility of the tests.The process of unmarshalling and decoding the blob transaction ensures that the decorator is tested with the correct input format, closely mimicking real-world usage.
Consider extracting the common setup code (keyring creation, signer setup) into a separate helper function to reduce duplication and improve readability.
app/test/process_proposal_test.go (2)
84-96: LGTM: New helper variable for oversized blob transactionThe new
tooManyShareBtxvariable is well-implemented and serves its purpose of creating a blob transaction that exceeds the allowed number of shares. It uses existing test utilities and follows the pattern of other test data setup.Consider parameterizing the number of blobs (currently set to 4000) to make the test more flexible and easier to adjust in the future. For example:
- testfactory.RandomBlobNamespaces(tmrand.NewRand(), 4000), - [][]int{repeat(4000, 1)}, + testfactory.RandomBlobNamespaces(tmrand.NewRand(), maxBlobsPerTx), + [][]int{repeat(maxBlobsPerTx, 1)},Where
maxBlobsPerTxcould be a constant defined at the package level or a parameter passed to the test function.
317-329: LGTM: New test case for oversized blob transactionThe new test case "blob tx that takes up too many shares" is well-structured and correctly tests the scenario where a transaction exceeds the allowed number of shares. It appropriately uses the
v3.Versionand expects the proposal to be rejected.Consider adding a brief comment explaining the significance of this test case for v3. For example:
{ + // This test case ensures that transactions exceeding the maximum allowed shares + // are rejected in v3, which introduced or modified this limit. name: "blob tx that takes up too many shares", input: &tmproto.Data{ Txs: [][]byte{}, }, mutator: func(d *tmproto.Data) { // this tx will get filtered out by prepare proposal before this // so we add it here d.Txs = append(d.Txs, tooManyShareBtx) }, appVersion: v3.Version, expectedResult: abci.ResponseProcessProposal_REJECT, },This comment would help future developers understand the context and importance of this test case.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- app/process_proposal.go (2 hunks)
- app/test/prepare_proposal_test.go (3 hunks)
- app/test/process_proposal_test.go (3 hunks)
- app/validate_txs.go (2 hunks)
- x/blob/ante/blob_share_decorator.go (2 hunks)
- x/blob/ante/blob_share_decorator_test.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
app/process_proposal.go
[failure] 98-98:
ineffectual assignment to ctx (ineffassign)x/blob/ante/blob_share_decorator_test.go
[failure] 175-175:
funcrepeatis unused (unused)
🔇 Additional comments (15)
app/validate_txs.go (2)
48-48: LGTM: Context enrichment with transaction bytesThe addition of
ctx = ctx.WithTxBytes(tx)is a good optimization. It enriches the context with the transaction bytes before decoding, which can be beneficial for logging, error handling, or other operations that might need access to the raw transaction data during processing.
82-82: LGTM: Consistent context enrichment for blob transactionsThe addition of
ctx = ctx.WithTxBytes(tx.Tx)for blob transactions is consistent with the change made infilterStdTxs. This ensures that the context is uniformly enriched with transaction bytes across different transaction types, maintaining a consistent approach to transaction processing.x/blob/ante/blob_share_decorator_test.go (4)
10-13: LGTM: Import statements updated appropriately.The new imports are correctly added to support the changes in the test implementation. They are all being used in the updated test cases and setup.
Also applies to: 17-17, 21-23
33-36: Improved test case structure for better granularity and version testing.The updated test case structure with
blobsPerPFBandblobSizeprovides more precise control over blob characteristics in test scenarios. The addition ofappVersionenables testing across different app versions, which is crucial for ensuring compatibility and correct behavior across versions.
Line range hint
1-181: LGTM: Code style and consistency.The code adheres to Go style guidelines, uses descriptive and consistent variable names, and is well-formatted. This contributes to the overall readability and maintainability of the test suite.
Line range hint
175-181: Address unusedrepeatfunction.The newly added
repeatfunction is well-implemented and leverages Go's generics feature. However, the static analysis tool indicates that this function is currently unused in the file.Consider one of the following actions:
- If the function is intended for future use or is used in other files, add a comment explaining its purpose and intended usage to suppress the linter warning.
- If the function is not needed, remove it to keep the codebase clean.
- If the function should be used in the current tests, update the test cases to utilize it where appropriate.
To verify the usage of the
repeatfunction across the codebase, you can run the following command:This will help determine if the function is used elsewhere in the project.
✅ Verification successful
Verified:
repeatfunction is utilized in multiple test files.The
repeatfunction is actively used in bothblob_share_decorator_test.goandprepare_proposal_test.go. The static analysis tool's indication of it being unused withinblob_share_decorator_test.goappears to be a false positive.
- Ensure that the purpose of the
repeatfunction is documented to clarify its usage within the test suite.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the repeat function across the codebase rg --type go "repeat\[.+\]" .Length of output: 188
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 175-175:
funcrepeatis unused (unused)app/process_proposal.go (3)
70-71: Improved context handling for individual transactionsThe introduction of a new
ctxvariable with transaction bytes for each transaction is a good practice. It ensures that each transaction is processed with its own context, which can improve isolation and potentially help with debugging.
Line range hint
70-98: Verify the impact of context handling changesThe changes introduced to optimize the
checkTxfunction, particularly the new context handling with thectxvariable, are consistent with the PR objectives. These modifications should improve the processing of individual transactions by ensuring each one has its own context.However, it's crucial to verify that these changes don't inadvertently alter the overall behavior of the
ProcessProposalmethod.To ensure the changes don't introduce any regressions, please run the following tests:
- Unit tests for the
ProcessProposalmethod:#!/bin/bash # Run unit tests for ProcessProposal go test -v -run TestProcessProposal ./app
- Integration tests that involve transaction processing:
#!/bin/bash # Run integration tests related to transaction processing go test -v -run TestTxProcessing ./integration
- Benchmark tests to verify performance improvements:
#!/bin/bash # Run benchmark tests for ProcessProposal go test -v -run=^$ -bench=BenchmarkProcessProposal ./appPlease review the results of these tests to ensure that the functionality remains correct and that there are indeed performance improvements as intended by this optimization.
98-98:⚠️ Potential issuePotential issue with ineffectual assignment to
ctxThe static analysis tool reports an ineffectual assignment to
ctxon this line. This means that the result of thehandlerfunction is not being used, which could lead to unexpected behavior.To address this, consider updating the line as follows:
- ctx, err = handler(ctx, sdkTx, false) + _, err = handler(ctx, sdkTx, false)This change acknowledges that we're not using the returned context while still capturing any potential errors.
To ensure this change doesn't impact other parts of the code, please run the following command to check for other usages of the
ctxvariable after this line:🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 98-98:
ineffectual assignment to ctx (ineffassign)app/test/process_proposal_test.go (1)
23-23: LGTM: New import for v3 constantsThe new import for
v3constants is correctly added and follows the existing import style. This import is necessary for using thev3.Versionconstant in the new test case.x/blob/ante/blob_share_decorator.go (2)
39-39: Update share calculation to include transaction sizeIncluding
uint32(len(ctx.TxBytes()))in thegetSharesNeededfunction ensures that the transaction's base size is accounted for in the share calculation. This change enhances the accuracy of share requirements.
78-82: Verify all usages ofgetSharesNeededare updated to the new signatureWith the updated function signature of
getSharesNeededto include thebaseparameter, ensure that all function calls togetSharesNeededin the codebase are updated accordingly.Run the following script to find any calls to
getSharesNeededthat may not have been updated:This script searches for any instances where
getSharesNeededis called with only one argument, which would indicate places in the codebase that have not been updated to the new function signature.✅ Verification successful
All usages of
getSharesNeededhave been successfully updated to the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `getSharesNeeded` with only one argument. # Test: Search for function calls to `getSharesNeeded` with a single argument. ast-grep --lang go --pattern 'getSharesNeeded($_)'Length of output: 52
Script:
#!/bin/bash # Description: Find all calls to `getSharesNeeded` with only one argument using ripgrep. # Search for `getSharesNeeded` followed by an opening parenthesis, a single argument, and a closing parenthesis. rg 'getSharesNeeded\s*\(\s*[^,]+\s*\)'Length of output: 189
app/test/prepare_proposal_test.go (3)
140-155: Construction of oversized transaction is appropriateThe code correctly constructs a transaction (
tooManyShareBtx) intended to exceed the block size limits, which is essential for testing the pruning logic inPrepareProposal. The use ofrepeat(4000, 1)efficiently creates the necessary blob sizes.
200-206: New test case effectively tests pruning of oversized transactionsThe addition of the test case
"blob tx with too many shares"enhances the test coverage by ensuring that transactions exceeding the share limit are correctly pruned during proposal preparation.
243-250: Ensure project supports Go generics used in therepeatfunctionThe
repeatfunction utilizes generics ([T any]), a feature introduced in Go 1.18. Verify that the project is configured to use Go 1.18 or higher to prevent any build or compatibility issues.You can confirm the Go version specified in
go.modusing the following command:Expected result: The output should show
go 1.18or a higher version.✅ Verification successful
Project is configured to use Go 1.23.1, which supports generics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check that the project specifies Go 1.18 or newer in go.mod grep '^go ' go.modLength of output: 30
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/process_proposal.go (1 hunks)
- x/blob/ante/blob_share_decorator.go (3 hunks)
- x/blob/ante/blob_share_decorator_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/process_proposal.go
- x/blob/ante/blob_share_decorator_test.go
🧰 Additional context used
🔇 Additional comments (1)
x/blob/ante/blob_share_decorator.go (1)
80-84: Inclusion of transaction size in share calculation is appropriateThe updated
getSharesNeededfunction now accounts for the base transaction size, which enhances the accuracy of the share requirement calculation.
x/blob/ante/blob_share_decorator.go
Outdated
| @@ -32,11 +34,11 @@ func (d BlobShareDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool | |||
| if ctx.BlockHeader().Version.App == v1.Version { | |||
| return next(ctx, tx, simulate) | |||
| } | |||
|
|
|||
| fmt.Println("calling the ante handler lol") | |||
There was a problem hiding this comment.
Remove debug print statement from production code
The fmt.Println("calling the ante handler lol") statement appears to be a leftover debugging statement and should be removed to maintain clean and professional code.
Apply this diff to remove the statement:
- fmt.Println("calling the ante handler lol")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Println("calling the ante handler lol") |
| if sharesNeeded := getSharesNeeded(uint32(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares { | ||
| return ctx, errors.Wrapf(blobtypes.ErrBlobsTooLarge, "the number of shares occupied by blobs in this MsgPayForBlobs %d exceeds the max number of shares available for blob data %d", sharesNeeded, maxBlobShares) | ||
| } |
There was a problem hiding this comment.
Potential integer overflow when casting transaction size to uint32
Casting len(ctx.TxBytes()) to uint32 may cause an integer overflow if the transaction size exceeds the maximum value of uint32. To prevent potential issues, consider using uint64 instead.
Apply this diff to use uint64:
- if sharesNeeded := getSharesNeeded(uint32(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares {
+ if sharesNeeded := getSharesNeeded(uint64(len(ctx.TxBytes())), pfb.BlobSizes); sharesNeeded > maxBlobShares {Also, update the getSharesNeeded function signature:
-func getSharesNeeded(base uint32, blobSizes []uint32) (sum int) {
+func getSharesNeeded(base uint64, blobSizes []uint32) (sum int) {Ensure that any functions called within getSharesNeeded, such as share.CompactSharesNeeded, accept uint64 parameters.
Committable suggestion was skipped due to low confidence.
Co-authored-by: CHAMI Rachid <[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)
- x/blob/ante/blob_share_decorator.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: test / test-race
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Check: test / test-coverage
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Check: test / test-fuzz
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Check: test / test
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Check: test / test-short
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used
[failure] 4-4:
"fmt" imported and not used
🪛 GitHub Check: lint / golangci-lint
x/blob/ante/blob_share_decorator.go
[failure] 4-4:
"fmt" imported and not used (typecheck)
| // create a tx that can't be included in a 64 x 64 when accounting for the | ||
| // pfb along with the shares |
There was a problem hiding this comment.
[optional]
| // create a tx that can't be included in a 64 x 64 when accounting for the | |
| // pfb along with the shares | |
| // create a tx that can't be included in a 64 x 64 when accounting for the | |
| // PFB shares along with the blob shares. |
| @@ -137,6 +137,22 @@ func TestPrepareProposalFiltering(t *testing.T) { | |||
| require.NoError(t, err) | |||
| noAccountTx := []byte(testutil.SendTxWithManualSequence(t, encConf.TxConfig, kr, nilAccount, accounts[0], 1000, "", 0, 6)) | |||
|
|
|||
| // create a tx that can't be included in a 64 x 64 when accounting for the | |||
| // pfb along with the shares | |||
| tooManyShareBtx := blobfactory.ManyMultiBlobTx( | |||
There was a problem hiding this comment.
[optional] rename for clarity
| tooManyShareBtx := blobfactory.ManyMultiBlobTx( | |
| tooManyShareBlobTx := blobfactory.ManyMultiBlobTx( |
…unted for in a different function
…p into evan/minor-bug
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/blob/ante/blob_share_decorator_test.go (1)
43-131: Comprehensive test case coverage with room for one more edge case.The updated test cases provide excellent coverage of various scenarios, including different blob sizes, numbers of blobs, and app versions. The edge cases are well-represented, aligning with the PR objective of optimizing
checkTx.Consider adding one more test case:
- A test with multiple blobs of varying sizes within the same transaction, to ensure correct handling of heterogeneous blob sizes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- x/blob/ante/blob_share_decorator.go (3 hunks)
- x/blob/ante/blob_share_decorator_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
x/blob/ante/blob_share_decorator_test.go (6)
10-13: LGTM: Import changes are appropriate.The new imports align well with the updated test structure and blob transaction handling. They provide necessary utilities for improved test setup and execution.
Also applies to: 17-17, 21-23
33-36: Improved test case structure.The updated test case structure with
blobsPerPFBandblobSizeprovides more granular control over blob characteristics. The addition ofappVersionenables testing across different app versions, enhancing compatibility checks.
39-40: LGTM: Enhanced test randomness.The addition of a random number generator improves test robustness by introducing randomness in blob transaction creation.
135-156: Improved test realism and validity.The new setup significantly enhances test realism by using actual signed transactions instead of mocks. This change aligns well with the previous suggestion to make the test more meaningful. The process of creating a signer, generating random blob transactions, and then unmarshalling and decoding them ensures that the test is working with valid and realistic blob transactions.
159-163: LGTM: Realistic context setup for decorator testing.The context setup with specific parameters, including app version and actual transaction bytes, ensures that the
BlobShareDecoratoris tested under realistic conditions. This approach improves the accuracy and reliability of the test.
Line range hint
1-167: Clarification needed: Missingrepeatfunction.The AI-generated summary mentioned an update to the
repeatfunction, making it generic. However, this function is not present in the current code. Could you please clarify if this change was reverted or if it's located in a different file?x/blob/ante/blob_share_decorator.go (3)
39-39: Correct inclusion of transaction size in share calculationsThe updated call to
getSharesNeededwithtxSizeensures that the shares used by the transaction are properly accounted for, enhancing the accuracy of share estimation.
52-53: Simplified calculation of maximum blob sharesReturning
totalSharesdirectly is appropriate since transaction shares are now included ingetSharesNeeded, ensuring the calculation remains accurate.
75-77: Accurate computation of total shares neededIncluding
txSizeinshare.CompactSharesNeeded(txSize)correctly accounts for the transaction's compact shares alongside the blobs' sparse shares, resulting in an accurate total.
| // todo: uncomment once we're sure this isn't consensus breaking | ||
| // sdkCtx = sdkCtx.WithTxBytes(tx) |
There was a problem hiding this comment.
sorry for one more review @rootulp
I commented this out as I think there are scenarios where this could be consensus breaking and this is not explicitly tested. It should likely be protected by an if statement for v3
There was a problem hiding this comment.
hmm this PR targets main so we can technically include consensus breaking things b/c main is the basis for v3.0.0.
IIUC we just can't backport this to v2.x
There was a problem hiding this comment.
do we want to remove this for now? and after this is backported to v2 we'll have it for main with version gates
|
for posterity, this is the PR that is being backported to v2, therefore, its a non-breaking version. ontop of this PR for v3, a breaking change can be made by uncommenting out the ctx change in process proposal edit: ref #3960 |
## Overview this PR makes a simple optimization for checkTx --------- Co-authored-by: Rootul P <[email protected]> Co-authored-by: CHAMI Rachid <[email protected]> (cherry picked from commit ca222a8) # Conflicts: # app/test/process_proposal_test.go # x/blob/ante/blob_share_decorator_test.go
## Overview this PR makes a simple optimization for checkTx <hr>This is an automatic backport of pull request #3954 done by [Mergify](https://mergify.com). --------- Co-authored-by: Evan Forbes <[email protected]> Co-authored-by: Rootul P <[email protected]>
Overview
this PR makes a simple optimization for checkTx