refactor(server/v2/cometbft): add codec.Codec and clean-up APIs
#22566
Conversation
julienrbrt
requested review from
kocubinski,
a team,
facundomedica and
sontrinh16
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant refactoring of the Changes
Assessment against linked 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
|
github-actions
bot
added
C:server/v2
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
server/v2/cometbft/options.go (1)
50-52: Consider more robust default initializationThe current initialization with empty values might lead to runtime issues if not properly overridden.
Consider this approach:
-StreamingManager: streaming.Manager{}, +StreamingManager: streaming.NewManager(), // if available -SnapshotExtensions: []snapshots.ExtensionSnapshotter{}, +SnapshotExtensions: make([]snapshots.ExtensionSnapshotter, 0),server/v2/cometbft/query.go (1)
Line range hint
14-33: Consider enhancing error messages for better debuggingThe implementation looks good with proper validation and error handling. However, the error messages could be more specific to help with debugging.
Consider including the actual path length in the error message:
- return nil, errorsmod.Wrap(cometerrors.ErrUnknownRequest, "path should be p2p filter <addr|id> <parameter>") + return nil, errorsmod.Wrapf(cometerrors.ErrUnknownRequest, "path should be p2p filter <addr|id> <parameter>; got %d elements", len(path))server/v2/cometbft/streaming.go (3)
Line range hint
22-42: Consider extracting transaction result conversion logicThe transaction result conversion logic could be moved to a separate helper function to improve readability and maintainability.
+func convertToStreamingTxResults(txResults []server.TxResult, cfg Config) ([]*streaming.ExecTxResult, error) { + streamingTxResults := make([]*streaming.ExecTxResult, len(txResults)) + for i, txResult := range txResults { + space, code, log := errorsmod.ABCIInfo(txResult.Error, cfg.AppTomlConfig.Trace) + + events, err := streaming.IntoStreamingEvents(txResult.Events) + if err != nil { + return nil, err + } + + streamingTxResults[i] = &streaming.ExecTxResult{ + Code: code, + Codespace: space, + Log: log, + GasWanted: uint64ToInt64(txResult.GasWanted), + GasUsed: uint64ToInt64(txResult.GasUsed), + Events: events, + } + } + return streamingTxResults, nil +}
Line range hint
43-58: Optimize events conversion by moving it outside the loopThe events conversion is currently performed for each listener, which is inefficient. Consider moving it outside the loop.
+ events, err := streaming.IntoStreamingEvents(events) + if err != nil { + return err + } for _, streamingListener := range c.streamingManager.Listeners { - events, err := streaming.IntoStreamingEvents(events) - if err != nil { - return err - } if err := streamingListener.ListenDeliverBlock(ctx, streaming.ListenDeliverBlockRequest{
Line range hint
59-104: Multiple TODOs referencing issue #22009 need implementationThere are several TODOs related to header and JSON handling that need to be addressed. These are currently tracked in issue #22009.
Would you like me to help create implementation proposals for these TODOs or create subtasks in the issue tracker?
server/v2/cometbft/snapshots.go (1)
Line range hint
68-91: Consider pre-allocating the response snapshots sliceWhile the implementation is correct, consider pre-allocating the slice to avoid potential reallocations:
- resp := &abci.ListSnapshotsResponse{} + resp := &abci.ListSnapshotsResponse{ + Snapshots: make([]*abci.Snapshot, 0, len(snapshots)), + }server/v2/cometbft/utils.go (2)
Line range hint
271-304: Add documentation for height validation casesWhile the implementation is correct, consider adding documentation to explain the different height validation scenarios:
- First block with initial height > 1
- First block with initial height = 1
- Subsequent blocks
This will help future maintainers understand the logic more easily.
305-323: Improve error handling in type assertionThe type assertion error handling could be enhanced to provide more context about the failure.
Consider this improvement:
- if r, ok := res.(*types.QueryParamsResponse); !ok { - return nil, errors.New("failed to query consensus params") + r, ok := res.(*types.QueryParamsResponse) + if !ok { + return nil, fmt.Errorf("unexpected response type %T, expected *types.QueryParamsResponse", res)server/v2/cometbft/grpc.go (2)
35-41: Consider grouping function parameters into a structThe
gRPCServiceRegistrarfunction has multiple parameters, which may affect readability. Consider grouping related parameters into a configuration struct to simplify the function signature and improve maintainability.
221-223: Wrap consensus info error with gRPC statusWhen
s.consensus.Inforeturns an error, it's advisable to convert it into a gRPC status error for consistency in error handling.Apply this diff to wrap the error:
if err != nil { - return nil, err + return nil, status.Errorf(codes.Internal, "failed to get consensus info: %v", err) }server/v2/cometbft/server.go (1)
372-374: Add proper doc comment for exported functionGRPCServiceRegistrarThe comment for the exported function
GRPCServiceRegistrarshould start with the function name to comply with Go conventions.Apply this diff to correct the documentation comment:
- // gRPCServiceRegistrar returns a function that registers the CometBFT gRPC service + // GRPCServiceRegistrar returns a function that registers the CometBFT gRPC service
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (11)
server/v2/cometbft/abci.go(14 hunks)server/v2/cometbft/abci_test.go(3 hunks)server/v2/cometbft/commands.go(1 hunks)server/v2/cometbft/grpc.go(7 hunks)server/v2/cometbft/options.go(3 hunks)server/v2/cometbft/query.go(3 hunks)server/v2/cometbft/server.go(5 hunks)server/v2/cometbft/snapshots.go(4 hunks)server/v2/cometbft/streaming.go(2 hunks)server/v2/cometbft/utils.go(4 hunks)simapp/v2/simdv2/cmd/commands.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/options.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/snapshots.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/streaming.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.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (22)
server/v2/cometbft/options.go (3)
11-12: LGTM: Clean import additions
The new imports for streaming and snapshots are properly organized and align with the new functionality being added.
27-28: LGTM: Improved documentation
The added comment clarifies the purpose of the Mempool field, enhancing code readability.
29-34: Consider adding validation for StreamingManager and SnapshotExtensions
While the new fields are well-documented, consider adding validation in the server initialization to ensure these components are properly configured, especially since they're critical for consensus operations.
For example, you might want to:
- Validate that StreamingManager is properly initialized before server start
- Ensure SnapshotExtensions implementations are compatible with your snapshot strategy
server/v2/cometbft/query.go (1)
Line range hint 86-134: Address TODO comment and verify proof handling
The implementation looks good overall, but there are two points to consider:
- There's a TODO comment about fastpath optimization that should be addressed
- The proof handling creates a new ProofOps struct with a single operation, which might not be the intended behavior if multiple proof operations are expected
Would you like me to help investigate the fastpath optimization mentioned in the TODO comment?
server/v2/cometbft/streaming.go (1)
Line range hint 15-21: LGTM! Verify impact of receiver type change.
The method signature follows Go best practices. The change from Consensus[T] to consensus[T] aligns with the PR objectives to reduce type exposure.
server/v2/cometbft/snapshots.go (3)
Line range hint 37-65: LGTM! Well-structured error handling and logging
The implementation properly handles different error cases with appropriate ABCI responses and includes contextual error logging.
Line range hint 94-112: LGTM! Comprehensive error handling with contextual logging
The implementation includes proper nil checks and detailed error logging with relevant context.
Line range hint 115-164: Review and update the IAVL stores comment
The implementation is solid, but there's a comment that might need review:
// We currently don't support resetting the IAVL stores and retrying a
// different snapshot, so we ask CometBFT to abort all snapshot restoration.Please verify if this limitation still applies or if it should be updated to reflect current capabilities.
simapp/v2/simdv2/cmd/commands.go (3)
43-43: LGTM: Minor documentation improvement
The comment wording enhancement provides slightly better clarity.
109-114: LGTM: Proper codec integration
The addition of simApp.AppCodec() aligns with the PR objective of implementing codec.Codec for proper request parsing. The parameter ordering is correct, with the codec injected before the global config.
133-136: LGTM: Improved API structure
The simplified path to GRPCServiceRegistrar reduces API surface area and improves maintainability while preserving functionality. This change aligns well with the PR's objective of cleaning up APIs.
server/v2/cometbft/utils.go (3)
22-22: LGTM: Import addition is appropriate
The addition of the consensus types import is necessary for the new query parameter types.
Line range hint 324-378: LGTM: Well-structured block retention logic
The implementation effectively handles all edge cases and includes clear documentation explaining the retention height calculation process.
Line range hint 379-394: LGTM: Clear halt condition implementation
The halt condition checking is well-implemented with informative error messages.
server/v2/cometbft/commands.go (1)
391-394: LGTM: API simplification aligns with refactoring goals
The change simplifies store access by removing the unnecessary Consensus layer while maintaining the same functionality and error handling. This aligns well with the PR's objective of cleaning up APIs and reducing exposure of internal types.
server/v2/cometbft/abci_test.go (3)
9-9: LGTM: Import addition for sync package
The sync package import is correctly added to support the new OnceValues functionality used in consensus initialization.
Line range hint 641-713: Verify test coverage for new consensus initialization
The setUpConsensus function has been updated to use the new consensus type and initialization pattern. While the changes look correct, consider adding specific test cases to verify:
- The behavior of
getProtoRegistrywith the new sync.OnceValues implementation - Edge cases around registry initialization
Consider extracting the consensus configuration into a separate test helper function to improve test maintainability:
func getTestConsensusConfig() Config {
return Config{
AppTomlConfig: DefaultAppTomlConfig(),
}
}Line range hint 641-713: Add test cases for proto registry initialization
While the test suite provides good coverage for consensus operations, consider adding specific test cases for:
- Proto registry initialization behavior
- Concurrent access patterns to the registry
- Error cases in registry initialization
Example test case structure:
func TestConsensus_ProtoRegistry(t *testing.T) {
c := setUpConsensus(t, 100_000, mempool.NoOpMempool[mock.Tx]{})
// Test concurrent access
var wg sync.WaitGroup
for i := 0; i < 10; i++ {
wg.Add(1)
go func() {
defer wg.Done()
registry := c.getProtoRegistry()
require.NotNil(t, registry)
}()
}
wg.Wait()
}server/v2/cometbft/grpc.go (3)
28-30: LGTM: Definition of appSimulator interface
The appSimulator interface is correctly defined and enhances abstraction.
99-100: LGTM: Updates to txServer struct
Adding txCodec and app fields to the txServer struct aligns with the new architecture and is implemented correctly.
200-202: LGTM: Updates to nodeServer struct
The addition of cfg, cometBFTAppConfig, and consensus fields to the nodeServer struct is appropriate and fits the refactoring objectives.
server/v2/cometbft/server.go (1)
55-55: Verify that consensus[T] implements abci.Application interface
The assignment of &consensus[T]{...} to srv.Consensus requires that consensus[T] implements the abci.Application interface. Please ensure that consensus[T] fully implements all required methods for seamless integration.
Also applies to: 167-192
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
server/v2/cometbft/abci.go (3)
Line range hint
292-375: Consider adding debug logs for state initialization stepsWhile the implementation is solid, adding debug logs for key initialization steps would help with troubleshooting.
func (c *consensus[T]) InitChain(ctx context.Context, req *abciproto.InitChainRequest) (*abciproto.InitChainResponse, error) { c.logger.Info("InitChain", "initialHeight", req.InitialHeight, "chainID", req.ChainId) + c.logger.Debug("Starting genesis state initialization") // store chainID to be used later on in execution c.chainID = req.ChainId
Line range hint
446-535: Consider batching mempool removalsThe current implementation removes transactions from the mempool one by one. Consider batching these operations for better performance.
-// remove txs from the mempool -for _, tx := range decodedTxs { - if err = c.mempool.Remove(tx); err != nil { - return nil, fmt.Errorf("unable to remove tx: %w", err) - } -} +// remove txs from the mempool in batch +if err = c.mempool.RemoveBatch(decodedTxs); err != nil { + return nil, fmt.Errorf("unable to remove txs: %w", err) +}
Line range hint
554-635: Consider consolidating vote extension enablement checksThe duplicate logic for checking vote extension enablement could be extracted into a helper method for better maintainability.
+func (c *consensus[T]) isVoteExtensionEnabled(cp *consensustypes.MsgUpdateParams, height int64) bool { + if cp.Feature.VoteExtensionsEnableHeight != nil && + height >= cp.Feature.VoteExtensionsEnableHeight.Value && + cp.Feature.VoteExtensionsEnableHeight.Value != 0 { + return true + } + return cp.Abci != nil && + height >= cp.Abci.VoteExtensionsEnableHeight && + cp.Abci.VoteExtensionsEnableHeight != 0 +}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(15 hunks)server/v2/cometbft/grpc.go(7 hunks)server/v2/cometbft/query.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/query.go
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
server/v2/cometbft/grpc.go (4)
27-29: LGTM: Well-defined generic interface
The appSimulator[T] interface is well-designed with clear generic constraints and a focused single responsibility for transaction simulation.
34-47: LGTM: Clean dependency injection
The refactored function follows good design principles by:
- Accepting specific dependencies rather than a large consensus object
- Using the new codec interface for transaction handling
- Properly registering all required gRPC services
97-99: LGTM: Clean separation of concerns
The txServer struct now properly separates transaction coding and simulation responsibilities.
Line range hint 219-231: LGTM: Proper error handling in Status method
The Status method correctly uses the ABCI Application interface and properly handles potential errors from the Info call.
server/v2/cometbft/abci.go (3)
49-66: LGTM! Good encapsulation of the consensus type
The change to make consensus unexported and the addition of appCodec improve encapsulation and codec handling respectively.
Line range hint 201-291: LGTM! Error handling improvements
The error wrapping format has been improved as per previous review suggestions.
Line range hint 376-445: LGTM! Robust implementation with proper validations
The implementations include proper height validation and clear error handling.
There was a problem hiding this comment.
Copilot reviewed 5 out of 11 changed files in this pull request and generated no suggestions.
Files not reviewed (6)
- server/v2/cometbft/server.go: Evaluated as low risk
- server/v2/cometbft/query.go: Evaluated as low risk
- server/v2/cometbft/streaming.go: Evaluated as low risk
- server/v2/cometbft/snapshots.go: Evaluated as low risk
- server/v2/cometbft/commands.go: Evaluated as low risk
- server/v2/cometbft/abci.go: Evaluated as low risk
…ckport #22566) (#22575) Co-authored-by: Julien Robert <[email protected]>
* main: build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580) fix(server/v2/api/telemetry): enable global metrics (#22571) refactor(server/v2/cometbft): add `codec.Codec` and clean-up APIs (#22566) feat(core/coretesting): make memDB satisfy db.Db interface (#22570) Merge commit from fork fix(server(/v2)): fix fallback genesis path (#22564) fix: only overwrite context chainID when necessary (#22568) docs(client): Update setFeeGranter and setFeePayer comments (#22526) fix(baseapp): populate header info in `NewUncachedContext` (#22557) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.35.1-20240701160653-fedbb9acfd2f.1 to 1.35.2-20240701160653-fedbb9acfd2f.1 in /api (#22551) build(deps): Bump github.com/creachadair/atomicfile from 0.3.5 to 0.3.6 in /tools/confix (#22552) docs: Update reference of Approximation (#22550) fix(server/v2/comebft): wire missing services + fix simulation (#21964) ci: fix permissions for GITHUB_TOKEN on dependabot workflows (#22547) ci: fix permissions for GITHUB_TOKEN in spell check workflow (#22545) build(deps): Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 (#22537) fix(cosmovisor): premature upgrade on restart (#22528) fix(store/v2/pebble): handle version 0 in keys (#22524) refactor(server/v2/telemetry): swap redirects (#22520) docs: Update content in CODE_OF_CONDUCT.md (#22518)
Description
Closes: #21105
Use codec.Codec as issue described.
Clean-up APIs by leaking fewer types and APIs.
The comet server now exposes the cometbft.Application instead of the internal consensus type.
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
Refactor
Consensustoconsensusfor clarity across multiple components.Documentation