Add a helper to validate nil attestation (#8272)

* Add verify nil attestation function and apply all

* Remove invalid attestatione debug log

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>

beacon-chain/core/blocks/attestation.go

@@ -109,10 +109,9 @@ func ProcessAttestationNoVerifySignature(
 	ctx, span := trace.StartSpan(ctx, "core.ProcessAttestationNoVerifySignature")
 	defer span.End()
 
-	if att == nil || att.Data == nil || att.Data.Target == nil {
-		return nil, errors.New("nil attestation data target")
+	if err := helpers.ValidateNilAttestation(att); err != nil {
+		return nil, err
 	}
-
 	currEpoch := helpers.SlotToEpoch(beaconState.Slot())
 	var prevEpoch uint64
 	if currEpoch == 0 {
@@ -274,8 +273,8 @@ func VerifyAttestationsSignatures(ctx context.Context, beaconState *stateTrie.Be
 // VerifyAttestationSignature converts and attestation into an indexed attestation and verifies
 // the signature in that attestation.
 func VerifyAttestationSignature(ctx context.Context, beaconState *stateTrie.BeaconState, att *ethpb.Attestation) error {
-	if att == nil || att.Data == nil || att.AggregationBits.Count() == 0 {
-		return fmt.Errorf("nil or missing attestation data: %v", att)
+	if err := helpers.ValidateNilAttestation(att); err != nil {
+		return err
 	}
 	committee, err := helpers.BeaconCommitteeFromState(beaconState, att.Data.Slot, att.Data.CommitteeIndex)
 	if err != nil {

beacon-chain/core/blocks/attestation_test.go

@@ -24,12 +24,12 @@ import (
 
 func TestProcessAttestations_InclusionDelayFailure(t *testing.T) {
 	attestations := []*ethpb.Attestation{
-		{
+		testutil.HydrateAttestation(&ethpb.Attestation{
 			Data: &ethpb.AttestationData{
 				Target: &ethpb.Checkpoint{Epoch: 0, Root: make([]byte, 32)},
 				Slot:   5,
 			},
-		},
+		}),
 	}
 	b := testutil.NewBeaconBlock()
 	b.Block = &ethpb.BeaconBlock{
@@ -50,10 +50,10 @@ func TestProcessAttestations_InclusionDelayFailure(t *testing.T) {
 }
 
 func TestProcessAttestations_NeitherCurrentNorPrevEpoch(t *testing.T) {
-	att := &ethpb.Attestation{
+	att := testutil.HydrateAttestation(&ethpb.Attestation{
 		Data: &ethpb.AttestationData{
 			Source: &ethpb.Checkpoint{Epoch: 0, Root: []byte("hello-world")},
-			Target: &ethpb.Checkpoint{Epoch: 0}}}
+			Target: &ethpb.Checkpoint{Epoch: 0}}})
 
 	b := testutil.NewBeaconBlock()
 	b.Block = &ethpb.BeaconBlock{
@@ -383,12 +383,12 @@ func TestProcessAggregatedAttestation_NoOverlappingBits(t *testing.T) {
 func TestProcessAttestationsNoVerify_IncorrectSlotTargetEpoch(t *testing.T) {
 	beaconState, _ := testutil.DeterministicGenesisState(t, 1)
 
-	att := &ethpb.Attestation{
+	att := testutil.HydrateAttestation(&ethpb.Attestation{
 		Data: &ethpb.AttestationData{
 			Slot:   params.BeaconConfig().SlotsPerEpoch,
 			Target: &ethpb.Checkpoint{Root: make([]byte, 32)},
 		},
-	}
+	})
 	wanted := fmt.Sprintf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(att.Data.Slot), att.Data.Target.Epoch)
 	_, err := blocks.ProcessAttestationNoVerifySignature(context.TODO(), beaconState, att)
 	assert.ErrorContains(t, wanted, err)

beacon-chain/core/helpers/attestation.go

@@ -2,6 +2,7 @@ package helpers
 
 import (
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"time"
 
@@ -12,6 +13,28 @@ import (
 	"github.com/prysmaticlabs/prysm/shared/timeutils"
 )
 
+// ValidateNilAttestation checks if any composite field of input attestation is nil.
+// Access to these nil fields will result in run time panic,
+// it is recommended to run these checks as first line of defense.
+func ValidateNilAttestation(attestation *ethpb.Attestation) error {
+	if attestation == nil {
+		return errors.New("attestation can't be nil")
+	}
+	if attestation.Data == nil {
+		return errors.New("attestation's data can't be nil")
+	}
+	if attestation.Data.Source == nil {
+		return errors.New("attestation's source can't be nil")
+	}
+	if attestation.Data.Target == nil {
+		return errors.New("attestation's target can't be nil")
+	}
+	if attestation.AggregationBits == nil {
+		return errors.New("attestation's bitfield can't be nil")
+	}
+	return nil
+}
+
 // IsAggregator returns true if the signature is from the input validator. The committee
 // count is provided as an argument rather than imported implementation from spec. Having
 // committee count as an argument allows cheaper computation at run time.

beacon-chain/core/helpers/attestation_test.go

@@ -225,3 +225,72 @@ func TestVerifyCheckpointEpoch_Ok(t *testing.T) {
 	assert.Equal(t, false, helpers.VerifyCheckpointEpoch(&ethpb.Checkpoint{Epoch: 4}, genesis))
 	assert.Equal(t, false, helpers.VerifyCheckpointEpoch(&ethpb.Checkpoint{Epoch: 2}, genesis))
 }
+
+func TestValidateNilAttestation(t *testing.T) {
+	tests := []struct {
+		name        string
+		attestation *ethpb.Attestation
+		errString   string
+	}{
+		{
+			name:        "nil attestation",
+			attestation: nil,
+			errString:   "attestation can't be nil",
+		},
+		{
+			name:        "nil attestation data",
+			attestation: &ethpb.Attestation{},
+			errString:   "attestation's data can't be nil",
+		},
+		{
+			name: "nil attestation source",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Source: nil,
+					Target: &ethpb.Checkpoint{},
+				},
+			},
+			errString: "attestation's source can't be nil",
+		},
+		{
+			name: "nil attestation target",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Target: nil,
+					Source: &ethpb.Checkpoint{},
+				},
+			},
+			errString: "attestation's target can't be nil",
+		},
+		{
+			name: "nil attestation bitfield",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Target: &ethpb.Checkpoint{},
+					Source: &ethpb.Checkpoint{},
+				},
+			},
+			errString: "attestation's bitfield can't be nil",
+		},
+		{
+			name: "good attestation",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Target: &ethpb.Checkpoint{},
+					Source: &ethpb.Checkpoint{},
+				},
+				AggregationBits: []byte{},
+			},
+			errString: "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.errString != "" {
+				require.ErrorContains(t, tt.errString, helpers.ValidateNilAttestation(tt.attestation))
+			} else {
+				require.NoError(t, helpers.ValidateNilAttestation(tt.attestation))
+			}
+		})
+	}
+}

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

@@ -82,8 +82,8 @@ func (p *AttCaches) aggregateUnaggregatedAttestations(unaggregatedAtts []*ethpb.
 
 // SaveAggregatedAttestation saves an aggregated attestation in cache.
 func (p *AttCaches) SaveAggregatedAttestation(att *ethpb.Attestation) error {
-	if att == nil || att.Data == nil {
-		return nil
+	if err := helpers.ValidateNilAttestation(att); err != nil {
+		return err
 	}
 	if !helpers.IsAggregated(att) {
 		return errors.New("attestation is not aggregated")
@@ -169,8 +169,8 @@ func (p *AttCaches) AggregatedAttestationsBySlotIndex(slot, committeeIndex uint6
 
 // DeleteAggregatedAttestation deletes the aggregated attestations in cache.
 func (p *AttCaches) DeleteAggregatedAttestation(att *ethpb.Attestation) error {
-	if att == nil || att.Data == nil {
-		return nil
+	if err := helpers.ValidateNilAttestation(att); err != nil {
+		return err
 	}
 	if !helpers.IsAggregated(att) {
 		return errors.New("attestation is not aggregated")
@@ -208,8 +208,8 @@ func (p *AttCaches) DeleteAggregatedAttestation(att *ethpb.Attestation) error {
 
 // HasAggregatedAttestation checks if the input attestations has already existed in cache.
 func (p *AttCaches) HasAggregatedAttestation(att *ethpb.Attestation) (bool, error) {
-	if att == nil || att.Data == nil {
-		return false, nil
+	if err := helpers.ValidateNilAttestation(att); err != nil {
+		return false, err
 	}
 	r, err := hashFn(att.Data)
 	if err != nil {

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

@@ -98,12 +98,14 @@ func TestKV_Aggregated_SaveAggregatedAttestation(t *testing.T) {
 		wantErrString string
 	}{
 		{
-			name: "nil attestation",
-			att:  nil,
+			name:          "nil attestation",
+			att:           nil,
+			wantErrString: "attestation can't be nil",
 		},
 		{
-			name: "nil attestation data",
-			att:  &ethpb.Attestation{},
+			name:          "nil attestation data",
+			att:           &ethpb.Attestation{},
+			wantErrString: "attestation's data can't be nil",
 		},
 		{
 			name: "not aggregated",
@@ -114,9 +116,9 @@ func TestKV_Aggregated_SaveAggregatedAttestation(t *testing.T) {
 		{
 			name: "invalid hash",
 			att: &ethpb.Attestation{
-				Data: &ethpb.AttestationData{
+				Data: testutil.HydrateAttestationData(&ethpb.AttestationData{
 					BeaconBlockRoot: []byte{0b0},
-				},
+				}),
 				AggregationBits: bitfield.Bitlist{0b10111},
 			},
 			wantErrString: "could not tree hash attestation: " + fssz.ErrBytesLength.Error(),
@@ -222,7 +224,7 @@ func TestKV_Aggregated_AggregatedAttestations(t *testing.T) {
 func TestKV_Aggregated_DeleteAggregatedAttestation(t *testing.T) {
 	t.Run("nil attestation", func(t *testing.T) {
 		cache := NewAttCaches()
-		assert.NoError(t, cache.DeleteAggregatedAttestation(nil))
+		assert.ErrorContains(t, "attestation can't be nil", cache.DeleteAggregatedAttestation(nil))
 		att := testutil.HydrateAttestation(&ethpb.Attestation{AggregationBits: bitfield.Bitlist{0b10101}, Data: &ethpb.AttestationData{Slot: 2}})
 		assert.NoError(t, cache.DeleteAggregatedAttestation(att))
 	})
@@ -239,7 +241,9 @@ func TestKV_Aggregated_DeleteAggregatedAttestation(t *testing.T) {
 		att := &ethpb.Attestation{
 			AggregationBits: bitfield.Bitlist{0b1111},
 			Data: &ethpb.AttestationData{
-				Slot: 2,
+				Slot:   2,
+				Source: &ethpb.Checkpoint{},
+				Target: &ethpb.Checkpoint{},
 			},
 		}
 		err := cache.DeleteAggregatedAttestation(att)
@@ -296,18 +300,21 @@ func TestKV_Aggregated_HasAggregatedAttestation(t *testing.T) {
 		existing []*ethpb.Attestation
 		input    *ethpb.Attestation
 		want     bool
+		error    bool
 	}{
 		{
 			name:  "nil attestation",
 			input: nil,
 			want:  false,
+			error: true,
 		},
 		{
 			name: "nil attestation data",
-			input: testutil.HydrateAttestation(&ethpb.Attestation{
+			input: &ethpb.Attestation{
 				AggregationBits: bitfield.Bitlist{0b1111},
-			}),
-			want: false,
+			},
+			want:  false,
+			error: true,
 		},
 		{
 			name: "empty cache aggregated",
@@ -470,17 +477,22 @@ func TestKV_Aggregated_HasAggregatedAttestation(t *testing.T) {
 				tt.input.Signature = make([]byte, 96)
 			}
 
-			result, err := cache.HasAggregatedAttestation(tt.input)
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, result)
+			if tt.error == true {
+				_, err := cache.HasAggregatedAttestation(tt.input)
+				require.ErrorContains(t, "can't be nil", err)
+			} else {
+				result, err := cache.HasAggregatedAttestation(tt.input)
+				require.NoError(t, err)
+				assert.Equal(t, tt.want, result)
 
-			// Same test for block attestations
-			cache = NewAttCaches()
-			assert.NoError(t, cache.SaveBlockAttestations(tt.existing))
+				// Same test for block attestations
+				cache = NewAttCaches()
+				assert.NoError(t, cache.SaveBlockAttestations(tt.existing))
 
-			result, err = cache.HasAggregatedAttestation(tt.input)
-			require.NoError(t, err)
-			assert.Equal(t, tt.want, result)
+				result, err = cache.HasAggregatedAttestation(tt.input)
+				require.NoError(t, err)
+				assert.Equal(t, tt.want, result)
+			}
 		})
 	}
 }

beacon-chain/sync/validate_aggregate_proof.go

@@ -45,9 +45,13 @@ func (s *Service) validateAggregateAndProof(ctx context.Context, pid peer.ID, ms
 	if !ok {
 		return pubsub.ValidationReject
 	}
-	if m.Message == nil || m.Message.Aggregate == nil || m.Message.Aggregate.Data == nil {
+	if m.Message == nil {
 		return pubsub.ValidationReject
 	}
+	if err := helpers.ValidateNilAttestation(m.Message.Aggregate); err != nil {
+		return pubsub.ValidationReject
+	}
+
 	if helpers.SlotToEpoch(m.Message.Aggregate.Data.Slot) != m.Message.Aggregate.Data.Target.Epoch {
 		return pubsub.ValidationReject
 	}

beacon-chain/sync/validate_beacon_attestation.go

@@ -59,11 +59,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p
 		return pubsub.ValidationReject
 	}
 
-	if att.Data == nil {
-		return pubsub.ValidationReject
-	}
-	// Attestation aggregation bits must exist.
-	if att.AggregationBits == nil {
+	if err := helpers.ValidateNilAttestation(att); err != nil {
 		return pubsub.ValidationReject
 	}