feat: decrease block time to 6s #3953
Conversation
| // 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. | ||
| DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks. | ||
| // interval of 6 seconds, this is 7 days. | ||
| DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 6) // 7 days * 24 hours * 60 minutes * 60 seconds / 6 seconds per block = 100,800 blocks. | ||
| ) |
There was a problem hiding this comment.
Hmm I think this is doable because users of the earlier version will still crash in any case when the state machine transitions to v3 (even if nodes in the network have different values of the DefaultUpgradeHeightDelay
There was a problem hiding this comment.
Isn't doable? Or is?
Thanks for the review!
jcstein
requested review from
rootulp and
staheri14
and removed request for
a team
| @@ -9,5 +9,5 @@ const ( | |||
| // interval isn't enforced at consensus, the real block interval isn't | |||
| // guaranteed to exactly match GoalBlockTime. GoalBlockTime is currently targeted | |||
| // through static timeouts (i.e. TimeoutPropose, TimeoutCommit). | |||
| GoalBlockTime = time.Second * 15 | |||
| GoalBlockTime = time.Second * 6 | |||
There was a problem hiding this comment.
I'm curious if this will cause block time to be less than 6s? As before it was 15, but block times average around 12s on all networks.
There was a problem hiding this comment.
Decreasing the value for this constant won't actually lower block times. It looks like this constant is used in 3 places:
- To determine Mempool TTLDuration
- To determine Evidence MaxAgeNumBlocks
- To determine x/ibc MaxExpectedTimePerBlock
Instead the timeouts that need to be decreased to modify the block time are TimeoutPropose and TimeoutCommit which Sanaz is working on in #3882
There was a problem hiding this comment.
thank you for following up here. I think then it may be safe to close this PR?
and do these need to be modified in #3882?
- To determine Mempool TTLDuration
- To determine Evidence MaxAgeNumBlocks
- To determine x/ibc MaxExpectedTimePerBlock
There was a problem hiding this comment.
sweet, thanks for the help and context.
📝 WalkthroughWalkthroughThe changes involve modifications to block timing and genesis state settings across three files. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IBCModule
participant ConsensusConsts
User->>IBCModule: Request DefaultGenesis
IBCModule->>ConsensusConsts: Retrieve GoalBlockTime
ConsensusConsts-->>IBCModule: Return GoalBlockTime (6 seconds)
IBCModule->>User: Return updated genesis state
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
|
![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)
pkg/appconsts/consensus_consts.go (1)
12-12: Consider adjusting related timeout values.While
GoalBlockTimehas been reduced,TimeoutProposeandTimeoutCommitremain unchanged. These values might need adjustment to optimize for the new target block time.Consider reviewing and possibly adjusting
TimeoutProposeandTimeoutCommitto ensure they are appropriate for the new 6-second block time target. This may involve proportionally reducing these timeouts or conducting performance tests to determine optimal values.pkg/appconsts/global_consts.go (1)
32-33: Approved change, but consider adding a compatibility noteThe update to
DefaultUpgradeHeightDelaycorrectly reflects the new 6-second block interval while maintaining the 7-day upgrade delay period. This aligns well with the PR objective of decreasing block time to 6 seconds.However, based on the previous review comments, there might be compatibility issues with earlier versions. Consider adding a comment to highlight this:
// interval of 6 seconds, this is 7 days. +// Note: This change may cause incompatibility with nodes running earlier versions. DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 6) // 7 days * 24 hours * 60 minutes * 60 seconds / 6 seconds per block = 100,800 blocks.app/default_overrides.go (1)
Align Time-Related Parameters with Unbonding Time
The current settings for
SignedBlocksWindowandDowntimeJailDurationmay not align appropriately with the definedDefaultUnbondingTimeof 3 weeks:
- SignedBlocksWindow: Set to 5000 blocks (~8.3 hours). This is significantly shorter than the 3-week unbonding period and may lead to inconsistencies in validator state management.
- DowntimeJailDuration: Set to 1 minute, which may be insufficient for handling validator downtime effectively.
Recommendations:
Adjust
SignedBlocksWindow:
- Increase the value to reflect the 3-week
DefaultUnbondingTime. For example, calculating the number of blocks in 3 weeks:3 weeks * 7 days/week * 24 hours/day * 60 minutes/hour * 60 seconds/minute / 6 seconds/block ≈ 30240 blocks- Update
params.SignedBlocksWindowaccordingly to ensure consistency.Modify
DowntimeJailDuration:
- Consider increasing the duration to provide validators with sufficient time to recover from downtime events. A longer duration can prevent premature jailing and enhance network stability.
Implementing these adjustments will help maintain consistency across time-related parameters and ensure the intended behavior of the staking and slashing modules.
🔗 Analysis chain
Line range hint
1-324: Consider reviewing time-related parameters in other modulesWhile the block time change is correctly implemented in the
ibcModule, it's important to consider its potential impact on other modules. Specifically, thestakingModuleandslashingModulehave time-related parameters that might need adjustment to align with the new block time:
- In the
stakingModule:
params.UnbondingTimeis set toappconsts.DefaultUnbondingTime- In the
slashingModule:
params.SignedBlocksWindowis set to 5000params.DowntimeJailDurationis set to 1 minutePlease review these parameters to ensure they are still appropriate with the new 6-second block time. You may want to run the following script to check the current values of these constants:
Consider adjusting these values if necessary to maintain the intended behavior of these modules with the new block time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check current values of time-related constants echo "Checking DefaultUnbondingTime:" rg --type go "DefaultUnbondingTime.*=" echo "Checking SignedBlocksWindow and DowntimeJailDuration:" rg --type go "SignedBlocksWindow.*=|DowntimeJailDuration.*="Length of output: 501
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/default_overrides.go (1 hunks)
- pkg/appconsts/consensus_consts.go (1 hunks)
- pkg/appconsts/global_consts.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
pkg/appconsts/consensus_consts.go (1)
12-12: Investigate potential for sub-6 second block times.As noted in a previous comment by jcstein, there's a possibility that this change could result in block times less than 6 seconds, given that the previous 15-second target resulted in ~12-second actual block times.
To better understand the relationship between
GoalBlockTimeand actual block times, please provide data from test networks or simulations showing:
- The distribution of actual block times with the new 6-second target.
- Any potential impacts on network stability or performance with shorter block times.
This information will help ensure that the change meets the intended goals without introducing new issues.
app/default_overrides.go (1)
Line range hint
146-152: LGTM: Block time adjustment implemented correctly.The changes to the
DefaultGenesisfunction for theibcModulecorrectly implement the reduction in block time from 15 seconds to 6 seconds. ThemaxBlockTimeis now set to 5 times theappconsts.GoalBlockTime, which aligns with the IBC documentation recommendation of 3-5 times the expected block time.To ensure this change doesn't negatively impact other parts of the system, please run the following verification script:
This script will help identify any other parts of the codebase that might need adjustment due to the change in block time.
✅ Verification successful
Verified: Block time adjustment does not affect other parts of the codebase.
All references to the old block time are confined to test files and do not impact the production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of block time assumptions in the codebase # Search for references to the old block time (15 seconds) echo "Checking for references to old block time (15 seconds):" rg --type go "15.*second" # Search for other block time related constants or variables echo "Checking for other block time related constants or variables:" rg --type go "block.*time|time.*block" # Search for GoalBlockTime usage echo "Checking for GoalBlockTime usage:" rg --type go "GoalBlockTime"Length of output: 6723
| @@ -9,5 +9,5 @@ const ( | |||
| // interval isn't enforced at consensus, the real block interval isn't | |||
| // guaranteed to exactly match GoalBlockTime. GoalBlockTime is currently targeted | |||
| // through static timeouts (i.e. TimeoutPropose, TimeoutCommit). | |||
| GoalBlockTime = time.Second * 15 | |||
| GoalBlockTime = time.Second * 6 | |||
There was a problem hiding this comment.
💡 Codebase verification
Remaining references to 15-second block time found.
- Files and locations to update:
x/mint/abci_test.go:blockInterval := time.Second * 15x/mint/test/mint_test.go:previous := current.Add(-time.Second * 15)test/txsim/run_test.go:name: "one 15 second block during the first year"
Please revise these instances to reflect the new GoalBlockTime of 6 seconds to ensure consistency across the codebase.
🔗 Analysis chain
Change aligns with PR objective, but consider broader impacts.
The reduction of GoalBlockTime from 15 seconds to 6 seconds aligns with the PR objective to decrease block time. However, this significant change may have broader implications on the system.
To ensure this change doesn't introduce unexpected behaviors, please run the following verification:
Review the results to ensure that all relevant parts of the codebase are updated to accommodate the new block time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded assumptions about block time in the codebase
# Search for any mentions of block time in seconds
rg --type go "(?i)(block\s*time|block\s*interval).*?(\d+)\s*(s|second)"
# Search for any time durations that might be related to block time
rg --type go "time\.(Second|Minute|Hour)\s*\*\s*\d+"
# Search for any mentions of the old block time (15 seconds)
rg --type go "15\s*(s|second)"
Length of output: 1323
Overview
Demo of decreasing block time to 6s, from 12s.