Lighthouse sending excessive redundant data column by root requests to the same peer

[jimmygchen]

Prysm reported the the following issue with a Lighthouse peer on a LH-Prysm PeerDAS devnet:

• The peer requests several time the exact same column (root, index)
• Instead of sending only one by root request with multiple indices for the same root, the peer sends n by root requests with only one index
• At this time, the Prysm node was not even synced to the first slot of the retention window, so not to this perticular block. There is not chance for Prysm to be able to respond to this request.

It looks like there are multiple issues here:

• We're not sending request to a peer that has the block (lookup_peers). Instead, we're sending it to any custodial peer we can find from our global peer list:
  

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

  Lines 241 to 243 in d60c24e

  	// TODO(das): When is a fork and only a subset of your peers know about a block, we should
  	// only query the peers on that fork. Should this case be handled? How to handle it?
  	let custodial_peers = cx.get_custodial_peers(*column_index);

• Assuming we're sending the request to the right peer, if the peer doesn't respond back with the columns, we should add it to failed peers and penalise them. So the retry request should be sent to a different peer
• Not sure why the requests are not batched together. The following code suggested we do batching by peer_id, so i suspect there might be multiple lookups happening simultaneously:
  

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

  Lines 290 to 304 in d60c24e

  	for (peer_id, indices) in columns_to_request_by_peer.into_iter() {
  	let request_result = cx
  	.data_column_lookup_request(
  	DataColumnsByRootRequester::Custody(self.custody_id),
  	peer_id,
  	DataColumnsByRootSingleBlockRequest {
  	block_root: self.block_root,
  	indices: indices.clone(),
  	},
  	// If peer is in the lookup peer set, it claims to have imported the block and
  	// must have its columns in custody. In that case, set `true = enforce max_requests`
  	// and downscore if data_columns_by_root does not returned the expected custody
  	// columns. For the rest of peers, don't downscore if columns are missing.
  	lookup_peers.contains(&peer_id),
  	)

Logs

[2025-03-24 09:37:24] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[60]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:24] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[63]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:24] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[94]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:24] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[58]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:25] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[58]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:25] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[58]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:25] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[60]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
[2025-03-24 09:37:25] DEBUG sync: Serving data column sidecar by root request columns=map[0x0e2939f566245f788d2de7b10ded6d966e4f2b275dadbbe366e102d41df24975:[58]] peer=16Uiu2HAmH77XYainBfaerpEXAc1WfZ3YHBpE7aC9u7Q9BsZFPBM5
...

Discord message here: https://discord.com/channels/595666850260713488/1331931919260057602/1353650763712167958

[eserilev]

we try to prioritize peers that are in lookup_peers

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

Lines 250 to 265 in 80fe133

	let mut priorized_peers = custodial_peers
	.iter()
	.map(|peer| {
	(
	// Prioritize peers that claim to know have imported this block
	if lookup_peers.contains(peer) { 0 } else { 1 },
	// De-prioritize peers that have failed to successfully respond to
	// requests recently
	self.failed_peers.contains(peer),
	// Prefer peers with less requests to load balance across peers
	active_requests_by_peer.get(peer).copied().unwrap_or(0),
	// Final random factor to give all peers a shot in each retry
	rand::thread_rng().gen::<u32>(),
	*peer,
	)
	})

as part of this PR: #6815

However, if there are no matches in lookup_peers, we will fallback to peers that aren't claiming to have imported this block yet

[dapplion]

• We should revisit after completing Improve range sync with PeerDAS #6258

[jimmygchen]

@dapplion I believe #7352 would improve this by downscoring and better peer prioritisation right?