fix(consensus): do not override app version in msg update params #23622
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 WalkthroughWalkthroughThe changes update how the application version and consensus parameters are handled. The Changes
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package sqlite3: could not load export data: no export data for "github.com/bvinc/go-sqlite-lite/sqlite3"" Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:x/consensus
C:server/v2
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
server/v2/cometbft/abci.go (1)
144-160: Improve error handling in Info method.The error handling logic could be simplified and made more robust:
- The error check for
GetConsensusParamsis duplicated (lines 157-159).- The error handling for
collections.ErrNotFoundcould be combined with the nil check.Apply this diff to improve the error handling:
// if height is 0, we dont know the consensus params var appVersion = InitialAppVersion if version > 0 { cp, err := GetConsensusParams(ctx, c.app) - // if the consensus params are not found, we set the app version to 0 - // in the case that the start version is > 0 - if cp == nil || errors.Is(err, collections.ErrNotFound) { + if err != nil { + if errors.Is(err, collections.ErrNotFound) { + appVersion = 0 + } else { + return nil, err + } + } else if cp == nil { appVersion = 0 - } else if err != nil { - return nil, err } else { appVersion = cp.Version.GetApp() } - if err != nil { - return nil, err - } }
🧹 Nitpick comments (2)
server/v2/cometbft/abci.go (2)
37-39: Consider using a constant instead of a mutable global variable.Using a mutable global variable for
InitialAppVersioncould lead to race conditions and make the code harder to reason about. Unless there's a specific requirement for runtime modification, consider using a constant.Apply this diff to make it a constant:
-// InitialAppVersion returned by Info at height 0. -// Afterwards, the consensus params are queried from the app. -var InitialAppVersion uint64 = 0 +// InitialAppVersion is returned by Info at height 0. +// Afterwards, the consensus params are queried from the app. +const InitialAppVersion uint64 = 0
287-290: Add clarifying comments about app version handling in genesis.The comments explain the app version handling policy well, but they could be more detailed about the implications and rationale.
Add more detailed comments:
// note the app version is not read from genesis // user can update InitialAppVersion to that value if needed // from height 1, we will query the app for the version +// This design allows for flexible app version management: +// 1. At height 0: Uses InitialAppVersion +// 2. At height 1+: Queries app's consensus params +// 3. For custom genesis: User can modify InitialAppVersion
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
baseapp/params.go(1 hunks)server/v2/cometbft/abci.go(3 hunks)simapp/v2/sim_runner.go(2 hunks)tests/integration/v2/app.go(2 hunks)x/consensus/depinject.go(1 hunks)x/consensus/keeper/keeper.go(3 hunks)x/consensus/types/msgs.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/consensus/types/msgs.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/params.gox/consensus/depinject.gosimapp/v2/sim_runner.goserver/v2/cometbft/abci.gotests/integration/v2/app.gox/consensus/keeper/keeper.go
`tests/**/*`: "Assess the integration and e2e test code asse...
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/app.go
🪛 GitHub Actions: Lint
simapp/v2/sim_runner.go
[error] 15-15: package "github.com/cometbft/cometbft/api/cometbft/types/v1" is being imported more than once (stylecheck)
🪛 golangci-lint (1.62.2)
x/consensus/keeper/keeper.go
10-10: ST1019(related information): other import of "github.com/cometbft/cometbft/api/cometbft/types/v1"
(stylecheck)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (5)
baseapp/params.go (1)
9-9: LGTM! The change from const to var is appropriate.This change allows for runtime modification of the initial app version, which aligns with the PR's objective.
x/consensus/depinject.go (1)
102-104: LGTM! Good simplification of the AppVersion method.The change reduces code duplication by directly delegating to the keeper's implementation, improving maintainability without losing any functionality.
x/consensus/keeper/keeper.go (1)
155-158: LGTM! Good protection of version parameters.The added comments and logic ensure that version parameters are not accidentally overridden during updates, which aligns with the PR's objective.
tests/integration/v2/app.go (1)
279-286: LGTM! Clean transition to v1.ConsensusParams.The context initialization correctly uses the new type while preserving all necessary consensus parameters.
server/v2/cometbft/abci.go (1)
296-298: LGTM!The simplification of consensus params context value setting is a good improvement.
|
Can we get a test for this? |
There's not really a functional change. The test is kind of that the app still start 😅 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/consensus/keeper/keeper.go (1)
66-77: Consider simplifying validation logic.The validation is performed by converting back to
MsgUpdateParamsjust for validation purposes. This seems redundant since we already have the consensus params in the correct type.Consider validating the consensus params directly:
- // validate the consensus params - // this avoids to duplicate the validation in here as well - if _, err := (&types.MsgUpdateParams{ - Block: value.Block, - Evidence: value.Evidence, - Validator: value.Validator, - Abci: value.Abci, - Synchrony: value.Synchrony, - Feature: value.Feature, - }).ToProtoConsensusParams(); err != nil { + // validate the consensus params directly + if err := validateConsensusParams(value); err != nil { return err }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
baseapp/abci.go(1 hunks)server/v2/cometbft/abci.go(4 hunks)simapp/v2/sim_runner.go(1 hunks)tests/integration/v2/app.go(1 hunks)x/consensus/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/v2/app.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.gox/consensus/keeper/keeper.gosimapp/v2/sim_runner.gobaseapp/abci.go
📓 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: 2024-11-10T03:53:32.474Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (6)
x/consensus/keeper/keeper.go (2)
60-64: LGTM! Improved type safety in context value handling.The change from
MsgUpdateParamsto*cmtproto.ConsensusParamsaligns with the direct usage of consensus parameters, reducing unnecessary type conversions.
154-157: LGTM! Clear documentation of version handling.The comments clearly explain why version parameters should not be overridden, as they are managed by the upgrade module.
simapp/v2/sim_runner.go (1)
374-386: LGTM! Clear initialization of consensus parameters.The initialization of consensus parameters is well-structured and includes all necessary fields with appropriate values for simulation testing.
server/v2/cometbft/abci.go (2)
38-40: LGTM! Well-documented version initialization.The
InitialAppVersionvariable is clearly documented, explaining its usage at height 0 and subsequent behavior.
296-304: LGTM! Proper version parameter initialization.The code correctly initializes version parameters when they are nil or zero, using
InitialAppVersionas the default value.baseapp/abci.go (1)
76-87: LGTM! Consistent version parameter handling.The code maintains consistency with other files by initializing version parameters with
InitialAppVersionwhen they are nil or zero.
Description
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
Refactor
Tests