fix(baseapp): Use CometBFT's ComputeProtoSizeForTxs in defaultTxSelector.SelectTxForProposal
#18551
Conversation
WalkthroughWalkthroughThe changes involve a fix to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- baseapp/abci_utils.go (2 hunks)
Additional comments: 1
baseapp/abci_utils.go (1)
- 10-16: The import of the
tmtypespackage fromgit.luolix.top/cometbft/cometbft/typesis correctly added to use theComputeProtoSizeForTxsfunction. Ensure that there are no existing functions within the Cosmos SDK that already provide the same functionality to avoid redundancy.
baseapp/abci_utils.go
Outdated
| } | ||
|
|
||
| func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool { | ||
| txSize := uint64(len(txBz)) | ||
| txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz})) | ||
|
|
||
| var txGasLimit uint64 | ||
| if memTx != nil { |
There was a problem hiding this comment.
The SelectTxForProposal function now uses tmtypes.ComputeProtoSizeForTxs to compute the size of transactions, which aligns with the cometbft calculation methods. However, there is a potential issue with the check for memTx != nil after the txSize has been calculated. If memTx is nil, the txGasLimit will remain uninitialized, potentially leading to incorrect behavior when adding the transaction to the proposal. This should be handled before attempting to cast memTx to GasTx.
- txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz}))
-
- var txGasLimit uint64
- if memTx != nil {
+ txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz}))
+ var txGasLimit uint64
+ if memTx == nil {
+ return true // or handle the error appropriately
+ }Commitable 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.
| } | |
| func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool { | |
| txSize := uint64(len(txBz)) | |
| txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz})) | |
| var txGasLimit uint64 | |
| if memTx != nil { | |
| } | |
| func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool { | |
| txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz})) | |
| var txGasLimit uint64 | |
| if memTx == nil { | |
| return true // or handle the error appropriately | |
| } |
|
This looks good to me but I would love to see if there's a way to do this without having to use more Comet methods. Or maybe we do this now but we think about having a configurable "TxSize" per consensus engine |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 77-77:
The changelog entry is clear and accurately reflects the changes made in the pull request.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/abci_utils.go (2 hunks)
Additional comments: 3
CHANGELOG.md (1)
- 77-77:
The changelog entry for PR fix(baseapp): Use CometBFT'sComputeProtoSizeForTxsindefaultTxSelector.SelectTxForProposal#18551 is correctly added and provides a clear description of the changes made.baseapp/abci_utils.go (2)
10-16:
The import oftmtypesfromgit.luolix.top/cometbft/cometbft/typesis correctly added to useComputeProtoSizeForTxs.369-375:
The issue with the check formemTx != nilafter thetxSizehas been calculated is still present. IfmemTxisnil, thetxGasLimitwill remain uninitialized, potentially leading to incorrect behavior when adding the transaction to the proposal. This should be handled before attempting to castmemTxtoGasTx.
baseapp/abci_utils.go
Outdated
| @@ -368,7 +369,7 @@ func (ts *defaultTxSelector) Clear() { | |||
| } | |||
|
|
|||
| func (ts *defaultTxSelector) SelectTxForProposal(_ context.Context, maxTxBytes, maxBlockGas uint64, memTx sdk.Tx, txBz []byte) bool { | |||
| txSize := uint64(len(txBz)) | |||
| txSize := uint64(tmtypes.ComputeProtoSizeForTxs([]tmtypes.Tx{txBz})) | |||
There was a problem hiding this comment.
TestDefaultProposalHandler_NoOpMempoolTxSelection
Can this test be used
There was a problem hiding this comment.
Sure, adding a case there works!
baseapp/abci_utils.go
Outdated
| @@ -10,6 +10,7 @@ import ( | |||
| cryptoenc "github.com/cometbft/cometbft/crypto/encoding" | |||
| cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" | |||
| cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" | |||
| tmtypes "github.com/cometbft/cometbft/types" | |||
There was a problem hiding this comment.
| tmtypes "github.com/cometbft/cometbft/types" | |
| cmttypes "github.com/cometbft/cometbft/types" |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- baseapp/abci_utils.go (2 hunks)
Additional comments: 3
CHANGELOG.md (1)
- 77-77:
The changelog entry for PR fix(baseapp): Use CometBFT'sComputeProtoSizeForTxsindefaultTxSelector.SelectTxForProposal#18551 is correctly added and provides the necessary information about the changes made to theSelectTxForProposalfunction.baseapp/abci_utils.go (2)
10-16:
The import ofcmttypesis correctly added to support the new transaction size calculation method.369-375:
The change to usecmttypes.ComputeProtoSizeForTxsfor calculatingtxSizeis correctly implemented and aligns with CometBFT standards.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci_utils.go (2 hunks)
- baseapp/abci_utils_test.go (5 hunks)
Additional comments: 7
baseapp/abci_utils.go (2)
10-16:
The import ofcmttypesfrom"github.com/cometbft/cometbft/types"is correctly added to facilitate the use ofComputeProtoSizeForTxs.369-375:
The change to calculatetxSizeusingcmttypes.ComputeProtoSizeForTxsaligns with the CometBFT standards as described in the summary.baseapp/abci_utils_test.go (5)
3-8:
The import aliascmttypesfor the packagegit.luolix.top/cometbft/cometbft/typesis correctly added to facilitate the use of theComputeProtoSizeForTxsfunction.311-315:
The test case now correctly calculates the transaction size usingComputeProtoSizeForTxsfrom thecmttypespackage, aligning with the CometBFT standards.335-360: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [335-367]
The test cases "small max tx bytes" and "small max gas" have been updated with new
MaxTxBytesvalues to accommodate the changes in transaction size calculation.
- 335-360: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [342-368]
New test cases "large max tx bytes" and "large max tx bytes len calculation" have been added to validate the transaction size calculation for different
MaxTxByteslimits.
- 372-378:
The loop in the test case ensures that the transaction selector is cleared each time, which is a good practice to avoid state leakage between iterations.
There was a problem hiding this comment.
We do not use the CometBFT ComputeProtoSizeForTxs API anywhere in the codebase. I'd like to better understand what and where the failures are for you. Note, we use len(txBz) as the tx in many places, e.g. gas computation.
EDIT: Apologies, I somehow missed the created issue for this. I understand now 👍
| @@ -78,6 +78,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * (client/server) [#18345](https://github.com/cosmos/cosmos-sdk/pull/18345) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server. | |||
| * (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test. | |||
| * (simulation) [#18196](https://github.com/cosmos/cosmos-sdk/pull/18196) Fix the problem of `validator set is empty after InitGenesis` in simulation test. | |||
| * (baseapp) [#18551](https://github.com/cosmos/cosmos-sdk/pull/18551) Fix SelectTxForProposal the calculation method of tx bytes size is inconsistent with CometBFT | |||
There was a problem hiding this comment.
The changelog entry could be more descriptive by mentioning the specific method tmtypes.ComputeProtoSizeForTxs used for transaction size calculation to align with CometBFT standards.
|
@LX-Xiang could you run |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 83-83: The changelog entry could be more descriptive by mentioning the specific method
tmtypes.ComputeProtoSizeForTxsused for transaction size calculation to align with CometBFT standards.
|
@tac0turtle i running |
Yes, you should run |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/abci_utils.go (2 hunks)
- baseapp/abci_utils_test.go (5 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional comments: 8
baseapp/abci_utils.go (1)
- 369-375: The use of
ComputeProtoSizeForTxsfrom thecmttypespackage for calculating transaction size is a good change for consistency with CometBFT standards. However, ensure that theComputeProtoSizeForTxsfunction is thoroughly tested, especially since transaction size calculation is critical for block production.baseapp/abci_utils_test.go (7)
8-14: The import alias
cmttypeshas been removed in the diff summary, but it is still present in the hunk. Please ensure that the code reflects the intended changes.314-315: The update to use
types.ComputeProtoSizeForTxsfor calculatingtxDataSizeis consistent with the summary and the hunk. Ensure that thetypespackage provides the same functionality as thecmttypespackage to avoid any unexpected behavior.350-356: The summary mentions the addition of a new test case "large max tx bytes len calculation," which seems to be a new scenario within the existing test cases. Ensure that this new scenario is adequately tested and that the expected behavior aligns with the changes made to the transaction byte size calculation.
338-338: The
MaxTxBytesvalue in the test case "small max gas" has been updated. Verify that this value is consistent with the expected transaction size after the changes to the transaction byte size calculation.346-346: The
MaxTxBytesvalue in the test case "large max tx bytes" has been updated. Verify that this value is consistent with the expected transaction size after the changes to the transaction byte size calculation.354-354: The
MaxTxBytesvalue in the test case "large max tx bytes len calculation" has been updated. Verify that this value is consistent with the expected transaction size after the changes to the transaction byte size calculation.366-366: The
MaxTxBytesvalue in the test case "max gas and tx bytes" has been updated. Verify that this value is consistent with the expected transaction size after the changes to the transaction byte size calculation.
|
@julienrbrt sorry, I didn't pay attention to this modification，I recommitted |
julienrbrt
changed the title
ComputeProtoSizeForTxs in defaultTxSelector.SelectTxForProposal
Description
ref: #18547
Tx byte size calculations are consistent with cometbft
PrepareProposalHandler SelectTxForProposal Tx bytes the inconsistent calculation methods are causing block production failures.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Bug Fixes
Documentation