From ab83d3307dc7eed079450bcb5f0b951c0301bd20 Mon Sep 17 00:00:00 2001 From: William Banfield Date: Thu, 12 May 2022 18:00:53 -0400 Subject: [PATCH] validation shimmed into place for switch to consensus --- internal/consensus/reactor_test.go | 101 +++++++++++++++++++++++++++++ internal/consensus/state.go | 9 ++- types/block.go | 32 ++++++--- types/vote.go | 7 +- types/vote_set.go | 10 ++- 5 files changed, 142 insertions(+), 17 deletions(-) diff --git a/internal/consensus/reactor_test.go b/internal/consensus/reactor_test.go index c6a8869db..81a9de379 100644 --- a/internal/consensus/reactor_test.go +++ b/internal/consensus/reactor_test.go @@ -32,6 +32,7 @@ import ( "github.com/tendermint/tendermint/internal/test/factory" "github.com/tendermint/tendermint/libs/log" tmcons "github.com/tendermint/tendermint/proto/tendermint/consensus" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" "github.com/tendermint/tendermint/types" ) @@ -600,6 +601,106 @@ func TestReactorCreatesBlockWhenEmptyBlocksFalse(t *testing.T) { wg.Wait() } +func TestSwitchToConsensusVoteExtensions(t *testing.T) { + for _, testCase := range []struct { + name string + storedHeight int64 + initialRequiredHeight int64 + includeExtensions bool + shouldPanic bool + }{ + { + name: "no vote extensions but not required", + initialRequiredHeight: 0, + storedHeight: 2, + includeExtensions: false, + shouldPanic: false, + }, + { + name: "no vote extensions but required this height", + initialRequiredHeight: 2, + storedHeight: 2, + includeExtensions: false, + shouldPanic: true, + }, + { + name: "no vote extensions and required in future", + initialRequiredHeight: 3, + storedHeight: 2, + includeExtensions: false, + shouldPanic: false, + }, + { + name: "no vote extensions and required previous height", + initialRequiredHeight: 1, + storedHeight: 2, + includeExtensions: false, + shouldPanic: true, + }, + { + name: "vote extensions and required previous height", + initialRequiredHeight: 1, + storedHeight: 2, + includeExtensions: true, + shouldPanic: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + cs, vs := makeState(ctx, t, makeStateArgs{validators: 1}) + validator := vs[0] + validator.Height = testCase.storedHeight + + cs.state.LastBlockHeight = testCase.storedHeight + cs.state.LastValidators = cs.state.Validators.Copy() + cs.state.ConsensusParams.Vote.ExtensionRequireHeight = testCase.initialRequiredHeight + + propBlock, err := cs.createProposalBlock(ctx) + require.NoError(t, err) + + // Consensus is preparing to do the next height after the stored height. + cs.Height = testCase.storedHeight + 1 + propBlock.Height = testCase.storedHeight + blockParts, err := propBlock.MakePartSet(types.BlockPartSizeBytes) + require.NoError(t, err) + + voteSet := types.NewVoteSet(cs.state.ChainID, testCase.storedHeight, 0, tmproto.PrecommitType, cs.state.Validators) + signedVote := signVote(ctx, t, validator, tmproto.PrecommitType, cs.state.ChainID, types.BlockID{ + Hash: propBlock.Hash(), + PartSetHeader: blockParts.Header(), + }) + + if !testCase.includeExtensions { + signedVote.Extension = nil + signedVote.ExtensionSignature = nil + } + + added, err := voteSet.AddVote(signedVote) + require.NoError(t, err) + require.True(t, added) + cs.blockStore.SaveBlock(propBlock, blockParts, voteSet.MakeExtendedCommit()) + reactor := NewReactor( + log.NewNopLogger(), + cs, + nil, + nil, + cs.eventBus, + true, + NopMetrics(), + ) + + if testCase.shouldPanic { + assert.Panics(t, func() { + reactor.SwitchToConsensus(ctx, cs.state, false) + }) + } else { + reactor.SwitchToConsensus(ctx, cs.state, false) + } + }) + } +} + func TestReactorRecordsVotesAndBlockParts(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() diff --git a/internal/consensus/state.go b/internal/consensus/state.go index 4b24ae13c..c574a1b39 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -703,6 +703,13 @@ func (cs *State) reconstructLastCommit(state sm.State) { )) } + requireHeight := cs.state.ConsensusParams.Vote.ExtensionRequireHeight + if requireHeight != 0 && extCommit.Height >= requireHeight { + if err := extCommit.EnsureExtensions(); err != nil { + panic(fmt.Sprintf("failed to reconstruct last commit; invalid vote extensions: %v", err)) + } + } + lastPrecommits := extCommit.ToVoteSet(state.ChainID, state.LastValidators) if !lastPrecommits.HasTwoThirdsMajority() { panic("failed to reconstruct last commit; does not have +2/3 maj") @@ -2350,7 +2357,7 @@ func (cs *State) addVote( // 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() + err := vote.EnsureExtension() if err == nil { _, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex) err = vote.VerifyWithExtension(cs.state.ChainID, val.PubKey) diff --git a/types/block.go b/types/block.go index c783ad68b..d5d99bd44 100644 --- a/types/block.go +++ b/types/block.go @@ -757,9 +757,6 @@ func (ecs ExtendedCommitSig) ValidateBasic() error { if len(ecs.Extension) > MaxVoteExtensionSize { return fmt.Errorf("vote extension is too big (max: %d)", MaxVoteExtensionSize) } - if len(ecs.ExtensionSignature) == 0 { - return errors.New("vote extension signature is missing") - } if len(ecs.ExtensionSignature) > MaxSignatureSize { return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize) } @@ -777,6 +774,13 @@ func (ecs ExtendedCommitSig) ValidateBasic() error { return nil } +func (ecs ExtendedCommitSig) ValidateExtension() error { + if len(ecs.ExtensionSignature) == 0 { + return errors.New("vote extension signature is missing") + } + return nil +} + // ToProto converts the ExtendedCommitSig to its Protobuf representation. func (ecs *ExtendedCommitSig) ToProto() *tmproto.ExtendedCommitSig { if ecs == nil { @@ -1017,7 +1021,6 @@ func (ec *ExtendedCommit) Clone() *ExtendedCommit { // Panics if signatures from the commit can't be added to the voteset. // Inverse of VoteSet.MakeExtendedCommit(). func (ec *ExtendedCommit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet { - requireExtensions := false voteSet := NewVoteSet(chainID, ec.Height, ec.Round, tmproto.PrecommitType, vals) for idx, ecs := range ec.ExtendedSignatures { if ecs.BlockIDFlag == BlockIDFlagAbsent { @@ -1027,11 +1030,6 @@ func (ec *ExtendedCommit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet if err := vote.ValidateBasic(); err != nil { panic(fmt.Errorf("failed to validate vote reconstructed from LastCommit: %w", err)) } - if requireExtensions { - if err := vote.ValidateExtension(); err != nil { - panic(fmt.Errorf("failed to validate vote extensions reconstructed from LastCommit: %w", err)) - } - } added, err := voteSet.AddVote(vote) if !added || err != nil { panic(fmt.Errorf("failed to reconstruct vote set from extended commit: %w", err)) @@ -1040,6 +1038,22 @@ func (ec *ExtendedCommit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet return voteSet } +// TODO Comment +// this should probably also verify the signature +// probably want to change to just verify when present. +func (ec *ExtendedCommit) EnsureExtensions() error { + for idx, ecs := range ec.ExtendedSignatures { + if ecs.BlockIDFlag == BlockIDFlagAbsent { + continue + } + vote := ec.GetExtendedVote(int32(idx)) + if err := vote.EnsureExtension(); err != nil { + return err + } + } + return nil +} + // StripExtensions converts an ExtendedCommit to a Commit by removing all vote // extension-related fields. func (ec *ExtendedCommit) StripExtensions() *Commit { diff --git a/types/vote.go b/types/vote.go index 4983cb3f8..fb5ee3578 100644 --- a/types/vote.go +++ b/types/vote.go @@ -313,7 +313,7 @@ func (vote *Vote) ValidateWithExtension() error { return err } - if err := vote.ValidateExtension(); err != nil { + if err := vote.EnsureExtension(); err != nil { return err } @@ -321,15 +321,12 @@ func (vote *Vote) ValidateWithExtension() error { } // -func (vote *Vote) ValidateExtension() error { +func (vote *Vote) EnsureExtension() 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 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 2ce053fcb..9ea2d59ca 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -201,8 +201,14 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) } } else { - 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) + if len(vote.ExtensionSignature) != 0 { + if err := vote.VerifyWithExtension(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) + } + } else { + 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) + } } }