Make range sync peer loadbalancing PeerDAS-friendly

[dapplion]

Issue Addressed

• Re-opens Make range sync peer loadbalancing PeerDAS-friendly #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.

Proposed Changes

• Upstream peer prioritization to the network context, it knows exactly how many active requests a peer (including columns by range)
• 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
  • Backfill sync: pauses the sync until another peer joins - EDIT: Same logic as unstable

TODOs

• Add tests :)
• Manually test backfill sync

Note: this touches the mainnet path!

[dapplion]

I would like to wait to merge this PR first to have more test coverage

• Add more range sync tests #6872

[dapplion]

@jimmygchen I have reduced the scope of this PR. I intended to deprecate the check good_peers_on_sampling_subnets in this PR but it's a very sensitive change. I left that logic untouched, what we do now is:

• Check if good_peers_on_sampling_subnets
  • If no, don't create batch
  • If yes, create batch and send it
    • If we don't have enough custody peers, error and drop chain

In the future we can deprecate the good_peers_on_sampling_subnets check by allowing batches to remain in AwaitingDownload state. It's essentially duplicate code as we check for peer twice. It should make sync less likely to drop chains too, like we did in lookup sync by allowing batches to be peer-less for some time.

I added a TODO(das) to tackle in another PR

lighthouse/beacon_node/network/src/sync/range_sync/chain.rs

Lines 913 to 918 in 1debbbf

	// TODO(das): Handle the NoPeer case explicitly and don't drop the batch. For
	// sync to work properly it must be okay to have "stalled" batches in
	// AwaitingDownload state. Currently it will error with invalid state if
	// that happens. Sync manager must periodicatlly prune stalled batches like
	// we do for lookup sync. Then we can deprecate the redundant
	// `good_peers_on_sampling_subnets` checks.

[mergify]

This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏

[jimmygchen]

@dapplion @pawanjay176 as discussed, we need more testing on this PR, but would like to avoid having too many sync branches. Once this is reviewed, we can merge this to peerdas-devnet-6 branch for testing all the sync fixes together.

I've updated the base branch for this PR.

[Copilot]

Pull Request Overview

This PR refactors peer selection and request handling for both range and backfill syncs to support PeerDAS while improving error handling and load balancing. The changes include updates to peer management and removal in range sync, refactoring of batch state transitions and error handling, and modifications to the network context to select peers based on active request counts.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
beacon_node/network/src/sync/range_sync/range.rs	Removed the network argument from remove_peer callbacks.
beacon_node/network/src/sync/range_sync/chain.rs	Refactored peer removal, updated batch removal and download completion APIs.
beacon_node/network/src/sync/range_sync/batch.rs	Updated BatchInfo state variants and renamed current_peer to processing_peer.
beacon_node/network/src/sync/network_context/requests.rs	Added helper method to iterate over active request peers.
beacon_node/network/src/sync/network_context/custody.rs	Renamed error variant from NoPeers to NoPeer for consistency.
beacon_node/network/src/sync/network_context.rs	Modified peer selection logic for block components requests using active request counts.
beacon_node/network/src/sync/manager.rs	Updated backfill sync peer disconnect handling.
beacon_node/network/src/sync/backfill_sync/mod.rs	Refactored backfill sync to remove active request tracking and update retry logic.
beacon_node/lighthouse_network/src/types/globals.rs	Introduced a helper to verify custody peer membership.

Comments suppressed due to low confidence (1)

beacon_node/network/src/sync/backfill_sync/mod.rs:652

• [nitpick] Consider resolving the TODO regarding penalizing custody column peers to ensure all relevant peers are consistently handled during backfill sync.

// TODO(das): `participating_peers` only includes block peers. Should we penalize the custody column peers too?

[jimmygchen]

I still can't figure out why the tests are failing, seems like adding peers doesn't cause a new chain to be created.
I think we'll need this PR to prevent Lighthouse from requesting from peers that are either out of sync or not on the same chain.

[dapplion]

Fixed the test by requesting peers from the global peer pool, only synced peers. To request from the SyncingChain pool of peers we need more tweaks that we should do in another PR

[jimmygchen]

Changes look good to me.
I'll start testing this while waiting for Pawan's review.

[pawanjay176]

LGTM.
I like the direction of moving the peer selection to the network context. I have tested this in the mainnet context and found no issues.
I haven't tested this yet on a peerdas network yet.

Given that we still have a bunch of todos wrt the attribution, I think it might be a good idea to have a peerdas-syncing branch that we merge all sync improvements to and test extensively before merging it back to unstable.
The new branch is only meant as a testing branch for all sync improvements so we still review properly before merging into it.
cc @dapplion @jimmygchen what do you guys think?

[jimmygchen]

LGTM. I like the direction of moving the peer selection to the network context. I have tested this in the mainnet context and found no issues. I haven't tested this yet on a peerdas network yet.

Given that we still have a bunch of todos wrt the attribution, I think it might be a good idea to have a peerdas-syncing branch that we merge all sync improvements to and test extensively before merging it back to unstable. The new branch is only meant as a testing branch for all sync improvements so we still review properly before merging into it. cc @dapplion @jimmygchen what do you guys think?

Yeah sounds good to me. I can also confirm this works fine with mainnet. I synced a node from scratch and it completed backfill successfully.

I haven't been able to get it to work under PeerDAS though. I'll check the logs to confirm we haven't broke anything there.

Comments addressed

[jimmygchen]

I couldn't get sync to work on a local devnet, seems to be getting stuck due to metadata being acquired after peer is added to chain:

May 06 06:03:14.963 DEBUG Syncing new finalized chain                   id: 1, component: "range_sync"
May 06 06:03:14.963 DEBUG Waiting for peers to be available on sampling column subnets chain: 1service: "range_sync"
May 06 06:03:14.968 DEBUG Finalization sync peer joined                 peer_id: 16Uiu2HAmFWoqu6Y2EFTWzuZtdoupagnacQunLH2HoAkzT8ZuDi9y, component: "range_sync"
May 06 06:03:14.968 DEBUG Adding peer to known chain                    peer_id: 16Uiu2HAmFWoqu6Y2EFTWzuZtdoupagnacQunLH2HoAkzT8ZuDi9y, sync_type: Finalized, id: 1, component: "range_sync"
May 06 06:03:14.968 DEBUG Waiting for peers to be available on sampling column subnets chain: 1service: "range_sync"
May 06 06:03:14.968 DEBUG Waiting for peers to be available on sampling column subnets chain: 1service: "range_sync"
May 06 06:03:14.973 DEBUG Waiting for peers to be available on sampling column subnets chain: 1service: "range_sync"

I'm going to try revive #6975 and retest again.

[jimmygchen]

Confirm this works on peerdas devnet too with #6975. Merging this now.