From ce7df7be4b0caa20e0114ba5cfc47fa6bf5cc358 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 9 Jun 2026 23:05:11 +0200 Subject: [PATCH] Review PR --- .../network/src/sync/block_lookups/mod.rs | 17 +++-- .../sync/block_lookups/single_block_lookup.rs | 75 ++++++++----------- .../network/src/sync/network_context.rs | 10 +-- 3 files changed, 45 insertions(+), 57 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index f522adf1ba..d403382e9e 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -371,7 +371,7 @@ impl BlockLookups { && !self .single_block_lookups .iter() - .any(|(_, lookup)| lookup.is_for_block(awaiting_parent.parent_root())) + .any(|(_, lookup)| lookup.block_root() == awaiting_parent.parent_root()) { warn!(block_root = ?awaiting_parent, "Ignoring child lookup parent lookup not found"); return false; @@ -523,9 +523,9 @@ impl BlockLookups { cx, ); } - // Then if this lookup had only empty children, and no children now, we can remove - // it. We must make sure that no other lookup is awaiting this one, and that no - // requests are on-going. + // Then if this lookup happens to have only empty children we can remove it now. We + // must make sure that no other lookup is awaiting this one, and that no requests + // are on-going. if !lookup_is_awaiting_event && !self.has_any_awaiting_children(block_root) { Ok(LookupResult::Completed) } else { @@ -558,7 +558,8 @@ 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.maybe_resolve_awaiting_parent(parent_root, imported_parent) { + if lookup.is_awaiting_parent(parent_root, imported_parent) { + lookup.resolve_awaiting_parent(); debug!( ?imported_parent, id, @@ -786,7 +787,7 @@ impl BlockLookups { if let Some(lookup) = self .single_block_lookups .values() - .find(|l| l.is_parent_of(awaiting_parent)) + .find(|l| l.block_root() == awaiting_parent.parent_root()) { self.find_oldest_ancestor_lookup(lookup) } else { @@ -832,12 +833,12 @@ impl BlockLookups { if let Some((&parent_id, _)) = self .single_block_lookups .iter() - .find(|(_, l)| l.is_parent_of(&awaiting_parent)) + .find(|(_, l)| l.block_root() == awaiting_parent.parent_root()) { self.add_peers_to_lookup_and_ancestors( parent_id, peers, - &(&awaiting_parent).into(), + &awaiting_parent.into_peer_type(), cx, ) } else { 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 20fbe55010..58713eeec4 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 @@ -74,16 +74,17 @@ impl AwaitingParent { parent_block_hash, } } + pub fn parent_root(&self) -> Hash256 { self.parent_root } + + pub fn into_peer_type(self) -> PeerType { + PeerType::new(self.parent_block_hash) + } } type PeerSet = Arc>>; -/// Peers that claim to have imported a FULL child of this lookup's block, keyed by the child's bid -/// `parent_block_hash` (which equals this block's bid `block_hash` when the child is FULL). Only -/// such peers are proven to hold this block's execution payload envelope and its data columns. -type GloasChildPeers = Arc>>; #[derive(Debug)] struct BlockRequest { @@ -173,12 +174,6 @@ impl PeerType { } } -impl From<&AwaitingParent> for PeerType { - fn from(value: &AwaitingParent) -> Self { - Self::new(value.parent_block_hash) - } -} - #[derive(Debug, Clone, Copy)] pub enum ImportedParent { LookupComplete, @@ -203,7 +198,7 @@ pub struct SingleBlockLookup { /// child's bid `parent_block_hash`. These (not `peers`) are the peers proven to hold this /// block's payload envelope and data columns. #[educe(Debug(method(fmt_peer_map_as_len)))] - gloas_child_peers: GloasChildPeers, + gloas_child_peers: HashMap, awaiting_parent: Option, created: Instant, pub(crate) span: Span, @@ -239,7 +234,7 @@ impl SingleBlockLookup { data_request: DataRequest::WaitingForBlock, payload_request: PayloadRequest::WaitingForBlock, peers: block_peers, - gloas_child_peers: Arc::new(RwLock::new(gloas_child_peers)), + gloas_child_peers, awaiting_parent, created: Instant::now(), span: lookup_span, @@ -277,10 +272,6 @@ impl SingleBlockLookup { self.awaiting_parent.as_ref() } - pub fn is_parent_of(&self, child_awaiting_parent: &AwaitingParent) -> bool { - self.block_root == child_awaiting_parent.parent_root - } - pub fn is_awaiting_block(&self, block_root: Hash256) -> bool { if let Some(awaiting_parent) = &self.awaiting_parent { awaiting_parent.parent_root() == block_root @@ -297,7 +288,13 @@ impl SingleBlockLookup { /// Mark this lookup as no longer awaiting a parent lookup. Components can be sent for /// processing. - pub fn maybe_resolve_awaiting_parent( + pub fn resolve_awaiting_parent(&mut self) { + self.awaiting_parent = None; + } + + /// Check if this lookup awaiting_parent status can be resolved given that `parent_root` and + /// `imported_parent` have just been imported + pub fn is_awaiting_parent( &mut self, parent_root: Hash256, imported_parent: ImportedParent, @@ -308,7 +305,7 @@ impl SingleBlockLookup { if awaiting_parent.parent_root() != parent_root { return false; } - let should_resolve = match imported_parent { + match imported_parent { ImportedParent::LookupComplete => true, ImportedParent::OnlyGloasBlock(bid_block_hash) => { if let Some(parent_block_hash) = awaiting_parent.parent_block_hash { @@ -323,11 +320,7 @@ impl SingleBlockLookup { false } } - }; - if should_resolve { - self.awaiting_parent = None; } - should_resolve } /// Returns the time elapsed since this lookup was created @@ -402,7 +395,7 @@ impl SingleBlockLookup { } else if cx.chain.should_fetch_custody_columns(block_epoch) { DataRequest::Request { slot: block.slot(), - peers: self.get_data_peers(block), + peers: self.get_data_peers(block.payload_bid_block_hash().ok()), state: SingleLookupRequestState::new(), } } else { @@ -443,7 +436,7 @@ impl SingleBlockLookup { if let Some(block) = self.block_request.state.peek_downloaded_data() { self.payload_request = if block.fork_name_unchecked().gloas_enabled() { PayloadRequest::Request { - peers: self.get_data_peers(block), + peers: self.get_data_peers(block.payload_bid_block_hash().ok()), state: SingleLookupRequestState::new(), } } else { @@ -494,18 +487,17 @@ impl SingleBlockLookup { /// Gloas blocks these are the peers that claimed to have imported a FULL child of this block /// (keyed by this block's bid `block_hash`). Pre-Gloas blocks carry no bid, so this returns the /// lookup's `peers` unchanged. - fn get_data_peers(&self, block: &SignedBeaconBlock) -> PeerSet { - match block.payload_bid_block_hash() { + fn get_data_peers(&mut self, bid_block_hash: Option) -> PeerSet { + if let Some(bid_block_hash) = bid_block_hash { // Gloas: the child-attested peer set for this bid is the canonical peer set. DO NOT // default to `self.peers`: post-Gloas `self.peers` have not claimed to import this // block's data nor its payload. This set may remain empty until a FULL child arrives. - Ok(block_hash) => self - .gloas_child_peers - .write() - .entry(block_hash) + self.gloas_child_peers + .entry(bid_block_hash) .or_default() - .clone(), - Err(_) => self.peers.clone(), + .clone() + } else { + self.peers.clone() } } @@ -670,7 +662,6 @@ impl SingleBlockLookup { // payload envelope and data columns. added |= self .gloas_child_peers - .write() .entry(*execution_hash) .or_default() .write() @@ -686,7 +677,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.gloas_child_peers.read().values() { + for set in self.gloas_child_peers.values() { set.write().remove(peer_id); } } @@ -695,16 +686,18 @@ impl SingleBlockLookup { pub fn has_no_peers(&self) -> bool { if self.block_request.is_complete() && let Some(block) = self.block_request.state.peek_downloaded_data() - && block.fork_name_unchecked().gloas_enabled() + && let Ok(bid_block_hash) = block.payload_bid_block_hash() { // Gloas block request complete, the main peer set is irrelevant. Check only the gloas // child peers - self.get_data_peers(block).read().is_empty() + match self.gloas_child_peers.get(&bid_block_hash) { + Some(set) => set.read().is_empty(), + None => false, + } } else { self.peers.read().is_empty() && self .gloas_child_peers - .read() .values() .all(|set| set.read().is_empty()) } @@ -1020,13 +1013,9 @@ fn fmt_peer_set_as_len( } fn fmt_peer_map_as_len( - peer_map: &GloasChildPeers, + peer_map: &HashMap, f: &mut std::fmt::Formatter, ) -> Result<(), std::fmt::Error> { - let total = peer_map - .read() - .values() - .map(|set| set.read().len()) - .sum::(); + let total = peer_map.values().map(|set| set.read().len()).sum::(); write!(f, "{}", total) } diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index e3741c51b1..c9a48c9d5e 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -1061,12 +1061,10 @@ impl SyncNetworkContext { block_slot: Slot, lookup_peers: Arc>>, ) -> Result>, RpcRequestSendError> { - if self - .spec() - .fork_name_at_slot::(block_slot) - .gloas_enabled() - && lookup_peers.read().is_empty() - { + // Code below will issue column requests even if `lookup_peers` is empty. This is not okay, + // as we want to have at least one signal that some of our peers has already seen the + // block's data. + if lookup_peers.read().is_empty() { return Ok(LookupRequestResult::Pending("no peers")); }