From 701204da7c43330f7bb56bb8117402e1b1bddb06 Mon Sep 17 00:00:00 2001 From: Alessandro Sforzin Date: Fri, 10 Jan 2025 21:31:43 +0100 Subject: [PATCH] Added bool flag to enable/disable signature verification when converting a commit to a vote set. --- internal/consensus/reactor.go | 2 +- internal/consensus/reactor_test.go | 2 +- internal/consensus/state.go | 2 +- internal/consensus/types/height_vote_set.go | 2 +- internal/test/commit.go | 2 +- privval/file_test.go | 4 ++-- types/block.go | 10 +++++---- types/block_test.go | 2 +- types/evidence.go | 8 +++---- types/test_util.go | 2 +- types/vote.go | 24 +++++++++++++++------ types/vote_set.go | 8 +++---- types/vote_set_test.go | 2 +- types/vote_test.go | 16 +++++++------- 14 files changed, 49 insertions(+), 37 deletions(-) diff --git a/internal/consensus/reactor.go b/internal/consensus/reactor.go index 60941c7fa0a..0faf4e65290 100644 --- a/internal/consensus/reactor.go +++ b/internal/consensus/reactor.go @@ -1863,7 +1863,7 @@ type VoteMessage struct { // ValidateBasic checks whether the vote within the message is well-formed. func (m *VoteMessage) ValidateBasic() error { - return m.Vote.ValidateBasic() + return m.Vote.ValidateBasic(false) } // String returns a string representation. diff --git a/internal/consensus/reactor_test.go b/internal/consensus/reactor_test.go index 4cd27e8d859..b876d96e67b 100644 --- a/internal/consensus/reactor_test.go +++ b/internal/consensus/reactor_test.go @@ -391,7 +391,7 @@ func TestSwitchToConsensusVoteExtensions(t *testing.T) { require.Nil(t, signedVote.ExtensionSignature) } - added, err := voteSet.AddVote(signedVote) + added, err := voteSet.AddVote(signedVote, false) require.NoError(t, err) require.True(t, added) diff --git a/internal/consensus/state.go b/internal/consensus/state.go index 2aa171de584..d382749e022 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -2320,7 +2320,7 @@ func (cs *State) addVote(vote *types.Vote, peerID p2p.ID) (added bool, err error return added, err } - added, err = cs.LastCommit.AddVote(vote) + added, err = cs.LastCommit.AddVote(vote, false) if !added { // If the vote wasn't added but there's no error, its a duplicate vote if err == nil { diff --git a/internal/consensus/types/height_vote_set.go b/internal/consensus/types/height_vote_set.go index 47a1fae075a..684a30abfc2 100644 --- a/internal/consensus/types/height_vote_set.go +++ b/internal/consensus/types/height_vote_set.go @@ -148,7 +148,7 @@ func (hvs *HeightVoteSet) AddVote(vote *types.Vote, peerID p2p.ID, extEnabled bo voteSet = hvs.getVoteSet(vote.Round, vote.Type) hvs.peerCatchupRounds[peerID] = append(rndz, vote.Round) } - added, err = voteSet.AddVote(vote) + added, err = voteSet.AddVote(vote, false) return added, err } diff --git a/internal/test/commit.go b/internal/test/commit.go index 92b6e0c191c..145cf6fa062 100644 --- a/internal/test/commit.go +++ b/internal/test/commit.go @@ -29,7 +29,7 @@ func MakeCommitFromVoteSet(blockID types.BlockID, voteSet *types.VoteSet, valida return nil, err } vote.Signature = v.Signature - if _, err := voteSet.AddVote(vote); err != nil { + if _, err := voteSet.AddVote(vote, false); err != nil { return nil, err } } diff --git a/privval/file_test.go b/privval/file_test.go index 685da99bbe7..72ce65699ef 100644 --- a/privval/file_test.go +++ b/privval/file_test.go @@ -197,13 +197,13 @@ func TestSignVote(t *testing.T) { err := privVal.SignVote(chainID, v, false) require.NoError(t, err, "expected no error signing vote") vote.Signature = v.Signature - err = vote.ValidateBasic() + err = vote.ValidateBasic(false) require.NoError(t, err) // Verify vote signature pubKey, err := privVal.GetPubKey() require.NoError(t, err) - err = vote.Verify(chainID, pubKey) + err = vote.Verify(chainID, pubKey, false) require.NoError(t, err) // try to sign the same vote again; should be fine diff --git a/types/block.go b/types/block.go index f28070a5eb1..64e770342ff 100644 --- a/types/block.go +++ b/types/block.go @@ -1139,10 +1139,10 @@ func (ec *ExtendedCommit) addSigsToVoteSet(voteSet *VoteSet) { continue // OK, some precommits can be missing. } vote := ec.GetExtendedVote(int32(idx)) - if err := vote.ValidateBasic(); err != nil { + if err := vote.ValidateBasic(false); err != nil { panic(fmt.Errorf("failed to validate vote reconstructed from LastCommit: %w", err)) } - added, err := voteSet.AddVote(vote) + added, err := voteSet.AddVote(vote, false) if !added || err != nil { panic(fmt.Errorf("failed to reconstruct vote set from extended commit: %w", err)) } @@ -1159,10 +1159,12 @@ func (commit *Commit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet { continue // OK, some precommits can be missing. } vote := commit.GetVote(int32(idx)) - if err := vote.ValidateBasic(); err != nil { + if err := vote.ValidateBasic(true); err != nil { panic(fmt.Errorf("failed to validate vote reconstructed from commit: %w", err)) } - added, err := voteSet.AddVote(vote) + + // NOTE: commit is always an aggregate commit! + added, err := voteSet.AddVote(vote, true) if !added || err != nil { panic(fmt.Errorf("failed to reconstruct vote set from commit: %w", err)) } diff --git a/types/block_test.go b/types/block_test.go index a02491dbac3..8345dc8290a 100644 --- a/types/block_test.go +++ b/types/block_test.go @@ -553,7 +553,7 @@ func TestVoteSetToExtendedCommit(t *testing.T) { if testCase.includeExtension { vote.ExtensionSignature = v.ExtensionSignature } - added, err := voteSet.AddVote(vote) + added, err := voteSet.AddVote(vote, false) require.NoError(t, err) require.True(t, added) } diff --git a/types/evidence.go b/types/evidence.go index ecb00840541..c29dc590dc9 100644 --- a/types/evidence.go +++ b/types/evidence.go @@ -132,10 +132,10 @@ func (dve *DuplicateVoteEvidence) ValidateBasic() error { if dve.VoteA == nil || dve.VoteB == nil { return fmt.Errorf("one or both of the votes are empty %v, %v", dve.VoteA, dve.VoteB) } - if err := dve.VoteA.ValidateBasic(); err != nil { + if err := dve.VoteA.ValidateBasic(false); err != nil { return fmt.Errorf("invalid VoteA: %w", err) } - if err := dve.VoteB.ValidateBasic(); err != nil { + if err := dve.VoteB.ValidateBasic(false); err != nil { return fmt.Errorf("invalid VoteB: %w", err) } // Enforce Votes are lexicographically sorted on blockID @@ -172,7 +172,7 @@ func DuplicateVoteEvidenceFromProto(pb *cmtproto.DuplicateVoteEvidence) (*Duplic if err != nil { return nil, err } - if err = vA.ValidateBasic(); err != nil { + if err = vA.ValidateBasic(false); err != nil { return nil, err } } @@ -184,7 +184,7 @@ func DuplicateVoteEvidenceFromProto(pb *cmtproto.DuplicateVoteEvidence) (*Duplic if err != nil { return nil, err } - if err = vB.ValidateBasic(); err != nil { + if err = vB.ValidateBasic(false); err != nil { return nil, err } } diff --git a/types/test_util.go b/types/test_util.go index 993c6364a56..706076d9ff9 100644 --- a/types/test_util.go +++ b/types/test_util.go @@ -50,7 +50,7 @@ func signAddVote(privVal PrivValidator, vote *Vote, voteSet *VoteSet) (bool, err if _, err := SignAndCheckVote(vote, privVal, voteSet.ChainID(), voteSet.extensionsEnabled); err != nil { return false, err } - return voteSet.AddVote(vote) + return voteSet.AddVote(vote, false) } func MakeVote( diff --git a/types/vote.go b/types/vote.go index 1692054966d..2b6caab50e9 100644 --- a/types/vote.go +++ b/types/vote.go @@ -224,11 +224,16 @@ func (vote *Vote) String() string { ) } -func (vote *Vote) verifyAndReturnProto(chainID string, pubKey crypto.PubKey) (*cmtproto.Vote, error) { +func (vote *Vote) verifyAndReturnProto(chainID string, pubKey crypto.PubKey, skipSigVerification bool) (*cmtproto.Vote, error) { if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { return nil, ErrVoteInvalidValidatorAddress } v := vote.ToProto() + + if skipSigVerification { + return v, nil + } + if !pubKey.VerifySignature(VoteSignBytes(chainID, v), vote.Signature) { return nil, ErrVoteInvalidSignature } @@ -238,8 +243,8 @@ func (vote *Vote) verifyAndReturnProto(chainID string, pubKey crypto.PubKey) (*c // Verify checks whether the signature associated with this vote corresponds to // the given chain ID and public key. This function does not validate vote // extension signatures - to do so, use VerifyWithExtension instead. -func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error { - _, err := vote.verifyAndReturnProto(chainID, pubKey) +func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey, skipSigVerification bool) error { + _, err := vote.verifyAndReturnProto(chainID, pubKey, skipSigVerification) return err } @@ -248,7 +253,8 @@ func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error { // given chain ID and public key. We only verify vote extension signatures for // precommits. func (vote *Vote) VerifyVoteAndExtension(chainID string, pubKey crypto.PubKey) error { - v, err := vote.verifyAndReturnProto(chainID, pubKey) + + v, err := vote.verifyAndReturnProto(chainID, pubKey, false) if err != nil { return err } @@ -286,7 +292,7 @@ func (vote *Vote) VerifyExtension(chainID string, pubKey crypto.PubKey) error { // ValidateBasic checks whether the vote is well-formed. It does not, however, // check vote extensions - for vote validation with vote extension validation, // use ValidateWithExtension. -func (vote *Vote) ValidateBasic() error { +func (vote *Vote) ValidateBasic(skipSigVerification bool) error { if !IsVoteTypeValid(vote.Type) { return errors.New("invalid Type") } @@ -320,8 +326,12 @@ func (vote *Vote) ValidateBasic() error { if vote.ValidatorIndex < 0 { return errors.New("negative ValidatorIndex") } - if len(vote.Signature) == 0 { - return errors.New("signature is missing") + + // vote.Signature sometimes it's not there. + if !skipSigVerification { + if len(vote.Signature) == 0 { + return errors.New("signature is missing") + } } if len(vote.Signature) > MaxSignatureSize { diff --git a/types/vote_set.go b/types/vote_set.go index 13c90f15fcc..53d9e2c4dcd 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -156,18 +156,18 @@ func (voteSet *VoteSet) Size() int { // NOTE: vote should not be mutated after adding. // NOTE: VoteSet must not be nil // NOTE: Vote must not be nil. -func (voteSet *VoteSet) AddVote(vote *Vote) (added bool, err error) { +func (voteSet *VoteSet) AddVote(vote *Vote, skipSigVerification bool) (added bool, err error) { if voteSet == nil { panic("AddVote() on nil VoteSet") } voteSet.mtx.Lock() defer voteSet.mtx.Unlock() - return voteSet.addVote(vote) + return voteSet.addVote(vote, skipSigVerification) } // NOTE: Validates as much as possible before attempting to verify the signature. -func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { +func (voteSet *VoteSet) addVote(vote *Vote, skipSigVerification bool) (added bool, err error) { if vote == nil { return false, ErrVoteNil } @@ -221,7 +221,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { return false, fmt.Errorf("failed to verify extended vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) } } else { - if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil { + if err := vote.Verify(voteSet.chainID, val.PubKey, skipSigVerification); 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 || len(vote.Extension) > 0 { diff --git a/types/vote_set_test.go b/types/vote_set_test.go index 347c1e0f7a7..520da8c7e18 100644 --- a/types/vote_set_test.go +++ b/types/vote_set_test.go @@ -536,7 +536,7 @@ func TestVoteSet_VoteExtensionsEnabled(t *testing.T) { vote.ExtensionSignature = v.ExtensionSignature } - added, err := voteSet.AddVote(vote) + added, err := voteSet.AddVote(vote, false) if tc.exepectError { require.Error(t, err) require.False(t, added) diff --git a/types/vote_test.go b/types/vote_test.go index f41d708958a..e56ac42969a 100644 --- a/types/vote_test.go +++ b/types/vote_test.go @@ -276,12 +276,12 @@ func TestVoteVerify(t *testing.T) { vote := examplePrevote() vote.ValidatorAddress = pubkey.Address() - err = vote.Verify("test_chain_id", ed25519.GenPrivKey().PubKey()) + err = vote.Verify("test_chain_id", ed25519.GenPrivKey().PubKey(), false) if assert.Error(t, err) { //nolint:testifylint // require.Error doesn't work with the conditional here assert.Equal(t, ErrVoteInvalidValidatorAddress, err) } - err = vote.Verify("test_chain_id", pubkey) + err = vote.Verify("test_chain_id", pubkey, false) if assert.Error(t, err) { //nolint:testifylint // require.Error doesn't work with the conditional here assert.Equal(t, ErrVoteInvalidSignature, err) } @@ -326,7 +326,7 @@ func TestValidVotes(t *testing.T) { for _, tc := range testCases { signVote(t, privVal, tc.vote) tc.malleateVote(tc.vote) - require.NoError(t, tc.vote.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.NoError(t, tc.vote.ValidateBasic(false), "ValidateBasic for %s", tc.name) require.NoError(t, tc.vote.EnsureExtension(), "EnsureExtension for %s", tc.name) } } @@ -351,13 +351,13 @@ func TestInvalidVotes(t *testing.T) { prevote := examplePrevote() signVote(t, privVal, prevote) tc.malleateVote(prevote) - require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s in invalid prevote", tc.name) + require.Error(t, prevote.ValidateBasic(false), "ValidateBasic for %s in invalid prevote", tc.name) require.NoError(t, prevote.EnsureExtension(), "EnsureExtension for %s in invalid prevote", tc.name) precommit := examplePrecommit() signVote(t, privVal, precommit) tc.malleateVote(precommit) - require.Error(t, precommit.ValidateBasic(), "ValidateBasic for %s in invalid precommit", tc.name) + require.Error(t, precommit.ValidateBasic(false), "ValidateBasic for %s in invalid precommit", tc.name) require.NoError(t, precommit.EnsureExtension(), "EnsureExtension for %s in invalid precommit", tc.name) } } @@ -376,7 +376,7 @@ func TestInvalidPrevotes(t *testing.T) { prevote := examplePrevote() signVote(t, privVal, prevote) tc.malleateVote(prevote) - require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.Error(t, prevote.ValidateBasic(false), "ValidateBasic for %s", tc.name) require.NoError(t, prevote.EnsureExtension(), "EnsureExtension for %s", tc.name) } } @@ -399,7 +399,7 @@ func TestInvalidPrecommitExtensions(t *testing.T) { signVote(t, privVal, precommit) tc.malleateVote(precommit) // ValidateBasic ensures that vote extensions, if present, are well formed - require.Error(t, precommit.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.Error(t, precommit.ValidateBasic(false), "ValidateBasic for %s", tc.name) } } @@ -458,7 +458,7 @@ func TestVoteProtobuf(t *testing.T) { require.Error(t, err) } - err = v.ValidateBasic() + err = v.ValidateBasic(false) if tc.passesValidateBasic { require.NoError(t, err) require.Equal(t, tc.vote, v, tc.msg)