Merge pull request #7918 from onflow/yurii/6127-vote-double-counting

[BFT] Extend `CombinedVoteProcessorV3`

Author: durkmurder

consensus/hotstuff/model/errors.go

@@ -307,8 +307,9 @@ func IsByzantineThresholdExceededError(err error) bool {
 	return errors.As(err, &target)
 }
 
-// DoubleVoteError indicates that a consensus replica has voted for two different
-// blocks, or has provided two semantically different votes for the same block.
+// DoubleVoteError indicates that a consensus replica has voted for two different blocks
+// in the same view, or has provided two semantically different votes for the same block.
+// This is a PROTOCOL VIOLATION (slashable equivocation attack).
 type DoubleVoteError struct {
 	FirstVote       *Vote
 	ConflictingVote *Vote
@@ -340,6 +341,9 @@ func (e DoubleVoteError) Unwrap() error {
 	return e.err
 }
 
+// NewDoubleVoteErrorf creates an error signalling that a consensus replica has voted for two different
+// blocks in the same view, or has provided two semantically different votes for the same block.
+// This is a PROTOCOL VIOLATION (slashable equivocation attack).
 func NewDoubleVoteErrorf(firstVote, conflictingVote *Vote, msg string, args ...interface{}) error {
 	return DoubleVoteError{
 		FirstVote:       firstVote,
@@ -370,7 +374,7 @@ func IsDuplicatedSignerError(err error) bool {
 	return errors.As(err, &e)
 }
 
-// InvalidSignatureIncludedError indicates that some signatures, included via TrustedAdd, are invalid
+// InvalidSignatureIncludedError indicates that some signatures are invalid
 type InvalidSignatureIncludedError struct {
 	err error
 }

consensus/hotstuff/signature/randombeacon_inspector.go

@@ -5,16 +5,19 @@ import (
 
 	"github.com/onflow/crypto"
 
+	"github.com/onflow/flow-go/consensus/hotstuff"
 	"github.com/onflow/flow-go/consensus/hotstuff/model"
 	"github.com/onflow/flow-go/module/signature"
 )
 
-// randomBeaconInspector implements hotstuff.RandomBeaconInspector interface.
+// randomBeaconInspector implements [hotstuff.RandomBeaconInspector] interface.
 // All methods of this structure are concurrency-safe.
 type randomBeaconInspector struct {
 	inspector crypto.ThresholdSignatureInspector
 }
 
+var _ hotstuff.RandomBeaconInspector = (*randomBeaconInspector)(nil)
+
 // NewRandomBeaconInspector instantiates a new randomBeaconInspector.
 // The constructor errors with a `model.ConfigurationError` in any of the following cases
 //   - n is not between `ThresholdSignMinSize` and `ThresholdSignMaxSize`,
@@ -50,8 +53,8 @@ func NewRandomBeaconInspector(
 // execute the business logic, without interfering with each other).
 // It allows concurrent verification of the given signature.
 // Returns :
-//   - model.InvalidSignerError if signerIndex is invalid
-//   - model.ErrInvalidSignature if signerID is valid but signature is cryptographically invalid
+//   - [model.InvalidSignerError] if signerIndex is invalid
+//   - [model.ErrInvalidSignature] if signerID is valid but signature is cryptographically invalid
 //   - other error if there is an unexpected exception.
 func (r *randomBeaconInspector) Verify(signerIndex int, share crypto.Signature) error {
 	valid, err := r.inspector.VerifyShare(signerIndex, share)
@@ -75,14 +78,14 @@ func (r *randomBeaconInspector) Verify(signerIndex int, share crypto.Signature)
 // are returned) through a post-check (verifying the threshold signature
 // _after_ reconstruction before returning it).
 // The function is thread-safe but locks its internal state, thereby permitting only
-// one routine at a time to add a signature.
+// one routine at a time to add a signature (inherited from the underlying inspector).
 // Returns:
 //   - (true, nil) if the signature has been added, and enough shares have been collected.
 //   - (false, nil) if the signature has been added, but not enough shares were collected.
 //
 // The following errors are expected during normal operations:
-//   - model.InvalidSignerError if signerIndex is invalid (out of the valid range)
-//   - model.DuplicatedSignerError if the signer has been already added
+//   - [model.InvalidSignerError] if signerIndex is invalid (out of the valid range)
+//   - [model.DuplicatedSignerError] if the signer has been already added
 //   - other error if there is an unexpected exception.
 func (r *randomBeaconInspector) TrustedAdd(signerIndex int, share crypto.Signature) (bool, error) {
 	// Trusted add to the crypto layer
@@ -112,8 +115,8 @@ func (r *randomBeaconInspector) EnoughShares() bool {
 //
 // Returns:
 //   - (signature, nil) if no error occurred
-//   - (nil, model.InsufficientSignaturesError) if not enough shares were collected
-//   - (nil, model.InvalidSignatureIncluded) if at least one collected share does not serialize to a valid BLS signature,
+//   - (nil, [model.InsufficientSignaturesError]) if not enough shares were collected
+//   - (nil, [model.InvalidSignatureIncluded]) if at least one collected share does not serialize to a valid BLS signature,
 //     or if the constructed signature failed to verify against the group public key and stored message. This post-verification
 //     is required  for safety, as `TrustedAdd` allows adding invalid signatures.
 //   - (nil, error) for any other unexpected error.

consensus/hotstuff/signature/randombeacon_reconstructor.go

@@ -11,8 +11,8 @@ import (
 	"github.com/onflow/flow-go/state/protocol"
 )
 
-// RandomBeaconReconstructor implements hotstuff.RandomBeaconReconstructor.
-// The implementation wraps the hotstuff.RandomBeaconInspector and translates the signer identity into signer index.
+// RandomBeaconReconstructor implements [hotstuff.RandomBeaconReconstructor].
+// The implementation wraps the [hotstuff.RandomBeaconInspector] and translates the signer identity into signer index.
 // It has knowledge about DKG to be able to map signerID to signerIndex
 type RandomBeaconReconstructor struct {
 	hotstuff.RandomBeaconInspector              // a stateful object for this epoch. It's used for both verifying all sig shares and reconstructing the threshold signature.
@@ -33,8 +33,8 @@ func NewRandomBeaconReconstructor(dkg hotstuff.DKG, randomBeaconInspector hotstu
 // execute the business logic, without interfering with each other).
 // It allows concurrent verification of the given signature.
 // Returns :
-//   - model.InvalidSignerError if signerID is invalid
-//   - model.ErrInvalidSignature if signerID is valid but signature is cryptographically invalid
+//   - [model.InvalidSignerError] if signerID is invalid
+//   - [model.ErrInvalidSignature] if signerID is valid but signature is cryptographically invalid
 //   - other error if there is an unexpected exception.
 func (r *RandomBeaconReconstructor) Verify(signerID flow.Identifier, sig crypto.Signature) error {
 	signerIndex, err := r.dkg.Index(signerID)
@@ -60,8 +60,8 @@ func (r *RandomBeaconReconstructor) Verify(signerID flow.Identifier, sig crypto.
 //   - (false, nil) if the signature has been added, but not enough shares were collected.
 //
 // The following errors are expected during normal operations:
-//   - model.InvalidSignerError if signerIndex is invalid (out of the valid range)
-//   - model.DuplicatedSignerError if the signer has been already added
+//   - [model.InvalidSignerError] if signerIndex is invalid (out of the valid range)
+//   - [model.DuplicatedSignerError] if the signer has been already added
 //   - other error if there is an unexpected exception.
 func (r *RandomBeaconReconstructor) TrustedAdd(signerID flow.Identifier, sig crypto.Signature) (bool, error) {
 	signerIndex, err := r.dkg.Index(signerID)

consensus/hotstuff/signature/weighted_signature_aggregator.go

@@ -19,26 +19,24 @@ type signerInfo struct {
 	index  int
 }
 
-// WeightedSignatureAggregator implements consensus/hotstuff.WeightedSignatureAggregator.
-// It is a wrapper around module/signature.SignatureAggregatorSameMessage, which implements a
+// WeightedSignatureAggregator implements [hotstuff.WeightedSignatureAggregator].
+// It is a wrapper around [signature.SignatureAggregatorSameMessage], implementing a
 // mapping from node IDs (as used by HotStuff) to index-based addressing of authorized
-// signers (as used by SignatureAggregatorSameMessage).
+// signers (as used by [signature.SignatureAggregatorSameMessage]).
 //
-// Similarly to module/signature.SignatureAggregatorSameMessage, this module assumes proofs of possession (PoP)
-// of all identity public keys are valid.
+// We delegate the handling of duplicate signatures to the underlying [signature.SignatureAggregatorSameMessage].
+// NOTE: This is possible, because [signature.SignatureAggregatorSameMessage] does not support signatures with
+// multiplicity higher than 1, i.e. each signer is allowed to sign at most once. Should this constraint every be
+// changed in [signature.SignatureAggregatorSameMessage], this module would need to be updated accordingly.
+//
+// Similarly to [signature.SignatureAggregatorSameMessage], this module assumes proofs
+// of possession (PoP) of all identity public keys are valid.
 type WeightedSignatureAggregator struct {
 	aggregator  *signature.SignatureAggregatorSameMessage // low level crypto BLS aggregator, agnostic of weights and flow IDs
 	ids         flow.IdentityList                         // all possible ids (only gets updated by constructor)
 	idToInfo    map[flow.Identifier]signerInfo            // auxiliary map to lookup signer weight and index by ID (only gets updated by constructor)
 	totalWeight uint64                                    // weight collected (gets updated)
 	lock        sync.RWMutex                              // lock for atomic updates to totalWeight and collectedIDs
-
-	// collectedIDs tracks the Identities of all nodes whose signatures have been collected so far.
-	// The reason for tracking the duplicate signers at this module level is that having no duplicates
-	// is a Hotstuff constraint, rather than a cryptographic aggregation constraint. We are planning to
-	// extend the cryptographic primitives to support multiplicity higher than 1 in the future.
-	// Therefore, we already add the logic for identifying duplicates here.
-	collectedIDs map[flow.Identifier]struct{} // map of collected IDs (gets updated)
 }
 
 var _ hotstuff.WeightedSignatureAggregator = (*WeightedSignatureAggregator)(nil)
@@ -81,17 +79,16 @@ func NewWeightedSignatureAggregator(
 	}
 
 	return &WeightedSignatureAggregator{
-		aggregator:   agg,
-		ids:          ids,
-		idToInfo:     idToInfo,
-		collectedIDs: make(map[flow.Identifier]struct{}),
+		aggregator: agg,
+		ids:        ids,
+		idToInfo:   idToInfo,
 	}, nil
 }
 
 // Verify verifies the signature under the stored public keys and message.
 // Expected errors during normal operations:
-//   - model.InvalidSignerError if signerID is invalid (not a consensus participant)
-//   - model.ErrInvalidSignature if signerID is valid but signature is cryptographically invalid
+//   - [model.InvalidSignerError] if signerID is invalid (not a consensus participant)
+//   - [model.ErrInvalidSignature] if signerID is valid but signature is cryptographically invalid
 //
 // The function is thread-safe.
 func (w *WeightedSignatureAggregator) Verify(signerID flow.Identifier, sig crypto.Signature) error {
@@ -116,8 +113,8 @@ func (w *WeightedSignatureAggregator) Verify(signerID flow.Identifier, sig crypt
 // The total weight of all collected signatures (excluding duplicates) is returned regardless
 // of any returned error.
 // The function errors with:
-//   - model.InvalidSignerError if signerID is invalid (not a consensus participant)
-//   - model.DuplicatedSignerError if the signer has been already added
+//   - [model.InvalidSignerError] if signerID is invalid (not a consensus participant)
+//   - [model.DuplicatedSignerError] if the signer has been already added
 //
 // The function is thread-safe.
 func (w *WeightedSignatureAggregator) TrustedAdd(signerID flow.Identifier, sig crypto.Signature) (uint64, error) {
@@ -130,19 +127,20 @@ func (w *WeightedSignatureAggregator) TrustedAdd(signerID flow.Identifier, sig c
 	w.lock.Lock()
 	defer w.lock.Unlock()
 
-	// check for repeated occurrence of signerID (in anticipation of aggregator supporting multiplicities larger than 1 in the future)
-	if _, duplicate := w.collectedIDs[signerID]; duplicate {
-		return w.totalWeight, model.NewDuplicatedSignerErrorf("signature from %v was already added", signerID)
-	}
-
+	// NOTE: We delegate the handling of duplicate signatures to the underlying [signature.SignatureAggregatorSameMessage].
+	// This is valid only because [signature.SignatureAggregatorSameMessage] does not support signatures with multiplicity
+	// higher than 1, i.e. each signer is allowed to sign at most once. Should this constraint every be relaxed in
+	// [signature.SignatureAggregatorSameMessage], we need to implement it here in the `WeightedSignatureAggregator`,
+	// because in the context of HotStuff each consensus replica is allowed to vote at most once.
 	err := w.aggregator.TrustedAdd(info.index, sig)
 	if err != nil {
-		// During normal operations, signature.InvalidSignerIdxError or signature.DuplicatedSignerIdxError should never occur.
+		if signature.IsDuplicatedSignerIdxError(err) {
+			return w.totalWeight, model.NewDuplicatedSignerErrorf("signature from %v was already added", signerID)
+		}
+		// During normal operations, signature.InvalidSignerIdxError should never occur.
 		return w.totalWeight, fmt.Errorf("unexpected exception while trusted add of signature from %v: %w", signerID, err)
 	}
 	w.totalWeight += info.weight
-	w.collectedIDs[signerID] = struct{}{}
-
 	return w.totalWeight, nil
 }
 
@@ -158,12 +156,12 @@ func (w *WeightedSignatureAggregator) TotalWeight() uint64 {
 // The function performs a final verification and errors if the aggregated signature is invalid. This is
 // required for the function safety since `TrustedAdd` allows adding invalid signatures.
 // The function errors with:
-//   - model.InsufficientSignaturesError if no signatures have been added yet
-//   - model.InvalidSignatureIncludedError if:
+//   - [model.InsufficientSignaturesError] if no signatures have been added yet
+//   - [model.InvalidSignatureIncludedError] if:
 //   - some signature(s), included via TrustedAdd, fail to deserialize (regardless of the aggregated public key)
 //     -- or all signatures deserialize correctly but some signature(s), included via TrustedAdd, are
 //     invalid (while aggregated public key is valid)
-//     -- model.InvalidAggregatedKeyError if all signatures deserialize correctly but the signer's
+//     -- [model.InvalidAggregatedKeyError] if all signatures deserialize correctly but the signer's
 //     staking public keys sum up to an invalid key (BLS identity public key).
 //     Any aggregated signature would fail the cryptographic verification under the identity public
 //     key and therefore such signature is considered invalid. Such scenario can only happen if

consensus/hotstuff/vote_collector.go

@@ -59,14 +59,15 @@ type VoteCollector interface {
 	// ProcessBlock performs validation of block signature and processes block with respected collector.
 	// Calling this function will mark conflicting collector as stale and change state of valid collectors
 	// It returns nil if the block is valid.
-	// It returns model.InvalidProposalError if block is invalid.
+	// It returns [model.InvalidProposalError] if block is invalid.
 	// It returns other error if there is exception processing the block.
 	ProcessBlock(block *model.SignedProposal) error
 
-	// AddVote adds a vote to the collector
-	// When enough votes have been added to produce a QC, the QC will be created asynchronously, and
-	// passed to EventLoop through a callback.
-	// No errors are expected during normal operations.
+	// AddVote adds a vote to the vote collector. The vote must be for the `VoteCollector`'s view (otherwise,
+	// an exception is returned). When enough votes have been added to produce a QC, the QC will be created
+	// asynchronously, and passed to EventLoop through a callback.
+	// All byzantine edge cases are handled internally via callbacks to notifier.
+	// Under normal execution only exceptions are propagated to caller.
 	AddVote(vote *model.Vote) error
 
 	// RegisterVoteConsumer registers a VoteConsumer. Upon registration, the collector
@@ -88,19 +89,27 @@ type VoteCollector interface {
 // Depending on their implementation, a VoteProcessor might drop votes or attempt to construct a QC.
 type VoteProcessor interface {
 	// Process performs processing of single vote. This function is safe to call from multiple goroutines.
+	//
 	// Expected error returns during normal operations:
-	// * VoteForIncompatibleBlockError - submitted vote for incompatible block
-	// * VoteForIncompatibleViewError - submitted vote for incompatible view
-	// * model.InvalidVoteError - submitted vote with invalid signature
-	// * model.DuplicatedSignerError - vote from a signer whose vote was previously already processed
+	//   - [VoteForIncompatibleBlockError] if vote is for incompatible block
+	//   - [VoteForIncompatibleViewError] if vote is for incompatible view
+	//   - [model.InvalidVoteError] if vote has invalid signature
+	//   - [model.DuplicatedSignerError] if the same vote from the same signer has been already added
+	//
 	// All other errors should be treated as exceptions.
 	Process(vote *model.Vote) error
 
 	// Status returns the status of the vote processor
 	Status() VoteCollectorStatus
 }
 
-// VerifyingVoteProcessor is a VoteProcessor that attempts to construct a QC for the given block.
+// VerifyingVoteProcessor is a VoteProcessor that attempts to construct a QC for a specific block
+// (provided at construction time).
+//
+// IMPORTANT: The VerifyingVoteProcessor provides the final defense against any vote-equivocation attacks
+// for its specific block. These attacks typically aim at multiple votes from the same node being counted
+// towards the supermajority threshold. The VerifyingVoteProcessor must withstand attacks by the
+// leader concurrently utilizing stand-alone votes and votes embedded into the proposal.
 type VerifyingVoteProcessor interface {
 	VoteProcessor
 
@@ -115,6 +124,6 @@ type VoteProcessorFactory interface {
 	// Create instantiates a VerifyingVoteProcessor for processing votes for a specific proposal.
 	// Caller can be sure that proposal vote was successfully verified and processed.
 	// Expected error returns during normal operations:
-	// * model.InvalidProposalError - proposal has invalid proposer vote
+	// * [model.InvalidProposalError] - proposal has invalid proposer vote
 	Create(log zerolog.Logger, proposal *model.SignedProposal) (VerifyingVoteProcessor, error)
 }

consensus/hotstuff/voteaggregator/vote_collectors_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/gammazero/workerpool"
 	"github.com/stretchr/testify/require"
@@ -49,7 +50,7 @@ func (s *VoteCollectorsTestSuite) SetupTest() {
 }
 
 func (s *VoteCollectorsTestSuite) TearDownTest() {
-	s.workerPool.StopWait()
+	unittest.AssertReturnsBefore(s.T(), s.workerPool.StopWait, time.Second)
 }
 
 // prepareMockedCollector prepares a mocked collector and stores it in map, later it will be used