Move attestation's source checkpoint validation to VerifyAttestationNoVerifySignature (#8598)

* Move attestation's source checkpoint validation to VerifyAttestationNoVerifySignature

* change parameter type to ReadOnlyBeaconState

Co-authored-by: terence tsao <terence@prysmaticlabs.com>

beacon-chain/core/blocks/attestation.go

@@ -103,35 +103,46 @@ func ProcessAttestationsNoVerifySignature(
 // used before processing attestation with the beacon state.
 func VerifyAttestationNoVerifySignature(
 	ctx context.Context,
-	beaconState iface.BeaconState,
+	beaconState iface.ReadOnlyBeaconState,
 	att *ethpb.Attestation,
-) (iface.BeaconState, error) {
+) error {
 	ctx, span := trace.StartSpan(ctx, "core.VerifyAttestationNoVerifySignature")
 	defer span.End()
 
 	if err := helpers.ValidateNilAttestation(att); err != nil {
-		return nil, err
+		return err
 	}
 	currEpoch := helpers.CurrentEpoch(beaconState)
 	prevEpoch := helpers.PrevEpoch(beaconState)
 	data := att.Data
 	if data.Target.Epoch != prevEpoch && data.Target.Epoch != currEpoch {
-		return nil, fmt.Errorf(
+		return fmt.Errorf(
 			"expected target epoch (%d) to be the previous epoch (%d) or the current epoch (%d)",
 			data.Target.Epoch,
 			prevEpoch,
 			currEpoch,
 		)
 	}
+
+	if data.Target.Epoch == currEpoch {
+		if !beaconState.MatchCurrentJustifiedCheckpoint(data.Source) {
+			return errors.New("source check point not equal to current justified checkpoint")
+		}
+	} else {
+		if !beaconState.MatchPreviousJustifiedCheckpoint(data.Source) {
+			return errors.New("source check point not equal to previous justified checkpoint")
+		}
+	}
+
 	if err := helpers.ValidateSlotTargetEpoch(att.Data); err != nil {
-		return nil, err
+		return err
 	}
 
 	s := att.Data.Slot
 	minInclusionCheck := s+params.BeaconConfig().MinAttestationInclusionDelay <= beaconState.Slot()
 	epochInclusionCheck := beaconState.Slot() <= s+params.BeaconConfig().SlotsPerEpoch
 	if !minInclusionCheck {
-		return nil, fmt.Errorf(
+		return fmt.Errorf(
 			"attestation slot %d + inclusion delay %d > state slot %d",
 			s,
 			params.BeaconConfig().MinAttestationInclusionDelay,
@@ -139,7 +150,7 @@ func VerifyAttestationNoVerifySignature(
 		)
 	}
 	if !epochInclusionCheck {
-		return nil, fmt.Errorf(
+		return fmt.Errorf(
 			"state slot %d > attestation slot %d + SLOTS_PER_EPOCH %d",
 			beaconState.Slot(),
 			s,
@@ -148,28 +159,28 @@ func VerifyAttestationNoVerifySignature(
 	}
 	activeValidatorCount, err := helpers.ActiveValidatorCount(beaconState, att.Data.Target.Epoch)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	c := helpers.SlotCommitteeCount(activeValidatorCount)
 	if uint64(att.Data.CommitteeIndex) >= c {
-		return nil, fmt.Errorf("committee index %d >= committee count %d", att.Data.CommitteeIndex, c)
+		return fmt.Errorf("committee index %d >= committee count %d", att.Data.CommitteeIndex, c)
 	}
 
 	if err := helpers.VerifyAttestationBitfieldLengths(beaconState, att); err != nil {
-		return nil, errors.Wrap(err, "could not verify attestation bitfields")
+		return errors.Wrap(err, "could not verify attestation bitfields")
 	}
 
 	// Verify attesting indices are correct.
 	committee, err := helpers.BeaconCommitteeFromState(beaconState, att.Data.Slot, att.Data.CommitteeIndex)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	indexedAtt, err := attestationutil.ConvertToIndexed(ctx, att, committee)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return nil, attestationutil.IsValidAttestationIndices(ctx, indexedAtt)
+	return attestationutil.IsValidAttestationIndices(ctx, indexedAtt)
 }
 
 // ProcessAttestationNoVerifySignature processes the attestation without verifying the attestation signature. This
@@ -182,7 +193,7 @@ func ProcessAttestationNoVerifySignature(
 	ctx, span := trace.StartSpan(ctx, "core.ProcessAttestationNoVerifySignature")
 	defer span.End()
 
-	if _, err := VerifyAttestationNoVerifySignature(ctx, beaconState, att); err != nil {
+	if err := VerifyAttestationNoVerifySignature(ctx, beaconState, att); err != nil {
 		return nil, err
 	}
 
@@ -201,16 +212,10 @@ func ProcessAttestationNoVerifySignature(
 	}
 
 	if data.Target.Epoch == currEpoch {
-		if !beaconState.MatchCurrentJustifiedCheckpoint(data.Source) {
-			return nil, errors.New("source check point not equal to current justified checkpoint")
-		}
 		if err := beaconState.AppendCurrentEpochAttestations(pendingAtt); err != nil {
 			return nil, err
 		}
 	} else {
-		if !beaconState.MatchPreviousJustifiedCheckpoint(data.Source) {
-			return nil, errors.New("source check point not equal to previous justified checkpoint")
-		}
 		if err := beaconState.AppendPreviousEpochAttestations(pendingAtt); err != nil {
 			return nil, err
 		}

beacon-chain/core/blocks/attestation_regression_test.go

@@ -6,9 +6,13 @@ import (
 	"testing"
 
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
+	"github.com/prysmaticlabs/go-bitfield"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
 	"github.com/prysmaticlabs/prysm/beacon-chain/state"
 	pb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1"
+	"github.com/prysmaticlabs/prysm/shared/params"
+	"github.com/prysmaticlabs/prysm/shared/testutil"
+	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 )
 
@@ -42,3 +46,35 @@ func TestProcessAttestationNoVerifySignature_BeaconFuzzIssue78(t *testing.T) {
 	_, err = blocks.ProcessAttestationNoVerifySignature(ctx, st, att)
 	require.ErrorContains(t, "committee index 1 >= committee count 1", err)
 }
+
+// Regression introduced in https://github.com/prysmaticlabs/prysm/pull/8566.
+func TestVerifyAttestationNoVerifySignature_IncorrectSourceEpoch(t *testing.T) {
+	// Attestation with an empty signature
+
+	beaconState, _ := testutil.DeterministicGenesisState(t, 100)
+
+	aggBits := bitfield.NewBitlist(3)
+	aggBits.SetBitAt(1, true)
+	var mockRoot [32]byte
+	copy(mockRoot[:], "hello-world")
+	att := &ethpb.Attestation{
+		Data: &ethpb.AttestationData{
+			Source: &ethpb.Checkpoint{Epoch: 99, Root: mockRoot[:]},
+			Target: &ethpb.Checkpoint{Epoch: 0, Root: make([]byte, 32)},
+		},
+		AggregationBits: aggBits,
+	}
+
+	zeroSig := [96]byte{}
+	att.Signature = zeroSig[:]
+
+	err := beaconState.SetSlot(beaconState.Slot() + params.BeaconConfig().MinAttestationInclusionDelay)
+	require.NoError(t, err)
+	ckp := beaconState.CurrentJustifiedCheckpoint()
+	copy(ckp.Root, "hello-world")
+	require.NoError(t, beaconState.SetCurrentJustifiedCheckpoint(ckp))
+	require.NoError(t, beaconState.SetCurrentEpochAttestations([]*pb.PendingAttestation{}))
+
+	err = blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
+	assert.NotEqual(t, nil, err)
+}

beacon-chain/core/blocks/attestation_test.go

@@ -366,7 +366,7 @@ func TestVerifyAttestationNoVerifySignature_IncorrectSlotTargetEpoch(t *testing.
 		},
 	})
 	wanted := "slot 32 does not match target epoch 0"
-	_, err := blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
+	err := blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
 	assert.ErrorContains(t, wanted, err)
 }
 
@@ -428,7 +428,7 @@ func TestVerifyAttestationNoVerifySignature_OK(t *testing.T) {
 	require.NoError(t, beaconState.SetCurrentJustifiedCheckpoint(ckp))
 	require.NoError(t, beaconState.SetCurrentEpochAttestations([]*pb.PendingAttestation{}))
 
-	_, err = blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
+	err = blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
 	assert.NoError(t, err)
 }
 
@@ -453,7 +453,7 @@ func TestVerifyAttestationNoVerifySignature_BadAttIdx(t *testing.T) {
 	copy(ckp.Root, "hello-world")
 	require.NoError(t, beaconState.SetCurrentJustifiedCheckpoint(ckp))
 	require.NoError(t, beaconState.SetCurrentEpochAttestations([]*pb.PendingAttestation{}))
-	_, err := blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
+	err := blocks.VerifyAttestationNoVerifySignature(context.TODO(), beaconState, att)
 	require.ErrorContains(t, "committee index 100 >= committee count 1", err)
 }
 

beacon-chain/rpc/beaconv1/pool.go

@@ -35,7 +35,7 @@ func (bs *Server) SubmitAttestations(ctx context.Context, req *ethpb.SubmitAttes
 	var validAttestations []*ethpb_alpha.Attestation
 	for _, sourceAtt := range req.Data {
 		att := migration.V1AttToV1Alpha1(sourceAtt)
-		_, err = blocks.VerifyAttestationNoVerifySignature(ctx, headState, att)
+		err = blocks.VerifyAttestationNoVerifySignature(ctx, headState, att)
 		if err != nil {
 			continue
 		}