From b39b5de2076dcbdae5410710ebfeee501fac8e38 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 21:23:37 -0400 Subject: [PATCH 1/8] fix compile after merge --- beacon_node/network/src/sync/block_lookups/common.rs | 2 +- .../network/src/sync/block_lookups/single_block_lookup.rs | 4 ++-- beacon_node/network/src/sync/manager.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 8a78a10ddc8..d7d3c9a061f 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -180,7 +180,7 @@ impl RequestState for BlockRequestState { } } -impl RequestState for BlobRequestState { +impl RequestState for BlobRequestState { type RequestType = BlobsByRootSingleBlockRequest; type VerifiedResponseType = FixedBlobSidecarList; 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 830a08e1a71..9349e24c69c 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 @@ -179,12 +179,12 @@ impl SingleBlockLookup { } /// The state of the blob request component of a `SingleBlockLookup`. -pub struct BlobRequestState { +pub struct BlobRequestState { pub block_root: Hash256, pub state: SingleLookupRequestState>, } -impl BlobRequestState { +impl BlobRequestState { pub fn new(block_root: Hash256, peer_source: &[PeerId]) -> Self { Self { block_root, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index bf097e4c25d..f7c9dd783c8 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -853,7 +853,7 @@ impl SyncManager { ) { if let Some(resp) = self.network.on_single_blob_response(id, blob) { self.block_lookups - .on_download_response::( + .on_download_response::>( id.lookup_id, peer_id, resp, From 171be11026acba431e9c757f5b7c5c5eaa33f9b6 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 22:51:49 -0400 Subject: [PATCH 2/8] remove todos, fix typos etc --- beacon_node/beacon_chain/src/beacon_chain.rs | 3 -- .../network/src/sync/block_lookups/common.rs | 9 ++---- .../network/src/sync/block_lookups/mod.rs | 32 +++++++------------ .../src/sync/block_lookups/parent_chain.rs | 14 ++++---- 4 files changed, 21 insertions(+), 37 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 746aa371ffe..9c7ded313b6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2898,9 +2898,6 @@ impl BeaconChain { .fork_choice_read_lock() .contains_block(&block_root) { - // TODO: Should also check for: - // - Parent block is known - // - Slot is not in the future return Err(BlockError::BlockIsAlreadyKnown(block_root)); } diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index d7d3c9a061f..d54f5a6c53b 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -55,8 +55,6 @@ pub trait RequestState { // Attempt to progress awaiting downloads if self.get_state().is_awaiting_download() { // Verify the current request has not exceeded the maximum number of attempts. - // TODO: Okay to use `SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS` for both current and parent - // lookups now? It not trivial to identify what is a "parent lookup" now. let request_state = self.get_state(); if request_state.failed_attempts() >= SINGLE_BLOCK_LOOKUP_MAX_ATTEMPTS { let cannot_process = request_state.more_failed_processing_attempts(); @@ -76,11 +74,8 @@ pub trait RequestState { // Otherwise, attempt to progress awaiting processing // If this request is awaiting a parent lookup to be processed, do not send for processing. // The request will be rejected with unknown parent error. - } else if !awaiting_parent && - // TODO: Blob processing / import does not check for unknown parent. As a temporary fix - // and to emulate the behaviour before this PR, hold blobs for processing until the - // block has been processed i.e. it has a known parent. - (block_is_processed || matches!(Self::response_type(), ResponseType::Block)) + } else if !awaiting_parent + && (block_is_processed || matches!(Self::response_type(), ResponseType::Block)) { // maybe_start_processing returns Some if state == AwaitingProcess. This pattern is // useful to conditionally access the result data. diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 99724837034..545c1053c53 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -106,8 +106,8 @@ impl BlockLookups { /* Lookup requests */ - /// Creates a lookup for the block with the given `block_root` and immediately triggers it. - /// Returns true if the lookup is created or already exists + /// Creates a parent lookup for the block with the given `block_root` and immediately triggers it. + /// If a parent lookup exists or is triggered, a current lookup will be created. pub fn search_child_and_parent( &mut self, block_root: Hash256, @@ -132,7 +132,7 @@ impl BlockLookups { } } - /// Seach a block that we don't known its parent root. + /// Seach a block whose parent root is unknown. /// Returns true if the lookup is created or already exists pub fn search_unknown_block( &mut self, @@ -187,7 +187,7 @@ impl BlockLookups { "chain_too_long", ); } - self.drop_lookup_and_childs(*lookup_id); + self.drop_lookup_and_children(*lookup_id); } } @@ -290,7 +290,7 @@ impl BlockLookups { ) { if let Err(e) = self.on_download_response_inner::(id, peer_id, response, cx) { debug!(self.log, "Dropping single lookup"; "id" => id, "err" => ?e); - self.drop_lookup_and_childs(id); + self.drop_lookup_and_children(id); self.update_metrics(); } } @@ -394,7 +394,7 @@ impl BlockLookups { BlockProcessType::SingleBlock { id } | BlockProcessType::SingleBlob { id } => id, }; debug!(self.log, "Dropping lookup on request error"; "component" => process_type.component(), "id" => process_type.id(), "error" => ?e); - self.drop_lookup_and_childs(id); + self.drop_lookup_and_children(id); self.update_metrics(); } } @@ -497,21 +497,11 @@ impl BlockLookups { { // There errors indicate internal problems and should not downscore the peer warn!(self.log, "Internal availability check failure"; "block_root" => %block_root, "error" => ?e); - // TODO: This lines represent an improper transition of download states, - // which can log errors in the future. If an error here causes the request - // to transition into a bad state, a future network message will cause - // the request to be dropped - // - // lookup.block_request_state.state.on_download_failure(); - // lookup.blob_request_state.state.on_download_failure(); Action::Drop } other => { debug!(self.log, "Invalid lookup component"; "block_root" => %block_root, "component" => ?R::response_type(), "error" => ?other); let peer_id = request_state.on_processing_failure()?; - // TODO: Why is the original code downscoring the block peer regardless of - // type of request? Sending a blob for verification can result in an error - // attributable to the block peer? cx.report_peer( peer_id, PeerAction::MidToleranceError, @@ -541,7 +531,7 @@ impl BlockLookups { } Action::Drop => { // Drop with noop - self.drop_lookup_and_childs(lookup_id); + self.drop_lookup_and_children(lookup_id); self.update_metrics(); } Action::Continue => { @@ -572,14 +562,14 @@ impl BlockLookups { } for id in failed_lookups { - self.drop_lookup_and_childs(id); + self.drop_lookup_and_children(id); } } /// Drops `dropped_id` lookup and all its children recursively. Lookups awaiting a parent need - /// the parent to make progress to resolve, therefore we must drop them is the parent is + /// the parent to make progress to resolve, therefore we must drop them if the parent is /// dropped. - pub fn drop_lookup_and_childs(&mut self, dropped_id: SingleLookupId) { + pub fn drop_lookup_and_children(&mut self, dropped_id: SingleLookupId) { if let Some(dropped_lookup) = self.single_block_lookups.remove(&dropped_id) { debug!(self.log, "Dropping child lookup"; "id" => ?dropped_id, "block_root" => %dropped_lookup.block_root()); @@ -591,7 +581,7 @@ impl BlockLookups { .collect::>(); for id in child_lookups { - self.drop_lookup_and_childs(id); + self.drop_lookup_and_children(id); } } } diff --git a/beacon_node/network/src/sync/block_lookups/parent_chain.rs b/beacon_node/network/src/sync/block_lookups/parent_chain.rs index 0571e6d0dbb..01a39a69713 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_chain.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_chain.rs @@ -51,23 +51,23 @@ pub(crate) fn compute_parent_chains(nodes: &[Node]) -> Vec { let mut parent_chains = vec![]; - // Iterate blocks which no child + // Iterate blocks with no children for tip in nodes { let mut block_root = tip.block_root; if parent_to_child.get(&block_root).is_none() { let mut chain = vec![]; // Resolve chain of blocks - loop { + 'inner: loop { if let Some(parent_root) = child_to_parent.get(&block_root) { // block_root is a known block that may or may not have a parent root chain.push(block_root); if let Some(parent_root) = parent_root { block_root = *parent_root; - continue; + continue 'inner; } } - break; + break 'inner; } if chain.len() > 1 { @@ -100,7 +100,9 @@ pub(crate) fn find_oldest_fork_ancestor( } // Should never happen - let parent_chain = parent_chains.get(chain_idx).ok_or("chain_idx off bounds")?; + let parent_chain = parent_chains + .get(chain_idx) + .ok_or("chain_idx out of bounds")?; // Find the first block in the target parent chain that is not in other parent chains // Iterate in ascending slot order for block in parent_chain.chain.iter().rev() { @@ -109,7 +111,7 @@ pub(crate) fn find_oldest_fork_ancestor( } } - // If no match means that the chain is fully contained within another chain. This should never + // No match means that the chain is fully contained within another chain. This should never // happen, but if that was the case just return the tip Ok(parent_chain.tip) } From f4d7a3d09c306ebe6ef57c8df36f7f8ffdc0eda7 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:03:59 -0400 Subject: [PATCH 3/8] fix compile --- .../src/network_beacon_processor/tests.rs | 4 +--- .../network/src/sync/block_lookups/common.rs | 18 +----------------- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index dd58eb83555..4ba4c4ddd1d 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -311,9 +311,7 @@ impl TestRig { block_root, RpcBlock::new_without_blobs(Some(block_root), self.next_block.clone()), std::time::Duration::default(), - BlockProcessType::ParentLookup { - chain_hash: Hash256::random(), - }, + BlockProcessType::SingleBlock { id: 0 }, ) .unwrap(); } diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index d54f5a6c53b..8370764091f 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -12,7 +12,7 @@ use beacon_chain::block_verification_types::RpcBlock; use beacon_chain::BeaconChainTypes; use std::sync::Arc; use types::blob_sidecar::FixedBlobSidecarList; -use types::{Hash256, SignedBeaconBlock}; +use types::SignedBeaconBlock; use super::single_block_lookup::DownloadResult; use super::SingleLookupId; @@ -98,10 +98,6 @@ pub trait RequestState { /* Response handling methods */ - /// A getter for the parent root of the response. Returns an `Option` because we won't know - /// the blob parent if we don't end up getting any blobs in the response. - fn get_parent_root(verified_response: &Self::VerifiedResponseType) -> Option; - /// Send the response to the beacon processor. fn send_for_processing( id: Id, @@ -143,10 +139,6 @@ impl RequestState for BlockRequestState { .map_err(LookupRequestError::SendFailed) } - fn get_parent_root(verified_response: &Arc>) -> Option { - Some(verified_response.parent_root()) - } - fn send_for_processing( id: SingleLookupId, (block, block_root, seen_timestamp, _): DownloadResult, @@ -195,14 +187,6 @@ impl RequestState for BlobRequestState { .map_err(LookupRequestError::SendFailed) } - fn get_parent_root(verified_response: &FixedBlobSidecarList) -> Option { - verified_response - .into_iter() - .filter_map(|blob| blob.as_ref()) - .map(|blob| blob.block_parent_root()) - .next() - } - fn send_for_processing( id: Id, (verified, block_root, seen_timestamp, _): DownloadResult, From 1bbb78ed5c99a4bfce35eba6ef4c18523ae50e9a Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:14:24 -0400 Subject: [PATCH 4/8] stable rng --- beacon_node/network/src/sync/block_lookups/tests.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index ff02a19a554..335c2a0f66d 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -703,11 +703,10 @@ impl TestRig { fn stable_rng() { let mut rng = XorShiftRng::from_seed([42; 16]); let (block, _) = generate_rand_block_and_blobs::(ForkName::Base, NumBlobs::None, &mut rng); - // TODO: Make rand block generation stable - assert_ne!( + assert_eq!( block.canonical_root(), Hash256::from_slice( - &hex::decode("9cfcfc321759d8a2c38d6541a966da5e88fe8729ed5a5ab37013781ff097b0d6") + &hex::decode("adfd2e9e7a7976e8ccaed6eaf0257ed36a5b476732fee63ff44966602fd099ec") .unwrap() ), "rng produces a consistent value" From b0b66e8def3b831f57b72db5f199297d8c8db423 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:18:53 -0400 Subject: [PATCH 5/8] delete TODO and unfilled out test --- beacon_node/network/src/sync/block_lookups/tests.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 335c2a0f66d..1d1b1cce088 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1237,13 +1237,6 @@ fn test_same_chain_race_condition() { rig.expect_no_active_lookups(); } -#[test] -fn test_penalize_wrong_peer_with_cached_child() { - // peer A sends blob with malicious data as unknown parent - // peer B serves parent and rest of blocks - // All components are sent as RpcBlock, penalizing peer B -} - mod deneb_only { use super::*; use beacon_chain::{ @@ -1392,7 +1385,6 @@ mod deneb_only { self } - // TODO: Eventually deprecate this function fn set_block_id_for_import(mut self) -> Self { let lookup_id = self.rig.find_single_lookup_for(self.block_root); self.block_req_id = Some(SingleLookupReqId { From 1af1894be4640c709c9703ad04b70cc423a29fa1 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:29:38 -0400 Subject: [PATCH 6/8] make download result a struct --- .../network/src/sync/block_lookups/common.rs | 20 +++++++++++++++---- .../network/src/sync/block_lookups/mod.rs | 13 ++++++------ .../sync/block_lookups/single_block_lookup.rs | 16 ++++++++++----- beacon_node/network/src/sync/manager.rs | 19 ++++++++++++------ 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 8370764091f..48436dbd4ae 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -141,12 +141,18 @@ impl RequestState for BlockRequestState { fn send_for_processing( id: SingleLookupId, - (block, block_root, seen_timestamp, _): DownloadResult, + download_result: DownloadResult, cx: &SyncNetworkContext, ) -> Result<(), LookupRequestError> { + let DownloadResult { + value, + block_root, + seen_timestamp, + peer_id: _, + } = download_result; cx.send_block_for_processing( block_root, - RpcBlock::new_without_blobs(Some(block_root), block), + RpcBlock::new_without_blobs(Some(block_root), value), seen_timestamp, BlockProcessType::SingleBlock { id }, ) @@ -189,12 +195,18 @@ impl RequestState for BlobRequestState { fn send_for_processing( id: Id, - (verified, block_root, seen_timestamp, _): DownloadResult, + download_result: DownloadResult, cx: &SyncNetworkContext, ) -> Result<(), LookupRequestError> { + let DownloadResult { + value, + block_root, + seen_timestamp, + peer_id: _, + } = download_result; cx.send_blobs_for_processing( block_root, - verified, + value, seen_timestamp, BlockProcessType::SingleBlob { id }, ) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 545c1053c53..3549b938d6c 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -1,5 +1,6 @@ use self::parent_chain::{compute_parent_chains, NodeChain}; -use self::single_block_lookup::{DownloadResult, LookupRequestError, SingleBlockLookup}; +pub use self::single_block_lookup::DownloadResult; +use self::single_block_lookup::{LookupRequestError, SingleBlockLookup}; use super::manager::{BlockProcessType, BlockProcessingResult}; use super::network_context::{RpcProcessingResult, SyncNetworkContext}; use crate::metrics; @@ -38,8 +39,8 @@ pub enum BlockComponent { impl BlockComponent { fn parent_root(&self) -> Hash256 { match self { - BlockComponent::Block(block) => block.0.parent_root(), - BlockComponent::Blob(blob) => blob.0.block_parent_root(), + BlockComponent::Block(block) => block.value.parent_root(), + BlockComponent::Blob(blob) => blob.value.block_parent_root(), } } } @@ -333,12 +334,12 @@ impl BlockLookups { // Register the download peer here. Once we have received some data over the wire we // attribute it to this peer for scoring latter regardless of how the request was // done. - request_state.on_download_success(( - response, + request_state.on_download_success(DownloadResult { + value: response, block_root, seen_timestamp, peer_id, - ))?; + })?; // continue_request will send for processing as the request state is AwaitingProcessing lookup.continue_request::(cx) } 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 9349e24c69c..9ef88a45038 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 @@ -208,7 +208,13 @@ impl BlockRequestState { } } -pub type DownloadResult = (T, Hash256, Duration, PeerId); +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct DownloadResult { + pub value: T, + pub block_root: Hash256, + pub seen_timestamp: Duration, + pub peer_id: PeerId, +} #[derive(Debug, PartialEq, Eq)] pub enum State { @@ -274,8 +280,8 @@ impl SingleLookupRequestState { match &self.state { State::AwaitingDownload => None, State::Downloading { .. } => None, - State::AwaitingProcess(result) => Some(&result.0), - State::Processing(result) => Some(&result.0), + State::AwaitingProcess(result) => Some(&result.value), + State::Processing(result) => Some(&result.value), State::Processed { .. } => None, } } @@ -362,7 +368,7 @@ impl SingleLookupRequestState { pub fn on_processing_failure(&mut self) -> Result { match &self.state { State::Processing(result) => { - let peer_id = result.3; + let peer_id = result.peer_id; self.failed_processing = self.failed_processing.saturating_add(1); self.state = State::AwaitingDownload; Ok(peer_id) @@ -376,7 +382,7 @@ impl SingleLookupRequestState { pub fn on_processing_success(&mut self) -> Result { match &self.state { State::Processing(result) => { - let peer_id = result.3; + let peer_id = result.peer_id; self.state = State::Processed(peer_id); Ok(peer_id) } diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index f7c9dd783c8..7d512733c6c 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -41,7 +41,9 @@ use super::range_sync::{RangeSync, RangeSyncType, EPOCHS_PER_BATCH}; use crate::network_beacon_processor::{ChainSegmentProcessId, NetworkBeaconProcessor}; use crate::service::NetworkMessage; use crate::status::ToStatusMessage; -use crate::sync::block_lookups::{BlobRequestState, BlockComponent, BlockRequestState}; +use crate::sync::block_lookups::{ + BlobRequestState, BlockComponent, BlockRequestState, DownloadResult, +}; use crate::sync::block_sidecar_coupling::BlocksAndBlobsRequestInfo; use beacon_chain::block_verification_types::AsBlock; use beacon_chain::block_verification_types::RpcBlock; @@ -593,12 +595,12 @@ impl SyncManager { block_root, parent_root, block_slot, - BlockComponent::Block(( - block.block_cloned(), + BlockComponent::Block(DownloadResult { + value: block.block_cloned(), block_root, - timestamp_now(), + seen_timestamp: timestamp_now(), peer_id, - )), + }), ); } SyncMessage::UnknownParentBlob(peer_id, blob) => { @@ -611,7 +613,12 @@ impl SyncManager { block_root, parent_root, blob_slot, - BlockComponent::Blob((blob, block_root, timestamp_now(), peer_id)), + BlockComponent::Blob(DownloadResult { + value: blob, + block_root, + seen_timestamp: timestamp_now(), + peer_id, + }), ); } SyncMessage::UnknownBlockHashFromAttestation(peer_id, block_root) => { From 107d514d41f1272f1edc91b52b1fee736dce0def Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:35:03 -0400 Subject: [PATCH 7/8] enums instead of bools as params --- .../network/src/sync/block_lookups/common.rs | 19 +++++++++++++++---- .../sync/block_lookups/single_block_lookup.rs | 13 +++++++++++-- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/beacon_node/network/src/sync/block_lookups/common.rs b/beacon_node/network/src/sync/block_lookups/common.rs index 48436dbd4ae..1dfe4310324 100644 --- a/beacon_node/network/src/sync/block_lookups/common.rs +++ b/beacon_node/network/src/sync/block_lookups/common.rs @@ -28,6 +28,16 @@ pub enum ResponseType { /// is further back than the most recent head slot. pub(crate) const PARENT_DEPTH_TOLERANCE: usize = SLOT_IMPORT_TOLERANCE * 2; +pub enum AwaitingParent { + True, + False, +} + +pub enum BlockIsProcessed { + True, + False, +} + /// This trait unifies common single block lookup functionality across blocks and blobs. This /// includes making requests, verifying responses, and handling processing results. A /// `SingleBlockLookup` includes both a `BlockRequestState` and a `BlobRequestState`, this trait is @@ -47,9 +57,9 @@ pub trait RequestState { fn continue_request( &mut self, id: Id, - awaiting_parent: bool, + awaiting_parent: AwaitingParent, downloaded_block_expected_blobs: Option, - block_is_processed: bool, + block_is_processed: BlockIsProcessed, cx: &mut SyncNetworkContext, ) -> Result<(), LookupRequestError> { // Attempt to progress awaiting downloads @@ -74,8 +84,9 @@ pub trait RequestState { // Otherwise, attempt to progress awaiting processing // If this request is awaiting a parent lookup to be processed, do not send for processing. // The request will be rejected with unknown parent error. - } else if !awaiting_parent - && (block_is_processed || matches!(Self::response_type(), ResponseType::Block)) + } else if matches!(awaiting_parent, AwaitingParent::False) + && (matches!(block_is_processed, BlockIsProcessed::True) + || matches!(Self::response_type(), ResponseType::Block)) { // maybe_start_processing returns Some if state == AwaitingProcess. This pattern is // useful to conditionally access the result data. 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 9ef88a45038..19b3f5326d0 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 @@ -1,3 +1,4 @@ +use super::common::{AwaitingParent, BlockIsProcessed}; use super::{BlockComponent, PeerId}; use crate::sync::block_lookups::common::RequestState; use crate::sync::block_lookups::Id; @@ -125,13 +126,21 @@ impl SingleBlockLookup { cx: &mut SyncNetworkContext, ) -> Result<(), LookupRequestError> { let id = self.id; - let awaiting_parent = self.awaiting_parent.is_some(); + let awaiting_parent = if self.awaiting_parent.is_some() { + AwaitingParent::True + } else { + AwaitingParent::False + }; let downloaded_block_expected_blobs = self .block_request_state .state .peek_downloaded_data() .map(|block| block.num_expected_blobs()); - let block_is_processed = self.block_request_state.state.is_processed(); + let block_is_processed = if self.block_request_state.state.is_processed() { + BlockIsProcessed::True + } else { + BlockIsProcessed::False + }; R::request_state_mut(self).continue_request( id, awaiting_parent, From 6a9bc1d16bc52cc56b079a6db547562a3e3edf41 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 29 Apr 2024 23:41:06 -0400 Subject: [PATCH 8/8] fix comment --- beacon_node/network/src/sync/block_lookups/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 1d1b1cce088..699f581eebe 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1173,7 +1173,7 @@ fn test_parent_lookup_ignored_response() { rig.trigger_unknown_parent_block(peer_id, block.clone().into()); let id = rig.expect_parent_request_block_and_blobs(parent_root); // Note: single block lookup for current `block` does not trigger any request because it does - // not has blobs, and the block is already cached + // not have blobs, and the block is already cached // Peer sends the right block, it should be sent for processing. Peer should not be penalized. rig.parent_lookup_block_response(id, peer_id, Some(parent.into()));