fix(cosmovisor): premature upgrade on restart #22528
Conversation
📝 WalkthroughWalkthroughThe pull request updates the changelog for the Changes
Assessment against linked issues
Possibly related PRs
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
|
akhilkumarpilli
requested review from
julienrbrt,
facundomedica and
a team
as code owners
This comment has been minimized.
This comment has been minimized.
![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 (3)
tools/cosmovisor/scanner.go (2)
154-156: Improve error handling clarity and documentationThe current implementation combines multiple conditions in a way that makes the logic hard to follow. Consider separating the conditions and improving the comment.
Apply this change:
- currentHeight, err := fw.checkHeight() - if (err != nil || currentHeight < info.Height) && !errors.Is(err, errUntestAble) { // ignore this check for tests - return false - } + currentHeight, err := fw.checkHeight() + // Skip height verification in test environment + if errors.Is(err, errHeightCheckUnavailable) { + return true + } + // Return false if there's an error or if we haven't reached the upgrade height + if err != nil || currentHeight < info.Height { + return false + }
186-188: Approve with minor comment improvementThe implementation correctly prevents premature upgrades by handling test scenarios appropriately. However, the comment could be more descriptive.
Apply this change:
- if testing.Testing() { // we cannot test the command in the test environment + if testing.Testing() { // Skip height check in test environment as status command is not available return 0, errUntestAble }tools/cosmovisor/CHANGELOG.md (1)
52-52: Enhance the bug fix description with more context.While the entry follows the format, consider adding more details to help users understand the issue and its resolution. Suggested revision:
-* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor. +* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor where upgrades were incorrectly applied before reaching the designated block height.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
tools/cosmovisor/CHANGELOG.md(1 hunks)tools/cosmovisor/scanner.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tools/cosmovisor/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
tools/cosmovisor/scanner.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
tools/cosmovisor/scanner.go (1)
Line range hint 154-188: Verify the fix for premature upgrades
The implementation looks correct, but let's verify that it properly handles the restart scenario described in issue #22167.
✅ Verification successful
Let me gather more information about the height checking implementation to ensure it's properly handling edge cases.
Let me check the test coverage and implementation details one more time to ensure the restart scenario is properly tested.
Implementation correctly handles restart scenarios and premature upgrades
The code review confirms that the implementation properly addresses the premature upgrade issue with multiple safeguards:
- The
initializedflag correctly tracks daemon restarts - Height checks are properly guarded against test scenarios and errors
- The upgrade name comparison during restart prevents incorrect upgrades
- Empty file handling with retry mechanism prevents race conditions
- Height comparison ensures upgrades only trigger at the right block height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions or edge cases in height checking
# Look for other height checks that might need similar protection
rg -n "latest_block_height|LatestBlockHeight" --type go
# Check for any restart-related error handling
rg -n "daemon has restarted" --type go -A 5
Length of output: 1480
Script:
#!/bin/bash
# Check the complete implementation of height checking and upgrade logic
ast-grep --pattern 'func ($_ *fileWatcher) checkHeight() {
$$$
}'
# Look for upgrade-related error handling
rg -n "needsUpdate" --type go -A 3
# Check for any test cases related to restart scenarios
rg -n "TestFileWatcher.*restart|TestUpgrade.*restart" --type go
Length of output: 1506
Script:
#!/bin/bash
# Check test files for upgrade and restart scenarios
rg -n "func Test.*" tools/cosmovisor/scanner_test.go -B 1 -A 5
# Look for the complete implementation of the scanner
cat tools/cosmovisor/scanner.go
# Check for any upgrade-related test utilities
rg -n "func.*upgrade" tools/cosmovisor/testdata/ --type go
Length of output: 6583
tools/cosmovisor/CHANGELOG.md (2)
43-43: LGTM! Well-formatted changelog entry.
The new feature entry follows the changelog guidelines with proper PR reference and clear description.
Line range hint 1-52: LGTM! Changelog structure is well-maintained.
The changelog follows the Keep a Changelog format consistently, with proper organization of sections and version history.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tools/cosmovisor/CHANGELOG.md (2)
39-40: Update the release dateThe release date is set to November 15, 2024, which is in the future. Consider updating it to the actual release date or moving these changes to the "Unreleased" section until the release is made.
52-54: Enhance the bug fix descriptionWhile the entry correctly references the PR, it would be more helpful to:
- Add a reference to the original issue ([Bug]: Cosmovisor prematurely upgrades after systemctl restart cosmovisor.service #22167)
- Provide more context about the bug and its impact
- Mention that it specifically fixes the issue where Cosmovisor was upgrading prematurely when the node was restarted before reaching the target height
Suggested revision:
-* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor. +* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor where upgrades were incorrectly applied before reaching the target height (fixes #22167).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
tools/cosmovisor/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (1)
tools/cosmovisor/CHANGELOG.md (1)
42-45: LGTM! Well-documented features
The Features section is well-structured, with clear descriptions and proper issue references for each new command.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/cosmovisor/CHANGELOG.md (1)
54-54: Enhance the bug fix description to better reflect the PR objectives.While the entry correctly references PR #22528, it could be more descriptive about the specific issue being fixed.
Consider expanding the description to:
-* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor. +* [#22528](https://github.com/cosmos/cosmos-sdk/pull/22528) Fix premature upgrades on restarting cosmovisor by ensuring upgrades only occur at the specified block height.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
tools/cosmovisor/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/cosmovisor/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (2)
tools/cosmovisor/CHANGELOG.md (2)
39-40: LGTM: Version entry follows the changelog format.
The new version entry follows the correct format with version number and date.
42-45: LGTM: Features section is well-documented.
Each feature entry properly includes:
- PR reference link
- Clear description of the new functionality
- Consistent formatting
* main: build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580) fix(server/v2/api/telemetry): enable global metrics (#22571) refactor(server/v2/cometbft): add `codec.Codec` and clean-up APIs (#22566) feat(core/coretesting): make memDB satisfy db.Db interface (#22570) Merge commit from fork fix(server(/v2)): fix fallback genesis path (#22564) fix: only overwrite context chainID when necessary (#22568) docs(client): Update setFeeGranter and setFeePayer comments (#22526) fix(baseapp): populate header info in `NewUncachedContext` (#22557) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.35.1-20240701160653-fedbb9acfd2f.1 to 1.35.2-20240701160653-fedbb9acfd2f.1 in /api (#22551) build(deps): Bump github.com/creachadair/atomicfile from 0.3.5 to 0.3.6 in /tools/confix (#22552) docs: Update reference of Approximation (#22550) fix(server/v2/comebft): wire missing services + fix simulation (#21964) ci: fix permissions for GITHUB_TOKEN on dependabot workflows (#22547) ci: fix permissions for GITHUB_TOKEN in spell check workflow (#22545) build(deps): Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 (#22537) fix(cosmovisor): premature upgrade on restart (#22528) fix(store/v2/pebble): handle version 0 in keys (#22524) refactor(server/v2/telemetry): swap redirects (#22520) docs: Update content in CODE_OF_CONDUCT.md (#22518)
Description
Closes: #22167
When on restarting, unable to fetch current height and height will be 0, which will update binary
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
cosmovisor show-upgrade-info,add-batch-upgrade, andprepare-upgradefor enhanced functionality.Bug Fixes
cosmovisor.Documentation