refactor: remove consensus messages #21248
Conversation
|
Warning Rate limit exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 31 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes across the codebase introduce several enhancements focused on improving code readability, documentation, and the initialization of consensus parameters. Key modifications include refined struct formatting, new context keys for better management of initialization data, and updates to the handling of consensus parameters in various methods. These adjustments aim to enhance clarity and maintainability while also addressing the retrieval of consensus parameters in the CometBFT ABCI Info call. 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 Configuration File (
|
github-actions
bot
added
C:x/consensus
C:server/v2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (6)
x/consensus/module.go (1)
52-55: Implement theDefaultGenesismethod.The
DefaultGenesismethod inx/consensus/module.gocurrently returnsnil, while other modules implement it to provide a meaningful default genesis state. Implementing this method will ensure consistency and potentially avoid issues during application initialization. Consider what default state should be returned for the consensus module.Analysis chain
Implement
DefaultGenesismethod.The
DefaultGenesismethod currently returnsnil. Consider implementing a default state if necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and necessity of `DefaultGenesis`. # Test: Search for `DefaultGenesis` usage in the codebase. rg 'DefaultGenesis' --type goLength of output: 16644
x/consensus/keeper/keeper.go (2)
47-47: Replacefmt.Printlnwith structured logging.Using
fmt.Printlnfor logging is not recommended in production code. Consider using a structured logger to provide better context and control over log output.k.Logger(ctx).Info("Initializing genesis state for consensus")
99-101: Enhance error messages for better debugging.Consider providing more context in error messages to aid in debugging. For example, include the parameter values being set when an error occurs.
if err := k.ParamsStore.Set(ctx, nextParams.ToProto()); err != nil { return nil, fmt.Errorf("failed to set consensus parameters: %w", err) }server/v2/cometbft/utils.go (1)
306-306: Improve logging message clarity.The log message
"err2"is not descriptive and does not provide context about the error. Consider using a more informative message.- fmt.Println("err2") + fmt.Println("failed to assert type for consensus params response")server/v2/stf/stf.go (2)
331-333: Add context to the preBlock comment.The comment for
preBlockcould be enhanced by explaining what "pre block logic" entails.- // preBlock executes the pre block logic. + // preBlock executes logic that prepares the state before the block is processed, such as initializing state variables.
351-353: Add context to the beginBlock comment.The comment for
beginBlockcould be more descriptive about what the "begin block logic" involves.- // beginBlock executes the begin block logic. + // beginBlock executes logic at the start of block processing, such as updating timestamps or setting initial conditions.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 367d94a and 5f33a78b6da3ab7332e6d15213d64497ad8e1dd8.
Files ignored due to path filters (2)
server/v2/appmanager/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sum
Files selected for processing (13)
- core/app/app.go (1 hunks)
- core/context/context.go (1 hunks)
- server/v2/appmanager/appmanager.go (3 hunks)
- server/v2/appmanager/go.mod (1 hunks)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/cometbft/config.go (1 hunks)
- server/v2/cometbft/utils.go (1 hunks)
- server/v2/go.mod (7 hunks)
- server/v2/stf/stf.go (5 hunks)
- simapp/v2/app_config.go (1 hunks)
- x/consensus/keeper/keeper.go (4 hunks)
- x/consensus/module.go (4 hunks)
- x/consensus/proto/cosmos/consensus/v1/query.proto (1 hunks)
Files skipped from review due to trivial changes (4)
- core/app/app.go
- server/v2/appmanager/appmanager.go
- server/v2/cometbft/config.go
- x/consensus/proto/cosmos/consensus/v1/query.proto
Additional context used
Path-based instructions (7)
core/context/context.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_config.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/stf/stf.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.
Learnings (1)
x/consensus/keeper/keeper.go (1)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction. - The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module. - The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
Additional comments not posted (16)
server/v2/appmanager/go.mod (3)
3-3: Update Go version to 1.22.2.The Go version has been updated from 1.21 to 1.22.2. Ensure that your environment supports this version and check for any compatibility issues with existing code.
12-12: Updategolang.org/x/expdependency.The
golang.org/x/expdependency has been updated to a newer commit. Ensure that any experimental features used are stable and compatible.
7-7: Updatecosmossdk.io/coredependency.The
cosmossdk.io/coredependency has been updated to a newer version. Verify that this update does not introduce breaking changes.core/context/context.go (2)
6-6: AddinitInfoKeyfor consensus params.The
initInfoKeystruct has been added for initialization-related data. Ensure this key is used correctly throughout the codebase.
11-16: Enhance documentation for context keys.The comments for
ExecModeKey,CometInfoKey, andInitInfoKeyimprove code readability and maintainability by clarifying their purposes.x/consensus/module.go (5)
5-5: Addencoding/jsonimport.The addition of
encoding/jsonis necessary for handling JSON data in the new genesis methods.
29-29: ImplementHasGenesisinterface.The
AppModulenow implements theHasGenesisinterface, enabling genesis state operations. Ensure all methods are correctly implemented.
47-50: ImplementInitGenesismethod.The
InitGenesismethod initializes the module's state. Ensure it correctly interacts with thekeeper.
57-60: ImplementValidateGenesismethod.The
ValidateGenesismethod is a noop. Ensure validation logic is added if needed.Verification successful
Consider implementing validation logic in
ValidateGenesisif necessary.The
ValidateGenesismethod inx/consensus/module.gois currently a no-op and does not seem to impact other parts of the codebase. However, implementing validation logic might be beneficial if the consensus module has specific requirements for its genesis state. Review the module's requirements to determine if validation is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity of validation logic in `ValidateGenesis`. # Test: Search for references to `ValidateGenesis`. rg 'ValidateGenesis' --type goLength of output: 15197
62-65: ImplementExportGenesismethod.The
ExportGenesismethod currently returnsnil. Review if exporting logic is required.server/v2/go.mod (1)
Line range hint
3-109: LGTM! Dependency updates are appropriate.The updates to the Go version and dependencies are appropriate and align with routine maintenance practices.
simapp/v2/app_config.go (1)
142-142: LGTM! Consensus module initialization is correctly added.Adding
consensustypes.ModuleNameto theInitGenesislist ensures proper initialization of the consensus module.server/v2/stf/stf.go (1)
159-164: Verify removal of consensus message handling.The
DeliverBlockfunction no longer handles consensus messages. Ensure that this logic is appropriately managed elsewhere in the codebase to maintain functionality.server/v2/cometbft/abci.go (3)
264-269: Review changes in BlockRequest construction.The
BlockRequeststruct construction inInitChainhas been simplified. Ensure that the removal ofConsensusMessagesdoes not impact other functionalities.Verification successful
No impact from removal of
ConsensusMessagesinBlockRequestconstruction.The search results did not show any dependencies on the
ConsensusMessagesfield within theBlockRequeststruct, indicating that its removal does not affect other functionalities.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changes to `BlockRequest` construction. # Test: Search for all usages of `BlockRequest` to ensure no dependencies on the removed `ConsensusMessages` field. rg --type go 'BlockRequest'Length of output: 76139
156-164: Ensure dynamic app version handling is correct.The dynamic setting of
AppVersionbased on consensus parameters is a significant change. Verify that all scenarios whereversion > 0are correctly handled and that the consensus parameters are always available when needed.Verification successful
Dynamic App Version Handling is Correct
The dynamic setting of
AppVersionbased on consensus parameters is correctly implemented. All usages ofGetConsensusParamsinserver/v2/cometbft/abci.goinclude proper error handling, ensuring that consensus parameters are available when needed. No further issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify scenarios where `version > 0` and consensus parameters are accessed. # Test: Search for all usages of `GetConsensusParams` to ensure proper error handling and availability. rg --type go 'GetConsensusParams'Length of output: 2278
Script:
#!/bin/bash # Extract more context around the usages of `GetConsensusParams` in `server/v2/cometbft/abci.go` rg --type go --context 5 'GetConsensusParams' server/v2/cometbft/abci.goLength of output: 1752
244-251: Clarify context value setting in InitChain.The use of
context.WithValueinInitChainis crucial for setting consensus parameters. Ensure that all necessary parameters are included and correctly utilized in the context.Verification successful
Context Value Setting in InitChain is Correct
The
MsgUpdateParamsstruct used in both setting and retrieving the context value is consistent across the codebase, ensuring proper handling and usage. No issues were found with the current implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct setting and usage of context values in `InitChain`. # Test: Search for all usages of `corecontext.InitInfoKey` to ensure proper handling and retrieval. rg --type go 'corecontext.InitInfoKey'Length of output: 253
Script:
#!/bin/bash # Check the definition of `consensustypes.MsgUpdateParams` ast-grep --lang go --pattern 'type MsgUpdateParams struct { $$$ }' --json # Check the definition of `types.MsgUpdateParams` ast-grep --lang go --pattern 'type MsgUpdateParams struct { $$$ }' --jsonLength of output: 47683
x/consensus/keeper/keeper.go
Outdated
| fmt.Println("InitGenesis consensus") | ||
| value := ctx.Value(corecontext.InitInfoKey).(*types.MsgUpdateParams) |
There was a problem hiding this comment.
Use type assertion with check to prevent panics.
The direct type assertion ctx.Value(corecontext.InitInfoKey).(*types.MsgUpdateParams) can cause a panic if the value is not of the expected type. Consider using a type assertion with a check to handle this safely.
value, ok := ctx.Value(corecontext.InitInfoKey).(*types.MsgUpdateParams)
if !ok {
return fmt.Errorf("unexpected type for InitInfoKey")
}| // endBlock executes the end block logic. | ||
| func (s STF[T]) endBlock( | ||
| ctx *executionContext, |
There was a problem hiding this comment.
Correct the mode attribute in endBlock events.
The mode attribute in endBlock events is incorrectly set to "BeginBlock". It should reflect the end block context.
- event.Attribute{Key: "mode", Value: "BeginBlock"},
+ event.Attribute{Key: "mode", Value: "EndBlock"},Committable suggestion was skipped due to low confidence.
tac0turtle
force-pushed
the
marko/consensusmsg_cleanup
branch
from
5f33a78 to
f2fa48c
Compare
| ExecModeKey = execModeKey{} | ||
| // ExecModeKey is the context key for setting the execution mode. | ||
| ExecModeKey = execModeKey{} | ||
| // CometInfoKey is the context key for allowing modules to get CometInfo. |
There was a problem hiding this comment.
nit: maybe we want to use something more generic than CometInfoKey? Maybe something like ConsensusInfoKey? It's a tiny nit, no need to block merge because of this
There was a problem hiding this comment.
Since the info is called comet I would opt to keep it for consistency.
x/consensus/keeper/keeper.go
Outdated
| @@ -41,6 +42,27 @@ func (k *Keeper) GetAuthority() string { | |||
| return k.authority | |||
| } | |||
|
|
|||
| // InitGenesis initializes the initial state of the module | |||
| func (k *Keeper) InitGenesis(ctx context.Context) error { | |||
| value := ctx.Value(corecontext.InitInfoKey).(*types.MsgUpdateParams) | |||
There was a problem hiding this comment.
value, ok := to fix the tests
tac0turtle
force-pushed
the
marko/consensusmsg_cleanup
branch
from
3224fc8 to
f304637
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 5f33a78b6da3ab7332e6d15213d64497ad8e1dd8 and 429d184.
Files ignored due to path filters (1)
server/v2/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/go.mod (6 hunks)
- simapp/app.go (2 hunks)
- simapp/app_config.go (1 hunks)
- x/consensus/keeper/keeper.go (4 hunks)
- x/consensus/proto/cosmos/consensus/v1/tx.proto (1 hunks)
Files skipped from review due to trivial changes (1)
- x/consensus/proto/cosmos/consensus/v1/tx.proto
Files skipped from review as they are similar to previous changes (1)
- server/v2/go.mod
Additional context used
Path-based instructions (4)
x/consensus/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_config.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.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Learnings (1)
x/consensus/keeper/keeper.go (1)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction. - The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module. - The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
Additional comments not posted (7)
x/consensus/keeper/keeper.go (2)
45-68: LGTM!The
InitGenesismethod properly initializes the module's state using context values and handles errors effectively. The use of type assertion with a check is a good practice.
98-114: Streamlined parameter update process is effective.The
UpdateParamsmethod efficiently updates consensus parameters and handles errors appropriately. The consolidation of parameter setting and event emission improves clarity.simapp/app_config.go (1)
152-152: Inclusion ofconsensustypes.ModuleNameis appropriate.Adding
consensustypes.ModuleNameto theInitGenesissequence ensures that consensus parameters are initialized correctly, enhancing the application's configuration.server/v2/cometbft/abci.go (2)
Line range hint
156-174: DynamicAppVersionretrieval enhances accuracy.The
Infomethod's use of consensus parameters to setAppVersionimproves the accuracy of the information returned, aligning with best practices.
244-251: Effective integration of consensus parameters in context.The
InitChainmethod's modification of the context with consensus parameters allows for streamlined access and manipulation, enhancing the initialization process.simapp/app.go (2)
54-54: Import statement added forconsensustypes.The addition of the
consensustypesimport is appropriate and aligns with the changes in the module initialization order.
505-524: Consensus module prioritized in initialization.The inclusion of
consensustypes.ModuleNameat the beginning ofgenesisModuleOrderensures that the consensus module is initialized first. This aligns with the goal of enhancing consistency and maintainability.Verify if this change affects the initialization dependencies of other modules.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 5f33a78b6da3ab7332e6d15213d64497ad8e1dd8 and 429d184.
Files ignored due to path filters (1)
server/v2/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/go.mod (6 hunks)
- simapp/app.go (2 hunks)
- simapp/app_config.go (1 hunks)
- x/consensus/keeper/keeper.go (4 hunks)
- x/consensus/proto/cosmos/consensus/v1/tx.proto (1 hunks)
Files skipped from review due to trivial changes (2)
- server/v2/go.mod
- x/consensus/proto/cosmos/consensus/v1/tx.proto
Additional context used
Path-based instructions (4)
x/consensus/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_config.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.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Learnings (1)
x/consensus/keeper/keeper.go (1)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction. - The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module. - The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
Additional comments not posted (8)
x/consensus/keeper/keeper.go (3)
Line range hint
117-142:
Validation logic approved!The
paramCheckfunction effectively validates consensus parameters with appropriate error handling.
45-68: LGTM! Verify the function usage in the codebase.The
InitGenesisfunction is well-implemented with proper type assertion checks and validation logic.Ensure that the function is correctly integrated and used across the codebase.
98-114: Refactoring approved! Verify the function usage in the codebase.The
UpdateParamsfunction has been refactored for improved readability and maintainability.Ensure that the function is correctly integrated and used across the codebase.
simapp/app_config.go (1)
152-152: Initialization logic enhancement approved!Adding
consensustypes.ModuleNametoInitGenesisensures proper setup of consensus parameters during initialization.server/v2/cometbft/abci.go (2)
Line range hint
156-174:
Application versioning logic approved!The
Infomethod's enhancement to conditionally assignappVersionbased on consensus parameters improves the accuracy of application version reflection.
244-251: Chain initialization logic approved!The
InitChainmethod's inclusion of additional parameters fromreq.ConsensusParamsin the context enhances the interaction with consensus parameters.simapp/app.go (2)
54-54: Import Statement Approved.The import of
consensustypesfromcosmossdk.io/x/consensus/typesis appropriate for the changes made in thegenesisModuleOrder.
505-524: Initialization Order Update Approved.Adding
consensustypes.ModuleNameto the beginning of thegenesisModuleOrderensures that consensus-related types are initialized first, which aligns with the PR objectives.However, verify that this change does not affect the initialization of other modules adversely.
Verification successful
Initialization Order Change Verified.
The addition of
consensustypes.ModuleNameat the beginning ofgenesisModuleOrderis correctly reflected in the initialization and export order of modules. This change aligns with the intended functionality to prioritize consensus-related modules without adversely affecting other modules.
- The
genesisModuleOrderis used inSetOrderInitGenesisandSetOrderExportGenesis, ensuring consistent initialization and export order.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization order of modules in the genesisModuleOrder. # Test: Check the usage of genesisModuleOrder in the codebase. Expect: Proper initialization order. rg --type go $'genesisModuleOrder'Length of output: 234
|
This touches cometbft and simapp/v2 so we should partially backport |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit 3e41324) # Conflicts: # core/app/app.go # core/context/context.go # server/v2/appmanager/appmanager.go # server/v2/go.mod # server/v2/go.sum # server/v2/stf/stf.go
Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
this pr proposes an alternative to consensus messages. This keeps inline with all the other usage of context key
closes #20996
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.mdref: #21176
Reviewers 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
Improvements
Bug Fixes
Maintenance