docs: ADR-115 predictable block times by melekes · Pull Request #2966 · cometbft/cometbft

melekes · 2024-05-02T10:46:57Z

PR checklist

~~Tests written/updated~~
~~Changelog entry added in .changelog (we use unclog to manage our changelog)~~
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

ValarDragon · 2024-05-02T14:45:28Z

Making this a network parameter is great! I fully support doing that!

I actually think we can do one-step better as well though. Right now a block's time is roughly set by 4 steps
---------- Start time counting

Proposer(N+1) receives 2/3rd precommits for block N
Proposer Executes + Commits block N
Proposer waits Timeout_commit
Proposer builds block N + 1 and gossips it
2 rounds of 2/3rds voting amongst validator set
Proposer(N+2) recieves 2/3rds precommits for block N+1
---------- Stop counting

Each of the steps could have variable delay for whatever reason. It would be nice if we made timeout_commit react to the time-since-last-block.

In PBTS, we could do this by saying, block N+1 proposer should wait max(true_minimum_timeout_commit, current_time - block N timestamp - expected_block_time).

And then we lower the true_minimum_timeout_commit to perhaps something like .5s by default, and have most networks parameterize around the expected block time. (which is the happy path for most networks, except the ones trying to shave down block times)

tac0turtle

super glad to see this work taking shape. ❤️

melekes · 2024-05-03T12:48:59Z

In PBTS, we could do this by saying, block N+1 proposer should wait max(true_minimum_timeout_commit, current_time - block N timestamp - expected_block_time).

What I like about the current proposal is that it doesn't restrict the ABCI dev in any way. There's no max(x, y), no min. The ABCI dev has access to all the info (I believe), so no one is stopping him/her from calculating max(true_minimum_timeout_commit, current_time - block N timestamp - expected_block_time) if he/she desires to do so.

ValarDragon · 2024-05-03T16:17:19Z

Oh true, didnt think about that. That sounds great to me!

cason

I think that the ADR can be extended/reworded as follows:

The problem description should make it more precise what we mean by predictable block times. If I understand correctly, we want the medium-to-long term average block time to converge to a desired value. To achieve that, we need to define a form of minimum block time, but also be able to changing it dynamically. Namely, if a block takes longer than expected, we should be able to render the next block(s) faster in order to converge to the desired (average) block time.
Properly define how timeout_commit works, at which extent it can be adopted to achieve the predictable block times goal, and what are its limitations. In addition to the one explained in the text (it is not a global parameter), we have the fact that it is static.
There is a confusion in the text, that I assume also comes from the confusing in the operation of timeout_commit, between block time and the delay between committing a block and proposing the next block. And this is a main concern: we cannot precisely predict the consensus delay for a block, but we can establish a minimum duration for the processing step of a block, i.e., the time from which consensus decides it and the next consensus instance effectively starts.
We need to make it clear that this approach is always best-effort, not only because delays are variable (by block size, by system size, by network conditions), but also because we have nodes that don't follow the protocol. By the way, running a validator node with a timeout_commit parameter that differs from the one defined for the network is a form of misbehavior.

docs/references/architecture/adr-115-predictable-block-times.md

cason · 2024-05-13T09:11:05Z

docs/references/architecture/adr-115-predictable-block-times.md

+`next_block_delay`. This field's semantics stays essentially the same: delay
+between this block and the time when the next block is proposed.
+
+If Comet goes into multiple rounds of consensus, the ABCI application can react


Not sure if the application is aware of the commit round. In any case, different nodes may commit at different rounds.

different nodes may commit at different rounds.

could you please explain?

decided_last_commit https://docs.cometbft.com/v0.38/spec/abci/abci++_methods#finalizeblock ?

A node can commit at round 0, other might decide the same block on block 1. This happens if part of the nodes don't see the precommits of round 0 timely and proceed to round 1.

The canonical commit, in fact, is the one observed by the proposer (for the previous block) and added to the proposed block.

Not sure if the application is aware of the commit round.

does the ABCI dev need it? They can simply use real time, no?

Yes, it should use real time, in particular because the duration of rounds can be pretty variable.

But the problem of real time is, of course, that different nodes will process a block at (slightly) different times.

I have read up to here, so what I say here might be redundant with text below.
My impression is that most applications, in order to leverage this feature:

will need to use real --wallclock-- time (having Berachain's use case in mind, where you want to conform to regular wall-clock deadlines)

will mandate its nodes to have synchronized clocks (NTP, or other). This is not a big deal since we also require this for PBTS

docs/references/architecture/adr-115-predictable-block-times.md

evan-forbes · 2024-05-15T12:43:46Z

docs/references/architecture/adr-115-predictable-block-times.md

+Move `timeout_commit` into `FinalizeBlockResponse` as `next_block_delay`. This
+field's semantics stays essentially the same: delay between the time when the
+current block is committed and the next height is started. The idea is
+literally to have the same behavior as `timeout_commit`, while allowing the
+application to pick a different value for each height.


fwiw, if I'm interpreting this correctly we've tried this here

in our network tests, everything worked as expected and we have very consistent block times. However when we shipped this to a testnet, we frequently failed to reach consensus.

replicating and debugging this issue by changing the clocks of the network instances is next on our list of todos after we switch network testing backends celestiaorg/celestia-core#1333

Thanks for flagging this. We will keep this in mind when implementing this feature.

evan-forbes

lgtm fwiw

sergio-mena

Thanks @melekes for this ADR!

I support this way of dealing with timeout_commit over the alternative that has been discussed internally in the past months: moving timeout_commit to ConsensusParams. Your solution is more versatile and powerful for apps and I believe the upgrade path can be made painless (unlike the "move timeout_commit to ConsensusParams" solution).

sergio-mena · 2024-05-16T08:54:20Z

docs/references/architecture/adr-115-predictable-block-times.md

+`timeout_commit` dynamically (if such feature existed) to give the state
+machine some extra time to finish execution.


If a block takes a lot of time to execute, that time is expected to be roughly the same at all nodes (although there may be HW differences). So I don't clearly see how increasing timeout_commit in this case would help. While all nodes are executing a long block, the consensus algorithm stays blocked until FinalizeBlock returns.

Agree. I copied this from tendermint/tendermint#5911 (comment). @ValarDragon @sunnya97 can you please clarify what you're hoping to archive in tendermint/tendermint#5911?

Because timeout_commit encompasses the block execution.

So, faster nodes will start next height (proposing blocks or expecting for proposals) before slower nodes.

sergio-mena · 2024-05-16T09:00:09Z

docs/references/architecture/adr-115-predictable-block-times.md

+`next_block_delay`. This field's semantics stays essentially the same: delay
+between this block and the time when the next block is proposed.
+
+If Comet goes into multiple rounds of consensus, the ABCI application can react


I have read up to here, so what I say here might be redundant with text below.
My impression is that most applications, in order to leverage this feature:

will need to use real --wallclock-- time (having Berachain's use case in mind, where you want to conform to regular wall-clock deadlines)

will mandate its nodes to have synchronized clocks (NTP, or other). This is not a big deal since we also require this for PBTS

docs/references/architecture/adr-115-predictable-block-times.md

as `next_block_delay` ADR-115: #2966

as `next_block_delay` ADR-115: #2966 Closes #2655 --- #### PR checklist - [ ] ~~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>

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>

…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)

docs: ADR-115 predictable block times

97bbd0b

Refs #2655

melekes requested a review from a team as a code owner May 2, 2024 10:46

melekes requested a review from a team May 2, 2024 10:46

adizere linked an issue May 2, 2024 that may be closed by this pull request

PBTS: Allow Application to Set BlockTimestamp #2655

Closed

melekes self-assigned this May 2, 2024

tac0turtle reviewed May 3, 2024

View reviewed changes

Merge branch 'main' into 2655-predictable-block-times

bd3ba3e

ValarDragon approved these changes May 6, 2024

View reviewed changes

cason reviewed May 13, 2024

View reviewed changes

melekes added 3 commits May 13, 2024 15:59

Merge branch 'main' into 2655-predictable-block-times

1b7370d

replace predictable w/ constant

36310c2

reword some sentences

b520640

cason reviewed May 13, 2024

View reviewed changes

docs/references/architecture/adr-115-predictable-block-times.md Outdated Show resolved Hide resolved

docs/references/architecture/adr-115-predictable-block-times.md Outdated Show resolved Hide resolved

melekes added 3 commits May 14, 2024 10:14

Merge branch 'main' into 2655-predictable-block-times

cab230a

clarify ADR

8bb3068

improve formatting

4e591fc

evan-forbes reviewed May 15, 2024

View reviewed changes

melekes added 2 commits May 15, 2024 18:20

Celestia's usecase

02a4854

Merge branch 'main' into 2655-predictable-block-times

6de9a69

evan-forbes approved these changes May 16, 2024

View reviewed changes

sergio-mena approved these changes May 16, 2024

View reviewed changes

melekes added a commit that referenced this pull request May 16, 2024

feat(abci)!: move timeout_commit into FinalizeBlockResponse

9c089ef

as `next_block_delay` ADR-115: #2966

melekes mentioned this pull request May 16, 2024

feat(abci): move timeout_commit into FinalizeBlockResponse #3089

Merged

4 tasks

melekes added 3 commits May 16, 2024 16:04

changes after @sergio-mena review

75bf5bb

remove negative

8f772cc

Merge branch 'main' into 2655-predictable-block-times

65bcb1e

melekes added this pull request to the merge queue May 16, 2024

Merged via the queue into main with commit c812822 May 16, 2024

melekes deleted the 2655-predictable-block-times branch May 16, 2024 18:47

teddyding mentioned this pull request Jun 19, 2024

[DO NOT MERGE, backport]feat(abci): move timeout_commit into FinalizeBlockResponse (#3089) dydxprotocol/cometbft#29

Open

7 tasks

		`timeout_commit` dynamically (if such feature existed) to give the state
		machine some extra time to finish execution.

Conversation

melekes commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

ValarDragon commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tac0turtle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented May 3, 2024

Uh oh!

ValarDragon commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evan-forbes left a comment

Choose a reason for hiding this comment

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

melekes commented May 2, 2024 •

edited

Loading

ValarDragon commented May 2, 2024 •

edited

Loading

tac0turtle left a comment •

edited

Loading

ValarDragon commented May 3, 2024 •

edited

Loading

sergio-mena May 16, 2024 •

edited

Loading

sergio-mena May 16, 2024 •

edited

Loading