RFC-017: add section on vote extension upgrade path#8484
RFC-017: add section on vote extension upgrade path#8484williambanfield wants to merge 5 commits intomasterfrom
Conversation
| vote extensions, we will provide application developers with a [ConsensusParam](https://github.com/tendermint/tendermint/blob/cec0a9798/proto/tendermint/types/params.proto#L13) | ||
| to transition the chain from no history of vote extensions to requiring vote extensions. | ||
| This parameter will be an `int64` representing the first height where vote extensions | ||
| will be required for votes to be considered valid. Once the configured height occurs, |
There was a problem hiding this comment.
Just to be clear: This height is the height where you require vote extensions to be present in order to (have) participate(d) in consensus for that height? Or is it the height after that, when we start enforcing that they're present (e.g., after the point where the first batch of them was introduced)?
There was a problem hiding this comment.
This height is the height where you require vote extensions to be present in order to (have) participate(d) in consensus for that height?
Yes. This is 'during height H, an all nodes are expected to provide vote extensions to have legally participated in consensus'.
There was a problem hiding this comment.
#8453 explains it in a clear way (Section: Effect of the variable on consensus). Consider bringing that section here.
There was a problem hiding this comment.
Added additional description, let me know if this clear.
| to transition the chain from no history of vote extensions to requiring vote extensions. | ||
| This parameter will be an `int64` representing the first height where vote extensions | ||
| will be required for votes to be considered valid. Once the configured height occurs, | ||
| the parameter will be disallowed from changing, meaning that vote extensions cannot flip from being |
There was a problem hiding this comment.
We enforce validation of consensus parameters in ApplyBlock at the moment. If validation fails, we fail to apply the block and return to consensus. This is a pretty inelegant mechanism, that may actually be slightly broken, but I was imagining validation would occur for this is in the same way.
| Vote extensions pose an issue for Tendermint upgrades. Chains that upgrade from | ||
| v0.35 to v0.36 will attempt to produce the first height running v0.36 without vote | ||
| extension data from the previous height. We intend to allow chains to _require_ | ||
| vote extensions data. Chains that do so will not make progress without vote |
There was a problem hiding this comment.
Can you please add a reference to Solution 3, stating something like: the key property of Solution 3 above must be broken in order to upgrade.
There was a problem hiding this comment.
Added, please take a look.
| parameter changed so that Tendermint can determine whether or not to enforce | ||
| vote extension presence on the `PrepareProposal` call. For further clarification | ||
| and implementation, see [issue 8453](https://github.com/tendermint/tendermint/issues/8453) | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
I would like to have a modified version of:
(a) Solution 3
(b) the base solution we have implemented
Would you mind clarifying what you mean by this? In this document you'd like this written?
There was a problem hiding this comment.
If I am not mistaken, the upgrade mechanism will "break" both Solution 3 and the base solution described here and implemented by #8433... as they are today.
So IMO we need to explain how we are going to modify those to make them work.
For (b) I would suggest nothing really involved, apart from bringing the conditions you specified in #8453, a few words on how this will change the solution would do. Probably best to do when you have a clear idea on the code.
A priori, I can think of;
- the condition to switch from blocksync to consensus might have to change
- in blocksync, when we check if a "received block" message is valid, we need to extend the current check (now we could receive no extension in some conditions and the message would still be valid)
- how we verify in blocksync might have to be adapted as well (not sure)
- and I'm probably missing stuff
For (a), I can do it later (when I'm done with the spec cleanup).
What do you think?
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@williambanfield are you planning on merging this? If you're not, then if you're OK with that, I will reuse most of the text here (probably cherry-picking) as part of one of my upcoming spec-related PR's. |
* Squashed tendermint/tendermint#8484 * Moved new text from rfc017 to rfc100 * Adapted the new text and finished the items in the PR (changes needed to main solutions) * Added log line in changelog section * Added link to references * Fix reference * Apply suggestions from code review Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> * Addressed @jmalicevic's comment * Update docs/rfc/rfc-100-abci-vote-extension-propag.md --------- Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
This pull requests adds a section to RFC-017 to detail the upgrade path for nodes transitioning from v0.35 to v0.36. The information being added to this RFC reflects the work being done in issue #8453 and is added to this RFC for posterity.