Handle some errors from hasSeenBit and insertSeenBit differently (#15018)

* Ignore errors from `hasSeenBit` and don't pack unaggregated attestations

* changelog <3

* Revert "Ignore errors from `hasSeenBit` and don't pack unaggregated attestations"

This reverts commit dc86cb6.

Author: rkapka

beacon-chain/operations/attestations/kv/unaggregated.go

@@ -9,6 +9,7 @@ import (
 	ethpb "github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1"
 	"github.com/prysmaticlabs/prysm/v5/proto/prysm/v1alpha1/attestation"
 	"github.com/prysmaticlabs/prysm/v5/runtime/version"
+	log "github.com/sirupsen/logrus"
 )
 
 // SaveUnaggregatedAttestation saves an unaggregated attestation in cache.
@@ -60,7 +61,8 @@ func (c *AttCaches) UnaggregatedAttestations() ([]ethpb.Att, error) {
 	for _, att := range unAggregatedAtts {
 		seen, err := c.hasSeenBit(att)
 		if err != nil {
-			return nil, err
+			log.WithError(err).Debug("Could not check if unaggregated attestation's bit has been seen. Attestation will not be returned")
+			continue
 		}
 		if !seen {
 			atts = append(atts, att.Clone())
@@ -137,7 +139,7 @@ func (c *AttCaches) DeleteUnaggregatedAttestation(att ethpb.Att) error {
 	}
 
 	if err := c.insertSeenBit(att); err != nil {
-		return err
+		log.WithError(err).Debug("Could not insert seen bit of unaggregated attestation. Attestation will be deleted")
 	}
 
 	id, err := attestation.NewId(att, attestation.Full)
@@ -163,7 +165,12 @@ func (c *AttCaches) DeleteSeenUnaggregatedAttestations() (int, error) {
 		if att == nil || att.IsNil() || att.IsAggregated() {
 			continue
 		}
-		if seen, err := c.hasSeenBit(att); err == nil && seen {
+		seen, err := c.hasSeenBit(att)
+		if err != nil {
+			log.WithError(err).Debug("Could not check if unaggregated attestation's bit has been seen. Attestation will be deleted")
+			seen = true
+		}
+		if seen {
 			delete(c.unAggregatedAtt, r)
 			count++
 		}

beacon-chain/operations/attestations/kv/unaggregated_test.go

@@ -17,6 +17,24 @@ import (
 	"github.com/prysmaticlabs/prysm/v5/testing/util"
 )
 
+func TestKV_Unaggregated_UnaggregatedAttestations(t *testing.T) {
+	t.Run("not returned when hasSeenBit fails", func(t *testing.T) {
+		att := util.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 1}, AggregationBits: bitfield.Bitlist{0b101}})
+		id, err := attestation.NewId(att, attestation.Data)
+		require.NoError(t, err)
+
+		cache := NewAttCaches()
+		require.NoError(t, cache.SaveUnaggregatedAttestation(att))
+		cache.seenAtt.Delete(id.String())
+		// cache a bitlist whose length is different from the attestation bitlist's length
+		cache.seenAtt.Set(id.String(), []bitfield.Bitlist{{0b1001}}, c.DefaultExpiration)
+
+		atts, err := cache.UnaggregatedAttestations()
+		require.NoError(t, err)
+		assert.Equal(t, 0, len(atts))
+	})
+}
+
 func TestKV_Unaggregated_SaveUnaggregatedAttestation(t *testing.T) {
 	tests := []struct {
 		name          string
@@ -155,6 +173,21 @@ func TestKV_Unaggregated_DeleteUnaggregatedAttestation(t *testing.T) {
 		require.NoError(t, err)
 		assert.DeepEqual(t, []ethpb.Att{}, returned)
 	})
+
+	t.Run("deleted when insertSeenBit fails", func(t *testing.T) {
+		att := util.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 1}, AggregationBits: bitfield.Bitlist{0b101}})
+		id, err := attestation.NewId(att, attestation.Data)
+		require.NoError(t, err)
+
+		cache := NewAttCaches()
+		require.NoError(t, cache.SaveUnaggregatedAttestation(att))
+		cache.seenAtt.Delete(id.String())
+		// cache a bitlist whose length is different from the attestation bitlist's length
+		cache.seenAtt.Set(id.String(), []bitfield.Bitlist{{0b1001}}, c.DefaultExpiration)
+
+		require.NoError(t, cache.DeleteUnaggregatedAttestation(att))
+		assert.Equal(t, 0, len(cache.unAggregatedAtt), "Attestation was not deleted")
+	})
 }
 
 func TestKV_Unaggregated_DeleteSeenUnaggregatedAttestations(t *testing.T) {
@@ -232,6 +265,23 @@ func TestKV_Unaggregated_DeleteSeenUnaggregatedAttestations(t *testing.T) {
 		require.NoError(t, err)
 		assert.DeepEqual(t, []ethpb.Att{}, returned)
 	})
+
+	t.Run("deleted when hasSeenBit fails", func(t *testing.T) {
+		att := util.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 1}, AggregationBits: bitfield.Bitlist{0b101}})
+		id, err := attestation.NewId(att, attestation.Data)
+		require.NoError(t, err)
+
+		cache := NewAttCaches()
+		require.NoError(t, cache.SaveUnaggregatedAttestation(att))
+		cache.seenAtt.Delete(id.String())
+		// cache a bitlist whose length is different from the attestation bitlist's length
+		cache.seenAtt.Set(id.String(), []bitfield.Bitlist{{0b1001}}, c.DefaultExpiration)
+
+		count, err := cache.DeleteSeenUnaggregatedAttestations()
+		require.NoError(t, err)
+		assert.Equal(t, 1, count)
+		assert.Equal(t, 0, len(cache.unAggregatedAtt), "Attestation was not deleted")
+	})
 }
 
 func TestKV_Unaggregated_UnaggregatedAttestationsBySlotIndex(t *testing.T) {

changelog/radek_ignore-bit-error.md

@@ -0,0 +1,3 @@
+### Changed
+
+- Ignore errors from `hasSeenBit` and don't pack unaggregated attestations.