coderabbitai[bot]
commented
4 weeks ago 📝 Walkthrough
## Walkthrough The changes in this pull request primarily focus on enhancing the upgrade logic and timeout configurations within the application. Modifications include updating the `UpgradeHeightDelay` function to incorporate a `chainID`, adjusting test cases to reflect these changes, and introducing new parameters for upgrade height delays. Additionally, deprecated methods have been replaced with more flexible configurations for block timing. Overall, the changes aim to improve the handling of version upgrades and their associated parameters in the testing environment. ## Changes | File Path | Change Summary | |---------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | app/test/square_size_test.go | Modified `SetupSuite` in `SquareSizeIntegrationTest` to add a timeout commit configuration with `WithTimeoutCommit(time.Second)`. | | app/test/upgrade_test.go | Updated `TestAppUpgradeV3` to calculate upgrade height delay using `chainID` in calls to `appconsts.UpgradeHeightDelay`. Adjusted assertions related to timeout parameters. | | pkg/appconsts/versioned_consts.go | Updated `UpgradeHeightDelay` function signature to include `chainID`. Adjusted switch statement for `v2.Version` to return `v3.UpgradeHeightDelay` for `ArabicaChainID`. Added `strings` import. | | pkg/appconsts/versioned_consts_test.go | Added `TestUpgradeHeightDelay` to validate `UpgradeHeightDelay` behavior with various scenarios, including chain IDs and version-specific behavior. | | test/util/testnode/config.go | Introduced `WithBlockTime` method to set block time, deprecated `WithTimeoutCommit`. Updated `DefaultAppCreator` to accept multiple options. | | x/signal/integration_test.go | Modified `TestUpgradeIntegration` to use `chainID` in `appconsts.UpgradeHeightDelay` calls, affecting block height calculations. | | x/signal/keeper.go | Updated `TryUpgrade` method to calculate `UpgradeHeight` using both `ChainID` and `App` version. | | x/signal/keeper_test.go | Updated test logic to reflect new `UpgradeHeightDelay` method signature, adjusting block height calculations and context initialization. | | pkg/appconsts/chain_ids.go | Added new constant `ArabicaChainID` with value `"arabica-11"`. | | specs/src/parameters_v2.md | Updated to include new global parameter `UpgradeHeightDelay` with a default value of `50400`. | | specs/src/parameters_v3.md | Updated to include new global parameter `UpgradeHeightDelay` with a default value of `100800`. | ## Possibly related PRs - **#3560**: Introduces a delay between quorum and upgrade height, relating to timeout behavior for block commits. - **#3882**: Focuses on versioned timeouts, updating timeout settings based on application version. - **#3980**: Discusses modifications to `UpgradeHeightDelay` and its integration into application logic. - **#3910**: Implements an end-to-end upgrade test for version 3, potentially involving similar timeout configurations. - **#4006**: Adds notes about the upgrade process for validators, including considerations for timeout settings during upgrades. ## Suggested labels `enhancement` ## Suggested reviewers - staheri14 - ninabarbakadze - rootulp - cmwatersThank 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?
github-actions[bot]
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.