Drop block data from BlockError and BlobError (#5735)

* Drop block data from BlockError and BlobError

* Debug release tests

* Fix release tests

* Revert unnecessary changes to lighthouse_metrics

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -780,7 +780,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         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<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
             }
             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<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 );
                 return None;
             }
-            Err(BlockError::ParentUnknown(block)) => {
+            Err(BlockError::ParentUnknown { .. }) => {
                 debug!(
                     self.log,
                     "Unknown parent for gossip block";
@@ -1516,7 +1519,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                     "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<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
         invalid_block_storage: &InvalidBlockStorage,
         block_root: Hash256,
         block: &SignedBeaconBlock<T::EthSpec>,
-        error: &BlockError<T::EthSpec>,
+        error: &BlockError,
         log: &Logger,
     ) {
         if let InvalidBlockStorage::Enabled(base_dir) = invalid_block_storage {

beacon_node/network/src/network_beacon_processor/sync_methods.rs

@@ -706,15 +706,12 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
     }
 
     /// Helper function to handle a `BlockError` from `process_chain_segment`
-    fn handle_failed_chain_segment(
-        &self,
-        error: BlockError<T::EthSpec>,
-    ) -> 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),
                 })

beacon_node/network/src/sync/block_lookups/mod.rs

@@ -473,7 +473,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     pub fn on_processing_result(
         &mut self,
         process_type: BlockProcessType,
-        result: BlockProcessingResult<T::EthSpec>,
+        result: BlockProcessingResult,
         cx: &mut SyncNetworkContext<T>,
     ) {
         let lookup_result = match process_type {
@@ -493,7 +493,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
     pub fn on_processing_result_inner<R: RequestState<T>>(
         &mut self,
         lookup_id: SingleLookupId,
-        result: BlockProcessingResult<T::EthSpec>,
+        result: BlockProcessingResult,
         cx: &mut SyncNetworkContext<T>,
     ) -> Result<LookupResult, LookupRequestError> {
         let Some(lookup) = self.single_block_lookups.get_mut(&lookup_id) else {
@@ -556,16 +556,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
                         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

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<SignedBeaconBlock<E>>) {
         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<E>) {
@@ -440,12 +436,12 @@ impl TestRig {
         *parent_chain.last().unwrap()
     }
 
-    fn parent_block_processed(&mut self, chain_hash: Hash256, result: BlockProcessingResult<E>) {
+    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<E>) {
+    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<E>) {
+    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<E>) {
+    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

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<E: EthSpec> {
     },
 
     /// A block with an unknown parent has been received.
-    UnknownParentBlock(PeerId, RpcBlock<E>, Hash256),
+    UnknownParentBlock(PeerId, Arc<SignedBeaconBlock<E>>, Hash256),
 
     /// A blob with an unknown parent has been received.
     UnknownParentBlob(PeerId, Arc<BlobSidecar<E>>),
@@ -150,7 +149,7 @@ pub enum SyncMessage<E: EthSpec> {
     /// Block processed
     BlockComponentProcessed {
         process_type: BlockProcessType,
-        result: BlockProcessingResult<E>,
+        result: BlockProcessingResult,
     },
 
     /// Sample data column verified
@@ -182,9 +181,9 @@ impl BlockProcessType {
 }
 
 #[derive(Debug)]
-pub enum BlockProcessingResult<E: EthSpec> {
+pub enum BlockProcessingResult {
     Ok(AvailabilityProcessingStatus),
-    Err(BlockError<E>),
+    Err(BlockError),
     Ignored,
 }
 
@@ -1187,10 +1186,8 @@ impl<T: BeaconChainTypes> SyncManager<T> {
     }
 }
 
-impl<E: EthSpec> From<Result<AvailabilityProcessingStatus, BlockError<E>>>
-    for BlockProcessingResult<E>
-{
-    fn from(result: Result<AvailabilityProcessingStatus, BlockError<E>>) -> Self {
+impl From<Result<AvailabilityProcessingStatus, BlockError>> for BlockProcessingResult {
+    fn from(result: Result<AvailabilityProcessingStatus, BlockError>) -> Self {
         match result {
             Ok(status) => BlockProcessingResult::Ok(status),
             Err(e) => BlockProcessingResult::Err(e),
@@ -1198,8 +1195,8 @@ impl<E: EthSpec> From<Result<AvailabilityProcessingStatus, BlockError<E>>>
     }
 }
 
-impl<E: EthSpec> From<BlockError<E>> for BlockProcessingResult<E> {
-    fn from(e: BlockError<E>) -> Self {
+impl From<BlockError> for BlockProcessingResult {
+    fn from(e: BlockError) -> Self {
         BlockProcessingResult::Err(e)
     }
 }