Remove block clone (#1448)

## Issue Addressed

#1028 

A bit late, but I think if `BlockError` had a kind (the current `BlockError` minus everything on the variants that comes directly from the block) and the original block, more clones could be removed

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -71,14 +71,14 @@ pub const FORK_CHOICE_DB_KEY: [u8; 32] = [0; 32];
 
 /// The result of a chain segment processing.
 #[derive(Debug)]
-pub enum ChainSegmentResult {
+pub enum ChainSegmentResult<T: EthSpec> {
     /// Processing this chain segment finished successfully.
     Successful { imported_blocks: usize },
     /// There was an error processing this chain segment. Before the error, some blocks could
     /// have been imported.
     Failed {
         imported_blocks: usize,
-        error: BlockError,
+        error: BlockError<T>,
     },
 }
 
@@ -1153,7 +1153,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn process_chain_segment(
         &self,
         chain_segment: Vec<SignedBeaconBlock<T::EthSpec>>,
-    ) -> ChainSegmentResult {
+    ) -> ChainSegmentResult<T::EthSpec> {
         let mut filtered_chain_segment = Vec::with_capacity(chain_segment.len());
         let mut imported_blocks = 0;
 
@@ -1286,7 +1286,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn verify_block_for_gossip(
         &self,
         block: SignedBeaconBlock<T::EthSpec>,
-    ) -> Result<GossipVerifiedBlock<T>, BlockError> {
+    ) -> Result<GossipVerifiedBlock<T>, BlockError<T::EthSpec>> {
         let slot = block.message.slot;
         let graffiti_string = String::from_utf8(block.message.body.graffiti[..].to_vec())
             .unwrap_or_else(|_| format!("{:?}", &block.message.body.graffiti[..]));
@@ -1332,7 +1332,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn process_block<B: IntoFullyVerifiedBlock<T>>(
         &self,
         unverified_block: B,
-    ) -> Result<Hash256, BlockError> {
+    ) -> Result<Hash256, BlockError<T::EthSpec>> {
         // Start the Prometheus timer.
         let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES);
 
@@ -1343,7 +1343,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         let block = unverified_block.block().clone();
 
         // A small closure to group the verification and import errors.
-        let import_block = |unverified_block: B| -> Result<Hash256, BlockError> {
+        let import_block = |unverified_block: B| -> Result<Hash256, BlockError<T::EthSpec>> {
             let fully_verified = unverified_block.into_fully_verified_block(self)?;
             self.import_block(fully_verified)
         };
@@ -1411,7 +1411,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     fn import_block(
         &self,
         fully_verified_block: FullyVerifiedBlock<T>,
-    ) -> Result<Hash256, BlockError> {
+    ) -> Result<Hash256, BlockError<T::EthSpec>> {
         let signed_block = fully_verified_block.block;
         let block = &signed_block.message;
         let block_root = fully_verified_block.block_root;
@@ -2133,8 +2133,8 @@ impl From<BeaconStateError> for Error {
     }
 }
 
-impl ChainSegmentResult {
+impl<T: EthSpec> ChainSegmentResult<T> {
-    pub fn into_block_error(self) -> Result<(), BlockError> {
+    pub fn into_block_error(self) -> Result<(), BlockError<T>> {
         match self {
             ChainSegmentResult::Failed { error, .. } => Err(error),
             ChainSegmentResult::Successful { .. } => Ok(()),

beacon_node/beacon_chain/src/block_verification.rs

@@ -83,14 +83,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<T: EthSpec> {
     /// 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(Hash256),
+    ParentUnknown(Box<SignedBeaconBlock<T>>),
     /// The block slot is greater than the present slot.
     ///
     /// ## Peer scoring
@@ -199,7 +199,7 @@ pub enum BlockError {
     BeaconChainError(BeaconChainError),
 }
 
-impl From<BlockSignatureVerifierError> for BlockError {
+impl<T: EthSpec> From<BlockSignatureVerifierError> for BlockError<T> {
     fn from(e: BlockSignatureVerifierError) -> Self {
         match e {
             // Make a special distinction for `IncorrectBlockProposer` since it indicates an
@@ -216,25 +216,25 @@ impl From<BlockSignatureVerifierError> for BlockError {
     }
 }
 
-impl From<BeaconChainError> for BlockError {
+impl<T: EthSpec> From<BeaconChainError> for BlockError<T> {
     fn from(e: BeaconChainError) -> Self {
         BlockError::BeaconChainError(e)
     }
 }
 
-impl From<BeaconStateError> for BlockError {
+impl<T: EthSpec> From<BeaconStateError> for BlockError<T> {
     fn from(e: BeaconStateError) -> Self {
         BlockError::BeaconChainError(BeaconChainError::BeaconStateError(e))
     }
 }
 
-impl From<SlotProcessingError> for BlockError {
+impl<T: EthSpec> From<SlotProcessingError> for BlockError<T> {
     fn from(e: SlotProcessingError) -> Self {
         BlockError::BeaconChainError(BeaconChainError::SlotProcessingError(e))
     }
 }
 
-impl From<DBError> for BlockError {
+impl<T: EthSpec> From<DBError> for BlockError<T> {
     fn from(e: DBError) -> Self {
         BlockError::BeaconChainError(BeaconChainError::DBError(e))
     }
@@ -251,15 +251,17 @@ impl From<DBError> for BlockError {
 /// The given `chain_segment` must span no more than two epochs, otherwise an error will be
 /// returned.
 pub fn signature_verify_chain_segment<T: BeaconChainTypes>(
-    chain_segment: Vec<(Hash256, SignedBeaconBlock<T::EthSpec>)>,
+    mut chain_segment: Vec<(Hash256, SignedBeaconBlock<T::EthSpec>)>,
     chain: &BeaconChain<T>,
-) -> Result<Vec<SignatureVerifiedBlock<T>>, BlockError> {
+) -> Result<Vec<SignatureVerifiedBlock<T>>, BlockError<T::EthSpec>> {
-    let (mut parent, slot) = if let Some(block) = chain_segment.first().map(|(_, block)| block) {
+    if chain_segment.is_empty() {
-        let parent = load_parent(&block.message, chain)?;
-        (parent, block.slot())
-    } else {
         return Ok(vec![]);
-    };
+    }
+
+    let (first_root, first_block) = chain_segment.remove(0);
+    let (mut parent, first_block) = load_parent(first_block, chain)?;
+    let slot = first_block.slot();
+    chain_segment.insert(0, (first_root, first_block));
 
     let highest_slot = chain_segment
         .last()
@@ -343,7 +345,7 @@ pub trait IntoFullyVerifiedBlock<T: BeaconChainTypes> {
     fn into_fully_verified_block(
         self,
         chain: &BeaconChain<T>,
-    ) -> Result<FullyVerifiedBlock<T>, BlockError>;
+    ) -> Result<FullyVerifiedBlock<T>, BlockError<T::EthSpec>>;
 
     fn block(&self) -> &SignedBeaconBlock<T::EthSpec>;
 }
@@ -356,7 +358,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
     pub fn new(
         block: SignedBeaconBlock<T::EthSpec>,
         chain: &BeaconChain<T>,
-    ) -> Result<Self, BlockError> {
+    ) -> Result<Self, BlockError<T::EthSpec>> {
         // Do not gossip or process blocks from future slots.
         let present_slot_with_tolerance = chain
             .slot_clock
@@ -384,7 +386,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             });
         }
 
-        let mut parent = load_parent(&block.message, chain)?;
+        let (mut parent, block) = load_parent(block, chain)?;
         let block_root = get_block_root(&block);
 
         let state = cheap_state_advance_to_obtain_committees(
@@ -453,7 +455,7 @@ impl<T: BeaconChainTypes> IntoFullyVerifiedBlock<T> for GossipVerifiedBlock<T> {
     fn into_fully_verified_block(
         self,
         chain: &BeaconChain<T>,
-    ) -> Result<FullyVerifiedBlock<T>, BlockError> {
+    ) -> Result<FullyVerifiedBlock<T>, BlockError<T::EthSpec>> {
         let fully_verified = SignatureVerifiedBlock::from_gossip_verified_block(self, chain)?;
         fully_verified.into_fully_verified_block(chain)
     }
@@ -471,8 +473,8 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
     pub fn new(
         block: SignedBeaconBlock<T::EthSpec>,
         chain: &BeaconChain<T>,
-    ) -> Result<Self, BlockError> {
+    ) -> Result<Self, BlockError<T::EthSpec>> {
-        let mut parent = load_parent(&block.message, chain)?;
+        let (mut parent, block) = load_parent(block, chain)?;
         let block_root = get_block_root(&block);
 
         let state = cheap_state_advance_to_obtain_committees(
@@ -503,7 +505,7 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
     pub fn from_gossip_verified_block(
         from: GossipVerifiedBlock<T>,
         chain: &BeaconChain<T>,
-    ) -> Result<Self, BlockError> {
+    ) -> Result<Self, BlockError<T::EthSpec>> {
         let mut parent = from.parent;
         let block = from.block;
 
@@ -536,12 +538,12 @@ impl<T: BeaconChainTypes> IntoFullyVerifiedBlock<T> for SignatureVerifiedBlock<T
     fn into_fully_verified_block(
         self,
         chain: &BeaconChain<T>,
-    ) -> Result<FullyVerifiedBlock<T>, BlockError> {
+    ) -> Result<FullyVerifiedBlock<T>, BlockError<T::EthSpec>> {
-        let block = self.block;
+        let (parent, block) = if let Some(parent) = self.parent {
-        let parent = self
+            (parent, self.block)
-            .parent
+        } else {
-            .map(Result::Ok)
+            load_parent(self.block, chain)?
-            .unwrap_or_else(|| load_parent(&block.message, chain))?;
+        };
 
         FullyVerifiedBlock::from_signature_verified_components(
             block,
@@ -562,7 +564,7 @@ impl<T: BeaconChainTypes> IntoFullyVerifiedBlock<T> for SignedBeaconBlock<T::Eth
     fn into_fully_verified_block(
         self,
         chain: &BeaconChain<T>,
-    ) -> Result<FullyVerifiedBlock<T>, BlockError> {
+    ) -> Result<FullyVerifiedBlock<T>, BlockError<T::EthSpec>> {
         SignatureVerifiedBlock::new(self, chain)?.into_fully_verified_block(chain)
     }
 
@@ -584,7 +586,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
         block_root: Hash256,
         parent: BeaconSnapshot<T::EthSpec>,
         chain: &BeaconChain<T>,
-    ) -> Result<Self, BlockError> {
+    ) -> Result<Self, BlockError<T::EthSpec>> {
         // Reject any block if its parent is not known to fork choice.
         //
         // A block that is not in fork choice is either:
@@ -600,7 +602,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
             .read()
             .contains_block(&block.parent_root())
         {
-            return Err(BlockError::ParentUnknown(block.parent_root()));
+            return Err(BlockError::ParentUnknown(Box::new(block)));
         }
 
         /*
@@ -749,7 +751,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
 fn check_block_against_finalized_slot<T: BeaconChainTypes>(
     block: &BeaconBlock<T::EthSpec>,
     chain: &BeaconChain<T>,
-) -> Result<(), BlockError> {
+) -> Result<(), BlockError<T::EthSpec>> {
     let finalized_slot = chain
         .head_info()?
         .finalized_checkpoint
@@ -777,7 +779,7 @@ pub fn check_block_relevancy<T: BeaconChainTypes>(
     signed_block: &SignedBeaconBlock<T::EthSpec>,
     block_root: Option<Hash256>,
     chain: &BeaconChain<T>,
-) -> Result<Hash256, BlockError> {
+) -> Result<Hash256, BlockError<T::EthSpec>> {
     let block = &signed_block.message;
 
     // Do not process blocks from the future.
@@ -830,12 +832,11 @@ pub fn get_block_root<E: EthSpec>(block: &SignedBeaconBlock<E>) -> Hash256 {
 ///
 /// Returns `Err(BlockError::ParentUnknown)` if the parent is not found, or if an error occurs
 /// whilst attempting the operation.
+#[allow(clippy::type_complexity)]
 fn load_parent<T: BeaconChainTypes>(
-    block: &BeaconBlock<T::EthSpec>,
+    block: SignedBeaconBlock<T::EthSpec>,
     chain: &BeaconChain<T>,
-) -> Result<BeaconSnapshot<T::EthSpec>, BlockError> {
+) -> Result<(BeaconSnapshot<T::EthSpec>, SignedBeaconBlock<T::EthSpec>), BlockError<T::EthSpec>> {
-    let db_read_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_READ);
-
     // Reject any block if its parent is not known to fork choice.
     //
     // A block that is not in fork choice is either:
@@ -846,50 +847,58 @@ fn load_parent<T: BeaconChainTypes>(
     //  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).
-    if !chain.fork_choice.read().contains_block(&block.parent_root) {
+    if !chain
-        return Err(BlockError::ParentUnknown(block.parent_root));
+        .fork_choice
+        .read()
+        .contains_block(&block.parent_root())
+    {
+        return Err(BlockError::ParentUnknown(Box::new(block)));
     }
 
-    // Load the parent block and state from disk, returning early if it's not available.
+    let db_read_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_READ);
-    let result = chain
+
+    let result = if let Some(snapshot) = chain
         .snapshot_cache
         .try_write_for(BLOCK_PROCESSING_CACHE_LOCK_TIMEOUT)
-        .and_then(|mut snapshot_cache| snapshot_cache.try_remove(block.parent_root))
+        .and_then(|mut snapshot_cache| snapshot_cache.try_remove(block.parent_root()))
-        .map(|snapshot| Ok(Some(snapshot)))
+    {
-        .unwrap_or_else(|| {
+        Ok((snapshot, block))
-            // Load the blocks parent block from the database, returning invalid if that block is not
+    } else {
-            // found.
+        // Load the blocks parent block from the database, returning invalid if that block is not
-            //
+        // found.
-            // We don't return a DBInconsistent error here since it's possible for a block to
+        //
-            // exist in fork choice but not in the database yet. In such a case we simply
+        // We don't return a DBInconsistent error here since it's possible for a block to
-            // indicate that we don't yet know the parent.
+        // exist in fork choice but not in the database yet. In such a case we simply
-            let parent_block = if let Some(block) = chain.get_block(&block.parent_root)? {
+        // indicate that we don't yet know the parent.
-                block
+        let root = block.parent_root();
-            } else {
+        let parent_block = if let Some(block) = chain
-                return Ok(None);
+            .get_block(&block.parent_root())
-            };
+            .map_err(BlockError::BeaconChainError)?
+        {
+            block
+        } else {
+            return Err(BlockError::ParentUnknown(Box::new(block)));
+        };
 
-            // Load the parent blocks state from the database, returning an error if it is not found.
+        // Load the parent blocks state from the database, returning an error if it is not found.
-            // It is an error because if we know the parent block we should also know the parent state.
+        // It is an error because if we know the parent block we should also know the parent state.
-            let parent_state_root = parent_block.state_root();
+        let parent_state_root = parent_block.state_root();
-            let parent_state = chain
+        let parent_state = chain
-                .get_state(&parent_state_root, Some(parent_block.slot()))?
+            .get_state(&parent_state_root, Some(parent_block.slot()))?
-                .ok_or_else(|| {
+            .ok_or_else(|| {
-                    BeaconChainError::DBInconsistent(format!(
+                BeaconChainError::DBInconsistent(format!("Missing state {:?}", parent_state_root))
-                        "Missing state {:?}",
+            })?;
-                        parent_state_root
-                    ))
-                })?;
 
-            Ok(Some(BeaconSnapshot {
+        Ok((
+            BeaconSnapshot {
                 beacon_block: parent_block,
-                beacon_block_root: block.parent_root,
+                beacon_block_root: root,
                 beacon_state: parent_state,
                 beacon_state_root: parent_state_root,
-            }))
+            },
-        })
+            block,
-        .map_err(BlockError::BeaconChainError)?
+        ))
-        .ok_or_else(|| BlockError::ParentUnknown(block.parent_root));
+    };
 
     metrics::stop_timer(db_read_timer);
 
@@ -911,7 +920,7 @@ fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
     state: &'a mut BeaconState<E>,
     block_slot: Slot,
     spec: &ChainSpec,
-) -> Result<Cow<'a, BeaconState<E>>, BlockError> {
+) -> Result<Cow<'a, BeaconState<E>>, BlockError<E>> {
     let block_epoch = block_slot.epoch(E::slots_per_epoch());
 
     if state.current_epoch() == block_epoch {
@@ -943,7 +952,7 @@ fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>(
 /// Obtains a read-locked `ValidatorPubkeyCache` from the `chain`.
 fn get_validator_pubkey_cache<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
-) -> Result<RwLockReadGuard<ValidatorPubkeyCache>, BlockError> {
+) -> Result<RwLockReadGuard<ValidatorPubkeyCache>, BlockError<T::EthSpec>> {
     chain
         .validator_pubkey_cache
         .try_read_for(VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT)

beacon_node/network/src/router/mod.rs

@@ -240,13 +240,15 @@ impl<T: BeaconChainTypes> Router<T> {
                 }
             }
             PubsubMessage::BeaconBlock(block) => {
-                match self.processor.should_forward_block(&peer_id, block) {
+                match self.processor.should_forward_block(block) {
                     Ok(verified_block) => {
                         info!(self.log, "New block received"; "slot" => verified_block.block.slot(), "hash" => verified_block.block_root.to_string());
                         self.propagate_message(id, peer_id.clone());
                         self.processor.on_block_gossip(peer_id, verified_block);
                     }
-                    Err(BlockError::ParentUnknown { .. }) => {} // performing a parent lookup
+                    Err(BlockError::ParentUnknown(block)) => {
+                        self.processor.on_unknown_parent(peer_id, block);
+                    }
                     Err(e) => {
                         // performing a parent lookup
                         warn!(self.log, "Could not verify block for gossip";
@@ -260,7 +262,7 @@ impl<T: BeaconChainTypes> Router<T> {
                     .processor
                     .verify_voluntary_exit_for_gossip(&peer_id, *exit)
                 {
-                    self.propagate_message(id, peer_id.clone());
+                    self.propagate_message(id, peer_id);
                     self.processor.import_verified_voluntary_exit(verified_exit);
                 }
             }
@@ -274,7 +276,7 @@ impl<T: BeaconChainTypes> Router<T> {
                     .processor
                     .verify_proposer_slashing_for_gossip(&peer_id, *proposer_slashing)
                 {
-                    self.propagate_message(id, peer_id.clone());
+                    self.propagate_message(id, peer_id);
                     self.processor
                         .import_verified_proposer_slashing(verified_proposer_slashing);
                 }
@@ -289,7 +291,7 @@ impl<T: BeaconChainTypes> Router<T> {
                     .processor
                     .verify_attester_slashing_for_gossip(&peer_id, *attester_slashing)
                 {
-                    self.propagate_message(id, peer_id.clone());
+                    self.propagate_message(id, peer_id);
                     self.processor
                         .import_verified_attester_slashing(verified_attester_slashing);
                 }

beacon_node/network/src/router/processor.rs

@@ -503,17 +503,17 @@ impl<T: BeaconChainTypes> Processor<T> {
     /// across the network.
     pub fn should_forward_block(
         &mut self,
-        peer_id: &PeerId,
         block: Box<SignedBeaconBlock<T::EthSpec>>,
-    ) -> Result<GossipVerifiedBlock<T>, BlockError> {
+    ) -> Result<GossipVerifiedBlock<T>, BlockError<T::EthSpec>> {
-        let result = self.chain.verify_block_for_gossip(*block.clone());
+        self.chain.verify_block_for_gossip(*block)
+    }
 
-        if let Err(BlockError::ParentUnknown(_)) = result {
+    pub fn on_unknown_parent(
-            // if we don't know the parent, start a parent lookup
+        &mut self,
-            // TODO: Modify the return to avoid the block clone.
+        peer_id: PeerId,
-            self.send_to_sync(SyncMessage::UnknownBlock(peer_id.clone(), block));
+        block: Box<SignedBeaconBlock<T::EthSpec>>,
-        }
+    ) {
-        result
+        self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block));
     }
 
     /// Process a gossip message declaring a new block.

beacon_node/network/src/sync/block_processor.rs

@@ -6,7 +6,7 @@ use eth2_libp2p::PeerId;
 use slog::{debug, error, trace, warn};
 use std::sync::{Arc, Weak};
 use tokio::sync::mpsc;
-use types::SignedBeaconBlock;
+use types::{EthSpec, SignedBeaconBlock};
 
 /// Id associated to a block processing request, either a batch or a single block.
 #[derive(Clone, Debug, PartialEq)]
@@ -178,12 +178,18 @@ fn run_fork_choice<T: BeaconChainTypes>(chain: Arc<BeaconChain<T>>, log: &slog::
 }
 
 /// Helper function to handle a `BlockError` from `process_chain_segment`
-fn handle_failed_chain_segment(error: BlockError, log: &slog::Logger) -> Result<(), String> {
+fn handle_failed_chain_segment<T: EthSpec>(
+    error: BlockError<T>,
+    log: &slog::Logger,
+) -> Result<(), String> {
     match error {
-        BlockError::ParentUnknown(parent) => {
+        BlockError::ParentUnknown(block) => {
             // blocks should be sequential and all parents should exist
 
-            Err(format!("Block has an unknown parent: {}", parent))
+            Err(format!(
+                "Block has an unknown parent: {}",
+                block.parent_root()
+            ))
         }
         BlockError::BlockIsAlreadyKnown => {
             // This can happen for many reasons. Head sync's can download multiples and parent