refactor and fixes after review

Author: AdoAdoAdo

consensus/spos/bls/v2/proofSelfAssembly.go

@@ -34,6 +34,7 @@ func trySelfAssembleProof(sr *spos.Subround, store signatureEvidenceHandler, ev
 
 	bitmap, agg, err := aggregateAndVerifyEvidence(sr, ev)
 	if err != nil {
+		stripInvalidShares(sr, ev)
 		if ev.getCount() < ev.threshold {
 			// inflated signal was adversarial; the decision tiers demote to the wait tier
 			store.DropRetained(ev.nonce)
@@ -99,26 +100,24 @@ func abortOnExistingProof(sr *spos.Subround, store signatureEvidenceHandler, ev
 	return true
 }
 
-// aggregateAndVerifyEvidence aggregates the snapshotted shares and verifies the result; on
-// failure it strips the invalid shares so a retry or demotion can follow
+// aggregateAndVerifyEvidence aggregates the snapshotted shares and verifies the result
 func aggregateAndVerifyEvidence(sr *spos.Subround, ev *roundSignatureEvidence) ([]byte, []byte, error) {
 	bitmap, shares := ev.getAggregationData()
 
 	agg, err := sr.SigningHandler().AggregateSigsWithKeys(ev.consensusGroup, bitmap, shares, ev.epoch)
 	if err == nil {
 		err = sr.SigningHandler().VerifyAggregatedSigWithKeys(ev.consensusGroup, bitmap, ev.headerHash, agg, ev.epoch)
 	}
-	if err == nil {
-		return bitmap, agg, nil
+	if err != nil {
+		return nil, nil, err
 	}
 
-	stripInvalidShares(sr, ev, bitmap, shares)
-
-	return nil, nil, err
+	return bitmap, agg, nil
 }
 
 // stripInvalidShares verifies each share in parallel and drops the invalid ones from the evidence
-func stripInvalidShares(sr *spos.Subround, ev *roundSignatureEvidence, bitmap []byte, shares [][]byte) {
+func stripInvalidShares(sr *spos.Subround, ev *roundSignatureEvidence) {
+	bitmap, shares := ev.getAggregationData()
 	invalidIndices := make([]int, 0)
 	var mut sync.Mutex
 

consensus/spos/bls/v2/signatureEvidence.go

@@ -86,7 +86,9 @@ type signatureEvidenceStore struct {
 
 	mut      sync.RWMutex
 	previous *roundSignatureEvidence
-	retained *roundSignatureEvidence
+	// retainedQuorum keeps the last quorum-level evidence for a still unsettled nonce beyond
+	// the one-round lifetime of the previous slot, so self-assembly can be retried
+	retainedQuorum *roundSignatureEvidence
 }
 
 func newSignatureEvidenceStore(proofsPool consensus.EquivalentProofsPool) (*signatureEvidenceStore, error) {
@@ -118,7 +120,7 @@ func (store *signatureEvidenceStore) Capture(ev *roundSignatureEvidence) {
 		return
 	}
 
-	store.retained = replaced
+	store.retainedQuorum = replaced
 }
 
 // GetPreviousRoundEvidence returns the previous-round slot only if it matches the given
@@ -140,7 +142,7 @@ func (store *signatureEvidenceStore) GetRetainedQuorumEvidence(nonce uint64) (*r
 	store.mut.RLock()
 	defer store.mut.RUnlock()
 
-	ev := store.retained
+	ev := store.retainedQuorum
 	if ev == nil || ev.nonce != nonce {
 		return nil, false
 	}
@@ -158,7 +160,7 @@ func (store *signatureEvidenceStore) GetAssemblyCandidate() (*roundSignatureEvid
 		return ev, true
 	}
 
-	ev = store.retained
+	ev = store.retainedQuorum
 	if ev != nil && ev.getCount() >= ev.threshold && !store.isNonceSettled(ev) {
 		return ev, true
 	}
@@ -171,8 +173,8 @@ func (store *signatureEvidenceStore) DropRetained(nonce uint64) {
 	store.mut.Lock()
 	defer store.mut.Unlock()
 
-	if store.retained != nil && store.retained.nonce == nonce {
-		store.retained = nil
+	if store.retainedQuorum != nil && store.retainedQuorum.nonce == nonce {
+		store.retainedQuorum = nil
 	}
 }
 

consensus/spos/bls/v2/signatureEvidenceFlow_test.go

@@ -158,6 +158,34 @@ func TestSubroundStartRound_CaptureSignatureEvidence(t *testing.T) {
 		assert.False(t, found)
 	})
 
+	t.Run("settled nonce skips the snapshot", func(t *testing.T) {
+		t.Parallel()
+
+		container := consensusMocks.InitConsensusCore()
+		container.SetSigningHandler(&consensusMocks.SigningHandlerStub{
+			SignatureShareCalled: func(index uint16) ([]byte, error) {
+				return []byte(fmt.Sprintf("share_%d", index)), nil
+			},
+		})
+		container.SetEquivalentProofsPool(createSettledProofsPool([]byte("header hash")))
+
+		store, _ := newSignatureEvidenceStore(createUnsettledProofsPool())
+		srStartRound := createFlowStartRound(t, container, store)
+
+		srStartRound.SetRoundIndex(10)
+		srStartRound.SetHeader(&block.Header{Nonce: 5})
+		srStartRound.SetData([]byte("header hash"))
+		group := srStartRound.ConsensusGroup()
+		for i := 0; i < 3; i++ {
+			_ = srStartRound.SetJobDone(group[i], bls.SrSignature, true)
+		}
+
+		srStartRound.captureSignatureEvidence()
+
+		_, found := store.GetPreviousRoundEvidence(5, 11)
+		assert.False(t, found)
+	})
+
 	t.Run("no signatures observed clears the slot", func(t *testing.T) {
 		t.Parallel()
 

consensus/spos/bls/v2/subroundSignature.go

@@ -449,20 +449,21 @@ func (sr *subroundSignature) shouldAbortOnSignatureEvidence(ctx context.Context,
 	}
 
 	count := ev.getCount()
-	switch {
-	case count >= ev.threshold:
+	if count >= ev.threshold {
 		go trySelfAssembleProof(sr.Subround, sr.signatureEvidence, ev)
 		log.Debug("refusing to sign competing block: previous round reached quorum",
 			"nonce", nonce,
 			"previousHash", ev.headerHash,
 			"observedShares", count)
 		return true
-	case float64(count) >= significantEvidenceFraction*float64(ev.threshold):
+	}
+
+	if float64(count) >= significantEvidenceFraction*float64(ev.threshold) {
 		return sr.waitForPreviousBlockProof(ctx, ev.headerHash, nonce)
-	default:
-		// few shares observed: the previous block lacked support, proceed
-		return false
 	}
+
+	// few shares observed: the previous block lacked support, proceed
+	return false
 }
 
 // waitIfCompetingBlock waits if this node already signed a different block for the same nonce

consensus/spos/bls/v2/subroundStartRound.go

@@ -138,28 +138,28 @@ func (sr *subroundStartRound) buildSignatureEvidence() *roundSignatureEvidence {
 		return nil
 	}
 
-	group := sr.ConsensusGroup()
-	bitmapSize := len(group) / 8
-	if len(group)%8 != 0 {
-		bitmapSize++
+	// settled nonce: the evidence could never be used, skip the snapshot
+	proof, err := sr.EquivalentProofsPool().GetProofByNonce(header.GetNonce(), header.GetShardID())
+	if err == nil && !check.IfNil(proof) {
+		return nil
 	}
 
-	bitmap := make([]byte, bitmapSize)
+	group := sr.ConsensusGroup()
+	bitmap := sr.GenerateBitmap(bls.SrSignature)
 	shares := make([][]byte, len(group))
 	count := 0
-	for i, pk := range group {
-		isJobDone, err := sr.JobDone(pk, bls.SrSignature)
-		if err != nil || !isJobDone {
+	for i := range group {
+		if !isIndexSetInBitmap(i, bitmap) {
 			continue
 		}
 
 		share, err := sr.SigningHandler().SignatureShare(uint16(i))
 		if err != nil {
+			bitmap[i/8] &^= 1 << (uint16(i) % 8)
 			continue
 		}
 
 		shares[i] = share
-		bitmap[i/8] |= 1 << (uint16(i) % 8)
 		count++
 	}
 

dataRetriever/dataPool/proofsCache/proofsCache.go

@@ -80,8 +80,8 @@ func (pc *proofsCache) addProof(proof data.HeaderProofHandler) {
 	pc.proofsByHash[newHash] = proof
 }
 
-// addProofIfNoneAtNonce adds the proof only if its nonce slot is free or already holds the same
-// header hash; returns the added flag and the pre-existing proof for the nonce, if any
+// addProofIfNoneAtNonce adds the proof only if its nonce slot is free; an occupied slot (same or
+// different hash) rejects the add and returns the pre-existing proof, never overwriting it
 func (pc *proofsCache) addProofIfNoneAtNonce(proof data.HeaderProofHandler) (bool, data.HeaderProofHandler) {
 	if check.IfNil(proof) {
 		return false, nil

dataRetriever/dataPool/proofsCache/proofsPool.go

@@ -73,8 +73,8 @@ func (pp *proofsPool) AddProof(
 	return pp.addProof(headerProof)
 }
 
-// AddProofIfNoneAtNonce will add the provided proof only if its (nonce, shard) slot is free or
-// holds the same header hash; unlike AddProof, it never overwrites the pre-existing proof
+// AddProofIfNoneAtNonce will add the provided proof only if its (nonce, shard) slot is free; an
+// occupied slot (same or different hash) rejects the add and returns the pre-existing proof
 func (pp *proofsPool) AddProofIfNoneAtNonce(
 	headerProof data.HeaderProofHandler,
 ) (bool, data.HeaderProofHandler) {

factory/crypto/signingHandler.go

@@ -327,7 +327,12 @@ func (sh *signingHandler) AggregateSigs(bitmap []byte, epoch uint32) ([]byte, er
 // AggregateSigsWithKeys aggregates the provided signature shares over the provided group,
 // without touching the per-round state; bitmap indices are aligned with pubKeys and sigShares
 func (sh *signingHandler) AggregateSigsWithKeys(pubKeys []string, bitmap []byte, sigShares [][]byte, epoch uint32) ([]byte, error) {
-	signatures, pubKeysSigners, err := sh.selectByBitmap(pubKeys, bitmap, sigShares)
+	pubKeysSigners, err := sh.selectPubKeysByBitmap(pubKeys, bitmap)
+	if err != nil {
+		return nil, err
+	}
+
+	signatures, err := selectSigSharesByBitmap(pubKeys, bitmap, sigShares)
 	if err != nil {
 		return nil, err
 	}
@@ -343,7 +348,7 @@ func (sh *signingHandler) AggregateSigsWithKeys(pubKeys []string, bitmap []byte,
 // VerifyAggregatedSigWithKeys verifies the aggregated signature over the provided group,
 // without touching the per-round state
 func (sh *signingHandler) VerifyAggregatedSigWithKeys(pubKeys []string, bitmap []byte, message []byte, aggSig []byte, epoch uint32) error {
-	_, pubKeysSigners, err := sh.selectByBitmap(pubKeys, bitmap, nil)
+	pubKeysSigners, err := sh.selectPubKeysByBitmap(pubKeys, bitmap)
 	if err != nil {
 		return err
 	}
@@ -376,41 +381,63 @@ func (sh *signingHandler) VerifySigShareWithKey(pubKey []byte, sigShare []byte,
 	return multiSigner.VerifySignatureShareV2(pk, message, sigShare)
 }
 
-// selectByBitmap returns the signatures and public keys whose index is set in the bitmap;
-// sigShares may be nil when only the public keys are needed
-func (sh *signingHandler) selectByBitmap(pubKeys []string, bitmap []byte, sigShares [][]byte) ([][]byte, []crypto.PublicKey, error) {
+func validateBitmap(pubKeys []string, bitmap []byte) error {
 	if bitmap == nil {
-		return nil, nil, ErrNilBitmap
+		return ErrNilBitmap
 	}
 	if len(bitmap)*8 < len(pubKeys) {
-		return nil, nil, ErrBitmapMismatch
+		return ErrBitmapMismatch
 	}
-	if sigShares != nil && len(sigShares) != len(pubKeys) {
-		return nil, nil, ErrIndexOutOfBounds
+
+	return nil
+}
+
+// selectPubKeysByBitmap returns the public keys whose index is set in the bitmap
+func (sh *signingHandler) selectPubKeysByBitmap(pubKeys []string, bitmap []byte) ([]crypto.PublicKey, error) {
+	err := validateBitmap(pubKeys, bitmap)
+	if err != nil {
+		return nil, err
 	}
 
-	signatures := make([][]byte, 0, len(pubKeys))
 	pubKeysSigners := make([]crypto.PublicKey, 0, len(pubKeys))
 	for i, pubKeyStr := range pubKeys {
 		if bitmap[i/8]&(1<<(uint16(i)%8)) == 0 {
 			continue
 		}
 
-		if sigShares != nil {
-			if len(sigShares[i]) == 0 {
-				return nil, nil, ErrNilElement
-			}
-			signatures = append(signatures, sigShares[i])
-		}
-
 		pubKey, err := sh.getPubKeyFromBytes([]byte(pubKeyStr))
 		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 		pubKeysSigners = append(pubKeysSigners, pubKey)
 	}
 
-	return signatures, pubKeysSigners, nil
+	return pubKeysSigners, nil
+}
+
+// selectSigSharesByBitmap returns the signature shares whose index is set in the bitmap
+func selectSigSharesByBitmap(pubKeys []string, bitmap []byte, sigShares [][]byte) ([][]byte, error) {
+	err := validateBitmap(pubKeys, bitmap)
+	if err != nil {
+		return nil, err
+	}
+	if len(sigShares) != len(pubKeys) {
+		return nil, ErrIndexOutOfBounds
+	}
+
+	signatures := make([][]byte, 0, len(pubKeys))
+	for i := range sigShares {
+		if bitmap[i/8]&(1<<(uint16(i)%8)) == 0 {
+			continue
+		}
+
+		if len(sigShares[i]) == 0 {
+			return nil, ErrNilElement
+		}
+		signatures = append(signatures, sigShares[i])
+	}
+
+	return signatures, nil
 }
 
 func (sh *signingHandler) getPubKeyFromBytes(