refactor(server/v2): Update prepare & process proposal #21237
Conversation
|
Warning Rate limit exceeded@hieuvubk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe recent updates enhance error handling and robustness across several components, particularly in transaction validation, proposal preparation, and processing. Key modifications include checks for nil handlers, validation of proposal heights, and improved memory efficiency in transaction decoding through dynamic slicing. Additionally, code clarity and correctness are improved by replacing outdated references, ensuring that invalid states are caught early and providing clearer feedback during operations. Changes
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
|
github-actions
bot
added
C:server/v2
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- server/v2/appmanager/appmanager.go (1 hunks)
- server/v2/cometbft/abci.go (3 hunks)
- server/v2/cometbft/handlers/defaults.go (3 hunks)
Additional context used
Path-based instructions (3)
server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (10)
server/v2/cometbft/handlers/defaults.go (4)
44-44: Ensure correct error handling for consensus parameter queries.The use of
consensustypes.QueryParamsRequestaligns with the updated consensus types. Ensure that any potential errors from the query are properly logged or handled to aid in debugging.
118-118: Ensure correct error handling for consensus parameter queries.The use of
consensustypes.QueryParamsRequestaligns with the updated consensus types. Ensure that any potential errors from the query are properly logged or handled to aid in debugging.
49-51: Verify type assertion for consensus parameters response.The type assertion for
consensustypes.QueryParamsResponseshould be validated thoroughly to avoid runtime panics. Consider logging the error details for better traceability.Verification successful
Consider adding logging for type assertion failures.
The type assertion for
consensustypes.QueryParamsResponseis correctly handled to avoid runtime panics by returning an error. However, adding logging for these errors could improve traceability and aid in debugging. Consider incorporating logging to capture these error details.
- File:
server/v2/cometbft/handlers/defaults.go- Lines: 49-51
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `consensustypes.QueryParamsResponse` in the codebase. # Test: Search for the usage of `consensustypes.QueryParamsResponse`. Expect: Proper error handling around its usage. rg --type go 'consensustypes.QueryParamsResponse'Length of output: 1148
123-125: Verify type assertion for consensus parameters response.The type assertion for
consensustypes.QueryParamsResponseshould be validated thoroughly to avoid runtime panics. Consider logging the error details for better traceability.Verification successful
Type assertion for consensus parameters response is properly handled.
The type assertion for
consensustypes.QueryParamsResponseinserver/v2/cometbft/handlers/defaults.gois already accompanied by error handling that logs an appropriate error message if the assertion fails. This should prevent runtime panics and provide necessary traceability. No further changes are needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `consensustypes.QueryParamsResponse` in the codebase. # Test: Search for the usage of `consensustypes.QueryParamsResponse`. Expect: Proper error handling around its usage. rg --type go 'consensustypes.QueryParamsResponse'Length of output: 1148
server/v2/appmanager/appmanager.go (1)
131-132: Improved error handling inValidateTx.The change to return
res.Errorexplicitly enhances the robustness of the transaction validation logic by providing clearer feedback on validation outcomes.server/v2/cometbft/abci.go (5)
318-320: Check for nilprepareProposalHandler.The addition of a nil check for
prepareProposalHandlerprevents the method from proceeding with an invalid state, enhancing robustness.
Line range hint
322-332:
Use dynamic slicing fordecodedTxs.Replacing fixed-length slice initialization with appending to a dynamically sized slice improves memory efficiency and flexibility in handling transactions.
363-365: Check for valid proposal height.The check for
req.Heightbeing greater than or equal to 1 prevents processing proposals with invalid heights, enhancing robustness.
367-369: Check for nilprocessProposalHandler.The addition of a nil check for
processProposalHandlerprevents the method from proceeding with an invalid state, enhancing robustness.
371-371: Use dynamic slicing fordecodedTxs.Replacing fixed-length slice initialization with appending to a dynamically sized slice improves memory efficiency and flexibility in handling transactions.
julienrbrt
added
the
backport/v0.52.x
server/v2/cometbft/abci.go
Outdated
| return nil, errors.New("no prepare proposal function was set") | ||
| } | ||
|
|
||
| var decodedTxs []T |
There was a problem hiding this comment.
we should preallocate here as we know the length
There was a problem hiding this comment.
Thought the same but we skip undecodable txs, so the index would then be nil
There was a problem hiding this comment.
The only case we can't decode is if it's a vote exetension or some other data type of the state machine right?
There was a problem hiding this comment.
need to remove the decoding part and make it part of the handler. it wont work this way, and requiring users to modify the decoder feels weird
There was a problem hiding this comment.
makes sense to move it to handler.
server/v2/cometbft/abci.go
Outdated
| @@ -325,7 +329,7 @@ func (c *Consensus[T]) PrepareProposal( | |||
| continue | |||
| } | |||
|
|
|||
| decodedTxs[i] = decTx | |||
| decodedTxs = append(decodedTxs, decTx) | |||
There was a problem hiding this comment.
i dont believe this is correct. in the case of vote extensions we will not be able to decode the txs meaning we will always remove it from the block. This should happen in the handler instead of here. This is how its handled in abci.
|
Move the tx decode part to the handler. ProposalTxVerifier interface {
PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error)
ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error)
TxDecode(txBz []byte) (sdk.Tx, error)
TxEncode(tx sdk.Tx) ([]byte, error)
} |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
server/v2/cometbft/handlers/defaults.go (1)
149-162: Consider handling decoding errors indecodeTxs.The
decodeTxsfunction currently continues on decoding errors, which may lead to silent failures. Consider logging or handling these errors more explicitly.decTx, err := codec.Decode(tx) if err != nil { // Log or handle the decoding error continue }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- server/v2/cometbft/abci.go (4 hunks)
- server/v2/cometbft/handlers/defaults.go (6 hunks)
- server/v2/cometbft/handlers/handlers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/abci.go
Additional context used
Path-based instructions (2)
server/v2/cometbft/handlers/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
server/v2/cometbft/handlers/handlers.go (2)
15-15: Function signature update approved.The
PrepareHandlerfunction signature changes improve type safety and align with the ABCI protocol.
19-19: Function signature update approved.The
ProcessHandlerfunction signature changes improve type safety and align with the ABCI protocol.server/v2/cometbft/handlers/defaults.go (3)
14-14: Import statement update approved.The import of
consensustypesaligns with the updated handling of consensus parameters.
101-116: Update to consensus parameter handling approved.The use of
consensustypes.QueryParamsRequestandconsensustypes.QueryParamsResponseis correctly implemented inProcessHandler.Ensure that the new types are correctly used throughout the codebase.
35-45: Update to consensus parameter handling approved.The use of
consensustypes.QueryParamsRequestandconsensustypes.QueryParamsResponseis correctly implemented.Ensure that the new types are correctly used throughout the codebase.
Verification successful
Consistent Usage of Consensus Types Verified
The
consensustypes.QueryParamsRequestandconsensustypes.QueryParamsResponseare used consistently across the codebase, including in test files and expected keeper interfaces. This suggests that the integration is thorough and correctly implemented.
- Files Verified:
x/staking/keeper/keeper_test.gox/staking/types/expected_keepers.gox/auth/ante/testutil_test.gox/auth/ante/expected_keepers.goserver/v2/cometbft/handlers/defaults.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `consensustypes.QueryParamsRequest` and `consensustypes.QueryParamsResponse`. # Test: Search for the usage of the new types. Expect: Consistent usage across the codebase. rg --type go 'consensustypes.QueryParams(Request|Response)'Length of output: 1380
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/appmanager/appmanager.go (1 hunks)
- server/v2/cometbft/abci.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/abci.go
Additional context used
Path-based instructions (1)
server/v2/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
server/v2/appmanager/appmanager.go (1)
129-130: Improved error handling inValidateTx.The change to capture and return the error from
a.stf.ValidateTxenhances the robustness of the transaction validation process by ensuring errors are not silently ignored.To ensure the changes work as intended, verify the usage of
ValidateTxin the codebase to confirm that the error handling is correctly propagated.Verification successful
Error handling for
ValidateTxis correctly implemented across the codebase.The review of the
ValidateTxfunction's usage confirms that errors are appropriately handled in all instances where this function is called. The changes made inserver/v2/appmanager/appmanager.goalign with the overall error handling strategy.
- Files with proper error handling:
x/auth/module.gox/auth/ante/basic.gox/auth/ante/sigverify.goserver/v2/cometbft/abci.goserver/v2/cometbft/handlers/defaults.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ValidateTx` to ensure proper error handling. # Test: Search for the function usage. Expect: Proper handling of the returned error. rg --type go -A 5 $'ValidateTx'Length of output: 16376
lets recreate the same logic for simplicity for the defaults |
Nvm my bad, it was replaced by |
…smos-sdk into hieu/refactor-proposal
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
server/v2/cometbft/handlers/defaults.go (4)
38-45: Enhance error handling with more context.The error handling for the query response type assertion could include more context to aid debugging.
- return nil, fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res) + return nil, fmt.Errorf("unexpected consensus params response type in PrepareHandler; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res)
109-116: Enhance error handling with more context.Similar to
PrepareHandler, the error handling for the query response type assertion could include more context.- return fmt.Errorf("unexpected consensus params response type; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res) + return fmt.Errorf("unexpected consensus params response type in ProcessHandler; expected: %T, got: %T", &consensustypes.QueryParamsResponse{}, res)
170-171: Inconsistent Error Handling fordecodeTxsThe
decodeTxsfunction has inconsistent error handling across its implementations. Inabci.go, it returns an error and handles it, while inhandlers/defaults.go, it does not return an error. This discrepancy could lead to unhandled errors in transaction decoding, particularly in theNoOpPrepareProposalfunction.
- Review the
decodeTxsfunction inhandlers/defaults.goto ensure it aligns with the error handling approach inabci.go.- Consider updating the
NoOpPrepareProposalfunction to handle potential errors fromdecodeTxs.Analysis chain
Verify consistency in error handling for
decodeTxs.Ensure that any changes to error handling in
decodeTxsare consistently applied across all usages, including inNoOpPrepareProposal.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency in error handling for `decodeTxs`. # Test: Search for all usages of `decodeTxs` to ensure consistent error handling. rg --type go 'decodeTxs' -A 3Length of output: 1796
52-52: Consider implementing error handling fordecodeTxsfunction.The
decodeTxsfunction is used inserver/v2/cometbft/handlers/defaults.go, but its potential errors are not currently handled or logged. It is advisable to handle these errors for improved traceability and robustness.
server/v2/cometbft/handlers/defaults.go: Ensure errors fromdecodeTxsare properly handled or logged.Analysis chain
Consider error handling in transaction decoding.
The
decodeTxsfunction currently ignores decoding errors. Consider handling these errors or logging them for better traceability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if decoding errors are logged or handled in the codebase. # Test: Search for any logging or error handling related to decodeTxs function. rg --type go 'decodeTxs' -A 5 | rg 'log|error'Length of output: 166
Script:
#!/bin/bash # Search for the usage of decodeTxs in the specified file and check if the error is handled or logged. rg 'decodeTxs' server/v2/cometbft/handlers/defaults.go -A 5Length of output: 975
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/cometbft/handlers/defaults.go (6 hunks)
Additional context used
Path-based instructions (1)
server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
server/v2/cometbft/handlers/defaults.go (2)
124-124: Consider error handling in transaction decoding.The
decodeTxsfunction is used here as well, and similar considerations for error handling or logging apply.
178-179: LGTM!The
NoOpProcessProposalfunction is correctly implemented as a no-op handler.
| // decodeTxs decodes the txs bytes into a decoded txs | ||
| // If there a fail decoding tx, remove from the list | ||
| // TODO: should return an err here? | ||
| func decodeTxs[T transaction.Tx](codec transaction.Codec[T], txsBz [][]byte) []T { | ||
| var txs []T | ||
| for _, tx := range txsBz { | ||
| decTx, err := codec.Decode(tx) | ||
| if err != nil { | ||
| // TODO: vote extension meta data as a custom type to avoid possibly accepting invalid txs | ||
| // continue even if tx decoding fails | ||
| continue | ||
| } | ||
|
|
||
| txs = append(txs, decTx) | ||
| } | ||
| return txs | ||
| } |
There was a problem hiding this comment.
Improve error handling in decodeTxs.
The function skips transactions that fail to decode without reporting errors. Consider returning an error or logging the failure to improve traceability. The TODO comment also suggests this improvement.
func decodeTxs[T transaction.Tx](codec transaction.Codec[T], txsBz [][]byte) ([]T, error) {
var txs []T
var decodeErrors []error
for _, tx := range txsBz {
decTx, err := codec.Decode(tx)
if err != nil {
decodeErrors = append(decodeErrors, err)
continue
}
txs = append(txs, decTx)
}
if len(decodeErrors) > 0 {
return txs, fmt.Errorf("failed to decode some transactions: %v", decodeErrors)
}
return txs, nil
}
| } | ||
|
|
||
| decodedTxs[i] = decTx | ||
| if c.prepareProposalHandler == nil { |
There was a problem hiding this comment.
shouldn't this fail at app build time?
There was a problem hiding this comment.
if you don't pass anything to the comet server, we use the default options (no op handlers).
so to get this you would have to purposely pass nil. as we don't want to make those mandatory, making it fail at compile time isn't best.
| for _, tx := range txsBz { | ||
| decTx, err := codec.Decode(tx) | ||
| if err != nil { | ||
| // TODO: vote extension meta data as a custom type to avoid possibly accepting invalid txs |
There was a problem hiding this comment.
i think it's important to understand what to do here, is it fine to continue with txs if they fail decoding?
There was a problem hiding this comment.
yes agree, I think we should decide about it. Like v1 we reject all if have a tx decoded fail.
(cherry picked from commit 95b8092) # Conflicts: # server/v2/appmanager/appmanager.go
…) (#21536) Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
Closes: #XXXX
stf.ValidateTxresult errAuthor 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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes