diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index da6ab2c06d..596873146b 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -1018,7 +1018,7 @@ impl BlockLookups { if let Some(awaiting) = lookup.awaiting_parent() { let parent_root = awaiting.parent_root(); - if let Some((&parent_id, parent_lookup)) = self + if let Some((&parent_id, _)) = self .single_block_lookups .iter() .find(|(_, l)| l.block_root() == parent_root) diff --git a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs index 302dcb18f7..cbbae310a8 100644 --- a/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/single_block_lookup.rs @@ -238,7 +238,6 @@ impl BlockRequest { #[derive(Debug)] struct DataRequest { peers: PeerSet, - slot: Slot, state: DataRequestState, } @@ -317,19 +316,9 @@ impl DataDownload { fn take_download_result(&mut self) -> Option<(DownloadedData, PeerGroup)> { match self { - DataDownload::Blobs { - expected_blobs, - state, - .. - } => state.take_download_result().map(|r| { - ( - DownloadedData::Blobs { - blobs: r.value, - expected_blobs: *expected_blobs, - }, - r.peer_group, - ) - }), + DataDownload::Blobs { state, .. } => state + .take_download_result() + .map(|r| (DownloadedData::Blobs(r.value), r.peer_group)), DataDownload::Columns { state, .. } => state .take_download_result() .map(|r| (DownloadedData::Columns(r.value), r.peer_group)), @@ -342,36 +331,15 @@ impl DataDownload { DataDownload::Columns { state, .. } => state.is_awaiting_event(), } } - - fn peek_downloaded_peer_group(&self) -> Option<&PeerGroup> { - match self { - DataDownload::Blobs { state, .. } => state.peek_downloaded_peer_group(), - DataDownload::Columns { state, .. } => state.peek_downloaded_peer_group(), - } - } } /// Downloaded data, waiting to be sent for processing #[derive(Debug)] enum DownloadedData { - Blobs { - blobs: FixedBlobSidecarList, - expected_blobs: usize, - }, + Blobs(FixedBlobSidecarList), Columns(DataColumnSidecarList), } -/// Enough info to reconstruct a fresh `DataDownload` when we need to retry data download -/// after a processing failure. We can't call `create_data_request` again from here because -/// we're past the `WaitingForBlock` state and don't have the `SyncNetworkContext` (and -/// therefore no `ChainSpec`) — so the request kind (blobs vs columns, plus the expected -/// blob count) is cached alongside the in-flight request instead. -#[derive(Debug, Clone, Copy)] -enum DataDownloadKind { - Blobs { expected_blobs: usize }, - Columns, -} - // === Payload request: WaitingForBlock → Downloading → Downloaded → Processing → Complete === #[derive(Debug)] @@ -501,7 +469,7 @@ pub struct SingleBlockLookup { peers: PeerSet, /// Peers for payload download (0 initially, Gloas only). #[educe(Debug(method(fmt_peer_map_as_len)))] - gload_child_peers: GloasChildPeers, + gloas_child_peers: GloasChildPeers, // Parent tracking awaiting_parent: Option, @@ -557,7 +525,7 @@ impl SingleBlockLookup { data_request: None, payload_request: None, peers: block_peers, - gload_child_peers: Arc::new(RwLock::new(gloas_child_peers)), + gloas_child_peers: Arc::new(RwLock::new(gloas_child_peers)), awaiting_parent, created: Instant::now(), failed_processing: 0, @@ -632,9 +600,14 @@ impl SingleBlockLookup { self.block_request.peer() } - /// Returns custody column peer group if data has been downloaded. Used for peer penalization. + /// Returns the peer group that served the downloaded data (blobs or custody columns) if + /// available, used for peer penalization on data-processing failures. pub fn data_peer_group(&self) -> Option<&PeerGroup> { - todo!(); + match &self.data_request.as_ref()?.state { + DataRequestState::Downloaded { peer_group, .. } + | DataRequestState::Processing { peer_group } => Some(peer_group), + DataRequestState::Downloading(_) | DataRequestState::Complete => None, + } } // -- Main state machine driver -- @@ -701,25 +674,24 @@ impl SingleBlockLookup { } let parent_root = block.parent_root(); - // Zero hash is the parent of the genesis block — not a real block. - if parent_root == Hash256::ZERO { - todo!(); - } - - let Some(parent_in_fork_choice) = cx - .chain - .canonical_head - .fork_choice_read_lock() - .get_block(&parent_root) - else { + // Zero hash is the parent of the genesis block — not a real block, so no + // parent-known check is needed. Fall through to send the block for processing. + if parent_root != Hash256::ZERO + && cx + .chain + .canonical_head + .fork_choice_read_lock() + .get_block(&parent_root) + .is_none() + { let awaiting_parent = AwaitingParent::from_block(block); - self.awaiting_parent = Some(awaiting_parent.clone()); + self.awaiting_parent = Some(awaiting_parent); return Ok(LookupResult::ParentUnknown { awaiting_parent, block_root: self.block_root, peers: self.all_peers(), }); - }; + } let block = block.clone(); let peer = *peer; @@ -765,7 +737,6 @@ impl SingleBlockLookup { .map_err(LookupRequestError::InternalError)?; self.data_request = Some(DataRequest { peers, - slot: block.slot(), state: DataRequestState::new( block.slot(), self.block_root, @@ -798,7 +769,7 @@ impl SingleBlockLookup { } DataRequestState::Downloaded { data, peer_group } => { match data { - DownloadedData::Blobs { blobs, .. } => { + DownloadedData::Blobs(blobs) => { cx.send_blobs_for_processing( id, self.block_root, @@ -935,7 +906,7 @@ impl SingleBlockLookup { let Some(execution_hash) = execution_hash else { return Err("execution_hash is None post gloas".to_string()); }; - self.gload_child_peers + self.gloas_child_peers .write() .entry(execution_hash) .or_default() @@ -1139,16 +1110,15 @@ impl SingleBlockLookup { match peer_type { PeerType::PostGloas(execution_hash) => { // This peer claims to have imported a child of this block with parent_hash. We - // can't known if the child is full or empty until we know the payload hash of this - // lookup - added - != self - .gload_child_peers - .write() - .entry(*execution_hash) - .or_default() - .write() - .insert(peer_id); + // can't know whether the child is full or empty until we know the payload hash of + // this lookup. + added |= self + .gloas_child_peers + .write() + .entry(*execution_hash) + .or_default() + .write() + .insert(peer_id); } PeerType::PreGloas => {} } @@ -1161,7 +1131,7 @@ impl SingleBlockLookup { /// Remove peer from available peers. pub fn remove_peer(&mut self, peer_id: &PeerId) { self.peers.write().remove(peer_id); - for set in self.gload_child_peers.write().values_mut() { + for set in self.gloas_child_peers.write().values_mut() { set.write().remove(peer_id); } } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 326e2e89ad..74aabeacb7 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -955,6 +955,14 @@ impl SyncNetworkContext { lookup_peers: Arc>>, block_root: Hash256, ) -> Result { + // Skip the download if fork-choice already saw this envelope (e.g. imported via gossip + // before the lookup got here). + if self.chain.envelope_is_known_to_fork_choice(&block_root) { + return Ok(LookupRequestResult::NoRequestNeeded( + "envelope already known to fork-choice", + )); + } + let active_request_count_by_peer = self.active_request_count_by_peer(); let Some(peer_id) = lookup_peers .read() diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index 8333d7a239..8c6034db90 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1522,7 +1522,12 @@ impl TestRig { } fn trigger_unknown_parent_blob(&mut self, peer_id: PeerId, blob: Arc>) { - self.send_sync_message(SyncMessage::UnknownParentBlob(peer_id, blob)); + self.send_sync_message(SyncMessage::UnknownParentSidecarHeader { + peer_id, + block_root: blob.block_root(), + parent_root: blob.block_parent_root(), + slot: blob.slot(), + }); } fn trigger_unknown_parent_column( @@ -1530,7 +1535,17 @@ impl TestRig { peer_id: PeerId, column: Arc>, ) { - self.send_sync_message(SyncMessage::UnknownParentDataColumn(peer_id, column)); + let DataColumnSidecar::Fulu(col) = column.as_ref() else { + panic!( + "trigger_unknown_parent_column is Fulu-only; Gloas columns use the partial-column path" + ); + }; + self.send_sync_message(SyncMessage::UnknownParentSidecarHeader { + peer_id, + block_root: col.block_root(), + parent_root: col.block_parent_root(), + slot: col.slot(), + }); } fn trigger_unknown_block_from_attestation(&mut self, block_root: Hash256, peer_id: PeerId) {