diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index 528a261bb8..5e39c9bc1e 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -962,7 +962,7 @@ impl NetworkBeaconProcessor { /// The classified outcome of submitting a block / blob / column for processing, ready for the /// lookup state machine to act on without re-inspecting `BlockError`. -#[derive(Debug, Clone)] +#[derive(Debug)] pub enum BlockProcessingResult { /// `fully_imported` is true if the lookup is complete; false if `MissingComponents` (the /// lookup must keep fetching). `info` is a stable label for logs / metrics. diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f8a43af9b8..e986d7a88e 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -195,9 +195,7 @@ impl BlockLookups { Some(AwaitingParent::new(parent_root, parent_block_hash)), // On a `UnknownParentBlock` or `UnknownParentSidecarHeader` event the peer is not // required to have the rest of the block components. Create the lookup with zero - // peers to house the block components. We don't know the child's fork yet, so use - // `Block` conservatively; the correct peer set is established when the child's - // block downloads and its FULL children begin attesting. + // peers to house the block components. &[], &PeerType::Block, cx, @@ -230,9 +228,6 @@ impl BlockLookups { pub fn search_parent_of_child( &mut self, block_root_to_search: Hash256, - // Classifies `peers` relative to the parent being searched: `GloasChild` when they imported - // the FULL child (and so can serve the parent's payload envelope and data columns), else - // `Block`. peer_type: &PeerType, child_block_root_trigger: Hash256, peers: &[PeerId], @@ -419,12 +414,9 @@ impl BlockLookups { self.metrics.created_lookups += 1; let result = lookup.continue_requests(cx); - if self.on_lookup_result(id, result, "new_current_lookup", cx) { - self.update_metrics(); - true - } else { - false - } + self.on_lookup_result(id, result, "new_current_lookup"); + self.update_metrics(); + self.single_block_lookups.contains_key(&id) } /* Lookup responses */ @@ -441,7 +433,7 @@ impl BlockLookups { return; }; let result = lookup.on_block_download_response(id.req_id, response, cx); - self.on_lookup_result(id.lookup_id, result, "block_download_response", cx); + self.on_lookup_result(id.lookup_id, result, "block_download_response"); } pub fn on_custody_download_response( @@ -455,7 +447,7 @@ impl BlockLookups { return; }; let result = lookup.on_custody_download_response(id.req_id, response, cx); - self.on_lookup_result(id.lookup_id, result, "custody_download_response", cx); + self.on_lookup_result(id.lookup_id, result, "custody_download_response"); } pub fn on_payload_download_response( @@ -472,7 +464,7 @@ impl BlockLookups { return; }; let result = lookup.on_payload_download_response(id.req_id, response, cx); - self.on_lookup_result(id.lookup_id, result, "payload_download_response", cx); + self.on_lookup_result(id.lookup_id, result, "payload_download_response"); } /* Error responses */ @@ -584,7 +576,7 @@ impl BlockLookups { BlockProcessingResult::Error { .. } => {} } - self.on_lookup_result(id, lookup_result, "processing_result", cx); + self.on_lookup_result(id, lookup_result, "processing_result"); } pub fn has_any_awaiting_children(&self, block_root: Hash256) -> bool { @@ -602,10 +594,6 @@ impl BlockLookups { let mut lookup_results = vec![]; // < need to buffer lookup results to not re-borrow &mut self for (id, lookup) in self.single_block_lookups.iter_mut() { - // If lookup is awaiting parent? - // - If Some - // - If parent_root lookup got block - // - Check if the child is FULL, if so keep waiting, otherwise continue and resolve if lookup.maybe_resolve_awaiting_parent(import_action) { debug!( ?import_action, @@ -619,7 +607,7 @@ impl BlockLookups { } for (id, result) in lookup_results { - self.on_lookup_result(id, result, "continue_child_lookups", cx); + self.on_lookup_result(id, result, "continue_child_lookups"); } } @@ -658,52 +646,13 @@ impl BlockLookups { id: SingleLookupId, result: Result<(), LookupRequestError>, source: &str, - cx: &mut SyncNetworkContext, - ) -> bool { + ) { match result { - Ok(_) => { - // The lookup may have become complete from already-cached components during - // `continue_requests` (e.g. the block became available via the da_checker), in - // which case no `Imported` processing result is emitted. Detect that here. - if self - .single_block_lookups - .get(&id) - .is_some_and(|lookup| lookup.is_complete()) - && let Some(lookup) = self.single_block_lookups.remove(&id) - { - let block_root = lookup.block_root(); - debug!(?block_root, id, "Dropping completed lookup (cached)"); - metrics::inc_counter(&metrics::SYNC_LOOKUP_COMPLETED); - self.metrics.completed_lookups += 1; - self.continue_child_lookups(ImportedAction::LookupComplete { block_root }, cx); - self.update_metrics(); - } - true - } - // If UnknownLookup do not log the request error. No need to drop child lookups nor - // update metrics because the lookup does not exist. + Ok(_) => {} Err(error) => { - // A FULL Gloas child re-awaits its parent's payload once the parent's block - // imports. A failed payload download must not cascade-drop the parent (and the - // child) — the payload may still arrive (e.g. via gossip). Retain the parent; - // genuinely stuck lookups are pruned by `drop_stuck_lookups`. - if source == "payload_download_response" - && let Some(block_root) = - self.single_block_lookups.get(&id).map(|l| l.block_root()) - && self.has_any_awaiting_children(block_root) - { - debug!( - id, - source, - ?error, - "Retaining parent with a child awaiting its payload" - ); - return false; - } debug!(id, source, ?error, "Dropping lookup on request error"); self.drop_lookup_and_children(id, error.into()); self.update_metrics(); - false } } } @@ -827,12 +776,12 @@ impl BlockLookups { lookup: &'a SingleBlockLookup, ) -> Result<&'a SingleBlockLookup, String> { if let Some(awaiting_parent) = lookup.awaiting_parent() { - if let Some(parent_lookup) = self + if let Some(lookup) = self .single_block_lookups .values() .find(|l| l.is_parent_of(awaiting_parent)) { - self.find_oldest_ancestor_lookup(parent_lookup) + self.find_oldest_ancestor_lookup(lookup) } else { Err(format!( "Lookup references unknown parent {awaiting_parent:?}" @@ -873,11 +822,10 @@ impl BlockLookups { if let Some(&awaiting_parent) = lookup.awaiting_parent() { // Regardless of gloas full/empty the lookup to add peers to is keyed by block_root - if let Some(parent_id) = self + if let Some((&parent_id, _)) = self .single_block_lookups .iter() .find(|(_, l)| l.is_parent_of(&awaiting_parent)) - .map(|(parent_id, _)| *parent_id) { self.add_peers_to_lookup_and_ancestors( parent_id, @@ -895,7 +843,7 @@ impl BlockLookups { // pruned with `drop_lookups_without_peers` because it has peers. This is rare corner // case, but it can result in stuck lookups. let result = lookup.continue_requests(cx); - self.on_lookup_result(lookup_id, result, "add_peers", cx); + self.on_lookup_result(lookup_id, result, "add_peers"); Ok(()) } else { Ok(()) diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 6eb9b7e147..7a90163852 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -852,8 +852,6 @@ impl SyncManager { SyncMessage::UnknownParentBlock(peer_id, block, block_root) => { let block_slot = block.slot(); let parent_root = block.parent_root(); - // Post-Gloas: the child's bid `parent_block_hash` lets the parent lookup partition - // peers and know it's FULL. let parent_block_hash = block.payload_bid_parent_block_hash().ok(); debug!(%block_root, %parent_root, "Received unknown parent block message"); self.handle_unknown_parent( @@ -880,8 +878,8 @@ impl SyncManager { peer_id, block_root, parent_root, - // No block downloaded yet, so the bid hash is unknown. The correct peer set is - // established once the child's block downloads. + // The event `UnknownParentSidecarHeader` only fires for pre-Gloas data + // structues, so the bid parent hash is None. None, slot, BlockComponent::Sidecar, diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 6ca20fdb6a..2c1195b491 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -17,7 +17,7 @@ use std::{ }; use types::{ AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, - SignedExecutionPayloadBid, Slot, + Slot, }; pub const DEFAULT_PRUNE_THRESHOLD: usize = 256; @@ -292,19 +292,6 @@ impl Block { } } } - - pub fn is_child_full(&self, child_bid: &SignedExecutionPayloadBid) -> bool { - if let Some(execution_payload_block_hash) = self.execution_payload_block_hash { - execution_payload_block_hash == child_bid.message.parent_block_hash - } else if let Some(execution_block_hash) = self.execution_status.block_hash() { - // Parent is before Gloas, and child is gloas - execution_block_hash == child_bid.message.parent_block_hash - } else { - // TODO(gloas): What to return here? The child is Gloas but parent doesn't have an - // execution hash - false - } - } } /// A Vec-wrapper which will grow to match any request. diff --git a/consensus/types/src/block/signed_beacon_block.rs b/consensus/types/src/block/signed_beacon_block.rs index 1ade0f82a3..1a87a519d0 100644 --- a/consensus/types/src/block/signed_beacon_block.rs +++ b/consensus/types/src/block/signed_beacon_block.rs @@ -361,14 +361,6 @@ impl> SignedBeaconBlock .unwrap_or(0) } - pub fn parent_block_hash(&self) -> Option { - self.message() - .body() - .signed_execution_payload_bid() - .ok() - .map(|bid| bid.message.parent_block_hash) - } - /// Used for displaying commitments in logs. pub fn commitments_formatted(&self) -> String { let Ok(commitments) = self.message().body().blob_kzg_commitments() else {