From 32790dedfcea8e791da412c4f9257b648e6211fa Mon Sep 17 00:00:00 2001 From: William Banfield Date: Mon, 16 May 2022 17:02:43 -0400 Subject: [PATCH] do not verify extension if not enabled --- internal/consensus/state.go | 51 ++++++++++++++++---------------- internal/consensus/state_test.go | 29 +++++++++++------- 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/internal/consensus/state.go b/internal/consensus/state.go index bdb15d77e..78d6ee733 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -2376,34 +2376,35 @@ func (cs *State) addVote( return } - var myAddr []byte - if cs.privValidatorPubKey != nil { - myAddr = cs.privValidatorPubKey.Address() - } - // Verify VoteExtension if precommit and not nil - // https://github.com/tendermint/tendermint/issues/8487 - if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() && - !bytes.Equal(vote.ValidatorAddress, myAddr) { - // The core fields of the vote message were already validated in the - // consensus reactor when the vote was received. - // Here, we validate that the vote extension was included in the vote - // message. - // Chains that are not configured to require vote extensions - // will consider the vote valid even if the extension is absent. - // VerifyVoteExtension will not be called in this case if the extension - // is absent. - err := vote.EnsureExtension() - if err == nil { - _, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex) - err = vote.VerifyExtension(cs.state.ChainID, val.PubKey) + // Check to see if the chain is configured to extend votes. + if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) { + // The chain is configured to extend votes, check that the vote is + // not for a nil block and verify the extensions signature against the + // corresponding public key. + + var myAddr []byte + if cs.privValidatorPubKey != nil { + myAddr = cs.privValidatorPubKey.Address() } - if err == nil { + // Verify VoteExtension if precommit and not nil + // https://github.com/tendermint/tendermint/issues/8487 + if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() && + !bytes.Equal(vote.ValidatorAddress, myAddr) { // Skip the VerifyVoteExtension call if the vote was issued by this validator. + + // The core fields of the vote message were already validated in the + // consensus reactor when the vote was received. + // Here, we verify the signature of the vote extension included in the vote + // message. + _, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex) + if err := vote.VerifyExtension(cs.state.ChainID, val.PubKey); err != nil { + return false, err + } + err := cs.blockExec.VerifyVoteExtension(ctx, vote) cs.metrics.MarkVoteExtensionReceived(err == nil) - } else if !errors.Is(err, types.ErrVoteExtensionAbsent) { - return false, err - } else if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) { - return false, err + if err != nil { + return false, err + } } } diff --git a/internal/consensus/state_test.go b/internal/consensus/state_test.go index fa1a88a8e..267a0fdb9 100644 --- a/internal/consensus/state_test.go +++ b/internal/consensus/state_test.go @@ -2115,6 +2115,7 @@ func TestVerifyVoteExtensionNotCalledOnAbsentPrecommit(t *testing.T) { m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m}) height, round := cs1.Height, cs1.Round + cs1.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = cs1.Height proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) @@ -2253,44 +2254,50 @@ func TestPrepareProposalReceivesVoteExtensions(t *testing.T) { } } -// TestVoteExtensionRequiredHeight tests that 'ExtensionRequireHeight' correctly +// TestVoteExtensionEnableHeight tests that 'ExtensionRequireHeight' correctly // enforces that vote extensions be present in consensus for heights greater than // or equal to the configured value. -func TestVoteExtensionRequiredHeight(t *testing.T) { +func TestVoteExtensionEnableHeight(t *testing.T) { for _, testCase := range []struct { name string - initialRequiredHeight int64 + enableHeight int64 hasExtension bool + expectVerifyCalled bool expectSuccessfulRound bool }{ { - name: "extension present but not required", + name: "extension present but not enabled", hasExtension: true, - initialRequiredHeight: 0, + enableHeight: 0, + expectVerifyCalled: false, expectSuccessfulRound: true, }, { name: "extension absent but not required", hasExtension: false, - initialRequiredHeight: 0, + enableHeight: 0, + expectVerifyCalled: false, expectSuccessfulRound: true, }, { name: "extension present and required", hasExtension: true, - initialRequiredHeight: 1, + enableHeight: 1, + expectVerifyCalled: true, expectSuccessfulRound: true, }, { name: "extension absent but required", hasExtension: false, - initialRequiredHeight: 1, + enableHeight: 1, + expectVerifyCalled: false, expectSuccessfulRound: false, }, { name: "extension absent but required in future height", hasExtension: false, - initialRequiredHeight: 2, + enableHeight: 2, + expectVerifyCalled: false, expectSuccessfulRound: true, }, } { @@ -2306,7 +2313,7 @@ func TestVoteExtensionRequiredHeight(t *testing.T) { }, nil) m.On("PrepareProposal", mock.Anything, mock.Anything).Return(&abci.ResponsePrepareProposal{}, nil) m.On("ExtendVote", mock.Anything, mock.Anything).Return(&abci.ResponseExtendVote{}, nil) - if testCase.hasExtension { + if testCase.expectVerifyCalled { m.On("VerifyVoteExtension", mock.Anything, mock.Anything).Return(&abci.ResponseVerifyVoteExtension{ Status: abci.ResponseVerifyVoteExtension_ACCEPT, }, nil).Times(numValidators - 1) @@ -2314,7 +2321,7 @@ func TestVoteExtensionRequiredHeight(t *testing.T) { m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m, validators: numValidators}) - cs1.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = testCase.initialRequiredHeight + cs1.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = testCase.enableHeight height, round := cs1.Height, cs1.Round timeoutCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryTimeoutPropose)