Add individual by_range sync requests #6497

dapplion · 2024-10-16T22:37:57Z

Issue Addressed

Part of

Improve range sync with PeerDAS #6258

To address PeerDAS sync issues we need to make individual by_range requests within a batch retriable. We should adopt the same pattern for lookup sync where each request (block/blobs/columns) is tracked individually within a "meta" request that group them all and handles retry logic.

Proposed Changes

Building on Generalize sync ActiveRequests #6398

second step is to add individual request accumulators for blocks_by_range, blobs_by_range, and data_columns_by_range. This will allow each request to progress independently and be retried separately.

Most of the logic is just piping, excuse the large diff. This PR does not change the logic of how requests are handled or retried. This will be done in a future PR changing the logic of RangeBlockComponentsRequest.

Before

Sync manager receives block with SyncRequestId::RangeBlockAndBlobs
Insert block into SyncNetworkContext::range_block_components_requests
(If received stream terminators of all requests)
- Return Vec<RpcBlock>, and insert into range_sync

Now

Sync manager receives block with SyncRequestId::RangeBlockAndBlobs
Insert block into SyncNetworkContext:: blocks_by_range_requests
(If received stream terminator of this request)
- Return Vec<SignedBlock>, and insert into SyncNetworkContext::components_by_range_requests
- (If received a result for all requests)
  - Return Vec<RpcBlock>, and insert into range_sync

dapplion · 2024-10-16T22:39:22Z

beacon_node/network/src/sync/range_sync/range.rs

+        // practice
+        //
+        // rig.expect_chain_segment();
+        // rig.expect_chain_segment();


I want to migrate the range sync tests to the event-driven format but would prefer to do so in a separate PR

realbigsean

Looks really good overall!

realbigsean · 2024-10-17T15:00:57Z

beacon_node/lighthouse_network/src/service/api_types.rs

@@ -43,12 +47,50 @@ pub struct DataColumnsByRootRequestId {
    pub requester: DataColumnsByRootRequester,
 }

+#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
+pub struct BlocksByRangeRequestId {
+    pub id: Id,


i'm understanding this is the sub-request id and requester.id is the meta-request id, can we rename one of them? it's sort of confusing

What name(s) do you have in mind? I can add docs otherwise

realbigsean · 2024-10-17T17:17:16Z

beacon_node/network/src/sync/network_context/requests/blobs_by_range.rs

+        {
+            return Err(LookupVerifyError::UnrequestedSlot(item.slot()));
+        }
+        // TODO: Should check if index is within bounds


As in the blob index? might as well throw that check in now right

Addressed in 5fa2912

realbigsean · 2024-10-17T17:33:00Z

beacon_node/network/src/sync/network_context.rs

@@ -211,21 +208,9 @@ pub struct SyncNetworkContext<T: BeaconChainTypes> {

 /// Small enumeration to make dealing with block and blob requests easier.
 pub enum BlockOrBlob<E: EthSpec> {


Might be time to rename this.. maybe RangeBlockComponent because it should be the wrapper for the return type of RangeBlockComponentRequest?

Addressed in 5fa2912

realbigsean · 2024-10-17T17:42:28Z

beacon_node/network/src/sync/network_context.rs

-                "epoch" => epoch,
-                "peer" => %peer_id,
-            );
+        let _blocks_req_id = self.send_blocks_by_range_request(peer_id, request.clone(), id)?;


The _blocks_req_id here is unused and blobs_req_id is only used to check if a blobs request was made. I don't see these send_ methods used anywhere else so I think we can return Result<(), ..> rather than Result<BlocksByRangeRequestId, ..> for blocks. Maybe a bool for blobs. Looks like for custody columns, we care about the count requested but not particular ids

In a future PR RangeBlockComponentsRequest will track individual requests by ID. Left this variable named but unused as a hint that it should be used.

realbigsean · 2024-10-17T17:51:57Z

beacon_node/network/src/sync/network_context.rs

-            None
-        }
-    }
-
    /// Received a blocks by range or blobs by range response for a request that couples blocks '
    /// and blobs.
    pub fn range_block_and_blob_response(


another rename suggestion range_block_component_response

Addressed in 5fa2912

realbigsean · 2024-10-17T17:54:00Z

beacon_node/network/src/sync/network_context.rs

+        if let Err(e) = {
+            let request = entry.get_mut();
+            match block_or_blob {
+                BlockOrBlob::Block(resp) => match resp {


Can do this here and for blobs and columns as well

resp.map(|(blocks, _)| { request.add_blocks(blocks); })

Addressed in 5fa2912

dapplion added ready-for-review The code is ready for review syncing labels Oct 16, 2024

dapplion commented Oct 16, 2024

View reviewed changes

dapplion mentioned this pull request Oct 16, 2024

Generalize sync block lookup tests #6498

Open

realbigsean requested changes Oct 17, 2024

View reviewed changes

Base automatically changed from sync-active-request-generalize to unstable October 17, 2024 18:14

dapplion added 2 commits October 18, 2024 17:20

Add individual by_range sync requests

ff9e2f4

Address review

5fa2912

dapplion force-pushed the sync-active-request-byrange branch from df1a6c7 to 5fa2912 Compare October 18, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add individual by_range sync requests #6497

Add individual by_range sync requests #6497

dapplion commented Oct 16, 2024

dapplion Oct 16, 2024

realbigsean left a comment

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

realbigsean Oct 17, 2024

dapplion Oct 18, 2024

		@@ -211,21 +208,9 @@ pub struct SyncNetworkContext<T: BeaconChainTypes> {

		/// Small enumeration to make dealing with block and blob requests easier.
		pub enum BlockOrBlob<E: EthSpec> {

Add individual by_range sync requests #6497

Are you sure you want to change the base?

Add individual by_range sync requests #6497

Conversation

dapplion commented Oct 16, 2024

Issue Addressed

Proposed Changes

Before

Now

Choose a reason for hiding this comment

realbigsean 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment