Fix failure in block production (#13840)

- After Electra, the aggregation bits indicate the bit-maps that appear
only in the committee bits, so merge them in a way similar to merge sort
- Fix bugs in BitList structure
- Remove the problematic SetVersion() function that leads to the empty
attestation bug

Author: domiwei

cl/aggregation/pool_impl.go

@@ -19,12 +19,12 @@ package aggregation
 import (
 	"context"
 	"errors"
-	"fmt"
 	"sync"
 	"time"
 
 	"github.com/Giulio2002/bls"
 	"github.com/erigontech/erigon-lib/common"
+	"github.com/erigontech/erigon-lib/log/v3"
 	"github.com/erigontech/erigon/cl/clparams"
 	"github.com/erigontech/erigon/cl/cltypes/solid"
 	"github.com/erigontech/erigon/cl/phase1/core/state/lru"
@@ -44,7 +44,7 @@ type aggregationPoolImpl struct {
 	netConfig      *clparams.NetworkConfig
 	ethClock       eth_clock.EthereumClock
 	aggregatesLock sync.RWMutex
-	aggregates     map[common.Hash]*solid.Attestation
+	aggregates     map[common.Hash]*solid.Attestation // don't need this anymore after electra upgrade
 	// aggregationInCommittee is a cache for aggregation in committee, which is used after electra upgrade
 	aggregatesInCommittee *lru.CacheWithTTL[keyAggrInCommittee, *solid.Attestation]
 }
@@ -78,68 +78,48 @@ func (p *aggregationPoolImpl) AddAttestation(inAtt *solid.Attestation) error {
 	if err != nil {
 		return err
 	}
-	p.aggregatesLock.Lock()
-	defer p.aggregatesLock.Unlock()
-	att, ok := p.aggregates[hashRoot]
-	if !ok {
-		p.aggregates[hashRoot] = inAtt.Copy()
-		return nil
-	}
-
-	if utils.IsOverlappingSSZBitlist(att.AggregationBits.Bytes(), inAtt.AggregationBits.Bytes()) {
-		// the on bit is already set, so ignore
-		return ErrIsSuperset
-	}
 
-	// merge signature
-	baseSig := att.Signature
-	inSig := inAtt.Signature
-	merged, err := blsAggregate([][]byte{baseSig[:], inSig[:]})
-	if err != nil {
-		return err
-	}
-	if len(merged) != 96 {
-		return errors.New("merged signature is too long")
-	}
-	var mergedSig [96]byte
-	copy(mergedSig[:], merged)
-
-	epoch := p.ethClock.GetEpochAtSlot(att.Data.Slot)
+	epoch := p.ethClock.GetEpochAtSlot(inAtt.Data.Slot)
 	clversion := p.ethClock.StateVersionByEpoch(epoch)
 	if clversion.BeforeOrEqual(clparams.DenebVersion) {
-		// merge aggregation bits
-		mergedBits, err := att.AggregationBits.Union(inAtt.AggregationBits)
-		if err != nil {
-			return err
+		p.aggregatesLock.Lock()
+		defer p.aggregatesLock.Unlock()
+		att, ok := p.aggregates[hashRoot]
+		if !ok {
+			p.aggregates[hashRoot] = inAtt.Copy()
+			return nil
 		}
-		// update attestation
-		p.aggregates[hashRoot] = &solid.Attestation{
-			AggregationBits: mergedBits,
-			Data:            att.Data,
-			Signature:       mergedSig,
+
+		if utils.IsOverlappingSSZBitlist(att.AggregationBits.Bytes(), inAtt.AggregationBits.Bytes()) {
+			// the on bit is already set, so ignore
+			return ErrIsSuperset
 		}
-	} else {
-		// Electra and after case
-		aggrBitSize := p.beaconConfig.MaxCommitteesPerSlot * p.beaconConfig.MaxValidatorsPerCommittee
-		mergedAggrBits, err := att.AggregationBits.Union(inAtt.AggregationBits)
+		// merge signature
+		baseSig := att.Signature
+		inSig := inAtt.Signature
+		merged, err := blsAggregate([][]byte{baseSig[:], inSig[:]})
 		if err != nil {
 			return err
 		}
-		if mergedAggrBits.Cap() != int(aggrBitSize) {
-			return fmt.Errorf("incorrect aggregation bits size: %d", mergedAggrBits.Cap())
+		if len(merged) != 96 {
+			return errors.New("merged signature is too long")
 		}
-		mergedCommitteeBits, err := att.CommitteeBits.Union(inAtt.CommitteeBits)
+		var mergedSig [96]byte
+		copy(mergedSig[:], merged)
+
+		// merge aggregation bits
+		mergedBits, err := att.AggregationBits.Merge(inAtt.AggregationBits)
 		if err != nil {
 			return err
 		}
+		// update attestation
 		p.aggregates[hashRoot] = &solid.Attestation{
-			AggregationBits: mergedAggrBits,
-			CommitteeBits:   mergedCommitteeBits,
+			AggregationBits: mergedBits,
 			Data:            att.Data,
 			Signature:       mergedSig,
 		}
-
-		// aggregate by committee
+	} else {
+		// Electra and after case, aggregate by committee
 		p.aggregateByCommittee(inAtt)
 	}
 	return nil
@@ -166,9 +146,15 @@ func (p *aggregationPoolImpl) aggregateByCommittee(inAtt *solid.Attestation) err
 		return nil
 	}
 
-	// merge aggregation bits and signature
-	mergedAggrBits, err := att.AggregationBits.Union(inAtt.AggregationBits)
+	if utils.IsOverlappingSSZBitlist(att.AggregationBits.Bytes(), inAtt.AggregationBits.Bytes()) {
+		// the on bit is already set, so ignore
+		return ErrIsSuperset
+	}
+
+	// It's fine to directly merge aggregation bits here, because the attestation is from the same committee
+	mergedAggrBits, err := att.AggregationBits.Merge(inAtt.AggregationBits)
 	if err != nil {
+		log.Debug("failed to merge aggregation bits", "err", err)
 		return err
 	}
 	merged, err := blsAggregate([][]byte{att.Signature[:], inAtt.Signature[:]})

cl/aggregation/pool_test.go

@@ -40,22 +40,22 @@ var (
 		},
 	}
 	att1_1 = &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000001, 0, 0, 0}, 2048),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00000011}, 2048),
 		Data:            attData1,
 		Signature:       [96]byte{'a', 'b', 'c', 'd', 'e', 'f'},
 	}
 	att1_2 = &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000001, 0, 0, 0}, 2048),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00000011}, 2048),
 		Data:            attData1,
 		Signature:       [96]byte{'d', 'e', 'f', 'g', 'h', 'i'},
 	}
 	att1_3 = &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000100, 0, 0, 0}, 2048),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00001100}, 2048),
 		Data:            attData1,
 		Signature:       [96]byte{'g', 'h', 'i', 'j', 'k', 'l'},
 	}
 	att1_4 = &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00100000, 0, 0, 0}, 2048),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b01100000}, 2048),
 		Data:            attData1,
 		Signature:       [96]byte{'m', 'n', 'o', 'p', 'q', 'r'},
 	}
@@ -72,7 +72,7 @@ var (
 		},
 	}
 	att2_1 = &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000001, 0, 0, 0}, 2048),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00000001}, 2048),
 		Data:            attData2,
 		Signature:       [96]byte{'t', 'e', 's', 't', 'i', 'n'},
 	}
@@ -107,21 +107,21 @@ func (t *PoolTestSuite) TearDownTest() {
 
 func (t *PoolTestSuite) TestAddAttestationElectra() {
 	cBits1 := solid.NewBitVector(64)
-	cBits1.SetBitAt(0, true)
+	cBits1.SetBitAt(10, true)
 	cBits2 := solid.NewBitVector(64)
 	cBits2.SetBitAt(10, true)
 	expectedCommitteeBits := solid.NewBitVector(64)
-	expectedCommitteeBits.SetBitAt(0, true)
+	expectedCommitteeBits.SetBitAt(10, true)
 	expectedCommitteeBits.SetBitAt(10, true)
 
 	att1 := &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000001, 0, 0, 0}, 2048*64),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00000011}, 2048*64),
 		Data:            attData1,
 		Signature:       [96]byte{'a', 'b', 'c', 'd', 'e', 'f'},
 		CommitteeBits:   cBits1,
 	}
 	att2 := &solid.Attestation{
-		AggregationBits: solid.BitlistFromBytes([]byte{0b00000000, 0b00001000, 0, 0}, 2048*64),
+		AggregationBits: solid.BitlistFromBytes([]byte{0b00001100}, 2048*64),
 		Data:            attData1,
 		Signature:       [96]byte{'d', 'e', 'f', 'g', 'h', 'i'},
 		CommitteeBits:   cBits2,
@@ -141,11 +141,11 @@ func (t *PoolTestSuite) TestAddAttestationElectra() {
 			},
 			hashRoot: attData1Root,
 			mockFunc: func() {
-				t.mockEthClock.EXPECT().GetEpochAtSlot(gomock.Any()).Return(uint64(1)).Times(1)
-				t.mockEthClock.EXPECT().StateVersionByEpoch(gomock.Any()).Return(clparams.ElectraVersion).Times(1)
+				t.mockEthClock.EXPECT().GetEpochAtSlot(gomock.Any()).Return(uint64(1)).Times(2)
+				t.mockEthClock.EXPECT().StateVersionByEpoch(gomock.Any()).Return(clparams.ElectraVersion).Times(2)
 			},
 			expect: &solid.Attestation{
-				AggregationBits: solid.BitlistFromBytes([]byte{0b0000001, 0b00001000, 0, 0}, 2048*64),
+				AggregationBits: solid.BitlistFromBytes([]byte{0b00001101}, 2048*64),
 				Data:            attData1,
 				Signature:       mockAggrResult,
 				CommitteeBits:   expectedCommitteeBits,
@@ -162,9 +162,7 @@ func (t *PoolTestSuite) TestAddAttestationElectra() {
 		for i := range tc.atts {
 			pool.AddAttestation(tc.atts[i])
 		}
-		att := pool.GetAggregatationByRoot(tc.hashRoot)
-		//h1, _ := tc.expect.HashSSZ()
-		//h2, _ := att.HashSSZ()
+		att := pool.GetAggregatationByRootAndCommittee(tc.hashRoot, 10)
 		t.Equal(tc.expect, att, tc.name)
 	}
 }
@@ -184,7 +182,11 @@ func (t *PoolTestSuite) TestAddAttestation() {
 				att2_1,
 			},
 			hashRoot: attData1Root,
-			expect:   att1_1,
+			mockFunc: func() {
+				t.mockEthClock.EXPECT().GetEpochAtSlot(gomock.Any()).Return(uint64(1)).AnyTimes()
+				t.mockEthClock.EXPECT().StateVersionByEpoch(gomock.Any()).Return(clparams.DenebVersion).AnyTimes()
+			},
+			expect: att1_1,
 		},
 		{
 			name: "att1_2 is a super set of att1_1. skip att1_1",
@@ -194,7 +196,11 @@ func (t *PoolTestSuite) TestAddAttestation() {
 				att2_1, // none of its business
 			},
 			hashRoot: attData1Root,
-			expect:   att1_2,
+			mockFunc: func() {
+				t.mockEthClock.EXPECT().GetEpochAtSlot(gomock.Any()).Return(uint64(1)).AnyTimes()
+				t.mockEthClock.EXPECT().StateVersionByEpoch(gomock.Any()).Return(clparams.DenebVersion).AnyTimes()
+			},
+			expect: att1_2,
 		},
 		{
 			name: "merge att1_2, att1_3, att1_4",
@@ -209,7 +215,7 @@ func (t *PoolTestSuite) TestAddAttestation() {
 				t.mockEthClock.EXPECT().StateVersionByEpoch(gomock.Any()).Return(clparams.DenebVersion).AnyTimes()
 			},
 			expect: &solid.Attestation{
-				AggregationBits: solid.BitlistFromBytes([]byte{0b00100101, 0, 0, 0}, 2048),
+				AggregationBits: solid.BitlistFromBytes([]byte{0b01100101}, 2048),
 				Data:            attData1,
 				Signature:       mockAggrResult,
 			},
@@ -226,8 +232,6 @@ func (t *PoolTestSuite) TestAddAttestation() {
 			pool.AddAttestation(tc.atts[i])
 		}
 		att := pool.GetAggregatationByRoot(tc.hashRoot)
-		//h1, _ := tc.expect.HashSSZ()
-		//h2, _ := att.HashSSZ()
 		t.Equal(tc.expect, att, tc.name)
 	}
 }