Skip to content

Commit

Permalink
Added bool flag to enable/disable signature verification when convert…
Browse files Browse the repository at this point in the history
…ing a commit to

a vote set.
  • Loading branch information
alesforz committed Jan 10, 2025
1 parent 0203b88 commit 701204d
Show file tree
Hide file tree
Showing 14 changed files with 49 additions and 37 deletions.
2 changes: 1 addition & 1 deletion internal/consensus/reactor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion internal/consensus/reactor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion internal/consensus/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/consensus/types/height_vote_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion internal/test/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
4 changes: 2 additions & 2 deletions privval/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions types/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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))
}
Expand Down
2 changes: 1 addition & 1 deletion types/block_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
8 changes: 4 additions & 4 deletions types/evidence.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand Down
2 changes: 1 addition & 1 deletion types/test_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
24 changes: 17 additions & 7 deletions types/vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions types/vote_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion types/vote_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 8 additions & 8 deletions types/vote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
}
Expand All @@ -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)
}
}
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 701204d

Please sign in to comment.