Stricter match of BlockError in lookup sync by dapplion · Pull Request #6321 · sigp/lighthouse

dapplion · 2024-08-28T13:38:26Z

Issue Addressed

Adds a strict match without fallbacks to handle all processing errors in lookup sync explicitly. With this we can:

Be explicit on what errors must result in penalties
Do not retry errors that are deterministic on the block root

Example problem 1

Today, if a block has incorrect state root we will download and process it 5 times. This is wasteful, we should discard the block immediately

Example problem 2

Assume we introduce a new BlockError variant in a future fork or network change. Lookup sync is very sensitive code, and not handling this error variant properly may result in sync getting stuck

Proposed Changes

In the network processor convert BlockError to LookupSyncProcessingResult.

Then lookup sync logic is quite simple match on LookupSyncProcessingResult, without fallbacks.

dapplion · 2024-08-28T13:40:57Z

+                                for peer in peer_group.of_index(index) {
+                                    cx.report_peer(
+                                        *peer,
+                                        PeerAction::MidToleranceError,


I think we should up this to LowToleranceError

dapplion · 2024-08-28T13:42:08Z

-                            );
+                        let recoverable = match other.rpc_scoring() {
+                            ErrorCategory::Internal { recoverable } => {
+                                if matches!(other, BlockError::ExecutionPayloadError(_)) {


I kept this extra condition from the original match as I imagine that when EL goes offline the node would spam non-actionable errors

dapplion · 2024-10-20T23:02:37Z

@pawanjay176 I have moved the big match into the network beacon processor. This is analogous to how we handle the result of processing chain segments and gossip objects.

I have applied all your comments to the current version.

I think the PR makes much more sense now, ready for a re-review

dapplion · 2024-10-20T23:03:58Z

+                    "Error processing block component";
+                    "block_root" => %block_root,
+                    "err" => ?err,
+                );


Do we want to log internal errors at a higher level than debug? If yes I can move the log statement below and match the type of ErrorCategory

# Conflicts: # beacon_node/network/src/sync/tests/lookups.rs

jimmygchen

I still haven't finished reviewing this but I'll try to get back to this later today or Monday.

jimmygchen · 2024-12-12T23:14:09Z

                            "block_hash" => %hash,
                        );
-                        self.chain.recompute_head_at_current_slot().await;
+                        // Head will be recomputed `handle_lookup_sync_processing_result`


This doesn't cover the gossip case right?

Feels like it might be easier to move the recompute_head logic back here, otherwise we'll have to either do it in two places or having to pass a flag here and conditionally recompute head.

jimmygchen · 2024-12-12T23:29:24Z

+    /// The error also indicates which block component index is malicious if applicable.
+    Malicious { retry: bool, index: usize },


I found index slightly confusing - what do you think about calling this sidecar_index or data_sidecar_index, and potentially make this an Option, given we're using this for blocks as well?

Does it make sense to call this Faulty instead? we don't know if the peer is malicious - i think it might be more consistent with the terminology used in sync, although it doesn't really matter, a faulty peer and malicious peer both send invalid data.

jimmygchen · 2024-12-12T23:35:05Z

            process_type,
-            result: result.into(),
+            result: self
+                .handle_lookup_sync_processing_result(block_root, result)


Looks like we're potentially recomputing head twice here - one above at

lighthouse/beacon_node/network/src/network_beacon_processor/sync_methods.rs

Line 245 in 2c80738

self.chain.recompute_head_at_current_slot().await;

and one in self.handle_lookup_sync_processing_result.

jimmygchen · 2024-12-16T11:48:56Z

+    pub fn malicious_no_retry() -> Self {
+        Self::Malicious {
+            retry: false,
+            index: 0,


I think it make sense to make this Option, otherwise we can't distinguish between 0 index or None

jimmygchen · 2024-12-16T12:16:27Z

+                    // unreachable, this error is only part of gossip
+                    BlockError::BlobNotRequired(_) => ErrorCategory::malicious_retry().into(),
+                    // Unreachable: This variants never happen in lookup sync, only in range sync.
+                    // Does not matter what we set here, just setting `internal_recoverable` to


Suggested change

// Does not matter what we set here, just setting `internal_recoverable` to

// Does not matter what we set here, just setting `internal_retry` to

jimmygchen · 2024-12-16T12:19:31Z

+                    // Unreachable: This variants never happen in lookup sync, only in range sync.
+                    // Does not matter what we set here, just setting `internal_recoverable` to
+                    // put something.
+                    BlockError::NonLinearParentRoots | BlockError::NonLinearSlots => {


For code path that's expected to be unreachable, is it worth logging a warning here?

jimmygchen · 2024-12-16T12:37:48Z

+                    BlockError::Slashable => ErrorCategory::internal_no_retry().into(),
+                    // TODO: BeaconChainError should be retried?
+                    BlockError::BeaconChainError(_) | BlockError::InternalError(_) => {
+                        ErrorCategory::internal_no_retry().into()


Both of these variants cover quite a few scenarios, I'm not sure if it's safe to say no retry to them?

Complements - #6321 by detecting if the proposer signature is valid or not during RPC block processing. In lookup sync, if the invalid signature signature is the proposer signature, it's not deterministic on the block root. So we should only penalize the sending peer and retry. Otherwise, if it's on the body we should drop the lookup and penalize all peers that claim to have imported the block

mergify · 2025-02-10T00:36:50Z

This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏

dapplion · 2025-03-13T14:33:04Z

Closing for stale for Stricter match of BlockError in sync #7129

dapplion commented Aug 28, 2024

View reviewed changes

Comment thread beacon_node/beacon_chain/src/block_verification.rs Outdated

dapplion commented Aug 28, 2024

View reviewed changes

Comment thread beacon_node/beacon_chain/src/block_verification.rs Outdated

dapplion force-pushed the peerdas-peergroup-scoring branch from a2ffbd2 to 5a69a7b Compare August 28, 2024 13:46

dapplion requested review from jimmygchen, pawanjay176 and realbigsean August 28, 2024 13:47

pawanjay176 added the under-review A reviewer has only partially completed a review. label Aug 29, 2024

pawanjay176 reviewed Sep 6, 2024

View reviewed changes

dapplion changed the base branch from unstable to peerdas-peergroup-scoring-only September 9, 2024 11:55

dapplion changed the title ~~Attribute invalid column proof error to correct peer~~ Stricter match of BlockError in sync Sep 9, 2024

mergify Bot deleted the branch sigp:unstable September 23, 2024 18:49

mergify Bot closed this Sep 23, 2024

realbigsean reopened this Sep 23, 2024

realbigsean changed the base branch from peerdas-peergroup-scoring-only to unstable September 23, 2024 23:07

dapplion mentioned this pull request Oct 20, 2024

Blobs by range failures should penalize the peer #6517

Closed

Full match process error in lookup sync

33d10e5

dapplion force-pushed the peerdas-peergroup-scoring branch from 5a69a7b to 33d10e5 Compare October 20, 2024 22:53

dapplion changed the title ~~Stricter match of BlockError in sync~~ Stricter match of BlockError in lookup sync Oct 20, 2024

dapplion commented Oct 20, 2024

View reviewed changes

dapplion added ready-for-review The code is ready for review syncing and removed under-review A reviewer has only partially completed a review. labels Oct 20, 2024

This was referenced Oct 20, 2024

Detect invalid proposer signature on RPC block processing #6519

Merged

Strict match of errors in backfill sync #6520

Merged

Strict match of errors in range sync #6521

Closed

dapplion mentioned this pull request Oct 21, 2024

Penalize peer if missing blobs #6525

Closed

jimmygchen self-assigned this Dec 10, 2024

Merge branch 'unstable' into peerdas-peergroup-scoring

2c80738

# Conflicts: # beacon_node/network/src/sync/tests/lookups.rs

jimmygchen reviewed Dec 13, 2024

View reviewed changes

jimmygchen added the under-review A reviewer has only partially completed a review. label Dec 16, 2024

jimmygchen reviewed Dec 16, 2024

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 22, 2025

jimmygchen removed their assignment Feb 10, 2025

dapplion mentioned this pull request Mar 13, 2025

Stricter match of BlockError in sync #7129

Open

dapplion closed this Mar 13, 2025

		/// The error also indicates which block component index is malicious if applicable.
		Malicious { retry: bool, index: usize },

	// Does not matter what we set here, just setting `internal_recoverable` to
	// Does not matter what we set here, just setting `internal_retry` to

Conversation

dapplion commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dapplion commented Oct 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmygchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Feb 10, 2025

Uh oh!

dapplion commented Mar 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dapplion commented Aug 28, 2024 •

edited

Loading