review from James and Sammy

beacon-chain/blockchain/process_block.go

@@ -420,36 +420,32 @@ func (s *Service) savePostStateInfo(ctx context.Context, r [32]byte, b interface
 	return nil
 }
 
-// This removes the attestations in block `b` from the attestation mem pool.
+// pruneAttsFromPool removes these attestations from the attestation pool
+// which are covered by attestations from the received block.
 func (s *Service) pruneAttsFromPool(ctx context.Context, headState state.BeaconState, headBlock interfaces.ReadOnlySignedBeaconBlock) error {
-	atts := headBlock.Block().Body().Attestations()
-	for _, att := range atts {
-		if !att.IsAggregated() {
-			if err := s.cfg.AttPool.DeleteUnaggregatedAttestation(att); err != nil {
-				return err
-			}
-			continue
-		}
-
-		if att.Version() == version.Phase0 {
-			if features.Get().EnableExperimentalAttestationPool {
-				if err := s.cfg.AttestationCache.DeleteCovered(att); err != nil {
-					return err
-				}
-			} else if err := s.cfg.AttPool.DeleteAggregatedAttestation(att); err != nil {
-				return err
-			}
-			continue
-		}
-
-		if err := s.pruneElectraAttsFromPool(ctx, headState, att); err != nil {
+	for _, att := range headBlock.Block().Body().Attestations() {
+		if err := s.pruneCoveredAttsFromPool(ctx, headState, att); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-// pruneElectraAttsFromPool handles removing aggregated Electra attestations from the pool after receiving a block.
+func (s *Service) pruneCoveredAttsFromPool(ctx context.Context, headState state.BeaconState, att ethpb.Att) error {
+	switch {
+	case !att.IsAggregated():
+		return s.cfg.AttPool.DeleteUnaggregatedAttestation(att)
+	case att.Version() == version.Phase0:
+		if features.Get().EnableExperimentalAttestationPool {
+			return errors.Wrap(s.cfg.AttestationCache.DeleteCovered(att), "could not delete covered attestation")
+		}
+		return errors.Wrap(s.cfg.AttPool.DeleteAggregatedAttestation(att), "could not delete aggregated attestation")
+	default:
+		return s.pruneCoveredElectraAttsFromPool(ctx, headState, att)
+	}
+}
+
+// pruneCoveredElectraAttsFromPool handles removing aggregated Electra attestations from the pool after receiving a block.
 // Because in Electra block attestations can combine aggregates for multiple committees, comparing attestation bits
 // of a block attestation with attestations bits of an aggregate can cause unexpected results, leading to covered
 // aggregates not being removed from the pool.
@@ -458,9 +454,9 @@ func (s *Service) pruneAttsFromPool(ctx context.Context, headState state.BeaconS
 // aggregate accounting for one committee. This allows us to compare aggregates in the same way it's done for
 // Phase0. Even though we can't provide a valid signature for the dummy aggregate, it does not matter because
 // signatures play no part in pruning attestations.
-func (s *Service) pruneElectraAttsFromPool(ctx context.Context, headState state.BeaconState, att ethpb.Att) error {
+func (s *Service) pruneCoveredElectraAttsFromPool(ctx context.Context, headState state.BeaconState, att ethpb.Att) error {
 	if att.Version() == version.Phase0 {
-		log.Error("Called pruneElectraAttsFromPool with a Phase0 attestation")
+		log.Error("Called pruneCoveredElectraAttsFromPool with a Phase0 attestation")
 		return nil
 	}
 
@@ -469,7 +465,11 @@ func (s *Service) pruneElectraAttsFromPool(ctx context.Context, headState state.
 	committeeIndices := att.CommitteeBitsVal().BitIndices()
 	committees, err := helpers.AttestationCommittees(ctx, headState, att)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "could not get attestation committees")
+	}
+	// Sanity check as this should never happen
+	if len(committeeIndices) != len(committees) {
+		return errors.New("committee indices and committees have different lengths")
 	}
 
 	for i, c := range committees {
@@ -489,10 +489,10 @@ func (s *Service) pruneElectraAttsFromPool(ctx context.Context, headState state.
 
 		if features.Get().EnableExperimentalAttestationPool {
 			if err = s.cfg.AttestationCache.DeleteCovered(a); err != nil {
-				return err
+				return errors.Wrap(err, "could not delete covered attestation")
 			}
 		} else if err = s.cfg.AttPool.DeleteAggregatedAttestation(a); err != nil {
-			return err
+			return errors.Wrap(err, "could not delete aggregated attestation")
 		}
 
 		offset += uint64(len(c))

proto/prysm/v1alpha1/attestation/attestation_utils.go

@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"runtime/debug"
 	"slices"
 	"sort"
 
@@ -100,6 +101,10 @@ func AttestingIndices(att ethpb.Att, committees ...[]primitives.ValidatorIndex)
 	for _, c := range committees {
 		committeesLen += len(c)
 	}
+	if aggBits.Len() == 0 {
+		fmt.Printf("committee_bits: %v, aggregation_bits: %v, slot: %d", att.CommitteeBitsVal(), att.GetAggregationBits(), att.GetData().Slot)
+		debug.PrintStack()
+	}
 	if aggBits.Len() != uint64(committeesLen) {
 		return nil, fmt.Errorf("bitfield length %d is not equal to committee length %d", aggBits.Len(), committeesLen)
 	}