celestiaorg / celestia-app

Celestia consensus node
https://celestiaorg.github.io/celestia-app/
Apache License 2.0
346 stars 293 forks source link

Override the `timeout_commit` and `timeout_propose` when switching to v3 #3859

Closed evan-forbes closed 1 month ago

evan-forbes commented 2 months ago

We should override the timeouts upon the switch to app version v3

rootulp commented 2 months ago

Context

Validator's locally configure these timeouts in config.toml:

# How long we wait for a proposal block before prevoting nil
timeout_propose = "10s"
# How much timeout_propose increases with each round
timeout_propose_delta = "500ms"
# How long we wait after receiving +2/3 prevotes for “anything” (ie. not a single block or nil)
timeout_prevote = "1s"
# How much the timeout_prevote increases with each round
timeout_prevote_delta = "500ms"
# How long we wait after receiving +2/3 precommits for “anything” (ie. not a single block or nil)
timeout_precommit = "1s"
# How much the timeout_precommit increases with each round
timeout_precommit_delta = "500ms"
# 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).
timeout_commit = "11s"

Problem

We'd like to decrease the block time from ~11.75 seconds to ~6 seconds at the same time that celestia-app v3 gets deployed to networks.

We don't want to rely on validator's manually configuring timeouts in their config.toml because we can't guarantee that they will be modified at the same time. Therefore, we'd like to add ADR-115's next_block_delay as a field in EndBlockResponse. If it exists, use that instead of timeout commit.

Proposal

Backport ADR-115 to celestia-core

Related

rootulp commented 2 months ago

Notes from discussion w/ @evan-forbes :

staheri14 commented 1 month ago

Question: At height X-1 (before upgrade height X), should we suggest timeouts for the current app version (v2) or the upcoming version (v3)?

Option 1: Use Timeouts for Next Version (v3)

Option 2: Use Timeouts for Current Version (v2)

evan-forbes commented 1 month ago

good question, my understanding of this is that we should only change the timeouts when the app version is changed, so we should do option 2

evan-forbes commented 1 month ago

to add more reasoning to the above, we don't know if the upgrade is successful until we produce a block that has that version, therefore that is the soonest we possibly could change.

staheri14 commented 1 month ago

Thanks, @evan-forbes! That makes sense—I'll go ahead with the second option.

staheri14 commented 1 month ago

Design Update/Decision:

For the genesis state, the start time of block height 1 is calculated using cs.CommitTime + timeoutCommit (also see here), where timeoutCommit (in the new desing), depends on the timeouts of the state after applying the previous block. Since there is no block before the genesis block (hence no state), two options can be considered:

  1. Calculating the cs.StartTime of the next height i.e., H=1, by using timeoutCommit of zero.
  2. Using the timeoutCommit from the genesis state itself. This will be somewhat an exception where the cs.StartTime calculated at height 0 uses timeouts meant for height 1.

Currently, I went with the second option in the implementation [link], but please let me know if you think the first option makes more sense.

evan-forbes commented 1 month ago

either or should be okay if I'm understanding correctly, especially for our existing testnets

we could also just use a default timeouts for the first block, as it shouldn't be super important for us atm