Stricter match of BlockError in lookup sync #6321

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

beacon_node/network/src/sync/block_lookups/mod.rs

+                                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

beacon_node/network/src/sync/block_lookups/mod.rs

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

beacon_node/beacon_chain/src/block_verification.rs

beacon_node/network/src/network_beacon_processor/sync_methods.rs

beacon_node/beacon_chain/src/data_availability_checker/error.rs

beacon_node/network/src/sync/network_context.rs

beacon_node/beacon_chain/src/block_verification.rs

beacon_node/beacon_chain/src/data_availability_checker.rs

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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

+                    "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

beacon_node/network/src/network_beacon_processor/mod.rs

@@ -983,7 +985,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                            "result" => "imported block and custody columns",
                            "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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

@@ -214,7 +269,9 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
        // Sync handles these results
        self.send_sync_message(SyncMessage::BlockComponentProcessed {
            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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

+    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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

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

beacon_node/network/src/network_beacon_processor/sync_methods.rs

+                    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?

dapplion commented Aug 28, 2024

View reviewed changes

beacon_node/beacon_chain/src/block_verification.rs Outdated Show resolved Hide resolved

dapplion commented Aug 28, 2024

View reviewed changes

beacon_node/beacon_chain/src/block_verification.rs Outdated Show resolved Hide resolved

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

Open

Strict match of errors in backfill sync #6520

Merged

Strict match of errors in range sync #6521

Draft

dapplion mentioned this pull request Oct 21, 2024

Penalize peer if missing blobs #6525

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stricter match of BlockError in lookup sync #6321

Stricter match of BlockError in lookup sync #6321

dapplion commented Aug 28, 2024 •

edited

Loading

dapplion Aug 28, 2024

dapplion Aug 28, 2024

dapplion commented Oct 20, 2024

dapplion Oct 20, 2024

jimmygchen left a comment

jimmygchen Dec 12, 2024

jimmygchen Dec 16, 2024

jimmygchen Dec 12, 2024

jimmygchen Dec 16, 2024

jimmygchen Dec 12, 2024

jimmygchen Dec 16, 2024

jimmygchen Dec 16, 2024

jimmygchen Dec 16, 2024

jimmygchen Dec 16, 2024

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

Stricter match of BlockError in lookup sync #6321

Are you sure you want to change the base?

Stricter match of BlockError in lookup sync #6321

Conversation

dapplion commented Aug 28, 2024 • edited Loading

Issue Addressed

Proposed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapplion commented Oct 20, 2024

Choose a reason for hiding this comment

jimmygchen 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

Choose a reason for hiding this comment

dapplion commented Aug 28, 2024 •

edited

Loading