Ignore some untimely attestations #12387

potuz · 2023-05-12T02:13:49Z

When we receive an attestation for an old target, a node needs to regenerate the beacon state at that time in order to validate the attestation. Performing several of these replays at the same time is what stresses the nodes' CPUs and bloats memory usage. A node has a cache of seen attestation targets designed to address this situation. With the increase in validator sizes and the number of this untimely attestation, this cache quickly filled and forced us to replay several times in the same state. This PR implements a heuristic such that when a node receives many unviable checkpoint attestations, a node will simply ignore them if the target of the attestation is known to be old and not have been a checkpoint in any chain known to our node

This PR also made a change to validate aggregated p2p pipeline to not process slots for state used to verify selection proof and signature

james-prysm · 2023-05-12T15:02:28Z

beacon-chain/blockchain/process_attestation_helpers.go

+		return nil, errors.Wrap(err, "could not compute epoch start")
+	}
+	cachedState = transition.NextSlotState(c.Root, slot)
+	if cachedState != nil {


cachedState, err := s.checkpointStateCache.StateByCheckpoint(c) has a check
!cachedState.IsNil() on line 33. is this cachedState different as we don't need that IsNil check?

I think it's safe to add !cachedState.IsNil() check here

james-prysm · 2023-05-12T15:06:49Z

beacon-chain/blockchain/process_attestation_helpers.go

+	// 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))


should there be a check for cfg or ForkChoice initialized

I don't think so, this is in the blockchain package and forkchoice starts up before in the node.

terencechain · 2023-05-12T15:41:24Z

beacon-chain/blockchain/process_attestation_test.go

-		{
-			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",
-		},


Note I removed this test. This becomes an impossible scenario with the addition of getting head state read-only

terencechain · 2023-05-12T15:42:20Z

beacon-chain/sync/validate_aggregate_proof.go

@@ -152,20 +151,6 @@ func (s *Service) validateAggregatedAtt(ctx context.Context, signed *ethpb.Signe
 		return pubsub.ValidationIgnore, err
 	}

-	attSlot := signed.Message.Aggregate.Data.Slot


This change is quite irrelevant but it's worth noting to the audience:
Processing slots is unnecessary here

james-prysm · 2023-05-12T15:49:59Z

beacon-chain/blockchain/receive_attestation.go

 	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
 	}
+	s.cfg.ForkChoiceStore.RLock()


Should this be outside of getAttPreState instead of inside?
getAttPreState is used in OnAttestation which don't have the same locks.

OnAttestation path is called only from UpdateHead that grabs the lock.

…ysm into ignore_old_attestations

…nore_old_attestations

terencechain

Lgtm

* 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>

infosecual · 2023-05-12T20:41:17Z

beacon-chain/blockchain/chain_info.go

@@ -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) {


I am not seeing that this function is called from anywhere

Is this just to ensure s.cfg.ForkChoiceStore.RLock() is grabbed if it this accessor is used in the future? I did check that the locks are acquired correctly in the other call path for s.cfg.ForkChoiceStore.IsViableForCheckpoint(cp). The only other call path I see is UpdateHead->processAttestations->receiveAttestationNoPubsub->OnAttestation->getAttPreState)

the lock is acquired in UpdateHead FYI

+1. I think it can be removed

infosecual · 2023-05-12T20:52:00Z

beacon-chain/blockchain/chain_info.go

@@ -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) {


Is this just to ensure s.cfg.ForkChoiceStore.RLock() is grabbed if it this accessor is used in the future? I did check that the locks are acquired correctly in the other call path for s.cfg.ForkChoiceStore.IsViableForCheckpoint(cp). The only other call path I see is UpdateHead->processAttestations->receiveAttestationNoPubsub->OnAttestation->getAttPreState)

infosecual · 2023-05-12T20:55:50Z

beacon-chain/blockchain/chain_info.go

@@ -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) {


the lock is acquired in UpdateHead FYI

infosecual · 2023-05-12T20:58:31Z

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

+	if node.slot > epochStart {
+		return false, nil
+	}
+


I might remove this space for OCD unless it is to separate these conditionals into groups but def not a big deal

Hi David yeah this function was just mostly for future reference that locking of forlchoice is tricky within the Blockchain package but any external user has to use the service one that is self locking

potuz added 3 commits May 11, 2023 23:13

Ignore some untimely attestations

96ccc5b

correct child slot check

2011de3

consider tips as viable for checkpoints

3dc041f

potuz force-pushed the ignore_old_attestations branch from f81436a to 3dc041f Compare May 12, 2023 02:39

potuz added 3 commits May 12, 2023 09:20

deal with canonical blocks

dc005c3

forkchoice unit tests

e9d7f89

blockchain readonly beacon state

d4bbbb5

james-prysm reviewed May 12, 2023

View reviewed changes

terencechain reviewed May 12, 2023

View reviewed changes

james-prysm reviewed May 12, 2023

View reviewed changes

potuz and others added 9 commits May 12, 2023 08:50

Ignore some untimely attestations

a0b0d52

correct child slot check

4d1bd27

consider tips as viable for checkpoints

4dd002b

deal with canonical blocks

cb063b1

forkchoice unit tests

1b689f4

blockchain readonly beacon state

8afd19b

Fix AttestationTargetState mock

9a6950d

Fix ineffectual assignment lint

500ddb4

Fix blockchain tests

8dd8373

terencechain force-pushed the ignore_old_attestations branch from 2e6a9d5 to 8dd8373 Compare May 12, 2023 15:50

terencechain marked this pull request as ready for review May 12, 2023 15:50

terencechain requested a review from a team as a code owner May 12, 2023 15:50

terencechain requested review from rauljordan, saolyn and terencechain May 12, 2023 15:50

prestonvanloon and others added 3 commits May 12, 2023 10:55

Merge branch 'develop' into ignore_old_attestations

6821382

Fix build

80b1dc2

Merge branch 'ignore_old_attestations' of github.com:prysmaticlabs/pr…

0242a4f

…ysm into ignore_old_attestations

potuz added 3 commits May 12, 2023 13:29

Merge remote-tracking branch 'origin/ignore_old_attestations' into ig…

8d06d41

…nore_old_attestations

Add Nil check

603da23

add comment on lock

ed7a1cd

terencechain approved these changes May 12, 2023

View reviewed changes

prestonvanloon approved these changes May 12, 2023

View reviewed changes

prestonvanloon added the OK to merge label May 12, 2023

prylabs-bulldozer bot merged commit b991780 into develop May 12, 2023

prylabs-bulldozer bot deleted the ignore_old_attestations branch May 12, 2023 16:55

infosecual reviewed May 12, 2023

View reviewed changes

infosecual reviewed May 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore some untimely attestations #12387

Ignore some untimely attestations #12387

potuz commented May 12, 2023 •

edited by terencechain

Loading

james-prysm May 12, 2023

james-prysm May 12, 2023

james-prysm May 12, 2023

potuz May 12, 2023

terencechain May 12, 2023

terencechain May 12, 2023

james-prysm May 12, 2023

potuz May 12, 2023

terencechain left a comment

infosecual May 12, 2023

infosecual May 12, 2023

infosecual May 12, 2023

terencechain May 12, 2023

infosecual May 12, 2023

infosecual May 12, 2023

infosecual May 12, 2023

potuz May 13, 2023

Ignore some untimely attestations #12387

Ignore some untimely attestations #12387

Conversation

potuz commented May 12, 2023 • edited by terencechain Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potuz commented May 12, 2023 •

edited by terencechain

Loading