-
Notifications
You must be signed in to change notification settings - Fork 957
Add individual by_range sync requests #6497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
realbigsean
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good overall!
beacon_node/network/src/sync/network_context/requests/blobs_by_range.rs
Outdated
Show resolved
Hide resolved
| "epoch" => epoch, | ||
| "peer" => %peer_id, | ||
| ); | ||
| let _blocks_req_id = self.send_blocks_by_range_request(peer_id, request.clone(), id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
df1a6c7 to
5fa2912
Compare
beacon_node/network/src/sync/network_context/requests/blocks_by_range.rs
Show resolved
Hide resolved
beacon_node/network/src/sync/network_context/requests/data_columns_by_range.rs
Show resolved
Hide resolved
5fa2912 to
a2d1d69
Compare
afe72d8 to
7f71079
Compare
|
Rebased on latest unstable, @jimmygchen ready for another review |
7f71079 to
9f85064
Compare
| let Entry::Occupied(mut entry) = self.range_block_components_requests.entry(request_id) | ||
| else { | ||
| id: ComponentsByRangeRequestId, | ||
| block_or_blob: RangeBlockComponent<T::EthSpec>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename this to block_component or range_block_component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! 000bc5512
9f85064 to
d72b61d
Compare
b0774e0 to
8e0758c
Compare
realbigsean
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, couldn't find any issues
jimmygchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks
- PR sigp#6497 made obsolete some consistency checks inside the batch I forgot to remove the consumers of those errors Remove un-used batch sync error condition, which was a nested `Result<_, Result<_, E>>`
- PR #6497 made obsolete some consistency checks inside the batch I forgot to remove the consumers of those errors Remove un-used batch sync error condition, which was a nested `Result<_, Result<_, E>>`
- PR sigp#6497 made obsolete some consistency checks inside the batch I forgot to remove the consumers of those errors Remove un-used batch sync error condition, which was a nested `Result<_, Result<_, E>>`
- PR sigp#6497 made obsolete some consistency checks inside the batch I forgot to remove the consumers of those errors Remove un-used batch sync error condition, which was a nested `Result<_, Result<_, E>>`
Issue Addressed
Part of
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
second step is to add individual request accumulators for
blocks_by_range,blobs_by_range, anddata_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
SyncRequestId::RangeBlockAndBlobsSyncNetworkContext::range_block_components_requestsVec<RpcBlock>, and insert intorange_syncNow
SyncRequestId::RangeBlockAndBlobsSyncNetworkContext:: blocks_by_range_requestsVec<SignedBlock>, and insert intoSyncNetworkContext::components_by_range_requestsVec<RpcBlock>, and insert intorange_sync