feat(consensus): add consensus message #19483
Conversation
WalkthroughWalkthroughThe changes introduce and integrate consensus messages within the cosmos-sdk framework, specifically targeting the CometBFT consensus algorithm. New functionalities include setting and testing consensus parameters, registering consensus messages, and converting custom consensus parameters to Tendermint consensus parameters. The update enhances the consensus module's capability to modify ABCI consensus parameters through state management, parameter storage, and message handling for consensus actions. Changes
Assessment against linked issues
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 (
|
tac0turtle
force-pushed
the
marko/19246
branch
from
8ece1f9
to
2007967
Compare
![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: 0
Configuration used: .coderabbit.yml
Files selected for processing (8)
- api/cosmos/consensus/v1/consensus.pulsar.go (1 hunks)
- proto/cosmos/consensus/v1/consensus.proto (1 hunks)
- x/consensus/keeper/keeper.go (1 hunks)
- x/consensus/keeper/keeper_test.go (1 hunks)
- x/consensus/module.go (1 hunks)
- x/consensus/types/consensus.go (1 hunks)
- x/consensus/types/consensus.pb.go (1 hunks)
- x/consensus/types/msgs.go (1 hunks)
Files not summarized due to errors (1)
- api/cosmos/consensus/v1/consensus.pulsar.go: Error: Message exceeds token limit
Additional comments: 25
proto/cosmos/consensus/v1/consensus.proto (2)
- 9-19: The protobuf message
ConMsgParamsis well-defined, correctly usingtendermint.typesfor consensus parameter types. This ensures compatibility with Tendermint's consensus parameters and facilitates clear communication of consensus-related configurations.- 21-23: The
ConMsgParamsResponsemessage is currently empty. This is acceptable as it may serve as a placeholder for future response data or to fulfill RPC method response requirements.x/consensus/types/msgs.go (2)
- 5-12: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The package declaration and imports are correctly defined, ensuring the necessary dependencies are available for the
ToProtoConsensusParamsfunction.
- 5-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-41]
The
ToProtoConsensusParamsfunction is well-implemented, correctly convertingMsgUpdateParamstocmtproto.ConsensusParams. It properly checks for nil parameters to prevent nil pointer dereferences and uses default values for the version field, ensuring robustness and compatibility.x/consensus/types/consensus.go (2)
- 1-1: The package declaration and imports are correctly defined, ensuring the necessary dependencies are available for the
ToProtoConsensusParamsfunction.- 3-41: The
ToProtoConsensusParamsfunction is well-implemented, correctly convertingConMsgParamstocmtproto.ConsensusParams. It properly checks for nil parameters, including theVersionfield, to prevent nil pointer dereferences and uses default values for the version field, ensuring robustness and compatibility. The additional handling of theVersion.Appfield is commendable for ensuring accurate version information.x/consensus/module.go (1)
- 80-84: The
RegisterConsensusMessagesfunction includes a commented-out call toRegisterConsensusHandler, indicating a placeholder for future integration. This is a common practice for indicating pending work or dependencies on external updates. It's important to track this for future implementation to ensure the consensus messages are correctly registered and handled.x/consensus/keeper/keeper.go (1)
- 89-104: The
SetParamsfunction is well-implemented, correctly handling the setting of consensus parameters during chain initialization. It properly convertsConMsgParamstocmtproto.ConsensusParams, validates them, and stores them in theParamsStore, ensuring robustness and adherence to best practices in Cosmos SDK development.x/consensus/keeper/keeper_test.go (1)
- 236-332: The
TestSetParamsfunction provides comprehensive testing for theSetParamsfunction in the consensus keeper. It correctly covers various scenarios, including valid and invalid parameter settings, ensuring robust testing. The use of table-driven tests is commendable, aligning with best practices in Go testing.x/consensus/types/consensus.pb.go (8)
- 26-36: The definition of
ConMsgParamsstruct is well-formed, correctly utilizing protobuf field annotations for serialization. It encapsulates various consensus parameters, aligning with the PR's objective to enhance consensus message handling.- 106-109: The
ConMsgParamsResponsestruct is defined but appears to be empty. While this might be intentional, indicating a response type without specific fields, it's important to ensure that this aligns with the intended use cases. If future extensions are anticipated, documenting the rationale for its current empty state could be beneficial for maintainers.Verification successful
The
ConMsgParamsResponsestruct is intentionally empty and is used within the codebase, particularly in the consensus mechanism, as indicated by its usage inconsensus.pulsar.goandkeeper.go. This pattern is common in protocol buffer definitions, where empty messages may serve as placeholders for future data or to indicate completion of an operation without transferring data. There's no direct evidence of documentation or comments regarding future extension plans in the provided output, but the usage pattern supports the struct's intentional design as it currently stands.* 177-185: The `Marshal` method for `ConMsgParams` is correctly implemented, following the standard pattern for protobuf message marshaling in Go. It ensures that the data is marshaled into a byte slice of the appropriate size. * 192-257: The `MarshalToSizedBuffer` method for `ConMsgParams` is correctly implemented. It efficiently marshals the message into a pre-allocated buffer in reverse order to avoid reallocations. This method is crucial for performance-sensitive applications. * 260-267: The `Marshal` method for `ConMsgParamsResponse` follows the same pattern as `ConMsgParams`. Given that `ConMsgParamsResponse` currently has no fields, this method effectively initializes a byte slice of size 0. This is correct but reinforces the need to verify the intended use of `ConMsgParamsResponse`. * 338-567: The `Unmarshal` method for `ConMsgParams` is correctly implemented, providing comprehensive error handling and validation for the wire format. It ensures that the message is correctly parsed from a byte slice, adhering to the protobuf encoding rules. * 568-616: The `Unmarshal` method for `ConMsgParamsResponse` is correctly implemented. Given the current structure of `ConMsgParamsResponse`, this method effectively performs no operation but is necessary for interface compliance and future extensibility. * 697-701: The error variables defined (`ErrInvalidLengthConsensus`, `ErrIntOverflowConsensus`, `ErrUnexpectedEndOfGroupConsensus`) are standard for protobuf-generated code, ensuring consistent error handling across marshaling and unmarshaling operations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if ConMsgParamsResponse is intended to be empty and check for future extension plans. grep -r "ConMsgParamsResponse" . --include=*.go | grep -v "consensus.pb.go"Length of output: 8628
api/cosmos/consensus/v1/consensus.pulsar.go (8)
- 4-14: The imports are correctly organized and include necessary packages for protobuf message handling, reflection, and I/O operations. No unused imports are detected.
- 16-22: The global variable declarations for message and field descriptors are correctly defined and follow the naming conventions for protobuf-generated Go code.
- 25-33: The
initfunction correctly initializes the message descriptors using theFile_cosmos_consensus_v1_consensus_proto.Messages().ByNamemethod. This is standard practice for protobuf-generated Go code.- 35-370: The
fastReflection_ConMsgParamsstruct and its methods are correctly implemented for efficient reflection-based operations onConMsgParamsmessages. This includes methods for message reflection, field manipulation, and proto message interface compliance.- 371-765: The
ProtoMethodsfunction forConMsgParamsprovides custom implementations for size calculation, marshaling, and unmarshaling, optimizing these operations. The implementation follows the expected patterns for protobuf-generated code, ensuring compatibility with the protobuf runtime.- 776-1121: Similarly, the
fastReflection_ConMsgParamsResponsestruct and its methods, including theProtoMethodsfunction, are correctly implemented for theConMsgParamsResponsemessage. These methods ensure efficient and correct handling of message operations, following the protobuf-generated code standards.- 1138-1207: The
ConMsgParamsmessage type is correctly defined with appropriate fields for version, block, evidence, validator, and abci parameters. Each field uses the correct type and protobuf field options. However, it's important to ensure that the referenced types (e.g.,types.VersionParams) are correctly defined elsewhere in the codebase.Verification successful
The executed shell scripts successfully located references to
VersionParams,BlockParams,EvidenceParams,ValidatorParams, andABCIParamsacross various files in the codebase. This indicates that these types are indeed defined and utilized in multiple locations, including theConMsgParamsstruct in question, test files, protobuf definitions, and other parts of the codebase. Given this evidence, it can be concluded that the referenced types are correctly defined and available within the codebase, as required by theConMsgParamsmessage type.* 1210-1235: The `ConMsgParamsResponse` message type is defined as an empty message, which is typical for response messages that do not need to carry data. This definition is correct given the context, but it's important to ensure that this aligns with the intended use cases for this message type.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the referenced types are correctly defined and available. fd --type f --exec grep -Hn "VersionParams" {} fd --type f --exec grep -Hn "BlockParams" {} fd --type f --exec grep -Hn "EvidenceParams" {} fd --type f --exec grep -Hn "ValidatorParams" {} fd --type f --exec grep -Hn "ABCIParams" {}Length of output: 21457
CHANGELOG.md
Outdated
| @@ -53,6 +53,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * (types) [#18768](https://github.com/cosmos/cosmos-sdk/pull/18768) Add MustValAddressFromBech32 function. | |||
| * (gRPC) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) Add debug log prints for each gRPC request. | |||
| * (server) [#19280](https://github.com/cosmos/cosmos-sdk/pull/19280) Adds in-place testnet CLI command. | |||
| * (x/consensus) [#19483](https://github.com/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module. | |||
There was a problem hiding this comment.
The URL in the PR reference appears to have a typo. It should point to the cosmos/cosmos-sdk repository, not cosmos-sdk. Correcting this will ensure the link is functional and users can navigate to the PR directly from the changelog.
- * (x/consensus) [#19483](https://github.com/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module.
+ * (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module.Committable 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.
| * (x/consensus) [#19483](https://github.com/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module. | |
| * (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| option go_package = "github.com/cosmos/cosmos-sdk/x/consensus/types"; | ||
|
|
||
| // ConMsgParams is the Msg/Params request type. This is a consensus message that is sent from cometbft. | ||
| message ConMsgParams { |
There was a problem hiding this comment.
nit: Is there a reason to abbreviate Consensus?
There was a problem hiding this comment.
smaller footprint since larger names impact overall size
|
can you please fix the failing lints too ? |
Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/consensus/keeper/keeper_test.go (7 hunks)
- x/consensus/types/msgs.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/consensus/keeper/keeper_test.go
- x/consensus/types/msgs.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/consensus/keeper/keeper_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/consensus/keeper/keeper_test.go
There was a problem hiding this comment.
That would be super useful to add some docs that explain the difference beet consensus MsgUpdateParams and that new ConsensusParams.
The only difference in the message is that one contains an authority and the other one a version, so adding a rationale would help.
Additionally, as this message as quite difference than the rest (no handler, or msg server), additional godoc there would be helpful.
added, lmk if you thnk anything else is missing |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: .coderabbit.yml
Files selected for processing (6)
- api/cosmos/consensus/v1/consensus.pulsar.go (1 hunks)
- proto/cosmos/consensus/v1/consensus.proto (1 hunks)
- x/consensus/README.md (1 hunks)
- x/consensus/keeper/keeper.go (1 hunks)
- x/consensus/types/consensus.go (1 hunks)
- x/consensus/types/consensus.pb.go (1 hunks)
Files not summarized due to errors (1)
- api/cosmos/consensus/v1/consensus.pulsar.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (3)
- proto/cosmos/consensus/v1/consensus.proto
- x/consensus/keeper/keeper.go
- x/consensus/types/consensus.go
Additional comments: 18
x/consensus/types/consensus.pb.go (6)
- 26-36: The definition of
ConsensusMsgParamsstruct is well-formed and follows the protobuf specification. It correctly defines fields for various consensus parameters, ensuring that they are optional withomitemptyJSON tags. This structure allows for flexibility in specifying only the necessary parameters for a consensus message.- 108-109: The
ConsensusMsgParamsResponsestruct is defined but appears to be empty. While this might be intentional, indicating a response that doesn't require data, it's important to ensure that this aligns with the design intentions of the consensus message handling. If future extensions are anticipated, documenting the expected use or extension points could be beneficial for maintainability.- 177-185: The
Marshalmethod forConsensusMsgParamsis correctly implemented, following the standard pattern for protobuf-generated marshal methods in Go. It calculates the size of the message, allocates a byte slice of that size, and then marshals the message into the byte slice. This is a standard and efficient approach for marshaling protobuf messages in Go.- 192-257: The
MarshalToSizedBuffermethod forConsensusMsgParamsis correctly implemented and follows the protobuf pattern for marshaling messages into a pre-allocated buffer in reverse order. This method is particularly important for performance in scenarios where marshaling needs to be done into a buffer of known size to avoid additional allocations. The method correctly handles optional fields by checking fornilbefore attempting to marshal them.- 338-567: The
Unmarshalmethod forConsensusMsgParamsis correctly implemented, following the standard pattern for protobuf-generated unmarshal methods in Go. It correctly handles the parsing of fields based on wire type and field number, with appropriate error handling for unexpected wire types, illegal tags, and integer overflow. The method also ensures that fields are unmarshaled only if they are notnil, which is crucial for correctly handling optional fields in protobuf messages.- 619-695: The
skipConsensusfunction is a utility function used to skip over unknown fields when unmarshaling a protobuf message. This function is crucial for forward compatibility, allowing messages to be extended with new fields without breaking existing code that unmarshals those messages. The implementation correctly handles various wire types and properly calculates the number of bytes to skip. This function is a good example of defensive programming in the context of protocol buffers.api/cosmos/consensus/v1/consensus.pulsar.go (12)
- 4-14: The import of
syncpackage on line 13 appears to be unused in the provided code segment. However, since this is generated code, manual removal or modification is not recommended unless it's confirmed that the generation template is corrected to avoid such issues in future generations.- 25-33: Initialization function
initcorrectly initializes message descriptors and field descriptors forConsensusMsgParams. This is a standard pattern in protobuf-generated Go code to ensure that descriptors are correctly set up for use by reflection and other runtime operations.- 39-53: The method
ProtoReflectand its slow counterpartslowProtoReflectare correctly implemented to provide reflection capabilities forConsensusMsgParams. This is essential for various operations that require runtime information about protobuf messages, such as serialization and deserialization.- 99-130: The
Rangemethod implementation forConsensusMsgParamscorrectly iterates over all populated fields and invokes the provided functionffor each field. This method is crucial for operations that need to inspect or manipulate fields of a message dynamically.- 143-161: The
Hasmethod implementation forConsensusMsgParamscorrectly reports whether a field is populated. This method is particularly important for distinguishing between unset fields and fields set to their default values, especially in the context of proto3 where fields have default values.- 189-218: The
Getmethod implementation forConsensusMsgParamscorrectly retrieves the value for a field, providing default values for unpopulated scalar fields and empty, read-only views for unpopulated composite types. This is a standard and expected behavior in protobuf-generated code.- 220-248: The
Setmethod implementation forConsensusMsgParamscorrectly stores the value for a field, with special handling for oneof fields and extension fields. This method is essential for modifying protobuf messages in a type-safe manner.- 771-774: Initialization function
initforConsensusMsgParamsResponsecorrectly initializes the message descriptor. This pattern ensures that the descriptor is correctly set up for use by reflection and other runtime operations for the response type.- 780-794: The method
ProtoReflectand its slow counterpartslowProtoReflectforConsensusMsgParamsResponseare correctly implemented, similar toConsensusMsgParams. This consistency in implementation across message types is good for maintainability and understanding of the codebase.- 841-841: The
Rangemethod forConsensusMsgParamsResponseis implemented but does not iterate over fields sinceConsensusMsgParamsResponsedoes not contain any fields. This is an expected behavior for message types that do not have fields but still need to conform to theprotoreflect.Messageinterface.- 855-861: The
Hasmethod forConsensusMsgParamsResponsecorrectly handles the case where there are no fields to check for population. This is an expected implementation detail for message types without fields, ensuring compatibility with theprotoreflect.Messageinterface.- 881-894: The
Getmethod forConsensusMsgParamsResponsecorrectly handles the case where there are no fields to retrieve. This implementation is consistent with the expected behavior for message types without fields, ensuring that the method can be called without causing runtime errors.
x/consensus/README.md
Outdated
| The bank module stores it's params in state with the prefix of `0x05`, | ||
| it can be updated with governance or the address with authority. |
There was a problem hiding this comment.
There's a minor inconsistency in the documentation under the Params section. It mentions the "bank module" instead of the "consensus module," which could confuse readers.
- The bank module stores it's params in state with the prefix of `0x05`,
+ The consensus module stores its params in state with the prefix of `0x05`,Committable 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.
| The bank module stores it's params in state with the prefix of `0x05`, | |
| it can be updated with governance or the address with authority. | |
| The consensus module stores its params in state with the prefix of `0x05`, | |
| it can be updated with governance or the address with authority. |
x/consensus/README.md
Outdated
|
|
||
| ## Params | ||
|
|
||
| The bank module stores it's params in state with the prefix of `0x05`, |
| ## Abstract | ||
|
|
There was a problem hiding this comment.
The abstract succinctly introduces the functionality of modifying CometBFT's ABCI consensus parameters. However, it could be enhanced by briefly explaining the significance of this capability in the broader context of blockchain consensus mechanisms.
Consider expanding the abstract to briefly explain why modifying ABCI consensus parameters is crucial for blockchain networks using CometBFT, highlighting its impact on network flexibility, security, and performance.
| ## State | ||
|
|
||
| The `x/consensus` module keeps state of the consensus params from cometbft.: |
There was a problem hiding this comment.
The State section provides a clear but brief description of the module's state management concerning consensus parameters. It would be beneficial to include more detail on how these parameters influence CometBFT's behavior and the overall consensus process.
Consider adding a sentence or two explaining the implications of consensus parameter state changes on CometBFT's operation, such as how they can affect block validation, transaction throughput, or network security.
|
|
||
| ## Keepers | ||
|
|
||
| The consensus module provides methods to Set and Get consensus params. It is recommended to use the `x/consensus` module keeper to get consensus params instead of accessing them through the context. |
There was a problem hiding this comment.
The Keepers section mentions the provision of methods to set and get consensus parameters but lacks an example. Including a code snippet would significantly enhance comprehension and usability for developers.
Consider adding an example code snippet showing how to use the x/consensus module keeper for setting or getting consensus parameters, demonstrating the practical application of these methods.
| The message will fail under the following conditions: | ||
|
|
||
| * The signer is not the set authority | ||
| * Not all values are set | ||
|
|
There was a problem hiding this comment.
The Messages section outlines the conditions under which the UpdateParams message will fail but does not specify what values can be set or their significance. Providing this information would greatly aid understanding and usability.
Consider adding details about the specific consensus parameters that can be updated with the UpdateParams message, including their significance and potential impact on the network's operation.
|
|
||
| ## Consensus Messages | ||
|
|
||
| The consensus module has a consensus message that is used to set the consensus params when the chain initializes. It is similar to the `UpdateParams` message but it is only used once at the start of the chain. |
There was a problem hiding this comment.
The Consensus Messages section introduces a crucial functionality for chain initialization but lacks detail on how this differs from regular parameter updates post-initialization. Clarifying this distinction could provide valuable context for developers.
Consider adding a brief explanation of how consensus messages during chain initialization differ from regular consensus parameter updates, highlighting why this distinction is important for the initial setup of a blockchain network.
| The consensus module emits the following events: | ||
|
|
||
| ### Message Events | ||
|
|
||
| #### MsgUpdateParams | ||
|
|
||
| | Type | Attribute Key | Attribute Value | | ||
| |--------|---------------|---------------------| | ||
| | string | authority | msg.Signer | | ||
| | string | parameters | consensus Parmeters | |
There was a problem hiding this comment.
The Events section, particularly the Message Events subsection, is well-documented. However, including examples of how these events can be subscribed to or used within the Cosmos SDK framework would enhance practical understanding.
Consider adding examples or guidance on subscribing to or handling these events within the Cosmos SDK framework, providing developers with actionable insights into leveraging these events for custom logic or monitoring.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/consensus/keeper/keeper_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/consensus/keeper/keeper_test.go
Description
Closes: #19246
this pr adds consensus message to consensus for chain init
this pr does not plug it into module.go because im not sure how that api will look
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.
I have...
Summary by CodeRabbit
New Features
Improvements
Tests
Refactor