Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: decrease block time to 6s #3953

Closed
wants to merge 2 commits into from
Closed

feat: decrease block time to 6s #3953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bacaa84
feat: decrease block time to 6s
jcstein Oct 8, 2024
b6d3c1f
Merge branch 'main' into main
jcstein Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/default_overrides.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ type ibcModule struct {
// DefaultGenesis returns custom x/ibc module genesis state.
func (ibcModule) DefaultGenesis(cdc codec.JSONCodec) json.RawMessage {
// per ibc documentation, this value should be 3-5 times the expected block
// time. The expected block time is 15 seconds, therefore this value is 75
// time. The expected block time is 6 seconds, therefore this value is 30
// seconds.
maxBlockTime := appconsts.GoalBlockTime * 5
gs := ibctypes.DefaultGenesisState()
Expand Down
2 changes: 1 addition & 1 deletion pkg/appconsts/consensus_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@rootulp rootulp Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decreasing the value for this constant won't actually lower block times. It looks like this constant is used in 3 places:

  1. To determine Mempool TTLDuration
  2. To determine Evidence MaxAgeNumBlocks
  3. 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

Copy link
Member Author

@jcstein jcstein Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

  1. To determine Mempool TTLDuration
  2. To determine Evidence MaxAgeNumBlocks
  3. To determine x/ibc MaxExpectedTimePerBlock

Copy link
Collaborator

@rootulp rootulp Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea we should re-evaluate what those values should be. It doesn't have to strictly happen in #3882 but let's create a follow-up to tackle that. Thanks!

Update: created #3959

Agreed safe to close this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, thanks for the help and context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remaining references to 15-second block time found.

  • Files and locations to update:
    • x/mint/abci_test.go: blockInterval := time.Second * 15
    • x/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

)
4 changes: 2 additions & 2 deletions pkg/appconsts/global_consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ const (

// 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.
)
Comment on lines 30 to 34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't doable? Or is?

Thanks for the review!


var (
Expand Down
Loading