From 4215160bc19c00163336afe0783856e7898af376 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 1 Aug 2023 18:31:01 -0400 Subject: [PATCH] smol cleanup and a bugfix --- .../src/network_beacon_processor/tests.rs | 2 - .../network/src/sync/block_lookups/mod.rs | 83 +++++++++---------- 2 files changed, 38 insertions(+), 47 deletions(-) diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 1bdc3a8816..2c37d177aa 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -318,7 +318,6 @@ impl TestRig { } pub fn enqueue_single_lookup_rpc_blobs(&self) { if let Some(blobs) = self.next_blobs.clone() { - dbg!(blobs.len()); let blobs = FixedBlobSidecarList::from( blobs .into_iter() @@ -1004,7 +1003,6 @@ async fn test_rpc_block_reprocessing() { rig.enqueue_single_lookup_rpc_blobs(); if rig.next_blobs.as_ref().map(|b| b.len()).unwrap_or(0) > 0 { - dbg!("here"); rig.assert_event_journal(&[RPC_BLOBS, WORKER_FREED, NOTHING_TO_DO]) .await; } diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 43fb533018..4e6e5f987f 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -5,6 +5,7 @@ use super::BatchProcessResult; use super::{manager::BlockProcessType, network_context::SyncNetworkContext}; use crate::metrics; use crate::network_beacon_processor::ChainSegmentProcessId; +use crate::sync::block_lookups::common::LookupType; use crate::sync::block_lookups::parent_lookup::{ParentLookup, RequestError}; use crate::sync::block_lookups::single_block_lookup::{ CachedChild, LookupRequestError, LookupVerifyError, @@ -16,7 +17,6 @@ use beacon_chain::validator_monitor::timestamp_now; use beacon_chain::{AvailabilityProcessingStatus, BeaconChainTypes, BlockError}; pub use common::Current; pub use common::Lookup; -use common::LookupType; pub use common::Parent; pub use common::RequestState; use fnv::FnvHashMap; @@ -359,16 +359,10 @@ impl BlockLookups { /// necessary because we want to make sure all parents are processed before sending a child /// for processing, otherwise the block will fail validation and will be returned to the network /// layer with an `UnknownParent` error. - pub fn has_pending_parent_request(&self, target_id: Id) -> bool { - self.single_block_lookups.iter().any(|(id, lookup)| { - if *id == target_id { - self.parent_lookups - .iter() - .any(|parent_lookup| parent_lookup.chain_hash() == lookup.block_root()) - } else { - false - } - }) + pub fn has_pending_parent_request(&self, block_root: Hash256) -> bool { + self.parent_lookups + .iter() + .any(|parent_lookup| parent_lookup.chain_hash() == block_root) } /// Process a block or blob response received from a single lookup request. @@ -383,8 +377,6 @@ impl BlockLookups { let id = lookup_id.id; let response_type = R::response_type(); - let chain_hash_opt = self.has_pending_parent_request(id); - let Some(lookup) = self.get_single_lookup::(lookup_id) else { if response.is_some() { warn!( @@ -398,14 +390,8 @@ impl BlockLookups { let expected_block_root = lookup.block_root(); - match self.single_lookup_response_inner::( - peer_id, - response, - seen_timestamp, - cx, - chain_hash_opt, - lookup, - ) { + match self.single_lookup_response_inner::(peer_id, response, seen_timestamp, cx, lookup) + { Ok(lookup) => { self.single_block_lookups.insert(id, lookup); } @@ -432,7 +418,6 @@ impl BlockLookups { response: Option, seen_timestamp: Duration, cx: &SyncNetworkContext, - delay_send: bool, mut lookup: SingleBlockLookup, ) -> Result, LookupRequestError> { let response_type = R::response_type(); @@ -445,8 +430,7 @@ impl BlockLookups { self.handle_verified_response::( seen_timestamp, cx, - delay_send, - None, + BlockProcessType::SingleBlock { id: lookup.id }, verified_response, &mut lookup, )?; @@ -481,8 +465,7 @@ impl BlockLookups { &self, seen_timestamp: Duration, cx: &SyncNetworkContext, - delay_send: bool, - chain_hash: Option, + process_type: BlockProcessType, verified_response: R::VerifiedResponseType, lookup: &mut SingleBlockLookup, ) -> Result<(), LookupRequestError> { @@ -494,23 +477,24 @@ impl BlockLookups { .component_downloaded = true; let cached_child = lookup.add_response::(verified_response.clone()); - match cached_child { CachedChild::Ok(block) => { - if let Some(chain_hash) = chain_hash { - if !delay_send { - let process_type = match L::lookup_type() { - LookupType::Parent => BlockProcessType::ParentLookup { chain_hash }, - LookupType::Current => BlockProcessType::SingleBlock { id }, - }; - self.send_block_for_processing( - block_root, - block, - seen_timestamp, - process_type, - cx, - )? - } + // If we have an outstanding parent request for this block, delay sending the response until + // all parent blocks have been processed, otherwise we will fail validation with an + // `UnknownParent`. + let delay_send = match L::lookup_type() { + LookupType::Parent => false, + LookupType::Current => self.has_pending_parent_request(lookup.block_root()), + }; + + if !delay_send { + self.send_block_for_processing( + block_root, + block, + seen_timestamp, + process_type, + cx, + )? } } CachedChild::DownloadIncomplete => { @@ -628,8 +612,9 @@ impl BlockLookups { self.handle_verified_response::( seen_timestamp, cx, - false, - Some(parent_lookup.chain_hash()), + BlockProcessType::ParentLookup { + chain_hash: parent_lookup.chain_hash(), + }, verified_response, &mut parent_lookup.current_parent_request, )?; @@ -1181,7 +1166,11 @@ impl BlockLookups { "chain_hash" => ?chain_hash ); child_lookup.handle_consistency_failure(cx); - if child_lookup.request_block_and_blobs(cx).is_err() { + if let Err(e) = child_lookup.request_block_and_blobs(cx) { + debug!(self.log, + "Failed to request block and blobs, dropping lookup"; + "error" => ?e + ); self.single_block_lookups.remove(&child_lookup_id); } } @@ -1313,7 +1302,11 @@ impl BlockLookups { "error" => ?e ); lookup.handle_consistency_failure(cx); - if lookup.request_block_and_blobs(cx).is_err() { + if let Err(e) = lookup.request_block_and_blobs(cx) { + debug!(self.log, + "Failed to request block and blobs, droppingn lookup"; + "error" => ?e + ); self.single_block_lookups.remove(&id); } }