feat!: add a delay between quorum and upgrade height #3560
Conversation
rootulp
requested review from
ramin,
rach-id,
cmwaters and
evan-forbes
and removed request for
a team,
ramin and
rach-id
WalkthroughWalkthroughThe recent changes introduce a delay mechanism for network upgrades and enhancements to the signal module. Key improvements include adding context-based conditional checks for version upgrading, integrating new query methods, and providing better error handling and unit test coverage to ensure robust upgrade processing. Changes
Sequence DiagramsequenceDiagram
participant User
participant SignalModule
participant UpgradeScheduler
User->>SignalModule: MsgTryUpgrade
SignalModule->>UpgradeScheduler: Validate Upgrade
UpgradeScheduler-->>SignalModule: Schedule Upgrade (with delay)
SignalModule-->>User: Acknowledge Upgrade
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Given the upgrade process now spans across multiple heights we need to be aware of node crashes, thus the signal keeper will need to persist the upgrade height and the quorum version to state.
I think it would also be helpful if we expose a grpc query for that upgrade height so users are able to know when exactly that upgrade height is.
Breaks TestResetTally I think because the current keeper iterates over all keys and values and delete them. I think we want to find a way to constrain the keys and values that validator signals are in. Or separate those signals from the key used to persist the upgrade.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/signal/keeper.go
Outdated
| // DefaultUpgradeHeightDelay is the number of blocks after a quorum has been | ||
| // reached that the chain should upgrade to the new version. Assuming a | ||
| // block interval of 12 seconds, this is 48 hours. | ||
| const DefaultUpgradeHeightDelay = int64(3 * 7 * 24 * 60 * 60 / 12) // 3 weeks * 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 151,200 blocks. |
There was a problem hiding this comment.
Clarify the calculation of DefaultUpgradeHeightDelay.
Consider adding a comment to explain the calculation of DefaultUpgradeHeightDelay in more detail, especially how the block interval and total time are used to derive the number of blocks.
// DefaultUpgradeHeightDelay calculates the number of blocks needed for a 3-week delay, assuming a block interval of 12 seconds.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DefaultUpgradeHeightDelay is the number of blocks after a quorum has been | |
| // reached that the chain should upgrade to the new version. Assuming a | |
| // block interval of 12 seconds, this is 48 hours. | |
| const DefaultUpgradeHeightDelay = int64(3 * 7 * 24 * 60 * 60 / 12) // 3 weeks * 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 151,200 blocks. | |
| // DefaultUpgradeHeightDelay calculates the number of blocks needed for a 3-week delay, assuming a block interval of 12 seconds. | |
| const DefaultUpgradeHeightDelay = int64(3 * 7 * 24 * 60 * 60 / 12) // 3 weeks * 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 151,200 blocks. |
| // DefaultUpgradeHeightDelay is the number of blocks after a quorum has been | ||
| // reached that the chain should upgrade to the new version. Assuming a block | ||
| // interval of 12 seconds, this is 7 days. | ||
| const DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks. |
There was a problem hiding this comment.
Clarify the calculation of DefaultUpgradeHeightDelay.
Consider adding a comment to explain the calculation of DefaultUpgradeHeightDelay in more detail, especially how the block interval and total time are used to derive the number of blocks.
// DefaultUpgradeHeightDelay calculates the number of blocks needed for a 7-day delay, assuming a block interval of 12 seconds.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DefaultUpgradeHeightDelay is the number of blocks after a quorum has been | |
| // reached that the chain should upgrade to the new version. Assuming a block | |
| // interval of 12 seconds, this is 7 days. | |
| const DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks. | |
| // DefaultUpgradeHeightDelay calculates the number of blocks needed for a 7-day delay, assuming a block interval of 12 seconds. | |
| const DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks. |
Co-authored-by: Sanaz Taheri <[email protected]>
Closes #3552
Testing
Manually tested the new CLI commands by staring single node script and then: