From 138c0cf7f09fdbf644b01651dffc76470bbe751f Mon Sep 17 00:00:00 2001 From: divma Date: Thu, 6 Aug 2020 04:29:17 +0000 Subject: [PATCH] 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 | 18 +- .../beacon_chain/src/block_verification.rs | 157 +++++++++--------- beacon_node/network/src/router/mod.rs | 12 +- beacon_node/network/src/router/processor.rs | 18 +- .../network/src/sync/block_processor.rs | 14 +- 5 files changed, 118 insertions(+), 101 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 4e6329ee73a..433198e4ed4 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/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 { /// 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, }, } @@ -1153,7 +1153,7 @@ impl BeaconChain { pub fn process_chain_segment( &self, chain_segment: Vec>, - ) -> ChainSegmentResult { + ) -> ChainSegmentResult { let mut filtered_chain_segment = Vec::with_capacity(chain_segment.len()); let mut imported_blocks = 0; @@ -1286,7 +1286,7 @@ impl BeaconChain { pub fn verify_block_for_gossip( &self, block: SignedBeaconBlock, - ) -> Result, BlockError> { + ) -> Result, BlockError> { 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 BeaconChain { pub fn process_block>( &self, unverified_block: B, - ) -> Result { + ) -> Result> { // Start the Prometheus timer. let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES); @@ -1343,7 +1343,7 @@ impl BeaconChain { let block = unverified_block.block().clone(); // A small closure to group the verification and import errors. - let import_block = |unverified_block: B| -> Result { + let import_block = |unverified_block: B| -> Result> { let fully_verified = unverified_block.into_fully_verified_block(self)?; self.import_block(fully_verified) }; @@ -1411,7 +1411,7 @@ impl BeaconChain { fn import_block( &self, fully_verified_block: FullyVerifiedBlock, - ) -> Result { + ) -> Result> { 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 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/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index dfad1b85f6f..f4581d77ca9 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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 { /// 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>), /// The block slot is greater than the present slot. /// /// ## Peer scoring @@ -199,7 +199,7 @@ pub enum BlockError { BeaconChainError(BeaconChainError), } -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 @@ -216,25 +216,25 @@ 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)) } @@ -251,15 +251,17 @@ impl From 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( - chain_segment: Vec<(Hash256, SignedBeaconBlock)>, + mut chain_segment: Vec<(Hash256, SignedBeaconBlock)>, chain: &BeaconChain, -) -> Result>, BlockError> { - let (mut parent, slot) = if let Some(block) = chain_segment.first().map(|(_, block)| block) { - let parent = load_parent(&block.message, chain)?; - (parent, block.slot()) - } else { +) -> Result>, BlockError> { + if chain_segment.is_empty() { 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 { fn into_fully_verified_block( self, chain: &BeaconChain, - ) -> Result, BlockError>; + ) -> Result, BlockError>; fn block(&self) -> &SignedBeaconBlock; } @@ -356,7 +358,7 @@ impl GossipVerifiedBlock { pub fn new( block: SignedBeaconBlock, chain: &BeaconChain, - ) -> Result { + ) -> Result> { // Do not gossip or process blocks from future slots. let present_slot_with_tolerance = chain .slot_clock @@ -384,7 +386,7 @@ impl GossipVerifiedBlock { }); } - 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 IntoFullyVerifiedBlock for GossipVerifiedBlock { fn into_fully_verified_block( self, chain: &BeaconChain, - ) -> Result, BlockError> { + ) -> Result, BlockError> { let fully_verified = SignatureVerifiedBlock::from_gossip_verified_block(self, chain)?; fully_verified.into_fully_verified_block(chain) } @@ -471,8 +473,8 @@ impl SignatureVerifiedBlock { pub fn new( block: SignedBeaconBlock, chain: &BeaconChain, - ) -> Result { - let mut parent = load_parent(&block.message, chain)?; + ) -> Result> { + 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 SignatureVerifiedBlock { pub fn from_gossip_verified_block( from: GossipVerifiedBlock, chain: &BeaconChain, - ) -> Result { + ) -> Result> { let mut parent = from.parent; let block = from.block; @@ -536,12 +538,12 @@ impl IntoFullyVerifiedBlock for SignatureVerifiedBlock, - ) -> Result, BlockError> { - let block = self.block; - let parent = self - .parent - .map(Result::Ok) - .unwrap_or_else(|| load_parent(&block.message, chain))?; + ) -> Result, BlockError> { + let (parent, block) = if let Some(parent) = self.parent { + (parent, self.block) + } else { + load_parent(self.block, chain)? + }; FullyVerifiedBlock::from_signature_verified_components( block, @@ -562,7 +564,7 @@ impl IntoFullyVerifiedBlock for SignedBeaconBlock, - ) -> Result, BlockError> { + ) -> Result, BlockError> { 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, chain: &BeaconChain, - ) -> Result { + ) -> Result> { // 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( block: &BeaconBlock, chain: &BeaconChain, -) -> Result<(), BlockError> { +) -> Result<(), BlockError> { let finalized_slot = chain .head_info()? .finalized_checkpoint @@ -777,7 +779,7 @@ pub fn check_block_relevancy( signed_block: &SignedBeaconBlock, block_root: Option, chain: &BeaconChain, -) -> Result { +) -> Result> { let block = &signed_block.message; // Do not process blocks from the future. @@ -830,12 +832,11 @@ pub fn get_block_root(block: &SignedBeaconBlock) -> 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( - block: &BeaconBlock, + block: SignedBeaconBlock, chain: &BeaconChain, -) -> Result, BlockError> { - let db_read_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_READ); - +) -> Result<(BeaconSnapshot, SignedBeaconBlock), BlockError> { // 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( // 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) { - return Err(BlockError::ParentUnknown(block.parent_root)); + if !chain + .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 result = chain + let db_read_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_READ); + + 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)) - .map(|snapshot| Ok(Some(snapshot))) - .unwrap_or_else(|| { - // 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 - // indicate that we don't yet know the parent. - let parent_block = if let Some(block) = chain.get_block(&block.parent_root)? { - block - } else { - return Ok(None); - }; + .and_then(|mut snapshot_cache| snapshot_cache.try_remove(block.parent_root())) + { + Ok((snapshot, block)) + } else { + // 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 + // indicate that we don't yet know the parent. + let root = block.parent_root(); + let parent_block = if let Some(block) = chain + .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. - // 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 = chain - .get_state(&parent_state_root, Some(parent_block.slot()))? - .ok_or_else(|| { - BeaconChainError::DBInconsistent(format!( - "Missing state {:?}", - parent_state_root - )) - })?; - - Ok(Some(BeaconSnapshot { + // 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. + let parent_state_root = parent_block.state_root(); + let parent_state = chain + .get_state(&parent_state_root, Some(parent_block.slot()))? + .ok_or_else(|| { + BeaconChainError::DBInconsistent(format!("Missing state {:?}", parent_state_root)) + })?; + + 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, - })) - }) - .map_err(BlockError::BeaconChainError)? - .ok_or_else(|| BlockError::ParentUnknown(block.parent_root)); + }, + block, + )) + }; metrics::stop_timer(db_read_timer); @@ -911,7 +920,7 @@ fn cheap_state_advance_to_obtain_committees<'a, E: EthSpec>( state: &'a mut BeaconState, block_slot: Slot, spec: &ChainSpec, -) -> Result>, BlockError> { +) -> Result>, BlockError> { 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( chain: &BeaconChain, -) -> Result, BlockError> { +) -> Result, BlockError> { chain .validator_pubkey_cache .try_read_for(VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT) diff --git a/beacon_node/network/src/router/mod.rs b/beacon_node/network/src/router/mod.rs index dd6ecf55e02..e487330f8d6 100644 --- a/beacon_node/network/src/router/mod.rs +++ b/beacon_node/network/src/router/mod.rs @@ -240,13 +240,15 @@ impl Router { } } 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 Router { .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 Router { .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 Router { .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); } diff --git a/beacon_node/network/src/router/processor.rs b/beacon_node/network/src/router/processor.rs index 23199f86ed4..9d11e785445 100644 --- a/beacon_node/network/src/router/processor.rs +++ b/beacon_node/network/src/router/processor.rs @@ -503,17 +503,17 @@ impl Processor { /// across the network. pub fn should_forward_block( &mut self, - peer_id: &PeerId, block: Box>, - ) -> Result, BlockError> { - let result = self.chain.verify_block_for_gossip(*block.clone()); + ) -> Result, BlockError> { + self.chain.verify_block_for_gossip(*block) + } - if let Err(BlockError::ParentUnknown(_)) = result { - // if we don't know the parent, start a parent lookup - // TODO: Modify the return to avoid the block clone. - self.send_to_sync(SyncMessage::UnknownBlock(peer_id.clone(), block)); - } - result + pub fn on_unknown_parent( + &mut self, + peer_id: PeerId, + block: Box>, + ) { + self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block)); } /// Process a gossip message declaring a new block. diff --git a/beacon_node/network/src/sync/block_processor.rs b/beacon_node/network/src/sync/block_processor.rs index 4e47baa7919..a4fe5b418ff 100644 --- a/beacon_node/network/src/sync/block_processor.rs +++ b/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(chain: Arc>, 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( + error: BlockError, + 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