diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1eda0ce0333..fa9a0c2e697 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -206,7 +206,7 @@ impl TryInto for AvailabilityProcessingStatus { } /// The result of a chain segment processing. -pub enum ChainSegmentResult { +pub enum ChainSegmentResult { /// Processing this chain segment finished successfully. Successful { imported_blocks: Vec<(Hash256, Slot)>, @@ -215,7 +215,7 @@ pub enum ChainSegmentResult { /// have been imported. Failed { imported_blocks: Vec<(Hash256, Slot)>, - error: BlockError, + error: BlockError, }, } @@ -2159,7 +2159,7 @@ impl BeaconChain { self: &Arc, blob_sidecar: Arc>, subnet_id: u64, - ) -> Result, GossipBlobError> { + ) -> Result, GossipBlobError> { metrics::inc_counter(&metrics::BLOBS_SIDECAR_PROCESSING_REQUESTS); let _timer = metrics::start_timer(&metrics::BLOBS_SIDECAR_GOSSIP_VERIFICATION_TIMES); GossipVerifiedBlob::new(blob_sidecar, subnet_id, self).map(|v| { @@ -2698,7 +2698,7 @@ impl BeaconChain { pub fn filter_chain_segment( self: &Arc, chain_segment: Vec>, - ) -> Result>, ChainSegmentResult> { + ) -> Result>, ChainSegmentResult> { // This function will never import any blocks. let imported_blocks = vec![]; let mut filtered_chain_segment = Vec::with_capacity(chain_segment.len()); @@ -2805,7 +2805,7 @@ impl BeaconChain { self: &Arc, chain_segment: Vec>, notify_execution_layer: NotifyExecutionLayer, - ) -> ChainSegmentResult { + ) -> ChainSegmentResult { let mut imported_blocks = vec![]; // Filter uninteresting blocks from the chain segment in a blocking task. @@ -2938,7 +2938,7 @@ impl BeaconChain { pub async fn verify_block_for_gossip( self: &Arc, block: Arc>, - ) -> Result, BlockError> { + ) -> Result, BlockError> { let chain = self.clone(); self.task_executor .clone() @@ -2986,7 +2986,7 @@ impl BeaconChain { pub async fn process_gossip_blob( self: &Arc, blob: GossipVerifiedBlob, - ) -> Result> { + ) -> Result { let block_root = blob.block_root(); // If this block has already been imported to forkchoice it must have been available, so @@ -3026,7 +3026,7 @@ impl BeaconChain { AvailabilityProcessingStatus, DataColumnsToPublish, ), - BlockError, + BlockError, > { let Ok((slot, block_root)) = data_columns .iter() @@ -3062,7 +3062,7 @@ impl BeaconChain { slot: Slot, block_root: Hash256, blobs: FixedBlobSidecarList, - ) -> Result> { + ) -> Result { // If this block has already been imported to forkchoice it must have been available, so // we don't need to process its blobs again. if self @@ -3099,7 +3099,7 @@ impl BeaconChain { AvailabilityProcessingStatus, DataColumnsToPublish, ), - BlockError, + BlockError, > { let Ok((slot, block_root)) = custody_columns .iter() @@ -3133,8 +3133,8 @@ impl BeaconChain { fn remove_notified( &self, block_root: &Hash256, - r: Result>, - ) -> Result> { + r: Result, + ) -> Result { let has_missing_components = matches!(r, Ok(AvailabilityProcessingStatus::MissingComponents(_, _))); if !has_missing_components { @@ -3148,8 +3148,8 @@ impl BeaconChain { fn remove_notified_custody_columns

( &self, block_root: &Hash256, - r: Result<(AvailabilityProcessingStatus, P), BlockError>, - ) -> Result<(AvailabilityProcessingStatus, P), BlockError> { + r: Result<(AvailabilityProcessingStatus, P), BlockError>, + ) -> Result<(AvailabilityProcessingStatus, P), BlockError> { let has_missing_components = matches!( r, Ok((AvailabilityProcessingStatus::MissingComponents(_, _), _)) @@ -3168,7 +3168,7 @@ impl BeaconChain { unverified_block: B, block_source: BlockImportSource, notify_execution_layer: NotifyExecutionLayer, - ) -> Result> { + ) -> Result { self.reqresp_pre_import_cache .write() .insert(block_root, unverified_block.block_cloned()); @@ -3204,8 +3204,8 @@ impl BeaconChain { unverified_block: B, notify_execution_layer: NotifyExecutionLayer, block_source: BlockImportSource, - publish_fn: impl FnOnce() -> Result<(), BlockError> + Send + 'static, - ) -> Result> { + publish_fn: impl FnOnce() -> Result<(), BlockError> + Send + 'static, + ) -> Result { // Start the Prometheus timer. let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES); @@ -3326,7 +3326,7 @@ impl BeaconChain { pub async fn into_executed_block( self: Arc, execution_pending_block: ExecutionPendingBlock, - ) -> Result, BlockError> { + ) -> Result, BlockError> { let ExecutionPendingBlock { block, import_data, @@ -3381,7 +3381,7 @@ impl BeaconChain { async fn check_block_availability_and_import( self: &Arc, block: AvailabilityPendingExecutedBlock, - ) -> Result> { + ) -> Result { let slot = block.block.slot(); let availability = self .data_availability_checker @@ -3394,7 +3394,7 @@ impl BeaconChain { async fn check_gossip_blob_availability_and_import( self: &Arc, blob: GossipVerifiedBlob, - ) -> Result> { + ) -> Result { let slot = blob.slot(); if let Some(slasher) = self.slasher.as_ref() { slasher.accept_block_header(blob.signed_block_header()); @@ -3416,7 +3416,7 @@ impl BeaconChain { AvailabilityProcessingStatus, DataColumnsToPublish, ), - BlockError, + BlockError, > { if let Some(slasher) = self.slasher.as_ref() { for data_colum in &data_columns { @@ -3440,7 +3440,7 @@ impl BeaconChain { slot: Slot, block_root: Hash256, blobs: FixedBlobSidecarList, - ) -> Result> { + ) -> Result { // Need to scope this to ensure the lock is dropped before calling `process_availability` // Even an explicit drop is not enough to convince the borrow checker. { @@ -3450,7 +3450,7 @@ impl BeaconChain { .filter_map(|b| b.as_ref().map(|b| b.signed_block_header.clone())) .unique() { - if verify_header_signature::>(self, &header).is_ok() { + if verify_header_signature::(self, &header).is_ok() { slashable_cache .observe_slashable( header.message.slot, @@ -3484,7 +3484,7 @@ impl BeaconChain { AvailabilityProcessingStatus, DataColumnsToPublish, ), - BlockError, + BlockError, > { // Need to scope this to ensure the lock is dropped before calling `process_availability` // Even an explicit drop is not enough to convince the borrow checker. @@ -3493,7 +3493,7 @@ impl BeaconChain { // Assumes all items in custody_columns are for the same block_root if let Some(column) = custody_columns.first() { let header = &column.signed_block_header; - if verify_header_signature::>(self, header).is_ok() { + if verify_header_signature::(self, header).is_ok() { slashable_cache .observe_slashable( header.message.slot, @@ -3530,7 +3530,7 @@ impl BeaconChain { self: &Arc, slot: Slot, availability: Availability, - ) -> Result> { + ) -> Result { match availability { Availability::Available(block) => { // Block is fully available, import into fork choice @@ -3545,7 +3545,7 @@ impl BeaconChain { pub async fn import_available_block( self: &Arc, block: Box>, - ) -> Result> { + ) -> Result { let AvailableExecutedBlock { block, import_data, @@ -3624,7 +3624,7 @@ impl BeaconChain { parent_block: SignedBlindedBeaconBlock, parent_eth1_finalization_data: Eth1FinalizationData, mut consensus_context: ConsensusContext, - ) -> Result> { + ) -> Result { // ----------------------------- BLOCK NOT YET ATTESTABLE ---------------------------------- // Everything in this initial section is on the hot path between processing the block and // being able to attest to it. DO NOT add any extra processing in this initial section @@ -3934,7 +3934,7 @@ impl BeaconChain { block: BeaconBlockRef, block_root: Hash256, state: &BeaconState, - ) -> Result<(), BlockError> { + ) -> Result<(), BlockError> { // Only perform the weak subjectivity check if it was configured. let Some(wss_checkpoint) = self.config.weak_subjectivity_checkpoint else { return Ok(()); @@ -4269,7 +4269,7 @@ impl BeaconChain { &self, block_root: Hash256, state: &mut BeaconState, - ) -> Result<(), BlockError> { + ) -> Result<(), BlockError> { for relative_epoch in [RelativeEpoch::Current, RelativeEpoch::Next] { let shuffling_id = AttestationShufflingId::new(block_root, state, relative_epoch)?; @@ -7042,8 +7042,8 @@ impl From for Error { } } -impl ChainSegmentResult { - pub fn into_block_error(self) -> Result<(), BlockError> { +impl ChainSegmentResult { + pub fn into_block_error(self) -> Result<(), BlockError> { match self { ChainSegmentResult::Failed { error, .. } => Err(error), ChainSegmentResult::Successful { .. } => Ok(()), diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 99fc5d9d0c0..e4646d62882 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -22,7 +22,7 @@ use types::{ /// An error occurred while validating a gossip blob. #[derive(Debug)] -pub enum GossipBlobError { +pub enum GossipBlobError { /// The blob sidecar is from a slot that is later than the current slot (with respect to the /// gossip clock disparity). /// @@ -95,7 +95,7 @@ pub enum GossipBlobError { /// ## Peer scoring /// /// We cannot process the blob without validating its parent, the peer isn't necessarily faulty. - BlobParentUnknown(Arc>), + BlobParentUnknown { parent_root: Hash256 }, /// Invalid kzg commitment inclusion proof /// ## Peer scoring @@ -145,28 +145,19 @@ pub enum GossipBlobError { NotFinalizedDescendant { block_parent_root: Hash256 }, } -impl std::fmt::Display for GossipBlobError { +impl std::fmt::Display for GossipBlobError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - GossipBlobError::BlobParentUnknown(blob_sidecar) => { - write!( - f, - "BlobParentUnknown(parent_root:{})", - blob_sidecar.block_parent_root() - ) - } - other => write!(f, "{:?}", other), - } + write!(f, "{:?}", self) } } -impl From for GossipBlobError { +impl From for GossipBlobError { fn from(e: BeaconChainError) -> Self { GossipBlobError::BeaconChainError(e) } } -impl From for GossipBlobError { +impl From for GossipBlobError { fn from(e: BeaconStateError) -> Self { GossipBlobError::BeaconChainError(BeaconChainError::BeaconStateError(e)) } @@ -190,12 +181,12 @@ impl GossipVerifiedBlob { blob: Arc>, subnet_id: u64, chain: &BeaconChain, - ) -> Result> { + ) -> Result { let header = blob.signed_block_header.clone(); // We only process slashing info if the gossip verification failed // since we do not process the blob any further in that case. validate_blob_sidecar_for_gossip(blob, subnet_id, chain).map_err(|e| { - process_block_slash_info::<_, GossipBlobError>( + process_block_slash_info::<_, GossipBlobError>( chain, BlockSlashInfo::from_early_error_blob(header, e), ) @@ -384,7 +375,7 @@ pub fn validate_blob_sidecar_for_gossip( blob_sidecar: Arc>, subnet: u64, chain: &BeaconChain, -) -> Result, GossipBlobError> { +) -> Result, GossipBlobError> { let blob_slot = blob_sidecar.slot(); let blob_index = blob_sidecar.index; let block_parent_root = blob_sidecar.block_parent_root(); @@ -466,7 +457,9 @@ pub fn validate_blob_sidecar_for_gossip( // We have already verified that the blob is past finalization, so we can // just check fork choice for the block's parent. let Some(parent_block) = fork_choice.get_block(&block_parent_root) else { - return Err(GossipBlobError::BlobParentUnknown(blob_sidecar)); + return Err(GossipBlobError::BlobParentUnknown { + parent_root: block_parent_root, + }); }; // Do not process a blob that does not descend from the finalized root. @@ -516,7 +509,7 @@ pub fn validate_blob_sidecar_for_gossip( )) })?; - let state = cheap_state_advance_to_obtain_committees::<_, GossipBlobError>( + let state = cheap_state_advance_to_obtain_committees::<_, GossipBlobError>( &mut parent_state, Some(parent_state_root), blob_slot, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index d9662d59f9e..8bd93a3753c 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -144,14 +144,14 @@ const WRITE_BLOCK_PROCESSING_SSZ: bool = cfg!(feature = "write_ssz_files"); /// - The block is malformed/invalid (indicated by all results other than `BeaconChainError`. /// - We encountered an error whilst trying to verify the block (a `BeaconChainError`). #[derive(Debug)] -pub enum BlockError { +pub enum BlockError { /// The parent block was unknown. /// /// ## Peer scoring /// /// It's unclear if this block is valid, but it cannot be processed without already knowing /// its parent. - ParentUnknown(RpcBlock), + ParentUnknown { parent_root: Hash256 }, /// The block slot is greater than the present slot. /// /// ## Peer scoring @@ -328,7 +328,7 @@ pub enum BlockError { InternalError(String), } -impl From for BlockError { +impl From for BlockError { fn from(e: AvailabilityCheckError) -> Self { Self::AvailabilityCheck(e) } @@ -436,30 +436,25 @@ impl From for ExecutionPayloadError { } } -impl From for BlockError { +impl From for BlockError { fn from(e: ExecutionPayloadError) -> Self { BlockError::ExecutionPayloadError(e) } } -impl From for BlockError { +impl From for BlockError { fn from(e: InconsistentFork) -> Self { BlockError::InconsistentFork(e) } } -impl std::fmt::Display for BlockError { +impl std::fmt::Display for BlockError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - BlockError::ParentUnknown(block) => { - write!(f, "ParentUnknown(parent_root:{})", block.parent_root()) - } - other => write!(f, "{:?}", other), - } + write!(f, "{:?}", self) } } -impl From for BlockError { +impl From for BlockError { fn from(e: BlockSignatureVerifierError) -> Self { match e { // Make a special distinction for `IncorrectBlockProposer` since it indicates an @@ -476,31 +471,31 @@ impl From for BlockError { } } -impl From for BlockError { +impl From for BlockError { fn from(e: BeaconChainError) -> Self { BlockError::BeaconChainError(e) } } -impl From for BlockError { +impl From for BlockError { fn from(e: BeaconStateError) -> Self { BlockError::BeaconChainError(BeaconChainError::BeaconStateError(e)) } } -impl From for BlockError { +impl From for BlockError { fn from(e: SlotProcessingError) -> Self { BlockError::BeaconChainError(BeaconChainError::SlotProcessingError(e)) } } -impl From for BlockError { +impl From for BlockError { fn from(e: DBError) -> Self { BlockError::BeaconChainError(BeaconChainError::DBError(e)) } } -impl From for BlockError { +impl From for BlockError { fn from(e: ArithError) -> Self { BlockError::BeaconChainError(BeaconChainError::ArithError(e)) } @@ -524,8 +519,8 @@ pub enum BlockSlashInfo { SignatureValid(SignedBeaconBlockHeader, TErr), } -impl BlockSlashInfo> { - pub fn from_early_error_block(header: SignedBeaconBlockHeader, e: BlockError) -> Self { +impl BlockSlashInfo { + pub fn from_early_error_block(header: SignedBeaconBlockHeader, e: BlockError) -> Self { match e { BlockError::ProposalSignatureInvalid => BlockSlashInfo::SignatureInvalid(e), // `InvalidSignature` could indicate any signature in the block, so we want @@ -535,8 +530,8 @@ impl BlockSlashInfo> { } } -impl BlockSlashInfo> { - pub fn from_early_error_blob(header: SignedBeaconBlockHeader, e: GossipBlobError) -> Self { +impl BlockSlashInfo { + pub fn from_early_error_blob(header: SignedBeaconBlockHeader, e: GossipBlobError) -> Self { match e { GossipBlobError::ProposalSignatureInvalid => BlockSlashInfo::SignatureInvalid(e), // `InvalidSignature` could indicate any signature in the block, so we want @@ -604,7 +599,7 @@ pub(crate) fn process_block_slash_info( mut chain_segment: Vec<(Hash256, RpcBlock)>, chain: &BeaconChain, -) -> Result>, BlockError> { +) -> Result>, BlockError> { if chain_segment.is_empty() { return Ok(vec![]); } @@ -619,7 +614,7 @@ pub fn signature_verify_chain_segment( .map(|(_, block)| block.slot()) .unwrap_or_else(|| slot); - let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( + let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( &mut parent.pre_state, parent.beacon_state_root, highest_slot, @@ -689,8 +684,7 @@ pub struct SignatureVerifiedBlock { } /// Used to await the result of executing payload with a remote EE. -type PayloadVerificationHandle = - JoinHandle>>>; +type PayloadVerificationHandle = JoinHandle>>; /// A wrapper around a `SignedBeaconBlock` that indicates that this block is fully verified and /// ready to import into the `BeaconChain`. The validation includes: @@ -706,14 +700,14 @@ type PayloadVerificationHandle = pub struct ExecutionPendingBlock { pub block: MaybeAvailableBlock, pub import_data: BlockImportData, - pub payload_verification_handle: PayloadVerificationHandle, + pub payload_verification_handle: PayloadVerificationHandle, } pub trait IntoGossipVerifiedBlockContents: Sized { fn into_gossip_verified_block( self, chain: &BeaconChain, - ) -> Result, BlockContentsError>; + ) -> Result, BlockContentsError>; fn inner_block(&self) -> &SignedBeaconBlock; } @@ -721,7 +715,7 @@ impl IntoGossipVerifiedBlockContents for GossipVerifiedB fn into_gossip_verified_block( self, _chain: &BeaconChain, - ) -> Result, BlockContentsError> { + ) -> Result, BlockContentsError> { Ok(self) } fn inner_block(&self) -> &SignedBeaconBlock { @@ -733,7 +727,7 @@ impl IntoGossipVerifiedBlockContents for PublishBlockReq fn into_gossip_verified_block( self, chain: &BeaconChain, - ) -> Result, BlockContentsError> { + ) -> Result, BlockContentsError> { let (block, blobs) = self.deconstruct(); let peer_das_enabled = chain.spec.is_peer_das_enabled_for_epoch(block.epoch()); @@ -765,7 +759,7 @@ fn build_gossip_verified_blobs( chain: &BeaconChain, block: &Arc>>, blobs: Option<(KzgProofs, BlobsList)>, -) -> Result>, BlockContentsError> { +) -> Result>, BlockContentsError> { blobs .map(|(kzg_proofs, blobs)| { let mut gossip_verified_blobs = vec![]; @@ -780,7 +774,7 @@ fn build_gossip_verified_blobs( gossip_verified_blobs.push(gossip_verified_blob); } let gossip_verified_blobs = VariableList::from(gossip_verified_blobs); - Ok::<_, BlockContentsError>(gossip_verified_blobs) + Ok::<_, BlockContentsError>(gossip_verified_blobs) }) .transpose() } @@ -789,7 +783,7 @@ fn build_gossip_verified_data_columns( chain: &BeaconChain, block: &SignedBeaconBlock>, blobs: Option>, -) -> Result>, BlockContentsError> { +) -> Result>, BlockContentsError> { blobs // Only attempt to build data columns if blobs is non empty to avoid skewing the metrics. .filter(|b| !b.is_empty()) @@ -819,7 +813,7 @@ fn build_gossip_verified_data_columns( chain.spec.number_of_columns, ) .map_err(DataColumnSidecarError::SszError)?; - Ok::<_, BlockContentsError>(gossip_verified_data_columns) + Ok::<_, BlockContentsError>(gossip_verified_data_columns) }) .transpose() } @@ -833,7 +827,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockError> { + ) -> Result, BlockError> { self.into_execution_pending_block_slashable(block_root, chain, notify_execution_layer) .map(|execution_pending| { // Supply valid block to slasher. @@ -842,9 +836,7 @@ pub trait IntoExecutionPendingBlock: Sized { } execution_pending }) - .map_err(|slash_info| { - process_block_slash_info::<_, BlockError>(chain, slash_info) - }) + .map_err(|slash_info| process_block_slash_info::<_, BlockError>(chain, slash_info)) } /// Convert the block to fully-verified form while producing data to aid checking slashability. @@ -853,7 +845,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>>; + ) -> Result, BlockSlashInfo>; fn block(&self) -> &SignedBeaconBlock; fn block_cloned(&self) -> Arc>; @@ -867,7 +859,7 @@ impl GossipVerifiedBlock { pub fn new( block: Arc>, chain: &BeaconChain, - ) -> Result> { + ) -> Result { // If the block is valid for gossip we don't supply it to the slasher here because // we assume it will be transformed into a fully verified block. We *do* need to supply // it to the slasher if an error occurs, because that's the end of this block's journey, @@ -877,7 +869,7 @@ impl GossipVerifiedBlock { // but it's way quicker to calculate root of the header since the hash of the tree rooted // at `BeaconBlockBody` is already computed in the header. Self::new_without_slasher_checks(block, &header, chain).map_err(|e| { - process_block_slash_info::<_, BlockError>( + process_block_slash_info::<_, BlockError>( chain, BlockSlashInfo::from_early_error_block(header, e), ) @@ -889,7 +881,7 @@ impl GossipVerifiedBlock { block: Arc>, block_header: &SignedBeaconBlockHeader, chain: &BeaconChain, - ) -> Result> { + ) -> Result { // Ensure the block is the correct structure for the fork at `block.slot()`. block .fork_name(&chain.spec) @@ -938,7 +930,7 @@ impl GossipVerifiedBlock { let block_epoch = block.slot().epoch(T::EthSpec::slots_per_epoch()); let (parent_block, block) = - verify_parent_block_is_known::(block_root, &fork_choice_read_lock, block)?; + verify_parent_block_is_known::(&fork_choice_read_lock, block)?; drop(fork_choice_read_lock); // Track the number of skip slots between the block and its parent. @@ -998,7 +990,7 @@ impl GossipVerifiedBlock { ); // The state produced is only valid for determining proposer/attester shuffling indices. - let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( + let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( &mut parent.pre_state, parent.beacon_state_root, block.slot(), @@ -1107,7 +1099,7 @@ impl IntoExecutionPendingBlock for GossipVerifiedBlock>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { + ) -> Result, BlockSlashInfo> { let execution_pending = SignatureVerifiedBlock::from_gossip_verified_block_check_slashable(self, chain)?; execution_pending.into_execution_pending_block_slashable( @@ -1135,7 +1127,7 @@ impl SignatureVerifiedBlock { block: MaybeAvailableBlock, block_root: Hash256, chain: &BeaconChain, - ) -> Result> { + ) -> Result { // Ensure the block is the correct structure for the fork at `block.slot()`. block .as_block() @@ -1147,7 +1139,7 @@ impl SignatureVerifiedBlock { let (mut parent, block) = load_parent(block, chain)?; - let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( + let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( &mut parent.pre_state, parent.beacon_state_root, block.slot(), @@ -1180,7 +1172,7 @@ impl SignatureVerifiedBlock { block: MaybeAvailableBlock, block_root: Hash256, chain: &BeaconChain, - ) -> Result>> { + ) -> Result> { let header = block.signed_block_header(); Self::new(block, block_root, chain) .map_err(|e| BlockSlashInfo::from_early_error_block(header, e)) @@ -1191,14 +1183,14 @@ impl SignatureVerifiedBlock { pub fn from_gossip_verified_block( from: GossipVerifiedBlock, chain: &BeaconChain, - ) -> Result> { + ) -> Result { let (mut parent, block) = if let Some(parent) = from.parent { (parent, from.block) } else { load_parent(from.block, chain)? }; - let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( + let state = cheap_state_advance_to_obtain_committees::<_, BlockError>( &mut parent.pre_state, parent.beacon_state_root, block.slot(), @@ -1234,7 +1226,7 @@ impl SignatureVerifiedBlock { pub fn from_gossip_verified_block_check_slashable( from: GossipVerifiedBlock, chain: &BeaconChain, - ) -> Result>> { + ) -> Result> { let header = from.block.signed_block_header(); Self::from_gossip_verified_block(from, chain) .map_err(|e| BlockSlashInfo::from_early_error_block(header, e)) @@ -1256,7 +1248,7 @@ impl IntoExecutionPendingBlock for SignatureVerifiedBloc block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { + ) -> Result, BlockSlashInfo> { let header = self.block.signed_block_header(); let (parent, block) = if let Some(parent) = self.parent { (parent, self.block) @@ -1293,7 +1285,7 @@ impl IntoExecutionPendingBlock for Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { + ) -> Result, BlockSlashInfo> { // Perform an early check to prevent wasting time on irrelevant blocks. let block_root = check_block_relevancy(&self, block_root, chain) .map_err(|e| BlockSlashInfo::SignatureNotChecked(self.signed_block_header(), e))?; @@ -1327,7 +1319,7 @@ impl IntoExecutionPendingBlock for RpcBlock block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { + ) -> Result, BlockSlashInfo> { // Perform an early check to prevent wasting time on irrelevant blocks. let block_root = check_block_relevancy(self.as_block(), block_root, chain) .map_err(|e| BlockSlashInfo::SignatureNotChecked(self.signed_block_header(), e))?; @@ -1368,7 +1360,7 @@ impl ExecutionPendingBlock { mut consensus_context: ConsensusContext, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result> { + ) -> Result { chain .observed_slashable .write() @@ -1404,7 +1396,9 @@ impl ExecutionPendingBlock { // because it will revert finalization. Note that the finalized block is stored in fork // choice, so we will not reject any child of the finalized block (this is relevant during // genesis). - return Err(BlockError::ParentUnknown(block.into_rpc_block())); + return Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }); } /* @@ -1781,7 +1775,7 @@ impl ExecutionPendingBlock { fn check_block_against_anchor_slot( block: BeaconBlockRef<'_, T::EthSpec>, chain: &BeaconChain, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { if let Some(anchor_slot) = chain.store.get_anchor_slot() { if block.slot() <= anchor_slot { return Err(BlockError::WeakSubjectivityConflict); @@ -1798,7 +1792,7 @@ fn check_block_against_finalized_slot( block: BeaconBlockRef<'_, T::EthSpec>, block_root: Hash256, chain: &BeaconChain, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { // The finalized checkpoint is being read from fork choice, rather than the cached head. // // Fork choice has the most up-to-date view of finalization and there's no point importing a @@ -1833,7 +1827,7 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< chain: &BeaconChain, fork_choice: &BeaconForkChoice, block: B, -) -> Result> { +) -> Result { if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) { Ok(block) } else { @@ -1854,7 +1848,9 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< block_parent_root: block.parent_root(), }) } else { - Err(BlockError::ParentUnknown(block.into_rpc_block())) + Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }) } } } @@ -1870,7 +1866,7 @@ pub fn check_block_relevancy( signed_block: &SignedBeaconBlock, block_root: Hash256, chain: &BeaconChain, -) -> Result> { +) -> Result { let block = signed_block.message(); // Do not process blocks from the future. @@ -1938,17 +1934,15 @@ pub fn get_block_header_root(block_header: &SignedBeaconBlockHeader) -> Hash256 /// fork choice. #[allow(clippy::type_complexity)] fn verify_parent_block_is_known( - block_root: Hash256, fork_choice_read_lock: &RwLockReadGuard>, block: Arc>, -) -> Result<(ProtoBlock, Arc>), BlockError> { +) -> Result<(ProtoBlock, Arc>), BlockError> { if let Some(proto_block) = fork_choice_read_lock.get_block(&block.parent_root()) { Ok((proto_block, block)) } else { - Err(BlockError::ParentUnknown(RpcBlock::new_without_blobs( - Some(block_root), - block, - ))) + Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }) } } @@ -1960,7 +1954,7 @@ fn verify_parent_block_is_known( fn load_parent>( block: B, chain: &BeaconChain, -) -> Result<(PreProcessingSnapshot, B), BlockError> { +) -> Result<(PreProcessingSnapshot, B), BlockError> { // Reject any block if its parent is not known to fork choice. // // A block that is not in fork choice is either: @@ -1976,7 +1970,9 @@ fn load_parent>( .fork_choice_read_lock() .contains_block(&block.parent_root()) { - return Err(BlockError::ParentUnknown(block.into_rpc_block())); + return Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }); } let db_read_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_READ); @@ -2072,7 +2068,7 @@ pub trait BlockBlobError: From + From + Debu fn proposer_signature_invalid() -> Self; } -impl BlockBlobError for BlockError { +impl BlockBlobError for BlockError { fn not_later_than_parent_error(block_slot: Slot, parent_slot: Slot) -> Self { BlockError::BlockIsNotLaterThanParent { block_slot, @@ -2089,7 +2085,7 @@ impl BlockBlobError for BlockError { } } -impl BlockBlobError for GossipBlobError { +impl BlockBlobError for GossipBlobError { fn not_later_than_parent_error(blob_slot: Slot, parent_slot: Slot) -> Self { GossipBlobError::BlobIsNotLaterThanParent { blob_slot, diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index b271f0a2f98..707dfa56d84 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -397,39 +397,39 @@ pub type GossipVerifiedBlockContents = ( ); #[derive(Debug)] -pub enum BlockContentsError { - BlockError(BlockError), - BlobError(GossipBlobError), +pub enum BlockContentsError { + BlockError(BlockError), + BlobError(GossipBlobError), BlobSidecarError(blob_sidecar::BlobSidecarError), DataColumnError(GossipDataColumnError), DataColumnSidecarError(data_column_sidecar::DataColumnSidecarError), } -impl From> for BlockContentsError { - fn from(value: BlockError) -> Self { +impl From for BlockContentsError { + fn from(value: BlockError) -> Self { Self::BlockError(value) } } -impl From> for BlockContentsError { - fn from(value: GossipBlobError) -> Self { +impl From for BlockContentsError { + fn from(value: GossipBlobError) -> Self { Self::BlobError(value) } } -impl From for BlockContentsError { +impl From for BlockContentsError { fn from(value: GossipDataColumnError) -> Self { Self::DataColumnError(value) } } -impl From for BlockContentsError { +impl From for BlockContentsError { fn from(value: data_column_sidecar::DataColumnSidecarError) -> Self { Self::DataColumnSidecarError(value) } } -impl std::fmt::Display for BlockContentsError { +impl std::fmt::Display for BlockContentsError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { BlockContentsError::BlockError(err) => { @@ -462,7 +462,6 @@ pub trait AsBlock { fn as_block(&self) -> &SignedBeaconBlock; fn block_cloned(&self) -> Arc>; fn canonical_root(&self) -> Hash256; - fn into_rpc_block(self) -> RpcBlock; } impl AsBlock for Arc> { @@ -501,10 +500,6 @@ impl AsBlock for Arc> { fn canonical_root(&self) -> Hash256 { SignedBeaconBlock::canonical_root(self) } - - fn into_rpc_block(self) -> RpcBlock { - RpcBlock::new_without_blobs(None, self) - } } impl AsBlock for MaybeAvailableBlock { @@ -547,15 +542,6 @@ impl AsBlock for MaybeAvailableBlock { fn canonical_root(&self) -> Hash256 { self.as_block().canonical_root() } - - fn into_rpc_block(self) -> RpcBlock { - match self { - MaybeAvailableBlock::Available(available_block) => available_block.into_rpc_block(), - MaybeAvailableBlock::AvailabilityPending { block_root, block } => { - RpcBlock::new_without_blobs(Some(block_root), block) - } - } - } } impl AsBlock for AvailableBlock { @@ -594,36 +580,6 @@ impl AsBlock for AvailableBlock { fn canonical_root(&self) -> Hash256 { self.block().canonical_root() } - - fn into_rpc_block(self) -> RpcBlock { - let number_of_columns = self.spec.number_of_columns; - let (block_root, block, blobs_opt, data_columns_opt) = self.deconstruct(); - // Circumvent the constructor here, because an Available block will have already had - // consistency checks performed. - let inner = match (blobs_opt, data_columns_opt) { - (None, None) => RpcBlockInner::Block(block), - (Some(blobs), _) => RpcBlockInner::BlockAndBlobs(block, blobs), - (_, Some(data_columns)) => RpcBlockInner::BlockAndCustodyColumns( - block, - RuntimeVariableList::new( - data_columns - .into_iter() - // TODO(das): This is an ugly hack that should be removed. After updating - // store types to handle custody data columns this should not be required. - // It's okay-ish because available blocks must have all the required custody - // columns. - .map(|d| CustodyDataColumn::from_asserted_custody(d)) - .collect(), - number_of_columns, - ) - .expect("data column list is within bounds"), - ), - }; - RpcBlock { - block_root, - block: inner, - } - } } impl AsBlock for RpcBlock { @@ -662,8 +618,4 @@ impl AsBlock for RpcBlock { fn canonical_root(&self) -> Hash256 { self.as_block().canonical_root() } - - fn into_rpc_block(self) -> RpcBlock { - self - } } diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 81bddf6a927..b9b98bfbc00 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -62,7 +62,7 @@ impl PayloadNotifier { block: Arc>, state: &BeaconState, notify_execution_layer: NotifyExecutionLayer, - ) -> Result> { + ) -> Result { let payload_verification_status = if is_execution_enabled(state, block.message().body()) { // Perform the initial stages of payload verification. // @@ -110,9 +110,7 @@ impl PayloadNotifier { }) } - pub async fn notify_new_payload( - self, - ) -> Result> { + pub async fn notify_new_payload(self) -> Result { if let Some(precomputed_status) = self.payload_verification_status { Ok(precomputed_status) } else { @@ -133,7 +131,7 @@ impl PayloadNotifier { async fn notify_new_payload<'a, T: BeaconChainTypes>( chain: &Arc>, block: BeaconBlockRef<'a, T::EthSpec>, -) -> Result> { +) -> Result { let execution_layer = chain .execution_layer .as_ref() @@ -237,7 +235,7 @@ pub async fn validate_merge_block<'a, T: BeaconChainTypes>( chain: &Arc>, block: BeaconBlockRef<'a, T::EthSpec>, allow_optimistic_import: AllowOptimisticImport, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { let spec = &chain.spec; let block_epoch = block.slot().epoch(T::EthSpec::slots_per_epoch()); let execution_payload = block.execution_payload()?; @@ -335,7 +333,7 @@ pub fn validate_execution_payload_for_gossip( parent_block: &ProtoBlock, block: BeaconBlockRef<'_, T::EthSpec>, chain: &BeaconChain, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { // Only apply this validation if this is a Bellatrix beacon block. if let Ok(execution_payload) = block.body().execution_payload() { // This logic should match `is_execution_enabled`. We use only the execution block hash of diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index eb2d78685b2..e08e35b08f6 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1986,7 +1986,7 @@ where slot: Slot, block_root: Hash256, block_contents: SignedBlockContentsTuple, - ) -> Result> { + ) -> Result { self.set_current_slot(slot); let (block, blob_items) = block_contents; @@ -2013,7 +2013,7 @@ where pub async fn process_block_result( &self, block_contents: SignedBlockContentsTuple, - ) -> Result> { + ) -> Result { let (block, blob_items) = block_contents; let sidecars = blob_items @@ -2098,7 +2098,7 @@ where SignedBlockContentsTuple, BeaconState, ), - BlockError, + BlockError, > { self.set_current_slot(slot); let (block_contents, new_state) = self.make_block(state, slot).await; @@ -2144,7 +2144,7 @@ where state: BeaconState, state_root: Hash256, validators: &[usize], - ) -> Result<(SignedBeaconBlockHash, BeaconState), BlockError> { + ) -> Result<(SignedBeaconBlockHash, BeaconState), BlockError> { self.add_attested_block_at_slot_with_sync( slot, state, @@ -2162,7 +2162,7 @@ where state_root: Hash256, validators: &[usize], sync_committee_strategy: SyncCommitteeStrategy, - ) -> Result<(SignedBeaconBlockHash, BeaconState), BlockError> { + ) -> Result<(SignedBeaconBlockHash, BeaconState), BlockError> { let (block_hash, block, state) = self.add_block_at_slot(slot, state).await?; self.attest_block(&state, state_root, block_hash, &block.0, validators); diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 1c494d99bf5..faa4d74a182 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -1098,8 +1098,8 @@ async fn block_gossip_verification() { assert!( matches!( unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(SignedBeaconBlock::from_block(block, signature))).await), - BlockError::ParentUnknown(block) - if block.parent_root() == parent_root + BlockError::ParentUnknown {parent_root: p} + if p == parent_root ), "should not import a block for an unknown parent" ); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index c7195e51ebf..b455c3bace4 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -212,7 +212,7 @@ impl InvalidPayloadRig { .unwrap(); } - async fn import_block_parametric) -> bool>( + async fn import_block_parametric bool>( &mut self, new_payload_response: Payload, forkchoice_response: Payload, diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index bbdfc31d430..e0fc518d46c 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -544,7 +544,7 @@ fn check_slashable( block_root: Hash256, block_clone: &SignedBeaconBlock>, log_clone: &Logger, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { let slashable_cache = chain_clone.observed_slashable.read(); if let Some(blobs) = blobs_opt.as_ref() { blobs.iter().try_for_each(|blob| { diff --git a/beacon_node/http_api/tests/broadcast_validation_tests.rs b/beacon_node/http_api/tests/broadcast_validation_tests.rs index fdc873f773e..59cdbb1c99e 100644 --- a/beacon_node/http_api/tests/broadcast_validation_tests.rs +++ b/beacon_node/http_api/tests/broadcast_validation_tests.rs @@ -81,9 +81,7 @@ pub async fn gossip_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is valid from a gossip perspective is accepted when using `broadcast_validation=gossip`. @@ -268,10 +266,7 @@ pub async fn consensus_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is only valid from a gossip perspective is rejected when using `broadcast_validation=consensus`. @@ -317,10 +312,7 @@ pub async fn consensus_gossip() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()); } /// This test checks that a block that is valid from both a gossip and consensus perspective, but nonetheless equivocates, is accepted when using `broadcast_validation=consensus`. @@ -489,10 +481,7 @@ pub async fn equivocation_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is valid from both a gossip and consensus perspective is rejected when using `broadcast_validation=consensus_and_equivocation`. @@ -566,9 +555,9 @@ pub async fn equivocation_consensus_early_equivocation() { let error_response: eth2::Error = response.err().unwrap(); assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(Slashable)".to_string()) + assert_server_message_error( + error_response, + "BAD_REQUEST: BlockError(Slashable)".to_string(), ); } @@ -616,10 +605,7 @@ pub async fn equivocation_gossip() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()); } /// This test checks that a block that is valid from both a gossip and consensus perspective but @@ -797,10 +783,7 @@ pub async fn blinded_gossip_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is valid from a gossip perspective is accepted when using `broadcast_validation=gossip`. @@ -978,10 +961,7 @@ pub async fn blinded_consensus_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is only valid from a gossip perspective is rejected when using `broadcast_validation=consensus`. @@ -1027,10 +1007,7 @@ pub async fn blinded_consensus_gossip() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()); } /// This test checks that a block that is valid from both a gossip and consensus perspective is accepted when using `broadcast_validation=consensus`. @@ -1122,10 +1099,7 @@ pub async fn blinded_equivocation_invalid() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: BlockError(NotFinalizedDescendant { block_parent_root: 0x0000000000000000000000000000000000000000000000000000000000000000 })".to_string()); } /// This test checks that a block that is valid from both a gossip and consensus perspective is rejected when using `broadcast_validation=consensus_and_equivocation`. @@ -1195,9 +1169,9 @@ pub async fn blinded_equivocation_consensus_early_equivocation() { let error_response: eth2::Error = response.err().unwrap(); assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: BlockError(Slashable)".to_string()) + assert_server_message_error( + error_response, + "BAD_REQUEST: BlockError(Slashable)".to_string(), ); } @@ -1246,9 +1220,7 @@ pub async fn blinded_equivocation_gossip() { /* mandated by Beacon API spec */ assert_eq!(error_response.status(), Some(StatusCode::BAD_REQUEST)); - assert!( - matches!(error_response, eth2::Error::ServerMessage(err) if err.message == "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()) - ); + assert_server_message_error(error_response, "BAD_REQUEST: Invalid block: StateRootMismatch { block: 0x0000000000000000000000000000000000000000000000000000000000000000, local: 0xfc675d642ff7a06458eb33c7d7b62a5813e34d1b2bb1aee3e395100b579da026 }".to_string()); } /// This test checks that a block that is valid from both a gossip and @@ -1401,3 +1373,10 @@ pub async fn blinded_equivocation_full_pass() { .chain .block_is_known_to_fork_choice(&block.canonical_root())); } + +fn assert_server_message_error(error_response: eth2::Error, expected_message: String) { + let eth2::Error::ServerMessage(err) = error_response else { + panic!("Not a eth2::Error::ServerMessage"); + }; + assert_eq!(err.message, expected_message); +} diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index d5d83d540a0..62f1371c811 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -780,7 +780,7 @@ impl NetworkBeaconProcessor { metrics::set_gauge(&metrics::BEACON_BLOB_DELAY_GOSSIP, delay.as_millis() as i64); match self .chain - .verify_blob_sidecar_for_gossip(blob_sidecar, blob_index) + .verify_blob_sidecar_for_gossip(blob_sidecar.clone(), blob_index) { Ok(gossip_verified_blob) => { metrics::inc_counter(&metrics::BEACON_PROCESSOR_GOSSIP_BLOB_VERIFIED_TOTAL); @@ -825,16 +825,19 @@ impl NetworkBeaconProcessor { } Err(err) => { match err { - GossipBlobError::BlobParentUnknown(blob) => { + GossipBlobError::BlobParentUnknown { parent_root } => { debug!( self.log, "Unknown parent hash for blob"; "action" => "requesting parent", - "block_root" => %blob.block_root(), - "parent_root" => %blob.block_parent_root(), + "block_root" => %root, + "parent_root" => %parent_root, "commitment" => %commitment, ); - self.send_sync_message(SyncMessage::UnknownParentBlob(peer_id, blob)); + self.send_sync_message(SyncMessage::UnknownParentBlob( + peer_id, + blob_sidecar, + )); } GossipBlobError::KzgNotInitialized | GossipBlobError::PubkeyCacheTimeout @@ -1224,7 +1227,7 @@ impl NetworkBeaconProcessor { ); return None; } - Err(BlockError::ParentUnknown(block)) => { + Err(BlockError::ParentUnknown { .. }) => { debug!( self.log, "Unknown parent for gossip block"; @@ -1516,7 +1519,7 @@ impl NetworkBeaconProcessor { "block_root" => %block_root, ); } - Err(BlockError::ParentUnknown(_)) => { + Err(BlockError::ParentUnknown { .. }) => { // This should not occur. It should be checked by `should_forward_block`. // Do not send sync message UnknownParentBlock to prevent conflicts with the // BlockComponentProcessed message below. If this error ever happens, lookup sync @@ -3131,7 +3134,7 @@ impl NetworkBeaconProcessor { invalid_block_storage: &InvalidBlockStorage, block_root: Hash256, block: &SignedBeaconBlock, - error: &BlockError, + error: &BlockError, log: &Logger, ) { if let InvalidBlockStorage::Enabled(base_dir) = invalid_block_storage { diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index 508576d9f52..c21054dab50 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -706,15 +706,12 @@ impl NetworkBeaconProcessor { } /// Helper function to handle a `BlockError` from `process_chain_segment` - fn handle_failed_chain_segment( - &self, - error: BlockError, - ) -> Result<(), ChainSegmentFailed> { + fn handle_failed_chain_segment(&self, error: BlockError) -> Result<(), ChainSegmentFailed> { match error { - BlockError::ParentUnknown(block) => { + BlockError::ParentUnknown { parent_root, .. } => { // blocks should be sequential and all parents should exist Err(ChainSegmentFailed { - message: format!("Block has an unknown parent: {}", block.parent_root()), + message: format!("Block has an unknown parent: {}", parent_root), // Peers are faulty if they send non-sequential blocks. peer_action: Some(PeerAction::LowToleranceError), }) diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 7a5cda20692..e31adb783c9 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -473,7 +473,7 @@ impl BlockLookups { pub fn on_processing_result( &mut self, process_type: BlockProcessType, - result: BlockProcessingResult, + result: BlockProcessingResult, cx: &mut SyncNetworkContext, ) { let lookup_result = match process_type { @@ -493,7 +493,7 @@ impl BlockLookups { pub fn on_processing_result_inner>( &mut self, lookup_id: SingleLookupId, - result: BlockProcessingResult, + result: BlockProcessingResult, cx: &mut SyncNetworkContext, ) -> Result { let Some(lookup) = self.single_block_lookups.get_mut(&lookup_id) else { @@ -556,16 +556,14 @@ impl BlockLookups { error!(self.log, "Beacon chain error processing lookup component"; "block_root" => %block_root, "error" => ?e); Action::Drop } - BlockError::ParentUnknown(block) => { + BlockError::ParentUnknown { parent_root, .. } => { // Reverts the status of this request to `AwaitingProcessing` holding the // downloaded data. A future call to `continue_requests` will re-submit it // once there are no pending parent requests. // Note: `BlockError::ParentUnknown` is only returned when processing // blocks, not blobs. request_state.revert_to_awaiting_processing()?; - Action::ParentUnknown { - parent_root: block.parent_root(), - } + Action::ParentUnknown { parent_root } } ref e @ BlockError::ExecutionPayloadError(ref epe) if !epe.penalize_peer() => { // These errors indicate that the execution layer is offline diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 4c63943e3f8..6d852b2572d 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -9,7 +9,7 @@ use super::*; use crate::sync::block_lookups::common::ResponseType; use beacon_chain::blob_verification::GossipVerifiedBlob; -use beacon_chain::block_verification_types::{BlockImportData, RpcBlock}; +use beacon_chain::block_verification_types::BlockImportData; use beacon_chain::builder::Witness; use beacon_chain::data_availability_checker::Availability; use beacon_chain::eth1_chain::CachingEth1Backend; @@ -211,11 +211,7 @@ impl TestRig { fn trigger_unknown_parent_block(&mut self, peer_id: PeerId, block: Arc>) { let block_root = block.canonical_root(); - self.send_sync_message(SyncMessage::UnknownParentBlock( - peer_id, - RpcBlock::new_without_blobs(Some(block_root), block), - block_root, - )) + self.send_sync_message(SyncMessage::UnknownParentBlock(peer_id, block, block_root)) } fn trigger_unknown_parent_blob(&mut self, peer_id: PeerId, blob: BlobSidecar) { @@ -440,12 +436,12 @@ impl TestRig { *parent_chain.last().unwrap() } - fn parent_block_processed(&mut self, chain_hash: Hash256, result: BlockProcessingResult) { + fn parent_block_processed(&mut self, chain_hash: Hash256, result: BlockProcessingResult) { let id = self.find_single_lookup_for(self.find_oldest_parent_lookup(chain_hash)); self.single_block_component_processed(id, result); } - fn parent_blob_processed(&mut self, chain_hash: Hash256, result: BlockProcessingResult) { + fn parent_blob_processed(&mut self, chain_hash: Hash256, result: BlockProcessingResult) { let id = self.find_single_lookup_for(self.find_oldest_parent_lookup(chain_hash)); self.single_blob_component_processed(id, result); } @@ -457,7 +453,7 @@ impl TestRig { ); } - fn single_block_component_processed(&mut self, id: Id, result: BlockProcessingResult) { + fn single_block_component_processed(&mut self, id: Id, result: BlockProcessingResult) { self.send_sync_message(SyncMessage::BlockComponentProcessed { process_type: BlockProcessType::SingleBlock { id }, result, @@ -472,7 +468,7 @@ impl TestRig { ) } - fn single_blob_component_processed(&mut self, id: Id, result: BlockProcessingResult) { + fn single_blob_component_processed(&mut self, id: Id, result: BlockProcessingResult) { self.send_sync_message(SyncMessage::BlockComponentProcessed { process_type: BlockProcessType::SingleBlob { id }, result, @@ -1440,7 +1436,9 @@ fn test_single_block_lookup_becomes_parent_request() { // parent request after processing. rig.single_block_component_processed( id.lookup_id, - BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(), + BlockProcessingResult::Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }), ); assert_eq!(rig.active_single_lookups_count(), 2); // 2 = current + parent rig.expect_block_parent_request(parent_root); @@ -1661,7 +1659,9 @@ fn test_parent_lookup_too_deep_grow_ancestor() { // the processing result rig.parent_block_processed( chain_hash, - BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(), + BlockProcessingResult::Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }), ) } @@ -1685,7 +1685,10 @@ fn test_parent_lookup_too_deep_grow_tip() { rig.expect_block_process(ResponseType::Block); rig.single_block_component_processed( id.lookup_id, - BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(), + BlockError::ParentUnknown { + parent_root: block.parent_root(), + } + .into(), ); } @@ -1840,7 +1843,9 @@ fn test_same_chain_race_condition() { rig.log(&format!("Block {i} ParentUnknown")); rig.parent_block_processed( chain_hash, - BlockError::ParentUnknown(RpcBlock::new_without_blobs(None, block)).into(), + BlockProcessingResult::Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }), ) } } @@ -2130,7 +2135,7 @@ mod deneb_only { RequestTrigger::GossipUnknownParentBlock { .. } => { rig.send_sync_message(SyncMessage::UnknownParentBlock( peer_id, - RpcBlock::new_without_blobs(Some(block_root), block.clone()), + block.clone(), block_root, )); @@ -2412,7 +2417,9 @@ mod deneb_only { .unwrap(); self.rig.parent_block_processed( self.block_root, - BlockProcessingResult::Err(BlockError::ParentUnknown(block)), + BlockProcessingResult::Err(BlockError::ParentUnknown { + parent_root: block.parent_root(), + }), ); assert_eq!(self.rig.active_parent_lookups_count(), 1); self diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index d6ce14adb16..ed91c73d8bf 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -48,7 +48,6 @@ use crate::sync::block_lookups::{ use crate::sync::block_sidecar_coupling::RangeBlockComponentsRequest; use crate::sync::network_context::PeerGroup; use beacon_chain::block_verification_types::AsBlock; -use beacon_chain::block_verification_types::RpcBlock; use beacon_chain::validator_monitor::timestamp_now; use beacon_chain::{ AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, BlockError, EngineState, @@ -115,7 +114,7 @@ pub enum SyncMessage { }, /// A block with an unknown parent has been received. - UnknownParentBlock(PeerId, RpcBlock, Hash256), + UnknownParentBlock(PeerId, Arc>, Hash256), /// A blob with an unknown parent has been received. UnknownParentBlob(PeerId, Arc>), @@ -150,7 +149,7 @@ pub enum SyncMessage { /// Block processed BlockComponentProcessed { process_type: BlockProcessType, - result: BlockProcessingResult, + result: BlockProcessingResult, }, /// Sample data column verified @@ -182,9 +181,9 @@ impl BlockProcessType { } #[derive(Debug)] -pub enum BlockProcessingResult { +pub enum BlockProcessingResult { Ok(AvailabilityProcessingStatus), - Err(BlockError), + Err(BlockError), Ignored, } @@ -1187,10 +1186,8 @@ impl SyncManager { } } -impl From>> - for BlockProcessingResult -{ - fn from(result: Result>) -> Self { +impl From> for BlockProcessingResult { + fn from(result: Result) -> Self { match result { Ok(status) => BlockProcessingResult::Ok(status), Err(e) => BlockProcessingResult::Err(e), @@ -1198,8 +1195,8 @@ impl From>> } } -impl From> for BlockProcessingResult { - fn from(e: BlockError) -> Self { +impl From for BlockProcessingResult { + fn from(e: BlockError) -> Self { BlockProcessingResult::Err(e) } }