From d8dbda319dbe9b8eab9d5fbd9a877b62a409a551 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 5 Nov 2024 09:39:58 +0200 Subject: [PATCH] Resolve some PeerDAS todos (#6434) * Resolve some PeerDAS todos --- beacon_node/network/src/sync/manager.rs | 12 ++---------- beacon_node/network/src/sync/network_context.rs | 2 -- beacon_node/network/src/sync/peer_sampling.rs | 15 ++++++++------- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 882f199b52..344e91711c 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -1188,22 +1188,14 @@ impl SyncManager { } fn on_sampling_result(&mut self, requester: SamplingRequester, result: SamplingResult) { - // TODO(das): How is a consumer of sampling results? - // - Fork-choice for trailing DA - // - Single lookups to complete import requirements - // - Range sync to complete import requirements? Can sampling for syncing lag behind and - // accumulate in fork-choice? - match requester { SamplingRequester::ImportedBlock(block_root) => { debug!(self.log, "Sampling result"; "block_root" => %block_root, "result" => ?result); - // TODO(das): Consider moving SamplingResult to the beacon_chain crate and import - // here. No need to add too much enum variants, just whatever the beacon_chain or - // fork-choice needs to make a decision. Currently the fork-choice only needs to - // be notified of successful samplings, i.e. sampling failures don't trigger pruning match result { Ok(_) => { + // Notify the fork-choice of a successful sampling result to mark the block + // branch as safe. if let Err(e) = self .network .beacon_processor() diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 5f7778ffcc..c4d987e858 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -769,7 +769,6 @@ impl SyncNetworkContext { self.log.clone(), ); - // TODO(das): start request // Note that you can only send, but not handle a response here match request.continue_requests(self) { Ok(_) => { @@ -779,7 +778,6 @@ impl SyncNetworkContext { self.custody_by_root_requests.insert(requester, request); Ok(LookupRequestResult::RequestSent(req_id)) } - // TODO(das): handle this error properly Err(e) => Err(RpcRequestSendError::CustodyRequestError(e)), } } diff --git a/beacon_node/network/src/sync/peer_sampling.rs b/beacon_node/network/src/sync/peer_sampling.rs index 7e725f5df5..289ed73cdd 100644 --- a/beacon_node/network/src/sync/peer_sampling.rs +++ b/beacon_node/network/src/sync/peer_sampling.rs @@ -24,7 +24,6 @@ pub type SamplingResult = Result<(), SamplingError>; type DataColumnSidecarList = Vec>>; pub struct Sampling { - // TODO(das): stalled sampling request are never cleaned up requests: HashMap>, sampling_config: SamplingConfig, log: slog::Logger, @@ -313,8 +312,8 @@ impl ActiveSamplingRequest { .iter() .position(|data| &data.index == column_index) else { - // Peer does not have the requested data. - // TODO(das) what to do? + // Peer does not have the requested data, mark peer as "dont have" and try + // again with a different peer. debug!(self.log, "Sampling peer claims to not have the data"; "block_root" => %self.block_root, @@ -373,7 +372,9 @@ impl ActiveSamplingRequest { sampling_request_id, }, ) { - // TODO(das): Beacon processor is overloaded, what should we do? + // Beacon processor is overloaded, drop sampling attempt. Failing to sample + // is not a permanent state so we should recover once the node has capacity + // and receives a descendant block. error!(self.log, "Dropping sampling"; "block" => %self.block_root, @@ -391,8 +392,8 @@ impl ActiveSamplingRequest { ); metrics::inc_counter_vec(&metrics::SAMPLE_DOWNLOAD_RESULT, &[metrics::FAILURE]); - // Error downloading, maybe penalize peer and retry again. - // TODO(das) with different peer or different peer? + // Error downloading, malicious network errors are already penalized before + // reaching this function. Mark the peer as failed and try again with another. for column_index in column_indexes { let Some(request) = self.column_requests.get_mut(column_index) else { warn!(self.log, @@ -453,7 +454,7 @@ impl ActiveSamplingRequest { debug!(self.log, "Sample verification failure"; "block_root" => %self.block_root, "column_indexes" => ?column_indexes, "reason" => ?err); metrics::inc_counter_vec(&metrics::SAMPLE_VERIFY_RESULT, &[metrics::FAILURE]); - // TODO(das): Peer sent invalid data, penalize and try again from different peer + // Peer sent invalid data, penalize and try again from different peer // TODO(das): Count individual failures for column_index in column_indexes { let Some(request) = self.column_requests.get_mut(column_index) else {