feat(server/v2,upgrade): add skip upgrade flag #22682
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
facundomedica,
cool-develope,
akhilkumarpilli,
lucaslopezf,
a team,
hieuvubk and
kocubinski
as code owners
📝 WalkthroughWalkthroughThe changes introduce a new flag, 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
|
|
@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: 0
🧹 Outside diff range and nitpick comments (3)
x/upgrade/depinject.go (2)
22-24: Enhance the constant documentation.While the comment explains the basic purpose, it would be more helpful to include:
- The expected value format
- The implications of using this flag
- A warning about the unsafe nature of skipping upgrades
Consider expanding the documentation:
-// flagUnsafeSkipUpgradesV2 is a custom flag that allows the user to skip upgrades -// It is used in a v2 chain. +// flagUnsafeSkipUpgradesV2 is a custom flag that allows users to skip specific upgrade heights +// in a v2 chain. The value should be a comma-separated list of block heights. +// WARNING: Using this flag is unsafe and may cause your node to panic or diverge from the network +// if not used carefully. Only use this when you know the exact implications of skipping upgrades.
74-81: Improve code structure and comments.While the logic is correct, we can make it more idiomatic and better documented.
Consider this refactoring:
-skipUpgrades, ok := in.ConfigMap[flagUnsafeSkipUpgradesV2] // check v2 -if !ok || skipUpgrades == nil { - skipUpgrades, ok = in.ConfigMap[server.FlagUnsafeSkipUpgrades] // check v1 - if !ok || skipUpgrades == nil { - skipUpgrades = []int{} - } -} +// First try the v2 flag, then fall back to v1 flag for backward compatibility +skipUpgrades := []int{} +if v2Upgrades, ok := in.ConfigMap[flagUnsafeSkipUpgradesV2]; ok && v2Upgrades != nil { + skipUpgrades = cast.ToIntSlice(v2Upgrades) +} else if v1Upgrades, ok := in.ConfigMap[server.FlagUnsafeSkipUpgrades]; ok && v1Upgrades != nil { + skipUpgrades = cast.ToIntSlice(v1Upgrades) +}This refactoring:
- Improves readability with better comments
- Reduces nesting
- Moves the cast operation closer to the source
- Sets a default value upfront
server/v2/server.go (1)
187-187: Consider adding documentation about upgrade skipping risks.While the flag is correctly prefixed with "Unsafe", it would be beneficial to expand the description to explicitly warn users about the potential risks of skipping upgrades, such as state inconsistencies or consensus failures.
Consider updating the description:
-flags.IntSlice(FlagUnsafeSkipUpgrades, []int{}, "Skip a set of upgrade heights to continue the old binary") +flags.IntSlice(FlagUnsafeSkipUpgrades, []int{}, "Skip a set of upgrade heights to continue the old binary. WARNING: Skipping upgrades may lead to state inconsistencies and consensus failures")
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/flags.go(1 hunks)server/v2/server.go(1 hunks)x/upgrade/depinject.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/flags.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/upgrade/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
server/v2/flags.go (2)
13-14: LGTM! Clean formatting changes.
The alignment of the = signs improves code readability while maintaining the same functionality.
15-15: Verify documentation for the new flag.
The implementation of FlagUnsafeSkipUpgrades aligns with the PR objectives and follows the correct naming pattern. Let's ensure it's properly documented.
✅ Verification successful
The new flag is properly documented and used
The unsafe-skip-upgrades flag is well documented in docs/build/building-apps/03-app-upgrade.md with clear explanation of its purpose and usage. The documentation includes:
- Detailed explanation of the flag's role in allowing social consensus to override planned upgrades
- Example usage:
<appd> start --unsafe-skip-upgrades <height1> <optional_height_2> ... <optional_height_N> - Warning about the implications of using this flag
The flag is also properly integrated in the codebase:
- Used in Cosmovisor for upgrade handling
- Implemented in both v1 (
server/start.go) and v2 (server/v2/flags.go) server implementations - Has associated test coverage in
tools/cosmovisor/process_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new flag is documented in help text or documentation files
# Check for documentation in markdown files
rg -l "unsafe-skip-upgrades" --type md
# Check for help text or usage documentation in Go files
rg "unsafe-skip-upgrades.+(?:help|usage|description)" --type go
Length of output: 147
Script:
#!/bin/bash
# Let's check the content of the documentation file and also search for any flag usage
# Check documentation content
cat docs/build/building-apps/03-app-upgrade.md
# Search for the flag usage in Go files
rg "unsafe-skip-upgrades" --type go -C 2
Length of output: 15922
x/upgrade/depinject.go (2)
43-43: LGTM!
The new flag is properly integrated into the ModuleConfigMap with a consistent default value.
Line range hint 22-81: Verify integration with server/v2 package.
The implementation looks good, but let's verify that the server/v2 package properly handles this flag.
✅ Verification successful
Integration with server/v2 package is properly implemented
The verification shows that the unsafe-skip-upgrades flag is correctly integrated in server/v2:
- Flag is properly defined in
server/v2/flags.goasFlagUnsafeSkipUpgrades - Flag is correctly registered in
server/v2/server.gowith appropriate description and default value - The implementation in
x/upgrade/depinject.gocorrectly handles both v1 and v2 versions of the flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that server/v2 properly integrates the new flag
# Check if server/v2 imports and uses the new flag
rg -l "unsafe-skip-upgrades" server/v2/
# Check for any related test files
fd -e go -e test "server.*test" server/v2/
# Look for flag registration in server/v2
ast-grep --pattern 'func $_(flags *pflag.FlagSet) {
$$$
flags.$(unsafe-skip-upgrades)
$$$
}'
Length of output: 293
Script:
#!/bin/bash
# Let's check the actual implementation in server/v2/flags.go
rg -A 5 -B 5 "unsafe-skip-upgrades" server/v2/flags.go
# Check test coverage
rg "unsafe-skip-upgrades" server/v2/server_test.go
Length of output: 352
Script:
#!/bin/bash
# Let's check how the flag is used in server/v2
rg -A 10 "FlagUnsafeSkipUpgrades" server/v2/
# Check if there are any flag registrations in server setup
ast-grep --pattern 'func $_(flags *pflag.FlagSet) {
$$$
}'
Length of output: 1732
server/v2/server.go (1)
187-187: LGTM! Implementation matches server v0 functionality.
The flag implementation correctly aligns with the PR objectives and maintains consistency with the existing codebase. The "Unsafe" prefix appropriately indicates the potential risks of skipping upgrades.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/upgrade/depinject.go (2)
22-24: Enhance the constant documentation.While the comment explains the basic purpose, it would be more helpful to include:
- The expected format of the flag value
- An example usage
- Why/when someone would use this flag
-// flagUnsafeSkipUpgradesV2 is a custom flag that allows the user to skip upgrades -// It is used in a v2 chain. +// flagUnsafeSkipUpgradesV2 is a flag that allows users to skip specific upgrade heights +// in a v2 chain. The value should be a comma-separated list of block heights. +// Example: --server.unsafe-skip-upgrades=1234,2000,3000 +// Use this flag with caution as it bypasses mandatory chain upgrades.
74-81: Consider refactoring the flag checking logic.The current implementation has some repetition in the flag checking logic. Consider extracting this into a helper function for better maintainability.
+func getSkipUpgrades(configMap coreserver.ConfigMap, flags ...string) []int { + for _, flag := range flags { + if skipUpgrades, ok := configMap[flag]; ok && skipUpgrades != nil { + return cast.ToIntSlice(skipUpgrades) + } + } + return []int{} +} func ProvideModule(in ModuleInputs) ModuleOutputs { var ( homePath string skipUpgradeHeights = make(map[int64]bool) ) - skipUpgrades, ok := in.ConfigMap[flagUnsafeSkipUpgradesV2] // check v2 - if !ok || skipUpgrades == nil { - skipUpgrades, ok = in.ConfigMap[server.FlagUnsafeSkipUpgrades] // check v1 - if !ok || skipUpgrades == nil { - skipUpgrades = []int{} - } - } + skipUpgrades := getSkipUpgrades(in.ConfigMap, + flagUnsafeSkipUpgradesV2, // check v2 first + server.FlagUnsafeSkipUpgrades, // fallback to v1 + )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/flags.go(1 hunks)server/v2/server.go(1 hunks)x/upgrade/depinject.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/flags.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/upgrade/depinject.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
server/v2/flags.go (2)
13-14: LGTM! Consistent formatting applied.
The reformatting of existing flags maintains consistency and improves readability.
15-15: Verify the flag usage across the codebase.
The new flag addition aligns with the PR objectives to support unsafe-skip-upgrades in server/v2. Let's verify its integration.
✅ Verification successful
The unsafe-skip-upgrades flag is properly integrated and documented
The flag is well integrated across the codebase:
- Defined in
server/v2/flags.goasFlagUnsafeSkipUpgrades - Referenced in
x/upgrade/depinject.gofor dependency injection - Extensively tested in
tools/cosmovisor/process_test.go - Properly documented in
docs/build/building-apps/03-app-upgrade.mdwith usage examples and implications - Implementation logic present in
tools/cosmovisor/process.gofor handling upgrade height skipping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of the new flag across the codebase
# Check if the flag is properly used in server implementation
echo "Checking server implementation..."
rg -A 5 "unsafe-skip-upgrades" server/v2/
# Check for any potential conflicts or duplicate declarations
echo "Checking for conflicts..."
rg -A 5 "unsafe-skip-upgrades" --glob '!server/v2/**'
# Check if the flag is documented in help text
echo "Checking documentation..."
rg -A 5 "unsafe-skip-upgrades" -g '*.md'
Length of output: 8286
x/upgrade/depinject.go (2)
43-43: LGTM!
The configuration map entry is properly added with a consistent default value.
74-81: Verify the flag handling in different scenarios.
The implementation looks good, but let's verify the behavior in different scenarios to ensure robustness.
✅ Verification successful
Let me gather more information about the flag usage and test coverage.
Flag implementation is properly handled with sufficient test coverage
The verification shows that:
- The flag is correctly implemented in both v1 (
server.FlagUnsafeSkipUpgrades) and v2 (server.unsafe-skip-upgrades) formats - There is comprehensive test coverage with multiple test cases:
TestSkipUpgradeSkippingAllTestUpgradeSkippingOneTestUpgradeSkippingOnlyTwoTestUpgradeWithoutSkip
- The flag is properly integrated into the application configuration and CLI flags
- Default empty slice is correctly set when the flag is not provided
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the upgrade flag handling implementation
# Check for any existing usage of the new flag
rg -l "server\.unsafe-skip-upgrades" --type go
# Check for test coverage of the new flag
rg -l "TestUnsafeSkipUpgrades" --type go
# Look for any potential conflicts or duplicate flag definitions
ast-grep --pattern 'const $_ = "server.unsafe-skip-upgrades"'
Length of output: 6443
Script:
#!/bin/bash
# Check the implementation details and test coverage
# Look for the v1 flag usage
rg "FlagUnsafeSkipUpgrades" --type go -A 2
# Check for any upgrade-related tests
rg "func.*Test.*Upgrade.*Skip" --type go
# Look for any upgrade handler implementations that might use these flags
ast-grep --pattern 'func $_($_) UpgradeHandler {'
Length of output: 1991
server/v2/server.go (1)
187-187: LGTM: Flag implementation aligns with PR objectives
The implementation correctly adds the unsafe-skip-upgrades flag support, allowing users to specify upgrade heights to skip. The flag type (IntSlice) and default value (empty slice) are appropriate for this use case.
Let's verify the flag declaration consistency:
(cherry picked from commit 6cfe2dc) # Conflicts: # server/v2/flags.go # server/v2/server.go
Description
Closes: #22238
Went with adding it directly to server/v2 as it was always present as well in server v0.
Backporting label for depinject provider config logic
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
FlagUnsafeSkipUpgrades, allowing users to specify upgrade heights to skip.flagUnsafeSkipUpgradesV2, enhancing upgrade management for version 2 chains.Bug Fixes
Documentation