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.

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(abci): move timeout_commit into FinalizeBlockResponse #22

Open
wants to merge 3 commits into
base: bera-v1.x
Choose a base branch
from

Conversation

melekes
Copy link

@melekes melekes commented Feb 11, 2025

Port of cometbft#3089

@melekes melekes changed the base branch from v1.x-bera to bera-v1.x February 12, 2025 04:40
melekes and others added 2 commits February 12, 2025 08:44
…bft#3089)

as `next_block_delay`

ADR-115: cometbft#2966
Closes cometbft#2655

---

- [ ] ~~Tests written/updated~~
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Sergio Mena <[email protected]>
@melekes melekes force-pushed the 2655-predictable-block-times-impl branch from 239aa3f to 175a808 Compare February 12, 2025 04:44
// though we already have +2/3).
// NOTE: when modifying, make sure to update time_iota_ms genesis parameter
// Set to 0 if you want to make progress as soon as the node has all the precommits.
// Deprecated: use `next_block_delay` in the ABCI application's `FinalizeBlockResponse`.
TimeoutCommit time.Duration `mapstructure:"timeout_commit"`

Choose a reason for hiding this comment

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

I would leave these docs (with the "Deprecated:..." line as well), because in internal/consensus/state.go we set it if NextBlockDelay isn't set, and we also check its value when setting skipTimeoutCommit.

So someone might still want to know what this legacy value is.

Copy link
Author

Choose a reason for hiding this comment

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

The more we diverge from the upstream v1.x, the harder it will be to keep up with it.

# How long we wait after committing a block, before starting on the new
# height (this gives us a chance to receive some more precommits, even
# though we already have +2/3).
# Set to 0 if you want to make progress as soon as the node has all the precommits.
# Deprecated: use `next_block_delay` in the ABCI application's `FinalizeBlockResponse`.
timeout_commit = "{{ .Consensus.TimeoutCommit }}"

Choose a reason for hiding this comment

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

Same comment as above about the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants