fix(simapp,systemtests): fix chain upgrade #22669
Conversation
alpe
requested review from
cool-develope,
kocubinski,
a team,
facundomedica and
julienrbrt
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to 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
|
|
@alpe your pull request is missing a changelog! |
| @@ -253,7 +253,7 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { | |||
|
|
|||
| store, err := rs.loadCommitStoreFromParams(key, commitID, storeParams) | |||
| if err != nil { | |||
| return errorsmod.Wrap(err, "failed to load store") | |||
| return errorsmod.Wrapf(err, "failed to load store for %s", key.Name()) | |||
There was a problem hiding this comment.
This change is not needed for the fix but it adds some very valuable debug output
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/systemtests/upgrade_test.go (3)
Line range hint
89-98: Enhance post-upgrade validationThe current smoke test only verifies basic transaction functionality. Consider adding:
- Version verification after upgrade
- State migration validation
- Negative test cases (e.g., invalid upgrade height)
Example addition:
got = cli.Run("tx", "protocolpool", "fund-community-pool", "100stake", "--from=node0") systest.RequireTxSuccess(t, got) + +// Verify chain version after upgrade +version := cli.CustomQuery("q", "upgrade", "version") +require.Contains(t, version, "0.54", "expected chain to be upgraded to v0.54") + +// Verify state migration +// Add specific state checks here based on what should be migrated
Line range hint
35-39: Improve test reliability and cleanupThe test setup could be improved for better reliability and isolation:
- The 5-second voting period might be too short for CI environments
- Missing proper cleanup in case of test failures
Consider these improvements:
+defer func() { + systest.Sut.StopChain() + systest.Sut.SetExecBinary(currentBranchBinary) + systest.Sut.SetTestnetInitializer(currentInitializer) +}() + votingPeriod := 5 * time.Second // enough time to vote +if os.Getenv("CI") == "true" { + votingPeriod = 15 * time.Second // longer timeout for CI +}
Line range hint
76-86: Enhance upgrade validation checksThe upgrade process validation could be strengthened by:
- Validating upgrade plan details before proceeding
- Adding configurable timeouts for CI environments
- Checking chain halt status
Example improvements:
raw = cli.CustomQuery("q", "gov", "proposal", proposalID) proposalStatus := gjson.Get(raw, "proposal.status").String() require.Equal(t, "PROPOSAL_STATUS_PASSED", proposalStatus, raw) + +// Validate upgrade plan details +upgradePlan := cli.CustomQuery("q", "upgrade", "plan") +require.Equal(t, upgradeName, gjson.Get(upgradePlan, "plan.name").String()) +require.Equal(t, fmt.Sprint(upgradeHeight), gjson.Get(upgradePlan, "plan.height").String()) + +// Verify chain halt status +nodeStatus := cli.CustomQuery("status") +require.True(t, gjson.Get(nodeStatus, "sync_info.catching_up").Bool(), "expected chain to be halted")store/rootmulti/store.go (1)
256-256: Improved error handling with store contextThe enhanced error message now includes the store name, making it easier to identify which store failed during version loading. This improvement is particularly valuable for debugging chain upgrade issues.
Consider standardizing this error handling pattern across other store operations for consistency. For example:
-if err := deleteKVStore(store.(types.KVStore)); err != nil { - return errorsmod.Wrapf(err, "failed to delete store %s", key.Name()) -} +if err := deleteKVStore(store.(types.KVStore)); err != nil { + return errorsmod.Wrapf(err, "failed to delete store %q", key.Name()) +}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
simapp/upgrades.go(1 hunks)store/rootmulti/store.go(1 hunks)tests/systemtests/upgrade_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
simapp/upgrades.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/rootmulti/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/upgrade_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
simapp/upgrades.go (3)
Line range hint 8-15: LGTM! Well-documented upgrade name constant.
The documentation clearly explains the purpose of this upgrade and its version scope.
35-36: Verify store upgrade changes
The simplified store upgrades configuration aligns with the PR objective of fixing version mismatch issues. However, please ensure:
- The removal of previously included store keys won't impact the upgrade process
- The empty
Deletedarray is intentional and won't leave any deprecated stores
✅ Verification successful
Store upgrade configuration is appropriate
The store upgrade configuration is correct and safe:
- The
bankv2types.StoreKeyaddition is the only required change for this upgrade - No store deletions are needed as this is adding a new store version rather than removing old stores
- The empty
Deletedarray is intentional and correct for this scenario
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any store key references that might need migration
rg -A 3 'StoreKey.*=.*"(accounts|protocolpool|epochs|crisis)"'
# Look for any store deletion patterns in upgrade handlers
ast-grep --pattern 'Deleted.*=.*\[.*\]'
Length of output: 106
Script:
#!/bin/bash
# Search for store keys in the codebase
rg -A 3 'StoreKey.*=.*".*"'
# Look for store upgrade configurations in other upgrade handlers
rg -A 5 'StoreUpgrades.*='
# Check for any store-related imports and configurations
rg -l 'storetypes\.StoreUpgrades'
Length of output: 17795
Script:
#!/bin/bash
# Check for bankv2 store key references
rg -A 3 'bankv2.*StoreKey'
# Look for previous store upgrade configurations
rg -A 5 'StoreUpgrades.*=' simapp/
# Check for any bank module store migrations
rg -l 'bank.*Migrate'
Length of output: 348
Line range hint 17-33: Verify completeness of migration handlers
The upgrade handler implementation looks correct, but given this is a critical upgrade path from v0.52, we should verify that all necessary module migrations are properly registered in the ModuleManager.
✅ Verification successful
Let me gather more information about module migrations to verify completeness.
Based on the search results, I can now provide a definitive response about the upgrade handler implementation.
Migration handler implementation is complete and correct
The upgrade handler correctly uses ModuleManager.RunMigrations which automatically handles all registered module migrations. The search results show:
- The upgrade name "v052-to-v054" is properly defined and used consistently
- The handler correctly implements store upgrades by adding the new bank store key
- All core modules have properly registered their migrations through
RegisterMigrations, including:- x/auth
- x/bank
- x/distribution
- x/gov (with v5 to v6 migrations)
- x/staking
- x/authz
- x/feegrant
- x/group
- x/mint
- x/slashing
The implementation follows the standard upgrade pattern and includes all necessary components for a complete upgrade from v0.52 to v0.54.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any migration registrations in the codebase
ast-grep --pattern 'RegisterMigration($_, $_)'
# Look for any other upgrade handlers that might be relevant
rg -A 5 'SetUpgradeHandler|RegisterUpgradeHandler'
Length of output: 20160
Script:
#!/bin/bash
# Search for module consensus versions and migrations
ast-grep --pattern 'ConsensusVersion() uint64'
# Look for any module migration registrations
rg -A 5 'RegisterMigration|MigrateStore'
# Check for any module version changes in the upgrade
rg -A 5 'v052-to-v054|UpgradeName'
Length of output: 32775
| // Scenario: | ||
| // start a legacy chain with some state | ||
| // when a chain upgrade proposal is executed | ||
| // then the chain upgrades successfully | ||
| systest.Sut.StopChain() | ||
|
|
||
| legacyBinary := FetchExecutable(t, "v0.52") | ||
| legacyBinary := FetchExecutable(t, "0.52.0-beta.3") |
There was a problem hiding this comment.
💡 Codebase verification
The choice of legacy binary version 0.52.0-beta.3 needs to be updated
The current version choice appears incorrect for the following reasons:
- The issue description specifically mentions testing with
v0.52from therelease/v0.52.xbranch - The upgrade test is designed to test migration from v0.52.x to v0.54.x (as seen in
simapp/upgrades.go) - The available beta versions (
v0.52.0-beta.1,v0.52.0-beta.2,v0.52.0-beta.3) are pre-release versions and not the final v0.52.x release
Recommendation:
- Use the binary built from
release/v0.52.xbranch as specified in the issue reproduction steps instead of a beta version
🔗 Analysis chain
Verify the choice of legacy binary version
The change to use a specific beta version 0.52.0-beta.3 improves test reproducibility. However, we should verify if this is the correct version that exhibits the upgrade issue mentioned in #22146.
Let me gather more information about the upgrade test implementation to better understand the version choice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the issue occurs with this specific version
gh issue view 22146 --json body | jq -r '.body' | grep -A 5 -B 5 "0.52"
# Check available versions
curl -s https://api.github.com/repos/cosmos/cosmos-sdk/releases | jq -r '.[].tag_name' | grep "^v0.52"
Length of output: 509
Script:
#!/bin/bash
# Check the upgrade test implementation
cat tests/systemtests/upgrade_test.go
# Check if there are any other references to v0.52 in the codebase
rg "0\.52" --type go
Length of output: 5505
Description
Closes: #22146
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
bankv2module.Bug Fixes
Tests