Make range sync peer loadbalancing PeerDAS-friendly #6864
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AgeManning
approved these changes
Feb 3, 2025
| } | ||
| // TODO(das): previously here we de-prioritize peers that had failed to download or | ||
| // process a batch | ||
| self.send_batch(network, batch_id) |
There was a problem hiding this comment.
It looks like this function does nothing now and can just be replaced with send_batch? (Unless implementing the TODO)
|
This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏 |
|
Blocked by |
|
@dapplion this PR got closed due to base branch being deleted, would you mind re-opening this against |
7 tasks
mergify bot
pushed a commit
that referenced
this pull request
May 7, 2025
- Re-opens #6864 targeting unstable Range sync and backfill sync still assume that each batch request is done by a single peer. This assumption breaks with PeerDAS, where we request custody columns to N peers. Issues with current unstable: - Peer prioritization counts batch requests per peer. This accounting is broken now, data columns by range request are not accounted - Peer selection for data columns by range ignores the set of peers on a syncing chain, instead draws from the global pool of peers - The implementation is very strict when we have no peers to request from. After PeerDAS this case is very common and we want to be flexible or easy and handle that case better than just hard failing everything. - [x] Upstream peer prioritization to the network context, it knows exactly how many active requests a peer (including columns by range) - [x] Upstream peer selection to the network context, now `block_components_by_range_request` gets a set of peers to choose from instead of a single peer. If it can't find a peer, it returns the error `RpcRequestSendError::NoPeer` - [ ] Range sync and backfill sync handle `RpcRequestSendError::NoPeer` explicitly - [ ] Range sync: leaves the batch in `AwaitingDownload` state and does nothing. **TODO**: we should have some mechanism to fail the chain if it's stale for too long - **EDIT**: Not done in this PR - [x] Backfill sync: pauses the sync until another peer joins - **EDIT**: Same logic as unstable ### TODOs - [ ] Add tests :) - [x] Manually test backfill sync Note: this touches the mainnet path!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Range sync and backfill sync still assume that each batch request is done by a single peer. This assumption breaks with PeerDAS, where we request custody columns to N peers.
Issues with current unstable:
Proposed Changes
block_components_by_range_requestgets a set of peers to choose from instead of a single peer. If it can't find a peer, it returns the errorRpcRequestSendError::NoPeerRpcRequestSendError::NoPeerexplicitlyAwaitingDownloadstate and does nothing. TODO: we should have some mechanism to fail the chain if it's stale for too longTODOs
Note: this touches the mainnet path!