abci++: add consensus parameter logic to control vote extension require height by williambanfield · Pull Request #8547 · tendermint/tendermint

williambanfield · 2022-05-13T19:14:11Z

What does this PR do?

This PR makes vote extensions optional within Tendermint. A new ConsensusParams field, called VoteParams.ExtensionRequireHeight, has been added to toggle whether or not extensions should be optional or required depending on the current height of the consensus engine. Related to: #8453

How does this PR make requiring vote extensions optional?

A new bool parameter was added to the VoteSet constructor. This bool toggles whether or not the VoteSet will panic on missing extensions. If the vote set is configured to check the extension, or the extension signature is present, the signature is verified with the public key.
The logic for receiving and validating votes was updated to not require extension data. Similarly though, if present, it was checked to be correctly formed.
Vote extensions are instead checked right before they are used, in the consensus state.go file. This check is done in the presence of the new VoteParams data and can conditionally check if the extension should be required.
BlockSync was updated to conditionally check for the presence of extension data.
The logic for loading and reconstructing the last saved commit from the block store was updated to allow for absent ExtendedCommit data. This facilitates updates because legacy chains will never have stored the ExtendedCommit and only have SeenCommit data.

Other changes

This change also updates the consensus logic to bypass the VerifyVoteExtension ABCI call if the vote was issued from the same validator. I believe this was discussed as the preferred logic previously but not implemented yet. @sergio-mena, can you confirm this?

Questions to reviewers

I don't love, nor do I think anyone does, the bool parameter to NewVoteSet, but it really seems like the simplest change without a larger refactor of the VoteSet type. The logic for verifying vote signatures lives within the VoteSet.AddVote(...) method, so teasing these apart would be a larger change. Is there a similarly simple change that can achieve the result without the bool parameter? changed to add NewStrictVoteSet and NewVoteSet with different validation criteria for added votes.
Is there a better name than VoteParams.ExtensionRequireHeight? It seems fine but perhaps there's better.

Next Steps

Add parameter to the generated .proto code.
add test for blocksync optional vote extension logic.

types/vote_set.go

creachadair

This looks good, I have only a few small comments/questions.

internal/consensus/reactor_test.go

internal/consensus/state.go

internal/consensus/state_test.go

internal/statesync/reactor_test.go

types/block.go

types/block_test.go

types/vote.go

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

sergio-mena · 2022-05-16T15:33:54Z

This change also updates the consensus logic to bypasses the VerifyVoteExtension ABCI call if the vote was issued from the same validator. I believe this was discussed as the preferred logic previously but not implemented yet. @sergio-mena, can you confirm this?

This is my understanding. I will make an extra pass at the spec to make sure its text fits this logic.

I'm starting to look at the changes now, but, from the PR description it seems the logic for (not) calling ExtendVote and VerifyVoteExtension depending on whether we are above or below ExtensionRequireHeight has not been modified. Is that so? If it has been modified in this PR, please disregard this paragraph

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

sergio-mena · 2022-05-16T15:53:23Z

Is there a better name than VoteParams.ExtensionRequireHeight? It seems fine but perhaps there's better.

Consider ExtensionsRequiredFromHeight. Not much longer, but clearer IMO

williambanfield · 2022-05-16T15:53:58Z

from the PR description it seems the logic for (not) calling ExtendVote and VerifyVoteExtension depending on whether we are above or below ExtensionRequireHeight has not been modified.

Ah! That was done here, but that could have been made more explicit in the PR description. That logic is implemented within the consensus state machine:

https://github.com/tendermint/tendermint/pull/8547/files#diff-c27a58d5a8b3c15166169543b3cdb94da91bb60efa27609ef446719474af78c5R2385-R2406

ExtendVote is always called, but VerifyVoteExtension is only called if the extension data is present. If the extension data is absent but required, the vote fails to validate so VerifyVoteExtension is not called. If the extension data is present, VerifyVoteExtension is always called, whether required or not. The idea is to make the extension data optional, not absent.

types/block.go

types/block_test.go

internal/blocksync/reactor.go

internal/consensus/state.go

sergio-mena · 2022-05-20T19:32:30Z

Thanks for all the improvements. I had approved yesterday, and between then and now I've been through the whole patch again several times.
If there is something that escaped through our fingers, it won't be because we didn't care enough :-)

williambanfield · 2022-05-20T19:37:25Z

@sergio-mena Thanks for the review! The only I'm working through is the e2e tests. I didn't catch that the test harness actually generates votes, so I have to make sure that it's using the proper types and methods.

sergio-mena · 2022-05-20T22:03:25Z

Yay!

…re height (#8547) This PR makes vote extensions optional within Tendermint. A new ConsensusParams field, called ABCIParams.VoteExtensionsEnableHeight, has been added to toggle whether or not extensions should be enabled or disabled depending on the current height of the consensus engine. Related to: #8453

… extension require height (#8547) This PR makes vote extensions optional within Tendermint. A new ConsensusParams field, called ABCIParams.VoteExtensionsEnableHeight, has been added to toggle whether or not extensions should be enabled or disabled depending on the current height of the consensus engine. Related to: #8453

…9862) * [cherry-picked] abci++: add consensus parameter logic to control vote extension require height (#8547) This PR makes vote extensions optional within Tendermint. A new ConsensusParams field, called ABCIParams.VoteExtensionsEnableHeight, has been added to toggle whether or not extensions should be enabled or disabled depending on the current height of the consensus engine. Related to: #8453 * Fix UTs * fix blocksync reactor import of state store * fixes1 * fixed_more_UTs * Fix TestHandshakeReplaySome * Fix all unit tests * Added hunk in original commit Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> Co-authored-by: Callum Waters <cmwaters19@gmail.com>

abci++: add consensus parameter to control vote extension require height

f1db90a

williambanfield force-pushed the wb/add-consensus-param-internal branch from e5121a3 to f1db90a Compare May 16, 2022 03:24

comment fixup

9d6e023

williambanfield changed the title ~~(WIP)add consensus param internal~~ abci++: add consensus parameter logic to control vote extension require height May 16, 2022

williambanfield marked this pull request as ready for review May 16, 2022 03:27

williambanfield requested review from ancazamfir, cmwaters, creachadair, ebuchman, jmalicevic, sergio-mena, thanethomson and tychoish as code owners May 16, 2022 03:27

williambanfield added 3 commits May 15, 2022 23:27

Merge branch 'master' into wb/add-consensus-param-internal

e80d2bd

Merge branch 'master' into wb/add-consensus-param-internal

6cb4150

Merge branch 'master' into wb/add-consensus-param-internal

f0b03ad

creachadair reviewed May 16, 2022

View reviewed changes

types/vote_set.go Outdated Show resolved Hide resolved

creachadair reviewed May 16, 2022

View reviewed changes

williambanfield and others added 3 commits May 16, 2022 11:24

change voteset constructor to strict vs non strict

bd26649

update heightvoteset constructor for strict vs non strict

c2f543b

Update types/block.go

331a7ce

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

williambanfield and others added 5 commits May 16, 2022 11:35

Update internal/consensus/state.go

d3c9ab6

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

Update internal/consensus/state.go

cb2f362

Co-authored-by: M. J. Fromberger <fromberger@interchain.io>

addr -> myAddr

19cc03f

shorter timeout in SwitchToConsensus test

f5cd75c

correct NewStrictVoteSet constructor to not call itself

62ecdf5

creachadair approved these changes May 16, 2022

View reviewed changes

types/block.go Outdated Show resolved Hide resolved

types/block_test.go Show resolved Hide resolved