chore: default TimeoutCommit to 3s (allow opt out of default overrides) #7769
Conversation
|
Warning Rate Limit Exceeded@czarcas7ic has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update, version Changes
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 (
|
czarcas7ic
added
V:state/compatible/backport
czarcas7ic
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- cmd/osmosisd/cmd/init.go (2 hunks)
- cmd/osmosisd/cmd/root.go (3 hunks)
Additional comments: 5
cmd/osmosisd/cmd/init.go (2)
- 43-44: The introduction of the
FlagRejectConfigDefaultsflag is a significant change, providing flexibility for node operators to opt out of default overrides. This change aligns with the PR objectives to allow customization and should be clearly documented in the user guide or help command to ensure users are aware of how to use this new flag effectively.- 115-116: Adjusting the
TimeoutCommitvalue from 4 seconds to 3 seconds is aimed at achieving faster block times, which is a critical performance optimization. However, it's essential to ensure that this change does not introduce any unintended side effects, such as increased risk of forks or decreased network stability, especially in networks with higher latencies. It would be beneficial to conduct thorough testing or simulations to assess the impact of this change under various network conditions.cmd/osmosisd/cmd/root.go (2)
- 443-443: The change to set
TimeoutCommitto 3 seconds aligns with the PR's objective to optimize block commit times. However, it's crucial to ensure that this new default does not negatively impact the network's stability or performance under different conditions.- 700-722: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [703-744]
The introduction of the
FlagRejectConfigDefaultsflag provides flexibility for node operators to opt out of default overrides, including the newTimeoutCommitvalue. This is a positive change that respects operator autonomy. However, it's important to document this flag clearly in the project's documentation to ensure operators are aware of how to use it.Consider adding detailed documentation or comments in the code explaining the purpose and usage of the
FlagRejectConfigDefaultsflag to improve user understanding.CHANGELOG.md (1)
- 73-73: The entry for the new feature in version
v23.0.8-iavl-v1 & v23.0.8is clear and concise, detailing the addition of a default timeout commit of 3 seconds and the introduction of a flag to prevent custom overrides. This change is significant as it directly impacts the blockchain's performance by potentially optimizing block times.
| // Add ctx logger line to indicate that config.toml and app.toml values are being overwritten | ||
| serverCtx.Logger.Info("Overwriting config.toml and app.toml values with some recommended defaults. To prevent this, set the --reject-config-defaults flag to true.") | ||
|
|
||
| err := overwriteConfigTomlValues(serverCtx) |
There was a problem hiding this comment.
AFAIR, there was also a code branch somewhere that would check if the old value is 5 seconds and only then overwrite it. Have we checked/changed that value?
I'm AFK rn but can help with checking this later
There was a problem hiding this comment.
Whoops yeah, I had that change locally when testing but didn't push, great catch
There was a problem hiding this comment.
Do we have to write ti the file, or can we pass the updated fike to tm? This lgtm!
Maybe worth asking in cometbft issue if we can achieve this w/o writing the file at runtime
This lgtm for this pr, nice job@
There was a problem hiding this comment.
I believe this can be done at the sdk level, I will make an issue and we can look into it at another time.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- cmd/osmosisd/cmd/root.go (4 hunks)
Additional comments: 2
cmd/osmosisd/cmd/root.go (2)
- 742-742: The addition of the
FlagRejectConfigDefaultsflag is well-implemented and aligns with the PR objectives to allow opting out of default overrides. Good job on providing a clear description for the flag.- 443-443: The logic to set the default
TimeoutCommitto 3 seconds and to conditionally overwriteconfig.tomlandapp.tomlvalues based on therejectConfigDefaultsflag is correctly implemented. This change is crucial for achieving the PR's objectives of optimizing block commit times and providing flexibility in configuration management.Also applies to: 469-470, 701-717
…s) (backport #7769) (#7781) * chore: default TimeoutCommit to 3s (allow opt out of default overrides) (#7769) * overwrite with defaults flag * add logic gate for overriding * add log line * make verbiage clearer * add changelog * dont check for specific value * unused var (cherry picked from commit bf01cf0) # Conflicts: # CHANGELOG.md * Update CHANGELOG.md * reject config defaults * reject defaults --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
…s) (backport #7769) (#7783) * chore: default TimeoutCommit to 3s (allow opt out of default overrides) (#7769) * overwrite with defaults flag * add logic gate for overriding * add log line * make verbiage clearer * add changelog * dont check for specific value * unused var (cherry picked from commit bf01cf0) * reject config defaults * reject defaults --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
…s) (backport #7769) (#7782) * chore: default TimeoutCommit to 3s (allow opt out of default overrides) (#7769) * overwrite with defaults flag * add logic gate for overriding * add log line * make verbiage clearer * add changelog * dont check for specific value * unused var (cherry picked from commit bf01cf0) * reject conflig defaults * reject defaults --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Closes: #XXX
What is the purpose of the change
Sets and default the timeout commit to 3s.
Adds flag that allows node operators to opt out of custom overrides.
Testing and Verifying
Logs show when no flag is provided, log does not show when flag is set to true.
Summary by CodeRabbit