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/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 d7d3c9a061f..1dfe4310324 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; @@ -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,16 +57,14 @@ 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 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 +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 && - // 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 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. @@ -103,10 +109,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, @@ -148,18 +150,20 @@ 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, + 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 }, ) @@ -200,22 +204,20 @@ 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, + 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 99724837034..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(), } } } @@ -106,8 +107,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 +133,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 +188,7 @@ impl BlockLookups { "chain_too_long", ); } - self.drop_lookup_and_childs(*lookup_id); + self.drop_lookup_and_children(*lookup_id); } } @@ -290,7 +291,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(); } } @@ -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) } @@ -394,7 +395,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 +498,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 +532,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 +563,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 +582,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) } 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..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, @@ -208,7 +217,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 +289,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 +377,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 +391,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/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index ff02a19a554..699f581eebe 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" @@ -1174,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())); @@ -1238,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::{ @@ -1393,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 { 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) => {