refactor(types): remove QuorumSingsVerifier (#727)

* refactor(types): remove QuorumSingsVerifier to simplify code

* test(consensus): fix tests

abci/example/kvstore/verify.go

@@ -17,7 +17,6 @@ func (app *Application) verifyBlockCommit(qsd types.QuorumSignData, commit abci.
 	if !bytes.Equal(commit.QuorumHash, vsu.QuorumHash) {
 		return fmt.Errorf("mismatch quorum hashes got %X, want %X", commit.QuorumHash, vsu.QuorumHash)
 	}
-	verifier := types.NewQuorumSignsVerifier(qsd)
 	pubKey, err := encoding.PubKeyFromProto(vsu.ThresholdPublicKey)
 	if err != nil {
 		return err
@@ -28,7 +27,7 @@ func (app *Application) verifyBlockCommit(qsd types.QuorumSignData, commit abci.
 		extSigs = append(extSigs, ext.Signature)
 	}
 
-	return verifier.Verify(pubKey, types.QuorumSigns{
+	return qsd.Verify(pubKey, types.QuorumSigns{
 		BlockSign:               commit.BlockSignature,
 		VoteExtensionSignatures: extSigs,
 	})

internal/consensus/state_test.go

@@ -3334,12 +3334,7 @@ func mockProposerApplicationCalls(t *testing.T, m *abcimocks.Application, round
 
 	if final {
 		m.On("ExtendVote", mock.Anything, roundMatcher).
-			Return(&abci.ResponseExtendVote{
-				VoteExtensions: []*abci.ExtendVoteExtension{{
-					Type:      tmproto.VoteExtensionType_THRESHOLD_RECOVER,
-					Extension: []byte("extension"),
-				}},
-			}, nil).Once()
+			Return(&abci.ResponseExtendVote{}, nil).Once()
 
 		m.On("VerifyVoteExtension", mock.Anything, roundMatcher).
 			Return(&abci.ResponseVerifyVoteExtension{

types/block.go

@@ -790,10 +790,11 @@ func (commit *Commit) ToCommitInfo() types.CommitInfo {
 // GetCanonicalVote returns the message that is being voted on in the form of a vote without signatures.
 func (commit *Commit) GetCanonicalVote() *Vote {
 	return &Vote{
-		Type:    tmproto.PrecommitType,
-		Height:  commit.Height,
-		Round:   commit.Round,
-		BlockID: commit.BlockID,
+		Type:           tmproto.PrecommitType,
+		Height:         commit.Height,
+		Round:          commit.Round,
+		BlockID:        commit.BlockID,
+		VoteExtensions: VoteExtensionsFromProto(commit.ThresholdVoteExtensions...),
 	}
 }
 

types/quorum.go

@@ -3,9 +3,6 @@ package types
 import (
 	"bytes"
 	"fmt"
-
-	"github.com/dashpay/tenderdash/crypto"
-	"github.com/dashpay/tenderdash/libs/log"
 )
 
 // CommitSigns is used to combine threshold signatures and quorum-hash that were used
@@ -51,113 +48,3 @@ func NewQuorumSignsFromCommit(commit *Commit) QuorumSigns {
 		VoteExtensionSignatures: sigs,
 	}
 }
-
-// QuorumSingsVerifier ...
-type QuorumSingsVerifier struct {
-	QuorumSignData
-	shouldVerifyBlock          bool
-	shouldVerifyVoteExtensions bool
-	logger                     log.Logger
-}
-
-// WithVerifyExtensions sets a flag that tells QuorumSingsVerifier to verify vote-extension signatures or not
-func WithVerifyExtensions(shouldVerify bool) func(*QuorumSingsVerifier) {
-	return func(verifier *QuorumSingsVerifier) {
-		verifier.shouldVerifyVoteExtensions = shouldVerify
-	}
-}
-
-// WithVerifyBlock sets a flag that tells QuorumSingsVerifier to verify block signature or not
-func WithVerifyBlock(shouldVerify bool) func(*QuorumSingsVerifier) {
-	return func(verifier *QuorumSingsVerifier) {
-		verifier.shouldVerifyBlock = shouldVerify
-	}
-}
-
-// WithVerifyReachedQuorum sets a flag that tells QuorumSingsVerifier to verify
-// vote-extension and stateID signatures or not
-func WithVerifyReachedQuorum(quorumReached bool) func(*QuorumSingsVerifier) {
-	return func(verifier *QuorumSingsVerifier) {
-		verifier.shouldVerifyVoteExtensions = quorumReached
-	}
-}
-
-// WithLogger sets a logger
-func WithLogger(logger log.Logger) func(*QuorumSingsVerifier) {
-	return func(verifier *QuorumSingsVerifier) {
-		verifier.logger = logger
-	}
-}
-
-// NewQuorumSignsVerifier creates and returns an instance of QuorumSingsVerifier that is used for verification
-// quorum signatures
-func NewQuorumSignsVerifier(quorumData QuorumSignData, opts ...func(*QuorumSingsVerifier)) *QuorumSingsVerifier {
-	verifier := &QuorumSingsVerifier{
-		QuorumSignData:             quorumData,
-		shouldVerifyBlock:          true,
-		shouldVerifyVoteExtensions: true,
-		logger:                     log.NewNopLogger(),
-	}
-	for _, opt := range opts {
-		opt(verifier)
-	}
-	return verifier
-}
-
-// Verify verifies quorum data using public key and passed signatures
-func (q *QuorumSingsVerifier) Verify(pubKey crypto.PubKey, signs QuorumSigns) error {
-	err := q.verifyBlock(pubKey, signs)
-	if err != nil {
-		return err
-	}
-	return q.verifyVoteExtensions(pubKey, signs)
-}
-
-func (q *QuorumSingsVerifier) verifyBlock(pubKey crypto.PubKey, signs QuorumSigns) error {
-	if !q.shouldVerifyBlock {
-		return nil
-	}
-	if !pubKey.VerifySignatureDigest(q.Block.SignHash, signs.BlockSign) {
-		return fmt.Errorf(
-			"threshold block signature is invalid: (%X) signID=%X: %w",
-			q.Block.Msg,
-			q.Block.SignHash,
-			ErrVoteInvalidBlockSignature,
-		)
-	}
-	return nil
-}
-
-// verify threshold-recoverable vote extensions signatures
-func (q *QuorumSingsVerifier) verifyVoteExtensions(
-	pubKey crypto.PubKey,
-	signs QuorumSigns,
-) error {
-	if !q.shouldVerifyVoteExtensions {
-		return nil
-	}
-
-	thresholdSigs := signs.VoteExtensionSignatures
-	signItems := q.VoteExtensionSignItems
-	if len(signItems) == 0 {
-		return nil
-	}
-	if len(signItems) != len(thresholdSigs) {
-		return fmt.Errorf("count of threshold vote extension signatures (%d) doesn't match recoverable vote extensions (%d)",
-			len(thresholdSigs), len(signItems),
-		)
-	}
-
-	for i, sig := range thresholdSigs {
-		if !pubKey.VerifySignatureDigest(signItems[i].SignHash, sig) {
-			return fmt.Errorf("vote-extension %d signature is invalid: raw %X, signature %X, pubkey %X, sigHash: %X",
-				i,
-				signItems[i].Msg,
-				sig,
-				pubKey.Bytes(),
-				signItems[i].SignHash,
-			)
-		}
-	}
-	return nil
-}

types/quorum_sign_data.go

@@ -6,10 +6,12 @@ import (
 
 	bls "github.com/dashpay/bls-signatures/go-bindings"
 	"github.com/dashpay/dashd-go/btcjson"
+	"github.com/hashicorp/go-multierror"
+	"github.com/rs/zerolog"
+
 	"github.com/dashpay/tenderdash/crypto"
 	tmbytes "github.com/dashpay/tenderdash/libs/bytes"
 	"github.com/dashpay/tenderdash/proto/tendermint/types"
-	"github.com/rs/zerolog"
 )
 
 // QuorumSignData holds data which is necessary for signing and verification block, state, and each vote-extension in a list
@@ -40,9 +42,52 @@ func (q QuorumSignData) SignWithPrivkey(key crypto.PrivKey) (QuorumSigns, error)
 	return signs, nil
 }
 
-// Verify verifies a quorum signatures: block, state and vote-extensions
-func (q QuorumSignData) Verify(pubKey crypto.PubKey, signs QuorumSigns) error {
-	return NewQuorumSignsVerifier(q).Verify(pubKey, signs)
+// Verify verifies a block and threshold vote extensions quorum signatures.
+// It needs quorum to be reached so that we have enough signatures to verify.
+func (q QuorumSignData) Verify(pubKey crypto.PubKey, signatures QuorumSigns) error {
+	var err error
+	if err1 := q.VerifyBlock(pubKey, signatures); err1 != nil {
+		err = multierror.Append(err, err1)
+	}
+
+	if err1 := q.VerifyVoteExtensions(pubKey, signatures); err1 != nil {
+		err = multierror.Append(err, err1)
+	}
+
+	return err
+}
+
+// VerifyBlock verifies block signature
+func (q QuorumSignData) VerifyBlock(pubKey crypto.PubKey, signatures QuorumSigns) error {
+	if !q.Block.VerifySignature(pubKey, signatures.BlockSign) {
+		return ErrVoteInvalidBlockSignature
+	}
+
+	return nil
+}
+
+// VerifyVoteExtensions verifies threshold vote extensions signatures
+func (q QuorumSignData) VerifyVoteExtensions(pubKey crypto.PubKey, signatures QuorumSigns) error {
+	if len(q.VoteExtensionSignItems) != len(signatures.VoteExtensionSignatures) {
+		return fmt.Errorf("count of vote extension signatures (%d) doesn't match recoverable vote extensions (%d)",
+			len(signatures.VoteExtensionSignatures), len(q.VoteExtensionSignItems),
+		)
+	}
+
+	var err error
+	for i, signItem := range q.VoteExtensionSignItems {
+		if !signItem.VerifySignature(pubKey, signatures.VoteExtensionSignatures[i]) {
+			err = multierror.Append(err, fmt.Errorf("vote-extension %d signature is invalid: pubkey %X, raw msg: %X, sigHash: %X, signature %X",
+				i,
+				pubKey.Bytes(),
+				signItem.Msg,
+				signItem.MsgHash,
+				signatures.VoteExtensionSignatures[i],
+			))
+		}
+	}
+
+	return err
 }
 
 // MakeQuorumSignsWithVoteSet creates and returns QuorumSignData struct built with a vote-set and an added vote
@@ -209,3 +254,8 @@ func (i *SignItem) UpdateSignHash(reverse bool) {
 
 	i.SignHash = signHash
 }
+
+// VerifySignature verifies signature for a sign item
+func (i *SignItem) VerifySignature(pubkey crypto.PubKey, sig []byte) bool {
+	return pubkey.VerifySignatureDigest(i.SignHash, sig)
+}

types/validation_test.go

@@ -68,8 +68,7 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) {
 		expErr      bool
 	}{
 		{"good", chainID, vote.BlockID, vote.Height, commit, false},
-
-		{"threshold block signature is invalid", "EpsilonEridani", vote.BlockID, vote.Height, commit, true},
+		{"invalid block signature", "EpsilonEridani", vote.BlockID, vote.Height, commit, true},
 		{"wrong block ID", chainID, makeBlockIDRandom(), vote.Height, commit, true},
 		{
 			description: "invalid commit -- wrong block ID",
@@ -84,17 +83,17 @@ func TestValidatorSet_VerifyCommit_All(t *testing.T) {
 			expErr: true,
 		},
 		{"wrong height", chainID, vote.BlockID, vote.Height - 1, commit, true},
-
-		{"threshold block signature is invalid", chainID, vote.BlockID, vote.Height,
+		// block sign malformed
+		{"invalid block signature", chainID, vote.BlockID, vote.Height,
 			NewCommit(vote.Height, vote.Round, vote.BlockID, vote.VoteExtensions,
 				&CommitSigns{
 					QuorumHash: quorumHash,
 					QuorumSigns: QuorumSigns{
 						BlockSign:               []byte("invalid block signature"),
 						VoteExtensionSignatures: sig.VoteExtensionSignatures,
 					}}), true},
-
-		{"threshold block signature is invalid", chainID, vote.BlockID, vote.Height,
+		// quorum signs are replaced with vote2 non-threshold signature
+		{"invalid block signature", chainID, vote.BlockID, vote.Height,
 			NewCommit(vote.Height, vote.Round, vote.BlockID,
 				vote.VoteExtensions,
 				&CommitSigns{
@@ -143,7 +142,7 @@ func TestValidatorSet_VerifyCommit_CheckThresholdSignatures(t *testing.T) {
 	commit.ThresholdBlockSignature = v.BlockSignature
 	err = valSet.VerifyCommit(chainID, blockID, h, commit)
 	if assert.Error(t, err) {
-		assert.Contains(t, err.Error(), "threshold block signature is invalid")
+		assert.Contains(t, err.Error(), "invalid block signature")
 	}
 
 	goodVote := voteSet.GetByIndex(0)

types/vote.go

@@ -172,11 +172,9 @@ func (vote *Vote) String() string {
 	)
 }
 
-// VerifyVoteAndExtension performs the same verification as Verify, but
-// additionally checks whether the vote extension signature corresponds to the
-// given chain ID and public key. We only verify vote extension signatures for
-// precommits.
-func (vote *Vote) VerifyWithExtension(
+// Verify performs vote signature verification. It checks whether the block signature
+// and vote extensions signatures correspond to the given chain ID and public key.
+func (vote *Vote) Verify(
 	chainID string,
 	quorumType btcjson.LLMQType,
 	quorumHash crypto.QuorumHash,
@@ -191,28 +189,8 @@ func (vote *Vote) VerifyWithExtension(
 	if err != nil {
 		return err
 	}
-	return vote.verifySign(pubKey, quorumSignData, WithVerifyExtensions(vote.Type == tmproto.PrecommitType))
-}
-
-func (vote *Vote) Verify(
-	chainID string,
-	quorumType btcjson.LLMQType,
-	quorumHash []byte,
-	pubKey crypto.PubKey,
-	proTxHash crypto.ProTxHash,
-	_stateID tmproto.StateID,
-) error {
-	err := vote.verifyBasic(proTxHash, pubKey)
-	if err != nil {
-		return err
-	}
-	quorumSignData, err := MakeQuorumSigns(chainID, quorumType, quorumHash, vote.ToProto())
-	if err != nil {
-		return err
-	}
 
-	// TODO check why we don't verify extensions here
-	return vote.verifySign(pubKey, quorumSignData, WithVerifyExtensions(false))
+	return quorumSignData.Verify(pubKey, vote.makeQuorumSigns())
 }
 
 func (vote *Vote) verifyBasic(proTxHash ProTxHash, pubKey crypto.PubKey) error {
@@ -223,6 +201,10 @@ func (vote *Vote) verifyBasic(proTxHash ProTxHash, pubKey crypto.PubKey) error {
 		return ErrVoteInvalidValidatorPubKeySize
 	}
 
+	if vote.Type != tmproto.PrecommitType && vote.VoteExtensions.Len() > 0 {
+		return ErrVoteInvalidExtension
+	}
+
 	return nil
 }
 
@@ -236,23 +218,8 @@ func (vote *Vote) VerifyExtensionSign(chainID string, pubKey crypto.PubKey, quor
 	if err != nil {
 		return err
 	}
-	verifier := NewQuorumSignsVerifier(
-		quorumSignData,
-		WithVerifyBlock(false),
-	)
-	return verifier.Verify(pubKey, vote.makeQuorumSigns())
-}
 
-func (vote *Vote) verifySign(
-	pubKey crypto.PubKey,
-	quorumSignData QuorumSignData,
-	opts ...func(verifier *QuorumSingsVerifier),
-) error {
-	verifier := NewQuorumSignsVerifier(
-		quorumSignData,
-		opts...,
-	)
-	return verifier.Verify(pubKey, vote.makeQuorumSigns())
+	return quorumSignData.VerifyVoteExtensions(pubKey, vote.makeQuorumSigns())
 }
 
 func (vote *Vote) makeQuorumSigns() QuorumSigns {