diff --git a/internal/consensus/state.go b/internal/consensus/state.go index b016e2687..a1f09cd68 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -2308,6 +2308,7 @@ func (cs *State) addVote( return } + // late votes still _must_ have extensions. added, err = cs.LastCommit.AddVote(vote) if !added { return @@ -2339,11 +2340,31 @@ func (cs *State) addVote( // Verify VoteExtension if precommit and not nil // https://github.com/tendermint/tendermint/issues/8487 - if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() { - err := cs.blockExec.VerifyVoteExtension(ctx, vote) - cs.metrics.MarkVoteExtensionReceived(err == nil) - if err != nil { - return false, err + if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() && + !bytes.Equal(vote.ValidatorAddress, cs.privValidatorPubKey.Address()) { + // The core fields of the vote message were already validated in the + // consensus reactor when the vote was received. + // Here, we valdiate 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.ValidateExtension() + if err == nil { + _, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex) + err = vote.VerifyWithExtension(cs.state.ChainID, val.PubKey) + } + if err == nil { + err := cs.blockExec.VerifyVoteExtension(ctx, vote) + cs.metrics.MarkVoteExtensionReceived(err == nil) + } else { + if !errors.Is(err, types.ErrVoteExtensionAbsent) { + return false, err + } + if cs.requireVoteExtension() { + return false, err + } } } @@ -2741,6 +2762,14 @@ func (cs *State) calculateProposalTimestampDifferenceMetric() { } } +func (cs *State) requireVoteExtension() bool { + requireHeight := cs.state.ConsensusParams.Vote.ExtensionRequireHeight + if requireHeight < cs.Height { + return false + } + return true +} + // proposerWaitTime determines how long the proposer should wait to propose its next block. // If the result is zero, a block can be proposed immediately. // diff --git a/internal/consensus/state_test.go b/internal/consensus/state_test.go index 6fa69a1a3..598be471b 100644 --- a/internal/consensus/state_test.go +++ b/internal/consensus/state_test.go @@ -2266,6 +2266,87 @@ func TestPrepareProposalReceivesVoteExtensions(t *testing.T) { } } +func TestVerifyVoteExtensionCalled(t *testing.T) { + for _, testCase := range []struct { + name string + hasExtension bool + initialRequireHeight int64 + }{ + { + name: "called when extension present", + hasExtension: true, + }, + { + name: "not called when extension absent", + hasExtension: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + config := configSetup(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + numValidators := 3 + m := abcimocks.NewApplication(t) + m.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{ + Status: abci.ResponseProcessProposal_ACCEPT, + }, 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 { + m.On("VerifyVoteExtension", mock.Anything, mock.Anything).Return(&abci.ResponseVerifyVoteExtension{ + Status: abci.ResponseVerifyVoteExtension_ACCEPT, + }, nil).Times(numValidators) + } + m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() + m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() + l := log.NewTestingLogger(t) + cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m, logger: l, validators: numValidators}) + height, round := cs1.Height, cs1.Round + + proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) + newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) + pv1, err := cs1.privValidator.GetPubKey(ctx) + require.NoError(t, err) + addr := pv1.Address() + voteCh := subscribeToVoter(ctx, t, cs1, addr) + + startTestRound(ctx, cs1, cs1.Height, round) + ensureNewRound(t, newRoundCh, height, round) + ensureNewProposal(t, proposalCh, height, round) + rs := cs1.GetRoundState() + + blockID := types.BlockID{ + Hash: rs.ProposalBlock.Hash(), + PartSetHeader: rs.ProposalBlockParts.Header(), + } + + // sign all of the votes + signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss[1:]...) + ensurePrevoteMatch(t, voteCh, height, round, rs.ProposalBlock.Hash()) + + var ext []byte + if testCase.hasExtension { + ext = []byte("extension") + } + // sign all of the precommits + for _, vs := range vss[1:] { + vote, err := vs.signVote(ctx, tmproto.PrecommitType, config.ChainID(), blockID, ext) + if !testCase.hasExtension { + vote.ExtensionSignature = nil + } + require.NoError(t, err) + addVotes(cs1, vote) + } + ensurePrecommit(t, voteCh, height, round) + + height++ + ensureNewRound(t, newRoundCh, height, round) + m.AssertExpectations(t) + }) + } +} + // 4 vals, 3 Nil Precommits at P0 // What we want: // P0 waits for timeoutPrecommit before starting next round diff --git a/internal/state/execution.go b/internal/state/execution.go index 2c88c793b..53ce369e0 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -3,6 +3,7 @@ package state import ( "bytes" "context" + "errors" "fmt" "time" @@ -102,13 +103,12 @@ func (blockExec *BlockExecutor) CreateProposalBlock( txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas) commit := lastExtCommit.StripExtensions() block := state.MakeBlock(height, txs, commit, evidence, proposerAddr) - rpp, err := blockExec.appClient.PrepareProposal( ctx, &abci.RequestPrepareProposal{ MaxTxBytes: maxDataBytes, Txs: block.Txs.ToSliceOfBytes(), - LocalLastCommit: buildExtendedCommitInfo(lastExtCommit, blockExec.store, state.InitialHeight), + LocalLastCommit: buildExtendedCommitInfo(lastExtCommit, blockExec.store, state.InitialHeight, state.ConsensusParams.Vote.ExtensionRequireHeight), ByzantineValidators: block.Evidence.ToABCI(), Height: block.Height, Time: block.Time, @@ -321,7 +321,7 @@ func (blockExec *BlockExecutor) VerifyVoteExtension(ctx context.Context, vote *t } if !resp.IsOK() { - return types.ErrVoteInvalidExtension + return errors.New("invalid vote extension") } return nil @@ -428,7 +428,7 @@ func buildLastCommitInfo(block *types.Block, store Store, initialHeight int64) a // data, it returns an empty record. // // Assumes that the commit signatures are sorted according to validator index. -func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeight int64) abci.ExtendedCommitInfo { +func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeight, extensionRequireHeight int64) abci.ExtendedCommitInfo { if ec.Height < initialHeight { // There are no extended commits for heights below the initial height. return abci.ExtendedCommitInfo{} @@ -469,6 +469,10 @@ func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeigh if ecs.BlockIDFlag == types.BlockIDFlagCommit { // We only care about vote extensions if a validator has voted to // commit. + if ecs.Extension == nil && ecs.ExtensionSignature == nil && ec.Height > extensionRequireHeight { + // TODO: this error is akward, make it better + panic(fmt.Errorf("commit received with missing vote extension data")) + } ext = ecs.Extension } diff --git a/types/vote.go b/types/vote.go index 446de130a..8d24c995e 100644 --- a/types/vote.go +++ b/types/vote.go @@ -27,7 +27,7 @@ var ( ErrVoteInvalidBlockHash = errors.New("invalid block hash") ErrVoteNonDeterministicSignature = errors.New("non-deterministic signature") ErrVoteNil = errors.New("nil vote") - ErrVoteInvalidExtension = errors.New("invalid vote extension") + ErrVoteExtensionAbsent = errors.New("vote extensions absent") ) type ErrVoteConflictingVotes struct { @@ -317,16 +317,24 @@ func (vote *Vote) ValidateWithExtension() error { return err } + if err := vote.ValidateExtension(); err != nil { + return err + } + + return nil +} + +// +func (vote *Vote) ValidateExtension() error { // We should always see vote extension signatures in non-nil precommits if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() { if len(vote.ExtensionSignature) == 0 { - return errors.New("vote extension signature is missing") + return ErrVoteExtensionAbsent } if len(vote.ExtensionSignature) > MaxSignatureSize { return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize) } } - return nil } diff --git a/types/vote_set.go b/types/vote_set.go index 224d4e4f8..31344d8fb 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -194,7 +194,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { } // Check signature. - if err := vote.VerifyWithExtension(voteSet.chainID, val.PubKey); err != nil { + if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil { return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) }