diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 5326336e7c..5de8006d0b 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -509,7 +509,9 @@ impl TryInto> for MaybeAvailableBlock { fn try_into(self) -> Result, Self::Error> { match self { Self::Available(block) => Ok(block), - Self::AvailabilityPending(_) => Err(AvailabilityCheckError::MissingBlobs), + Self::AvailabilityPending(block) => Err(AvailabilityCheckError::MissingBlobs( + block.as_block().canonical_root(), + )), } } } diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index efddc393bc..dd573f5cc9 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -13,6 +13,7 @@ use state_processing::per_block_processing::deneb::deneb::verify_kzg_commitments use std::collections::hash_map::{Entry, OccupiedEntry}; use std::collections::HashMap; use std::sync::Arc; +use strum::IntoStaticStr; use types::beacon_block_body::KzgCommitments; use types::blob_sidecar::{BlobIdentifier, BlobSidecar, FixedBlobSidecarList}; use types::consts::deneb::MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS; @@ -21,13 +22,13 @@ use types::{ SignedBeaconBlock, SignedBeaconBlockHeader, Slot, }; -#[derive(Debug)] +#[derive(Debug, IntoStaticStr)] pub enum AvailabilityCheckError { Kzg(KzgError), KzgVerificationFailed, KzgNotInitialized, SszTypes(ssz_types::Error), - MissingBlobs, + MissingBlobs(Hash256), NumBlobsMismatch { num_kzg_commitments: usize, num_blobs: usize, @@ -205,7 +206,7 @@ impl DataAvailabilityChecker { } if blob_count < expected_num_blobs { - return Err(AvailabilityCheckError::MissingBlobs); + return Err(AvailabilityCheckError::MissingBlobs(block_root)); } BlockWrapper::BlockAndBlobs(block, blobs) @@ -346,6 +347,7 @@ impl DataAvailabilityChecker { ) -> Result, AvailabilityCheckError> { if occupied_entry.get().has_all_blobs(&executed_block) { let num_blobs_expected = executed_block.num_blobs_expected(); + let block_root = executed_block.import_data.block_root; let AvailabilityPendingExecutedBlock { block, import_data, @@ -360,7 +362,9 @@ impl DataAvailabilityChecker { let verified_blobs = Vec::from(verified_blobs) .into_iter() .take(num_blobs_expected) - .map(|maybe_blob| maybe_blob.ok_or(AvailabilityCheckError::MissingBlobs)) + .map(|maybe_blob| { + maybe_blob.ok_or(AvailabilityCheckError::MissingBlobs(block_root)) + }) .collect::, _>>()?; let available_block = self.make_available(block, verified_blobs)?; diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 1c8c9ab9c7..c03a4243e9 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -26,7 +26,6 @@ use super::{ }; use crate::beacon_processor::{ChainSegmentProcessId, WorkEvent}; use crate::metrics; -use crate::sync::block_lookups::single_block_lookup::LookupVerifyError; mod parent_lookup; mod single_block_lookup; @@ -315,9 +314,9 @@ impl BlockLookups { let should_remove = match request_ref.verify_block(block) { Ok(Some((root, block))) => { if triggered_parent_request { - // The lookup status here is irrelevant because we wait until the parent chain - // is complete before processing the block. - if let Err(e) = request_ref.add_block(root, block) { + if let LookupDownloadStatus::AvailabilityCheck(e) = + request_ref.add_block(root, block) + { handle_block_lookup_verify_error( request_id_ref, request_ref, @@ -387,9 +386,9 @@ impl BlockLookups { let should_remove = match request_ref.verify_blob(blob) { Ok(Some((block_root, blobs))) => { if triggered_parent_request { - // The lookup status here is irrelevant because we wait until the parent chain - // is complete before processing the block. - if let Err(e) = request_ref.add_blobs(block_root, blobs) { + if let LookupDownloadStatus::AvailabilityCheck(e) = + request_ref.add_blobs(block_root, blobs) + { handle_block_lookup_verify_error( request_id_ref, request_ref, @@ -514,9 +513,9 @@ impl BlockLookups { match parent_lookup.verify_block(block, &mut self.failed_chains) { Ok(Some((block_root, block))) => { - let process_or_search = parent_lookup.add_block(block_root, block); //TODO(sean) fix + let process_or_search = parent_lookup.add_block(block_root, block); match process_or_search { - Ok(LookupDownloadStatus::Process(wrapper)) => { + LookupDownloadStatus::Process(wrapper) => { let chain_hash = parent_lookup.chain_hash(); if self .send_block_for_processing( @@ -531,7 +530,7 @@ impl BlockLookups { self.parent_lookups.push(parent_lookup) } } - Ok(LookupDownloadStatus::SearchBlock(block_root)) => { + LookupDownloadStatus::SearchBlock(block_root) => { if let Some(peer_source) = parent_lookup.peer_source(ResponseType::Block, peer_id) { @@ -541,7 +540,7 @@ impl BlockLookups { warn!(self.log, "Response from untracked peer"; "peer_id" => %peer_id, "block_root" => ?block_root); } } - Err(e) => {} + LookupDownloadStatus::AvailabilityCheck(e) => {} } } Ok(None) => { @@ -555,9 +554,7 @@ impl BlockLookups { | ParentVerifyError::ExtraBlocksReturned | ParentVerifyError::UnrequestedBlobId | ParentVerifyError::ExtraBlobsReturned - | ParentVerifyError::InvalidIndex(_) - //TODO(sean) treat this differntly? - | ParentVerifyError::AvailabilityCheck(_) => { + | ParentVerifyError::InvalidIndex(_) => { let e = e.into(); warn!(self.log, "Peer sent invalid response to parent request."; "peer_id" => %peer_id, "reason" => %e); @@ -616,7 +613,7 @@ impl BlockLookups { match parent_lookup.verify_blob(blob, &mut self.failed_chains) { Ok(Some((block_root, blobs))) => { - let processed_or_search = parent_lookup.add_blobs(block_root, blobs).unwrap(); //TODO(sean) fix + let processed_or_search = parent_lookup.add_blobs(block_root, blobs); match processed_or_search { LookupDownloadStatus::Process(wrapper) => { let chain_hash = parent_lookup.chain_hash(); @@ -643,6 +640,7 @@ impl BlockLookups { warn!(self.log, "Response from untracked peer"; "peer_id" => %peer_id, "block_root" => ?block_root); } } + LookupDownloadStatus::AvailabilityCheck(e) => {} } } Ok(None) => { @@ -656,9 +654,7 @@ impl BlockLookups { | ParentVerifyError::ExtraBlocksReturned | ParentVerifyError::UnrequestedBlobId | ParentVerifyError::ExtraBlobsReturned - | ParentVerifyError::InvalidIndex(_) - //TODO(sean) treat differently? - | ParentVerifyError::AvailabilityCheck(_) => { + | ParentVerifyError::InvalidIndex(_) => { let e = e.into(); warn!(self.log, "Peer sent invalid response to parent request."; "peer_id" => %peer_id, "reason" => %e); @@ -1328,16 +1324,16 @@ impl BlockLookups { } } -fn handle_block_lookup_verify_error( +fn handle_block_lookup_verify_error>( request_id_ref: &mut u32, request_ref: &mut SingleBlockLookup, response_type: ResponseType, peer_id: PeerId, - error: LookupVerifyError, + e: Err, cx: &mut SyncNetworkContext, log: &Logger, ) -> ShouldRemoveLookup { - let msg: &str = error.into(); + let msg = e.into(); cx.report_peer(peer_id, PeerAction::LowToleranceError, msg); debug!(log, "Single block lookup failed"; diff --git a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs index b65f36e588..d133f78f92 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_lookup.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_lookup.rs @@ -7,7 +7,7 @@ use crate::sync::{ }; use beacon_chain::blob_verification::AsBlock; use beacon_chain::blob_verification::BlockWrapper; -use beacon_chain::data_availability_checker::DataAvailabilityChecker; +use beacon_chain::data_availability_checker::{AvailabilityCheckError, DataAvailabilityChecker}; use beacon_chain::BeaconChainTypes; use lighthouse_network::PeerId; use std::sync::Arc; @@ -45,7 +45,6 @@ pub enum ParentVerifyError { ExtraBlobsReturned, InvalidIndex(u64), PreviousFailure { parent_root: Hash256 }, - AvailabilityCheck(String), } #[derive(Debug, PartialEq, Eq)] @@ -64,6 +63,19 @@ pub enum RequestError { pub enum LookupDownloadStatus { Process(BlockWrapper), SearchBlock(Hash256), + AvailabilityCheck(AvailabilityCheckError), +} + +impl From, AvailabilityCheckError>> for LookupDownloadStatus { + fn from(value: Result, AvailabilityCheckError>) -> Self { + match value { + Ok(wrapper) => LookupDownloadStatus::Process(wrapper), + Err(AvailabilityCheckError::MissingBlobs(block_root)) => { + LookupDownloadStatus::SearchBlock(block_root) + } + Err(e) => LookupDownloadStatus::AvailabilityCheck(e), + } + } } impl ParentLookup { @@ -172,22 +184,18 @@ impl ParentLookup { &mut self, block_root: Hash256, block: Arc>, - ) -> Result, ParentVerifyError> { + ) -> LookupDownloadStatus { self.current_parent_request_id = None; - self.current_parent_request - .add_block(block_root, block) - .map_err(Into::into) + self.current_parent_request.add_block(block_root, block) } pub fn add_blobs( &mut self, block_root: Hash256, blobs: FixedBlobSidecarList, - ) -> Result, ParentVerifyError> { + ) -> LookupDownloadStatus { self.current_parent_blob_request_id = None; - self.current_parent_request - .add_blobs(block_root, blobs) - .map_err(Into::into) + self.current_parent_request.add_blobs(block_root, blobs) } pub fn pending_block_response(&self, req_id: Id) -> bool { @@ -361,7 +369,6 @@ impl From for ParentVerifyError { E::UnrequestedBlobId => ParentVerifyError::UnrequestedBlobId, E::ExtraBlobsReturned => ParentVerifyError::ExtraBlobsReturned, E::InvalidIndex(index) => ParentVerifyError::InvalidIndex(index), - E::AvailabilityCheck(e) => ParentVerifyError::AvailabilityCheck(e), } } } 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 60f38f7ee4..65e96e179a 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 @@ -109,21 +109,15 @@ impl SingleBlockLookup Ok(LookupDownloadStatus::Process(wrapper)), - Err(AvailabilityCheckError::MissingBlobs) => { - Ok(LookupDownloadStatus::SearchBlock(block_root)) - } - Err(e) => Err(LookupVerifyError::AvailabilityCheck(format!("{e:?}"))), - } + ); + Ok(LookupDownloadStatus::from(result)) } else { Ok(LookupDownloadStatus::SearchBlock(block_root)) } @@ -136,24 +130,19 @@ impl SingleBlockLookup, - ) -> Result, LookupVerifyError> { + ) -> LookupDownloadStatus { for (index, blob_opt) in self.downloaded_blobs.iter_mut().enumerate() { if let Some(Some(downloaded_blob)) = blobs.get(index) { - //TODO(sean) should we log a warn if there is already a downloaded blob? *blob_opt = Some(downloaded_blob.clone()); } } if let Some(block) = self.downloaded_block.as_ref() { - match self.da_checker.wrap_block(block_root, block.clone(), blobs) { - Ok(wrapper) => Ok(LookupDownloadStatus::Process(wrapper)), - Err(AvailabilityCheckError::MissingBlobs) => { - Ok(LookupDownloadStatus::SearchBlock(block_root)) - } - Err(e) => Err(LookupVerifyError::AvailabilityCheck(format!("{e:?}"))), - } + self.da_checker + .wrap_block(block_root, block.clone(), blobs) + .into() } else { - Ok(LookupDownloadStatus::SearchBlock(block_root)) + LookupDownloadStatus::SearchBlock(block_root) } } @@ -161,31 +150,22 @@ impl SingleBlockLookup>, - ) -> Result, LookupVerifyError> { - //TODO(sean) check for existing block? + ) -> LookupDownloadStatus { self.downloaded_block = Some(block.clone()); - match self - .da_checker + self.da_checker .wrap_block(block_root, block, self.downloaded_blobs.clone()) - { - Ok(wrapper) => Ok(LookupDownloadStatus::Process(wrapper)), - Err(AvailabilityCheckError::MissingBlobs) => { - Ok(LookupDownloadStatus::SearchBlock(block_root)) - } - Err(e) => Err(LookupVerifyError::AvailabilityCheck(format!("{e:?}"))), - } + .into() } pub fn add_block_wrapper( &mut self, block_root: Hash256, block: BlockWrapper, - ) -> Result, LookupVerifyError> { + ) -> LookupDownloadStatus { match block { BlockWrapper::Block(block) => self.add_block(block_root, block), BlockWrapper::BlockAndBlobs(block, blobs) => { - //TODO(sean) check for existing block? self.downloaded_block = Some(block); self.add_blobs(block_root, blobs) }