Refactor attestation cache key generation outside critical section (#15572)

* Refactor attestation cache key generation outside critical section

* Improve attestation cache key error handling and logging

beacon-chain/sync/pending_attestations_queue.go

@@ -225,7 +225,12 @@ func (s *Service) processAtt(ctx context.Context, att ethpb.Att) {
 			}
 		}
 
-		s.setSeenUnaggregatedAtt(att)
+		attKey, err := generateUnaggregatedAttCacheKey(att)
+		if err != nil {
+			log.WithError(err).Error("Could not generate cache key for attestation tracking")
+		} else {
+			s.setSeenUnaggregatedAtt(attKey)
+		}
 
 		valCount, err := helpers.ActiveValidatorCount(ctx, preState, slots.ToEpoch(data.Slot))
 		if err != nil {

beacon-chain/sync/validate_beacon_attestation.go

@@ -88,9 +88,16 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(
 
 	committeeIndex := att.GetCommitteeIndex()
 
+	// Generate cache key for unaggregated attestation tracking
+	attKey, err := generateUnaggregatedAttCacheKey(att)
+	if err != nil {
+		log.WithError(err).Error("Could not generate cache key for attestation tracking")
+		return pubsub.ValidationIgnore, nil
+	}
+
 	if !s.slasherEnabled {
 		// Verify this the first attestation received for the participating validator for the slot.
-		if s.hasSeenUnaggregatedAtt(att) {
+		if s.hasSeenUnaggregatedAtt(attKey) {
 			return pubsub.ValidationIgnore, nil
 		}
 		// Reject an attestation if it references an invalid block.
@@ -209,7 +216,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(
 		Data: eventData,
 	})
 
-	s.setSeenUnaggregatedAtt(att)
+	s.setSeenUnaggregatedAtt(attKey)
 
 	// Attach final validated attestation to the message for further pipeline use
 	msg.ValidatorData = attForValidation
@@ -339,23 +346,18 @@ func validateAttestingIndex(
 	return pubsub.ValidationAccept, nil
 }
 
-// Returns true if the attestation was already seen for the participating validator for the slot.
-func (s *Service) hasSeenUnaggregatedAtt(att eth.Att) bool {
-	s.seenUnAggregatedAttestationLock.RLock()
-	defer s.seenUnAggregatedAttestationLock.RUnlock()
-
+// generateUnaggregatedAttCacheKey generates the cache key for unaggregated attestation tracking.
+func generateUnaggregatedAttCacheKey(att eth.Att) (string, error) {
 	var attester uint64
 	if att.Version() >= version.Electra {
 		if !att.IsSingle() {
-			log.Debug("Called hasSeenUnaggregatedAtt with a non-single Electra attestation")
-			return false
+			return "", errors.New("non-single Electra attestation")
 		}
 		attester = uint64(att.GetAttestingIndex())
 	} else {
 		aggBits := att.GetAggregationBits()
 		if aggBits.Count() != 1 {
-			log.Debug("Attestation does not have exactly 1 bit set")
-			return false
+			return "", errors.New("attestation does not have exactly 1 bit set")
 		}
 		attester = uint64(att.GetAggregationBits().BitIndices()[0])
 	}
@@ -364,36 +366,24 @@ func (s *Service) hasSeenUnaggregatedAtt(att eth.Att) bool {
 	binary.LittleEndian.PutUint64(b, uint64(att.GetData().Slot))
 	binary.LittleEndian.PutUint64(b[8:16], uint64(att.GetCommitteeIndex()))
 	binary.LittleEndian.PutUint64(b[16:], attester)
-	_, seen := s.seenUnAggregatedAttestationCache.Get(string(b))
+	return string(b), nil
+}
+
+// Returns true if the attestation was already seen for the participating validator for the slot.
+func (s *Service) hasSeenUnaggregatedAtt(key string) bool {
+	s.seenUnAggregatedAttestationLock.RLock()
+	defer s.seenUnAggregatedAttestationLock.RUnlock()
+
+	_, seen := s.seenUnAggregatedAttestationCache.Get(key)
 	return seen
 }
 
 // Set an incoming attestation as seen for the participating validator for the slot.
-func (s *Service) setSeenUnaggregatedAtt(att eth.Att) {
+func (s *Service) setSeenUnaggregatedAtt(key string) {
 	s.seenUnAggregatedAttestationLock.Lock()
 	defer s.seenUnAggregatedAttestationLock.Unlock()
 
-	var attester uint64
-	if att.Version() >= version.Electra {
-		if !att.IsSingle() {
-			log.Debug("Called setSeenUnaggregatedAtt with a non-single Electra attestation. It will not be marked as seen")
-			return
-		}
-		attester = uint64(att.GetAttestingIndex())
-	} else {
-		aggBits := att.GetAggregationBits()
-		if aggBits.Count() != 1 {
-			log.Debug("Attestation does not have exactly 1 bit set. It will not be marked as seen")
-			return
-		}
-		attester = uint64(att.GetAggregationBits().BitIndices()[0])
-	}
-
-	b := make([]byte, 24)
-	binary.LittleEndian.PutUint64(b, uint64(att.GetData().Slot))
-	binary.LittleEndian.PutUint64(b[8:16], uint64(att.GetCommitteeIndex()))
-	binary.LittleEndian.PutUint64(b[16:], attester)
-	s.seenUnAggregatedAttestationCache.Add(string(b), true)
+	s.seenUnAggregatedAttestationCache.Add(key, true)
 }
 
 // hasBlockAndState returns true if the beacon node knows about a block and associated state in the

beacon-chain/sync/validate_beacon_attestation_test.go

@@ -469,6 +469,13 @@ func TestService_validateCommitteeIndexBeaconAttestationElectra(t *testing.T) {
 func TestService_setSeenUnaggregatedAtt(t *testing.T) {
 	s := NewService(t.Context(), WithP2P(p2ptest.NewTestP2P(t)))
 
+	// Helper function to generate key and handle errors in tests
+	generateKey := func(t *testing.T, att ethpb.Att) string {
+		key, err := generateUnaggregatedAttCacheKey(att)
+		require.NoError(t, err)
+		return key
+	}
+
 	t.Run("phase0", func(t *testing.T) {
 		s.initCaches()
 
@@ -502,31 +509,41 @@ func TestService_setSeenUnaggregatedAtt(t *testing.T) {
 		}
 
 		t.Run("empty cache", func(t *testing.T) {
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c0a0))
+			key := generateKey(t, s0c0a0)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key))
 		})
 		t.Run("ok", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c0a0)
-			assert.Equal(t, true, s.hasSeenUnaggregatedAtt(s0c0a0))
+			key := generateKey(t, s0c0a0)
+			s.setSeenUnaggregatedAtt(key)
+			assert.Equal(t, true, s.hasSeenUnaggregatedAtt(key))
 		})
 		t.Run("different slot", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s1c0a0)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s2c0a0))
+			key1 := generateKey(t, s1c0a0)
+			key2 := generateKey(t, s2c0a0)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("different committee index", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c1a0)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c2a0))
+			key1 := generateKey(t, s0c1a0)
+			key2 := generateKey(t, s0c2a0)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("different bit", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c0a1)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c0a2))
+			key1 := generateKey(t, s0c0a1)
+			key2 := generateKey(t, s0c0a2)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("0 bits set is considered not seen", func(t *testing.T) {
 			a := &ethpb.Attestation{AggregationBits: bitfield.Bitlist{0b1000}}
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(a))
+			_, err := generateUnaggregatedAttCacheKey(a)
+			require.Equal(t, err != nil, true, "Should error because no bits set is invalid")
 		})
 		t.Run("multiple bits set is considered not seen", func(t *testing.T) {
 			a := &ethpb.Attestation{AggregationBits: bitfield.Bitlist{0b1111}}
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(a))
+			_, err := generateUnaggregatedAttCacheKey(a)
+			require.Equal(t, err != nil, true, "Should error because no bits set is invalid")
 		})
 	})
 	t.Run("electra", func(t *testing.T) {
@@ -569,27 +586,36 @@ func TestService_setSeenUnaggregatedAtt(t *testing.T) {
 		}
 
 		t.Run("empty cache", func(t *testing.T) {
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c0a0))
+			key := generateKey(t, s0c0a0)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key))
 		})
 		t.Run("ok", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c0a0)
-			assert.Equal(t, true, s.hasSeenUnaggregatedAtt(s0c0a0))
+			key := generateKey(t, s0c0a0)
+			s.setSeenUnaggregatedAtt(key)
+			assert.Equal(t, true, s.hasSeenUnaggregatedAtt(key))
 		})
 		t.Run("different slot", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s1c0a0)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s2c0a0))
+			key1 := generateKey(t, s1c0a0)
+			key2 := generateKey(t, s2c0a0)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("different committee index", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c1a0)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c2a0))
+			key1 := generateKey(t, s0c1a0)
+			key2 := generateKey(t, s0c2a0)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("different attester", func(t *testing.T) {
-			s.setSeenUnaggregatedAtt(s0c0a1)
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(s0c0a2))
+			key1 := generateKey(t, s0c0a1)
+			key2 := generateKey(t, s0c0a2)
+			s.setSeenUnaggregatedAtt(key1)
+			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(key2))
 		})
 		t.Run("single attestation is considered not seen", func(t *testing.T) {
 			a := &ethpb.AttestationElectra{}
-			assert.Equal(t, false, s.hasSeenUnaggregatedAtt(a))
+			_, err := generateUnaggregatedAttCacheKey(a)
+			require.Equal(t, err != nil, true, "Should error because no bits set is invalid")
 		})
 	})
 }

changelog/ttsao_fix-attestation-cache-key.md

@@ -0,0 +1,3 @@
+### Changed
+
+- Improved sync unaggregated attestation cache key outside of lock path