feat(abci): move timeout_commit into FinalizeBlockResponse#3089
feat(abci): move timeout_commit into FinalizeBlockResponse#3089
timeout_commit into FinalizeBlockResponse#3089Conversation
as `next_block_delay` ADR-115: #2966
| // that execution of the transactions was deterministic. | ||
| // It is up to the application to decide which algorithm to use. | ||
| bytes app_hash = 5; | ||
| // delay between the time when this block is committed and the next height is started. |
There was a problem hiding this comment.
@andynog and I had a discussion last night about how we are modifying protobufs on main and/or v1.x. We have added a point to discuss this in next week's meeting, as it is complex and we'd like everyone in the team to be aware of it, and provide feedback.
If you want to know what this is all about, take a look at #3090.
This shouldn't be blocking this PR, it's just to raise awareness.
| // delay between the time when this block is committed and the next height is started. | ||
| // previously `timeout_commit` in config.toml | ||
| google.protobuf.Duration next_block_delay = 6 [ | ||
| (gogoproto.nullable) = false, |
There was a problem hiding this comment.
We may need nil here to achieve our upgrade path:
- if
nilthe app doesn't know about this (new) feature, fall back totimeout_commit - If non-nil and value is 0, don't wait any time (and ignore value of
timeout_commit)
What do you think?
There was a problem hiding this comment.
The current upgrade path should also work.
- if
0andtimeout_commit=0s, then skip timeout - if
0andtimeout_commitis not zero, then usetimeout_commit - if
not-0, then use it
There was a problem hiding this comment.
Yeah, I was thinking about this and I think you're right, no need to "leverage" nil here. This is my reasoning:
If the app (and app's community) is aware of this feature, they have to be aware of the whole feature (including its upgrade path!).
So if you are setting next_block_delay to 0, you have to make sure timeout_commit is also 0 (or absent from the config). If you don't do that, you can't blame us for bad UX.
Do you agree? If yes, then, this thread can be dismissed.
There was a problem hiding this comment.
Do you agree? If yes, then, this thread can be dismissed.
Exactly.
| proposer a chance to receive some more precommits, even though it | ||
| already has +2/3). Set to 0 if you want a proposer to make progress as | ||
| soon as it has all the precommits. Previously `timeout_commit` in | ||
| CometBFT config. **Set to constant 1s to preserve the old (v0.34 - v1.0) behavior**. |
There was a problem hiding this comment.
Set to constant 1s to preserve the old (v0.34 - v1.0) behavior
See my comments above about an operator -- oblivious to this -- that wants to upgrade, and has e.g. their timeout_commit set to 0 (or to 3).
Also see my comment above about the possibility of using nil in this field to allow an app to signal "I'm not aware of this, please fall back to timeout_commit"
There was a problem hiding this comment.
Update: I'm now convinced we don't need to use nil here. But the first sentence of my comment above still applies
Co-authored-by: Sergio Mena <sergio@informal.systems>
timeout_commit into FinalizeBlockResponsetimeout_commit into FinalizeBlockResponse
|
@cason please review once you're back 🙏 |
…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 <sergio@informal.systems>
…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 <sergio@informal.systems>
cason
left a comment
There was a problem hiding this comment.
Just triple-checking, from an orthogonal view looks good.
|
What I maybe miss, from a documentation point of view, is to make it clear that is the commit_time that is considered to "start" the configured delay. Not when |
|
@cason thanks for triple-checking. Regarding your last comment? If you create a PR with the improved doc, I can take a look and approve |
|
@sergio-mena, here: #3542 |
|
Where do we set the next_block_delay? Is it done in the application and returned to cometBFT? |
Remember, it's not in |
|
Thanks @melekes for your quick reply. I am trying to cherry pick these changes as I want const block time. Btw is it safe to do so as I can see there is no consensus breaking change. My question is who set the next_block_delay in FinalizeBlockResponse? |
as `next_block_delay` ADR-115: #2966 Closes #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 <sergio@informal.systems>
using FinalizeBlockResponse.NextBlockDelay feature cometbft/cometbft#3089
…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 <sergio@informal.systems> (cherry picked from commit 259793a)
as
next_block_delayADR-115: #2966
Closes #2655
PR checklist
Tests written/updated.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments