diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index fa4e0d6a9d..4acc7da012 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -69,10 +69,6 @@ use types::{ PublicKey, RelativeEpoch, SignedBeaconBlock, Slot, }; -mod block_processing_outcome; - -pub use block_processing_outcome::BlockProcessingOutcome; - /// Maximum block slot number. Block with slots bigger than this constant will NOT be processed. const MAXIMUM_BLOCK_SLOT_NUMBER: u64 = 4_294_967_296; // 2^32 diff --git a/beacon_node/beacon_chain/src/block_verification/block_processing_outcome.rs b/beacon_node/beacon_chain/src/block_verification/block_processing_outcome.rs deleted file mode 100644 index 8e38af4734..0000000000 --- a/beacon_node/beacon_chain/src/block_verification/block_processing_outcome.rs +++ /dev/null @@ -1,130 +0,0 @@ -use crate::{BeaconChainError, BlockError}; -use state_processing::BlockProcessingError; -use types::{Hash256, Slot}; - -/// This is a legacy object that is being kept around to reduce merge conflicts. -/// -/// TODO: As soon as this is merged into master, it should be removed as soon as possible. -#[derive(Debug, PartialEq)] -pub enum BlockProcessingOutcome { - /// Block was valid and imported into the block graph. - Processed { - block_root: Hash256, - }, - InvalidSignature, - /// The proposal signature in invalid. - ProposalSignatureInvalid, - /// The `block.proposal_index` is not known. - UnknownValidator(u64), - /// The parent block was unknown. - ParentUnknown(Hash256), - /// The block slot is greater than the present slot. - FutureSlot { - present_slot: Slot, - block_slot: Slot, - }, - /// The block state_root does not match the generated state. - StateRootMismatch { - block: Hash256, - local: Hash256, - }, - /// The block was a genesis block, these blocks cannot be re-imported. - GenesisBlock, - /// The slot is finalized, no need to import. - WouldRevertFinalizedSlot { - block_slot: Slot, - finalized_slot: Slot, - }, - /// Block is already known, no need to re-import. - BlockIsAlreadyKnown, - /// A block for this proposer and slot has already been observed. - RepeatProposal { - proposer: u64, - slot: Slot, - }, - /// The block slot exceeds the MAXIMUM_BLOCK_SLOT_NUMBER. - BlockSlotLimitReached, - /// The provided block is from an earlier slot than its parent. - BlockIsNotLaterThanParent { - block_slot: Slot, - state_slot: Slot, - }, - /// The `BeaconBlock` has a `proposer_index` that does not match the index we computed locally. - /// - /// The block is invalid. - IncorrectBlockProposer { - block: u64, - local_shuffling: u64, - }, - /// At least one block in the chain segement did not have it's parent root set to the root of - /// the prior block. - NonLinearParentRoots, - /// The slots of the blocks in the chain segment were not strictly increasing. I.e., a child - /// had lower slot than a parent. - NonLinearSlots, - /// The block could not be applied to the state, it is invalid. - PerBlockProcessingError(BlockProcessingError), -} - -impl BlockProcessingOutcome { - pub fn shim( - result: Result, - ) -> Result { - match result { - Ok(block_root) => Ok(BlockProcessingOutcome::Processed { block_root }), - Err(BlockError::ParentUnknown(root)) => Ok(BlockProcessingOutcome::ParentUnknown(root)), - Err(BlockError::FutureSlot { - present_slot, - block_slot, - }) => Ok(BlockProcessingOutcome::FutureSlot { - present_slot, - block_slot, - }), - Err(BlockError::StateRootMismatch { block, local }) => { - Ok(BlockProcessingOutcome::StateRootMismatch { block, local }) - } - Err(BlockError::GenesisBlock) => Ok(BlockProcessingOutcome::GenesisBlock), - Err(BlockError::WouldRevertFinalizedSlot { - block_slot, - finalized_slot, - }) => Ok(BlockProcessingOutcome::WouldRevertFinalizedSlot { - block_slot, - finalized_slot, - }), - Err(BlockError::BlockIsAlreadyKnown) => Ok(BlockProcessingOutcome::BlockIsAlreadyKnown), - Err(BlockError::RepeatProposal { proposer, slot }) => { - Ok(BlockProcessingOutcome::RepeatProposal { proposer, slot }) - } - Err(BlockError::BlockSlotLimitReached) => { - Ok(BlockProcessingOutcome::BlockSlotLimitReached) - } - Err(BlockError::ProposalSignatureInvalid) => { - Ok(BlockProcessingOutcome::ProposalSignatureInvalid) - } - Err(BlockError::UnknownValidator(i)) => Ok(BlockProcessingOutcome::UnknownValidator(i)), - Err(BlockError::InvalidSignature) => Ok(BlockProcessingOutcome::InvalidSignature), - Err(BlockError::BlockIsNotLaterThanParent { - block_slot, - state_slot, - }) => Ok(BlockProcessingOutcome::BlockIsNotLaterThanParent { - block_slot, - state_slot, - }), - Err(BlockError::IncorrectBlockProposer { - block, - local_shuffling, - }) => Ok(BlockProcessingOutcome::IncorrectBlockProposer { - block, - local_shuffling, - }), - Err(BlockError::NonLinearParentRoots) => { - Ok(BlockProcessingOutcome::NonLinearParentRoots) - } - Err(BlockError::NonLinearSlots) => Ok(BlockProcessingOutcome::NonLinearSlots), - Err(BlockError::PerBlockProcessingError(e)) => { - Ok(BlockProcessingOutcome::PerBlockProcessingError(e)) - } - Err(BlockError::BeaconChainError(e)) => Err(e), - } - } -} diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 100ee58895..3c597bc72e 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -35,7 +35,7 @@ pub use self::beacon_snapshot::BeaconSnapshot; pub use self::errors::{BeaconChainError, BlockProductionError}; pub use attestation_verification::Error as AttestationError; pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceStoreError}; -pub use block_verification::{BlockError, BlockProcessingOutcome, GossipVerifiedBlock}; +pub use block_verification::{BlockError, GossipVerifiedBlock}; pub use eth1_chain::{Eth1Chain, Eth1ChainBackend}; pub use events::EventHandler; pub use metrics::scrape_for_metrics; diff --git a/beacon_node/network/src/router/processor.rs b/beacon_node/network/src/router/processor.rs index 74e641f5bd..35f3258e5c 100644 --- a/beacon_node/network/src/router/processor.rs +++ b/beacon_node/network/src/router/processor.rs @@ -6,8 +6,8 @@ use beacon_chain::{ VerifiedUnaggregatedAttestation, }, observed_operations::ObservationOutcome, - BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, BlockProcessingOutcome, - ForkChoiceError, GossipVerifiedBlock, + BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, ForkChoiceError, + GossipVerifiedBlock, }; use eth2_libp2p::rpc::*; use eth2_libp2p::{NetworkGlobals, PeerId, PeerRequestId, Request, Response}; @@ -103,7 +103,7 @@ impl Processor { debug!( self.log, "Sending Status Request"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "fork_digest" => format!("{:?}", status_message.fork_digest), "finalized_root" => format!("{:?}", status_message.finalized_root), "finalized_epoch" => format!("{:?}", status_message.finalized_epoch), @@ -127,7 +127,7 @@ impl Processor { debug!( self.log, "Received Status Request"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "fork_digest" => format!("{:?}", status.fork_digest), "finalized_root" => format!("{:?}", status.finalized_root), "finalized_epoch" => format!("{:?}", status.finalized_epoch), @@ -206,7 +206,7 @@ impl Processor { // clock is incorrect. debug!( self.log, "Handshake Failure"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "reason" => "different system clocks or genesis time" ); self.network @@ -226,7 +226,7 @@ impl Processor { // Therefore, the node is on a different chain and we should not communicate with them. debug!( self.log, "Handshake Failure"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "reason" => "different finalized chain" ); self.network @@ -248,7 +248,7 @@ impl Processor { debug!( self.log, "NaivePeer"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "reason" => "lower finalized epoch" ); } else if self @@ -259,7 +259,7 @@ impl Processor { { debug!( self.log, "Peer with known chain found"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "remote_head_slot" => remote.head_slot, "remote_latest_finalized_epoch" => remote.finalized_epoch, ); @@ -274,7 +274,7 @@ impl Processor { // head that are worth downloading. debug!( self.log, "UsefulPeer"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "local_finalized_epoch" => local.finalized_epoch, "remote_latest_finalized_epoch" => remote.finalized_epoch, ); @@ -302,7 +302,7 @@ impl Processor { debug!( self.log, "Peer requested unknown block"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "request_root" => format!("{:}", root), ); } @@ -310,7 +310,7 @@ impl Processor { debug!( self.log, "Received BlocksByRoot Request"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "requested" => request.block_roots.len(), "returned" => send_block_count, ); @@ -422,7 +422,7 @@ impl Processor { debug!( self.log, "BlocksByRange Response Sent"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "msg" => "Failed to return all requested blocks", "start_slot" => req.start_slot, "current_slot" => self.chain.slot().unwrap_or_else(|_| Slot::from(0_u64)).as_u64(), @@ -432,7 +432,7 @@ impl Processor { debug!( self.log, "Sending BlocksByRange Response"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "start_slot" => req.start_slot, "current_slot" => self.chain.slot().unwrap_or_else(|_| Slot::from(0_u64)).as_u64(), "requested" => req.count, @@ -455,7 +455,7 @@ impl Processor { trace!( self.log, "Received BlocksByRange Response"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), ); if let RequestId::Sync(id) = request_id { @@ -482,7 +482,7 @@ impl Processor { trace!( self.log, "Received BlocksByRoot Response"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), ); if let RequestId::Sync(id) = request_id { @@ -527,59 +527,55 @@ impl Processor { verified_block: GossipVerifiedBlock, ) -> bool { let block = Box::new(verified_block.block.clone()); - match BlockProcessingOutcome::shim(self.chain.process_block(verified_block)) { - Ok(outcome) => match outcome { - BlockProcessingOutcome::Processed { .. } => { - trace!(self.log, "Gossipsub block processed"; - "peer_id" => format!("{:?}",peer_id)); - - // TODO: It would be better if we can run this _after_ we publish the block to - // reduce block propagation latency. - // - // The `MessageHandler` would be the place to put this, however it doesn't seem - // to have a reference to the `BeaconChain`. I will leave this for future - // works. - match self.chain.fork_choice() { - Ok(()) => trace!( - self.log, - "Fork choice success"; - "location" => "block gossip" - ), - Err(e) => error!( - self.log, - "Fork choice failed"; - "error" => format!("{:?}", e), - "location" => "block gossip" - ), - } - } - BlockProcessingOutcome::ParentUnknown { .. } => { - // Inform the sync manager to find parents for this block - // This should not occur. It should be checked by `should_forward_block` - error!(self.log, "Block with unknown parent attempted to be processed"; - "peer_id" => format!("{:?}",peer_id)); - self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block)); - } - other => { - warn!( - self.log, - "Invalid gossip beacon block"; - "outcome" => format!("{:?}", other), - "block root" => format!("{}", block.canonical_root()), - "block slot" => block.slot() - ); - trace!( - self.log, - "Invalid gossip beacon block ssz"; - "ssz" => format!("0x{}", hex::encode(block.as_ssz_bytes())), - ); - } - }, - Err(_) => { - // error is logged during the processing therefore no error is logged here + match self.chain.process_block(verified_block) { + Ok(_block_root) => { trace!( self.log, - "Erroneous gossip beacon block ssz"; + "Gossipsub block processed"; + "peer_id" => peer_id.to_string() + ); + + // TODO: It would be better if we can run this _after_ we publish the block to + // reduce block propagation latency. + // + // The `MessageHandler` would be the place to put this, however it doesn't seem + // to have a reference to the `BeaconChain`. I will leave this for future + // works. + match self.chain.fork_choice() { + Ok(()) => trace!( + self.log, + "Fork choice success"; + "location" => "block gossip" + ), + Err(e) => error!( + self.log, + "Fork choice failed"; + "error" => format!("{:?}", e), + "location" => "block gossip" + ), + } + } + Err(BlockError::ParentUnknown { .. }) => { + // Inform the sync manager to find parents for this block + // This should not occur. It should be checked by `should_forward_block` + error!( + self.log, + "Block with unknown parent attempted to be processed"; + "peer_id" => peer_id.to_string() + ); + self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block)); + } + other => { + warn!( + self.log, + "Invalid gossip beacon block"; + "outcome" => format!("{:?}", other), + "block root" => format!("{}", block.canonical_root()), + "block slot" => block.slot() + ); + trace!( + self.log, + "Invalid gossip beacon block ssz"; "ssz" => format!("0x{}", hex::encode(block.as_ssz_bytes())), ); } @@ -601,7 +597,7 @@ impl Processor { self.log, "Invalid attestation from network"; "block" => format!("{}", beacon_block_root), - "peer_id" => format!("{:?}", peer_id), + "peer_id" => peer_id.to_string(), "type" => format!("{:?}", attestation_type), ); @@ -707,7 +703,7 @@ impl Processor { debug!( self.log, "Attestation for unknown block"; - "peer_id" => format!("{:?}", peer_id), + "peer_id" => peer_id.to_string(), "block" => format!("{}", beacon_block_root) ); // we don't know the block, get the sync manager to handle the block lookup @@ -790,7 +786,7 @@ impl Processor { error!( self.log, "Unable to validate aggregate"; - "peer_id" => format!("{:?}", peer_id), + "peer_id" => peer_id.to_string(), "error" => format!("{:?}", e), ); } @@ -837,7 +833,7 @@ impl Processor { self.log, "Attestation invalid for op pool"; "reason" => format!("{:?}", e), - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "beacon_block_root" => format!("{:?}", beacon_block_root) ) } @@ -887,7 +883,7 @@ impl Processor { self.log, "Attestation invalid for agg pool"; "reason" => format!("{:?}", e), - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "beacon_block_root" => format!("{:?}", beacon_block_root) ) } @@ -915,7 +911,7 @@ impl Processor { self.log, "Attestation invalid for fork choice"; "reason" => format!("{:?}", e), - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "beacon_block_root" => format!("{:?}", beacon_block_root) ) } @@ -923,7 +919,7 @@ impl Processor { self.log, "Error applying attestation to fork choice"; "reason" => format!("{:?}", e), - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "beacon_block_root" => format!("{:?}", beacon_block_root) ), } @@ -947,7 +943,7 @@ impl Processor { self.log, "Dropping exit for already exiting validator"; "validator_index" => validator_index, - "peer" => format!("{:?}", peer_id) + "peer" => peer_id.to_string() ); None } @@ -956,7 +952,7 @@ impl Processor { self.log, "Dropping invalid exit"; "validator_index" => validator_index, - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "error" => format!("{:?}", e) ); None @@ -994,7 +990,7 @@ impl Processor { "Dropping proposer slashing"; "reason" => "Already seen a proposer slashing for that validator", "validator_index" => validator_index, - "peer" => format!("{:?}", peer_id) + "peer" => peer_id.to_string() ); None } @@ -1003,7 +999,7 @@ impl Processor { self.log, "Dropping invalid proposer slashing"; "validator_index" => validator_index, - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "error" => format!("{:?}", e) ); None @@ -1038,7 +1034,7 @@ impl Processor { self.log, "Dropping attester slashing"; "reason" => "Slashings already known for all slashed validators", - "peer" => format!("{:?}", peer_id) + "peer" => peer_id.to_string() ); None } @@ -1046,7 +1042,7 @@ impl Processor { debug!( self.log, "Dropping invalid attester slashing"; - "peer" => format!("{:?}", peer_id), + "peer" => peer_id.to_string(), "error" => format!("{:?}", e) ); None diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 3040fc2451..2ad6d7b316 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -39,7 +39,7 @@ use super::peer_sync_info::{PeerSyncInfo, PeerSyncType}; use super::range_sync::{BatchId, ChainId, RangeSync, EPOCHS_PER_BATCH}; use super::RequestId; use crate::service::NetworkMessage; -use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; +use beacon_chain::{BeaconChain, BeaconChainTypes, BlockError}; use eth2_libp2p::rpc::{methods::MAX_REQUEST_BLOCKS, BlocksByRootRequest}; use eth2_libp2p::types::NetworkGlobals; use eth2_libp2p::PeerId; @@ -396,42 +396,38 @@ impl SyncManager { } // we have the correct block, try and process it - match BlockProcessingOutcome::shim(self.chain.process_block(block.clone())) { - Ok(outcome) => { - match outcome { - BlockProcessingOutcome::Processed { block_root } => { - info!(self.log, "Processed block"; "block" => format!("{}", block_root)); + match self.chain.process_block(block.clone()) { + Ok(block_root) => { + info!(self.log, "Processed block"; "block" => format!("{}", block_root)); - match self.chain.fork_choice() { - Ok(()) => trace!( - self.log, - "Fork choice success"; - "location" => "single block" - ), - Err(e) => error!( - self.log, - "Fork choice failed"; - "error" => format!("{:?}", e), - "location" => "single block" - ), - } - } - BlockProcessingOutcome::ParentUnknown { .. } => { - // We don't know of the blocks parent, begin a parent lookup search - self.add_unknown_block(peer_id, block); - } - BlockProcessingOutcome::BlockIsAlreadyKnown => { - trace!(self.log, "Single block lookup already known"); - } - _ => { - warn!(self.log, "Single block lookup failed"; "outcome" => format!("{:?}", outcome)); - self.network.downvote_peer(peer_id); - } + match self.chain.fork_choice() { + Ok(()) => trace!( + self.log, + "Fork choice success"; + "location" => "single block" + ), + Err(e) => error!( + self.log, + "Fork choice failed"; + "error" => format!("{:?}", e), + "location" => "single block" + ), } } - Err(e) => { + Err(BlockError::ParentUnknown { .. }) => { + // We don't know of the blocks parent, begin a parent lookup search + self.add_unknown_block(peer_id, block); + } + Err(BlockError::BlockIsAlreadyKnown) => { + trace!(self.log, "Single block lookup already known"); + } + Err(BlockError::BeaconChainError(e)) => { warn!(self.log, "Unexpected block processing error"; "error" => format!("{:?}", e)); } + outcome => { + warn!(self.log, "Single block lookup failed"; "outcome" => format!("{:?}", outcome)); + self.network.downvote_peer(peer_id); + } } } @@ -645,16 +641,15 @@ impl SyncManager { .downloaded_blocks .pop() .expect("There is always at least one block in the queue"); - match BlockProcessingOutcome::shim(self.chain.process_block(newest_block.clone())) { - Ok(BlockProcessingOutcome::ParentUnknown { .. }) => { + match self.chain.process_block(newest_block.clone()) { + Err(BlockError::ParentUnknown { .. }) => { // need to keep looking for parents // add the block back to the queue and continue the search parent_request.downloaded_blocks.push(newest_block); self.request_parent(parent_request); return; } - Ok(BlockProcessingOutcome::Processed { .. }) - | Ok(BlockProcessingOutcome::BlockIsAlreadyKnown { .. }) => { + Ok(_) | Err(BlockError::BlockIsAlreadyKnown { .. }) => { spawn_block_processor( Arc::downgrade(&self.chain), ProcessId::ParentLookup(parent_request.last_submitted_peer.clone()), @@ -663,7 +658,7 @@ impl SyncManager { self.log.clone(), ); } - Ok(outcome) => { + Err(outcome) => { // all else we consider the chain a failure and downvote the peer that sent // us the last block warn!( @@ -675,16 +670,6 @@ impl SyncManager { .downvote_peer(parent_request.last_submitted_peer); return; } - Err(e) => { - warn!( - self.log, "Parent chain processing error. Downvoting peer"; - "error" => format!("{:?}", e), - "last_peer" => format!("{:?}", parent_request.last_submitted_peer), - ); - self.network - .downvote_peer(parent_request.last_submitted_peer); - return; - } } } }