fix(consensus): use signer pubkey to check for unique signatures and optimize performance, close XFN-03 (#1625)

* use signer pubkey to check for unique signatures and optimize performance

* change waitgroup to errgroup

* optimize

* fix typo

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* format files

* after rebase new commits, refactor from snap.NextEpochCandidates to epochInfo.Masternodes

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

consensus/XDPoS/engines/engine_v2/engine.go

@@ -1,14 +1,12 @@
 package engine_v2
 
 import (
-	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"math/big"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strconv"
 	"sync"
 	"time"
@@ -28,7 +26,6 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/log"
 	"github.com/XinFinOrg/XDPoSChain/params"
 	"github.com/XinFinOrg/XDPoSChain/trie"
-	"golang.org/x/sync/errgroup"
 )
 
 type XDPoS_v2 struct {
@@ -754,53 +751,27 @@ func (x *XDPoS_v2) verifyQC(blockChainReader consensus.ChainReader, quorumCert *
 		return errors.New("fail to verify QC due to failure in getting epoch switch info")
 	}
 
-	signatures, duplicates := UniqueSignatures(quorumCert.Signatures)
+	signedVoteObj := types.VoteSigHash(&types.VoteForSign{
-	if len(duplicates) != 0 {
+		ProposedBlockInfo: quorumCert.ProposedBlockInfo,
-		for _, d := range duplicates {
+		GapNumber:         quorumCert.GapNumber,
-			log.Warn("[verifyQC] duplicated signature in QC", "duplicate", common.Bytes2Hex(d))
+	})
-		}
+	start := time.Now()
+	numValidSignatures, err := x.countValidSignatures(signedVoteObj, quorumCert.Signatures, epochInfo.Masternodes)
+	elapsed := time.Since(start)
+	log.Debug("[verifyQC] time verify message signatures of qc", "elapsed", elapsed)
+	if err != nil {
+		log.Error("[verifyQC] Error while verifying QC message signatures", "Error", err)
+		return err
 	}
 
 	qcRound := quorumCert.ProposedBlockInfo.Round
 	certThreshold := x.config.V2.Config(uint64(qcRound)).CertThreshold
-	if (qcRound > 0) && (signatures == nil || float64(len(signatures)) < float64(epochInfo.MasternodesLen)*certThreshold) {
+	if (qcRound > 0) && (float64(numValidSignatures) < float64(epochInfo.MasternodesLen)*certThreshold) {
 		//First V2 Block QC, QC Signatures is initial nil
-		log.Warn("[verifyHeader] Invalid QC Signature is nil or less then config", "QCNumber", quorumCert.ProposedBlockInfo.Number, "LenSignatures", len(signatures), "CertThreshold", float64(epochInfo.MasternodesLen)*certThreshold)
+		log.Warn("[verifyHeader] Invalid QC Signature is nil or less then config", "QCNumber", quorumCert.ProposedBlockInfo.Number, "numValidSignatures", numValidSignatures, "CertThreshold", float64(epochInfo.MasternodesLen)*certThreshold)
 		return utils.ErrInvalidQCSignatures
 	}
-	start := time.Now()
 
-	eg, ctx := errgroup.WithContext(context.Background())
-	eg.SetLimit(runtime.NumCPU())
-	for _, sig := range signatures {
-		eg.Go(func() error {
-			select {
-			case <-ctx.Done():
-				return ctx.Err()
-			default:
-				verified, _, err := x.verifyMsgSignature(types.VoteSigHash(&types.VoteForSign{
-					ProposedBlockInfo: quorumCert.ProposedBlockInfo,
-					GapNumber:         quorumCert.GapNumber,
-				}), sig, epochInfo.Masternodes)
-				if err != nil {
-					log.Error("[verifyQC] Error while verfying QC message signatures", "Error", err)
-					return errors.New("error while verfying QC message signatures")
-				}
-				if !verified {
-					log.Warn("[verifyQC] Signature not verified doing QC verification", "QC", quorumCert)
-					return errors.New("fail to verify QC due to signature mis-match")
-				}
-				return nil
-			}
-		})
-	}
-	err = eg.Wait()
-
-	elapsed := time.Since(start)
-	log.Debug("[verifyQC] time verify message signatures of qc", "elapsed", elapsed)
-	if err != nil {
-		return err
-	}
 	epochSwitchNumber := epochInfo.EpochSwitchBlockInfo.Number.Uint64()
 	gapNumber := epochSwitchNumber - epochSwitchNumber%x.config.Epoch
 	if gapNumber > x.config.Gap {

consensus/XDPoS/engines/engine_v2/timeout.go

@@ -1,10 +1,7 @@
 package engine_v2
 
 import (
-	"context"
-	"errors"
 	"fmt"
-	"runtime"
 	"strconv"
 	"strings"
 	"time"
@@ -14,7 +11,6 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/consensus/XDPoS/utils"
 	"github.com/XinFinOrg/XDPoSChain/core/types"
 	"github.com/XinFinOrg/XDPoSChain/log"
-	"golang.org/x/sync/errgroup"
 )
 
 func (x *XDPoS_v2) VerifyTimeoutMessage(chain consensus.ChainReader, timeoutMsg *types.Timeout) (bool, error) {
@@ -155,70 +151,29 @@ func (x *XDPoS_v2) verifyTC(chain consensus.ChainReader, timeoutCert *types.Time
 		return utils.ErrInvalidTC
 	}
 
-	snap, err := x.getSnapshot(chain, timeoutCert.GapNumber, true)
-	if err != nil {
-		log.Error("[verifyTC] Fail to get snapshot when verifying TC!", "tcGapNumber", timeoutCert.GapNumber)
-		return fmt.Errorf("[verifyTC] Unable to get snapshot, %s", err)
-	}
-	if snap == nil || len(snap.NextEpochCandidates) == 0 {
-		log.Error("[verifyTC] Something wrong with the snapshot from gapNumber", "messageGapNumber", timeoutCert.GapNumber, "snapshot", snap)
-		return errors.New("empty master node lists from snapshot")
-	}
-
-	signatures, duplicates := UniqueSignatures(timeoutCert.Signatures)
-	if len(duplicates) != 0 {
-		for _, d := range duplicates {
-			log.Warn("[verifyTC] duplicated signature in QC", "duplicate", common.Bytes2Hex(d))
-		}
-	}
-
 	epochInfo, err := x.getTCEpochInfo(chain, timeoutCert.Round)
 	if err != nil {
 		return err
 	}
 
-	certThreshold := x.config.V2.Config(uint64(timeoutCert.Round)).CertThreshold
-	if float64(len(signatures)) < float64(epochInfo.MasternodesLen)*certThreshold {
-		log.Warn("[verifyTC] Invalid TC Signature is less or empty", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "certThreshold", float64(epochInfo.MasternodesLen)*certThreshold)
-		return utils.ErrInvalidTCSignatures
-	}
-
 	signedTimeoutObj := types.TimeoutSigHash(&types.TimeoutForSign{
 		Round:     timeoutCert.Round,
 		GapNumber: timeoutCert.GapNumber,
 	})
 
-	eg, ctx := errgroup.WithContext(context.Background())
+	numValidSignatures, err := x.countValidSignatures(signedTimeoutObj, timeoutCert.Signatures, epochInfo.Masternodes)
-	eg.SetLimit(runtime.NumCPU())
-
-	for _, sig := range signatures {
-		eg.Go(func() error {
-			select {
-			case <-ctx.Done():
-				return ctx.Err()
-			default:
-				verified, _, err := x.verifyMsgSignature(signedTimeoutObj, sig, snap.NextEpochCandidates)
 	if err != nil {
-					log.Error("[verifyTC] Error while verifying TC message signatures",
+		log.Error("[verifyTC] Error while verifying TC message signatures", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "Error", err)
-						"tcRound", timeoutCert.Round,
+		return err
-						"tcGapNumber", timeoutCert.GapNumber,
-						"tcSignLen", len(signatures),
-						"error", err)
-					return fmt.Errorf("error while verifying TC message signatures: %w", err)
-				}
-				if !verified {
-					log.Warn("[verifyTC] Signature not verified during TC verification",
-						"tcRound", timeoutCert.Round,
-						"tcGapNumber", timeoutCert.GapNumber,
-						"tcSignLen", len(signatures))
-					return errors.New("fail to verify TC due to signature mis-match")
-				}
-				return nil
-			}
-		})
 	}
 
-	return eg.Wait()
+	certThreshold := x.config.V2.Config(uint64(timeoutCert.Round)).CertThreshold
+	if float64(numValidSignatures) < float64(epochInfo.MasternodesLen)*certThreshold {
+		log.Warn("[verifyTC] Invalid TC Signature is less or empty", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "certThreshold", float64(epochInfo.MasternodesLen)*certThreshold)
+		return utils.ErrInvalidTCSignatures
+	}
+
+	return nil
 }
 
 /*

consensus/XDPoS/engines/engine_v2/utils.go

@@ -1,9 +1,11 @@
 package engine_v2
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"math/big"
+	"runtime"
 
 	"github.com/XinFinOrg/XDPoSChain/accounts"
 	"github.com/XinFinOrg/XDPoSChain/common"
@@ -14,6 +16,7 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/log"
 	"github.com/XinFinOrg/XDPoSChain/rlp"
 	"golang.org/x/crypto/sha3"
+	"golang.org/x/sync/errgroup"
 )
 
 func sigHash(header *types.Header) (hash common.Hash) {
@@ -76,22 +79,6 @@ func decodeMasternodesFromHeaderExtra(checkpointHeader *types.Header) []common.A
 	return masternodes
 }
 
-func UniqueSignatures(signatureSlice []types.Signature) ([]types.Signature, []types.Signature) {
-	keys := make(map[string]bool)
-	list := []types.Signature{}
-	duplicates := []types.Signature{}
-	for _, signature := range signatureSlice {
-		hexOfSig := common.Bytes2Hex(signature)
-		if _, value := keys[hexOfSig]; !value {
-			keys[hexOfSig] = true
-			list = append(list, signature)
-		} else {
-			duplicates = append(duplicates, signature)
-		}
-	}
-	return list, duplicates
-}
-
 func (x *XDPoS_v2) signSignature(signingHash common.Hash) (types.Signature, error) {
 	// Don't hold the signFn for the whole signing operation
 	x.signLock.RLock()
@@ -108,6 +95,53 @@ func (x *XDPoS_v2) signSignature(signingHash common.Hash) (types.Signature, erro
 	return signedHash, nil
 }
 
+func (x *XDPoS_v2) countValidSignatures(messageHash common.Hash, signatures []types.Signature, candidates []common.Address) (int, error) {
+	signatureList := make([]types.Signature, len(signatures))
+	pubkeys := make([]common.Address, len(signatures))
+
+	eg, ctx := errgroup.WithContext(context.Background())
+	eg.SetLimit(runtime.NumCPU())
+	for i, signature := range signatures {
+		eg.Go(func() error {
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			default:
+				verified, signerAddress, err := x.verifyMsgSignature(messageHash, signature, candidates)
+				if err != nil {
+					log.Error("[verifySignatures] Error while verifying QC message signatures", "error", err)
+					return err
+				}
+				if !verified {
+					return fmt.Errorf("signature verification failed, signer is not part of masternode list. Signature: %v, SignedMessage: %v, SignerAddress: %v, Masternodes: %v", signature, messageHash.Hex(), signerAddress, candidates)
+				}
+				signatureList[i] = signature
+				pubkeys[i] = signerAddress
+
+				return nil
+			}
+		})
+	}
+	err := eg.Wait()
+	if err != nil {
+		return 0, err
+	}
+
+	// check uniqueness
+	keys := make(map[common.Address]struct{}, len(signatureList))
+	for i := range signatureList {
+		pubkeyHex := pubkeys[i]
+		if _, ok := keys[pubkeyHex]; !ok {
+			keys[pubkeyHex] = struct{}{}
+		} else {
+			log.Warn("[verifySignatures] duplicate signing found", "pubkey", pubkeyHex, "signedMessage", messageHash.Hex(), "signature", signatureList[i])
+			return 0, fmt.Errorf("duplicate signing found, pubkey: %v, message: %v, signature: %v", pubkeyHex, messageHash.Hex(), common.Bytes2Hex(signatureList[i]))
+		}
+	}
+
+	return len(keys), nil
+}
+
 func (x *XDPoS_v2) verifyMsgSignature(signedHashToBeVerified common.Hash, signature types.Signature, masternodes []common.Address) (bool, common.Address, error) {
 	var signerAddress common.Address
 	if len(masternodes) == 0 {
@@ -126,7 +160,7 @@ func (x *XDPoS_v2) verifyMsgSignature(signedHashToBeVerified common.Hash, signat
 		}
 	}
 
-	log.Warn("[verifyMsgSignature] signer is not part of masternode list", "signer", signerAddress, "masternodes", masternodes)
+	log.Warn("[verifyMsgSignature] signer is not part of masternode list", "signer", signerAddress, "masternodes", masternodes, "signature", common.Bytes2Hex(signature), "signedMessage", signedHashToBeVerified.Hex())
 	return false, signerAddress, nil
 }
 

consensus/tests/engine_v2_tests/verify_header_test.go

@@ -394,7 +394,8 @@ func TestShouldFailIfNotEnoughQCSignatures(t *testing.T) {
 	headerWithDuplicatedSignatures.Extra = extraInBytes
 	// Happy path
 	err = adaptor.VerifyHeader(blockchain, headerWithDuplicatedSignatures, true)
-	assert.Equal(t, utils.ErrInvalidQCSignatures, err)
+	assert.ErrorContains(t, err, "duplicate signing found")
+
 }
 
 func TestShouldVerifyHeaders(t *testing.T) {