feat(server/v2/cometbft): Implement necessary handlers and helpers for vote extensions #22830
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the application builder, server, and testing infrastructure across multiple files. The primary changes include adding a new 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
|
github-actions
bot
added
C:x/auth
C:server/v2
julienrbrt
added
the
backport/v0.52.x
facundomedica
requested review from
a team,
testinginprod,
aaronc,
kocubinski,
sontrinh16 and
hieuvubk
as code owners
facundomedica
changed the title
|
@facundomedica your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
tests/integration/v2/services.go (2)
Line range hint
235-248: Avoid re-initializing the gas meter if originalGasMeter is nil.The code unconditionally invokes Consume on the originalGasMeter. If the original context had no gas meter, this can panic. A nil-check or a default fallback could prevent unexpected runtime errors.
-originalGasMeter.Consume(gasUsed, "execute-with-gas-limit") +if originalGasMeter != nil { + originalGasMeter.Consume(gasUsed, "execute-with-gas-limit") +}
The handlers map in RouterService needs mutex protection for concurrent access
The code analysis reveals that RouterService is used in a concurrent environment:
- The handlers map is modified during registration via
RegisterHandlermethod- The map is accessed during
InvokeandCanInvokeoperations- These operations are used across multiple modules (gov, authz, accounts) that may run concurrently
To prevent potential race conditions, the handlers map should be protected by a mutex. Here's what needs to be addressed:
- Add a mutex field to RouterService struct
- Lock the mutex during RegisterHandler calls
- Use RLock during Invoke and CanInvoke operations
🔗 Analysis chain
Line range hint
291-291: Consider map accesses under concurrent usage.The handlers map in RouterService is not protected by any mutex. If there's a possibility of concurrent registration or invocation, this could cause race conditions.
Let me analyze the code to verify the concern about concurrent access to the handlers map in RouterService.
Let me analyze the RouterService implementation more closely to verify the concurrent access concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking usage patterns for `RouterService` to ensure no concurrent writes rg -A 5 "RouterService"Length of output: 46162
Script:
#!/bin/bash # Let's check if there are any concurrent write operations to RouterService ast-grep --pattern 'go func() { $$$ $router.$$$ $$$ }' # Also check for goroutines that might access RouterService rg -A 3 "go \w*\("Length of output: 28352
Script:
#!/bin/bash # Check the implementation of RouterService and its usage patterns ast-grep --pattern 'type RouterService struct { $$$ }' # Check for any concurrent invocations of RegisterHandler rg -A 5 "RegisterHandler.*RouterService" # Check if there are any concurrent invocations of Invoke or CanInvoke rg -A 5 "(\w+\s*:=\s*)?.*RouterService.*\.(Invoke|CanInvoke)"Length of output: 6407
🧹 Nitpick comments (6)
tests/integration/v2/services.go (2)
173-173: Consider returning an error instead of panicking.The code panics if the context fails type assertion, which terminates test execution abruptly. Converting panics into returned errors can provide a cleaner failure mode.
-func (e *eventService) EventManager(ctx context.Context) event.Manager { - iCtx, ok := ctx.Value(contextKey).(*integrationContext) - if !ok { - panic("context is not an integration context") - } - return &eventManager{ctx: iCtx} +func (e *eventService) EventManager(ctx context.Context) (event.Manager, error) { + iCtx, ok := ctx.Value(contextKey).(*integrationContext) + if !ok { + return nil, errors.New("context is not an integration context") + } + return &eventManager{ctx: iCtx}, nil }
Line range hint
339-346: Enhance gas configuration granularity.The returned GasConfig is empty, which may limit the ability to fine-tune gas usage during tests or expansions. Consider returning a test-specific configuration to exercise gas logic more thoroughly in integration tests.
server/v2/cometbft/utils.go (2)
456-479: Consider returning actual values in InjectedTx methods.
The methods GetGasLimit, GetMessages, and GetSenders all return hardcoded or empty values. If your use cases need fully functional transaction data, it might help to populate these with real values or handle them carefully at higher layers. Otherwise, consider documenting these method stubs clearly.
480-608: Looks good—recommend thorough testing for vote extensions.
ValidateVoteExtensions contains robust checks for vote extension signatures. Ensure there's comprehensive test coverage, especially around edge cases and error paths.Would you like me to help generate some baseline tests for this function?
simapp/v2/simdv2/cmd/config.go (1)
95-98: Consider clarifying the commented handler code.
These lines suggest a plan to implement custom vote extension handlers. If they are meant to be examples, consider adding a short comment explaining that they are placeholders. If they are a TODO, you might track it in a separate issue to make it more visible.x/auth/tx/gogotx.go (1)
Line range hint
35-50: LGTM! Consider adding more detailed commentsThe changes improve robustness by properly handling nil fee amounts and using the
Addmethod for fee accumulation. The inline comment about nil amounts is helpful, but consider expanding it to explain the rationale for usingAdd.Consider adding a more detailed comment:
- fees = sdk.Coins{} // decodedTx.Tx.AuthInfo.Fee.Amount might be nil + // Initialize empty fees since decodedTx.Tx.AuthInfo.Fee.Amount might be nil + // Use Add method for proper coin addition and validation + fees = sdk.Coins{}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
runtime/v2/builder.go(3 hunks)server/v2/cometbft/abci.go(9 hunks)server/v2/cometbft/handlers/defaults.go(4 hunks)server/v2/cometbft/handlers/handlers.go(1 hunks)server/v2/cometbft/options.go(1 hunks)server/v2/cometbft/utils.go(4 hunks)simapp/v2/go.mod(1 hunks)simapp/v2/simdv2/cmd/config.go(1 hunks)tests/integration/v2/gov/common_test.go(0 hunks)tests/integration/v2/gov/keeper/fixture_test.go(0 hunks)tests/integration/v2/services.go(1 hunks)x/auth/tx/gogotx.go(2 hunks)
💤 Files with no reviewable changes (2)
- tests/integration/v2/gov/common_test.go
- tests/integration/v2/gov/keeper/fixture_test.go
🧰 Additional context used
📓 Path-based instructions (9)
server/v2/cometbft/options.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/auth/tx/gogotx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/utils.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.
tests/integration/v2/services.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/utils.go
18-18: ST1019: package "github.com/cosmos/gogoproto/proto" is being imported more than once
(stylecheck)
🔇 Additional comments (17)
tests/integration/v2/services.go (1)
Line range hint 205-231: Verify concurrency safety when updating parent state from branched state.
Merging state changes from a branched context into the parent context could introduce race conditions if multiple goroutines branch and merge concurrently. Reviewing usage and ensuring that merges are protected by proper synchronization (or that execution remains single-threaded during merges) is advisable.
server/v2/cometbft/utils.go (2)
450-454: Function name matches functionality—no issues found.
RawTx returns an InjectedTx instance and is suitably named.
356-357: Verify usage of version 0 for consensus parameters.
The function GetConsensusParams hardcodes the height version to 0 when querying the consensus module. Please confirm that this is the intended behavior and that the latest version isn't required.
✅ Verification successful
Let me check the implementation of the function and its usage context to better understand the height parameter.
Let me check the QueryParamsRequest implementation and related code to understand if height parameter has any special meaning.
Let me check the consensus module's query implementation to understand if height parameter has any special meaning.
Let me check how the consensus parameters are used in the codebase, particularly around the height parameter.
Based on the code analysis, I can now provide a final response about the usage of height 0 in GetConsensusParams.
Using height 0 for consensus parameters query is the correct behavior
The height parameter of 0 is used to query the latest consensus parameters from the current state. This is evident from:
- The consensus module's QueryParamsRequest doesn't require a height parameter in its protobuf definition
- The function is used in critical paths like ABCI interface where it needs to get the latest consensus parameters
- The pattern of using height 0 to get latest state is a common pattern in Cosmos SDK
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# This script searches for all invocations of GetConsensusParams to see whether
# any call site expects a non-zero version.
ast-grep --pattern $'GetConsensusParams($_, $_)'
Length of output: 660
Script:
#!/bin/bash
# Check the implementation of GetConsensusParams and related code
rg -A 5 "func GetConsensusParams"
Length of output: 759
Script:
#!/bin/bash
# Search for QueryParamsRequest definition and usage
rg -A 5 "QueryParamsRequest"
# Also check if there are any comments or documentation about height parameter in query
ast-grep --pattern 'Query($_, $height, $_)'
Length of output: 75002
Script:
#!/bin/bash
# Search for consensus module's query implementation
rg -A 5 "func.*Query.*consensus"
# Also check if there are any comments about height in the consensus module
rg -B 2 -A 2 "height.*consensus"
Length of output: 46441
Script:
#!/bin/bash
# Search for usage of consensus parameters in the codebase
rg -B 2 -A 2 "GetConsensusParams.*height"
# Also check the implementation of the consensus module's Query method
rg -A 5 "func.*Query.*consensus.*Params"
Length of output: 555
server/v2/cometbft/handlers/handlers.go (1)
16-20: Name all arguments for clarity.
According to a past review comment, chainID was added as a parameter. It may help to name all parameters consistently for readability (rather than relying solely on types).
server/v2/cometbft/options.go (1)
23-23: Type name correction looks good.
The change from VerifyVoteExtensionhandler to VerifyVoteExtensionHandler resolves the naming inconsistency.
server/v2/cometbft/handlers/defaults.go (4)
177-179: LGTM!
The addition of the chainID parameter maintains consistency with the interface while preserving the no-op behavior.
185-187: LGTM!
The addition of the chainID parameter maintains consistency with the interface while preserving the no-op behavior.
Line range hint 200-204: LGTM!
The type name has been corrected to follow proper naming conventions.
35-35: Verify the usage of chainID parameter
The chainID parameter has been added to the function signature but is not used within the function body. This might indicate incomplete implementation or missing functionality.
runtime/v2/builder.go (2)
33-33: LGTM!
The preblocker implementation correctly chains the default module manager's preblocker with any custom preblocker provided. The error handling is appropriate, and the execution order ensures that the default preblocker runs before any custom implementation.
Also applies to: 99-109
236-244: LGTM!
The AppBuilderWithPreblocker function follows the established pattern of other AppBuilderOption functions and provides a clean way to set custom pre-blocking logic.
simapp/v2/go.mod (1)
283-283: LGTM!
The new replace directive for cosmossdk.io/x/auth follows the established pattern in the file and correctly points to the local directory.
server/v2/cometbft/abci.go (5)
71-71: LGTM: Fixed type name capitalization.
The type name correction follows Go naming conventions for proper capitalization of type names.
Line range hint 145-153: LGTM: Improved error handling for consensus parameters.
The code now properly handles the case where consensus parameters are not found by explicitly checking for collections.ErrNotFound and setting appVersion to 0 in such cases.
390-390: LGTM: Added chain ID to proposal handlers.
The change properly passes the chain ID to both prepare and process proposal handlers, ensuring proper validation context is available.
Also applies to: 436-436
536-536: LGTM: Standardized consensus parameters retrieval.
The code now consistently uses GetConsensusParams across all methods, improving code consistency and maintainability.
Also applies to: 603-603, 622-622, 661-661
705-706: Verify type assertion safety.
The type assertion RawTx(rawTx).(T) could potentially panic if type T doesn't implement the required interface. Please ensure that all possible types that can be used as T are compatible with RawTx.
✅ Verification successful
Type assertion is safe in this context.
The type assertion RawTx(rawTx).(T) is safe because:
RawTxreturns atransaction.Txinterface implementation (specificallyInjectedTx)- The generic type parameter
Tis constrained totransaction.Txin the function signaturedecodeTxs[T transaction.Tx] - The type assertion is used as a fallback mechanism when decoding fails, allowing access to raw transaction bytes while maintaining the expected interface
The implementation ensures type safety through Go's generic constraints, making it impossible for T to be anything that doesn't implement transaction.Tx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all types that could be used as T in this context
# to verify they implement the necessary interface.
# Find all type declarations that could be used as T
ast-grep --pattern 'type $name struct { $$$ }'
# Find all interface implementations
ast-grep --pattern 'func ($receiver $type) $method($$$) $$$'
Length of output: 111
Script:
#!/bin/bash
# Let's first find the RawTx type definition and its usage context
rg -A 5 "type RawTx"
# Find where RawTx is used in type parameters or generic constraints
rg "RawTx"
# Find the decodeTxs function definition and its context
rg -A 10 "func decodeTxs"
# Find any interface definitions that might be related to transactions
rg -A 5 "type.*interface"
Length of output: 82585
Script:
#!/bin/bash
# Let's find the transaction.Tx interface definition and its implementations
rg -A 5 "type.*Tx.*interface"
# Find the RawTx function implementation and its context
rg -A 10 "func RawTx"
# Find where type T is used in the decodeTxs function context
rg -B 5 -A 10 "func decodeTxs\[T"
Length of output: 19006
| "strings" | ||
| "time" | ||
|
|
||
| abci "github.com/cometbft/cometbft/api/cometbft/abci/v1" | ||
| cmtproto "github.com/cometbft/cometbft/api/cometbft/types/v1" | ||
| cryptoenc "github.com/cometbft/cometbft/crypto/encoding" | ||
| protoio "github.com/cosmos/gogoproto/io" | ||
| "github.com/cosmos/gogoproto/proto" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate import to adhere to style guidelines.
The package "github.com/cosmos/gogoproto/proto" is imported twice (lines 18 and 19). Please remove or consolidate one of these imports in order to comply with ST1019 (no duplicate imports).
Suggested diff:
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
18-18: ST1019: package "github.com/cosmos/gogoproto/proto" is being imported more than once
(stylecheck)
| @@ -98,7 +98,7 @@ func (h *DefaultProposalHandler[T]) PrepareHandler() PrepareHandler[T] { | |||
| } | |||
|
|
|||
| func (h *DefaultProposalHandler[T]) ProcessHandler() ProcessHandler[T] { | |||
| return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest) error { | |||
| return func(ctx context.Context, app AppManager[T], codec transaction.Codec[T], req *abci.ProcessProposalRequest, chainID string) error { | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove unused chainID parameter from ProcessHandler
The chainID parameter is indeed not used within the ProcessHandler implementation. The function body only deals with mempool operations and doesn't utilize the chainID parameter anywhere.
- Remove the
chainIDparameter from the ProcessHandler function signature inserver/v2/cometbft/handlers/defaults.go
🔗 Analysis chain
Verify the usage of chainID parameter
Similar to PrepareHandler, the chainID parameter has been added but is not utilized within the function body.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for chainID usage in process proposal handlers
rg -A 5 "chainID.*ProcessProposal"
Length of output: 36
Script:
#!/bin/bash
# Let's search for the ProcessHandler function implementation and check chainID usage
ast-grep --pattern 'func ProcessHandler[T any]($$$) func($$$) error {
$$$
}'
# Also search for any chainID usage in the file
rg -A 5 "chainID" server/v2/cometbft/handlers/defaults.go
Length of output: 1064
| @@ -219,3 +232,13 @@ func AppBuilderWithPostTxExec[T transaction.Tx]( | |||
| a.postTxExec = postTxExec | |||
| } | |||
| } | |||
|
|
|||
| func AppBuilderWithPreblocker[T transaction.Tx]( | |||
There was a problem hiding this comment.
We should precise as well that the module preblocker are ran before any logic there.
Which makes me think, do you think we should always run pre blocker before their logic?
What if we give them a callback to call preblocker themselves if needed.
Like
preblocker func(
ctx context.Context, txs []T, mmPreblocker func()
) error,And then above we do smth like
preblockerFn := func(ctx context.Context, txs []T, mmPreblocker func()) error {
if a.preblocker != nil {
return a.preblocker(ctx, txs, func() { if err := a.app.moduleManager.PreBlocker()(ctx, txs); err != nil {
panic(err)
} })
}
return .app.moduleManager.PreBlocker()(ctx, txs)
}Not sure if needed however.
There was a problem hiding this comment.
I like this, adding it
| } | ||
|
|
||
| res, err := c.app.Query(ctx, latestVersion, &types.QueryParamsRequest{}) | ||
| func GetConsensusParams[T transaction.Tx](ctx context.Context, app handlers.AppManager[T]) (*cmtproto.ConsensusParams, error) { |
There was a problem hiding this comment.
Why did this need to be refactored?
The use of c.GetConsensusParams is a lot clearer than the new syntax introduced
There was a problem hiding this comment.
I think because it is used in ValidateVoteExtensions and that is something you'd be calling in the app directly.
There was a problem hiding this comment.
Yeah, it's also a nice helper to have. I don't think it's much more complicated than what we had
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
runtime/v2/builder.go (1)
241-253: Enhance documentation for AppBuilderWithPreblocker.While the basic documentation is present, it would be beneficial to add:
- Example usage showing common patterns
- Error handling expectations
- Threading model and concurrency considerations
- Impact on block processing flow
Example documentation:
// AppBuilderWithPreblocker sets logic that will be executed before each block. // The preblocker function is called with the current context, pending transactions, // and a callback to execute the module manager's preblocker. // // The mmPreblocker callback can be used to call module manager's preblocker at any // point during custom preblocker execution, allowing flexible integration with // vote extensions and other pre-block processing requirements. // // Example usage: // // builder.Build( // AppBuilderWithPreblocker(func(ctx context.Context, txs []T, mmPreblocker func()) error { // // Custom pre-block processing // if err := processVoteExtensions(ctx); err != nil { // return err // } // // // Call module manager's preblocker // mmPreblocker() // // return nil // }), // ) // // Note: This function is executed synchronously during block processing. // Any errors returned will prevent block creation.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runtime/v2/builder.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
runtime/v2/builder.go (2)
33-33: Add documentation for the preblocker field.
The preblocker field needs documentation explaining its purpose, usage, and the significance of the mmPreblocker callback parameter.
120-120: LGTM!
The integration of preblockerFn with the STF constructor is clean and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
runtime/v2/builder.go (2)
99-108: Simplify preblockerFn implementation using early return pattern.The current implementation can be made more concise while maintaining the same logic.
Consider this refactoring:
preblockerFn := func(ctx context.Context, txs []T) error { - if a.preblocker != nil { - return a.preblocker(ctx, txs, func() error { - return a.app.moduleManager.PreBlocker()(ctx, txs) - }) - } - - // if there is no preblocker set, call the module manager's preblocker directly - return a.app.moduleManager.PreBlocker()(ctx, txs) + if a.preblocker == nil { + return a.app.moduleManager.PreBlocker()(ctx, txs) + } + return a.preblocker(ctx, txs, func() error { + return a.app.moduleManager.PreBlocker()(ctx, txs) + }) }
235-239: Enhance function documentation with examples and usage guidelines.While the current documentation provides basic information, it would be helpful to add:
- Example usage showing how to implement vote extensions
- Clear explanation of execution order and its impact
- Best practices for implementing the preblocker function
Example documentation:
// AppBuilderWithPreblocker sets logic that will be executed before each block. // The provided preblocker function receives a callback (mmPreblocker) that can be used // to execute the module manager's preblocker at any point during custom preprocessing. // // This is especially useful when implementing vote extensions. For example: // // app.Build( // AppBuilderWithPreblocker(func(ctx context.Context, txs []T, mmPreblocker func() error) error { // // Custom preprocessing logic // if err := processVoteExtensions(ctx); err != nil { // return err // } // // Execute module manager's preblocker // if err := mmPreblocker(); err != nil { // return err // } // // Additional preprocessing if needed // return nil // }), // )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runtime/v2/builder.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
runtime/v2/builder.go (2)
33-33: Add field documentation for preblocker.
Please add godoc for the preblocker field explaining its purpose, execution timing, and relationship with the module manager's preblocker.
114-114: LGTM!
The preblockerFn is correctly integrated into the stf.New call.
…r vote extensions (backport #22830) (#23006) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #XXXX
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...
!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
Documentation