Generalize sync ActiveRequests #6398

dapplion · 2024-09-16T11:41:19Z

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.

The first step is to add ActiveBlocksByRangeRequest, ActiveBlobsByRangeRequest and ActiveDataColumnsByRangeRequest. I'm in the progress of doing that in this branch https://github.com/sigp/lighthouse/compare/unstable...dapplion:lighthouse:sync-active-requests?expand=1 but I noted a lot of sensitive code repetition.

This PR is pre-step to add ActiveRequest to our existing ActiveRequest*ByRoot (the ones for by_root). Next PR will add the ActiveRequest*ByRange goodies :)

dapplion · 2024-09-17T09:41:42Z

beacon_node/lighthouse_network/src/service/api_types.rs

@@ -42,11 +37,19 @@ pub enum SyncRequestId {
    /// Request searching for a set of blobs given a hash.
    SingleBlob { id: SingleLookupReqId },
    /// Request searching for a set of data columns given a hash and list of column indices.
-    DataColumnsByRoot(DataColumnsByRootRequestId, DataColumnsByRootRequester),
+    DataColumnsByRoot(DataColumnsByRootRequestId),


Having the requester inside the ID just simplifies downstream code and makes it symmetrical with the other variants

dapplion · 2024-09-27T17:24:38Z

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

-            // Network context returns "download success" because the request has enough blobs + it
-            // downscores the peer for returning too many.
-            .expect_no_block_request();
-    }


This test needs a scenario where sync receives more blob chunks than originally requested. This can never happen in practice so I choose to not handle this scenario to reduce complexity

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

Lines 114 to 119 in 9446569

// Should never happen, ReqResp network behaviour enforces a max count of chunks

// When `max_remaining_chunks <= 1` a the inbound stream in terminated in

// `rpc/handler.rs`. Handling this case adds complexity for no gain. Even if an

// attacker could abuse this, there's no gain in sending garbage chunks that

// will be ignored anyway.

State::CompletedEarly => None,

realbigsean

Just had some nits, but looks good, overall like the direction 👌

beacon_node/lighthouse_network/src/service/api_types.rs

beacon_node/network/src/sync/network_context.rs

Co-authored-by: realbigsean <[email protected]>

…-generalize

dapplion · 2024-10-05T12:40:04Z

@realbigsean sir I fix the test sir

…-generalize

realbigsean · 2024-10-08T18:28:20Z

looks like there's another test broken, sorry @dapplion !

realbigsean · 2024-10-16T23:45:02Z

@mergify queue

mergify · 2024-10-16T23:45:12Z

queue

🛑 The pull request has been removed from the queue `default`

Pull request #6398 has been dequeued by a dequeue command.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

michaelsproul · 2024-10-17T03:53:56Z

@mergify dequeue

mergify · 2024-10-17T03:54:18Z

dequeue

✅ The pull request has been removed from the queue `default`

michaelsproul · 2024-10-17T05:13:11Z

@mergify queue

mergify · 2024-10-17T05:13:59Z

queue

🛑 The pull request has been removed from the queue `default`

Pull request #6398 has been dequeued by a dequeue command.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

…-generalize

realbigsean · 2024-10-17T14:06:54Z

@mergify queue

mergify · 2024-10-17T14:07:14Z

queue

🛑 The pull request has been removed from the queue `default`

The pull request #6398 has been manually updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

realbigsean · 2024-10-17T17:08:43Z

looks like CI here is hung on b9cd5ad ?

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

@mergify cancel

mergify · 2024-10-17T17:11:15Z

cancel

❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚.

…nc-active-request-generalize

realbigsean · 2024-10-17T18:10:55Z

@mergify queue

mergify · 2024-10-17T18:12:11Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a074e9e

* Generalize sync ActiveRequests * Remove impossible to hit test * Update beacon_node/lighthouse_network/src/service/api_types.rs Co-authored-by: realbigsean <[email protected]> * Update beacon_node/network/src/sync/network_context.rs Co-authored-by: realbigsean <[email protected]> * Update beacon_node/network/src/sync/network_context.rs Co-authored-by: realbigsean <[email protected]> * Simplify match * Fix display * Merge remote-tracking branch 'sigp/unstable' into sync-active-request-generalize * Sampling requests should not expect all responses * Merge remote-tracking branch 'sigp/unstable' into sync-active-request-generalize * Fix sampling_batch_requests_not_enough_responses_returned test * Merge remote-tracking branch 'sigp/unstable' into sync-active-request-generalize * Merge branch 'unstable' of https://github.com/sigp/lighthouse into sync-active-request-generalize

dapplion added ready-for-review The code is ready for review syncing and removed ready-for-review The code is ready for review labels Sep 16, 2024

dapplion marked this pull request as draft September 16, 2024 11:42

dapplion added the ready-for-review The code is ready for review label Sep 17, 2024

dapplion marked this pull request as ready for review September 17, 2024 12:19

Generalize sync ActiveRequests

043d52b

dapplion force-pushed the sync-active-request-generalize branch from 2e502a9 to 043d52b Compare September 17, 2024 12:21

dapplion commented Sep 17, 2024

View reviewed changes

Remove impossible to hit test

9446569

dapplion requested a review from realbigsean September 27, 2024 17:21

dapplion commented Sep 27, 2024

View reviewed changes

realbigsean approved these changes Sep 27, 2024

View reviewed changes

realbigsean 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 Sep 27, 2024

dapplion and others added 7 commits October 3, 2024 23:43

Update beacon_node/lighthouse_network/src/service/api_types.rs

b9cd5ad

Co-authored-by: realbigsean <[email protected]>

Update beacon_node/network/src/sync/network_context.rs

715ba8c

Co-authored-by: realbigsean <[email protected]>

Update beacon_node/network/src/sync/network_context.rs

0d593b8

Co-authored-by: realbigsean <[email protected]>

Simplify match

1270322

Fix display

1910bf6

Merge remote-tracking branch 'sigp/unstable' into sync-active-request…

746612d

…-generalize

Sampling requests should not expect all responses

af35a27

Merge remote-tracking branch 'sigp/unstable' into sync-active-request…

a3c1ffe

…-generalize

dapplion added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 6, 2024

dapplion mentioned this pull request Oct 8, 2024

Better assert message in lookup sampling test #6473

Merged

Fix sampling_batch_requests_not_enough_responses_returned test

a0844e1

dapplion mentioned this pull request Oct 16, 2024

Add individual by_range sync requests #6497

Open

mergify bot added a commit that referenced this pull request Oct 17, 2024

Merge of #6398

a3189fc

mergify bot mentioned this pull request Oct 17, 2024

merge queue: embarking unstable (772929f) and #6398 together #6500

Closed

5 tasks

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 17, 2024

Merge remote-tracking branch 'sigp/unstable' into sync-active-request…

fc67e59

…-generalize

Merge branch 'unstable' of https://github.com/sigp/lighthouse into sy…

b22278f

…nc-active-request-generalize

mergify bot merged commit a074e9e into unstable Oct 17, 2024
29 checks passed

mergify bot deleted the sync-active-request-generalize branch October 17, 2024 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize sync ActiveRequests #6398

Generalize sync ActiveRequests #6398

dapplion commented Sep 16, 2024

dapplion Sep 17, 2024

dapplion Sep 27, 2024

realbigsean left a comment

dapplion commented Oct 5, 2024

realbigsean commented Oct 8, 2024

realbigsean commented Oct 16, 2024

mergify bot commented Oct 16, 2024 •

edited

Loading

michaelsproul commented Oct 17, 2024

mergify bot commented Oct 17, 2024

michaelsproul commented Oct 17, 2024

mergify bot commented Oct 17, 2024

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024 •

edited

Loading

realbigsean commented Oct 17, 2024

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024 •

edited

Loading

	// Should never happen, ReqResp network behaviour enforces a max count of chunks
	// When `max_remaining_chunks <= 1` a the inbound stream in terminated in
	// `rpc/handler.rs`. Handling this case adds complexity for no gain. Even if an
	// attacker could abuse this, there's no gain in sending garbage chunks that
	// will be ignored anyway.
	State::CompletedEarly => None,

Generalize sync ActiveRequests #6398

Generalize sync ActiveRequests #6398

Conversation

dapplion commented Sep 16, 2024

dapplion Sep 17, 2024

Choose a reason for hiding this comment

dapplion Sep 27, 2024

Choose a reason for hiding this comment

realbigsean left a comment

Choose a reason for hiding this comment

dapplion commented Oct 5, 2024

realbigsean commented Oct 8, 2024

realbigsean commented Oct 16, 2024

mergify bot commented Oct 16, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

michaelsproul commented Oct 17, 2024

mergify bot commented Oct 17, 2024

✅ The pull request has been removed from the queue default

michaelsproul commented Oct 17, 2024

mergify bot commented Oct 17, 2024

🛑 The pull request has been removed from the queue default

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

realbigsean commented Oct 17, 2024

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024

❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚.

realbigsean commented Oct 17, 2024

mergify bot commented Oct 17, 2024 • edited Loading

✅ The pull request has been merged automatically

mergify bot commented Oct 16, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

✅ The pull request has been removed from the queue `default`

🛑 The pull request has been removed from the queue `default`

mergify bot commented Oct 17, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Oct 17, 2024 •

edited

Loading