From 74baeb4d08a958890b85a53ed93c6a382623b60b Mon Sep 17 00:00:00 2001 From: Age Manning Date: Mon, 2 Sep 2019 05:38:11 +1000 Subject: [PATCH 1/9] WIP - Upgrade Sync algorithm --- beacon_node/network/Cargo.toml | 1 + beacon_node/network/src/sync/manager.rs | 188 +++++++++++++++++--- beacon_node/network/src/sync/simple_sync.rs | 23 ++- 3 files changed, 180 insertions(+), 32 deletions(-) diff --git a/beacon_node/network/Cargo.toml b/beacon_node/network/Cargo.toml index dc08bd311..06fc06dde 100644 --- a/beacon_node/network/Cargo.toml +++ b/beacon_node/network/Cargo.toml @@ -19,3 +19,4 @@ futures = "0.1.25" error-chain = "0.12.0" tokio = "0.1.16" parking_lot = "0.9.0" +smallvec = "0.6.10" diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index b81da0991..9b2d780f4 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -1,33 +1,110 @@ +//! The `ImportManager` facilities the block syncing logic of lighthouse. The current networking +//! specification provides two methods from which to obtain blocks from peers. The `BeaconBlocks` +//! request and the `RecentBeaconBlocks` request. The former is used to obtain a large number of +//! blocks and the latter allows for searching for blocks given a block-hash. +//! +//! These two RPC methods are designed for two type of syncing. +//! - Long range (batch) sync, when a client is out of date and needs to the latest head. +//! - Parent lookup - when a peer provides us a block whose parent is unknown to us. +//! +//! Both of these syncing strategies are built into the `ImportManager`. +//! +//! +//! Currently the long-range (batch) syncing method functions by opportunistically downloading +//! batches blocks from all peers who know about a chain that we do not. When a new peer connects +//! which has a later head that is greater than `SLOT_IMPORT_TOLERANCE` from our current head slot, +//! the manager's state becomes `Syncing` and begins a batch syncing process with this peer. If +//! further peers connect, this process is run in parallel with those peers, until our head is +//! within `SLOT_IMPORT_TOLERANCE` of all connected peers. +//! +//! Batch Syncing +//! +//! This syncing process start by requesting `MAX_BLOCKS_PER_REQUEST` blocks from a peer with an +//! unknown chain (with a greater slot height) starting from our current head slot. If the earliest +//! block returned is known to us, then the group of blocks returned form part of a known chain, +//! and we process this batch of blocks, before requesting more batches forward and processing +//! those in turn until we reach the peer's chain's head. If the first batch doesn't contain a +//! block we know of, we must iteratively request blocks backwards (until our latest finalized head +//! slot) until we find a common ancestor before we can start processing the blocks. If no common +//! ancestor is found, the peer has a chain which is not part of our finalized head slot and we +//! drop the peer and the downloaded blocks. +//! Once we are fully synced with all known peers, the state of the manager becomes `Regular` which +//! then allows for parent lookups of propagated blocks. +//! +//! A schematic version of this logic with two chain variations looks like the following. +//! +//! |----------------------|---------------------------------| +//! ^finalized head ^current local head ^remotes head +//! +//! +//! An example of the remotes chain diverging before our current head. +//! |---------------------------| +//! ^---------------------------------------------| +//! ^chain diverges |initial batch| ^remotes head +//! +//! In this example, we cannot process the initial batch as it is not on a known chain. We must +//! then backwards sync until we reach a common chain to begin forwarding batch syncing. +//! +//! +//! Parent Lookup +//! +//! When a block with an unknown parent is received and we are in `Regular` sync mode, the block is +//! queued for lookup. A round-robin approach is used to request the parent from the known list of +//! fully sync'd peers. If `PARENT_FAIL_TOLERANCE` attempts at requesting the block fails, we +//! drop the propagated block and downvote the peer that sent it to us. + use super::simple_sync::{PeerSyncInfo, FUTURE_SLOT_TOLERANCE}; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; use eth2_libp2p::rpc::methods::*; use eth2_libp2p::rpc::RequestId; use eth2_libp2p::PeerId; use slog::{debug, info, trace, warn, Logger}; +use smallvec::SmallVec; use std::collections::{HashMap, HashSet}; use std::ops::{Add, Sub}; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use types::{BeaconBlock, EthSpec, Hash256, Slot}; +/// Blocks are downloaded in batches from peers. This constant specifies how many blocks per batch +/// is requested. Currently the value is small for testing. This will be incremented for +/// production. const MAX_BLOCKS_PER_REQUEST: u64 = 10; -/// The number of slots that we can import blocks ahead of us, before going into full Sync mode. +/// The number of slots ahead of us that is allowed before requesting a long-range (batch) Sync +/// from a peer. If a peer is within this tolerance (forwards or backwards), it is treated as a +/// fully sync'd peer. const SLOT_IMPORT_TOLERANCE: usize = 10; +/// How many attempts we try to find a parent of a block before we give up trying . const PARENT_FAIL_TOLERANCE: usize = 3; +/// The maximum depth we will search for a parent block. In principle we should have sync'd any +/// canonical chain to its head once the peer connects. A chain should not appear where it's depth +/// is further back than the most recent head slot. const PARENT_DEPTH_TOLERANCE: usize = SLOT_IMPORT_TOLERANCE * 2; #[derive(PartialEq)] +/// The current state of a block or batches lookup. enum BlockRequestsState { + /// The object is queued to be downloaded from a peer but has not yet been requested. Queued, + /// The batch or parent has been requested with the `RequestId` and we are awaiting a response. Pending(RequestId), - Complete, + /// The downloaded blocks are ready to be processed by the beacon chain. For a batch process + /// this means we have found a common chain. + ReadyToProcess, + /// A failure has occurred and we will drop and downvote the peer that caused the request. Failed, } +/// `BlockRequests` keep track of the long-range (batch) sync process per peer. struct BlockRequests { + /// The peer's head slot and the target of this batch download. target_head_slot: Slot, + /// The peer's head root, used to specify which chain of blocks we are downloading from the + /// blocks. target_head_root: Hash256, + /// The blocks that we have currently downloaded from the peer that are yet to be processed. downloaded_blocks: Vec>, + /// The current state of this batch request. state: BlockRequestsState, /// Specifies whether the current state is syncing forwards or backwards. forward_sync: bool, @@ -35,16 +112,22 @@ struct BlockRequests { current_start_slot: Slot, } +/// Maintains a sequential list of parents to lookup and the lookup's current state. struct ParentRequests { + /// The blocks that have currently been downloaded. downloaded_blocks: Vec>, + /// The number of failed attempts to retrieve a parent block. If too many attempts occur, this + /// lookup is failed and rejected. failed_attempts: usize, - last_submitted_peer: PeerId, // to downvote the submitting peer. + /// The peer who last submitted a block. If the chain ends or fails, this is the peer that is + /// downvoted. + last_submitted_peer: PeerId, + /// The current state of the parent lookup. state: BlockRequestsState, } impl BlockRequests { - // gets the start slot for next batch - // last block slot downloaded plus 1 + /// Gets the next start slot for a batch and transitions the state to a Queued state. fn update_start_slot(&mut self) { if self.forward_sync { self.current_start_slot += Slot::from(MAX_BLOCKS_PER_REQUEST); @@ -56,58 +139,104 @@ impl BlockRequests { } #[derive(PartialEq, Debug, Clone)] +/// The current state of the `ImportManager`. enum ManagerState { + /// The manager is performing a long-range (batch) sync. In this mode, parent lookups are + /// disabled. Syncing, + /// The manager is up to date with all known peers and is connected to at least one + /// fully-syncing peer. In this state, parent lookups are enabled. Regular, + /// No useful peers are connected. Long-range sync's cannot proceed and we have no useful + /// peers to download parents for. More peers need to be connected before we can proceed. Stalled, } +/// The output states that can occur from driving (polling) the manager state machine. pub(crate) enum ImportManagerOutcome { + /// There is no further work to complete. The manager is waiting for further input. Idle, + /// A `BeaconBlocks` request is required. RequestBlocks { peer_id: PeerId, request_id: RequestId, request: BeaconBlocksRequest, }, + /// A `RecentBeaconBlocks` request is required. + RecentRequest(PeerId, RecentBeaconBlocksRequest), /// Updates information with peer via requesting another HELLO handshake. Hello(PeerId), - RecentRequest(PeerId, RecentBeaconBlocksRequest), + /// A peer has caused a punishable error and should be downvoted. DownvotePeer(PeerId), } +/// The primary object for handling and driving all the current syncing logic. It maintains the +/// current state of the syncing process, the number of useful peers, downloaded blocks and +/// controls the logic behind both the long-range (batch) sync and the on-going potential parent +/// look-up of blocks. pub struct ImportManager { - /// A reference to the underlying beacon chain. - chain: Arc>, + /// A weak reference to the underlying beacon chain. + chain: Weak>, + /// The current state of the import manager. state: ManagerState, + /// A collection of `BlockRequest` per peer that is currently being downloaded. Used in the + /// long-range (batch) sync process. import_queue: HashMap>, - parent_queue: Vec>, + /// A collection of parent block lookups. + parent_queue: SmallVec<[ParentRequests; 3]>, + /// The collection of known, connected, fully-sync'd peers. full_peers: HashSet, + /// The current request Id. This is used to keep track of responses to various outbound + /// requests. This is an internal accounting mechanism, request id's are never sent to any + /// peers. current_req_id: usize, + /// The logger for the import manager. log: Logger, } impl ImportManager { + /// Generates a new `ImportManager` given a logger and an Arc reference to a beacon chain. The + /// import manager keeps a weak reference to the beacon chain, which allows the chain to be + /// dropped during the syncing process. The syncing handles this termination gracefully. pub fn new(beacon_chain: Arc>, log: &slog::Logger) -> Self { ImportManager { - chain: beacon_chain.clone(), + chain: Arc::downgrade(&beacon_chain), state: ManagerState::Regular, import_queue: HashMap::new(), - parent_queue: Vec::new(), + parent_queue: SmallVec::new(), full_peers: HashSet::new(), current_req_id: 0, log: log.clone(), } } + /// A peer has connected which has blocks that are unknown to us. + /// + /// This function handles the logic associated with the connection of a new peer. If the peer + /// is sufficiently ahead of our current head, a long-range (batch) sync is started and + /// batches of blocks are queued to download from the peer. Batched blocks begin at our + /// current head. If the resulting downloaded blocks are part of our current chain, we + /// continue with a forward sync. If not, we download blocks (in batches) backwards until we + /// reach a common ancestor. Batches are then processed and downloaded sequentially forwards. + /// + /// If the peer is within the `SLOT_IMPORT_TOLERANCE`, then it's head is sufficiently close to + /// ours that we consider it fully sync'd with respect to our current chain. pub fn add_peer(&mut self, peer_id: PeerId, remote: PeerSyncInfo) { - // TODO: Improve comments. - // initially try to download blocks from our current head - // then backwards search all the way back to our finalized epoch until we match on a chain - // has to be done sequentially to find next slot to start the batch from + // ensure the beacon chain still exists + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + warn!(self.log, + "Beacon chain dropped. Peer not considered for sync"; + "peer_id" => format!("{:?}", peer_id)); + return; + } + }; - let local = PeerSyncInfo::from(&self.chain); + let local = PeerSyncInfo::from(&chain); - // If a peer is within SLOT_IMPORT_TOLERANCE from our head slot, ignore a batch sync + // If a peer is within SLOT_IMPORT_TOLERANCE from our head slot, ignore a batch sync, + // consider it a fully-sync'd peer. if remote.head_slot.sub(local.head_slot).as_usize() < SLOT_IMPORT_TOLERANCE { trace!(self.log, "Ignoring full sync with peer"; "peer" => format!("{:?}", peer_id), @@ -116,34 +245,53 @@ impl ImportManager { ); // remove the peer from the queue if it exists self.import_queue.remove(&peer_id); + self.add_full_peer(peer_id); + // return; } + // Check if the peer is significantly is behind us. If within `SLOT_IMPORT_TOLERANCE` + // treat them as a fully synced peer. If not, ignore them in the sync process + if local.head_slot.sub(remote.head_slot).as_usize() < SLOT_IMPORT_TOLERANCE { + self.add_full_peer(peer_id); + } else { + debug!( + self.log, + "Out of sync peer connected"; + "peer" => format!("{:?}", peer_id), + ); + return; + } + + // Check if we are already downloading blocks from this peer, if so update, if not set up + // a new request structure if let Some(block_requests) = self.import_queue.get_mut(&peer_id) { // update the target head slot if remote.head_slot > block_requests.target_head_slot { block_requests.target_head_slot = remote.head_slot; } } else { + // not already downloading blocks from this peer let block_requests = BlockRequests { target_head_slot: remote.head_slot, // this should be larger than the current head. It is checked in the SyncManager before add_peer is called target_head_root: remote.head_root, downloaded_blocks: Vec::new(), state: BlockRequestsState::Queued, forward_sync: true, - current_start_slot: self.chain.best_slot(), + current_start_slot: chain.best_slot(), }; self.import_queue.insert(peer_id, block_requests); } } + /// A `BeaconBlocks` request has received a response. This function process the response. pub fn beacon_blocks_response( &mut self, peer_id: PeerId, request_id: RequestId, mut blocks: Vec>, ) { - // find the request + // find the request associated with this response let block_requests = match self .import_queue .get_mut(&peer_id) diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 573ac9dd1..dd857d8c3 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -16,8 +16,6 @@ use types::{Attestation, BeaconBlock, Epoch, EthSpec, Hash256, Slot}; /// Otherwise we queue it. pub(crate) const FUTURE_SLOT_TOLERANCE: u64 = 1; -/// The number of slots behind our head that we still treat a peer as a fully synced peer. -const FULL_PEER_TOLERANCE: u64 = 10; const SHOULD_FORWARD_GOSSIP_BLOCK: bool = true; const SHOULD_NOT_FORWARD_GOSSIP_BLOCK: bool = false; @@ -189,18 +187,17 @@ impl SimpleSync { .exists::>(&remote.head_root) .unwrap_or_else(|_| false) { + trace!( + self.log, "Out of date or potentially sync'd peer found"; + "peer" => format!("{:?}", peer_id), + "remote_head_slot" => remote.head_slot + "remote_latest_finalized_epoch" => remote.finalized_epoch, + ); + // If the node's best-block is already known to us and they are close to our current // head, treat them as a fully sync'd peer. - if self.chain.best_slot().sub(remote.head_slot).as_u64() < FULL_PEER_TOLERANCE { - self.manager.add_full_peer(peer_id); - self.process_sync(); - } else { - debug!( - self.log, - "Out of sync peer connected"; - "peer" => format!("{:?}", peer_id), - ); - } + self.manager.add_peer(peer_id, remote); + self.process_sync(); } else { // The remote node has an equal or great finalized epoch and we don't know it's head. // @@ -218,6 +215,8 @@ impl SimpleSync { } } + /// This function drives the `ImportManager` state machine. The outcomes it provides are + /// actioned until the `ImportManager` is idle. fn process_sync(&mut self) { loop { match self.manager.poll() { From cd7b6da88ed52a604e9ce3cf1d5985aff4f94a13 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 3 Sep 2019 00:34:41 +1000 Subject: [PATCH 2/9] Updates syncing, corrects CLI variables --- beacon_node/beacon_chain/src/beacon_chain.rs | 11 +- beacon_node/eth2-libp2p/src/discovery.rs | 10 +- beacon_node/eth2-libp2p/src/service.rs | 19 +- beacon_node/network/src/sync/manager.rs | 616 +++++++++++-------- beacon_node/network/src/sync/simple_sync.rs | 5 +- beacon_node/src/main.rs | 13 +- 6 files changed, 374 insertions(+), 300 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6380d03b3..a142816ae 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -442,6 +442,15 @@ impl BeaconChain { None } + /// Returns the block canonical root of the current canonical chain at a given slot. + /// + /// Returns None if a block doesn't exist at the slot. + pub fn root_at_slot(&self, target_slot: Slot) -> Option { + self.rev_iter_block_roots() + .find(|(_root, slot)| *slot == target_slot) + .map(|(root, _slot)| root) + } + /// Reads the slot clock (see `self.read_slot_clock()` and returns the number of slots since /// genesis. pub fn slots_since_genesis(&self) -> Option { @@ -1006,7 +1015,7 @@ impl BeaconChain { }; // Load the parent blocks state from the database, returning an error if it is not found. - // It is an error because if 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 = self .store diff --git a/beacon_node/eth2-libp2p/src/discovery.rs b/beacon_node/eth2-libp2p/src/discovery.rs index 6c80a8596..4a8aba2b1 100644 --- a/beacon_node/eth2-libp2p/src/discovery.rs +++ b/beacon_node/eth2-libp2p/src/discovery.rs @@ -341,13 +341,9 @@ fn save_enr_to_disc(dir: &Path, enr: &Enr, log: &slog::Logger) { } Err(e) => { warn!( - log, - <<<<<<< HEAD - "Could not write ENR to file"; "file" => format!("{:?}{:?}",dir, ENR_FILENAME), "error" => format!("{}", e) - ======= - "Could not write ENR to file"; "File" => format!("{:?}{:?}",dir, ENR_FILENAME), "Error" => format!("{}", e) - >>>>>>> interop - ); + log, + "Could not write ENR to file"; "file" => format!("{:?}{:?}",dir, ENR_FILENAME), "error" => format!("{}", e) + ); } } } diff --git a/beacon_node/eth2-libp2p/src/service.rs b/beacon_node/eth2-libp2p/src/service.rs index 589106f48..1ea1723b6 100644 --- a/beacon_node/eth2-libp2p/src/service.rs +++ b/beacon_node/eth2-libp2p/src/service.rs @@ -82,17 +82,10 @@ impl Service { // attempt to connect to user-input libp2p nodes for multiaddr in config.libp2p_nodes { match Swarm::dial_addr(&mut swarm, multiaddr.clone()) { -<<<<<<< HEAD Ok(()) => debug!(log, "Dialing libp2p peer"; "address" => format!("{}", multiaddr)), Err(err) => debug!( log, - "Could not connect to peer"; "address" => format!("{}", multiaddr), "Error" => format!("{:?}", err) -======= - Ok(()) => debug!(log, "Dialing libp2p peer"; "Address" => format!("{}", multiaddr)), - Err(err) => debug!( - log, - "Could not connect to peer"; "Address" => format!("{}", multiaddr), "Error" => format!("{:?}", err) ->>>>>>> interop + "Could not connect to peer"; "address" => format!("{}", multiaddr), "error" => format!("{:?}", err) ), }; } @@ -129,7 +122,6 @@ impl Service { let mut subscribed_topics = vec![]; for topic in topics { if swarm.subscribe(topic.clone()) { -<<<<<<< HEAD trace!(log, "Subscribed to topic"; "topic" => format!("{}", topic)); subscribed_topics.push(topic); } else { @@ -137,15 +129,6 @@ impl Service { } } info!(log, "Subscribed to topics"; "topics" => format!("{:?}", subscribed_topics.iter().map(|t| format!("{}", t)).collect::>())); -======= - trace!(log, "Subscribed to topic"; "Topic" => format!("{}", topic)); - subscribed_topics.push(topic); - } else { - warn!(log, "Could not subscribe to topic"; "Topic" => format!("{}", topic)); - } - } - info!(log, "Subscribed to topics"; "Topics" => format!("{:?}", subscribed_topics.iter().map(|t| format!("{}", t)).collect::>())); ->>>>>>> interop Ok(Service { local_peer_id, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 9b2d780f4..a48b43ad7 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -80,6 +80,9 @@ const PARENT_FAIL_TOLERANCE: usize = 3; /// canonical chain to its head once the peer connects. A chain should not appear where it's depth /// is further back than the most recent head slot. const PARENT_DEPTH_TOLERANCE: usize = SLOT_IMPORT_TOLERANCE * 2; +/// The number of empty batches we tolerate before dropping the peer. This prevents endless +/// requests to peers who never return blocks. +const EMPTY_BATCH_TOLERANCE: usize = 100; #[derive(PartialEq)] /// The current state of a block or batches lookup. @@ -95,6 +98,19 @@ enum BlockRequestsState { Failed, } +/// The state of batch requests. +enum SyncDirection { + /// The batch has just been initialised and we need to check to see if a backward sync is + /// required on first batch response. + Initial, + /// We are syncing forwards, the next batch should contain higher slot numbers than is + /// predecessor. + Forwards, + /// We are syncing backwards and looking for a common ancestor chain before we can start + /// processing the downloaded blocks. + Backwards, +} + /// `BlockRequests` keep track of the long-range (batch) sync process per peer. struct BlockRequests { /// The peer's head slot and the target of this batch download. @@ -104,10 +120,13 @@ struct BlockRequests { target_head_root: Hash256, /// The blocks that we have currently downloaded from the peer that are yet to be processed. downloaded_blocks: Vec>, + /// The number of empty batches we have consecutively received. If a peer returns more than + /// EMPTY_BATCHES_TOLERANCE, they are dropped. + consecutive_empty_batches: usize, /// The current state of this batch request. state: BlockRequestsState, - /// Specifies whether the current state is syncing forwards or backwards. - forward_sync: bool, + /// Specifies the current direction of this batch request. + sync_direction: SyncDirection, /// The current `start_slot` of the batched block request. current_start_slot: Slot, } @@ -129,10 +148,13 @@ struct ParentRequests { impl BlockRequests { /// Gets the next start slot for a batch and transitions the state to a Queued state. fn update_start_slot(&mut self) { - if self.forward_sync { - self.current_start_slot += Slot::from(MAX_BLOCKS_PER_REQUEST); - } else { - self.current_start_slot -= Slot::from(MAX_BLOCKS_PER_REQUEST); + match self.sync_direction { + SyncDirection::Initial | SyncDirection::Forwards => { + self.current_start_slot += Slot::from(MAX_BLOCKS_PER_REQUEST); + } + SyncDirection::Backwards => { + self.current_start_slot -= Slot::from(MAX_BLOCKS_PER_REQUEST); + } } self.state = BlockRequestsState::Queued; } @@ -175,6 +197,8 @@ pub(crate) enum ImportManagerOutcome { /// controls the logic behind both the long-range (batch) sync and the on-going potential parent /// look-up of blocks. pub struct ImportManager { + /// List of events to be processed externally. + event_queue: SmallVec<[ImportManagerOutcome; 20]>, /// A weak reference to the underlying beacon chain. chain: Weak>, /// The current state of the import manager. @@ -200,6 +224,7 @@ impl ImportManager { /// dropped during the syncing process. The syncing handles this termination gracefully. pub fn new(beacon_chain: Arc>, log: &slog::Logger) -> Self { ImportManager { + event_queue: SmallVec::new(), chain: Arc::downgrade(&beacon_chain), state: ManagerState::Regular, import_queue: HashMap::new(), @@ -253,7 +278,7 @@ impl ImportManager { // Check if the peer is significantly is behind us. If within `SLOT_IMPORT_TOLERANCE` // treat them as a fully synced peer. If not, ignore them in the sync process if local.head_slot.sub(remote.head_slot).as_usize() < SLOT_IMPORT_TOLERANCE { - self.add_full_peer(peer_id); + self.add_full_peer(peer_id.clone()); } else { debug!( self.log, @@ -275,9 +300,10 @@ impl ImportManager { let block_requests = BlockRequests { target_head_slot: remote.head_slot, // this should be larger than the current head. It is checked in the SyncManager before add_peer is called target_head_root: remote.head_root, + consecutive_empty_batches: 0, downloaded_blocks: Vec::new(), state: BlockRequestsState::Queued, - forward_sync: true, + sync_direction: SyncDirection::Initial, current_start_slot: chain.best_slot(), }; self.import_queue.insert(peer_id, block_requests); @@ -291,6 +317,16 @@ impl ImportManager { request_id: RequestId, mut blocks: Vec>, ) { + // ensure the underlying chain still exists + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + debug!(self.log, "Chain dropped. Sync terminating"); + self.event_queue.clear(); + return; + } + }; + // find the request associated with this response let block_requests = match self .import_queue @@ -315,10 +351,19 @@ impl ImportManager { if blocks.is_empty() { debug!(self.log, "BeaconBlocks response was empty"; "request_id" => request_id); - block_requests.update_start_slot(); + block_requests.consecutive_empty_batches += 1; + if block_requests.consecutive_empty_batches >= EMPTY_BATCH_TOLERANCE { + warn!(self.log, "Peer returned too many empty block batches"; + "peer" => format!("{:?}", peer_id)); + block_requests.state = BlockRequestsState::Failed; + } else { + block_requests.update_start_slot(); + } return; } + block_requests.consecutive_empty_batches = 0; + // verify the range of received blocks // Note that the order of blocks is verified in block processing let last_sent_slot = blocks[blocks.len() - 1].slot; @@ -328,83 +373,90 @@ impl ImportManager { .add(MAX_BLOCKS_PER_REQUEST) < last_sent_slot { - //TODO: Downvote peer - add a reason to failed - dbg!(&blocks); warn!(self.log, "BeaconBlocks response returned out of range blocks"; "request_id" => request_id, "response_initial_slot" => blocks[0].slot, "requested_initial_slot" => block_requests.current_start_slot); + self.event_queue + .push(ImportManagerOutcome::DownvotePeer(peer_id)); // consider this sync failed block_requests.state = BlockRequestsState::Failed; return; } // Determine if more blocks need to be downloaded. There are a few cases: - // - We have downloaded a batch from our head_slot, which has not reached the remotes head - // (target head). Therefore we need to download another sequential batch. - // - The latest batch includes blocks that greater than or equal to the target_head slot, - // which means we have caught up to their head. We then check to see if the first - // block downloaded matches our head. If so, we are on the same chain and can process - // the blocks. If not we need to sync back further until we are on the same chain. So - // request more blocks. - // - We are syncing backwards (from our head slot) and need to check if we are on the same - // chain. If so, process the blocks, if not, request more blocks all the way up to - // our last finalized slot. + // - We are in initial sync mode - We have requested blocks and need to determine if this + // is part of a known chain to determine the whether to start syncing backwards or continue + // syncing forwards. + // - We are syncing backwards and need to verify if we have found a common ancestor in + // order to start processing the downloaded blocks. + // - We are syncing forwards. We mark this as complete and check if any further blocks are + // required to download when processing the batch. - if block_requests.forward_sync { - // append blocks if syncing forward - block_requests.downloaded_blocks.append(&mut blocks); - } else { - // prepend blocks if syncing backwards - block_requests.downloaded_blocks.splice(..0, blocks); - } + match block_requests.sync_direction { + SyncDirection::Initial => { + block_requests.downloaded_blocks.append(&mut blocks); - // does the batch contain the target_head_slot - let last_element_index = block_requests.downloaded_blocks.len() - 1; - if block_requests.downloaded_blocks[last_element_index].slot - >= block_requests.target_head_slot - || !block_requests.forward_sync - { - // if the batch is on our chain, this is complete and we can then process. - // Otherwise start backwards syncing until we reach a common chain. - let earliest_slot = block_requests.downloaded_blocks[0].slot; - //TODO: Decide which is faster. Reading block from db and comparing or calculating - //the hash tree root and comparing. - if Some(block_requests.downloaded_blocks[0].canonical_root()) - == root_at_slot(&self.chain, earliest_slot) - { - block_requests.state = BlockRequestsState::Complete; - return; + // this batch is the first batch downloaded. Check if we can process or if we need + // to backwards search. + + //TODO: Decide which is faster. Reading block from db and comparing or calculating + //the hash tree root and comparing. + let earliest_slot = block_requests.downloaded_blocks[0].slot; + if Some(block_requests.downloaded_blocks[0].canonical_root()) + == chain.root_at_slot(earliest_slot) + { + // we have a common head, start processing and begin a forwards sync + block_requests.sync_direction = SyncDirection::Forwards; + block_requests.state = BlockRequestsState::ReadyToProcess; + return; + } + // no common head, begin a backwards search + block_requests.sync_direction = SyncDirection::Backwards; + block_requests.current_start_slot = + std::cmp::min(chain.best_slot(), block_requests.downloaded_blocks[0].slot); + block_requests.update_start_slot(); } - - // not on the same chain, request blocks backwards - let state = &self.chain.head().beacon_state; - let local_finalized_slot = state - .finalized_checkpoint - .epoch - .start_slot(T::EthSpec::slots_per_epoch()); - - // check that the request hasn't failed by having no common chain - if local_finalized_slot >= block_requests.current_start_slot { - warn!(self.log, "Peer returned an unknown chain."; "request_id" => request_id); - block_requests.state = BlockRequestsState::Failed; - return; + SyncDirection::Forwards => { + // continue processing all blocks forwards, verify the end in the processing + block_requests.downloaded_blocks.append(&mut blocks); + block_requests.state = BlockRequestsState::ReadyToProcess; } + SyncDirection::Backwards => { + block_requests.downloaded_blocks.splice(..0, blocks); - // if this is a forward sync, then we have reached the head without a common chain - // and we need to start syncing backwards. - if block_requests.forward_sync { - // Start a backwards sync by requesting earlier blocks - block_requests.forward_sync = false; - block_requests.current_start_slot = std::cmp::min( - self.chain.best_slot(), - block_requests.downloaded_blocks[0].slot, - ); + // verify the request hasn't failed by having no common ancestor chain + // get our local finalized_slot + let local_finalized_slot = { + let state = &chain.head().beacon_state; + state + .finalized_checkpoint + .epoch + .start_slot(T::EthSpec::slots_per_epoch()) + }; + + if local_finalized_slot >= block_requests.current_start_slot { + warn!(self.log, "Peer returned an unknown chain."; "request_id" => request_id); + block_requests.state = BlockRequestsState::Failed; + return; + } + + // check if we have reached a common chain ancestor + let earliest_slot = block_requests.downloaded_blocks[0].slot; + if Some(block_requests.downloaded_blocks[0].canonical_root()) + == chain.root_at_slot(earliest_slot) + { + // we have a common head, start processing and begin a forwards sync + block_requests.sync_direction = SyncDirection::Forwards; + block_requests.state = BlockRequestsState::ReadyToProcess; + return; + } + + // no common chain, haven't passed last_finalized_head, so continue backwards + // search + block_requests.update_start_slot(); } } - - // update the start slot and re-queue the batch - block_requests.update_start_slot(); } pub fn recent_blocks_response( @@ -447,7 +499,7 @@ impl ImportManager { } // queue for processing - parent_request.state = BlockRequestsState::Complete; + parent_request.state = BlockRequestsState::ReadyToProcess; } pub fn _inject_error(_peer_id: PeerId, _id: RequestId) { @@ -500,29 +552,41 @@ impl ImportManager { pub(crate) fn poll(&mut self) -> ImportManagerOutcome { loop { + //TODO: Optimize the lookups. Potentially keep state of whether each of these functions + //need to be called. + + // only break once everything has been processed + let mut re_run = false; + + // only process batch requests if there are any + if !self.import_queue.is_empty() { + // process potential block requests + self.process_potential_block_requests(); + + // process any complete long-range batches + re_run = self.process_complete_batches(); + } + + // only process parent objects if we are in regular sync + if let ManagerState::Regular = self.state { + // process any parent block lookup-requests + self.process_parent_requests(); + + // process any complete parent lookups + re_run = self.process_complete_parent_requests(); + } + + // return any queued events + if !self.event_queue.is_empty() { + let event = self.event_queue.remove(0); + self.event_queue.shrink_to_fit(); + return event; + } + // update the state of the manager self.update_state(); - // process potential block requests - if let Some(outcome) = self.process_potential_block_requests() { - return outcome; - } - - // process any complete long-range batches - if let Some(outcome) = self.process_complete_batches() { - return outcome; - } - - // process any parent block lookup-requests - if let Some(outcome) = self.process_parent_requests() { - return outcome; - } - - // process any complete parent lookups - let (re_run, outcome) = self.process_complete_parent_requests(); - if let Some(outcome) = outcome { - return outcome; - } else if !re_run { + if !re_run { break; } } @@ -549,11 +613,11 @@ impl ImportManager { } } - fn process_potential_block_requests(&mut self) -> Option { + fn process_potential_block_requests(&mut self) { // check if an outbound request is required // Managing a fixed number of outbound requests is maintained at the RPC protocol libp2p - // layer and not needed here. - // If any in queued state we submit a request. + // layer and not needed here. Therefore we create many outbound requests and let the RPC + // handle the number of simultaneous requests. Request all queued objects. // remove any failed batches let debug_log = &self.log; @@ -585,56 +649,84 @@ impl ImportManager { count: MAX_BLOCKS_PER_REQUEST, step: 0, }; - return Some(ImportManagerOutcome::RequestBlocks { + self.event_queue.push(ImportManagerOutcome::RequestBlocks { peer_id: peer_id.clone(), request, request_id, }); } - - None } - fn process_complete_batches(&mut self) -> Option { - let completed_batches = self - .import_queue - .iter() - .filter(|(_peer, block_requests)| block_requests.state == BlockRequestsState::Complete) - .map(|(peer, _)| peer) - .cloned() - .collect::>(); - for peer_id in completed_batches { - let block_requests = self.import_queue.remove(&peer_id).expect("key exists"); - match self.process_blocks(block_requests.downloaded_blocks.clone()) { - Ok(()) => { - //TODO: Verify it's impossible to have empty downloaded_blocks - let last_element = block_requests.downloaded_blocks.len() - 1; - debug!(self.log, "Blocks processed successfully"; - "peer" => format!("{:?}", peer_id), - "start_slot" => block_requests.downloaded_blocks[0].slot, - "end_slot" => block_requests.downloaded_blocks[last_element].slot, - "no_blocks" => last_element + 1, - ); - // Re-HELLO to ensure we are up to the latest head - return Some(ImportManagerOutcome::Hello(peer_id)); - } - Err(e) => { - let last_element = block_requests.downloaded_blocks.len() - 1; - warn!(self.log, "Block processing failed"; + fn process_complete_batches(&mut self) -> bool { + // flag to indicate if the manager can be switched to idle or not + let mut re_run = false; + + // create reference variables to be moved into subsequent closure + let chain_ref = self.chain.clone(); + let log_ref = &self.log; + let event_queue_ref = &mut self.event_queue; + + self.import_queue.retain(|peer_id, block_requests| { + // check that the chain still exists + if let Some(chain) = chain_ref.upgrade() { + let downloaded_blocks = + std::mem::replace(&mut block_requests.downloaded_blocks, Vec::new()); + let last_element = block_requests.downloaded_blocks.len() - 1; + let start_slot = block_requests.downloaded_blocks[0].slot; + let end_slot = block_requests.downloaded_blocks[last_element].slot; + + match process_blocks(chain, downloaded_blocks, log_ref) { + Ok(()) => { + debug!(log_ref, "Blocks processed successfully"; "peer" => format!("{:?}", peer_id), - "start_slot" => block_requests.downloaded_blocks[0].slot, - "end_slot" => block_requests.downloaded_blocks[last_element].slot, + "start_slot" => start_slot, + "end_slot" => end_slot, "no_blocks" => last_element + 1, - "error" => format!("{:?}", e), - ); - return Some(ImportManagerOutcome::DownvotePeer(peer_id)); + ); + + // check if the batch is complete, by verifying if we have reached the + // target head + if end_slot >= block_requests.target_head_slot { + // Completed, re-hello the peer to ensure we are up to the latest head + event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); + // remove the request + false + } else { + // have not reached the end, queue another batch + block_requests.update_start_slot(); + re_run = true; + // keep the batch + true + } + } + Err(e) => { + warn!(log_ref, "Block processing failed"; + "peer" => format!("{:?}", peer_id), + "start_slot" => start_slot, + "end_slot" => end_slot, + "no_blocks" => last_element + 1, + "error" => format!("{:?}", e), + ); + event_queue_ref.push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); + false + } } + } else { + // chain no longer exists, empty the queue and return + event_queue_ref.clear(); + return false; } - } - None + }); + + re_run } - fn process_parent_requests(&mut self) -> Option { + fn process_parent_requests(&mut self) { + // check to make sure there are peers to search for the parent from + if self.full_peers.is_empty() { + return; + } + // remove any failed requests let debug_log = &self.log; self.parent_queue.retain(|parent_request| { @@ -649,11 +741,6 @@ impl ImportManager { } }); - // check to make sure there are peers to search for the parent from - if self.full_peers.is_empty() { - return None; - } - // check if parents need to be searched for for parent_request in self.parent_queue.iter_mut() { if parent_request.failed_attempts >= PARENT_FAIL_TOLERANCE { @@ -677,23 +764,21 @@ impl ImportManager { // select a random fully synced peer to attempt to download the parent block let peer_id = self.full_peers.iter().next().expect("List is not empty"); - return Some(ImportManagerOutcome::RecentRequest(peer_id.clone(), req)); + self.event_queue + .push(ImportManagerOutcome::RecentRequest(peer_id.clone(), req)); } } - - None } - fn process_complete_parent_requests(&mut self) -> (bool, Option) { - // flag to determine if there is more process to drive or if the manager can be switched to - // an idle state + fn process_complete_parent_requests(&mut self) -> bool { + // returned value indicating whether the manager can be switched to idle or not let mut re_run = false; // Find any parent_requests ready to be processed for completed_request in self .parent_queue .iter_mut() - .filter(|req| req.state == BlockRequestsState::Complete) + .filter(|req| req.state == BlockRequestsState::ReadyToProcess) { // verify the last added block is the parent of the last requested block let last_index = completed_request.downloaded_blocks.len() - 1; @@ -711,7 +796,9 @@ impl ImportManager { "received_block" => format!("{}", block_hash), "expected_parent" => format!("{}", expected_hash), ); - return (true, Some(ImportManagerOutcome::DownvotePeer(peer))); + re_run = true; + self.event_queue + .push(ImportManagerOutcome::DownvotePeer(peer)); } // try and process the list of blocks up to the requested block @@ -720,154 +807,153 @@ impl ImportManager { .downloaded_blocks .pop() .expect("Block must exist exist"); - match self.chain.process_block(block.clone()) { - Ok(BlockProcessingOutcome::ParentUnknown { parent: _ }) => { - // need to keep looking for parents - completed_request.downloaded_blocks.push(block); - completed_request.state = BlockRequestsState::Queued; - re_run = true; - break; - } - Ok(BlockProcessingOutcome::Processed { block_root: _ }) => {} - Ok(outcome) => { - // it's a future slot or an invalid block, remove it and try again - completed_request.failed_attempts += 1; - trace!( - self.log, "Invalid parent block"; - "outcome" => format!("{:?}", outcome), - "peer" => format!("{:?}", completed_request.last_submitted_peer), - ); - completed_request.state = BlockRequestsState::Queued; - re_run = true; - return ( - re_run, - Some(ImportManagerOutcome::DownvotePeer( + + // check if the chain exists + if let Some(chain) = self.chain.upgrade() { + match chain.process_block(block.clone()) { + Ok(BlockProcessingOutcome::ParentUnknown { parent: _ }) => { + // need to keep looking for parents + completed_request.downloaded_blocks.push(block); + completed_request.state = BlockRequestsState::Queued; + re_run = true; + break; + } + Ok(BlockProcessingOutcome::Processed { block_root: _ }) => {} + Ok(outcome) => { + // it's a future slot or an invalid block, remove it and try again + completed_request.failed_attempts += 1; + trace!( + self.log, "Invalid parent block"; + "outcome" => format!("{:?}", outcome), + "peer" => format!("{:?}", completed_request.last_submitted_peer), + ); + completed_request.state = BlockRequestsState::Queued; + re_run = true; + self.event_queue.push(ImportManagerOutcome::DownvotePeer( completed_request.last_submitted_peer.clone(), - )), - ); - } - Err(e) => { - completed_request.failed_attempts += 1; - warn!( - self.log, "Parent processing error"; - "error" => format!("{:?}", e) - ); - completed_request.state = BlockRequestsState::Queued; - re_run = true; - return ( - re_run, - Some(ImportManagerOutcome::DownvotePeer( + )); + return re_run; + } + Err(e) => { + completed_request.failed_attempts += 1; + warn!( + self.log, "Parent processing error"; + "error" => format!("{:?}", e) + ); + completed_request.state = BlockRequestsState::Queued; + re_run = true; + self.event_queue.push(ImportManagerOutcome::DownvotePeer( completed_request.last_submitted_peer.clone(), - )), - ); + )); + return re_run; + } } + } else { + // chain doesn't exist - clear the event queue and return + self.event_queue.clear(); + return false; } } } - // remove any full completed and processed parent chains + // remove any fully processed parent chains self.parent_queue.retain(|req| { - if req.state == BlockRequestsState::Complete { + if req.state == BlockRequestsState::ReadyToProcess { false } else { true } }); - (re_run, None) + re_run } +} - fn process_blocks(&mut self, blocks: Vec>) -> Result<(), String> { - for block in blocks { - let processing_result = self.chain.process_block(block.clone()); +// Helper function to process blocks +fn process_blocks( + chain: Arc>, + blocks: Vec>, + log: &Logger, +) -> Result<(), String> { + for block in blocks { + let processing_result = chain.process_block(block.clone()); - if let Ok(outcome) = processing_result { - match outcome { - BlockProcessingOutcome::Processed { block_root } => { - // The block was valid and we processed it successfully. + if let Ok(outcome) = processing_result { + match outcome { + BlockProcessingOutcome::Processed { block_root } => { + // The block was valid and we processed it successfully. + trace!( + log, "Imported block from network"; + "slot" => block.slot, + "block_root" => format!("{}", block_root), + ); + } + BlockProcessingOutcome::ParentUnknown { parent } => { + // blocks should be sequential and all parents should exist + trace!( + log, "ParentBlockUnknown"; + "parent_root" => format!("{}", parent), + "baby_block_slot" => block.slot, + ); + return Err(format!( + "Block at slot {} has an unknown parent.", + block.slot + )); + } + BlockProcessingOutcome::FutureSlot { + present_slot, + block_slot, + } => { + if present_slot + FUTURE_SLOT_TOLERANCE >= block_slot { + // The block is too far in the future, drop it. trace!( - self.log, "Imported block from network"; - "slot" => block.slot, - "block_root" => format!("{}", block_root), - ); - } - BlockProcessingOutcome::ParentUnknown { parent } => { - // blocks should be sequential and all parents should exist - trace!( - self.log, "ParentBlockUnknown"; - "parent_root" => format!("{}", parent), - "baby_block_slot" => block.slot, + log, "FutureBlock"; + "msg" => "block for future slot rejected, check your time", + "present_slot" => present_slot, + "block_slot" => block_slot, + "FUTURE_SLOT_TOLERANCE" => FUTURE_SLOT_TOLERANCE, ); return Err(format!( - "Block at slot {} has an unknown parent.", + "Block at slot {} is too far in the future", block.slot )); - } - BlockProcessingOutcome::FutureSlot { - present_slot, - block_slot, - } => { - if present_slot + FUTURE_SLOT_TOLERANCE >= block_slot { - // The block is too far in the future, drop it. - trace!( - self.log, "FutureBlock"; - "msg" => "block for future slot rejected, check your time", - "present_slot" => present_slot, - "block_slot" => block_slot, - "FUTURE_SLOT_TOLERANCE" => FUTURE_SLOT_TOLERANCE, - ); - return Err(format!( - "Block at slot {} is too far in the future", - block.slot - )); - } else { - // The block is in the future, but not too far. - trace!( - self.log, "QueuedFutureBlock"; - "msg" => "queuing future block, check your time", - "present_slot" => present_slot, - "block_slot" => block_slot, - "FUTURE_SLOT_TOLERANCE" => FUTURE_SLOT_TOLERANCE, - ); - } - } - BlockProcessingOutcome::FinalizedSlot => { + } else { + // The block is in the future, but not too far. trace!( - self.log, "Finalized or earlier block processed"; - "outcome" => format!("{:?}", outcome), + log, "QueuedFutureBlock"; + "msg" => "queuing future block, check your time", + "present_slot" => present_slot, + "block_slot" => block_slot, + "FUTURE_SLOT_TOLERANCE" => FUTURE_SLOT_TOLERANCE, ); - // block reached our finalized slot or was earlier, move to the next block - } - _ => { - trace!( - self.log, "InvalidBlock"; - "msg" => "peer sent invalid block", - "outcome" => format!("{:?}", outcome), - ); - return Err(format!("Invalid block at slot {}", block.slot)); } } - } else { - trace!( - self.log, "BlockProcessingFailure"; - "msg" => "unexpected condition in processing block.", - "outcome" => format!("{:?}", processing_result) - ); - return Err(format!( - "Unexpected block processing error: {:?}", - processing_result - )); + BlockProcessingOutcome::FinalizedSlot => { + trace!( + log, "Finalized or earlier block processed"; + "outcome" => format!("{:?}", outcome), + ); + // block reached our finalized slot or was earlier, move to the next block + } + _ => { + trace!( + log, "InvalidBlock"; + "msg" => "peer sent invalid block", + "outcome" => format!("{:?}", outcome), + ); + return Err(format!("Invalid block at slot {}", block.slot)); + } } + } else { + trace!( + log, "BlockProcessingFailure"; + "msg" => "unexpected condition in processing block.", + "outcome" => format!("{:?}", processing_result) + ); + return Err(format!( + "Unexpected block processing error: {:?}", + processing_result + )); } - Ok(()) } -} - -fn root_at_slot( - chain: &Arc>, - target_slot: Slot, -) -> Option { - chain - .rev_iter_block_roots() - .find(|(_root, slot)| *slot == target_slot) - .map(|(root, _slot)| root) + Ok(()) } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index dd857d8c3..36947082e 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -6,7 +6,6 @@ use eth2_libp2p::rpc::{RPCEvent, RPCRequest, RPCResponse, RequestId}; use eth2_libp2p::PeerId; use slog::{debug, info, o, trace, warn}; use ssz::Encode; -use std::ops::Sub; use std::sync::Arc; use store::Store; use tokio::sync::mpsc; @@ -190,7 +189,7 @@ impl SimpleSync { trace!( self.log, "Out of date or potentially sync'd peer found"; "peer" => format!("{:?}", peer_id), - "remote_head_slot" => remote.head_slot + "remote_head_slot" => remote.head_slot, "remote_latest_finalized_epoch" => remote.finalized_epoch, ); @@ -386,7 +385,7 @@ impl SimpleSync { "peer" => format!("{:?}", peer_id), "msg" => "Failed to return all requested hashes", "start_slot" => req.start_slot, - "current_slot" => self.chain.present_slot(), + "current_slot" => self.chain.best_slot(), "requested" => req.count, "returned" => blocks.len(), ); diff --git a/beacon_node/src/main.rs b/beacon_node/src/main.rs index 26537c6f7..ea801cd8b 100644 --- a/beacon_node/src/main.rs +++ b/beacon_node/src/main.rs @@ -33,14 +33,14 @@ fn main() { .arg( Arg::with_name("logfile") .long("logfile") - .value_name("logfile") + .value_name("FILE") .help("File path where output will be written.") .takes_value(true), ) .arg( Arg::with_name("network-dir") .long("network-dir") - .value_name("NETWORK-DIR") + .value_name("DIR") .help("Data directory for network keys.") .takes_value(true) .global(true) @@ -83,7 +83,7 @@ fn main() { Arg::with_name("boot-nodes") .long("boot-nodes") .allow_hyphen_values(true) - .value_name("BOOTNODES") + .value_name("ENR-LIST") .help("One or more comma-delimited base64-encoded ENR's to bootstrap the p2p network.") .takes_value(true), ) @@ -128,13 +128,14 @@ fn main() { .arg( Arg::with_name("rpc-address") .long("rpc-address") - .value_name("Address") + .value_name("ADDRESS") .help("Listen address for RPC endpoint.") .takes_value(true), ) .arg( Arg::with_name("rpc-port") .long("rpc-port") + .value_name("PORT") .help("Listen port for RPC endpoint.") .conflicts_with("port-bump") .takes_value(true), @@ -149,14 +150,14 @@ fn main() { .arg( Arg::with_name("api-address") .long("api-address") - .value_name("APIADDRESS") + .value_name("ADDRESS") .help("Set the listen address for the RESTful HTTP API server.") .takes_value(true), ) .arg( Arg::with_name("api-port") .long("api-port") - .value_name("APIPORT") + .value_name("PORT") .help("Set the listen TCP port for the RESTful HTTP API server.") .conflicts_with("port-bump") .takes_value(true), From 13b5df56b3f4e81238733943e23bb3e11d602c5a Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 3 Sep 2019 07:50:44 +1000 Subject: [PATCH 3/9] Account manager, bootnodes, RPC display and sync fixes --- account_manager/src/main.rs | 10 +- beacon_node/eth2-libp2p/src/discovery.rs | 2 +- beacon_node/eth2-libp2p/src/rpc/methods.rs | 48 ++++++++ beacon_node/eth2-libp2p/src/rpc/mod.rs | 10 ++ beacon_node/eth2-libp2p/src/rpc/protocol.rs | 11 ++ beacon_node/eth2-libp2p/src/service.rs | 16 ++- beacon_node/network/src/service.rs | 4 +- beacon_node/network/src/sync/manager.rs | 124 ++++++++++++-------- beacon_node/network/src/sync/simple_sync.rs | 2 +- beacon_node/src/main.rs | 14 --- 10 files changed, 168 insertions(+), 73 deletions(-) diff --git a/account_manager/src/main.rs b/account_manager/src/main.rs index b7448ddf2..ae3823049 100644 --- a/account_manager/src/main.rs +++ b/account_manager/src/main.rs @@ -125,9 +125,13 @@ fn main() { } } } - _ => panic!( - "The account manager must be run with a subcommand. See help for more information." - ), + _ => { + crit!( + log, + "The account manager must be run with a subcommand. See help for more information." + ); + return; + } } } diff --git a/beacon_node/eth2-libp2p/src/discovery.rs b/beacon_node/eth2-libp2p/src/discovery.rs index 4a8aba2b1..c3f2522d8 100644 --- a/beacon_node/eth2-libp2p/src/discovery.rs +++ b/beacon_node/eth2-libp2p/src/discovery.rs @@ -114,7 +114,7 @@ impl Discovery { self.find_peers(); } - /// Add an Enr to the routing table of the discovery mechanism. + /// Add an ENR to the routing table of the discovery mechanism. pub fn add_enr(&mut self, enr: Enr) { self.discovery.add_enr(enr); } diff --git a/beacon_node/eth2-libp2p/src/rpc/methods.rs b/beacon_node/eth2-libp2p/src/rpc/methods.rs index d912bcfa1..c9610b000 100644 --- a/beacon_node/eth2-libp2p/src/rpc/methods.rs +++ b/beacon_node/eth2-libp2p/src/rpc/methods.rs @@ -157,3 +157,51 @@ impl ErrorMessage { String::from_utf8(self.error_message.clone()).unwrap_or_else(|_| "".into()) } } + +impl std::fmt::Display for HelloMessage { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Hello Message: Fork Version: {:?}, Finalized Root: {}, Finalized Epoch: {}, Head Root: {}, Head Slot: {}", self.fork_version, self.finalized_root, self.finalized_epoch, self.head_root, self.head_slot) + } +} + +impl std::fmt::Display for RPCResponse { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RPCResponse::Hello(hello) => write!(f, "{}", hello), + RPCResponse::BeaconBlocks(_) => write!(f, ""), + RPCResponse::RecentBeaconBlocks(_) => write!(f, ""), + } + } +} + +impl std::fmt::Display for RPCErrorResponse { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RPCErrorResponse::Success(res) => write!(f, "{}", res), + RPCErrorResponse::InvalidRequest(err) => write!(f, "Invalid Request: {:?}", err), + RPCErrorResponse::ServerError(err) => write!(f, "Server Error: {:?}", err), + RPCErrorResponse::Unknown(err) => write!(f, "Unknown Error: {:?}", err), + } + } +} + +impl std::fmt::Display for GoodbyeReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + GoodbyeReason::ClientShutdown => write!(f, "Client Shutdown"), + GoodbyeReason::IrrelevantNetwork => write!(f, "Irrelevant Network"), + GoodbyeReason::Fault => write!(f, "Fault"), + GoodbyeReason::Unknown => write!(f, "Unknown Reason"), + } + } +} + +impl std::fmt::Display for BeaconBlocksRequest { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Head Block Root: {}, Start Slot: {}, Count: {}, Step: {}", + self.head_block_root, self.start_slot, self.count, self.step + ) + } +} diff --git a/beacon_node/eth2-libp2p/src/rpc/mod.rs b/beacon_node/eth2-libp2p/src/rpc/mod.rs index 756a62e71..2076615a9 100644 --- a/beacon_node/eth2-libp2p/src/rpc/mod.rs +++ b/beacon_node/eth2-libp2p/src/rpc/mod.rs @@ -47,6 +47,16 @@ impl RPCEvent { } } +impl std::fmt::Display for RPCEvent { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RPCEvent::Request(id, req) => write!(f, "RPC Request(Id: {}, {})", id, req), + RPCEvent::Response(id, res) => write!(f, "RPC Response(Id: {}, {})", id, res), + RPCEvent::Error(id, err) => write!(f, "RPC Request(Id: {}, Error: {:?})", id, err), + } + } +} + /// Implements the libp2p `NetworkBehaviour` trait and therefore manages network-level /// logic. pub struct RPC { diff --git a/beacon_node/eth2-libp2p/src/rpc/protocol.rs b/beacon_node/eth2-libp2p/src/rpc/protocol.rs index be1efdf5d..401fa8b9e 100644 --- a/beacon_node/eth2-libp2p/src/rpc/protocol.rs +++ b/beacon_node/eth2-libp2p/src/rpc/protocol.rs @@ -288,3 +288,14 @@ impl std::error::Error for RPCError { } } } + +impl std::fmt::Display for RPCRequest { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RPCRequest::Hello(hello) => write!(f, "Hello Message: {}", hello), + RPCRequest::Goodbye(reason) => write!(f, "Goodbye: {}", reason), + RPCRequest::BeaconBlocks(req) => write!(f, "Beacon Blocks: {}", req), + RPCRequest::RecentBeaconBlocks(req) => write!(f, "Recent Beacon Blocks: {:?}", req), + } + } +} diff --git a/beacon_node/eth2-libp2p/src/service.rs b/beacon_node/eth2-libp2p/src/service.rs index 1ea1723b6..96b5a276e 100644 --- a/beacon_node/eth2-libp2p/src/service.rs +++ b/beacon_node/eth2-libp2p/src/service.rs @@ -79,8 +79,8 @@ impl Service { } }; - // attempt to connect to user-input libp2p nodes - for multiaddr in config.libp2p_nodes { + // helper closure for dialing peers + let mut dial_addr = |multiaddr: Multiaddr| { match Swarm::dial_addr(&mut swarm, multiaddr.clone()) { Ok(()) => debug!(log, "Dialing libp2p peer"; "address" => format!("{}", multiaddr)), Err(err) => debug!( @@ -88,6 +88,18 @@ impl Service { "Could not connect to peer"; "address" => format!("{}", multiaddr), "error" => format!("{:?}", err) ), }; + }; + + // attempt to connect to user-input libp2p nodes + for multiaddr in config.libp2p_nodes { + dial_addr(multiaddr); + } + + // attempt to connect to any specified boot-nodes + for bootnode_enr in config.boot_nodes { + for multiaddr in bootnode_enr.multiaddr() { + dial_addr(multiaddr); + } } // subscribe to default gossipsub topics diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index a8b3c74b6..ae7562033 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -161,7 +161,7 @@ fn network_service( Ok(Async::Ready(Some(message))) => match message { NetworkMessage::Send(peer_id, outgoing_message) => match outgoing_message { OutgoingMessage::RPC(rpc_event) => { - trace!(log, "Sending RPC Event: {:?}", rpc_event); + trace!(log, "{}", rpc_event); libp2p_service.lock().swarm.send_rpc(peer_id, rpc_event); } }, @@ -185,7 +185,7 @@ fn network_service( match libp2p_service.lock().poll() { Ok(Async::Ready(Some(event))) => match event { Libp2pEvent::RPC(peer_id, rpc_event) => { - trace!(log, "RPC Event: RPC message received: {:?}", rpc_event); + trace!(log, "{}", rpc_event); message_handler_send .try_send(HandlerMessage::RPC(peer_id, rpc_event)) .map_err(|_| "Failed to send RPC to handler")?; diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index a48b43ad7..8ba7486a5 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -68,7 +68,7 @@ use types::{BeaconBlock, EthSpec, Hash256, Slot}; /// Blocks are downloaded in batches from peers. This constant specifies how many blocks per batch /// is requested. Currently the value is small for testing. This will be incremented for /// production. -const MAX_BLOCKS_PER_REQUEST: u64 = 10; +const MAX_BLOCKS_PER_REQUEST: u64 = 100; /// The number of slots ahead of us that is allowed before requesting a long-range (batch) Sync /// from a peer. If a peer is within this tolerance (forwards or backwards), it is treated as a @@ -120,6 +120,8 @@ struct BlockRequests { target_head_root: Hash256, /// The blocks that we have currently downloaded from the peer that are yet to be processed. downloaded_blocks: Vec>, + /// The number of blocks successfully processed in this request. + blocks_processed: usize, /// The number of empty batches we have consecutively received. If a peer returns more than /// EMPTY_BATCHES_TOLERANCE, they are dropped. consecutive_empty_batches: usize, @@ -302,6 +304,7 @@ impl ImportManager { target_head_root: remote.head_root, consecutive_empty_batches: 0, downloaded_blocks: Vec::new(), + blocks_processed: 0, state: BlockRequestsState::Queued, sync_direction: SyncDirection::Initial, current_start_slot: chain.best_slot(), @@ -356,6 +359,10 @@ impl ImportManager { warn!(self.log, "Peer returned too many empty block batches"; "peer" => format!("{:?}", peer_id)); block_requests.state = BlockRequestsState::Failed; + } else if block_requests.current_start_slot >= block_requests.target_head_slot { + warn!(self.log, "Peer did not return blocks it claimed to possess"; + "peer" => format!("{:?}", peer_id)); + block_requests.state = BlockRequestsState::Failed; } else { block_requests.update_start_slot(); } @@ -561,19 +568,19 @@ impl ImportManager { // only process batch requests if there are any if !self.import_queue.is_empty() { // process potential block requests - self.process_potential_block_requests(); + re_run = re_run || self.process_potential_block_requests(); // process any complete long-range batches - re_run = self.process_complete_batches(); + re_run = re_run || self.process_complete_batches(); } // only process parent objects if we are in regular sync - if let ManagerState::Regular = self.state { + if !self.parent_queue.is_empty() { // process any parent block lookup-requests - self.process_parent_requests(); + re_run = re_run || self.process_parent_requests(); // process any complete parent lookups - re_run = self.process_complete_parent_requests(); + re_run = re_run || self.process_complete_parent_requests(); } // return any queued events @@ -613,20 +620,23 @@ impl ImportManager { } } - fn process_potential_block_requests(&mut self) { + fn process_potential_block_requests(&mut self) -> bool { // check if an outbound request is required // Managing a fixed number of outbound requests is maintained at the RPC protocol libp2p // layer and not needed here. Therefore we create many outbound requests and let the RPC // handle the number of simultaneous requests. Request all queued objects. + let mut re_run = false; // remove any failed batches let debug_log = &self.log; + let full_peer_ref = &mut self.full_peers; self.import_queue.retain(|peer_id, block_request| { if let BlockRequestsState::Failed = block_request.state { debug!(debug_log, "Block import from peer failed"; "peer_id" => format!("{:?}", peer_id), - "downloaded_blocks" => block_request.downloaded_blocks.len() + "downloaded_blocks" => block_request.blocks_processed ); + full_peer_ref.remove(peer_id); false } else { true @@ -654,7 +664,10 @@ impl ImportManager { request, request_id, }); + re_run = true; } + + re_run } fn process_complete_batches(&mut self) -> bool { @@ -667,66 +680,75 @@ impl ImportManager { let event_queue_ref = &mut self.event_queue; self.import_queue.retain(|peer_id, block_requests| { - // check that the chain still exists - if let Some(chain) = chain_ref.upgrade() { - let downloaded_blocks = - std::mem::replace(&mut block_requests.downloaded_blocks, Vec::new()); - let last_element = block_requests.downloaded_blocks.len() - 1; - let start_slot = block_requests.downloaded_blocks[0].slot; - let end_slot = block_requests.downloaded_blocks[last_element].slot; + if block_requests.state == BlockRequestsState::ReadyToProcess { + // check that the chain still exists + if let Some(chain) = chain_ref.upgrade() { + let downloaded_blocks = + std::mem::replace(&mut block_requests.downloaded_blocks, Vec::new()); + let last_element = downloaded_blocks.len() - 1; + let start_slot = downloaded_blocks[0].slot; + let end_slot = downloaded_blocks[last_element].slot; - match process_blocks(chain, downloaded_blocks, log_ref) { - Ok(()) => { - debug!(log_ref, "Blocks processed successfully"; - "peer" => format!("{:?}", peer_id), - "start_slot" => start_slot, - "end_slot" => end_slot, - "no_blocks" => last_element + 1, - ); - - // check if the batch is complete, by verifying if we have reached the - // target head - if end_slot >= block_requests.target_head_slot { - // Completed, re-hello the peer to ensure we are up to the latest head - event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); - // remove the request - false - } else { - // have not reached the end, queue another batch - block_requests.update_start_slot(); - re_run = true; - // keep the batch - true - } - } - Err(e) => { - warn!(log_ref, "Block processing failed"; + match process_blocks(chain, downloaded_blocks, log_ref) { + Ok(()) => { + debug!(log_ref, "Blocks processed successfully"; "peer" => format!("{:?}", peer_id), "start_slot" => start_slot, "end_slot" => end_slot, "no_blocks" => last_element + 1, - "error" => format!("{:?}", e), - ); - event_queue_ref.push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); - false + ); + block_requests.blocks_processed += last_element + 1; + + // check if the batch is complete, by verifying if we have reached the + // target head + if end_slot >= block_requests.target_head_slot { + // Completed, re-hello the peer to ensure we are up to the latest head + event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); + // remove the request + false + } else { + // have not reached the end, queue another batch + block_requests.update_start_slot(); + re_run = true; + // keep the batch + true + } + } + Err(e) => { + warn!(log_ref, "Block processing failed"; + "peer" => format!("{:?}", peer_id), + "start_slot" => start_slot, + "end_slot" => end_slot, + "no_blocks" => last_element + 1, + "error" => format!("{:?}", e), + ); + event_queue_ref + .push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); + false + } } + } else { + // chain no longer exists, empty the queue and return + event_queue_ref.clear(); + return false; } } else { - // chain no longer exists, empty the queue and return - event_queue_ref.clear(); - return false; + // not ready to process + true } }); re_run } - fn process_parent_requests(&mut self) { + fn process_parent_requests(&mut self) -> bool { // check to make sure there are peers to search for the parent from if self.full_peers.is_empty() { - return; + return false; } + let mut re_run = false; + // remove any failed requests let debug_log = &self.log; self.parent_queue.retain(|parent_request| { @@ -766,8 +788,10 @@ impl ImportManager { self.event_queue .push(ImportManagerOutcome::RecentRequest(peer_id.clone(), req)); + re_run = true; } } + re_run } fn process_complete_parent_requests(&mut self) -> bool { diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 36947082e..e1ca30b0a 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -453,7 +453,7 @@ impl SimpleSync { } BlockProcessingOutcome::ParentUnknown { parent: _ } => { // Inform the sync manager to find parents for this block - trace!(self.log, "Unknown parent gossip"; + trace!(self.log, "Block with unknown parent received"; "peer_id" => format!("{:?}",peer_id)); self.manager.add_unknown_block(block.clone(), peer_id); SHOULD_FORWARD_GOSSIP_BLOCK diff --git a/beacon_node/src/main.rs b/beacon_node/src/main.rs index ea801cd8b..ab9803eba 100644 --- a/beacon_node/src/main.rs +++ b/beacon_node/src/main.rs @@ -187,13 +187,6 @@ fn main() { .possible_values(&["info", "debug", "trace", "warn", "error", "crit"]) .default_value("trace"), ) - .arg( - Arg::with_name("verbosity") - .short("v") - .multiple(true) - .help("Sets the verbosity level") - .takes_value(true), - ) /* * The "testnet" sub-command. * @@ -332,13 +325,6 @@ fn main() { _ => unreachable!("guarded by clap"), }; - let drain = match matches.occurrences_of("verbosity") { - 0 => drain.filter_level(Level::Info), - 1 => drain.filter_level(Level::Debug), - 2 => drain.filter_level(Level::Trace), - _ => drain.filter_level(Level::Trace), - }; - let log = slog::Logger::root(drain.fuse(), o!()); warn!( From e7ab89a783f02b13a242d7ec528520c12a6e65d1 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 5 Sep 2019 02:06:39 +1000 Subject: [PATCH 4/9] Adds gossipsub object validation and verification --- beacon_node/eth2-libp2p/Cargo.toml | 4 +- beacon_node/eth2-libp2p/src/behaviour.rs | 28 +++++++++-- beacon_node/eth2-libp2p/src/config.rs | 3 +- beacon_node/eth2-libp2p/src/discovery.rs | 1 + beacon_node/eth2-libp2p/src/service.rs | 5 +- beacon_node/network/src/message_handler.rs | 49 +++++++++++++++---- beacon_node/network/src/service.rs | 53 ++++++++++++--------- beacon_node/network/src/sync/simple_sync.rs | 10 ++-- 8 files changed, 106 insertions(+), 47 deletions(-) diff --git a/beacon_node/eth2-libp2p/Cargo.toml b/beacon_node/eth2-libp2p/Cargo.toml index caa5b28e4..59c799105 100644 --- a/beacon_node/eth2-libp2p/Cargo.toml +++ b/beacon_node/eth2-libp2p/Cargo.toml @@ -7,8 +7,8 @@ edition = "2018" [dependencies] clap = "2.32.0" #SigP repository -libp2p = { git = "https://github.com/SigP/rust-libp2p", rev = "61036890d574f5b46573952b20def2baafd6a6e9" } -enr = { git = "https://github.com/SigP/rust-libp2p/", rev = "61036890d574f5b46573952b20def2baafd6a6e9", features = ["serde"] } +libp2p = { git = "https://github.com/SigP/rust-libp2p", rev = "76f7475e4b7063e663ad03c7524cf091f9961968" } +enr = { git = "https://github.com/SigP/rust-libp2p/", rev = "76f7475e4b7063e663ad03c7524cf091f9961968", features = ["serde"] } types = { path = "../../eth2/types" } serde = "1.0" serde_derive = "1.0" diff --git a/beacon_node/eth2-libp2p/src/behaviour.rs b/beacon_node/eth2-libp2p/src/behaviour.rs index 2c574e46a..a47d32ec2 100644 --- a/beacon_node/eth2-libp2p/src/behaviour.rs +++ b/beacon_node/eth2-libp2p/src/behaviour.rs @@ -15,7 +15,7 @@ use libp2p::{ tokio_io::{AsyncRead, AsyncWrite}, NetworkBehaviour, PeerId, }; -use slog::{debug, o, trace}; +use slog::{debug, o}; use std::num::NonZeroU32; use std::time::Duration; @@ -90,13 +90,15 @@ impl NetworkBehaviourEventProcess { - trace!(self.log, "Received GossipEvent"); - + GossipsubEvent::Message(propagation_source, gs_msg) => { + let id = gs_msg.id(); let msg = PubsubMessage::from_topics(&gs_msg.topics, gs_msg.data); + // Note: We are keeping track here of the peer that sent us the message, not the + // peer that originally published the message. self.events.push(BehaviourEvent::GossipMessage { - source: gs_msg.source, + id, + source: propagation_source, topics: gs_msg.topics, message: msg, }); @@ -199,6 +201,13 @@ impl Behaviour { } } + /// Forwards a message that is waiting in gossipsub's mcache. Messages are only propagated + /// once validated by the beacon chain. + pub fn propagate_message(&mut self, propagation_source: &PeerId, message_id: String) { + self.gossipsub + .propagate_message(&message_id, propagation_source); + } + /* Eth2 RPC behaviour functions */ /// Sends an RPC Request/Response via the RPC protocol. @@ -214,12 +223,21 @@ impl Behaviour { /// The types of events than can be obtained from polling the behaviour. pub enum BehaviourEvent { + /// A received RPC event and the peer that it was received from. RPC(PeerId, RPCEvent), + /// We have completed an initial connection to a new peer. PeerDialed(PeerId), + /// A peer has disconnected. PeerDisconnected(PeerId), + /// A gossipsub message has been received. GossipMessage { + /// The gossipsub message id. Used when propagating blocks after validation. + id: String, + /// The peer from which we received this message, not the peer that published it. source: PeerId, + /// The topics that this message was sent on. topics: Vec, + /// The message itself. message: PubsubMessage, }, } diff --git a/beacon_node/eth2-libp2p/src/config.rs b/beacon_node/eth2-libp2p/src/config.rs index 7cb501c1f..fd44b99af 100644 --- a/beacon_node/eth2-libp2p/src/config.rs +++ b/beacon_node/eth2-libp2p/src/config.rs @@ -74,7 +74,8 @@ impl Default for Config { // parameter. gs_config: GossipsubConfigBuilder::new() .max_transmit_size(1_048_576) - .heartbeat_interval(Duration::from_secs(20)) + .heartbeat_interval(Duration::from_secs(20)) // TODO: Reduce for mainnet + .propagate_messages(false) // require validation before propagation .build(), boot_nodes: vec![], libp2p_nodes: vec![], diff --git a/beacon_node/eth2-libp2p/src/discovery.rs b/beacon_node/eth2-libp2p/src/discovery.rs index 4a8aba2b1..759adc482 100644 --- a/beacon_node/eth2-libp2p/src/discovery.rs +++ b/beacon_node/eth2-libp2p/src/discovery.rs @@ -169,6 +169,7 @@ where fn inject_connected(&mut self, peer_id: PeerId, _endpoint: ConnectedPoint) { self.connected_peers.insert(peer_id); + // TODO: Drop peers if over max_peer limit metrics::inc_counter(&metrics::PEER_CONNECT_EVENT_COUNT); metrics::set_gauge(&metrics::PEERS_CONNECTED, self.connected_peers() as i64); diff --git a/beacon_node/eth2-libp2p/src/service.rs b/beacon_node/eth2-libp2p/src/service.rs index 34781927c..3559fb850 100644 --- a/beacon_node/eth2-libp2p/src/service.rs +++ b/beacon_node/eth2-libp2p/src/service.rs @@ -145,16 +145,16 @@ impl Stream for Service { fn poll(&mut self) -> Poll, Self::Error> { loop { match self.swarm.poll() { - //Behaviour events Ok(Async::Ready(Some(event))) => match event { - // TODO: Stub here for debugging BehaviourEvent::GossipMessage { + id, source, topics, message, } => { trace!(self.log, "Gossipsub message received"; "service" => "Swarm"); return Ok(Async::Ready(Some(Libp2pEvent::PubsubMessage { + id, source, topics, message, @@ -222,6 +222,7 @@ pub enum Libp2pEvent { PeerDisconnected(PeerId), /// Received pubsub message. PubsubMessage { + id: String, source: PeerId, topics: Vec, message: PubsubMessage, diff --git a/beacon_node/network/src/message_handler.rs b/beacon_node/network/src/message_handler.rs index c14fc970d..d6e9f8be8 100644 --- a/beacon_node/network/src/message_handler.rs +++ b/beacon_node/network/src/message_handler.rs @@ -21,6 +21,8 @@ pub struct MessageHandler { _chain: Arc>, /// The syncing framework. sync: SimpleSync, + /// A channel to the network service to allow for gossip propagation. + network_send: mpsc::UnboundedSender, /// The `MessageHandler` logger. log: slog::Logger, } @@ -34,8 +36,9 @@ pub enum HandlerMessage { PeerDisconnected(PeerId), /// An RPC response/request has been received. RPC(PeerId, RPCEvent), - /// A gossip message has been received. - PubsubMessage(PeerId, PubsubMessage), + /// A gossip message has been received. The fields are: message id, the peer that sent us this + /// message and the message itself. + PubsubMessage(String, PeerId, PubsubMessage), } impl MessageHandler { @@ -50,12 +53,13 @@ impl MessageHandler { let (handler_send, handler_recv) = mpsc::unbounded_channel(); // Initialise sync and begin processing in thread - let sync = SimpleSync::new(beacon_chain.clone(), network_send, &log); + let sync = SimpleSync::new(beacon_chain.clone(), network_send.clone(), &log); // generate the Message handler let mut handler = MessageHandler { _chain: beacon_chain.clone(), sync, + network_send, log: log.clone(), }; @@ -87,8 +91,8 @@ impl MessageHandler { self.handle_rpc_message(peer_id, rpc_event); } // An RPC message request/response has been received - HandlerMessage::PubsubMessage(peer_id, gossip) => { - self.handle_gossip(peer_id, gossip); + HandlerMessage::PubsubMessage(id, peer_id, gossip) => { + self.handle_gossip(id, peer_id, gossip); } } } @@ -194,24 +198,34 @@ impl MessageHandler { } /// Handle RPC messages - fn handle_gossip(&mut self, peer_id: PeerId, gossip_message: PubsubMessage) { + fn handle_gossip(&mut self, id: String, peer_id: PeerId, gossip_message: PubsubMessage) { match gossip_message { PubsubMessage::Block(message) => match self.decode_gossip_block(message) { Ok(block) => { - let _should_forward_on = self.sync.on_block_gossip(peer_id, block); + let should_forward_on = self.sync.on_block_gossip(peer_id.clone(), block); + // TODO: Apply more sophisticated validation and decoding logic + if should_forward_on { + self.propagate_message(id, peer_id.clone()); + } } Err(e) => { debug!(self.log, "Invalid gossiped beacon block"; "peer_id" => format!("{}", peer_id), "Error" => format!("{:?}", e)); } }, PubsubMessage::Attestation(message) => match self.decode_gossip_attestation(message) { - Ok(attestation) => self.sync.on_attestation_gossip(peer_id, attestation), + Ok(attestation) => { + // TODO: Apply more sophisticated validation and decoding logic + self.propagate_message(id, peer_id.clone()); + self.sync.on_attestation_gossip(peer_id, attestation); + } Err(e) => { debug!(self.log, "Invalid gossiped attestation"; "peer_id" => format!("{}", peer_id), "Error" => format!("{:?}", e)); } }, PubsubMessage::VoluntaryExit(message) => match self.decode_gossip_exit(message) { Ok(_exit) => { + // TODO: Apply more sophisticated validation and decoding logic + self.propagate_message(id, peer_id.clone()); // TODO: Handle exits debug!(self.log, "Received a voluntary exit"; "peer_id" => format!("{}", peer_id) ); } @@ -222,6 +236,8 @@ impl MessageHandler { PubsubMessage::ProposerSlashing(message) => { match self.decode_gossip_proposer_slashing(message) { Ok(_slashing) => { + // TODO: Apply more sophisticated validation and decoding logic + self.propagate_message(id, peer_id.clone()); // TODO: Handle proposer slashings debug!(self.log, "Received a proposer slashing"; "peer_id" => format!("{}", peer_id) ); } @@ -233,6 +249,8 @@ impl MessageHandler { PubsubMessage::AttesterSlashing(message) => { match self.decode_gossip_attestation_slashing(message) { Ok(_slashing) => { + // TODO: Apply more sophisticated validation and decoding logic + self.propagate_message(id, peer_id.clone()); // TODO: Handle attester slashings debug!(self.log, "Received an attester slashing"; "peer_id" => format!("{}", peer_id) ); } @@ -248,6 +266,21 @@ impl MessageHandler { } } + /// Informs the network service that the message should be forwarded to other peers. + fn propagate_message(&mut self, message_id: String, propagation_source: PeerId) { + self.network_send + .try_send(NetworkMessage::Propagate { + propagation_source, + message_id, + }) + .unwrap_or_else(|_| { + warn!( + self.log, + "Could not send propagation request to the network service" + ) + }); + } + /* Decoding of gossipsub objects from the network. * * The decoding is done in the message handler as it has access to to a `BeaconChain` and can diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index a8b3c74b6..5336c7118 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -159,12 +159,23 @@ fn network_service( // poll the network channel match network_recv.poll() { Ok(Async::Ready(Some(message))) => match message { - NetworkMessage::Send(peer_id, outgoing_message) => match outgoing_message { - OutgoingMessage::RPC(rpc_event) => { - trace!(log, "Sending RPC Event: {:?}", rpc_event); - libp2p_service.lock().swarm.send_rpc(peer_id, rpc_event); - } - }, + NetworkMessage::RPC(peer_id, rpc_event) => { + trace!(log, "Sending RPC Event: {:?}", rpc_event); + libp2p_service.lock().swarm.send_rpc(peer_id, rpc_event); + } + NetworkMessage::Propagate { + propagation_source, + message_id, + } => { + trace!(log, "Propagating gossipsub message"; + "propagation_peer" => format!("{:?}", propagation_source), + "message_id" => format!("{}", message_id), + ); + libp2p_service + .lock() + .swarm + .propagate_message(&propagation_source, message_id); + } NetworkMessage::Publish { topics, message } => { debug!(log, "Sending pubsub message"; "topics" => format!("{:?}",topics)); libp2p_service.lock().swarm.publish(&topics, message); @@ -203,13 +214,14 @@ fn network_service( .map_err(|_| "Failed to send PeerDisconnected to handler")?; } Libp2pEvent::PubsubMessage { - source, message, .. + id, + source, + message, + .. } => { - //TODO: Decide if we need to propagate the topic upwards. (Potentially for - //attestations) message_handler_send - .try_send(HandlerMessage::PubsubMessage(source, message)) - .map_err(|_| " failed to send pubsub message to handler")?; + .try_send(HandlerMessage::PubsubMessage(id, source, message)) + .map_err(|_| "Failed to send pubsub message to handler")?; } }, Ok(Async::Ready(None)) => unreachable!("Stream never ends"), @@ -225,19 +237,16 @@ fn network_service( /// Types of messages that the network service can receive. #[derive(Debug)] pub enum NetworkMessage { - /// Send a message to libp2p service. - //TODO: Define typing for messages across the wire - Send(PeerId, OutgoingMessage), - /// Publish a message to pubsub mechanism. + /// Send an RPC message to the libp2p service. + RPC(PeerId, RPCEvent), + /// Publish a message to gossipsub. Publish { topics: Vec, message: PubsubMessage, }, -} - -/// Type of outgoing messages that can be sent through the network service. -#[derive(Debug)] -pub enum OutgoingMessage { - /// Send an RPC request/response. - RPC(RPCEvent), + /// Propagate a received gossipsub message + Propagate { + propagation_source: PeerId, + message_id: String, + }, } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 573ac9dd1..789f5b6be 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -1,5 +1,5 @@ use super::manager::{ImportManager, ImportManagerOutcome}; -use crate::service::{NetworkMessage, OutgoingMessage}; +use crate::service::NetworkMessage; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; use eth2_libp2p::rpc::methods::*; use eth2_libp2p::rpc::{RPCEvent, RPCRequest, RPCResponse, RequestId}; @@ -468,7 +468,7 @@ impl SimpleSync { SHOULD_FORWARD_GOSSIP_BLOCK } BlockProcessingOutcome::BlockIsAlreadyKnown => SHOULD_FORWARD_GOSSIP_BLOCK, - _ => SHOULD_NOT_FORWARD_GOSSIP_BLOCK, + _ => SHOULD_NOT_FORWARD_GOSSIP_BLOCK, //TODO: Decide if we want to forward these } } else { SHOULD_NOT_FORWARD_GOSSIP_BLOCK @@ -554,12 +554,8 @@ impl NetworkContext { } fn send_rpc_event(&mut self, peer_id: PeerId, rpc_event: RPCEvent) { - self.send(peer_id, OutgoingMessage::RPC(rpc_event)) - } - - fn send(&mut self, peer_id: PeerId, outgoing_message: OutgoingMessage) { self.network_send - .try_send(NetworkMessage::Send(peer_id, outgoing_message)) + .try_send(NetworkMessage::RPC(peer_id, rpc_event)) .unwrap_or_else(|_| { warn!( self.log, From a3877b6135272a29a0d0e43f5a36f4c43d73a5ab Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 5 Sep 2019 08:07:57 +1000 Subject: [PATCH 5/9] Updates syncing stability, fixes large RPC message codec, corrects beacon chain referencing --- beacon_node/client/src/notifier.rs | 4 +- beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs | 36 ++- beacon_node/eth2-libp2p/src/rpc/methods.rs | 6 +- beacon_node/eth2-libp2p/src/service.rs | 5 + beacon_node/network/src/message_handler.rs | 7 +- beacon_node/network/src/sync/manager.rs | 211 +++++++------ beacon_node/network/src/sync/simple_sync.rs | 304 +++++++++++-------- 7 files changed, 312 insertions(+), 261 deletions(-) diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index d705637cb..343918d4d 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -34,10 +34,10 @@ pub fn run(client: &Client, executor: TaskExecutor, exit // Panics if libp2p is poisoned. let connected_peer_count = libp2p.lock().swarm.connected_peers(); - debug!(log, "Libp2p connected peer status"; "peer_count" => connected_peer_count); + debug!(log, "Connected peer status"; "peer_count" => connected_peer_count); if connected_peer_count <= WARN_PEER_COUNT { - warn!(log, "Low libp2p peer count"; "peer_count" => connected_peer_count); + warn!(log, "Low peer count"; "peer_count" => connected_peer_count); } Ok(()) diff --git a/beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs b/beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs index 260a00346..1966bab62 100644 --- a/beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs +++ b/beacon_node/eth2-libp2p/src/rpc/codec/ssz.rs @@ -152,45 +152,49 @@ impl Decoder for SSZOutboundCodec { type Error = RPCError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - match self.inner.decode(src).map_err(RPCError::from) { - Ok(Some(packet)) => match self.protocol.message_name.as_str() { + if src.is_empty() { + // the object sent could be empty. We return the empty object if this is the case + match self.protocol.message_name.as_str() { "hello" => match self.protocol.version.as_str() { - "1" => Ok(Some(RPCResponse::Hello(HelloMessage::from_ssz_bytes( - &packet, - )?))), + "1" => Err(RPCError::Custom( + "Hello stream terminated unexpectedly".into(), + )), // cannot have an empty HELLO message. The stream has terminated unexpectedly _ => unreachable!("Cannot negotiate an unknown version"), }, "goodbye" => Err(RPCError::InvalidProtocol("GOODBYE doesn't have a response")), "beacon_blocks" => match self.protocol.version.as_str() { - "1" => Ok(Some(RPCResponse::BeaconBlocks(packet.to_vec()))), + "1" => Ok(Some(RPCResponse::BeaconBlocks(Vec::new()))), _ => unreachable!("Cannot negotiate an unknown version"), }, "recent_beacon_blocks" => match self.protocol.version.as_str() { - "1" => Ok(Some(RPCResponse::RecentBeaconBlocks(packet.to_vec()))), + "1" => Ok(Some(RPCResponse::RecentBeaconBlocks(Vec::new()))), _ => unreachable!("Cannot negotiate an unknown version"), }, _ => unreachable!("Cannot negotiate an unknown protocol"), - }, - Ok(None) => { - // the object sent could be a empty. We return the empty object if this is the case - match self.protocol.message_name.as_str() { + } + } else { + match self.inner.decode(src).map_err(RPCError::from) { + Ok(Some(packet)) => match self.protocol.message_name.as_str() { "hello" => match self.protocol.version.as_str() { - "1" => Ok(None), // cannot have an empty HELLO message. The stream has terminated unexpectedly + "1" => Ok(Some(RPCResponse::Hello(HelloMessage::from_ssz_bytes( + &packet, + )?))), _ => unreachable!("Cannot negotiate an unknown version"), }, "goodbye" => Err(RPCError::InvalidProtocol("GOODBYE doesn't have a response")), "beacon_blocks" => match self.protocol.version.as_str() { - "1" => Ok(Some(RPCResponse::BeaconBlocks(Vec::new()))), + "1" => Ok(Some(RPCResponse::BeaconBlocks(packet.to_vec()))), _ => unreachable!("Cannot negotiate an unknown version"), }, "recent_beacon_blocks" => match self.protocol.version.as_str() { - "1" => Ok(Some(RPCResponse::RecentBeaconBlocks(Vec::new()))), + "1" => Ok(Some(RPCResponse::RecentBeaconBlocks(packet.to_vec()))), _ => unreachable!("Cannot negotiate an unknown version"), }, _ => unreachable!("Cannot negotiate an unknown protocol"), - } + }, + Ok(None) => Ok(None), // waiting for more bytes + Err(e) => Err(e), } - Err(e) => Err(e), } } } diff --git a/beacon_node/eth2-libp2p/src/rpc/methods.rs b/beacon_node/eth2-libp2p/src/rpc/methods.rs index c9610b000..49813abe9 100644 --- a/beacon_node/eth2-libp2p/src/rpc/methods.rs +++ b/beacon_node/eth2-libp2p/src/rpc/methods.rs @@ -168,8 +168,10 @@ impl std::fmt::Display for RPCResponse { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { RPCResponse::Hello(hello) => write!(f, "{}", hello), - RPCResponse::BeaconBlocks(_) => write!(f, ""), - RPCResponse::RecentBeaconBlocks(_) => write!(f, ""), + RPCResponse::BeaconBlocks(data) => write!(f, ", len: {}", data.len()), + RPCResponse::RecentBeaconBlocks(data) => { + write!(f, ", len: {}", data.len()) + } } } } diff --git a/beacon_node/eth2-libp2p/src/service.rs b/beacon_node/eth2-libp2p/src/service.rs index 9f08b1eda..dac011752 100644 --- a/beacon_node/eth2-libp2p/src/service.rs +++ b/beacon_node/eth2-libp2p/src/service.rs @@ -98,6 +98,11 @@ impl Service { // attempt to connect to any specified boot-nodes for bootnode_enr in config.boot_nodes { for multiaddr in bootnode_enr.multiaddr() { + // ignore udp multiaddr if it exists + let components = multiaddr.iter().collect::>(); + if let Protocol::Udp(_) = components[1] { + continue; + } dial_addr(multiaddr); } } diff --git a/beacon_node/network/src/message_handler.rs b/beacon_node/network/src/message_handler.rs index d6e9f8be8..cade65d63 100644 --- a/beacon_node/network/src/message_handler.rs +++ b/beacon_node/network/src/message_handler.rs @@ -17,8 +17,6 @@ use types::{Attestation, AttesterSlashing, BeaconBlock, ProposerSlashing, Volunt /// Handles messages received from the network and client and organises syncing. pub struct MessageHandler { - /// Currently loaded and initialised beacon chain. - _chain: Arc>, /// The syncing framework. sync: SimpleSync, /// A channel to the network service to allow for gossip propagation. @@ -53,13 +51,12 @@ impl MessageHandler { let (handler_send, handler_recv) = mpsc::unbounded_channel(); // Initialise sync and begin processing in thread - let sync = SimpleSync::new(beacon_chain.clone(), network_send.clone(), &log); + let sync = SimpleSync::new(Arc::downgrade(&beacon_chain), network_send.clone(), &log); // generate the Message handler let mut handler = MessageHandler { - _chain: beacon_chain.clone(), - sync, network_send, + sync, log: log.clone(), }; diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 1eec51843..2b2ed9dca 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -62,13 +62,13 @@ use slog::{debug, info, trace, warn, Logger}; use smallvec::SmallVec; use std::collections::{HashMap, HashSet}; use std::ops::{Add, Sub}; -use std::sync::{Arc, Weak}; +use std::sync::Weak; use types::{BeaconBlock, EthSpec, Hash256, Slot}; /// Blocks are downloaded in batches from peers. This constant specifies how many blocks per batch /// is requested. Currently the value is small for testing. This will be incremented for /// production. -const MAX_BLOCKS_PER_REQUEST: u64 = 100; +const MAX_BLOCKS_PER_REQUEST: u64 = 50; /// The number of slots ahead of us that is allowed before requesting a long-range (batch) Sync /// from a peer. If a peer is within this tolerance (forwards or backwards), it is treated as a @@ -224,10 +224,10 @@ impl ImportManager { /// Generates a new `ImportManager` given a logger and an Arc reference to a beacon chain. The /// import manager keeps a weak reference to the beacon chain, which allows the chain to be /// dropped during the syncing process. The syncing handles this termination gracefully. - pub fn new(beacon_chain: Arc>, log: &slog::Logger) -> Self { + pub fn new(beacon_chain: Weak>, log: &slog::Logger) -> Self { ImportManager { event_queue: SmallVec::new(), - chain: Arc::downgrade(&beacon_chain), + chain: beacon_chain, state: ManagerState::Regular, import_queue: HashMap::new(), parent_queue: SmallVec::new(), @@ -359,7 +359,9 @@ impl ImportManager { warn!(self.log, "Peer returned too many empty block batches"; "peer" => format!("{:?}", peer_id)); block_requests.state = BlockRequestsState::Failed; - } else if block_requests.current_start_slot >= block_requests.target_head_slot { + } else if block_requests.current_start_slot + MAX_BLOCKS_PER_REQUEST + >= block_requests.target_head_slot + { warn!(self.log, "Peer did not return blocks it claimed to possess"; "peer" => format!("{:?}", peer_id)); block_requests.state = BlockRequestsState::Failed; @@ -583,6 +585,11 @@ impl ImportManager { re_run = re_run || self.process_complete_parent_requests(); } + // exit early if the beacon chain is dropped + if let None = self.chain.upgrade() { + return ImportManagerOutcome::Idle; + } + // return any queued events if !self.event_queue.is_empty() { let event = self.event_queue.remove(0); @@ -681,56 +688,48 @@ impl ImportManager { self.import_queue.retain(|peer_id, block_requests| { if block_requests.state == BlockRequestsState::ReadyToProcess { - // check that the chain still exists - if let Some(chain) = chain_ref.upgrade() { - let downloaded_blocks = - std::mem::replace(&mut block_requests.downloaded_blocks, Vec::new()); - let last_element = downloaded_blocks.len() - 1; - let start_slot = downloaded_blocks[0].slot; - let end_slot = downloaded_blocks[last_element].slot; + let downloaded_blocks = + std::mem::replace(&mut block_requests.downloaded_blocks, Vec::new()); + let last_element = downloaded_blocks.len() - 1; + let start_slot = downloaded_blocks[0].slot; + let end_slot = downloaded_blocks[last_element].slot; - match process_blocks(chain, downloaded_blocks, log_ref) { - Ok(()) => { - debug!(log_ref, "Blocks processed successfully"; + match process_blocks(chain_ref.clone(), downloaded_blocks, log_ref) { + Ok(()) => { + debug!(log_ref, "Blocks processed successfully"; + "peer" => format!("{:?}", peer_id), + "start_slot" => start_slot, + "end_slot" => end_slot, + "no_blocks" => last_element + 1, + ); + block_requests.blocks_processed += last_element + 1; + + // check if the batch is complete, by verifying if we have reached the + // target head + if end_slot >= block_requests.target_head_slot { + // Completed, re-hello the peer to ensure we are up to the latest head + event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); + // remove the request + false + } else { + // have not reached the end, queue another batch + block_requests.update_start_slot(); + re_run = true; + // keep the batch + true + } + } + Err(e) => { + warn!(log_ref, "Block processing failed"; "peer" => format!("{:?}", peer_id), "start_slot" => start_slot, "end_slot" => end_slot, "no_blocks" => last_element + 1, - ); - block_requests.blocks_processed += last_element + 1; - - // check if the batch is complete, by verifying if we have reached the - // target head - if end_slot >= block_requests.target_head_slot { - // Completed, re-hello the peer to ensure we are up to the latest head - event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); - // remove the request - false - } else { - // have not reached the end, queue another batch - block_requests.update_start_slot(); - re_run = true; - // keep the batch - true - } - } - Err(e) => { - warn!(log_ref, "Block processing failed"; - "peer" => format!("{:?}", peer_id), - "start_slot" => start_slot, - "end_slot" => end_slot, - "no_blocks" => last_element + 1, - "error" => format!("{:?}", e), - ); - event_queue_ref - .push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); - false - } + "error" => format!("{:?}", e), + ); + event_queue_ref.push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); + false } - } else { - // chain no longer exists, empty the queue and return - event_queue_ref.clear(); - return false; } } else { // not ready to process @@ -894,42 +893,43 @@ impl ImportManager { // Helper function to process blocks fn process_blocks( - chain: Arc>, + weak_chain: Weak>, blocks: Vec>, log: &Logger, ) -> Result<(), String> { for block in blocks { - let processing_result = chain.process_block(block.clone()); + if let Some(chain) = weak_chain.upgrade() { + let processing_result = chain.process_block(block.clone()); - if let Ok(outcome) = processing_result { - match outcome { - BlockProcessingOutcome::Processed { block_root } => { - // The block was valid and we processed it successfully. - trace!( - log, "Imported block from network"; - "slot" => block.slot, - "block_root" => format!("{}", block_root), - ); - } - BlockProcessingOutcome::ParentUnknown { parent } => { - // blocks should be sequential and all parents should exist - trace!( - log, "Parent block is unknown"; - "parent_root" => format!("{}", parent), - "baby_block_slot" => block.slot, - ); - return Err(format!( - "Block at slot {} has an unknown parent.", - block.slot - )); - } - BlockProcessingOutcome::BlockIsAlreadyKnown => { - // this block is already known to us, move to the next - debug!( - log, "Imported a block that is already known"; - "parent_root" => format!("{}", parent), - "baby_block_slot" => block.slot, - ); + if let Ok(outcome) = processing_result { + match outcome { + BlockProcessingOutcome::Processed { block_root } => { + // The block was valid and we processed it successfully. + trace!( + log, "Imported block from network"; + "slot" => block.slot, + "block_root" => format!("{}", block_root), + ); + } + BlockProcessingOutcome::ParentUnknown { parent } => { + // blocks should be sequential and all parents should exist + trace!( + log, "Parent block is unknown"; + "parent_root" => format!("{}", parent), + "baby_block_slot" => block.slot, + ); + return Err(format!( + "Block at slot {} has an unknown parent.", + block.slot + )); + } + BlockProcessingOutcome::BlockIsAlreadyKnown => { + // this block is already known to us, move to the next + debug!( + log, "Imported a block that is already known"; + "block_slot" => block.slot, + ); + } BlockProcessingOutcome::FutureSlot { present_slot, block_slot, @@ -937,7 +937,7 @@ fn process_blocks( if present_slot + FUTURE_SLOT_TOLERANCE >= block_slot { // The block is too far in the future, drop it. trace!( - self.log, "Block is ahead of our slot clock"; + log, "Block is ahead of our slot clock"; "msg" => "block for future slot rejected, check your time", "present_slot" => present_slot, "block_slot" => block_slot, @@ -950,7 +950,7 @@ fn process_blocks( } else { // The block is in the future, but not too far. trace!( - self.log, "Block is slightly ahead of our slot clock, ignoring."; + log, "Block is slightly ahead of our slot clock, ignoring."; "present_slot" => present_slot, "block_slot" => block_slot, "FUTURE_SLOT_TOLERANCE" => FUTURE_SLOT_TOLERANCE, @@ -959,44 +959,41 @@ fn process_blocks( } BlockProcessingOutcome::WouldRevertFinalizedSlot { .. } => { trace!( - self.log, "Finalized or earlier block processed"; + log, "Finalized or earlier block processed"; "outcome" => format!("{:?}", outcome), ); // block reached our finalized slot or was earlier, move to the next block } BlockProcessingOutcome::GenesisBlock => { trace!( - self.log, "Genesis block was processed"; + log, "Genesis block was processed"; "outcome" => format!("{:?}", outcome), ); } - BlockProcessingOutcome::FinalizedSlot => { - trace!( - log, "Finalized or earlier block processed"; - "outcome" => format!("{:?}", outcome), - ); - // block reached our finalized slot or was earlier, move to the next block - } - _ => { - warn!( - log, "Invalid block received"; - "msg" => "peer sent invalid block", - "outcome" => format!("{:?}", outcome), - ); - return Err(format!("Invalid block at slot {}", block.slot)); + _ => { + warn!( + log, "Invalid block received"; + "msg" => "peer sent invalid block", + "outcome" => format!("{:?}", outcome), + ); + return Err(format!("Invalid block at slot {}", block.slot)); + } } + } else { + warn!( + log, "BlockProcessingFailure"; + "msg" => "unexpected condition in processing block.", + "outcome" => format!("{:?}", processing_result) + ); + return Err(format!( + "Unexpected block processing error: {:?}", + processing_result + )); } } else { - warn!( - log, "BlockProcessingFailure"; - "msg" => "unexpected condition in processing block.", - "outcome" => format!("{:?}", processing_result) - ); - return Err(format!( - "Unexpected block processing error: {:?}", - processing_result - )); + return Ok(()); // terminate early due to dropped beacon chain } } + Ok(()) } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 9d05b312b..a8b271700 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -6,7 +6,7 @@ use eth2_libp2p::rpc::{RPCEvent, RPCRequest, RPCResponse, RequestId}; use eth2_libp2p::PeerId; use slog::{debug, info, o, trace, warn}; use ssz::Encode; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use store::Store; use tokio::sync::mpsc; use types::{Attestation, BeaconBlock, Epoch, EthSpec, Hash256, Slot}; @@ -57,7 +57,7 @@ pub enum SyncState { /// Simple Syncing protocol. pub struct SimpleSync { /// A reference to the underlying beacon chain. - chain: Arc>, + chain: Weak>, manager: ImportManager, network: NetworkContext, log: slog::Logger, @@ -66,7 +66,7 @@ pub struct SimpleSync { impl SimpleSync { /// Instantiate a `SimpleSync` instance, with no peers and an empty queue. pub fn new( - beacon_chain: Arc>, + beacon_chain: Weak>, network_send: mpsc::UnboundedSender, log: &slog::Logger, ) -> Self { @@ -91,8 +91,10 @@ impl SimpleSync { /// /// Sends a `Hello` message to the peer. pub fn on_connect(&mut self, peer_id: PeerId) { - self.network - .send_rpc_request(None, peer_id, RPCRequest::Hello(hello_message(&self.chain))); + if let Some(chain) = self.chain.upgrade() { + self.network + .send_rpc_request(None, peer_id, RPCRequest::Hello(hello_message(&chain))); + } } /// Handle a `Hello` request. @@ -104,16 +106,19 @@ impl SimpleSync { request_id: RequestId, hello: HelloMessage, ) { - trace!(self.log, "HelloRequest"; "peer" => format!("{:?}", peer_id)); + // ignore hello responses if we are shutting down + if let Some(chain) = self.chain.upgrade() { + trace!(self.log, "HelloRequest"; "peer" => format!("{:?}", peer_id)); - // Say hello back. - self.network.send_rpc_response( - peer_id.clone(), - request_id, - RPCResponse::Hello(hello_message(&self.chain)), - ); + // Say hello back. + self.network.send_rpc_response( + peer_id.clone(), + request_id, + RPCResponse::Hello(hello_message(&chain)), + ); - self.process_hello(peer_id, hello); + self.process_hello(peer_id, hello); + } } /// Process a `Hello` response from a peer. @@ -128,88 +133,107 @@ impl SimpleSync { /// /// Disconnects the peer if required. fn process_hello(&mut self, peer_id: PeerId, hello: HelloMessage) { - let remote = PeerSyncInfo::from(hello); - let local = PeerSyncInfo::from(&self.chain); - - let start_slot = |epoch: Epoch| epoch.start_slot(T::EthSpec::slots_per_epoch()); - - if local.fork_version != remote.fork_version { - // The node is on a different network/fork, disconnect them. - debug!( - self.log, "HandshakeFailure"; - "peer" => format!("{:?}", peer_id), - "reason" => "network_id" - ); - - self.network - .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); - } else if remote.finalized_epoch <= local.finalized_epoch - && remote.finalized_root != Hash256::zero() - && local.finalized_root != Hash256::zero() - && (self.root_at_slot(start_slot(remote.finalized_epoch)) - != Some(remote.finalized_root)) + // If we update the manager we may need to drive the sync. This flag lies out of scope of + // the beacon chain so that the process sync command has no long-lived beacon chain + // references. + let mut process_sync = false; { - // The remotes finalized epoch is less than or greater than ours, but the block root is - // different to the one in our chain. - // - // Therefore, the node is on a different chain and we should not communicate with them. - debug!( - self.log, "HandshakeFailure"; - "peer" => format!("{:?}", peer_id), - "reason" => "different finalized chain" - ); - self.network - .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); - } else if remote.finalized_epoch < local.finalized_epoch { - // The node has a lower finalized epoch, their chain is not useful to us. There are two - // cases where a node can have a lower finalized epoch: - // - // ## The node is on the same chain - // - // If a node is on the same chain but has a lower finalized epoch, their head must be - // lower than ours. Therefore, we have nothing to request from them. - // - // ## The node is on a fork - // - // If a node is on a fork that has a lower finalized epoch, switching to that fork would - // cause us to revert a finalized block. This is not permitted, therefore we have no - // interest in their blocks. - debug!( - self.log, - "NaivePeer"; - "peer" => format!("{:?}", peer_id), - "reason" => "lower finalized epoch" - ); - } else if self - .chain - .store - .exists::>(&remote.head_root) - .unwrap_or_else(|_| false) - { - trace!( - self.log, "Out of date or potentially sync'd peer found"; - "peer" => format!("{:?}", peer_id), - "remote_head_slot" => remote.head_slot, - "remote_latest_finalized_epoch" => remote.finalized_epoch, - ); + // scope of beacon chain reference + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + info!(self.log, "Sync shutting down"; + "reason" => "Beacon chain dropped"); + return; + } + }; - // If the node's best-block is already known to us and they are close to our current - // head, treat them as a fully sync'd peer. - self.manager.add_peer(peer_id, remote); - self.process_sync(); - } else { - // The remote node has an equal or great finalized epoch and we don't know it's head. - // - // Therefore, there are some blocks between the local finalized epoch and the remote - // head that are worth downloading. - debug!( - self.log, "UsefulPeer"; - "peer" => format!("{:?}", peer_id), - "local_finalized_epoch" => local.finalized_epoch, - "remote_latest_finalized_epoch" => remote.finalized_epoch, - ); + let remote = PeerSyncInfo::from(hello); + let local = PeerSyncInfo::from(&chain); - self.manager.add_peer(peer_id, remote); + let start_slot = |epoch: Epoch| epoch.start_slot(T::EthSpec::slots_per_epoch()); + + if local.fork_version != remote.fork_version { + // The node is on a different network/fork, disconnect them. + debug!( + self.log, "HandshakeFailure"; + "peer" => format!("{:?}", peer_id), + "reason" => "network_id" + ); + + self.network + .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); + } else if remote.finalized_epoch <= local.finalized_epoch + && remote.finalized_root != Hash256::zero() + && local.finalized_root != Hash256::zero() + && (chain.root_at_slot(start_slot(remote.finalized_epoch)) + != Some(remote.finalized_root)) + { + // The remotes finalized epoch is less than or greater than ours, but the block root is + // different to the one in our chain. + // + // Therefore, the node is on a different chain and we should not communicate with them. + debug!( + self.log, "HandshakeFailure"; + "peer" => format!("{:?}", peer_id), + "reason" => "different finalized chain" + ); + self.network + .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); + } else if remote.finalized_epoch < local.finalized_epoch { + // The node has a lower finalized epoch, their chain is not useful to us. There are two + // cases where a node can have a lower finalized epoch: + // + // ## The node is on the same chain + // + // If a node is on the same chain but has a lower finalized epoch, their head must be + // lower than ours. Therefore, we have nothing to request from them. + // + // ## The node is on a fork + // + // If a node is on a fork that has a lower finalized epoch, switching to that fork would + // cause us to revert a finalized block. This is not permitted, therefore we have no + // interest in their blocks. + debug!( + self.log, + "NaivePeer"; + "peer" => format!("{:?}", peer_id), + "reason" => "lower finalized epoch" + ); + } else if chain + .store + .exists::>(&remote.head_root) + .unwrap_or_else(|_| false) + { + trace!( + self.log, "Peer with known chain found"; + "peer" => format!("{:?}", peer_id), + "remote_head_slot" => remote.head_slot, + "remote_latest_finalized_epoch" => remote.finalized_epoch, + ); + + // If the node's best-block is already known to us and they are close to our current + // head, treat them as a fully sync'd peer. + self.manager.add_peer(peer_id, remote); + process_sync = true; + } else { + // The remote node has an equal or great finalized epoch and we don't know it's head. + // + // Therefore, there are some blocks between the local finalized epoch and the remote + // head that are worth downloading. + debug!( + self.log, "UsefulPeer"; + "peer" => format!("{:?}", peer_id), + "local_finalized_epoch" => local.finalized_epoch, + "remote_latest_finalized_epoch" => remote.finalized_epoch, + ); + + self.manager.add_peer(peer_id, remote); + process_sync = true + } + } // end beacon chain reference scope + + if process_sync { self.process_sync(); } } @@ -226,11 +250,13 @@ impl SimpleSync { "method" => "HELLO", "peer" => format!("{:?}", peer_id) ); - self.network.send_rpc_request( - None, - peer_id, - RPCRequest::Hello(hello_message(&self.chain)), - ); + if let Some(chain) = self.chain.upgrade() { + self.network.send_rpc_request( + None, + peer_id, + RPCRequest::Hello(hello_message(&chain)), + ); + } } ImportManagerOutcome::RequestBlocks { peer_id, @@ -283,14 +309,6 @@ impl SimpleSync { } } - //TODO: Move to beacon chain - fn root_at_slot(&self, target_slot: Slot) -> Option { - self.chain - .rev_iter_block_roots() - .find(|(_root, slot)| *slot == target_slot) - .map(|(root, _slot)| root) - } - /// Handle a `RecentBeaconBlocks` request from the peer. pub fn on_recent_beacon_blocks_request( &mut self, @@ -298,11 +316,20 @@ impl SimpleSync { request_id: RequestId, request: RecentBeaconBlocksRequest, ) { + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + info!(self.log, "Sync shutting down"; + "reason" => "Beacon chain dropped"); + return; + } + }; + let blocks: Vec> = request .block_roots .iter() .filter_map(|root| { - if let Ok(Some(block)) = self.chain.store.get::>(root) { + if let Ok(Some(block)) = chain.store.get::>(root) { Some(block) } else { debug!( @@ -319,7 +346,7 @@ impl SimpleSync { debug!( self.log, - "BlockBodiesRequest"; + "RecentBeaconBlocksRequest"; "peer" => format!("{:?}", peer_id), "requested" => request.block_roots.len(), "returned" => blocks.len(), @@ -339,6 +366,15 @@ impl SimpleSync { request_id: RequestId, req: BeaconBlocksRequest, ) { + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + info!(self.log, "Sync shutting down"; + "reason" => "Beacon chain dropped"); + return; + } + }; + debug!( self.log, "BeaconBlocksRequest"; @@ -352,15 +388,14 @@ impl SimpleSync { // In the current implementation we read from the db then filter out out-of-range blocks. // Improving the db schema to prevent this would be ideal. - let mut blocks: Vec> = self - .chain + let mut blocks: Vec> = chain .rev_iter_block_roots() .filter(|(_root, slot)| { req.start_slot <= slot.as_u64() && req.start_slot + req.count > slot.as_u64() }) .take_while(|(_root, slot)| req.start_slot <= slot.as_u64()) .filter_map(|(root, _slot)| { - if let Ok(Some(block)) = self.chain.store.get::>(&root) { + if let Ok(Some(block)) = chain.store.get::>(&root) { Some(block) } else { warn!( @@ -378,18 +413,16 @@ impl SimpleSync { blocks.reverse(); blocks.dedup_by_key(|brs| brs.slot); - if blocks.len() as u64 != req.count { - debug!( - self.log, - "BeaconBlocksRequest response"; - "peer" => format!("{:?}", peer_id), - "msg" => "Failed to return all requested hashes", - "start_slot" => req.start_slot, - "current_slot" => format!("{:?}", self.chain.slot()), - "requested" => req.count, - "returned" => blocks.len(), - ); - } + debug!( + self.log, + "BeaconBlocksRequest response"; + "peer" => format!("{:?}", peer_id), + "msg" => "Failed to return all requested hashes", + "start_slot" => req.start_slot, + "current_slot" => chain.slot().unwrap_or_else(|_| Slot::from(0_u64)).as_u64(), + "requested" => req.count, + "returned" => blocks.len(), + ); self.network.send_rpc_response( peer_id, @@ -444,7 +477,16 @@ impl SimpleSync { /// /// Returns a `bool` which, if `true`, indicates we should forward the block to our peers. pub fn on_block_gossip(&mut self, peer_id: PeerId, block: BeaconBlock) -> bool { - if let Ok(outcome) = self.chain.process_block(block.clone()) { + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + info!(self.log, "Sync shutting down"; + "reason" => "Beacon chain dropped"); + return false; + } + }; + + if let Ok(outcome) = chain.process_block(block.clone()) { match outcome { BlockProcessingOutcome::Processed { .. } => { trace!(self.log, "Gossipsub block processed"; @@ -477,7 +519,16 @@ impl SimpleSync { /// /// Not currently implemented. pub fn on_attestation_gossip(&mut self, _peer_id: PeerId, msg: Attestation) { - match self.chain.process_attestation(msg) { + let chain = match self.chain.upgrade() { + Some(chain) => chain, + None => { + info!(self.log, "Sync shutting down"; + "reason" => "Beacon chain dropped"); + return; + } + }; + + match chain.process_attestation(msg) { Ok(outcome) => info!( self.log, "Processed attestation"; @@ -489,11 +540,6 @@ impl SimpleSync { } } } - - /// Generates our current state in the form of a HELLO RPC message. - pub fn generate_hello(&self) -> HelloMessage { - hello_message(&self.chain) - } } /// Build a `HelloMessage` representing the state of the given `beacon_chain`. From ee25766caea02711fceb71950f847678ccadc6fc Mon Sep 17 00:00:00 2001 From: Age Manning Date: Thu, 5 Sep 2019 22:18:17 +1000 Subject: [PATCH 6/9] Correct recent beacon block request bug --- beacon_node/network/src/sync/manager.rs | 20 ++++++++++++++------ beacon_node/network/src/sync/simple_sync.rs | 12 ++++++++---- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 2b2ed9dca..fa1315c39 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -187,7 +187,11 @@ pub(crate) enum ImportManagerOutcome { request: BeaconBlocksRequest, }, /// A `RecentBeaconBlocks` request is required. - RecentRequest(PeerId, RecentBeaconBlocksRequest), + RecentRequest { + peer_id: PeerId, + request_id: RequestId, + request: RecentBeaconBlocksRequest, + }, /// Updates information with peer via requesting another HELLO handshake. Hello(PeerId), /// A peer has caused a punishable error and should be downvoted. @@ -532,7 +536,7 @@ impl ImportManager { pub fn add_unknown_block(&mut self, block: BeaconBlock, peer_id: PeerId) { // if we are not in regular sync mode, ignore this block - if let ManagerState::Regular = self.state { + if self.state != ManagerState::Regular { return; } @@ -774,19 +778,23 @@ impl ImportManager { continue; } - parent_request.state = BlockRequestsState::Pending(self.current_req_id); + let request_id = self.current_req_id; + parent_request.state = BlockRequestsState::Pending(request_id); self.current_req_id += 1; let last_element_index = parent_request.downloaded_blocks.len() - 1; let parent_hash = parent_request.downloaded_blocks[last_element_index].parent_root; - let req = RecentBeaconBlocksRequest { + let request = RecentBeaconBlocksRequest { block_roots: vec![parent_hash], }; // select a random fully synced peer to attempt to download the parent block let peer_id = self.full_peers.iter().next().expect("List is not empty"); - self.event_queue - .push(ImportManagerOutcome::RecentRequest(peer_id.clone(), req)); + self.event_queue.push(ImportManagerOutcome::RecentRequest { + peer_id: peer_id.clone(), + request_id, + request, + }); re_run = true; } } diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index a8b271700..4a853f05d 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -277,18 +277,22 @@ impl SimpleSync { RPCRequest::BeaconBlocks(request), ); } - ImportManagerOutcome::RecentRequest(peer_id, req) => { + ImportManagerOutcome::RecentRequest { + peer_id, + request_id, + request, + } => { trace!( self.log, "RPC Request"; "method" => "RecentBeaconBlocks", - "count" => req.block_roots.len(), + "count" => request.block_roots.len(), "peer" => format!("{:?}", peer_id) ); self.network.send_rpc_request( - None, + Some(request_id), peer_id.clone(), - RPCRequest::RecentBeaconBlocks(req), + RPCRequest::RecentBeaconBlocks(request), ); } ImportManagerOutcome::DownvotePeer(peer_id) => { From 812e1fbe2691bcbebd623ffdeb2adc1101e2aa0e Mon Sep 17 00:00:00 2001 From: Age Manning Date: Sat, 7 Sep 2019 00:28:54 +1000 Subject: [PATCH 7/9] Implements a new thread dedicated for syncing --- beacon_node/network/src/message_handler.rs | 52 +- beacon_node/network/src/sync/manager.rs | 502 +++++++++++++------- beacon_node/network/src/sync/mod.rs | 2 +- beacon_node/network/src/sync/simple_sync.rs | 415 ++++++---------- 4 files changed, 514 insertions(+), 457 deletions(-) diff --git a/beacon_node/network/src/message_handler.rs b/beacon_node/network/src/message_handler.rs index cade65d63..be8fa21f8 100644 --- a/beacon_node/network/src/message_handler.rs +++ b/beacon_node/network/src/message_handler.rs @@ -1,6 +1,6 @@ use crate::error; use crate::service::NetworkMessage; -use crate::sync::SimpleSync; +use crate::sync::MessageProcessor; use beacon_chain::{BeaconChain, BeaconChainTypes}; use eth2_libp2p::{ behaviour::PubsubMessage, @@ -15,12 +15,16 @@ use std::sync::Arc; use tokio::sync::mpsc; use types::{Attestation, AttesterSlashing, BeaconBlock, ProposerSlashing, VoluntaryExit}; -/// Handles messages received from the network and client and organises syncing. +/// Handles messages received from the network and client and organises syncing. This +/// functionality of this struct is to validate an decode messages from the network before +/// passing them to the internal message processor. The message processor spawns a syncing thread +/// which manages which blocks need to be requested and processed. pub struct MessageHandler { - /// The syncing framework. - sync: SimpleSync, /// A channel to the network service to allow for gossip propagation. network_send: mpsc::UnboundedSender, + /// Processes validated and decoded messages from the network. Has direct access to the + /// sync manager. + message_processor: MessageProcessor, /// The `MessageHandler` logger. log: slog::Logger, } @@ -50,13 +54,15 @@ impl MessageHandler { trace!(log, "Service starting"); let (handler_send, handler_recv) = mpsc::unbounded_channel(); - // Initialise sync and begin processing in thread - let sync = SimpleSync::new(Arc::downgrade(&beacon_chain), network_send.clone(), &log); + + // Initialise a message instance, which itself spawns the syncing thread. + let message_processor = + MessageProcessor::new(executor, beacon_chain, network_send.clone(), &log); // generate the Message handler let mut handler = MessageHandler { network_send, - sync, + message_processor, log: log.clone(), }; @@ -66,7 +72,11 @@ impl MessageHandler { .for_each(move |msg| Ok(handler.handle_message(msg))) .map_err(move |_| { debug!(log, "Network message handler terminated."); - }), + }), /* + .then(move |_| { + debug!(log.clone(), "Message handler shutdown"); + }), + */ ); Ok(handler_send) @@ -77,11 +87,11 @@ impl MessageHandler { match message { // we have initiated a connection to a peer HandlerMessage::PeerDialed(peer_id) => { - self.sync.on_connect(peer_id); + self.message_processor.on_connect(peer_id); } // A peer has disconnected HandlerMessage::PeerDisconnected(peer_id) => { - self.sync.on_disconnect(peer_id); + self.message_processor.on_disconnect(peer_id); } // An RPC message request/response has been received HandlerMessage::RPC(peer_id, rpc_event) => { @@ -109,7 +119,7 @@ impl MessageHandler { fn handle_rpc_request(&mut self, peer_id: PeerId, request_id: RequestId, request: RPCRequest) { match request { RPCRequest::Hello(hello_message) => { - self.sync + self.message_processor .on_hello_request(peer_id, request_id, hello_message) } RPCRequest::Goodbye(goodbye_reason) => { @@ -118,13 +128,13 @@ impl MessageHandler { "peer" => format!("{:?}", peer_id), "reason" => format!("{:?}", goodbye_reason), ); - self.sync.on_disconnect(peer_id); + self.message_processor.on_disconnect(peer_id); } RPCRequest::BeaconBlocks(request) => self - .sync + .message_processor .on_beacon_blocks_request(peer_id, request_id, request), RPCRequest::RecentBeaconBlocks(request) => self - .sync + .message_processor .on_recent_beacon_blocks_request(peer_id, request_id, request), } } @@ -151,12 +161,13 @@ impl MessageHandler { RPCErrorResponse::Success(response) => { match response { RPCResponse::Hello(hello_message) => { - self.sync.on_hello_response(peer_id, hello_message); + self.message_processor + .on_hello_response(peer_id, hello_message); } RPCResponse::BeaconBlocks(response) => { match self.decode_beacon_blocks(&response) { Ok(beacon_blocks) => { - self.sync.on_beacon_blocks_response( + self.message_processor.on_beacon_blocks_response( peer_id, request_id, beacon_blocks, @@ -171,7 +182,7 @@ impl MessageHandler { RPCResponse::RecentBeaconBlocks(response) => { match self.decode_beacon_blocks(&response) { Ok(beacon_blocks) => { - self.sync.on_recent_beacon_blocks_response( + self.message_processor.on_recent_beacon_blocks_response( peer_id, request_id, beacon_blocks, @@ -199,7 +210,9 @@ impl MessageHandler { match gossip_message { PubsubMessage::Block(message) => match self.decode_gossip_block(message) { Ok(block) => { - let should_forward_on = self.sync.on_block_gossip(peer_id.clone(), block); + let should_forward_on = self + .message_processor + .on_block_gossip(peer_id.clone(), block); // TODO: Apply more sophisticated validation and decoding logic if should_forward_on { self.propagate_message(id, peer_id.clone()); @@ -213,7 +226,8 @@ impl MessageHandler { Ok(attestation) => { // TODO: Apply more sophisticated validation and decoding logic self.propagate_message(id, peer_id.clone()); - self.sync.on_attestation_gossip(peer_id, attestation); + self.message_processor + .on_attestation_gossip(peer_id, attestation); } Err(e) => { debug!(self.log, "Invalid gossiped attestation"; "peer_id" => format!("{}", peer_id), "Error" => format!("{:?}", e)); diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index fa1315c39..12bef95fa 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -1,4 +1,4 @@ -//! The `ImportManager` facilities the block syncing logic of lighthouse. The current networking +//! The `SyncManager` facilities the block syncing logic of lighthouse. The current networking //! specification provides two methods from which to obtain blocks from peers. The `BeaconBlocks` //! request and the `RecentBeaconBlocks` request. The former is used to obtain a large number of //! blocks and the latter allows for searching for blocks given a block-hash. @@ -7,7 +7,7 @@ //! - Long range (batch) sync, when a client is out of date and needs to the latest head. //! - Parent lookup - when a peer provides us a block whose parent is unknown to us. //! -//! Both of these syncing strategies are built into the `ImportManager`. +//! Both of these syncing strategies are built into the `SyncManager`. //! //! //! Currently the long-range (batch) syncing method functions by opportunistically downloading @@ -53,16 +53,18 @@ //! fully sync'd peers. If `PARENT_FAIL_TOLERANCE` attempts at requesting the block fails, we //! drop the propagated block and downvote the peer that sent it to us. -use super::simple_sync::{PeerSyncInfo, FUTURE_SLOT_TOLERANCE}; +use super::simple_sync::{hello_message, NetworkContext, PeerSyncInfo, FUTURE_SLOT_TOLERANCE}; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; use eth2_libp2p::rpc::methods::*; -use eth2_libp2p::rpc::RequestId; +use eth2_libp2p::rpc::{RPCRequest, RequestId}; use eth2_libp2p::PeerId; +use futures::prelude::*; use slog::{debug, info, trace, warn, Logger}; use smallvec::SmallVec; use std::collections::{HashMap, HashSet}; use std::ops::{Add, Sub}; use std::sync::Weak; +use tokio::sync::{mpsc, oneshot}; use types::{BeaconBlock, EthSpec, Hash256, Slot}; /// Blocks are downloaded in batches from peers. This constant specifies how many blocks per batch @@ -84,6 +86,31 @@ const PARENT_DEPTH_TOLERANCE: usize = SLOT_IMPORT_TOLERANCE * 2; /// requests to peers who never return blocks. const EMPTY_BATCH_TOLERANCE: usize = 100; +#[derive(Debug)] +/// A message than can be sent to the sync manager thread. +pub enum SyncMessage { + /// A useful peer has been discovered. + AddPeer(PeerId, PeerSyncInfo), + /// A `BeaconBlocks` response has been received. + BeaconBlocksResponse { + peer_id: PeerId, + request_id: RequestId, + beacon_blocks: Vec>, + }, + /// A `RecentBeaconBlocks` response has been received. + RecentBeaconBlocksResponse { + peer_id: PeerId, + request_id: RequestId, + beacon_blocks: Vec>, + }, + /// A block with an unknown parent has been received. + UnknownBlock(PeerId, BeaconBlock), + /// A peer has disconnected. + Disconnect(PeerId), + /// An RPC Error has occurred on a request. + _RPCError(RequestId), +} + #[derive(PartialEq)] /// The current state of a block or batches lookup. enum BlockRequestsState { @@ -176,39 +203,19 @@ enum ManagerState { Stalled, } -/// The output states that can occur from driving (polling) the manager state machine. -pub(crate) enum ImportManagerOutcome { - /// There is no further work to complete. The manager is waiting for further input. - Idle, - /// A `BeaconBlocks` request is required. - RequestBlocks { - peer_id: PeerId, - request_id: RequestId, - request: BeaconBlocksRequest, - }, - /// A `RecentBeaconBlocks` request is required. - RecentRequest { - peer_id: PeerId, - request_id: RequestId, - request: RecentBeaconBlocksRequest, - }, - /// Updates information with peer via requesting another HELLO handshake. - Hello(PeerId), - /// A peer has caused a punishable error and should be downvoted. - DownvotePeer(PeerId), -} - /// The primary object for handling and driving all the current syncing logic. It maintains the /// current state of the syncing process, the number of useful peers, downloaded blocks and /// controls the logic behind both the long-range (batch) sync and the on-going potential parent /// look-up of blocks. -pub struct ImportManager { - /// List of events to be processed externally. - event_queue: SmallVec<[ImportManagerOutcome; 20]>, +pub struct SyncManager { /// A weak reference to the underlying beacon chain. chain: Weak>, /// The current state of the import manager. state: ManagerState, + /// A receiving channel sent by the message processor thread. + input_channel: mpsc::UnboundedReceiver>, + /// A network context to contact the network service. + network: NetworkContext, /// A collection of `BlockRequest` per peer that is currently being downloaded. Used in the /// long-range (batch) sync process. import_queue: HashMap>, @@ -224,22 +231,51 @@ pub struct ImportManager { log: Logger, } -impl ImportManager { - /// Generates a new `ImportManager` given a logger and an Arc reference to a beacon chain. The - /// import manager keeps a weak reference to the beacon chain, which allows the chain to be - /// dropped during the syncing process. The syncing handles this termination gracefully. - pub fn new(beacon_chain: Weak>, log: &slog::Logger) -> Self { - ImportManager { - event_queue: SmallVec::new(), - chain: beacon_chain, - state: ManagerState::Regular, - import_queue: HashMap::new(), - parent_queue: SmallVec::new(), - full_peers: HashSet::new(), - current_req_id: 0, - log: log.clone(), - } - } +/// Spawns a new `SyncManager` thread which has a weak reference to underlying beacon +/// chain. This allows the chain to be +/// dropped during the syncing process which will gracefully end the `SyncManager`. +pub fn spawn( + executor: &tokio::runtime::TaskExecutor, + beacon_chain: Weak>, + network: NetworkContext, + log: slog::Logger, +) -> ( + mpsc::UnboundedSender>, + oneshot::Sender<()>, +) { + // generate the exit channel + let (sync_exit, exit_rx) = tokio::sync::oneshot::channel(); + // generate the message channel + let (sync_send, sync_recv) = mpsc::unbounded_channel::>(); + + // create an instance of the SyncManager + let sync_manager = SyncManager { + chain: beacon_chain, + state: ManagerState::Regular, + input_channel: sync_recv, + network, + import_queue: HashMap::new(), + parent_queue: SmallVec::new(), + full_peers: HashSet::new(), + current_req_id: 0, + log: log.clone(), + }; + + // spawn the sync manager thread + debug!(log, "Sync Manager started"); + executor.spawn( + sync_manager + .select(exit_rx.then(|_| Ok(()))) + .then(move |_| { + info!(log.clone(), "Sync Manager shutdown"); + Ok(()) + }), + ); + (sync_send, sync_exit) +} + +impl SyncManager { + /* Input Handling Functions */ /// A peer has connected which has blocks that are unknown to us. /// @@ -281,7 +317,7 @@ impl ImportManager { return; } - // Check if the peer is significantly is behind us. If within `SLOT_IMPORT_TOLERANCE` + // Check if the peer is significantly behind us. If within `SLOT_IMPORT_TOLERANCE` // treat them as a fully synced peer. If not, ignore them in the sync process if local.head_slot.sub(remote.head_slot).as_usize() < SLOT_IMPORT_TOLERANCE { self.add_full_peer(peer_id.clone()); @@ -328,8 +364,7 @@ impl ImportManager { let chain = match self.chain.upgrade() { Some(chain) => chain, None => { - debug!(self.log, "Chain dropped. Sync terminating"); - self.event_queue.clear(); + trace!(self.log, "Chain dropped. Sync terminating"); return; } }; @@ -390,8 +425,7 @@ impl ImportManager { "request_id" => request_id, "response_initial_slot" => blocks[0].slot, "requested_initial_slot" => block_requests.current_start_slot); - self.event_queue - .push(ImportManagerOutcome::DownvotePeer(peer_id)); + downvote_peer(&mut self.network, &self.log, peer_id); // consider this sync failed block_requests.state = BlockRequestsState::Failed; return; @@ -515,26 +549,7 @@ impl ImportManager { parent_request.state = BlockRequestsState::ReadyToProcess; } - pub fn _inject_error(_peer_id: PeerId, _id: RequestId) { - //TODO: Remove block state from pending - } - - pub fn peer_disconnect(&mut self, peer_id: &PeerId) { - self.import_queue.remove(peer_id); - self.full_peers.remove(peer_id); - self.update_state(); - } - - pub fn add_full_peer(&mut self, peer_id: PeerId) { - debug!( - self.log, "Fully synced peer added"; - "peer" => format!("{:?}", peer_id), - ); - self.full_peers.insert(peer_id); - self.update_state(); - } - - pub fn add_unknown_block(&mut self, block: BeaconBlock, peer_id: PeerId) { + fn add_unknown_block(&mut self, peer_id: PeerId, block: BeaconBlock) { // if we are not in regular sync mode, ignore this block if self.state != ManagerState::Regular { return; @@ -563,55 +578,29 @@ impl ImportManager { self.parent_queue.push(req); } - pub(crate) fn poll(&mut self) -> ImportManagerOutcome { - loop { - //TODO: Optimize the lookups. Potentially keep state of whether each of these functions - //need to be called. - - // only break once everything has been processed - let mut re_run = false; - - // only process batch requests if there are any - if !self.import_queue.is_empty() { - // process potential block requests - re_run = re_run || self.process_potential_block_requests(); - - // process any complete long-range batches - re_run = re_run || self.process_complete_batches(); - } - - // only process parent objects if we are in regular sync - if !self.parent_queue.is_empty() { - // process any parent block lookup-requests - re_run = re_run || self.process_parent_requests(); - - // process any complete parent lookups - re_run = re_run || self.process_complete_parent_requests(); - } - - // exit early if the beacon chain is dropped - if let None = self.chain.upgrade() { - return ImportManagerOutcome::Idle; - } - - // return any queued events - if !self.event_queue.is_empty() { - let event = self.event_queue.remove(0); - self.event_queue.shrink_to_fit(); - return event; - } - - // update the state of the manager - self.update_state(); - - if !re_run { - break; - } - } - - return ImportManagerOutcome::Idle; + fn inject_error(&mut self, _id: RequestId) { + //TODO: Remove block state from pending } + fn peer_disconnect(&mut self, peer_id: &PeerId) { + self.import_queue.remove(peer_id); + self.full_peers.remove(peer_id); + self.update_state(); + } + + fn add_full_peer(&mut self, peer_id: PeerId) { + debug!( + self.log, "Fully synced peer added"; + "peer" => format!("{:?}", peer_id), + ); + self.full_peers.insert(peer_id); + self.update_state(); + } + + /* Processing State Functions */ + // These functions are called in the main poll function to transition the state of the sync + // manager + fn update_state(&mut self) { let previous_state = self.state.clone(); self.state = { @@ -631,13 +620,12 @@ impl ImportManager { } } - fn process_potential_block_requests(&mut self) -> bool { + fn process_potential_block_requests(&mut self) { // check if an outbound request is required // Managing a fixed number of outbound requests is maintained at the RPC protocol libp2p // layer and not needed here. Therefore we create many outbound requests and let the RPC // handle the number of simultaneous requests. Request all queued objects. - let mut re_run = false; // remove any failed batches let debug_log = &self.log; let full_peer_ref = &mut self.full_peers; @@ -655,40 +643,40 @@ impl ImportManager { }); // process queued block requests - for (peer_id, block_requests) in self - .import_queue - .iter_mut() - .find(|(_peer_id, req)| req.state == BlockRequestsState::Queued) - { - let request_id = self.current_req_id; - block_requests.state = BlockRequestsState::Pending(request_id); - self.current_req_id += 1; + for (peer_id, block_requests) in self.import_queue.iter_mut() { + { + if block_requests.state == BlockRequestsState::Queued { + let request_id = self.current_req_id; + block_requests.state = BlockRequestsState::Pending(request_id); + self.current_req_id += 1; - let request = BeaconBlocksRequest { - head_block_root: block_requests.target_head_root, - start_slot: block_requests.current_start_slot.as_u64(), - count: MAX_BLOCKS_PER_REQUEST, - step: 0, - }; - self.event_queue.push(ImportManagerOutcome::RequestBlocks { - peer_id: peer_id.clone(), - request, - request_id, - }); - re_run = true; + let request = BeaconBlocksRequest { + head_block_root: block_requests.target_head_root, + start_slot: block_requests.current_start_slot.as_u64(), + count: MAX_BLOCKS_PER_REQUEST, + step: 0, + }; + request_blocks( + &mut self.network, + &self.log, + peer_id.clone(), + request_id, + request, + ); + } + } } - - re_run } fn process_complete_batches(&mut self) -> bool { - // flag to indicate if the manager can be switched to idle or not - let mut re_run = false; + // This function can queue extra blocks and the main poll loop will need to be re-executed + // to process these. This flag indicates that the main poll loop has to continue. + let mut re_run_poll = false; // create reference variables to be moved into subsequent closure let chain_ref = self.chain.clone(); let log_ref = &self.log; - let event_queue_ref = &mut self.event_queue; + let network_ref = &mut self.network; self.import_queue.retain(|peer_id, block_requests| { if block_requests.state == BlockRequestsState::ReadyToProcess { @@ -712,13 +700,13 @@ impl ImportManager { // target head if end_slot >= block_requests.target_head_slot { // Completed, re-hello the peer to ensure we are up to the latest head - event_queue_ref.push(ImportManagerOutcome::Hello(peer_id.clone())); + hello_peer(network_ref, log_ref, chain_ref.clone(), peer_id.clone()); // remove the request false } else { // have not reached the end, queue another batch block_requests.update_start_slot(); - re_run = true; + re_run_poll = true; // keep the batch true } @@ -731,7 +719,7 @@ impl ImportManager { "no_blocks" => last_element + 1, "error" => format!("{:?}", e), ); - event_queue_ref.push(ImportManagerOutcome::DownvotePeer(peer_id.clone())); + downvote_peer(network_ref, log_ref, peer_id.clone()); false } } @@ -741,17 +729,15 @@ impl ImportManager { } }); - re_run + re_run_poll } - fn process_parent_requests(&mut self) -> bool { + fn process_parent_requests(&mut self) { // check to make sure there are peers to search for the parent from if self.full_peers.is_empty() { - return false; + return; } - let mut re_run = false; - // remove any failed requests let debug_log = &self.log; self.parent_queue.retain(|parent_request| { @@ -790,20 +776,20 @@ impl ImportManager { // select a random fully synced peer to attempt to download the parent block let peer_id = self.full_peers.iter().next().expect("List is not empty"); - self.event_queue.push(ImportManagerOutcome::RecentRequest { - peer_id: peer_id.clone(), + recent_blocks_request( + &mut self.network, + &self.log, + peer_id.clone(), request_id, request, - }); - re_run = true; + ); } } - re_run } fn process_complete_parent_requests(&mut self) -> bool { // returned value indicating whether the manager can be switched to idle or not - let mut re_run = false; + let mut re_run_poll = false; // Find any parent_requests ready to be processed for completed_request in self @@ -827,9 +813,8 @@ impl ImportManager { "received_block" => format!("{}", block_hash), "expected_parent" => format!("{}", expected_hash), ); - re_run = true; - self.event_queue - .push(ImportManagerOutcome::DownvotePeer(peer)); + re_run_poll = true; + downvote_peer(&mut self.network, &self.log, peer); } // try and process the list of blocks up to the requested block @@ -846,7 +831,7 @@ impl ImportManager { // need to keep looking for parents completed_request.downloaded_blocks.push(block); completed_request.state = BlockRequestsState::Queued; - re_run = true; + re_run_poll = true; break; } Ok(BlockProcessingOutcome::Processed { block_root: _ }) => {} @@ -859,11 +844,13 @@ impl ImportManager { "peer" => format!("{:?}", completed_request.last_submitted_peer), ); completed_request.state = BlockRequestsState::Queued; - re_run = true; - self.event_queue.push(ImportManagerOutcome::DownvotePeer( + re_run_poll = true; + downvote_peer( + &mut self.network, + &self.log, completed_request.last_submitted_peer.clone(), - )); - return re_run; + ); + return re_run_poll; } Err(e) => { completed_request.failed_attempts += 1; @@ -872,16 +859,17 @@ impl ImportManager { "error" => format!("{:?}", e) ); completed_request.state = BlockRequestsState::Queued; - re_run = true; - self.event_queue.push(ImportManagerOutcome::DownvotePeer( + re_run_poll = true; + downvote_peer( + &mut self.network, + &self.log, completed_request.last_submitted_peer.clone(), - )); - return re_run; + ); + return re_run_poll; } } } else { // chain doesn't exist - clear the event queue and return - self.event_queue.clear(); return false; } } @@ -895,11 +883,83 @@ impl ImportManager { true } }); - re_run + re_run_poll } } -// Helper function to process blocks +/* Network Context Helper Functions */ + +fn hello_peer( + network: &mut NetworkContext, + log: &slog::Logger, + chain: Weak>, + peer_id: PeerId, +) { + trace!( + log, + "RPC Request"; + "method" => "HELLO", + "peer" => format!("{:?}", peer_id) + ); + if let Some(chain) = chain.upgrade() { + network.send_rpc_request(None, peer_id, RPCRequest::Hello(hello_message(&chain))); + } +} + +fn request_blocks( + network: &mut NetworkContext, + log: &slog::Logger, + peer_id: PeerId, + request_id: RequestId, + request: BeaconBlocksRequest, +) { + trace!( + log, + "RPC Request"; + "method" => "BeaconBlocks", + "id" => request_id, + "count" => request.count, + "peer" => format!("{:?}", peer_id) + ); + network.send_rpc_request( + Some(request_id), + peer_id.clone(), + RPCRequest::BeaconBlocks(request), + ); +} + +fn recent_blocks_request( + network: &mut NetworkContext, + log: &slog::Logger, + peer_id: PeerId, + request_id: RequestId, + request: RecentBeaconBlocksRequest, +) { + trace!( + log, + "RPC Request"; + "method" => "RecentBeaconBlocks", + "count" => request.block_roots.len(), + "peer" => format!("{:?}", peer_id) + ); + network.send_rpc_request( + Some(request_id), + peer_id.clone(), + RPCRequest::RecentBeaconBlocks(request), + ); +} + +fn downvote_peer(network: &mut NetworkContext, log: &slog::Logger, peer_id: PeerId) { + trace!( + log, + "Peer downvoted"; + "peer" => format!("{:?}", peer_id) + ); + // TODO: Implement reputation + network.disconnect(peer_id.clone(), GoodbyeReason::Fault); +} + +// Helper function to process blocks which only consumes the chain and blocks to process fn process_blocks( weak_chain: Weak>, blocks: Vec>, @@ -1005,3 +1065,99 @@ fn process_blocks( Ok(()) } + +impl Future for SyncManager { + type Item = (); + type Error = String; + + fn poll(&mut self) -> Result, Self::Error> { + // process any inbound messages + loop { + match self.input_channel.poll() { + Ok(Async::Ready(Some(message))) => match message { + SyncMessage::AddPeer(peer_id, info) => { + self.add_peer(peer_id, info); + dbg!("add peer"); + } + SyncMessage::BeaconBlocksResponse { + peer_id, + request_id, + beacon_blocks, + } => { + self.beacon_blocks_response(peer_id, request_id, beacon_blocks); + } + SyncMessage::RecentBeaconBlocksResponse { + peer_id, + request_id, + beacon_blocks, + } => { + self.recent_blocks_response(peer_id, request_id, beacon_blocks); + } + SyncMessage::UnknownBlock(peer_id, block) => { + self.add_unknown_block(peer_id, block); + } + SyncMessage::Disconnect(peer_id) => { + self.peer_disconnect(&peer_id); + } + SyncMessage::_RPCError(request_id) => { + self.inject_error(request_id); + } + }, + Ok(Async::NotReady) => break, + Ok(Async::Ready(None)) => { + return Err("Sync manager channel closed".into()); + } + Err(e) => { + return Err(format!("Sync Manager channel error: {:?}", e)); + } + } + } + + loop { + //TODO: Optimize the lookups. Potentially keep state of whether each of these functions + //need to be called. + let mut re_run = false; + + dbg!(self.import_queue.len()); + // only process batch requests if there are any + if !self.import_queue.is_empty() { + // process potential block requests + self.process_potential_block_requests(); + + dbg!(self.import_queue.len()); + // process any complete long-range batches + re_run = re_run || self.process_complete_batches(); + dbg!(self.import_queue.len()); + dbg!(&self.state); + } + + // only process parent objects if we are in regular sync + if !self.parent_queue.is_empty() { + // process any parent block lookup-requests + self.process_parent_requests(); + + // process any complete parent lookups + re_run = re_run || self.process_complete_parent_requests(); + } + + dbg!(self.import_queue.len()); + dbg!(&self.state); + + // Shutdown the thread if the chain has termined + if let None = self.chain.upgrade() { + return Ok(Async::Ready(())); + } + + if !re_run { + break; + } + } + dbg!(self.import_queue.len()); + dbg!(&self.state); + + // update the state of the manager + self.update_state(); + + return Ok(Async::NotReady); + } +} diff --git a/beacon_node/network/src/sync/mod.rs b/beacon_node/network/src/sync/mod.rs index b26d78c14..58ec386aa 100644 --- a/beacon_node/network/src/sync/mod.rs +++ b/beacon_node/network/src/sync/mod.rs @@ -4,7 +4,7 @@ mod manager; /// Stores the various syncing methods for the beacon chain. mod simple_sync; -pub use simple_sync::SimpleSync; +pub use simple_sync::MessageProcessor; /// Currently implemented sync methods. pub enum SyncMethod { diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index 4a853f05d..d8b5f2dbf 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -1,4 +1,4 @@ -use super::manager::{ImportManager, ImportManagerOutcome}; +use super::manager::SyncMessage; use crate::service::NetworkMessage; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockProcessingOutcome}; use eth2_libp2p::rpc::methods::*; @@ -6,11 +6,14 @@ use eth2_libp2p::rpc::{RPCEvent, RPCRequest, RPCResponse, RequestId}; use eth2_libp2p::PeerId; use slog::{debug, info, o, trace, warn}; use ssz::Encode; -use std::sync::{Arc, Weak}; +use std::sync::Arc; use store::Store; -use tokio::sync::mpsc; +use tokio::sync::{mpsc, oneshot}; use types::{Attestation, BeaconBlock, Epoch, EthSpec, Hash256, Slot}; +//TODO: Put a maximum limit on the number of block that can be requested. +//TODO: Rate limit requests + /// If a block is more than `FUTURE_SLOT_TOLERANCE` slots ahead of our slot clock, we drop it. /// Otherwise we queue it. pub(crate) const FUTURE_SLOT_TOLERANCE: u64 = 1; @@ -46,55 +49,71 @@ impl From<&Arc>> for PeerSyncInfo { } } -/// The current syncing state. -#[derive(PartialEq)] -pub enum SyncState { - _Idle, - _Downloading, - _Stopped, -} - -/// Simple Syncing protocol. -pub struct SimpleSync { +/// Processes validated messages from the network. It relays necessary data to the syncing thread +/// and processes blocks from the pubsub network. +pub struct MessageProcessor { /// A reference to the underlying beacon chain. - chain: Weak>, - manager: ImportManager, + chain: Arc>, + /// A channel to the syncing thread. + sync_send: mpsc::UnboundedSender>, + /// A oneshot channel for destroying the sync thread. + _sync_exit: oneshot::Sender<()>, + /// A nextwork context to return and handle RPC requests. network: NetworkContext, + /// The `RPCHandler` logger. log: slog::Logger, } -impl SimpleSync { - /// Instantiate a `SimpleSync` instance, with no peers and an empty queue. +impl MessageProcessor { + /// Instantiate a `MessageProcessor` instance pub fn new( - beacon_chain: Weak>, + executor: &tokio::runtime::TaskExecutor, + beacon_chain: Arc>, network_send: mpsc::UnboundedSender, log: &slog::Logger, ) -> Self { let sync_logger = log.new(o!("Service"=> "Sync")); + let sync_network_context = NetworkContext::new(network_send.clone(), sync_logger.clone()); - SimpleSync { - chain: beacon_chain.clone(), - manager: ImportManager::new(beacon_chain, log), + // spawn the sync thread + let (sync_send, _sync_exit) = super::manager::spawn( + executor, + Arc::downgrade(&beacon_chain), + sync_network_context, + sync_logger, + ); + + MessageProcessor { + chain: beacon_chain, + sync_send, + _sync_exit, network: NetworkContext::new(network_send, log.clone()), - log: sync_logger, + log: log.clone(), } } + fn send_to_sync(&mut self, message: SyncMessage) { + self.sync_send.try_send(message).unwrap_or_else(|_| { + warn!( + self.log, + "Could not send message to the sync service"; + ) + }); + } + /// Handle a peer disconnect. /// /// Removes the peer from the manager. pub fn on_disconnect(&mut self, peer_id: PeerId) { - self.manager.peer_disconnect(&peer_id); + self.send_to_sync(SyncMessage::Disconnect(peer_id)); } /// Handle the connection of a new peer. /// /// Sends a `Hello` message to the peer. pub fn on_connect(&mut self, peer_id: PeerId) { - if let Some(chain) = self.chain.upgrade() { - self.network - .send_rpc_request(None, peer_id, RPCRequest::Hello(hello_message(&chain))); - } + self.network + .send_rpc_request(None, peer_id, RPCRequest::Hello(hello_message(&self.chain))); } /// Handle a `Hello` request. @@ -107,18 +126,16 @@ impl SimpleSync { hello: HelloMessage, ) { // ignore hello responses if we are shutting down - if let Some(chain) = self.chain.upgrade() { - trace!(self.log, "HelloRequest"; "peer" => format!("{:?}", peer_id)); + trace!(self.log, "HelloRequest"; "peer" => format!("{:?}", peer_id)); - // Say hello back. - self.network.send_rpc_response( - peer_id.clone(), - request_id, - RPCResponse::Hello(hello_message(&chain)), - ); + // Say hello back. + self.network.send_rpc_response( + peer_id.clone(), + request_id, + RPCResponse::Hello(hello_message(&self.chain)), + ); - self.process_hello(peer_id, hello); - } + self.process_hello(peer_id, hello); } /// Process a `Hello` response from a peer. @@ -133,183 +150,86 @@ impl SimpleSync { /// /// Disconnects the peer if required. fn process_hello(&mut self, peer_id: PeerId, hello: HelloMessage) { - // If we update the manager we may need to drive the sync. This flag lies out of scope of - // the beacon chain so that the process sync command has no long-lived beacon chain - // references. - let mut process_sync = false; + let remote = PeerSyncInfo::from(hello); + let local = PeerSyncInfo::from(&self.chain); + + let start_slot = |epoch: Epoch| epoch.start_slot(T::EthSpec::slots_per_epoch()); + + if local.fork_version != remote.fork_version { + // The node is on a different network/fork, disconnect them. + debug!( + self.log, "HandshakeFailure"; + "peer" => format!("{:?}", peer_id), + "reason" => "network_id" + ); + + self.network + .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); + } else if remote.finalized_epoch <= local.finalized_epoch + && remote.finalized_root != Hash256::zero() + && local.finalized_root != Hash256::zero() + && (self.chain.root_at_slot(start_slot(remote.finalized_epoch)) + != Some(remote.finalized_root)) { - // scope of beacon chain reference - let chain = match self.chain.upgrade() { - Some(chain) => chain, - None => { - info!(self.log, "Sync shutting down"; - "reason" => "Beacon chain dropped"); - return; - } - }; + // The remotes finalized epoch is less than or greater than ours, but the block root is + // different to the one in our chain. + // + // Therefore, the node is on a different chain and we should not communicate with them. + debug!( + self.log, "HandshakeFailure"; + "peer" => format!("{:?}", peer_id), + "reason" => "different finalized chain" + ); + self.network + .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); + } else if remote.finalized_epoch < local.finalized_epoch { + // The node has a lower finalized epoch, their chain is not useful to us. There are two + // cases where a node can have a lower finalized epoch: + // + // ## The node is on the same chain + // + // If a node is on the same chain but has a lower finalized epoch, their head must be + // lower than ours. Therefore, we have nothing to request from them. + // + // ## The node is on a fork + // + // If a node is on a fork that has a lower finalized epoch, switching to that fork would + // cause us to revert a finalized block. This is not permitted, therefore we have no + // interest in their blocks. + debug!( + self.log, + "NaivePeer"; + "peer" => format!("{:?}", peer_id), + "reason" => "lower finalized epoch" + ); + } else if self + .chain + .store + .exists::>(&remote.head_root) + .unwrap_or_else(|_| false) + { + trace!( + self.log, "Peer with known chain found"; + "peer" => format!("{:?}", peer_id), + "remote_head_slot" => remote.head_slot, + "remote_latest_finalized_epoch" => remote.finalized_epoch, + ); - let remote = PeerSyncInfo::from(hello); - let local = PeerSyncInfo::from(&chain); - - let start_slot = |epoch: Epoch| epoch.start_slot(T::EthSpec::slots_per_epoch()); - - if local.fork_version != remote.fork_version { - // The node is on a different network/fork, disconnect them. - debug!( - self.log, "HandshakeFailure"; - "peer" => format!("{:?}", peer_id), - "reason" => "network_id" - ); - - self.network - .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); - } else if remote.finalized_epoch <= local.finalized_epoch - && remote.finalized_root != Hash256::zero() - && local.finalized_root != Hash256::zero() - && (chain.root_at_slot(start_slot(remote.finalized_epoch)) - != Some(remote.finalized_root)) - { - // The remotes finalized epoch is less than or greater than ours, but the block root is - // different to the one in our chain. - // - // Therefore, the node is on a different chain and we should not communicate with them. - debug!( - self.log, "HandshakeFailure"; - "peer" => format!("{:?}", peer_id), - "reason" => "different finalized chain" - ); - self.network - .disconnect(peer_id.clone(), GoodbyeReason::IrrelevantNetwork); - } else if remote.finalized_epoch < local.finalized_epoch { - // The node has a lower finalized epoch, their chain is not useful to us. There are two - // cases where a node can have a lower finalized epoch: - // - // ## The node is on the same chain - // - // If a node is on the same chain but has a lower finalized epoch, their head must be - // lower than ours. Therefore, we have nothing to request from them. - // - // ## The node is on a fork - // - // If a node is on a fork that has a lower finalized epoch, switching to that fork would - // cause us to revert a finalized block. This is not permitted, therefore we have no - // interest in their blocks. - debug!( - self.log, - "NaivePeer"; - "peer" => format!("{:?}", peer_id), - "reason" => "lower finalized epoch" - ); - } else if chain - .store - .exists::>(&remote.head_root) - .unwrap_or_else(|_| false) - { - trace!( - self.log, "Peer with known chain found"; - "peer" => format!("{:?}", peer_id), - "remote_head_slot" => remote.head_slot, - "remote_latest_finalized_epoch" => remote.finalized_epoch, - ); - - // If the node's best-block is already known to us and they are close to our current - // head, treat them as a fully sync'd peer. - self.manager.add_peer(peer_id, remote); - process_sync = true; - } else { - // The remote node has an equal or great finalized epoch and we don't know it's head. - // - // Therefore, there are some blocks between the local finalized epoch and the remote - // head that are worth downloading. - debug!( - self.log, "UsefulPeer"; - "peer" => format!("{:?}", peer_id), - "local_finalized_epoch" => local.finalized_epoch, - "remote_latest_finalized_epoch" => remote.finalized_epoch, - ); - - self.manager.add_peer(peer_id, remote); - process_sync = true - } - } // end beacon chain reference scope - - if process_sync { - self.process_sync(); - } - } - - /// This function drives the `ImportManager` state machine. The outcomes it provides are - /// actioned until the `ImportManager` is idle. - fn process_sync(&mut self) { - loop { - match self.manager.poll() { - ImportManagerOutcome::Hello(peer_id) => { - trace!( - self.log, - "RPC Request"; - "method" => "HELLO", - "peer" => format!("{:?}", peer_id) - ); - if let Some(chain) = self.chain.upgrade() { - self.network.send_rpc_request( - None, - peer_id, - RPCRequest::Hello(hello_message(&chain)), - ); - } - } - ImportManagerOutcome::RequestBlocks { - peer_id, - request_id, - request, - } => { - trace!( - self.log, - "RPC Request"; - "method" => "BeaconBlocks", - "id" => request_id, - "count" => request.count, - "peer" => format!("{:?}", peer_id) - ); - self.network.send_rpc_request( - Some(request_id), - peer_id.clone(), - RPCRequest::BeaconBlocks(request), - ); - } - ImportManagerOutcome::RecentRequest { - peer_id, - request_id, - request, - } => { - trace!( - self.log, - "RPC Request"; - "method" => "RecentBeaconBlocks", - "count" => request.block_roots.len(), - "peer" => format!("{:?}", peer_id) - ); - self.network.send_rpc_request( - Some(request_id), - peer_id.clone(), - RPCRequest::RecentBeaconBlocks(request), - ); - } - ImportManagerOutcome::DownvotePeer(peer_id) => { - trace!( - self.log, - "Peer downvoted"; - "peer" => format!("{:?}", peer_id) - ); - // TODO: Implement reputation - self.network - .disconnect(peer_id.clone(), GoodbyeReason::Fault); - } - ImportManagerOutcome::Idle => { - // nothing to do - return; - } - } + // If the node's best-block is already known to us and they are close to our current + // head, treat them as a fully sync'd peer. + self.send_to_sync(SyncMessage::AddPeer(peer_id, remote)); + } else { + // The remote node has an equal or great finalized epoch and we don't know it's head. + // + // Therefore, there are some blocks between the local finalized epoch and the remote + // head that are worth downloading. + debug!( + self.log, "UsefulPeer"; + "peer" => format!("{:?}", peer_id), + "local_finalized_epoch" => local.finalized_epoch, + "remote_latest_finalized_epoch" => remote.finalized_epoch, + ); + self.send_to_sync(SyncMessage::AddPeer(peer_id, remote)); } } @@ -320,20 +240,11 @@ impl SimpleSync { request_id: RequestId, request: RecentBeaconBlocksRequest, ) { - let chain = match self.chain.upgrade() { - Some(chain) => chain, - None => { - info!(self.log, "Sync shutting down"; - "reason" => "Beacon chain dropped"); - return; - } - }; - let blocks: Vec> = request .block_roots .iter() .filter_map(|root| { - if let Ok(Some(block)) = chain.store.get::>(root) { + if let Ok(Some(block)) = self.chain.store.get::>(root) { Some(block) } else { debug!( @@ -370,15 +281,6 @@ impl SimpleSync { request_id: RequestId, req: BeaconBlocksRequest, ) { - let chain = match self.chain.upgrade() { - Some(chain) => chain, - None => { - info!(self.log, "Sync shutting down"; - "reason" => "Beacon chain dropped"); - return; - } - }; - debug!( self.log, "BeaconBlocksRequest"; @@ -392,14 +294,15 @@ impl SimpleSync { // In the current implementation we read from the db then filter out out-of-range blocks. // Improving the db schema to prevent this would be ideal. - let mut blocks: Vec> = chain + let mut blocks: Vec> = self + .chain .rev_iter_block_roots() .filter(|(_root, slot)| { req.start_slot <= slot.as_u64() && req.start_slot + req.count > slot.as_u64() }) .take_while(|(_root, slot)| req.start_slot <= slot.as_u64()) .filter_map(|(root, _slot)| { - if let Ok(Some(block)) = chain.store.get::>(&root) { + if let Ok(Some(block)) = self.chain.store.get::>(&root) { Some(block) } else { warn!( @@ -423,7 +326,7 @@ impl SimpleSync { "peer" => format!("{:?}", peer_id), "msg" => "Failed to return all requested hashes", "start_slot" => req.start_slot, - "current_slot" => chain.slot().unwrap_or_else(|_| Slot::from(0_u64)).as_u64(), + "current_slot" => self.chain.slot().unwrap_or_else(|_| Slot::from(0_u64)).as_u64(), "requested" => req.count, "returned" => blocks.len(), ); @@ -449,10 +352,11 @@ impl SimpleSync { "count" => beacon_blocks.len(), ); - self.manager - .beacon_blocks_response(peer_id, request_id, beacon_blocks); - - self.process_sync(); + self.send_to_sync(SyncMessage::RecentBeaconBlocksResponse { + peer_id, + request_id, + beacon_blocks, + }); } /// Handle a `RecentBeaconBlocks` response from the peer. @@ -469,10 +373,11 @@ impl SimpleSync { "count" => beacon_blocks.len(), ); - self.manager - .recent_blocks_response(peer_id, request_id, beacon_blocks); - - self.process_sync(); + self.send_to_sync(SyncMessage::BeaconBlocksResponse { + peer_id, + request_id, + beacon_blocks, + }); } /// Process a gossip message declaring a new block. @@ -481,16 +386,7 @@ impl SimpleSync { /// /// Returns a `bool` which, if `true`, indicates we should forward the block to our peers. pub fn on_block_gossip(&mut self, peer_id: PeerId, block: BeaconBlock) -> bool { - let chain = match self.chain.upgrade() { - Some(chain) => chain, - None => { - info!(self.log, "Sync shutting down"; - "reason" => "Beacon chain dropped"); - return false; - } - }; - - if let Ok(outcome) = chain.process_block(block.clone()) { + if let Ok(outcome) = self.chain.process_block(block.clone()) { match outcome { BlockProcessingOutcome::Processed { .. } => { trace!(self.log, "Gossipsub block processed"; @@ -501,7 +397,7 @@ impl SimpleSync { // Inform the sync manager to find parents for this block trace!(self.log, "Block with unknown parent received"; "peer_id" => format!("{:?}",peer_id)); - self.manager.add_unknown_block(block.clone(), peer_id); + self.send_to_sync(SyncMessage::UnknownBlock(peer_id, block.clone())); SHOULD_FORWARD_GOSSIP_BLOCK } BlockProcessingOutcome::FutureSlot { @@ -523,16 +419,7 @@ impl SimpleSync { /// /// Not currently implemented. pub fn on_attestation_gossip(&mut self, _peer_id: PeerId, msg: Attestation) { - let chain = match self.chain.upgrade() { - Some(chain) => chain, - None => { - info!(self.log, "Sync shutting down"; - "reason" => "Beacon chain dropped"); - return; - } - }; - - match chain.process_attestation(msg) { + match self.chain.process_attestation(msg) { Ok(outcome) => info!( self.log, "Processed attestation"; @@ -547,7 +434,7 @@ impl SimpleSync { } /// Build a `HelloMessage` representing the state of the given `beacon_chain`. -fn hello_message(beacon_chain: &BeaconChain) -> HelloMessage { +pub(crate) fn hello_message(beacon_chain: &BeaconChain) -> HelloMessage { let state = &beacon_chain.head().beacon_state; HelloMessage { From 04b47a357b997e8da7bf37c4830c2a48090e0720 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Sat, 7 Sep 2019 09:31:05 +1000 Subject: [PATCH 8/9] Correct bugs in new sync threading --- beacon_node/network/src/message_handler.rs | 7 ++++--- beacon_node/network/src/service.rs | 9 ++------- beacon_node/network/src/sync/manager.rs | 20 +++++++------------- beacon_node/network/src/sync/simple_sync.rs | 6 +++--- tests/ef_tests/eth2.0-spec-tests | 2 +- 5 files changed, 17 insertions(+), 27 deletions(-) diff --git a/beacon_node/network/src/message_handler.rs b/beacon_node/network/src/message_handler.rs index be8fa21f8..782d2129e 100644 --- a/beacon_node/network/src/message_handler.rs +++ b/beacon_node/network/src/message_handler.rs @@ -9,7 +9,7 @@ use eth2_libp2p::{ }; use futures::future::Future; use futures::stream::Stream; -use slog::{debug, trace, warn}; +use slog::{debug, o, trace, warn}; use ssz::{Decode, DecodeError}; use std::sync::Arc; use tokio::sync::mpsc; @@ -51,7 +51,8 @@ impl MessageHandler { executor: &tokio::runtime::TaskExecutor, log: slog::Logger, ) -> error::Result> { - trace!(log, "Service starting"); + let message_handler_log = log.new(o!("Service"=> "Message Handler")); + trace!(message_handler_log, "Service starting"); let (handler_send, handler_recv) = mpsc::unbounded_channel(); @@ -63,7 +64,7 @@ impl MessageHandler { let mut handler = MessageHandler { network_send, message_processor, - log: log.clone(), + log: message_handler_log, }; // spawn handler task and move the message handler instance into the spawned thread diff --git a/beacon_node/network/src/service.rs b/beacon_node/network/src/service.rs index f54630615..1357b5495 100644 --- a/beacon_node/network/src/service.rs +++ b/beacon_node/network/src/service.rs @@ -34,13 +34,8 @@ impl Service { // build the network channel let (network_send, network_recv) = mpsc::unbounded_channel::(); // launch message handler thread - let message_handler_log = log.new(o!("Service" => "MessageHandler")); - let message_handler_send = MessageHandler::spawn( - beacon_chain, - network_send.clone(), - executor, - message_handler_log, - )?; + let message_handler_send = + MessageHandler::spawn(beacon_chain, network_send.clone(), executor, log.clone())?; let network_log = log.new(o!("Service" => "Network")); // launch libp2p service diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 12bef95fa..171d0fdf0 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -251,7 +251,7 @@ pub fn spawn( // create an instance of the SyncManager let sync_manager = SyncManager { chain: beacon_chain, - state: ManagerState::Regular, + state: ManagerState::Stalled, input_channel: sync_recv, network, import_queue: HashMap::new(), @@ -510,7 +510,7 @@ impl SyncManager { &mut self, peer_id: PeerId, request_id: RequestId, - blocks: Vec>, + mut blocks: Vec>, ) { // find the request let parent_request = match self @@ -545,6 +545,11 @@ impl SyncManager { return; } + // add the block to response + parent_request + .downloaded_blocks + .push(blocks.pop().expect("must exist")); + // queue for processing parent_request.state = BlockRequestsState::ReadyToProcess; } @@ -594,7 +599,6 @@ impl SyncManager { "peer" => format!("{:?}", peer_id), ); self.full_peers.insert(peer_id); - self.update_state(); } /* Processing State Functions */ @@ -1077,7 +1081,6 @@ impl Future for SyncManager { Ok(Async::Ready(Some(message))) => match message { SyncMessage::AddPeer(peer_id, info) => { self.add_peer(peer_id, info); - dbg!("add peer"); } SyncMessage::BeaconBlocksResponse { peer_id, @@ -1118,17 +1121,13 @@ impl Future for SyncManager { //need to be called. let mut re_run = false; - dbg!(self.import_queue.len()); // only process batch requests if there are any if !self.import_queue.is_empty() { // process potential block requests self.process_potential_block_requests(); - dbg!(self.import_queue.len()); // process any complete long-range batches re_run = re_run || self.process_complete_batches(); - dbg!(self.import_queue.len()); - dbg!(&self.state); } // only process parent objects if we are in regular sync @@ -1140,9 +1139,6 @@ impl Future for SyncManager { re_run = re_run || self.process_complete_parent_requests(); } - dbg!(self.import_queue.len()); - dbg!(&self.state); - // Shutdown the thread if the chain has termined if let None = self.chain.upgrade() { return Ok(Async::Ready(())); @@ -1152,8 +1148,6 @@ impl Future for SyncManager { break; } } - dbg!(self.import_queue.len()); - dbg!(&self.state); // update the state of the manager self.update_state(); diff --git a/beacon_node/network/src/sync/simple_sync.rs b/beacon_node/network/src/sync/simple_sync.rs index d8b5f2dbf..c54c481c7 100644 --- a/beacon_node/network/src/sync/simple_sync.rs +++ b/beacon_node/network/src/sync/simple_sync.rs @@ -352,7 +352,7 @@ impl MessageProcessor { "count" => beacon_blocks.len(), ); - self.send_to_sync(SyncMessage::RecentBeaconBlocksResponse { + self.send_to_sync(SyncMessage::BeaconBlocksResponse { peer_id, request_id, beacon_blocks, @@ -368,12 +368,12 @@ impl MessageProcessor { ) { debug!( self.log, - "BeaconBlocksResponse"; + "RecentBeaconBlocksResponse"; "peer" => format!("{:?}", peer_id), "count" => beacon_blocks.len(), ); - self.send_to_sync(SyncMessage::BeaconBlocksResponse { + self.send_to_sync(SyncMessage::RecentBeaconBlocksResponse { peer_id, request_id, beacon_blocks, diff --git a/tests/ef_tests/eth2.0-spec-tests b/tests/ef_tests/eth2.0-spec-tests index aaa1673f5..ae6dd9011 160000 --- a/tests/ef_tests/eth2.0-spec-tests +++ b/tests/ef_tests/eth2.0-spec-tests @@ -1 +1 @@ -Subproject commit aaa1673f508103e11304833e0456e4149f880065 +Subproject commit ae6dd9011df05fab8c7e651c09cf9c940973bf81 From 69442a2ab302ed04ecbbc86009f2179803a6dd00 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Sun, 8 Sep 2019 01:57:56 +1000 Subject: [PATCH 9/9] Correct warnings --- beacon_node/rest_api/src/helpers.rs | 4 +--- beacon_node/rest_api/src/url_query.rs | 2 +- beacon_node/rest_api/src/validator.rs | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/beacon_node/rest_api/src/helpers.rs b/beacon_node/rest_api/src/helpers.rs index 6b0211662..a21f1831e 100644 --- a/beacon_node/rest_api/src/helpers.rs +++ b/beacon_node/rest_api/src/helpers.rs @@ -2,9 +2,7 @@ use crate::{ApiError, ApiResult}; use beacon_chain::{BeaconChain, BeaconChainTypes}; use bls::PublicKey; use hex; -use hyper::{Body, Request, StatusCode}; -use serde::de::value::StringDeserializer; -use serde_json::Deserializer; +use hyper::{Body, Request}; use store::{iter::AncestorIter, Store}; use types::{BeaconState, EthSpec, Hash256, RelativeEpoch, Slot}; diff --git a/beacon_node/rest_api/src/url_query.rs b/beacon_node/rest_api/src/url_query.rs index e39a9a449..3802ff831 100644 --- a/beacon_node/rest_api/src/url_query.rs +++ b/beacon_node/rest_api/src/url_query.rs @@ -64,7 +64,7 @@ impl<'a> UrlQuery<'a> { /// Returns a vector of all values present where `key` is in `keys /// /// If no match is found, an `InvalidQueryParams` error is returned. - pub fn all_of(mut self, key: &str) -> Result, ApiError> { + pub fn all_of(self, key: &str) -> Result, ApiError> { let queries: Vec<_> = self .0 .filter_map(|(k, v)| { diff --git a/beacon_node/rest_api/src/validator.rs b/beacon_node/rest_api/src/validator.rs index 365b7e552..0440a7368 100644 --- a/beacon_node/rest_api/src/validator.rs +++ b/beacon_node/rest_api/src/validator.rs @@ -5,9 +5,8 @@ use bls::PublicKey; use hyper::{Body, Request}; use serde::{Deserialize, Serialize}; use std::sync::Arc; -use store::Store; use types::beacon_state::EthSpec; -use types::{BeaconBlock, BeaconState, Epoch, RelativeEpoch, Shard, Slot}; +use types::{Epoch, RelativeEpoch, Shard, Slot}; #[derive(Debug, Serialize, Deserialize)] pub struct ValidatorDuty { @@ -61,7 +60,7 @@ pub fn get_validator_duties(req: Request) - )) })?; //TODO: Handle an array of validators, currently only takes one - let mut validators: Vec = match query.all_of("validator_pubkeys") { + let validators: Vec = match query.all_of("validator_pubkeys") { Ok(v) => v .iter() .map(|pk| parse_pubkey(pk))