feat(server/v2): introduce cometbft v2 #20483
Conversation
|
Warning Rate limit exceeded@kocubinski has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 10 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. WalkthroughThe recent changes introduce extensive functionalities related to the CometBFT consensus mechanism, including new server setup, configuration, gRPC handling, transaction processing, querying, and mempool management. Additionally, a new 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@kocubinski 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: 18
Outside diff range and nitpick comments (14)
server/v2/cometbft/types/errors/errors.go (1)
7-17: The error definitions inerrors.goare clear and use thecosmossdk.io/errorsmodule appropriately. Consider adding more detailed documentation forErrUnknownRequestto explain when this error should be expected.server/v2/cometbft/client/grpc/cmtservice/autocli.go (1)
8-50: Ensure consistency in command descriptions.The command descriptions in
CometBFTAutoCLIDescriptorvary significantly in detail and style. For better user experience and consistency, align the verbosity and style of these descriptions. For example, some commands have aLongdescription while others do not. Consider adding similar detail to all commands or simplifying them uniformly.store/v2/root/factory.go (1)
30-30: Add a comment explaining the purpose ofSSTypeMemory.It would be beneficial to add a comment explaining the purpose and usage of the
SSTypeMemoryconstant for better code readability and maintainability.server/v2/cometbft/query.go (1)
34-34: Clarify error message for better user understanding.The error message "expected second parameter to be 'filter'" could be more descriptive. Consider specifying what the valid 'filter' values are (e.g., 'addr' or 'id') to make the error message more helpful to the user.
server/v2/cometbft/snapshots.go (1)
39-39: Log additional context when the snapshot manager is not configured.When logging that the snapshot manager is not configured, it might be helpful to include suggestions or actions that can be taken to resolve this issue. This could guide the user on how to proceed and potentially reduce frustration.
server/v2/cometbft/server.go (3)
85-85: Clarify the TODO comment regarding the presence of the Store interface.The TODO comment on line 85 is vague. It would be beneficial to specify what needs to be determined about the
Storeinterface's presence to make the code easier to maintain and understand.
94-94: Specify how to set options in theNewManagercall.The TODO comment on line 94 suggests that options need to be set somehow. It would be helpful to either implement this or provide more detailed comments on what options are needed and how they might be configured.
Line range hint
267-267: Consider using a more descriptive error message.The error message on line 267 could be more descriptive. It currently uses a generic message that might not provide enough context for debugging.
- return nil, status.Error(codes.Internal, "ABCI Query handler undefined") + return nil, status.Error(codes.Internal, "ABCI Query handler is undefined. Ensure that the query handler is properly initialized before calling.")server/v2/cometbft/client/grpc/cmtservice/service.go (1)
293-293: Register gRPC services dynamically.Consider enhancing the flexibility of the
RegisterTendermintServicefunction by allowing it to accept additional parameters or configurations that might be needed for different deployments or testing scenarios.server/v2/cometbft/utils.go (2)
28-29: Consider adding documentation for thequeryResponsefunction.It would be beneficial to add a comment explaining the purpose and usage of this function for better maintainability.
109-127: Review the use of pointers inintoABCITxResults.Consider whether using pointers here is necessary. If the objects are not large or not modified, using values could reduce the overhead of memory allocation.
server/v2/cometbft/abci.go (3)
34-57: Ensure proper documentation for theConsensusstruct.Adding comments to explain the purpose of each field in the
Consensusstruct would enhance code readability and maintainability.
77-91: Review the setter method for snapshot manager.Consider if the snapshot manager should be settable post-construction of the
Consensusobject, as this could lead to runtime issues if changed mid-operation.
117-124: Clarify the purpose ofBlockData.Adding a detailed comment explaining the use of
BlockDatawould be beneficial for future maintainers.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (5)
server/v2/cometbft/client/grpc/cmtservice/query.pb.gois excluded by!**/*.pb.goserver/v2/cometbft/client/grpc/cmtservice/query.pb.gw.gois excluded by!**/*.pb.gw.goserver/v2/cometbft/client/grpc/cmtservice/types.pb.gois excluded by!**/*.pb.goserver/v2/cometbft/go.modis excluded by!**/*.modserver/v2/cometbft/go.sumis excluded by!**/*.sum
Files selected for processing (31)
- core/app/app.go (1 hunks)
- server/v2/cometbft/abci.go (1 hunks)
- server/v2/cometbft/client/grpc/cmtservice/autocli.go (1 hunks)
- server/v2/cometbft/client/grpc/cmtservice/service.go (1 hunks)
- server/v2/cometbft/client/grpc/cmtservice/types.go (1 hunks)
- server/v2/cometbft/client/grpc/cmtservice/util.go (1 hunks)
- server/v2/cometbft/client/rpc/block.go (1 hunks)
- server/v2/cometbft/client/rpc/client.go (1 hunks)
- server/v2/cometbft/client/rpc/utils.go (1 hunks)
- server/v2/cometbft/commands.go (1 hunks)
- server/v2/cometbft/config.go (1 hunks)
- server/v2/cometbft/flags/flags.go (1 hunks)
- server/v2/cometbft/handlers/defaults.go (1 hunks)
- server/v2/cometbft/handlers/handlers.go (1 hunks)
- server/v2/cometbft/handlers/tx_selector.go (1 hunks)
- server/v2/cometbft/log/logger.go (1 hunks)
- server/v2/cometbft/mempool/config.go (1 hunks)
- server/v2/cometbft/mempool/doc.go (1 hunks)
- server/v2/cometbft/mempool/mempool.go (1 hunks)
- server/v2/cometbft/mempool/noop.go (1 hunks)
- server/v2/cometbft/query.go (1 hunks)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/cometbft/service.go (1 hunks)
- server/v2/cometbft/snapshots.go (1 hunks)
- server/v2/cometbft/streaming.go (1 hunks)
- server/v2/cometbft/types/errors/errors.go (1 hunks)
- server/v2/cometbft/types/peer.go (1 hunks)
- server/v2/cometbft/types/store.go (1 hunks)
- server/v2/cometbft/types/vote.go (1 hunks)
- server/v2/cometbft/utils.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
Files skipped from review due to trivial changes (2)
- server/v2/cometbft/mempool/doc.go
- server/v2/cometbft/types/peer.go
Additional Context Used
Path-based Instructions (29)
server/v2/cometbft/types/vote.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mempool/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/types/errors/errors.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/log/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mempool/noop.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/types/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/grpc/cmtservice/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/grpc/cmtservice/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/rpc/client.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mempool/mempool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/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.core/app/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/rpc/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/grpc/cmtservice/autocli.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/handlers/tx_selector.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/flags/flags.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/rpc/block.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/handlers/defaults.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/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/client/grpc/cmtservice/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/utils.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 (22)
server/v2/cometbft/types/vote.go (1)
9-13: TheVoteExtensionsHandlerinterface is well-defined and adheres to Go best practices.server/v2/cometbft/mempool/config.go (1)
3-10: TheConfigstruct is clearly documented, explaining the behavior of theMaxTxsfield comprehensively.server/v2/cometbft/log/logger.go (1)
11-23: TheCometLoggerWrapperis correctly implemented to adhere to thecmtlog.Loggerinterface. TheWithmethod is properly using variadic parameters for key-value pairs, which is a good practice for logging contexts.server/v2/cometbft/mempool/noop.go (1)
11-22: TheNoOpMempoolis well-implemented for its intended use case of discarding transactions. This is particularly useful for testing or scenarios where transaction ordering is handled differently. The generic typeTis a good use of Go generics to ensure type safety.server/v2/cometbft/types/store.go (1)
9-32: TheStoreinterface is comprehensive, covering a wide range of functionalities necessary for state management in blockchain systems. The use of interfaces fromcosmossdk.io/core/storeandcosmossdk.io/store/v2ensures modularity and flexibility. Ensure that all methods are implemented correctly in concrete classes to avoid runtime issues.server/v2/cometbft/client/grpc/cmtservice/util.go (2)
9-27: TheconvertHeaderfunction is correctly implemented and adheres to the expected data mapping between CometBFT headers and SDK headers.
29-38: TheconvertBlockfunction is well-implemented, making effective use of theconvertHeaderfunction to ensure consistent header conversion. The straightforward mapping of other block components is clear and maintainable.server/v2/cometbft/client/grpc/cmtservice/types.go (2)
7-15: TheToABCIRequestQueryfunction correctly transforms a gRPCABCIQueryRequestto an ABCIQueryRequest, ensuring all necessary fields are accurately mapped.
18-46: TheFromABCIResponseQueryfunction is implemented correctly, handling the optionalProofOpsfield safely and ensuring all data is transformed accurately from ABCI to gRPC formats.server/v2/cometbft/client/rpc/client.go (1)
10-35: TheCometRPCinterface is comprehensive and well-defined, covering a wide range of functionalities necessary for interacting with a CometBFT node. The use of context and optional parameters in method signatures is consistent and appropriate.server/v2/cometbft/mempool/mempool.go (2)
15-28: TheMempoolinterface is well-designed, utilizing generics to enhance type safety and flexibility. The methods are clearly defined, covering essential functionalities for mempool management.
31-41: TheIteratorinterface is minimalistic and effectively designed for iterating over transactions in the mempool. It adheres to idiomatic Go practices and is easy to understand and implement.server/v2/cometbft/config.go (1)
10-37: TheConfigstruct is thoroughly and thoughtfully designed, covering a comprehensive range of configuration options for the CometBFT application. The use of struct tags for serialization and the inclusion of custom and external types are correctly implemented.server/v2/cometbft/handlers/handlers.go (4)
13-16: ThePrepareHandlerfunction type is well-defined, allowing for stateful preprocessing of transactions in a flexible and type-safe manner using generics.
18-20: TheProcessHandlerfunction type is appropriately designed for processing transactions, with clear error handling and a straightforward signature.
22-25: TheVerifyVoteExtensionhandlerfunction type is correctly designed for verifying vote extensions, using a context and a store reader map to access necessary state data.
27-30: TheExtendVoteHandlerfunction type is effectively designed for extending votes, utilizing a context and a store reader map to handle the request appropriately.server/v2/cometbft/streaming.go (1)
12-51: Optimize error handling instreamDeliverBlockChanges.
[REFACTOR_SUGGESTion]
The functionstreamDeliverBlockChangeslogs errors but continues execution, which might lead to inconsistent states or missed errors in a distributed system. Consider modifying the function to return an error immediately when an error is encountered, allowing the caller to handle it appropriately.- if err := streamingListener.ListenDeliverBlock(ctx, streaming.ListenDeliverBlockRequest{ + err := streamingListener.ListenDeliverBlock(ctx, streaming.ListenDeliverBlockRequest{ BlockHeight: height, Txs: txs, TxResults: streamingTxResults, Events: streaming.IntoStreamingEvents(events), }); - if err != nil { - c.logger.Error("ListenDeliverBlock listening hook failed", "height", height, "err", err) + return err }server/v2/cometbft/flags/flags.go (1)
78-78: EnsureFlagChainIDis appropriately marked as required where necessary.server/v2/cometbft/utils.go (2)
49-51: Check the error handling inresponseExecTxResultWithEvents.Verification successful
The search results indicate that the
errorsmod.ABCIInfofunction is used in several places, includingbaseapp/abci_test.go,types/errors/abci.go, andserver/v2/cometbft/utils.go. To verify the consistency of error handling inresponseExecTxResultWithEvents, we need to compare the implementation inserver/v2/cometbft/utils.gowith the other instances.Let's review the specific lines in
server/v2/cometbft/utils.goand compare them with the other instances to ensure consistency.
The error handling in
responseExecTxResultWithEventsis consistent with other instances oferrorsmod.ABCIInfousage across the codebase. The pattern of extractingspace,code, andlogfrom the error and using them in the response object is uniformly applied.
server/v2/cometbft/utils.go:space, code, log := errorsmod.ABCIInfo(err, debug)types/errors/abci.go:space, code, log := errorsmod.ABCIInfo(err, debug)baseapp/abci_test.go:space, code, _ := errorsmod.ABCIInfo(err, false)These instances show a consistent approach to error handling, ensuring that the error information is correctly parsed and utilized in the response objects.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling in `responseExecTxResultWithEvents` is consistent across the codebase. # Test: Search for similar error handling patterns. Expect: Consistent error handling. rg --type go 'errorsmod.ABCIInfo'Length of output: 703
Script:
#!/bin/bash # Extract the relevant lines from the files where `errorsmod.ABCIInfo` is used to compare error handling. # Extract lines from server/v2/cometbft/utils.go rg --type go --context 5 'errorsmod.ABCIInfo' server/v2/cometbft/utils.go # Extract lines from baseapp/abci_test.go rg --type go --context 5 'errorsmod.ABCIInfo' baseapp/abci_test.go # Extract lines from types/errors/abci.go rg --type go --context 5 'errorsmod.ABCIInfo' types/errors/abci.goLength of output: 3809
77-93: Ensure error handling is robust infinalizeBlockResponse.server/v2/cometbft/abci.go (1)
59-75: Validate the initialization logic inNewConsensus.Verification successful
The initialization logic in
NewConsensuscorrectly sets all fields and handles the necessary handlers. No issues were found in the usage context.
server/v2/cometbft/server.go: TheNewConsensusfunction is used to create aconsensusobject, and several handlers are set on this object immediately after its creation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the initialization logic in `NewConsensus` correctly sets all fields and handles any potential errors. # Test: Search for usage of `NewConsensus` to ensure it is used correctly. Expect: Proper initialization. rg --type go 'NewConsensus'Length of output: 571
Script:
#!/bin/bash # Extract lines around the usage of `NewConsensus` in `server/v2/cometbft/server.go` to review the context and initialization logic. rg --type go -A 10 -B 10 'NewConsensus' server/v2/cometbft/server.goLength of output: 804
| FlagOrderBy = "order_by" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
do we still need all these cc @julienrbrt
There was a problem hiding this comment.
We should only have the one we use here for sure. The rest seems duplicated.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/cometbft/server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
* main: feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481)
* main: (120 commits) chore: update protoc-gen-swagger to protoc-gen-openapiv2 (#20448) ci: Add GitHub Action for go mod tidy and mocks (#20501) chore: Address linter issues (#20486) fix: wrap errors in auto CLI service registration (#20493) chore: fix comment (#20498) chore: fix the note box syntax error (#20499) feat(server/v2): introduce cometbft v2 (#20483) refactor(x/upgrade): migrate to appmodule.VersionMap (#20485) docs: code guidelines changes (#20482) feat(cosmovisor): load cosmovisor configuration from toml file (#19995) perf(math): Significantly speedup Dec quo truncate and quo Roundup (#20034) fix: Bump CometBFT versions (#20481) refactor(core): remove redundant ExecMode (#20322) feat(store/v2): pruning manager (#20430) chore: force reload sonar cloud (#20480) refactor(x/accounts): reuse calculated sum in `func safeAdd` (#20458) refactor: remove `defer` in loop (#20223) ci: remove livness test (#20474) build(deps): Bump bufbuild/buf-setup-action from 1.32.1 to 1.32.2 (#20477) chore: migrate a few diagrams to mermaid (#20473) ...
Description
Part of server/v2 work and #20412, this PR introduces the server v2 module for CometBFT.
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
Enhancements
IsGenesisto indicate whether a block is a genesis block.