Remove duplicated LMD FFG check for fork choice attestation (#8276)

* Rm redundant lmd<->ffg check for fork choice attestation

* Comments

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/blockchain/process_attestation.go

@@ -76,10 +76,8 @@ func (s *Service) onAttestation(ctx context.Context, a *ethpb.Attestation) ([]ui
 		return nil, errors.Wrap(err, "could not verify attestation beacon block")
 	}
 
-	// Verify LMG GHOST and FFG votes are consistent with each other.
-	if err := s.verifyLMDFFGConsistent(ctx, a.Data.Target.Epoch, a.Data.Target.Root, a.Data.BeaconBlockRoot); err != nil {
-		return nil, errors.Wrap(err, "could not verify attestation beacon block")
-	}
+	// Note that LMG GHOST and FFG consistency check is ignored because it was performed in sync's validation pipeline:
+	// validate_aggregate_proof.go and validate_beacon_attestation.go
 
 	// Verify attestations can only affect the fork choice of subsequent slots.
 	if err := helpers.VerifySlotTime(genesisTime, a.Data.Slot+1, params.BeaconNetworkConfig().MaximumGossipClockDisparity); err != nil {

beacon-chain/blockchain/process_attestation_helpers.go

@@ -1,7 +1,6 @@
 package blockchain
 
 import (
-	"bytes"
 	"context"
 	"fmt"
 	"strconv"
@@ -106,23 +105,6 @@ func (s *Service) verifyBeaconBlock(ctx context.Context, data *ethpb.Attestation
 	return nil
 }
 
-// verifyLMDFFGConsistent verifies LMD GHOST and FFG votes are consistent with each other.
-func (s *Service) verifyLMDFFGConsistent(ctx context.Context, ffgEpoch uint64, ffgRoot, lmdRoot []byte) error {
-	ffgSlot, err := helpers.StartSlot(ffgEpoch)
-	if err != nil {
-		return err
-	}
-	r, err := s.ancestor(ctx, lmdRoot, ffgSlot)
-	if err != nil {
-		return err
-	}
-	if !bytes.Equal(ffgRoot, r) {
-		return errors.New("FFG and LMD votes are not consistent")
-	}
-
-	return nil
-}
-
 // verifyAttestation validates input attestation is valid.
 func (s *Service) verifyAttestation(ctx context.Context, baseState *stateTrie.BeaconState, a *ethpb.Attestation) (*ethpb.IndexedAttestation, error) {
 	committee, err := helpers.BeaconCommitteeFromState(baseState, a.Data.Slot, a.Data.CommitteeIndex)

beacon-chain/blockchain/process_attestation_test.go

@@ -355,54 +355,6 @@ func TestVerifyBeaconBlock_OK(t *testing.T) {
 	assert.NoError(t, service.verifyBeaconBlock(ctx, d), "Did not receive the wanted error")
 }
 
-func TestVerifyLMDFFGConsistent_NotOK(t *testing.T) {
-	ctx := context.Background()
-	beaconDB := testDB.SetupDB(t)
-
-	cfg := &Config{BeaconDB: beaconDB, ForkChoiceStore: protoarray.New(0, 0, [32]byte{})}
-	service, err := NewService(ctx, cfg)
-	require.NoError(t, err)
-
-	b32 := testutil.NewBeaconBlock()
-	b32.Block.Slot = 32
-	require.NoError(t, service.beaconDB.SaveBlock(ctx, b32))
-	r32, err := b32.Block.HashTreeRoot()
-	require.NoError(t, err)
-	b33 := testutil.NewBeaconBlock()
-	b33.Block.Slot = 33
-	b33.Block.ParentRoot = r32[:]
-	require.NoError(t, service.beaconDB.SaveBlock(ctx, b33))
-	r33, err := b33.Block.HashTreeRoot()
-	require.NoError(t, err)
-
-	wanted := "FFG and LMD votes are not consistent"
-	assert.ErrorContains(t, wanted, service.verifyLMDFFGConsistent(context.Background(), 1, []byte{'a'}, r33[:]))
-}
-
-func TestVerifyLMDFFGConsistent_OK(t *testing.T) {
-	ctx := context.Background()
-	beaconDB := testDB.SetupDB(t)
-
-	cfg := &Config{BeaconDB: beaconDB, ForkChoiceStore: protoarray.New(0, 0, [32]byte{})}
-	service, err := NewService(ctx, cfg)
-	require.NoError(t, err)
-
-	b32 := testutil.NewBeaconBlock()
-	b32.Block.Slot = 32
-	require.NoError(t, service.beaconDB.SaveBlock(ctx, b32))
-	r32, err := b32.Block.HashTreeRoot()
-	require.NoError(t, err)
-	b33 := testutil.NewBeaconBlock()
-	b33.Block.Slot = 33
-	b33.Block.ParentRoot = r32[:]
-	require.NoError(t, service.beaconDB.SaveBlock(ctx, b33))
-	r33, err := b33.Block.HashTreeRoot()
-	require.NoError(t, err)
-
-	err = service.verifyLMDFFGConsistent(context.Background(), 1, r32[:], r33[:])
-	assert.NoError(t, err, "Could not verify LMD and FFG votes to be consistent")
-}
-
 func TestVerifyFinalizedConsistency_InconsistentRoot(t *testing.T) {
 	ctx := context.Background()
 	beaconDB := testDB.SetupDB(t)

beacon-chain/blockchain/receive_attestation.go

@@ -62,7 +62,18 @@ func (s *Service) AttestationPreState(ctx context.Context, att *ethpb.Attestatio
 
 // VerifyLmdFfgConsistency verifies that attestation's LMD and FFG votes are consistency to each other.
 func (s *Service) VerifyLmdFfgConsistency(ctx context.Context, a *ethpb.Attestation) error {
-	return s.verifyLMDFFGConsistent(ctx, a.Data.Target.Epoch, a.Data.Target.Root, a.Data.BeaconBlockRoot)
+	targetSlot, err := helpers.StartSlot(a.Data.Target.Epoch)
+	if err != nil {
+		return err
+	}
+	r, err := s.ancestor(ctx, a.Data.BeaconBlockRoot, targetSlot)
+	if err != nil {
+		return err
+	}
+	if !bytes.Equal(a.Data.Target.Root, r) {
+		return errors.New("FFG and LMD votes are not consistent")
+	}
+	return nil
 }
 
 // VerifyFinalizedConsistency verifies input root is consistent with finalized store.

beacon-chain/blockchain/receive_attestation_test.go

@@ -8,7 +8,9 @@ import (
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/helpers"
 	testDB "github.com/prysmaticlabs/prysm/beacon-chain/db/testing"
+	"github.com/prysmaticlabs/prysm/beacon-chain/forkchoice/protoarray"
 	"github.com/prysmaticlabs/prysm/shared/params"
+	"github.com/prysmaticlabs/prysm/shared/testutil"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 )
 
@@ -23,3 +25,59 @@ func TestAttestationCheckPtState_FarFutureSlot(t *testing.T) {
 	_, err := chainService.AttestationPreState(context.Background(), &ethpb.Attestation{Data: &ethpb.AttestationData{Target: &ethpb.Checkpoint{Epoch: e}}})
 	require.ErrorContains(t, "exceeds max allowed value relative to the local clock", err)
 }
+
+func TestVerifyLMDFFGConsistent_NotOK(t *testing.T) {
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+
+	cfg := &Config{BeaconDB: beaconDB, ForkChoiceStore: protoarray.New(0, 0, [32]byte{})}
+	service, err := NewService(ctx, cfg)
+	require.NoError(t, err)
+
+	b32 := testutil.NewBeaconBlock()
+	b32.Block.Slot = 32
+	require.NoError(t, service.beaconDB.SaveBlock(ctx, b32))
+	r32, err := b32.Block.HashTreeRoot()
+	require.NoError(t, err)
+	b33 := testutil.NewBeaconBlock()
+	b33.Block.Slot = 33
+	b33.Block.ParentRoot = r32[:]
+	require.NoError(t, service.beaconDB.SaveBlock(ctx, b33))
+	r33, err := b33.Block.HashTreeRoot()
+	require.NoError(t, err)
+
+	wanted := "FFG and LMD votes are not consistent"
+	a := testutil.NewAttestation()
+	a.Data.Target.Epoch = 1
+	a.Data.Target.Root = []byte{'a'}
+	a.Data.BeaconBlockRoot = r33[:]
+	require.ErrorContains(t, wanted, service.VerifyLmdFfgConsistency(context.Background(), a))
+}
+
+func TestVerifyLMDFFGConsistent_OK(t *testing.T) {
+	ctx := context.Background()
+	beaconDB := testDB.SetupDB(t)
+
+	cfg := &Config{BeaconDB: beaconDB, ForkChoiceStore: protoarray.New(0, 0, [32]byte{})}
+	service, err := NewService(ctx, cfg)
+	require.NoError(t, err)
+
+	b32 := testutil.NewBeaconBlock()
+	b32.Block.Slot = 32
+	require.NoError(t, service.beaconDB.SaveBlock(ctx, b32))
+	r32, err := b32.Block.HashTreeRoot()
+	require.NoError(t, err)
+	b33 := testutil.NewBeaconBlock()
+	b33.Block.Slot = 33
+	b33.Block.ParentRoot = r32[:]
+	require.NoError(t, service.beaconDB.SaveBlock(ctx, b33))
+	r33, err := b33.Block.HashTreeRoot()
+	require.NoError(t, err)
+
+	a := testutil.NewAttestation()
+	a.Data.Target.Epoch = 1
+	a.Data.Target.Root = r32[:]
+	a.Data.BeaconBlockRoot = r33[:]
+	err = service.VerifyLmdFfgConsistency(context.Background(), a)
+	require.NoError(t, err, "Could not verify LMD and FFG votes to be consistent")
+}