ADR-74: Migrate Timeout Parameters to Consensus Parameters#7503
ADR-74: Migrate Timeout Parameters to Consensus Parameters#7503mergify[bot] merged 23 commits intomasterfrom
Conversation
cmwaters
left a comment
There was a problem hiding this comment.
Excited for this to land!
I have a few comments which relate to my current understanding of these parameters
| * `TimeoutPrevote` | ||
| * How long the consensus algorithm waits after receiving +2/3 prevotes before | ||
| issuing a precommit. |
There was a problem hiding this comment.
Is this true? I thought the timeout started the moment I prevoted instead of starting after receiving +2/3 prevotes. Same with TimeoutPrecommit. Receiving +2/3 is what moves us into the next step and makes us precommit
There was a problem hiding this comment.
I found this to be a bit confusing as well, but this is the way this works. See the arxiv paper, line 34 and line 47. Loosely: it bounds how many steps apart any two validators may be during consensus.
creachadair
left a comment
There was a problem hiding this comment.
This seems good. My main open questions are around how we should handle rollout and migration.
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
| While Tendermint nodes often run with similar bandwidth and on similar cloud-hosted | ||
| machines, there are enough points of variability to make configuring | ||
| consensus timeouts meaningful. Namely, Tendermint network topologies are likely to be | ||
| very different from chain to chain. Additionally, applications may vary greatly in | ||
| how long the `Commit` phase may take. Applications that perform more work during `Commit` | ||
| require a longer `TimeoutCommit` to allow the application to complete its work | ||
| and be prepared for the next height. |
There was a problem hiding this comment.
Also don't forget that the size of a block and the number of validators could be vastly different
|
|
||
| ## Decision | ||
|
|
||
| None |
There was a problem hiding this comment.
No decision is being made here?
There was a problem hiding this comment.
was leaving til we seemed to have consensus.
| parameters will be ignored. If the consensus parameters are not set, Tendermint | ||
| will fall back to using the values contained in the `config.toml` file. |
There was a problem hiding this comment.
Does this mean that we are allowing networks to ignore the consensus params if they want and still use every nodes local config parameters. Are we sure we want to do this?
There was a problem hiding this comment.
Once the network sets a non-default values in the consensus parameters, we will use those. This just gives a release during which networks can make the upgrade.
I think that this is a good way for networks to gracefully upgrade with minimal disruption. The values in the config file are strictly fallbacks in case no values are set by the chain. It maintains a partial version of the already kind of bad thing for a release so that we can more seamlessly transition to better management of these params.
| During the v0.36 release cycle, if non-default values are set in the `config.toml`, | ||
| the node will log a warning alerting operators that the parameters will soon be removed | ||
| in the upcoming release. |
There was a problem hiding this comment.
But more importantly, will these non-default values be used?
There was a problem hiding this comment.
They will only be used if the chain does not set them as consensus parameters.
There was a problem hiding this comment.
But what do you mean by not setting the consensus params. Aren't they set to some reasonable default upon initialization? How would they be unset?
There was a problem hiding this comment.
So basically it would be as follows:
If the chain has not yet set them in EndBlock, use what is in config.toml.
Once the chain sets them in EndBlock, use those values and ignore config.toml.
There was a problem hiding this comment.
Would this work with state syncing? Will we need an extra flag to tell us if it has been set or not? I think it would be better if we transition to using consensus params directly when the chain starts/restarts i.e. use the genesis values or the defaults in the case of new params in an upgraded chain (I know you've written an entire RFC on this so perhaps I should check there first)
There was a problem hiding this comment.
Would this work with state syncing? Will we need an extra flag to tell us if it has been set or not?
We would not need this flag, no. We would just always treat 'value as absent' as 'value uses default'. This way historical blocks would always look to be using the defaults. Once the node that is syncing hits a height where the application changed the parameters via EndBlock it would then start to use the updated values. So far this is fine because all param changes we are proposing are timing related and validation of historical blocks does not consider timing at all.
(I'd def recommend the RFC since this is a bit of a fiddly detail and it's hopefully spelled out a little more clearly there).
related to: #7274 and #7275
Still somewhat uncertain on two things that I'd appreciate more feedback on: