fix!: v3 upgrade delay #4008
fix!: v3 upgrade delay #4008
Conversation
evan-forbes
added
bug
evan-forbes
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the upgrade logic and timeout configurations within the application. Modifications include updating the Changes
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
|
test/util/testnode/config.go
Outdated
| // WithBlockTime sets the block time and returns the Config. | ||
| func (c *Config) WithBlockTime(d time.Duration) *Config { | ||
| c.TmConfig.Consensus.TimeoutCommit = d | ||
| creator := DefaultAppCreator(d) | ||
| c.WithAppCreator(creator) | ||
| return c | ||
| } |
There was a problem hiding this comment.
added this to help fix the insanely flakey square size test
There was a problem hiding this comment.
Can we not still call this TimeoutCommit because it changes the TimeoutCommit not the BlockTime
There was a problem hiding this comment.
sorry, I'm not understanding this comment completely. the reasoning for this change was because we were overriding the timeout
There was a problem hiding this comment.
You deprecated the method WithTimeoutComimt and created a new method called WithBlockTime. My comment was why not just modify the WithTimeoutCommit? WithBlockTime is misleading because it doesn't change the block time to that duration, just the timeout commit.
There was a problem hiding this comment.
ahh I get it now thanks, good point
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
pkg/appconsts/versioned_consts.go (1)
Line range hint
83-102: Implementation aligns with PR objectives, but consider readability improvementThe function correctly implements the fix by using the appropriate version delay for non-arabica chains while maintaining the existing behavior for the arabica chain. However, the negative condition could be made more readable.
Consider restructuring the condition for better readability:
-case v == v2.Version && !strings.Contains(chainID, "arabica"): +case v == v2.Version: + if strings.Contains(chainID, "arabica") { + return v3.UpgradeHeightDelay + } return v2.UpgradeHeightDelaytest/util/testnode/config.go (2)
93-95: Consider adding version information to deprecation noticeThe deprecation notice is clear but would benefit from indicating which version will remove this method.
-// Deprecated: use WithBlockTime instead. WithTimeoutCommit sets the timeout +// Deprecated: Will be removed in v4. Use WithBlockTime instead. WithTimeoutCommit sets the timeout
Line range hint
184-199: Inconsistent block time handling in CustomAppCreatorWhile
DefaultAppCreatornow properly handles configurable block time,CustomAppCreatorstill uses a hardcodedtime.Millisecond*0. Consider updating it to accept ablockTimeparameter for consistency.-func CustomAppCreator(minGasPrice string) srvtypes.AppCreator { +func CustomAppCreator(minGasPrice string, blockTime time.Duration) srvtypes.AppCreator { // ... existing code ... - app.SetEndBlocker(wrapEndBlocker(app, time.Millisecond*0)) + app.SetEndBlocker(wrapEndBlocker(app, blockTime)) return app }Also applies to: 201-215
x/signal/keeper.go (1)
105-109: Consider adding defensive validation for header fieldsWhile the implementation is correct, it might be worth adding validation for the header fields to prevent potential panics.
Consider adding checks:
header := sdkCtx.BlockHeader() +if header.ChainID == "" { + return &types.MsgTryUpgradeResponse{}, types.ErrInvalidHeader.Wrap("chain ID is empty") +} upgrade := types.Upgrade{ AppVersion: version, UpgradeHeight: sdkCtx.BlockHeader().Height + appconsts.UpgradeHeightDelay(header.ChainID, header.Version.App), }app/test/upgrade_test.go (1)
Line range hint
109-155: Consider adding test cases for different chain IDsThe test currently verifies the upgrade behavior with a single chain ID. Given that the PR introduces chain ID-dependent behavior, it would be valuable to add test cases for both "arabica" and non-"arabica" chain IDs to ensure the correct upgrade delay is used in both scenarios.
Would you like me to help create additional test cases that cover both chain ID scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/test/priority_test.go (1 hunks)
- app/test/square_size_test.go (1 hunks)
- app/test/upgrade_test.go (4 hunks)
- pkg/appconsts/versioned_consts.go (2 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
- test/txsim/run_test.go (2 hunks)
- test/util/testnode/config.go (4 hunks)
- x/signal/integration_test.go (2 hunks)
- x/signal/keeper.go (1 hunks)
- x/signal/keeper_test.go (3 hunks)
🔇 Additional comments (16)
pkg/appconsts/versioned_consts.go (2)
5-5: LGTM: Import addition is correctThe strings package import is properly placed and necessary for the new chain ID checking functionality.
97-97: Verify chain ID matching robustnessThe current implementation uses case-sensitive string matching for "arabica". We should verify if this matches all possible chain ID formats.
✅ Verification successful
Based on the search results, I can now provide a final response about the chain ID matching concern:
Chain ID matching for "arabica" is consistent and properly tested
The search results show that:
- The chain ID format is consistently defined as "arabica-11" across the codebase
- There are specific test cases in
pkg/appconsts/versioned_consts_test.gothat verify the behavior for the arabica chain- The string matching is used in a specific context where case sensitivity is appropriate, as all arabica chain IDs follow the same format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for chain ID definitions or usage patterns echo "Searching for chain ID patterns in the codebase..." rg -i "chain.*id.*arabica" -A 2 -B 2 # Search for test files that might contain chain ID test cases echo "Searching for chain ID related tests..." fd -e go | xargs rg "TestChainID|chain.*id.*test"Length of output: 3756
x/signal/integration_test.go (2)
26-26: LGTM! Verify test coverage for different chain IDs.The addition of chainID parameter aligns with the PR objectives. However, since this is a consensus-breaking change with different behaviors for "arabica" vs non-arabica chains, consider adding test cases for both scenarios.
Let's verify if there are other test cases covering the "arabica" chain ID:
#!/bin/bash # Search for test files that might contain "arabica" chain ID tests rg -l "arabica.*chain" --type goAlso applies to: 31-31
82-82: Verify upgrade delay behavior for consensus-breaking change.The modification correctly implements the new chainID parameter for upgrade delay calculation. This is a critical change that affects consensus.
Let's verify the implementation of UpgradeHeightDelay to ensure it handles chain IDs correctly:
✅ Verification successful
Let me gather more information about the version-specific delay constants to ensure they are properly defined.
Implementation correctly handles chain-specific upgrade delays
The implementation has been verified and shows proper handling of upgrade delays with the following key aspects:
- Correctly implements chainID parameter to handle special case for "arabica" chain
- Well-defined version-specific delays:
- v1: 50,400 blocks (7 days with 12s blocks)
- v2: 50,400 blocks (7 days with 12s blocks), skipped for arabica chain
- v3: 100,800 blocks (7 days with 6s blocks)
- Includes override capability for testing purposes
- Proper fallback to v3 delay for unknown versions
The implementation is robust and handles the consensus-critical upgrade delays appropriately for different chains and versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the UpgradeHeightDelay implementation ast-grep --pattern 'func UpgradeHeightDelay($_, $_) $_'Length of output: 1506
Script:
#!/bin/bash # Search for version-specific UpgradeHeightDelay constants rg "UpgradeHeightDelay.*=.*" -A 1Length of output: 1173
pkg/appconsts/versioned_consts_test.go (1)
84-129: LGTM! Well-structured test cases that validate the version-specific upgrade delay behavior.The test cases effectively cover the core functionality and document the special handling for arabica chains. The table-driven test approach is clean and maintainable.
test/txsim/run_test.go (2)
154-154: LGTM: Block time configuration change in Setup function.The change from
WithTimeoutCommittoWithBlockTimeis appropriate here as it provides more precise control over block creation timing in the test environment.
168-170: Verify test coverage for upgrade delay scenarios.While the block time configuration change is correct, given that this PR fixes a v3 upgrade delay bug, consider adding specific test cases to verify:
- The correct upgrade delay is used for non-arabica chains
- The next version's delay is used for arabica chains
Let's verify the test coverage:
Consider adding a test helper function that explicitly verifies the upgrade delay calculation for different chain IDs to ensure the core fix is properly tested.
✅ Verification successful
Let me try a different search strategy to find test coverage for upgrade-related scenarios.
These broader search patterns should help us identify test coverage for upgrade scenarios, arabica chain handling, and version-specific test cases.
Let me search specifically for upgrade delay related tests.
This should help us find the specific test coverage for upgrade delays and v3 upgrade scenarios.
Based on the search results, I can now provide a final response about the test coverage for upgrade delay scenarios.
Test coverage for upgrade delay is already comprehensive
The codebase has thorough test coverage for upgrade delay scenarios, particularly in
test/e2e/major_upgrade_v3.go:
- The test explicitly verifies block time durations before and after v3 upgrade using
appconsts.GetTimeoutCommit()at specific heights- It checks for correct timing transitions at the upgrade boundary with detailed validation of block durations
- The test includes verification that block times decrease appropriately after v3 upgrade
- Edge cases are handled, including skipping blocks with multiple rounds and precise upgrade height checks
The test implementation specifically validates:
- Pre-upgrade block timing
- Post-upgrade block timing with v3 parameters
- Correct transition at the upgrade height
- Error cases for both too fast and too slow block times
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for upgrade delay related test cases rg -A 5 "UpgradeHeightDelay|upgrade.*delay" test/Length of output: 51
Script:
#!/bin/bash # Search for upgrade-related test cases with broader context rg -B 5 -A 10 "upgrade.*test|test.*upgrade" test/ --type go # Also search for any arabica chain related tests rg -B 5 -A 10 "arabica|chain.*id" test/ --type go # Look for version-related test configurations rg -B 5 -A 10 "WithVersion|SetVersion|version.*test" test/ --type goLength of output: 46842
Script:
#!/bin/bash # Search for upgrade delay related tests in test/e2e/major_upgrade_v3.go rg -B 10 -A 10 "upgradeHeight|v3\.Version|upgrade.*delay" test/e2e/major_upgrade_v3.go # Look for upgrade height configuration in test setup rg -B 5 -A 5 "upgradeHeight.*:=|SetupTestAppWithUpgradeHeight" test/Length of output: 5804
app/test/square_size_test.go (2)
51-52: Explicit block time configuration added.The change from implicit timing to explicit
WithBlockTime(time.Second)makes the test behavior more deterministic and consistent.Let's verify if this change is part of a broader pattern:
#!/bin/bash # Search for similar timing configuration changes across test files rg -B 2 -A 2 'WithBlockTime\(time\.Second\)' 'app/test'
49-52: Verify alignment with v3 upgrade delay fix.This test configuration change appears tangential to the PR's main objective of fixing v3 upgrade delay issues. Please clarify if this timing change is necessary for the upgrade delay fix or if it's an independent test improvement.
Let's check for upgrade-related changes:
test/util/testnode/config.go (1)
85-91: LGTM: Well-structured block time configurationThe new
WithBlockTimemethod properly handles both the consensus timeout and app creator update, providing a complete solution for configuring block timing in tests.x/signal/keeper.go (1)
105-109: Verify the consensus-breaking change for upgrade delay calculationThe modification correctly implements the PR objective by using the current version's delay based on the chain ID. However, we should verify that this change maintains backward compatibility for "arabica" networks as specified in the PR objectives.
Let's verify the implementation of
UpgradeHeightDelayto ensure it handles chain IDs correctly:✅ Verification successful
Let me gather more information about the implementation and tests to verify this change thoroughly.
Let me get the complete implementation to verify the version-specific behavior.
The upgrade delay implementation correctly handles backward compatibility for arabica networks
The code change is correct and properly maintains backward compatibility. Here's why:
The
UpgradeHeightDelayimplementation has special handling for arabica networks:
- For v1: Uses v1 delay (50,400 blocks) for all chains
- For v2: Uses v3 delay (100,800 blocks) specifically for arabica chains, and v2 delay (50,400 blocks) for other chains
- For v3 and default: Uses v3 delay (100,800 blocks)
The modification in
x/signal/keeper.gousingheader.ChainIDandheader.Version.Appensures the correct delay is applied based on both the chain ID and current version, which aligns with the implementation's behavior.The comprehensive test cases in
versioned_consts_test.goverify this behavior, including the special case for arabica networks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the UpgradeHeightDelay implementation for different chain IDs # Expected: Different behavior for arabica vs non-arabica chains # Search for the UpgradeHeightDelay implementation echo "Searching for UpgradeHeightDelay implementation..." ast-grep --pattern 'func UpgradeHeightDelay($chainID string, $version uint64)' # Search for any tests verifying this behavior echo "Searching for related tests..." rg -l "UpgradeHeightDelay.*arabica"Length of output: 339
Script:
#!/bin/bash # Get the implementation of UpgradeHeightDelay echo "=== UpgradeHeightDelay Implementation ===" rg -A 10 "func UpgradeHeightDelay" pkg/appconsts/ # Get the test implementation echo -e "\n=== Test Implementation ===" cat pkg/appconsts/versioned_consts_test.go # Check for any other usages of this function echo -e "\n=== Other Usages ===" rg "UpgradeHeightDelay\(" --type goLength of output: 6246
Script:
#!/bin/bash # Get the complete UpgradeHeightDelay implementation echo "=== Complete UpgradeHeightDelay Implementation ===" rg -B 5 -A 20 "func UpgradeHeightDelay" pkg/appconsts/versioned_consts.go # Get the version-specific constants echo -e "\n=== Version-specific Constants ===" echo "v1 Constants:" rg "UpgradeHeightDelay.*=" pkg/appconsts/v1/ echo -e "\nv2 Constants:" rg "UpgradeHeightDelay.*=" pkg/appconsts/v2/ echo -e "\nv3 Constants:" rg "UpgradeHeightDelay.*=" pkg/appconsts/v3/Length of output: 1873
app/test/upgrade_test.go (3)
144-146: Verify the block height calculation for v3The block height is correctly calculated using v3's upgrade delay with the chain ID. This is crucial for testing the upgrade behavior.
Let's verify the version consistency:
#!/bin/bash # Check if the App version in the header matches the constant rg -B 2 -A 2 "Version: tmversion.Consensus{App: 3}"
155-155:⚠️ Potential issueVerify the final end block height calculation
The end block height calculation correctly uses v3's upgrade delay with the chain ID. However, for consistency with the previous suggestion:
Consider reusing the height from the BeginBlock:
- RequestEndBlock{Height: initialHeight + appconsts.UpgradeHeightDelay(testApp.GetChainID(), v3.Version)}) + RequestEndBlock{Height: initialHeight + appconsts.UpgradeHeightDelay(testApp.GetChainID(), v3.Version)})Likely invalid or redundant comment.
109-109: Verify the upgrade delay calculation in the loop conditionThe loop now correctly uses the chain ID to determine the upgrade delay for v2. This aligns with the PR objective to fix the upgrade delay calculation.
Let's verify that this is the only loop using UpgradeHeightDelay:
✅ Verification successful
Let me try a different approach to verify the usage of UpgradeHeightDelay in loops.
Based on the search results, I can now provide a final response:
The upgrade delay calculation in the loop is correctly implemented
The usage of
UpgradeHeightDelaywith chain ID parameter is consistent with its implementation and other usages across the codebase. The function properly handles different versions and has special logic for the Arabica chain to fix the known bug. Key findings:
- The implementation in
pkg/appconsts/versioned_consts.goshows the fix for the Arabica chain bug where it was incorrectly using the next version's delay- The test file correctly uses the chain ID parameter in all four locations where
UpgradeHeightDelayis called- The function is well-tested with comprehensive test cases in
pkg/appconsts/versioned_consts_test.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other loops using UpgradeHeightDelay to ensure consistent usage rg -B 2 -A 2 "for.*UpgradeHeightDelay"Length of output: 295
Script:
#!/bin/bash # Search for all occurrences of UpgradeHeightDelay to analyze its usage in loops rg -B 3 -A 3 "UpgradeHeightDelay"Length of output: 12262
x/signal/keeper_test.go (2)
429-429: LGTM: Consistent with the upgrade delay changesThe test assertion correctly verifies that the upgrade height delay is calculated using the chain ID parameter.
186-186: Verify test coverage for chain-specific upgrade delaysThe test only covers the "test" chain ID case. Given that the PR objectives mention different behavior for "arabica" chain IDs, we should add test cases for both scenarios.
Let's verify if there are other test files covering the arabica chain ID case:
There was a problem hiding this comment.
Looks good. Just a few minor nits:
We can do this in a follow up PR, but it might be nice if we change:
- arabica-11 testnet having a 2 day upgrade instead of a week
- having a special test chain-id that we can use for e2e tests which has no upgrade delay.
Also note that I believe this is the first time that the chainID affects the state machine
test/util/testnode/config.go
Outdated
| // WithBlockTime sets the block time and returns the Config. | ||
| func (c *Config) WithBlockTime(d time.Duration) *Config { | ||
| c.TmConfig.Consensus.TimeoutCommit = d | ||
| creator := DefaultAppCreator(d) | ||
| c.WithAppCreator(creator) | ||
| return c | ||
| } |
There was a problem hiding this comment.
Can we not still call this TimeoutCommit because it changes the TimeoutCommit not the BlockTime
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
pkg/appconsts/versioned_consts.go (1)
82-82: Add documentation for the new chainID parameterThe signature change is well-justified to fix the upgrade delay bug. However, the function's documentation should be updated to explain the chainID parameter's purpose and its impact on the upgrade delay calculation.
pkg/appconsts/versioned_consts_test.go (1)
122-124: Use consistent version format.Replace the hardcoded version number with the constant for consistency and maintainability.
- version: 3, + version: v3.Version,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- pkg/appconsts/chain_ids.go (1 hunks)
- pkg/appconsts/versioned_consts.go (2 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/appconsts/chain_ids.go
🔇 Additional comments (2)
pkg/appconsts/versioned_consts.go (1)
94-99: Verify the ArabicaChainID constant definitionThe special case handling for Arabica chain looks correct, but we should verify the exact value of
ArabicaChainIDconstant to ensure precise matching.Additionally, consider:
- Using a more specific chain ID match (e.g., "arabica-11") as suggested in previous reviews
- Adding case-insensitive comparison for more robust chain ID matching
✅ Verification successful
ArabicaChainID constant is correctly defined and used
The constant
ArabicaChainIDis already defined as "arabica-11" which matches the previous review's suggestion. The special case handling in the code is using the exact string constant, making the comparison precise and secure. No changes are needed as both recommendations from the previous review are already implemented:
- Using specific chain ID "arabica-11" ✓
- Direct string constant comparison ✓
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ArabicaChainID constant definition rg -l "ArabicaChainID.*=.*"Length of output: 56
Script:
#!/bin/bash # Get the actual definition and context of ArabicaChainID rg "ArabicaChainID" pkg/appconsts/chain_ids.go -A 2 -B 2Length of output: 98
pkg/appconsts/versioned_consts_test.go (1)
128-133: LGTM: Well-structured test implementation.The test implementation follows good practices with table-driven tests, clear naming, and proper use of subtests.
Co-authored-by: Callum Waters <[email protected]>
…zyledger-app into evan/fix-v3-upgrade-delay
|
| | Parameter | Default | Summary | Changeable via Governance | | ||
| |-------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------| | ||
| | MaxBlockSizeBytes | 100MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False | | ||
| | MaxSquareSize | 128 | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False | | ||
| | Parameter | Default | Summary | Changeable via Governance | | ||
| |--------------------|---------|------------------------------------------------------------------------------------------------------------------------|---------------------------| | ||
| | MaxBlockSizeBytes | 100MiB | Hardcoded value in CometBFT for the protobuf encoded block. | False | | ||
| | MaxSquareSize | 128 | Hardcoded maximum square size determined per shares per row or column for the original data square (not yet extended). | False | | ||
| | UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False | |
There was a problem hiding this comment.
added the version const to the spec, but there are a lot of versioned consts unrelated to this PR missing here
test/util/testnode/config.go
Outdated
| // WithBlockTime sets the block time and returns the Config. | ||
| func (c *Config) WithBlockTime(d time.Duration) *Config { | ||
| c.TmConfig.Consensus.TimeoutCommit = d | ||
| creator := DefaultAppCreator(d) | ||
| c.WithAppCreator(creator) | ||
| return c | ||
| } |
There was a problem hiding this comment.
You deprecated the method WithTimeoutComimt and created a new method called WithBlockTime. My comment was why not just modify the WithTimeoutCommit? WithBlockTime is misleading because it doesn't change the block time to that duration, just the timeout commit.
test/util/testnode/config.go
Outdated
| @@ -171,7 +181,7 @@ func DefaultTendermintConfig() *tmconfig.Config { | |||
| return tmCfg | |||
| } | |||
|
|
|||
| func DefaultAppCreator() srvtypes.AppCreator { | |||
| func DefaultAppCreator(blockTime time.Duration) srvtypes.AppCreator { | |||
There was a problem hiding this comment.
Can we use the options paradigm to modify this. To me, this isn't really a default anymore if you need to specify the blockTime.
We can just have something like:
type AppCreationOptions func (app *App)
func WithTimeoutCommit(d time.Duration) {
return func (app *App) {
app.SetEndBlocker(wrapEndBlocker, app, d)
}
}
func DefaultAppCreator(AppCreationOptions...) srvtypes.AppCreator {
...
}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
specs/src/parameters_v2.md (1)
15-15: Enhance the parameter description and fix hyphenation.
The description "Height based delay after a successful MsgTryUpgrade has been submitted" could be more detailed given this parameter's critical role in upgrade timing. Also, "Height based" should be hyphenated.
Consider this enhanced description:
-| UpgradeHeightDelay | 50400 | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
+| UpgradeHeightDelay | 50400 | Height-based delay (in blocks) that must pass after a successful `MsgTryUpgrade` before the upgrade is applied. This delay provides network participants adequate time to prepare for the upgrade. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 50400 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md (2)
15-15: Add hyphen to "Height-based" in parameter description
For consistency and correct grammar, the description should use a hyphen.
-| UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade` has been submitted. | False |
+| UpgradeHeightDelay | 100800 | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
15-15: Enhance parameter documentation
The description should provide more context about the parameter's purpose and implications.
-| UpgradeHeightDelay | 100800 | Height-based delay after a successful `MsgTryUpgrade` has been submitted. | False |
+| UpgradeHeightDelay | 100800 | Height-based delay (approximately 7 days at 6s block times) that must pass after a successful `MsgTryUpgrade` before the upgrade can be applied. This delay provides network participants adequate time to prepare for the upgrade. | False |🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/test/square_size_test.go (1 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
- specs/src/parameters_v2.md (1 hunks)
- specs/src/parameters_v3.md (1 hunks)
- test/util/testnode/config.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/test/square_size_test.go
🧰 Additional context used
🪛 LanguageTool
specs/src/parameters_v2.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 50400 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
specs/src/parameters_v3.md
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
🔇 Additional comments (3)
pkg/appconsts/versioned_consts_test.go (1)
84-129: Well-structured test implementation!
The table-driven test approach is clean and the test cases are well-named. The implementation properly verifies the version-specific upgrade delays and the known arabica bug for v2.
test/util/testnode/config.go (1)
Line range hint 174-199: Verify wrapEndBlocker implementation
The implementation looks good and follows the functional options pattern. However, the wrapEndBlocker function is used but not visible in this file.
Let's verify the implementation of wrapEndBlocker:
✅ Verification successful
Let me check the implementation of wrapEndBlocker to ensure it's properly handling the timeout commit functionality.
Implementation of wrapEndBlocker is correct and properly handles timeout commit duration
The wrapEndBlocker function correctly wraps the app's EndBlocker to set the timeout commit duration. It preserves the original EndBlocker functionality while adding the timeout configuration through resp.Timeouts.TimeoutCommit.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wrapEndBlocker implementation
rg -n "func wrapEndBlocker" --type go
Length of output: 211
Script:
#!/bin/bash
# Get the implementation of wrapEndBlocker with context
rg -A 10 "func wrapEndBlocker" --type go
Length of output: 690
specs/src/parameters_v3.md (1)
15-15: Verify upgrade delay value across environments
The value of 100800 blocks (~7 days at 6s block times) aligns with other governance timeframes in the spec. However, let's verify this value is consistently used across different chain environments.
✅ Verification successful
Upgrade delay values are version-specific and consistently defined
The upgrade delay values are properly defined and consistently used across different versions:
- v1 and v2: 50,400 blocks (7 days at 12s block times)
- v3: 100,800 blocks (7 days at 6s block times)
The change in block count between versions is due to the adjustment in block times (from 12s to 6s), maintaining the same 7-day delay period. The system also includes:
- A versioned constants system that handles different delays per version
- Override capabilities for testing/emergency situations via environment variables
- Proper test coverage validating the version-specific delays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UpgradeHeightDelay usage across the codebase
echo "Searching for UpgradeHeightDelay constant definitions and usage..."
rg -A 2 "UpgradeHeightDelay.*=.*100800"
# Search for any other upgrade delay related constants
echo "Checking for other upgrade delay related constants..."
rg -A 2 "upgrade.*delay.*=.*\d+"
Length of output: 336
Script:
#!/bin/bash
# Search for UpgradeHeightDelay in a case-insensitive manner and with more flexible pattern
echo "Searching for upgrade height delay references..."
rg -i "upgradeheightdelay|upgrade.*height.*delay" -A 2
# Search for the specific number 100800 to catch any related constants
echo "Searching for the specific delay value..."
rg "100800" -A 2
# Search for upgrade-related parameters in markdown files
echo "Searching upgrade parameters in markdown..."
fd -e md -x rg -l "upgrade.*parameter|parameter.*upgrade"
Length of output: 10792
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This expression is usually spelled with a hyphen.
Context: ... | | UpgradeHeightDelay | 100800 | Height based delay after a successful `MsgTryUpgrade...
(BASED_HYPHEN)
Overview
Currently, there's a bug in v3 that will use the next version's upgrade delay instead of the current version's.
For non-arabica networks, this PR introduces a consensus breaking change that fixes this issue. For arabica networks, they will continue to use the next version's delay.