Add verify slot target epoch function and apply all (#8273)

* Add verify slot target epoch function and apply all

* Fix TestProcessAttestationsNoVerify_IncorrectSlotTargetEpoch

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

beacon-chain/blockchain/process_attestation.go

@@ -2,7 +2,6 @@ package blockchain
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/pkg/errors"
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
@@ -45,22 +44,14 @@ func (s *Service) onAttestation(ctx context.Context, a *ethpb.Attestation) ([]ui
 	ctx, span := trace.StartSpan(ctx, "blockChain.onAttestation")
 	defer span.End()
 
-	if a == nil {
-		return nil, errors.New("nil attestation")
+	if err := helpers.ValidateNilAttestation(a); err != nil {
+		return nil, err
 	}
-	if a.Data == nil {
-		return nil, errors.New("nil attestation.Data field")
+	if err := helpers.ValidateSlotTargetEpoch(a.Data); err != nil {
+		return nil, err
 	}
-	if a.Data.Target == nil {
-		return nil, errors.New("nil attestation.Data.Target field")
-	}
-
 	tgt := stateTrie.CopyCheckpoint(a.Data.Target)
 
-	if helpers.SlotToEpoch(a.Data.Slot) != a.Data.Target.Epoch {
-		return nil, fmt.Errorf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(a.Data.Slot), a.Data.Target.Epoch)
-	}
-
 	// Verify beacon node has seen the target block before.
 	if !s.hasBlock(ctx, bytesutil.ToBytes32(tgt.Root)) {
 		return nil, ErrTargetRootNotInDB

beacon-chain/blockchain/process_attestation_test.go

@@ -72,34 +72,34 @@ func TestStore_OnAttestation(t *testing.T) {
 	}{
 		{
 			name:      "attestation's data slot not aligned with target vote",
-			a:         &ethpb.Attestation{Data: &ethpb.AttestationData{Slot: params.BeaconConfig().SlotsPerEpoch, Target: &ethpb.Checkpoint{Root: make([]byte, 32)}}},
-			wantedErr: "data slot is not in the same epoch as target 1 != 0",
+			a:         testutil.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: params.BeaconConfig().SlotsPerEpoch, Target: &ethpb.Checkpoint{Root: make([]byte, 32)}}}),
+			wantedErr: "slot 32 does not match target epoch 0",
 		},
 		{
 			name:      "attestation's target root not in db",
-			a:         &ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Root: bytesutil.PadTo([]byte{'A'}, 32)}}},
+			a:         testutil.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Root: bytesutil.PadTo([]byte{'A'}, 32)}}}),
 			wantedErr: "target root does not exist in db",
 		},
 		{
 			name:      "no pre state for attestations's target block",
-			a:         &ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Root: BlkWithOutStateRoot[:]}}},
+			a:         testutil.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Root: BlkWithOutStateRoot[:]}}}),
 			wantedErr: "could not get pre state for epoch 0",
 		},
 		{
 			name: "process attestation doesn't match current epoch",
-			a: &ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 100 * params.BeaconConfig().SlotsPerEpoch, Target: &ethpb.Checkpoint{Epoch: 100,
-				Root: BlkWithStateBadAttRoot[:]}}},
+			a: testutil.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 100 * params.BeaconConfig().SlotsPerEpoch, Target: &ethpb.Checkpoint{Epoch: 100,
+				Root: BlkWithStateBadAttRoot[:]}}}),
 			wantedErr: "target epoch 100 does not match current epoch",
 		},
 		{
 			name:      "process nil attestation",
 			a:         nil,
-			wantedErr: "nil attestation",
+			wantedErr: "attestation can't be nil",
 		},
 		{
 			name:      "process nil field (a.Data) in attestation",
 			a:         &ethpb.Attestation{},
-			wantedErr: "nil attestation.Data field",
+			wantedErr: "attestation's data can't be nil",
 		},
 		{
 			name: "process nil field (a.Target) in attestation",
@@ -112,7 +112,7 @@ func TestStore_OnAttestation(t *testing.T) {
 				AggregationBits: make([]byte, 1),
 				Signature:       make([]byte, 96),
 			},
-			wantedErr: "nil attestation.Data.Target field",
+			wantedErr: "attestation's target can't be nil",
 		},
 	}
 

beacon-chain/core/blocks/attestation.go

@@ -128,8 +128,8 @@ func ProcessAttestationNoVerifySignature(
 			currEpoch,
 		)
 	}
-	if helpers.SlotToEpoch(data.Slot) != data.Target.Epoch {
-		return nil, fmt.Errorf("data slot is not in the same epoch as target %d != %d", helpers.SlotToEpoch(data.Slot), data.Target.Epoch)
+	if err := helpers.ValidateSlotTargetEpoch(att.Data); err != nil {
+		return nil, err
 	}
 
 	s := att.Data.Slot

beacon-chain/core/blocks/attestation_test.go

@@ -389,7 +389,7 @@ func TestProcessAttestationsNoVerify_IncorrectSlotTargetEpoch(t *testing.T) {
 			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)
+	wanted := "slot 32 does not match target epoch 0"
 	_, err := blocks.ProcessAttestationNoVerifySignature(context.TODO(), beaconState, att)
 	assert.ErrorContains(t, wanted, err)
 }

beacon-chain/core/helpers/attestation.go

@@ -35,6 +35,15 @@ func ValidateNilAttestation(attestation *ethpb.Attestation) error {
 	return nil
 }
 
+// ValidateSlotTargetEpoch checks if attestation data's epoch matches target checkpoint's epoch.
+// It is recommended to run `ValidateNilAttestation` first to ensure `data.Target` can't be nil.
+func ValidateSlotTargetEpoch(data *ethpb.AttestationData) error {
+	if SlotToEpoch(data.Slot) != data.Target.Epoch {
+		return fmt.Errorf("slot %d does not match target epoch %d", data.Slot, data.Target.Epoch)
+	}
+	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

@@ -294,3 +294,44 @@ func TestValidateNilAttestation(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateSlotTargetEpoch(t *testing.T) {
+	tests := []struct {
+		name        string
+		attestation *ethpb.Attestation
+		errString   string
+	}{
+		{
+			name: "incorrect slot",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Target: &ethpb.Checkpoint{Epoch: 1},
+					Source: &ethpb.Checkpoint{},
+				},
+				AggregationBits: []byte{},
+			},
+			errString: "slot 0 does not match target epoch 1",
+		},
+		{
+			name: "good attestation",
+			attestation: &ethpb.Attestation{
+				Data: &ethpb.AttestationData{
+					Slot:   2 * params.BeaconConfig().SlotsPerEpoch,
+					Target: &ethpb.Checkpoint{Epoch: 2},
+					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.ValidateSlotTargetEpoch(tt.attestation.Data))
+			} else {
+				require.NoError(t, helpers.ValidateSlotTargetEpoch(tt.attestation.Data))
+			}
+		})
+	}
+}

beacon-chain/sync/validate_aggregate_proof.go

@@ -52,7 +52,7 @@ func (s *Service) validateAggregateAndProof(ctx context.Context, pid peer.ID, ms
 		return pubsub.ValidationReject
 	}
 
-	if helpers.SlotToEpoch(m.Message.Aggregate.Data.Slot) != m.Message.Aggregate.Data.Target.Epoch {
+	if err := helpers.ValidateSlotTargetEpoch(m.Message.Aggregate.Data); err != nil {
 		return pubsub.ValidationReject
 	}
 	if err := helpers.ValidateAttestationTime(m.Message.Aggregate.Data.Slot, s.chain.GenesisTime()); err != nil {

beacon-chain/sync/validate_beacon_attestation.go

@@ -68,7 +68,7 @@ func (s *Service) validateCommitteeIndexBeaconAttestation(ctx context.Context, p
 		traceutil.AnnotateError(span, err)
 		return pubsub.ValidationIgnore
 	}
-	if helpers.SlotToEpoch(att.Data.Slot) != att.Data.Target.Epoch {
+	if err := helpers.ValidateSlotTargetEpoch(att.Data); err != nil {
 		return pubsub.ValidationReject
 	}