Ignore some untimely attestations (#12387)

* Ignore some untimely attestations

* correct child slot check

* consider tips as viable for checkpoints

* deal with canonical blocks

* forkchoice unit tests

* blockchain readonly beacon state

* Ignore some untimely attestations

* correct child slot check

* consider tips as viable for checkpoints

* deal with canonical blocks

* forkchoice unit tests

* blockchain readonly beacon state

* Fix AttestationTargetState mock

* Fix ineffectual assignment lint

* Fix blockchain tests

* Fix build

* Add Nil check

* add comment on lock

---------

Co-authored-by: terence tsao <terence@prysmaticlabs.com>
Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

beacon-chain/blockchain/chain_info.go

@@ -8,6 +8,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/helpers"
 	doublylinkedtree "github.com/prysmaticlabs/prysm/v4/beacon-chain/forkchoice/doubly-linked-tree"
+	forkchoicetypes "github.com/prysmaticlabs/prysm/v4/beacon-chain/forkchoice/types"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/state"
 	fieldparams "github.com/prysmaticlabs/prysm/v4/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/v4/config/params"
@@ -380,6 +381,14 @@ func (s *Service) InForkchoice(root [32]byte) bool {
 	return s.cfg.ForkChoiceStore.HasNode(root)
 }
 
+// IsViableForkCheckpoint returns whether the given checkpoint is a checkpoint in any
+// chain known to forkchoice
+func (s *Service) IsViableForCheckpoint(cp *forkchoicetypes.Checkpoint) (bool, error) {
+	s.cfg.ForkChoiceStore.RLock()
+	defer s.cfg.ForkChoiceStore.RUnlock()
+	return s.cfg.ForkChoiceStore.IsViableForCheckpoint(cp)
+}
+
 // IsOptimisticForRoot takes the root as argument instead of the current head
 // and returns true if it is optimistic.
 func (s *Service) IsOptimisticForRoot(ctx context.Context, root [32]byte) (bool, error) {

beacon-chain/blockchain/error.go

@@ -25,8 +25,11 @@ var (
 	errWSBlockNotFound = errors.New("weak subjectivity root not found in db")
 	// errWSBlockNotFoundInEpoch is returned when a block is not found in the WS cache or DB within epoch.
 	errWSBlockNotFoundInEpoch = errors.New("weak subjectivity root not found in db within epoch")
-	// errNotDescendantOfFinalized is returned when a block is not a descendant of the finalized checkpoint
+	// ErrNotDescendantOfFinalized is returned when a block is not a descendant of the finalized checkpoint
 	ErrNotDescendantOfFinalized = invalidBlock{error: errors.New("not descendant of finalized checkpoint")}
+	// ErrNotCheckpoint is returned when a given checkpoint is not a
+	// checkpoint in any chain known to forkchoice
+	ErrNotCheckpoint = errors.New("not a checkpoint in forkchoice")
 )
 
 // An invalid block is the block that fails state transition based on the core protocol rules.

beacon-chain/blockchain/process_attestation_helpers.go

@@ -8,6 +8,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v4/async"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/transition"
+	forkchoicetypes "github.com/prysmaticlabs/prysm/v4/beacon-chain/forkchoice/types"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/v4/config/params"
 	"github.com/prysmaticlabs/prysm/v4/consensus-types/blocks"
@@ -18,7 +19,7 @@ import (
 )
 
 // getAttPreState retrieves the att pre state by either from the cache or the DB.
-func (s *Service) getAttPreState(ctx context.Context, c *ethpb.Checkpoint) (state.BeaconState, error) {
+func (s *Service) getAttPreState(ctx context.Context, c *ethpb.Checkpoint) (state.ReadOnlyBeaconState, error) {
 	// Use a multilock to allow scoped holding of a mutex by a checkpoint root + epoch
 	// allowing us to behave smarter in terms of how this function is used concurrently.
 	epochKey := strconv.FormatUint(uint64(c.Epoch), 10 /* base 10 */)
@@ -32,7 +33,45 @@ func (s *Service) getAttPreState(ctx context.Context, c *ethpb.Checkpoint) (stat
 	if cachedState != nil && !cachedState.IsNil() {
 		return cachedState, nil
 	}
+	// If the attestation is recent and canonical we can use the head state to compute the shuffling.
+	headEpoch := slots.ToEpoch(s.HeadSlot())
+	if c.Epoch == headEpoch {
+		targetSlot, err := s.cfg.ForkChoiceStore.Slot([32]byte(c.Root))
+		if err == nil && slots.ToEpoch(targetSlot)+1 >= headEpoch {
+			if s.cfg.ForkChoiceStore.IsCanonical([32]byte(c.Root)) {
+				return s.HeadStateReadOnly(ctx)
+			}
+		}
+	}
+
+	// Try the next slot cache for the early epoch calls, this should mostly have been covered already
+	// but is cheap
+	slot, err := slots.EpochStart(c.Epoch)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not compute epoch start")
+	}
+	cachedState = transition.NextSlotState(c.Root, slot)
+	if cachedState != nil && !cachedState.IsNil() {
+		if cachedState.Slot() == slot {
+			return cachedState, nil
+		}
+		cachedState, err = transition.ProcessSlots(ctx, cachedState, slot)
+		if err != nil {
+			return nil, errors.Wrap(err, "could not process slots")
+		}
+		return cachedState, nil
+	}
+
+	// Do not process attestations for old non viable checkpoints otherwise
+	ok, err := s.cfg.ForkChoiceStore.IsViableForCheckpoint(&forkchoicetypes.Checkpoint{Root: [32]byte(c.Root), Epoch: c.Epoch})
+	if err != nil {
+		return nil, errors.Wrap(err, "could not check checkpoint condition in forkchoice")
+	}
+	if !ok {
+		return nil, ErrNotCheckpoint
+	}
 
+	// Fallback to state regeneration.
 	baseState, err := s.cfg.StateGen.StateByRoot(ctx, bytesutil.ToBytes32(c.Root))
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not get pre state for epoch %d", c.Epoch)

beacon-chain/blockchain/process_attestation_test.go

@@ -27,11 +27,20 @@ func TestStore_OnAttestation_ErrorConditions(t *testing.T) {
 	blkWithoutState := util.NewBeaconBlock()
 	blkWithoutState.Block.Slot = 0
 	util.SaveBlock(t, ctx, beaconDB, blkWithoutState)
-	BlkWithOutStateRoot, err := blkWithoutState.Block.HashTreeRoot()
+
+	cp := &ethpb.Checkpoint{}
+	st, blkRoot, err := prepareForkchoiceState(ctx, 0, [32]byte{}, [32]byte{}, params.BeaconConfig().ZeroHash, cp, cp)
 	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, blkRoot))
 
 	blkWithStateBadAtt := util.NewBeaconBlock()
 	blkWithStateBadAtt.Block.Slot = 1
+	r, err := blkWithStateBadAtt.Block.HashTreeRoot()
+	require.NoError(t, err)
+	cp = &ethpb.Checkpoint{Root: r[:]}
+	st, blkRoot, err = prepareForkchoiceState(ctx, blkWithStateBadAtt.Block.Slot, r, [32]byte{}, params.BeaconConfig().ZeroHash, cp, cp)
+	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, blkRoot))
 	util.SaveBlock(t, ctx, beaconDB, blkWithStateBadAtt)
 	BlkWithStateBadAttRoot, err := blkWithStateBadAtt.Block.HashTreeRoot()
 	require.NoError(t, err)
@@ -42,7 +51,7 @@ func TestStore_OnAttestation_ErrorConditions(t *testing.T) {
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, s, BlkWithStateBadAttRoot))
 
 	blkWithValidState := util.NewBeaconBlock()
-	blkWithValidState.Block.Slot = 2
+	blkWithValidState.Block.Slot = 32
 	util.SaveBlock(t, ctx, beaconDB, blkWithValidState)
 
 	blkWithValidStateRoot, err := blkWithValidState.Block.HashTreeRoot()
@@ -57,6 +66,10 @@ func TestStore_OnAttestation_ErrorConditions(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, s, blkWithValidStateRoot))
 
+	service.head = &head{
+		state: st,
+	}
+
 	tests := []struct {
 		name      string
 		a         *ethpb.Attestation
@@ -67,11 +80,6 @@ func TestStore_OnAttestation_ErrorConditions(t *testing.T) {
 			a:         util.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:      "no pre state for attestations's target block",
-			a:         util.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: util.HydrateAttestation(&ethpb.Attestation{Data: &ethpb.AttestationData{Slot: 100 * params.BeaconConfig().SlotsPerEpoch, Target: &ethpb.Checkpoint{Epoch: 100,
@@ -160,15 +168,27 @@ func TestStore_SaveCheckpointState(t *testing.T) {
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, s, bytesutil.ToBytes32([]byte{'A'})))
 	require.NoError(t, service.cfg.BeaconDB.SaveStateSummary(ctx, &ethpb.StateSummary{Root: bytesutil.PadTo([]byte{'A'}, fieldparams.RootLength)}))
 
+	st, root, err := prepareForkchoiceState(ctx, 1, [32]byte(cp1.Root), [32]byte{}, [32]byte{'R'}, cp1, cp1)
+	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, root))
 	s1, err := service.getAttPreState(ctx, cp1)
 	require.NoError(t, err)
 	assert.Equal(t, 1*params.BeaconConfig().SlotsPerEpoch, s1.Slot(), "Unexpected state slot")
 
 	cp2 := &ethpb.Checkpoint{Epoch: 2, Root: bytesutil.PadTo([]byte{'B'}, fieldparams.RootLength)}
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, s, bytesutil.ToBytes32([]byte{'B'})))
 	require.NoError(t, service.cfg.BeaconDB.SaveStateSummary(ctx, &ethpb.StateSummary{Root: bytesutil.PadTo([]byte{'B'}, fieldparams.RootLength)}))
+
 	s2, err := service.getAttPreState(ctx, cp2)
+	require.ErrorIs(t, ErrNotCheckpoint, err)
+
+	st, root, err = prepareForkchoiceState(ctx, 33, [32]byte(cp2.Root), [32]byte(cp1.Root), [32]byte{'R'}, cp2, cp2)
 	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, root))
+
+	s2, err = service.getAttPreState(ctx, cp2)
+	require.NoError(t, err)
+
 	assert.Equal(t, 2*params.BeaconConfig().SlotsPerEpoch, s2.Slot(), "Unexpected state slot")
 
 	s1, err = service.getAttPreState(ctx, cp1)
@@ -187,6 +207,10 @@ func TestStore_SaveCheckpointState(t *testing.T) {
 	cp3 := &ethpb.Checkpoint{Epoch: 1, Root: bytesutil.PadTo([]byte{'C'}, fieldparams.RootLength)}
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, s, bytesutil.ToBytes32([]byte{'C'})))
 	require.NoError(t, service.cfg.BeaconDB.SaveStateSummary(ctx, &ethpb.StateSummary{Root: bytesutil.PadTo([]byte{'C'}, fieldparams.RootLength)}))
+	st, root, err = prepareForkchoiceState(ctx, 31, [32]byte(cp3.Root), [32]byte(cp2.Root), [32]byte{'P'}, cp2, cp2)
+	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, root))
+
 	s3, err := service.getAttPreState(ctx, cp3)
 	require.NoError(t, err)
 	assert.Equal(t, s.Slot(), s3.Slot(), "Unexpected state slot")
@@ -195,11 +219,18 @@ func TestStore_SaveCheckpointState(t *testing.T) {
 func TestStore_UpdateCheckpointState(t *testing.T) {
 	service, tr := minimalTestService(t)
 	ctx := tr.ctx
+	baseState, _ := util.DeterministicGenesisState(t, 1)
 
 	epoch := primitives.Epoch(1)
-	baseState, _ := util.DeterministicGenesisState(t, 1)
-	checkpoint := &ethpb.Checkpoint{Epoch: epoch, Root: bytesutil.PadTo([]byte("hi"), fieldparams.RootLength)}
+	blk := util.NewBeaconBlock()
+	r1, err := blk.Block.HashTreeRoot()
+	require.NoError(t, err)
+	checkpoint := &ethpb.Checkpoint{Epoch: epoch, Root: r1[:]}
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, baseState, bytesutil.ToBytes32(checkpoint.Root)))
+	st, blkRoot, err := prepareForkchoiceState(ctx, blk.Block.Slot, r1, [32]byte{}, params.BeaconConfig().ZeroHash, checkpoint, checkpoint)
+	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, blkRoot))
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, r1))
 	returned, err := service.getAttPreState(ctx, checkpoint)
 	require.NoError(t, err)
 	assert.Equal(t, params.BeaconConfig().SlotsPerEpoch.Mul(uint64(checkpoint.Epoch)), returned.Slot(), "Incorrectly returned base state")
@@ -209,8 +240,16 @@ func TestStore_UpdateCheckpointState(t *testing.T) {
 	assert.Equal(t, returned.Slot(), cached.Slot(), "State should have been cached")
 
 	epoch = 2
-	newCheckpoint := &ethpb.Checkpoint{Epoch: epoch, Root: bytesutil.PadTo([]byte("bye"), fieldparams.RootLength)}
+	blk = util.NewBeaconBlock()
+	blk.Block.Slot = 64
+	r2, err := blk.Block.HashTreeRoot()
+	require.NoError(t, err)
+	newCheckpoint := &ethpb.Checkpoint{Epoch: epoch, Root: r2[:]}
 	require.NoError(t, service.cfg.BeaconDB.SaveState(ctx, baseState, bytesutil.ToBytes32(newCheckpoint.Root)))
+	st, blkRoot, err = prepareForkchoiceState(ctx, blk.Block.Slot, r2, r1, params.BeaconConfig().ZeroHash, newCheckpoint, newCheckpoint)
+	require.NoError(t, err)
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, blkRoot))
+	require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, st, r2))
 	returned, err = service.getAttPreState(ctx, newCheckpoint)
 	require.NoError(t, err)
 	s, err := slots.EpochStart(newCheckpoint.Epoch)

beacon-chain/blockchain/receive_attestation.go

@@ -26,7 +26,7 @@ const reorgLateBlockCountAttestations = 2 * time.Second
 // AttestationStateFetcher allows for retrieving a beacon state corresponding to the block
 // root of an attestation's target checkpoint.
 type AttestationStateFetcher interface {
-	AttestationTargetState(ctx context.Context, target *ethpb.Checkpoint) (state.BeaconState, error)
+	AttestationTargetState(ctx context.Context, target *ethpb.Checkpoint) (state.ReadOnlyBeaconState, error)
 }
 
 // AttestationReceiver interface defines the methods of chain service receive and processing new attestations.
@@ -37,14 +37,17 @@ type AttestationReceiver interface {
 }
 
 // AttestationTargetState returns the pre state of attestation.
-func (s *Service) AttestationTargetState(ctx context.Context, target *ethpb.Checkpoint) (state.BeaconState, error) {
+func (s *Service) AttestationTargetState(ctx context.Context, target *ethpb.Checkpoint) (state.ReadOnlyBeaconState, error) {
 	ss, err := slots.EpochStart(target.Epoch)
 	if err != nil {
 		return nil, err
 	}
 	if err := slots.ValidateClock(ss, uint64(s.genesisTime.Unix())); err != nil {
 		return nil, err
 	}
+	// We acquire the lock here instead than on gettAttPreState because that function gets called from UpdateHead that holds a write lock
+	s.cfg.ForkChoiceStore.RLock()
+	defer s.cfg.ForkChoiceStore.RUnlock()
 	return s.getAttPreState(ctx, target)
 }
 

beacon-chain/blockchain/testing/mock.go

@@ -320,7 +320,7 @@ func (_ *ChainService) ReceiveAttestation(_ context.Context, _ *ethpb.Attestatio
 }
 
 // AttestationTargetState mocks AttestationTargetState method in chain service.
-func (s *ChainService) AttestationTargetState(_ context.Context, _ *ethpb.Checkpoint) (state.BeaconState, error) {
+func (s *ChainService) AttestationTargetState(_ context.Context, _ *ethpb.Checkpoint) (state.ReadOnlyBeaconState, error) {
 	return s.State, nil
 }
 

beacon-chain/forkchoice/doubly-linked-tree/forkchoice.go

@@ -228,6 +228,39 @@ func (f *ForkChoice) AncestorRoot(ctx context.Context, root [32]byte, slot primi
 	return n.root, nil
 }
 
+// IsViableForCheckpoint returns whether the root passed is a checkpoint root for any
+// known chain in forkchoice.
+func (f *ForkChoice) IsViableForCheckpoint(cp *forkchoicetypes.Checkpoint) (bool, error) {
+	node, ok := f.store.nodeByRoot[cp.Root]
+	if !ok || node == nil {
+		return false, nil
+	}
+	epochStart, err := slots.EpochStart(cp.Epoch)
+	if err != nil {
+		return false, err
+	}
+	if node.slot > epochStart {
+		return false, nil
+	}
+
+	if len(node.children) == 0 {
+		return true, nil
+	}
+	if node.slot == epochStart {
+		return true, nil
+	}
+	nodeEpoch := slots.ToEpoch(node.slot)
+	if nodeEpoch >= cp.Epoch {
+		return false, nil
+	}
+	for _, child := range node.children {
+		if child.slot > epochStart {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 // updateBalances updates the balances that directly voted for each block taking into account the
 // validators' latest votes.
 func (f *ForkChoice) updateBalances() error {
@@ -594,3 +627,12 @@ func (f *ForkChoice) updateJustifiedBalances(ctx context.Context, root [32]byte)
 	f.store.committeeWeight /= uint64(params.BeaconConfig().SlotsPerEpoch)
 	return nil
 }
+
+// Slot returns the slot of the given root if it's known to forkchoice
+func (f *ForkChoice) Slot(root [32]byte) (primitives.Slot, error) {
+	n, ok := f.store.nodeByRoot[root]
+	if !ok || n == nil {
+		return 0, ErrNilNode
+	}
+	return n.slot, nil
+}