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

[williambanfield]

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.

[creachadair]

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

[sergio-mena]

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

[sergio-mena]

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]

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.

[sergio-mena]

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]

@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]

Yay!