chore: overwrite default cometbft timeout_commit
#438
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmd
participant Config
User->>Cmd: Execute command
Cmd->>Config: Load default configuration
Config-->>Cmd: Return config
Cmd->>Config: Set TimeoutCommit to 500ms
Cmd->>User: Command execution complete
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/root.go (1)
75-77: Consider improving maintainabilityWhile the implementation is correct, consider these improvements:
- Add a comment explaining why 500ms was chosen
- Consider making the timeout value a constant for easier future modifications
+// DefaultTimeoutCommit represents the standard block commit timeout across validators +const DefaultTimeoutCommit = 500 * time.Millisecond + // overwrite default timeout_commit from the cometbft configuration cmtCfg := cmtcfg.DefaultConfig() -cmtCfg.Consensus.TimeoutCommit = (500 * time.Millisecond) +// Set to 500ms to align with existing validator configurations +cmtCfg.Consensus.TimeoutCommit = DefaultTimeoutCommit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/root.go(2 hunks)local.sh(0 hunks)
💤 Files with no reviewable changes (1)
- local.sh
🔇 Additional comments (2)
cmd/root.go (2)
18-18: LGTM: Required import for duration configuration
The addition of the time package is necessary for setting the TimeoutCommit duration.
75-77: Implementation aligns with PR objective
The change correctly implements the 500ms TimeoutCommit configuration, matching the existing validator configurations. However, let's verify this change doesn't affect other parts of the system.
✅ Verification successful
The previous command failed because it was restricted to .toml files which might not exist. Let's broaden the search to all files and also check what config files exist in the repository.
Let's verify if there are any related timeout configurations in the app.yaml or other consensus timeout settings that might be affected by this change.
TimeoutCommit configuration change is safe to implement
The verification shows that:
- The TimeoutCommit configuration is only set in cmd/root.go
- No conflicting timeout configurations exist in app.yaml or other configuration files
- No other consensus timeout settings will be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other timeout_commit configurations that might conflict
rg -i "timeout.?commit" --type toml
Length of output: 186
Script:
#!/bin/bash
# Search for any timeout_commit configurations in all files
rg -i "timeout.?commit"
# Also check for any config files that might have different extensions
fd -e yaml -e toml -e json -e conf
Length of output: 227
Script:
#!/bin/bash
# Let's check the content of app.yaml for any timeout configurations
cat app.yaml
# Also search for any other consensus timeout related configurations
rg -i "consensus.*timeout"
Length of output: 4135
johnletey
changed the title
timeout_committimeout_commit
Co-authored-by: John Letey <john@noble.xyz>
This PR overwrites the cometbft default
timeout_commitfrom5sto500ms.You will be able to see this change take effect when initializing Noble (
nobled init).For context,
500msis what the current Noble validator set has manually adjusted in theconfig.tomlfile.