From 324299c43894d30acc549d98564ddbeb093888a8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 27 Nov 2019 15:01:35 +0100 Subject: [PATCH 01/21] Extract gossiping system from network --- Cargo.lock | 32 ++ Cargo.toml | 1 + client/finality-grandpa/Cargo.toml | 1 + .../src/communication/gossip.rs | 13 +- .../finality-grandpa/src/communication/mod.rs | 220 ++----------- .../src/communication/periodic.rs | 6 +- .../src/communication/tests.rs | 37 ++- client/finality-grandpa/src/environment.rs | 20 +- client/finality-grandpa/src/lib.rs | 35 +-- client/finality-grandpa/src/observer.rs | 17 +- client/finality-grandpa/src/tests.rs | 7 +- client/network-gossip/Cargo.toml | 19 ++ client/network-gossip/src/bridge.rs | 288 ++++++++++++++++++ client/network-gossip/src/lib.rs | 99 ++++++ .../src/state_machine.rs} | 12 +- client/network/src/behaviour.rs | 47 ++- client/network/src/lib.rs | 2 +- client/network/src/protocol.rs | 163 ++++++---- client/network/src/protocol/event.rs | 32 ++ client/network/src/service.rs | 146 +++++---- 20 files changed, 807 insertions(+), 390 deletions(-) create mode 100644 client/network-gossip/Cargo.toml create mode 100644 client/network-gossip/src/bridge.rs create mode 100644 client/network-gossip/src/lib.rs rename client/{network/src/protocol/consensus_gossip.rs => network-gossip/src/state_machine.rs} (99%) diff --git a/Cargo.lock b/Cargo.lock index 95f5217e33f5f..aa9f3ed176c7b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1749,6 +1749,11 @@ dependencies = [ "scopeguard 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "hashbrown" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "hashbrown" version = "0.6.3" @@ -2745,6 +2750,14 @@ dependencies = [ "cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "lru" +version = "0.1.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "hashbrown 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "lru" version = "0.4.0" @@ -6015,6 +6028,7 @@ dependencies = [ "substrate-keyring 2.0.0", "substrate-keystore 2.0.0", "substrate-network 2.0.0", + "substrate-network-gossip 2.0.0", "substrate-primitives 2.0.0", "substrate-state-machine 2.0.0", "substrate-telemetry 2.0.0", @@ -6163,6 +6177,22 @@ dependencies = [ "zeroize 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "substrate-network-gossip" +version = "2.0.0" +dependencies = [ + "async-std 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "futures 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)", + "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", + "futures-timer 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", + "libp2p 0.13.0 (registry+https://github.com/rust-lang/crates.io-index)", + "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", + "lru 0.1.17 (registry+https://github.com/rust-lang/crates.io-index)", + "parking_lot 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "sr-primitives 2.0.0", + "substrate-network 2.0.0", +] + [[package]] name = "substrate-offchain" version = "2.0.0" @@ -8096,6 +8126,7 @@ dependencies = [ "checksum hash-db 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)" = "d23bd4e7b5eda0d0f3a307e8b381fdc8ba9000f26fbe912250c0a4cc3956364a" "checksum hash256-std-hasher 0.15.2 (registry+https://github.com/rust-lang/crates.io-index)" = "92c171d55b98633f4ed3860808f004099b36c1cc29c42cfc53aa8591b21efcf2" "checksum hashbrown 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "3bae29b6653b3412c2e71e9d486db9f9df5d701941d86683005efb9f2d28e3da" +"checksum hashbrown 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "e1de41fb8dba9714efd92241565cdff73f78508c95697dd56787d3cba27e2353" "checksum hashbrown 0.6.3 (registry+https://github.com/rust-lang/crates.io-index)" = "8e6073d0ca812575946eb5f35ff68dbe519907b25c42530389ff946dc84c6ead" "checksum heapsize 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "1679e6ea370dee694f91f1dc469bf94cf8f52051d147aec3e1f9497c6fc22461" "checksum heck 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "20564e78d53d2bb135c343b3f47714a56af2061f1c928fdb541dc7b9fdd94205" @@ -8180,6 +8211,7 @@ dependencies = [ "checksum lock_api 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "f8912e782533a93a167888781b836336a6ca5da6175c05944c86cf28c31104dc" "checksum log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "e19e8d5c34a3e0e2223db8e060f9e8264aeeb5c5fc64a4ee9965c062211c024b" "checksum log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "14b6052be84e6b71ab17edffc2eeabf5c2c3ae1fdb464aae35ac50c67a44e1f7" +"checksum lru 0.1.17 (registry+https://github.com/rust-lang/crates.io-index)" = "5d8f669d42c72d18514dfca8115689c5f6370a17d980cb5bd777a67f404594c8" "checksum lru 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "26b0dca4ac5b5083c5169ab12205e6473df1c7659940e4978b94f363c6b54b22" "checksum mach 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)" = "86dd2487cdfea56def77b88438a2c915fb45113c5319bfe7e14306ca4cd0b0e1" "checksum malloc_size_of_derive 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "e37c5d4cd9473c5f4c9c111f033f15d4df9bd378fdf615944e360a4f55a05f0b" diff --git a/Cargo.toml b/Cargo.toml index 9b7ad19f8321b..25d602ca9bdae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ members = [ "client/tracing", "client/keystore", "client/network", + "client/network-gossip", "client/offchain", "client/peerset", "client/rpc-servers", diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index 71460966ad087..6c75b1ab71cb3 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -25,6 +25,7 @@ client = { package = "substrate-client", path = "../" } inherents = { package = "substrate-inherents", path = "../../primitives/inherents" } sp-blockchain = { path = "../../primitives/blockchain" } network = { package = "substrate-network", path = "../network" } +network-gossip = { package = "substrate-network-gossip", path = "../network-gossip" } sp-finality-tracker = { path = "../../primitives/finality-tracker" } fg_primitives = { package = "substrate-finality-grandpa-primitives", path = "../../primitives/finality-grandpa" } grandpa = { package = "finality-grandpa", version = "0.10.1", features = ["derive-codec"] } diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index 3e0f2a7ac4f7e..2004c9d8e9606 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -83,7 +83,7 @@ //! We only send polite messages to peers, use sr_primitives::traits::{NumberFor, Block as BlockT, Zero}; -use network::consensus_gossip::{self as network_gossip, MessageIntent, ValidatorContext}; +use network_gossip::{GossipEngine, MessageIntent, ValidatorContext}; use network::{config::Roles, PeerId}; use codec::{Encode, Decode}; use fg_primitives::AuthorityId; @@ -1455,29 +1455,26 @@ pub(super) struct ReportStream { impl ReportStream { /// Consume the report stream, converting it into a future that /// handles all reports. - pub(super) fn consume(self, net: N) + pub(super) fn consume(self, net: GossipEngine) -> impl Future + Send + 'static where B: BlockT, - N: super::Network + Send + 'static, { ReportingTask { reports: self.reports, net, - _marker: Default::default(), } } } /// A future for reporting peers. #[must_use = "Futures do nothing unless polled"] -struct ReportingTask { +struct ReportingTask { reports: mpsc::UnboundedReceiver, - net: N, - _marker: std::marker::PhantomData, + net: GossipEngine, } -impl> Future for ReportingTask { +impl Future for ReportingTask { type Item = (); type Error = (); diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 247f9efd2df18..bd2b64b60c876 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -30,13 +30,12 @@ use std::sync::Arc; use futures::prelude::*; -use futures::sync::{oneshot, mpsc}; -use futures03::stream::{StreamExt, TryStreamExt}; +use futures::sync::mpsc; +use futures03::{compat::Compat, stream::StreamExt}; use grandpa::Message::{Prevote, Precommit, PrimaryPropose}; use grandpa::{voter, voter_set::VoterSet}; use log::{debug, trace}; -use network::{consensus_gossip as network_gossip, NetworkService}; -use network_gossip::ConsensusMessage; +use network_gossip::{GossipEngine, Network}; use codec::{Encode, Decode}; use primitives::Pair; use sr_primitives::traits::{Block as BlockT, Hash as HashT, Header as HeaderT, NumberFor}; @@ -95,50 +94,6 @@ mod benefit { pub(super) const PER_EQUIVOCATION: i32 = 10; } -/// A handle to the network. This is generally implemented by providing some -/// handle to a gossip service or similar. -/// -/// Intended to be a lightweight handle such as an `Arc`. -pub trait Network: Clone + Send + 'static { - /// A stream of input messages for a topic. - type In: Stream; - - /// Get a stream of messages for a specific gossip topic. - fn messages_for(&self, topic: Block::Hash) -> Self::In; - - /// Register a gossip validator. - fn register_validator(&self, validator: Arc>); - - /// Gossip a message out to all connected peers. - /// - /// Force causes it to be sent to all peers, even if they've seen it already. - /// Only should be used in case of consensus stall. - fn gossip_message(&self, topic: Block::Hash, data: Vec, force: bool); - - /// Register a message with the gossip service, it isn't broadcast right - /// away to any peers, but may be sent to new peers joining or when asked to - /// broadcast the topic. Useful to register previous messages on node - /// startup. - fn register_gossip_message(&self, topic: Block::Hash, data: Vec); - - /// Send a message to a bunch of specific peers, even if they've seen it already. - fn send_message(&self, who: Vec, data: Vec); - - /// Report a peer's cost or benefit after some action. - fn report(&self, who: network::PeerId, cost_benefit: i32); - - /// Inform peers that a block with given hash should be downloaded. - fn announce(&self, block: Block::Hash, associated_data: Vec); - - /// Notifies the sync service to try and sync the given block from the given - /// peers. - /// - /// If the given vector of peers is empty then the underlying implementation - /// should make a best effort to fetch the block from any peers it is - /// connected to (NOTE: this assumption will change in the future #3629). - fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor); -} - /// Create a unique topic for a round and set-id combo. pub(crate) fn round_topic(round: RoundNumber, set_id: SetIdNumber) -> B::Hash { <::Hashing as HashT>::hash(format!("{}-{}", set_id, round).as_bytes()) @@ -149,141 +104,24 @@ pub(crate) fn global_topic(set_id: SetIdNumber) -> B::Hash { <::Hashing as HashT>::hash(format!("{}-GLOBAL", set_id).as_bytes()) } -impl Network for Arc> where - B: BlockT, - S: network::specialization::NetworkSpecialization, - H: network::ExHashT, -{ - type In = NetworkStream< - Box + Send + 'static>, - >; - - fn messages_for(&self, topic: B::Hash) -> Self::In { - // Given that one can only communicate with the Substrate network via the `NetworkService` via message-passing, - // and given that methods on the network consensus gossip are not exposed but only reachable by passing a - // closure into `with_gossip` on the `NetworkService` this function needs to make use of the `NetworkStream` - // construction. - // - // We create a oneshot channel and pass the sender within a closure to the network. At some point in the future - // the network passes the message channel back through the oneshot channel. But the consumer of this function - // expects a stream, not a stream within a oneshot. This complexity is abstracted within `NetworkStream`, - // waiting for the oneshot to resolve and from there on acting like a normal message channel. - let (tx, rx) = oneshot::channel(); - self.with_gossip(move |gossip, _| { - let inner_rx: Box + Send> = Box::new(gossip - .messages_for(GRANDPA_ENGINE_ID, topic) - .map(|x| Ok(x)) - .compat() - ); - let _ = tx.send(inner_rx); - }); - NetworkStream::PollingOneshot(rx) - } - - fn register_validator(&self, validator: Arc>) { - self.with_gossip( - move |gossip, context| gossip.register_validator(context, GRANDPA_ENGINE_ID, validator) - ) - } - - fn gossip_message(&self, topic: B::Hash, data: Vec, force: bool) { - let msg = ConsensusMessage { - engine_id: GRANDPA_ENGINE_ID, - data, - }; - - self.with_gossip( - move |gossip, ctx| gossip.multicast(ctx, topic, msg, force) - ) - } - - fn register_gossip_message(&self, topic: B::Hash, data: Vec) { - let msg = ConsensusMessage { - engine_id: GRANDPA_ENGINE_ID, - data, - }; - - self.with_gossip(move |gossip, _| gossip.register_message(topic, msg)) - } - - fn send_message(&self, who: Vec, data: Vec) { - let msg = ConsensusMessage { - engine_id: GRANDPA_ENGINE_ID, - data, - }; - - self.with_gossip(move |gossip, ctx| for who in &who { - gossip.send_message(ctx, who, msg.clone()) - }) - } - - fn report(&self, who: network::PeerId, cost_benefit: i32) { - self.report_peer(who, cost_benefit) - } - - fn announce(&self, block: B::Hash, associated_data: Vec) { - self.announce_block(block, associated_data) - } - - fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - NetworkService::set_sync_fork_request(self, peers, hash, number) - } -} - -/// A stream used by NetworkBridge in its implementation of Network. Given a oneshot that eventually returns a channel -/// which eventually returns messages, instead of: -/// -/// 1. polling the oneshot until it returns a message channel -/// -/// 2. polling the message channel for messages -/// -/// `NetworkStream` combines the two steps into one, requiring a consumer to only poll `NetworkStream` to retrieve -/// messages directly. -pub enum NetworkStream { - PollingOneshot(oneshot::Receiver), - PollingTopicNotifications(R), -} - -impl Stream for NetworkStream -where - R: Stream, -{ - type Item = R::Item; - type Error = (); - - fn poll(&mut self) -> Poll, Self::Error> { - match self { - NetworkStream::PollingOneshot(oneshot) => { - match oneshot.poll() { - Ok(futures::Async::Ready(mut stream)) => { - let poll_result = stream.poll(); - *self = NetworkStream::PollingTopicNotifications(stream); - poll_result - }, - Ok(futures::Async::NotReady) => Ok(futures::Async::NotReady), - Err(_) => Err(()) - } - }, - NetworkStream::PollingTopicNotifications(stream) => { - stream.poll() - }, - } - } +/// Registers the notifications protocol towards the network. +pub(crate) fn register_dummy_protocol>(network: N) { + network.register_notifications_protocol(GRANDPA_ENGINE_ID); } /// Bridge between the underlying network service, gossiping consensus messages and Grandpa -pub(crate) struct NetworkBridge> { - service: N, +pub(crate) struct NetworkBridge { + service: GossipEngine, validator: Arc>, neighbor_sender: periodic::NeighborPacketSender, } -impl> NetworkBridge { +impl NetworkBridge { /// Create a new NetworkBridge to the given NetworkService. Returns the service /// handle and a future that must be polled to completion to finish startup. /// On creation it will register previous rounds' votes with the gossip /// service taken from the VoterSetState. - pub(crate) fn new( + pub(crate) fn new + Clone + Send + 'static>( service: N, config: crate::Config, set_state: crate::environment::SharedVoterSetState, @@ -299,7 +137,7 @@ impl> NetworkBridge { ); let validator = Arc::new(validator); - service.register_validator(validator.clone()); + let service = GossipEngine::new(service, GRANDPA_ENGINE_ID, validator.clone()); { // register all previous votes with the gossip service so that they're @@ -407,7 +245,8 @@ impl> NetworkBridge { }); let topic = round_topic::(round.0, set_id.0); - let incoming = self.service.messages_for(topic) + let incoming = Compat::new(self.service.messages_for(topic) + .map(|item| Ok::<_, ()>(item))) .filter_map(|notification| { let decoded = GossipMessage::::decode(&mut ¬ification.message[..]); if let Err(ref e) = decoded { @@ -460,7 +299,7 @@ impl> NetworkBridge { .map_err(|()| Error::Network(format!("Failed to receive message on unbounded stream"))); let (tx, out_rx) = mpsc::unbounded(); - let outgoing = OutgoingMessages:: { + let outgoing = OutgoingMessages:: { round: round.0, set_id: set_id.0, network: self.service.clone(), @@ -507,7 +346,7 @@ impl> NetworkBridge { self.neighbor_sender.clone(), ); - let outgoing = CommitsOut::::new( + let outgoing = CommitsOut::::new( self.service.clone(), set_id.0, is_voter, @@ -534,8 +373,8 @@ impl> NetworkBridge { } } -fn incoming_global>( - mut service: N, +fn incoming_global( + mut service: GossipEngine, topic: B::Hash, voters: Arc>, gossip_validator: Arc>, @@ -544,7 +383,7 @@ fn incoming_global>( let process_commit = move | msg: FullCommitMessage, mut notification: network_gossip::TopicNotification, - service: &mut N, + service: &mut GossipEngine, gossip_validator: &Arc>, voters: &VoterSet, | { @@ -606,7 +445,7 @@ fn incoming_global>( let process_catch_up = move | msg: FullCatchUpMessage, mut notification: network_gossip::TopicNotification, - service: &mut N, + service: &mut GossipEngine, gossip_validator: &Arc>, voters: &VoterSet, | { @@ -641,7 +480,8 @@ fn incoming_global>( Some(voter::CommunicationIn::CatchUp(msg.message, cb)) }; - service.messages_for(topic) + Compat::new(service.messages_for(topic) + .map(|m| Ok::<_, ()>(m))) .filter_map(|notification| { // this could be optimized by decoding piecewise. let decoded = GossipMessage::::decode(&mut ¬ification.message[..]); @@ -665,7 +505,7 @@ fn incoming_global>( .map_err(|()| Error::Network(format!("Failed to receive message on unbounded stream"))) } -impl> Clone for NetworkBridge { +impl Clone for NetworkBridge { fn clone(&self) -> Self { NetworkBridge { service: self.service.clone(), @@ -712,16 +552,16 @@ pub(crate) fn check_message_sig( /// use the same raw message and key to sign. This is currently true for /// `ed25519` and `BLS` signatures (which we might use in the future), care must /// be taken when switching to different key types. -struct OutgoingMessages> { +struct OutgoingMessages { round: RoundNumber, set_id: SetIdNumber, locals: Option<(AuthorityPair, AuthorityId)>, sender: mpsc::UnboundedSender>, - network: N, + network: GossipEngine, has_voted: HasVoted, } -impl> Sink for OutgoingMessages +impl Sink for OutgoingMessages { type SinkItem = Message; type SinkError = Error; @@ -965,18 +805,18 @@ fn check_catch_up( } /// An output sink for commit messages. -struct CommitsOut> { - network: N, +struct CommitsOut { + network: GossipEngine, set_id: SetId, is_voter: bool, gossip_validator: Arc>, neighbor_sender: periodic::NeighborPacketSender, } -impl> CommitsOut { +impl CommitsOut { /// Create a new commit output stream. pub(crate) fn new( - network: N, + network: GossipEngine, set_id: SetIdNumber, is_voter: bool, gossip_validator: Arc>, @@ -992,7 +832,7 @@ impl> CommitsOut { } } -impl> Sink for CommitsOut { +impl Sink for CommitsOut { type SinkItem = (RoundNumber, Commit); type SinkError = Error; diff --git a/client/finality-grandpa/src/communication/periodic.rs b/client/finality-grandpa/src/communication/periodic.rs index 9dd662ce43461..d080701a390b5 100644 --- a/client/finality-grandpa/src/communication/periodic.rs +++ b/client/finality-grandpa/src/communication/periodic.rs @@ -25,8 +25,9 @@ use log::{debug, warn}; use tokio_timer::Delay; use network::PeerId; +use network_gossip::GossipEngine; use sr_primitives::traits::{NumberFor, Block as BlockT}; -use super::{gossip::{NeighborPacket, GossipMessage}, Network}; +use super::gossip::{NeighborPacket, GossipMessage}; // how often to rebroadcast, if no other const REBROADCAST_AFTER: Duration = Duration::from_secs(2 * 60); @@ -58,12 +59,11 @@ impl NeighborPacketSender { /// /// It may rebroadcast the last neighbor packet periodically when no /// progress is made. -pub(super) fn neighbor_packet_worker(net: N) -> ( +pub(super) fn neighbor_packet_worker(net: GossipEngine) -> ( impl Future + Send + 'static, NeighborPacketSender, ) where B: BlockT, - N: Network, { let mut last = None; let (tx, mut rx) = mpsc::unbounded::<(Vec, NeighborPacket>)>(); diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index e8b399aef39b6..cd032d289bfb3 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -18,14 +18,13 @@ use futures::sync::mpsc; use futures::prelude::*; -use network::consensus_gossip as network_gossip; -use network::test::{Block, Hash}; +use network::{PeerId, test::{Block, Hash}}; use network_gossip::Validator; use tokio::runtime::current_thread; use std::sync::Arc; use keyring::Ed25519Keyring; use codec::Encode; -use sr_primitives::traits::NumberFor; +use sr_primitives::{ConsensusEngineId, traits::NumberFor}; use crate::environment::SharedVoterSetState; use fg_primitives::AuthorityList; @@ -46,10 +45,25 @@ struct TestNetwork { sender: mpsc::UnboundedSender, } -impl super::Network for TestNetwork { - type In = mpsc::UnboundedReceiver; +impl network_gossip::Network for TestNetwork { + fn event_stream(&self) -> Box + Send> { + let (tx, rx) = mpsc::unbounded(); + Box::new(rx) + } + + fn report_peer(&self, who: network::PeerId, cost_benefit: i32) { + let _ = self.sender.unbounded_send(Event::Report(who, cost_benefit)); + } + + fn disconnect_peer(&self, _: PeerId) {} + + fn write_notification(&self, who: PeerId, engine_id: ConsensusEngineId, message: Vec) { - /// Get a stream of messages for a specific gossip topic. + } + + fn register_notifications_protocol(&self, _: ConsensusEngineId) {} + + /*/// Get a stream of messages for a specific gossip topic. fn messages_for(&self, topic: Hash) -> Self::In { let (tx, rx) = mpsc::unbounded(); let _ = self.sender.unbounded_send(Event::MessagesFor(topic, tx)); @@ -82,19 +96,12 @@ impl super::Network for TestNetwork { fn register_gossip_message(&self, _topic: Hash, _data: Vec) { // NOTE: only required to restore previous state on startup // not required for tests currently - } - - /// Report a peer's cost or benefit after some action. - fn report(&self, who: network::PeerId, cost_benefit: i32) { - let _ = self.sender.unbounded_send(Event::Report(who, cost_benefit)); - } + }*/ - /// Inform peers that a block with given hash should be downloaded. fn announce(&self, block: Hash, _associated_data: Vec) { let _ = self.sender.unbounded_send(Event::Announce(block)); } - /// Notify the sync service to try syncing the given chain. fn set_sync_fork_request(&self, _peers: Vec, _hash: Hash, _number: NumberFor) {} } @@ -111,7 +118,7 @@ impl network_gossip::ValidatorContext for TestNetwork { } struct Tester { - net_handle: super::NetworkBridge, + net_handle: super::NetworkBridge, gossip_validator: Arc>, events: mpsc::UnboundedReceiver, } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index af6e03743a42d..855fe682fad44 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -48,7 +48,7 @@ use sr_primitives::traits::{ use substrate_telemetry::{telemetry, CONSENSUS_INFO}; use crate::{ - CommandOrError, Commit, Config, Error, Network, Precommit, Prevote, + CommandOrError, Commit, Config, Error, Precommit, Prevote, PrimaryPropose, SignedMessage, NewAuthoritySet, VoterCommand, }; @@ -375,20 +375,20 @@ impl SharedVoterSetState { } /// The environment we run GRANDPA in. -pub(crate) struct Environment, RA, SC, VR> { +pub(crate) struct Environment { pub(crate) client: Arc>, pub(crate) select_chain: SC, pub(crate) voters: Arc>, pub(crate) config: Config, pub(crate) authority_set: SharedAuthoritySet>, pub(crate) consensus_changes: SharedConsensusChanges>, - pub(crate) network: crate::communication::NetworkBridge, + pub(crate) network: crate::communication::NetworkBridge, pub(crate) set_id: SetId, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, } -impl, RA, SC, VR> Environment { +impl Environment { /// Updates the voter set state using the given closure. The write lock is /// held during evaluation of the closure and the environment's voter set /// state is set to its result if successful. @@ -404,15 +404,13 @@ impl, RA, SC, VR> Environment, B, E, N, RA, SC, VR> +impl, B, E, RA, SC, VR> grandpa::Chain> -for Environment +for Environment where Block: 'static, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + 'static, - N::In: 'static, SC: SelectChain + 'static, VR: VotingRule>, RA: Send + Sync, @@ -555,15 +553,13 @@ pub(crate) fn ancestry, E, RA>( Ok(tree_route.retracted().iter().skip(1).map(|e| e.hash).collect()) } -impl, N, RA, SC, VR> +impl, RA, SC, VR> voter::Environment> -for Environment +for Environment where Block: 'static, B: Backend + 'static, E: CallExecutor + 'static + Send + Sync, - N: Network + 'static + Send, - N::In: 'static + Send, RA: 'static + Send + Sync, SC: SelectChain + 'static, VR: VotingRule>, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 2b83488d5990b..86d239990b582 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -90,7 +90,7 @@ mod observer; mod until_imported; mod voting_rule; -pub use communication::Network; +pub use network_gossip::Network; pub use finality_proof::FinalityProofProvider; pub use justification::GrandpaJustification; pub use light_import::light_block_import; @@ -276,9 +276,8 @@ pub(crate) trait BlockSyncRequester { fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor); } -impl BlockSyncRequester for NetworkBridge where +impl BlockSyncRequester for NetworkBridge where Block: BlockT, - N: communication::Network, { fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor) { NetworkBridge::set_sync_fork_request(self, peers, hash, number) @@ -447,11 +446,11 @@ where )) } -fn global_communication, B, E, N, RA>( +fn global_communication, B, E, RA>( set_id: SetId, voters: &Arc>, client: &Arc>, - network: &NetworkBridge, + network: &NetworkBridge, keystore: &Option, ) -> ( impl Stream< @@ -465,7 +464,6 @@ fn global_communication, B, E, N, RA>( ) where B: Backend, E: CallExecutor + Send + Sync, - N: Network, RA: Send + Sync, NumberFor: BlockNumberOps, { @@ -548,8 +546,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X>( Block::Hash: Ord, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Sync + 'static, - N::In: Send + 'static, + N: Network + Send + Clone + 'static, SC: SelectChain + 'static, VR: VotingRule> + Clone + 'static, NumberFor: BlockNumberOps, @@ -639,17 +636,15 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X>( /// Future that powers the voter. #[must_use] -struct VoterWork, RA, SC, VR> { +struct VoterWork { voter: Box>> + Send>, - env: Arc>, + env: Arc>, voter_commands_rx: mpsc::UnboundedReceiver>>, } -impl VoterWork +impl VoterWork where Block: BlockT, - N: Network + Sync, - N::In: Send + 'static, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -660,7 +655,7 @@ where fn new( client: Arc>, config: Config, - network: NetworkBridge, + network: NetworkBridge, select_chain: SC, voting_rule: VR, persistent_data: PersistentData, @@ -821,11 +816,9 @@ where } } -impl Future for VoterWork +impl Future for VoterWork where Block: BlockT, - N: Network + Sync, - N::In: Send + 'static, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -882,8 +875,7 @@ pub fn run_grandpa, N, RA, SC, VR, X>( Block::Hash: Ord, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Sync + 'static, - N::In: Send + 'static, + N: Network + Send + Clone + 'static, SC: SelectChain + 'static, NumberFor: BlockNumberOps, DigestFor: Encode, @@ -908,15 +900,14 @@ pub fn setup_disabled_grandpa, RA, N>( B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, RA: Send + Sync + 'static, - N: Network + Send + Sync + 'static, - N::In: Send + 'static, + N: Network + Send + Clone + 'static, { register_finality_tracker_inherent_data_provider( client, inherent_data_providers, )?; - network.register_validator(Arc::new(network::consensus_gossip::DiscardAll)); + communication::register_dummy_protocol(network); Ok(()) } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 83c2fac275ba6..19f8b0243332c 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -159,8 +159,7 @@ pub fn run_grandpa_observer, N, RA, SC>( ) -> ::sp_blockchain::Result + Send + 'static> where B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Sync + 'static, - N::In: Send + 'static, + N: Network + Send + Clone + 'static, SC: SelectChain + 'static, NumberFor: BlockNumberOps, RA: Send + Sync + 'static, @@ -200,20 +199,18 @@ pub fn run_grandpa_observer, N, RA, SC>( /// Future that powers the observer. #[must_use] -struct ObserverWork, N: Network, E, Backend, RA> { +struct ObserverWork, E, Backend, RA> { observer: Box>> + Send>, client: Arc>, - network: NetworkBridge, + network: NetworkBridge, persistent_data: PersistentData, keystore: Option, voter_commands_rx: mpsc::UnboundedReceiver>>, } -impl ObserverWork +impl ObserverWork where B: BlockT, - N: Network, - N::In: Send + 'static, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -221,7 +218,7 @@ where { fn new( client: Arc>, - network: NetworkBridge, + network: NetworkBridge, persistent_data: PersistentData, keystore: Option, voter_commands_rx: mpsc::UnboundedReceiver>>, @@ -325,11 +322,9 @@ where } } -impl Future for ObserverWork +impl Future for ObserverWork where B: BlockT, - N: Network, - N::In: Send + 'static, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 2ea15d90228de..1226176456458 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -499,6 +499,8 @@ fn finalize_3_voters_1_full_observer() { let mut keystore_paths = Vec::new(); + let mut voters = Vec::new(); + for (peer_id, local_key) in all_peers.enumerate() { let (client, net_service, link) = { let net = net.lock(); @@ -540,8 +542,11 @@ fn finalize_3_voters_1_full_observer() { telemetry_on_connect: None, voting_rule: (), }; - let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); + voters.push(run_grandpa_voter(grandpa_params).expect("all in order with client and network")); + } + + for voter in voters { runtime.spawn(voter); } diff --git a/client/network-gossip/Cargo.toml b/client/network-gossip/Cargo.toml new file mode 100644 index 0000000000000..7edceb126805a --- /dev/null +++ b/client/network-gossip/Cargo.toml @@ -0,0 +1,19 @@ +[package] +description = "Gossiping for the Substrate network protocol" +name = "substrate-network-gossip" +version = "2.0.0" +license = "GPL-3.0" +authors = ["Parity Technologies "] +edition = "2018" + +[dependencies] +async-std = "1.0" +log = "0.4.8" +futures01 = { package = "futures", version = "0.1.29" } +futures = { version = "0.3.1", features = ["compat"] } +futures-timer = "0.4.0" +lru = "0.1.2" +libp2p = { version = "0.13.0", default-features = false, features = ["libp2p-websocket"] } +network = { package = "substrate-network", path = "../network" } +parking_lot = "0.9.0" +sr-primitives = { path = "../../primitives/sr-primitives" } diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs new file mode 100644 index 0000000000000..4c09173ac027c --- /dev/null +++ b/client/network-gossip/src/bridge.rs @@ -0,0 +1,288 @@ +// Copyright 2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +use crate::Network; +use crate::state_machine::{ConsensusGossip, Validator, TopicNotification}; + +use network::Context; +use network::message::generic::ConsensusMessage; +use network::{Event, config::Roles}; + +use futures::{prelude::*, channel::mpsc, compat::Compat01As03}; +use libp2p::PeerId; +use parking_lot::Mutex; +use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use std::{sync::Arc, time::Duration}; + +pub struct GossipEngine { + inner: Arc>>, + engine_id: ConsensusEngineId, +} + +struct GossipEngineInner { + state_machine: ConsensusGossip, + context: Box + Send>, + context_ext: Box + Send>, +} + +impl GossipEngine { + /// Create a new instance. + pub fn new + Send + Clone + 'static>( + network: N, + engine_id: ConsensusEngineId, + validator: Arc>, + ) -> Self where B: 'static { + let mut state_machine = ConsensusGossip::new(); + let mut context = Box::new(ContextOverService { + network: network.clone(), + }); + let context_ext = Box::new(ContextOverService { + network: network.clone(), + }); + + // We grab the event stream before registering the notifications protocol, otherwise we + // might miss events. + let event_stream = network.event_stream(); + + network.register_notifications_protocol(engine_id); + state_machine.register_validator(&mut *context, engine_id, validator); + + let inner = Arc::new(Mutex::new(GossipEngineInner { + state_machine, + context, + context_ext, + })); + + let gossip_engine = GossipEngine { + inner: inner.clone(), + engine_id, + }; + + async_std::task::spawn({ + let inner = Arc::downgrade(&inner); + async move { + loop { + async_std::task::sleep(Duration::from_millis(1100)).await; + if let Some(inner) = inner.upgrade() { + let mut inner = inner.lock(); + let inner = &mut *inner; + inner.state_machine.tick(&mut *inner.context); + } else { + break; + } + } + } + }); + + async_std::task::spawn(async move { + let mut stream = Compat01As03::new(event_stream); + while let Some(Ok(event)) = stream.next().await { + match event { + Event::NotifOpened { remote, engine_id: msg_engine_id } => { + if msg_engine_id != engine_id { + continue; + } + let mut inner = inner.lock(); + let inner = &mut *inner; + // TODO: for now we hard-code the roles to FULL; fix that + inner.state_machine.new_peer(&mut *inner.context, remote, Roles::FULL); + } + Event::NotifClosed { remote, engine_id: msg_engine_id } => { + if msg_engine_id != engine_id { + continue; + } + let mut inner = inner.lock(); + let inner = &mut *inner; + inner.state_machine.peer_disconnected(&mut *inner.context, remote); + }, + Event::NotifMessages { remote, messages } => { + let mut inner = inner.lock(); + let inner = &mut *inner; + inner.state_machine.on_incoming( + &mut *inner.context, + remote, + messages.into_iter() + .filter_map(|(engine, data)| if engine == engine_id { + Some(ConsensusMessage { engine_id: engine, data: data.to_vec() }) + } else { None }) + .collect() + ); + }, + Event::Dht(_) => {} + } + } + }); + + gossip_engine + } + + /// Closes all notification streams. + pub fn abort(&self) { + self.inner.lock().state_machine.abort(); + } + + pub fn report(&self, who: PeerId, reputation: i32) { + self.inner.lock().context.report_peer(who, reputation); + } + + /// Registers a message without propagating it to any peers. The message + /// becomes available to new peers or when the service is asked to GossipEngine + /// the message's topic. No validation is performed on the message, if the + /// message is already expired it should be dropped on the next garbage + /// collection. + pub fn register_gossip_message( + &self, + topic: B::Hash, + message: Vec, + ) { + let message = ConsensusMessage { + engine_id: self.engine_id, + data: message, + }; + + self.inner.lock().state_machine.register_message(topic, message); + } + + /// Broadcast all messages with given topic. + pub fn broadcast_topic(&self, topic: B::Hash, force: bool) { + let mut inner = self.inner.lock(); + let inner = &mut *inner; + inner.state_machine.broadcast_topic(&mut *inner.context, topic, force); + } + + /// Get data of valid, incoming messages for a topic (but might have expired meanwhile) + pub fn messages_for(&self, topic: B::Hash) + -> mpsc::UnboundedReceiver + { + self.inner.lock().state_machine.messages_for(self.engine_id, topic) + } + + /// Send all messages with given topic to a peer. + pub fn send_topic( + &self, + who: &PeerId, + topic: B::Hash, + force: bool + ) { + let mut inner = self.inner.lock(); + let inner = &mut *inner; + inner.state_machine.send_topic(&mut *inner.context, who, topic, self.engine_id, force) + } + + /// Multicast a message to all peers. + pub fn gossip_message( + &self, + topic: B::Hash, + message: Vec, + force: bool, + ) { + let message = ConsensusMessage { + engine_id: self.engine_id, + data: message, + }; + + let mut inner = self.inner.lock(); + let inner = &mut *inner; + inner.state_machine.multicast(&mut *inner.context, topic, message, force) + } + + /// Send addressed message to the given peers. The message is not kept or multicast + /// later on. + pub fn send_message(&self, who: Vec, data: Vec) { + let mut inner = self.inner.lock(); + let inner = &mut *inner; + + for who in &who { + inner.state_machine.send_message(&mut *inner.context, who, ConsensusMessage { + engine_id: self.engine_id, + data: data.clone(), + }); + } + } + + /// Notify everyone we're connected to that we have the given block. + /// + /// Note: this method isn't strictly related to gossiping and should eventually be moved + /// somewhere else. + pub fn announce(&self, block: B::Hash, associated_data: Vec) { + self.inner.lock().context_ext.announce(block, associated_data); + } + + /// Notifies the sync service to try and sync the given block from the given + /// peers. + /// + /// If the given vector of peers is empty then the underlying implementation + /// should make a best effort to fetch the block from any peers it is + /// connected to (NOTE: this assumption will change in the future #3629). + /// + /// Note: this method isn't strictly related to gossiping and should eventually be moved + /// somewhere else. + pub fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { + self.inner.lock().context_ext.set_sync_fork_request(peers, hash, number); + } +} + +impl Clone for GossipEngine { + fn clone(&self) -> Self { + GossipEngine { + inner: self.inner.clone(), + engine_id: self.engine_id.clone(), + } + } +} + +struct ContextOverService { + network: N, +} + +impl> Context for ContextOverService { + fn report_peer(&mut self, who: PeerId, reputation: i32) { + self.network.report_peer(who, reputation); + } + + fn disconnect_peer(&mut self, who: PeerId) { + self.network.disconnect_peer(who) + } + + fn send_consensus(&mut self, who: PeerId, messages: Vec) { + // TODO: send batch + for message in messages { + self.network.write_notification(who.clone(), message.engine_id, message.data); + } + } + + fn send_chain_specific(&mut self, _: PeerId, _: Vec) { + log::error!( + target: "sub-libp2p", + "send_chain_specific has been called in a context where it shouldn't" + ); + } +} + +trait ContextExt { + fn announce(&self, block: B::Hash, associated_data: Vec); + fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor); +} + +impl> ContextExt for ContextOverService { + fn announce(&self, block: B::Hash, associated_data: Vec) { + Network::announce(&self.network, block, associated_data) + } + + fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { + Network::set_sync_fork_request(&self.network, peers, hash, number) + } +} diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs new file mode 100644 index 0000000000000..a2eb1507c113b --- /dev/null +++ b/client/network-gossip/src/lib.rs @@ -0,0 +1,99 @@ +// Copyright 2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +pub use self::bridge::GossipEngine; +// TODO: remove +pub use self::state_machine::*; + +use network::{specialization::NetworkSpecialization, Event, ExHashT, NetworkService, PeerId}; +use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use std::sync::Arc; + +mod bridge; +mod state_machine; + +/// Abstraction over a network. +pub trait Network { + /// Returns a stream of events representing what happens on the network. + fn event_stream(&self) -> Box + Send>; + + /// Adjust the reputation of a node. + fn report_peer(&self, peer_id: PeerId, reputation: i32); + + /// Force-disconnect a peer. + fn disconnect_peer(&self, who: PeerId); + + /// Send a notification to a peer. + fn write_notification(&self, who: PeerId, engine_id: ConsensusEngineId, message: Vec); + + /// Registers a notifications protocol. + /// + /// See the documentation of [`NetworkService:register_notifications_protocol`] for more information. + fn register_notifications_protocol( + &self, + engine_id: ConsensusEngineId + ); + + /// Notify everyone we're connected to that we have the given block. + /// + /// Note: this method isn't strictly related to gossiping and should eventually be moved + /// somewhere else. + fn announce(&self, block: B::Hash, associated_data: Vec); + + /// Notifies the sync service to try and sync the given block from the given + /// peers. + /// + /// If the given vector of peers is empty then the underlying implementation + /// should make a best effort to fetch the block from any peers it is + /// connected to (NOTE: this assumption will change in the future #3629). + /// + /// Note: this method isn't strictly related to gossiping and should eventually be moved + /// somewhere else. + fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor); +} + +impl, H: ExHashT> Network for Arc> { + fn event_stream(&self) -> Box + Send> { + Box::new(NetworkService::event_stream(self)) + } + + fn report_peer(&self, peer_id: PeerId, reputation: i32) { + NetworkService::report_peer(self, peer_id, reputation); + } + + fn disconnect_peer(&self, who: PeerId) { + NetworkService::disconnect_peer(self, who) + } + + fn write_notification(&self, who: PeerId, engine_id: ConsensusEngineId, message: Vec) { + NetworkService::write_notification(self, who, engine_id, message) + } + + fn register_notifications_protocol( + &self, + engine_id: ConsensusEngineId, + ) { + NetworkService::register_notifications_protocol(self, engine_id) + } + + fn announce(&self, block: B::Hash, associated_data: Vec) { + NetworkService::announce_block(self, block, associated_data) + } + + fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { + NetworkService::set_sync_fork_request(self, peers, hash, number) + } +} diff --git a/client/network/src/protocol/consensus_gossip.rs b/client/network-gossip/src/state_machine.rs similarity index 99% rename from client/network/src/protocol/consensus_gossip.rs rename to client/network-gossip/src/state_machine.rs index 8cde3c66feb81..4d3390a049b16 100644 --- a/client/network/src/protocol/consensus_gossip.rs +++ b/client/network-gossip/src/state_machine.rs @@ -48,14 +48,14 @@ use std::sync::Arc; use std::iter; use std::time; use log::{trace, debug}; -use futures03::channel::mpsc; +use futures::channel::mpsc; use lru::LruCache; use libp2p::PeerId; use sr_primitives::traits::{Block as BlockT, Hash, HashFor}; use sr_primitives::ConsensusEngineId; -pub use crate::message::generic::{Message, ConsensusMessage}; -use crate::protocol::Context; -use crate::config::Roles; +pub use network::message::generic::{Message, ConsensusMessage}; +use network::Context; +use network::config::Roles; // FIXME: Add additional spam/DoS attack protection: https://github.com/paritytech/substrate/issues/1115 const KNOWN_MESSAGES_CACHE_SIZE: usize = 4096; @@ -186,7 +186,7 @@ fn propagate<'a, B: BlockT, I>( validators: &HashMap>>, ) // (msg_hash, topic, message) - where I: Clone + IntoIterator, + where I: Clone + IntoIterator, { let mut check_fns = HashMap::new(); let mut message_allowed = move |who: &PeerId, intent: MessageIntent, topic: &B::Hash, message: &ConsensusMessage| { @@ -633,7 +633,7 @@ impl Validator for DiscardAll { mod tests { use std::sync::Arc; use sr_primitives::testing::{H256, Block as RawBlock, ExtrinsicWrapper}; - use futures03::executor::block_on_stream; + use futures::executor::block_on_stream; use super::*; diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index a0299bc340ce2..9e28dadf1e82d 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -16,10 +16,11 @@ use crate::{ debug_info, discovery::DiscoveryBehaviour, discovery::DiscoveryOut, DiscoveryNetBehaviour, - protocol::event::DhtEvent + Event, protocol::event::DhtEvent }; use crate::{ExHashT, specialization::NetworkSpecialization}; use crate::protocol::{CustomMessageOutcome, Protocol}; +use consensus::{BlockOrigin, import_queue::{IncomingBlock, Origin}}; use futures::prelude::*; use libp2p::NetworkBehaviour; use libp2p::core::{Multiaddr, PeerId, PublicKey}; @@ -27,7 +28,7 @@ use libp2p::kad::record; use libp2p::swarm::{NetworkBehaviourAction, NetworkBehaviourEventProcess}; use libp2p::core::{nodes::Substream, muxing::StreamMuxerBox}; use log::{debug, warn}; -use sr_primitives::traits::Block as BlockT; +use sr_primitives::{traits::{Block as BlockT, NumberFor}, Justification}; use std::iter; use void; @@ -50,8 +51,10 @@ pub struct Behaviour, H: ExHashT> { /// Event generated by `Behaviour`. pub enum BehaviourOut { - SubstrateAction(CustomMessageOutcome), - Dht(DhtEvent), + BlockImport(BlockOrigin, Vec>), + JustificationImport(Origin, B::Hash, NumberFor, Justification), + FinalityProofImport(Origin, B::Hash, NumberFor, Vec), + Event(Event), } impl, H: ExHashT> Behaviour { @@ -127,7 +130,33 @@ Behaviour { impl, H: ExHashT> NetworkBehaviourEventProcess> for Behaviour { fn inject_event(&mut self, event: CustomMessageOutcome) { - self.events.push(BehaviourOut::SubstrateAction(event)); + match event { + CustomMessageOutcome::BlockImport(origin, blocks) => + self.events.push(BehaviourOut::BlockImport(origin, blocks)), + CustomMessageOutcome::JustificationImport(origin, hash, nb, justification) => + self.events.push(BehaviourOut::JustificationImport(origin, hash, nb, justification)), + CustomMessageOutcome::FinalityProofImport(origin, hash, nb, proof) => + self.events.push(BehaviourOut::FinalityProofImport(origin, hash, nb, proof)), + CustomMessageOutcome::NotifOpened { remote, protocols } => + for engine_id in protocols { + self.events.push(BehaviourOut::Event(Event::NotifOpened { + remote: remote.clone(), + engine_id, + })); + }, + CustomMessageOutcome::NotifClosed { remote, protocols } => + for engine_id in protocols { + self.events.push(BehaviourOut::Event(Event::NotifClosed { + remote: remote.clone(), + engine_id, + })); + }, + CustomMessageOutcome::NotifMessages { remote, messages } => { + let ev = Event::NotifMessages { remote, messages }; + self.events.push(BehaviourOut::Event(ev)); + }, + CustomMessageOutcome::None => {} + } } } @@ -166,16 +195,16 @@ impl, H: ExHashT> NetworkBehaviourEventPr self.substrate.add_discovered_nodes(iter::once(peer_id)); } DiscoveryOut::ValueFound(results) => { - self.events.push(BehaviourOut::Dht(DhtEvent::ValueFound(results))); + self.events.push(BehaviourOut::Event(Event::Dht(DhtEvent::ValueFound(results)))); } DiscoveryOut::ValueNotFound(key) => { - self.events.push(BehaviourOut::Dht(DhtEvent::ValueNotFound(key))); + self.events.push(BehaviourOut::Event(Event::Dht(DhtEvent::ValueNotFound(key)))); } DiscoveryOut::ValuePut(key) => { - self.events.push(BehaviourOut::Dht(DhtEvent::ValuePut(key))); + self.events.push(BehaviourOut::Event(Event::Dht(DhtEvent::ValuePut(key)))); } DiscoveryOut::ValuePutFailed(key) => { - self.events.push(BehaviourOut::Dht(DhtEvent::ValuePutFailed(key))); + self.events.push(BehaviourOut::Event(Event::Dht(DhtEvent::ValuePutFailed(key)))); } } } diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index 96efd49958763..1b84337f93b7a 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -190,7 +190,7 @@ pub use service::{ NetworkService, NetworkWorker, TransactionPool, ExHashT, ReportHandle, NetworkStateInfo, }; -pub use protocol::{PeerInfo, Context, consensus_gossip, message, specialization}; +pub use protocol::{PeerInfo, Context, message, specialization}; pub use protocol::event::{Event, DhtEvent}; pub use protocol::sync::SyncState; pub use libp2p::{Multiaddr, PeerId}; diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 35e3d2453fcb9..67194a9c0e9d9 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -17,7 +17,7 @@ use crate::{DiscoveryNetBehaviour, config::ProtocolId}; use crate::legacy_proto::{LegacyProto, LegacyProtoOut}; use crate::utils::interval; -use bytes::BytesMut; +use bytes::{Bytes, BytesMut}; use futures::prelude::*; use futures03::{StreamExt as _, TryStreamExt as _}; use libp2p::{Multiaddr, PeerId}; @@ -38,7 +38,6 @@ use sr_primitives::traits::{ use sr_arithmetic::traits::SaturatedConversion; use message::{BlockAnnounce, BlockAttributes, Direction, FromBlock, Message, RequestId}; use message::generic::{Message as GenericMessage, ConsensusMessage}; -use consensus_gossip::{ConsensusGossip, MessageRecipient as GossipMessageRecipient}; use light_dispatch::{LightDispatch, LightDispatchNetwork, RequestData}; use specialization::NetworkSpecialization; use sync::{ChainSync, SyncState}; @@ -56,7 +55,6 @@ use crate::error; use util::LruHashSet; mod util; -pub mod consensus_gossip; pub mod message; pub mod event; pub mod light_dispatch; @@ -118,7 +116,6 @@ pub struct Protocol, H: ExHashT> { genesis_hash: B::Hash, sync: ChainSync, specialization: S, - consensus_gossip: ConsensusGossip, context_data: ContextData, /// List of nodes for which we perform additional logging because they are important for the /// user. @@ -132,6 +129,8 @@ pub struct Protocol, H: ExHashT> { finality_proof_provider: Option>>, /// Handles opening the unique substream and sending and receiving raw messages. behaviour: LegacyProto>, + /// List of notification protocols that have been registered. + registered_notif_protocols: HashSet, } #[derive(Default)] @@ -456,13 +455,13 @@ impl, H: ExHashT> Protocol { genesis_hash: info.chain.genesis_hash, sync, specialization, - consensus_gossip: ConsensusGossip::new(), handshaking_peers: HashMap::new(), important_peers, transaction_pool, finality_proof_provider, peerset_handle: peerset_handle.clone(), behaviour, + registered_notif_protocols: HashSet::new(), }; Ok((protocol, peerset_handle)) @@ -639,20 +638,38 @@ impl, H: ExHashT> Protocol { return self.on_finality_proof_response(who, response), GenericMessage::RemoteReadChildRequest(request) => self.on_remote_read_child_request(who, request), - GenericMessage::Consensus(msg) => { - self.consensus_gossip.on_incoming( - &mut ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle), - who, - vec![msg], - ); - } + GenericMessage::Consensus(msg) => + return if self.registered_notif_protocols.contains(&msg.engine_id) { + CustomMessageOutcome::NotifMessages { + remote: who.clone(), + messages: vec![(msg.engine_id, From::from(msg.data))], + } + } else { + warn!(target: "sync", "Received message on non-registered protocol: {:?}", msg.engine_id); + CustomMessageOutcome::None + }, GenericMessage::ConsensusBatch(messages) => { - self.consensus_gossip.on_incoming( - &mut ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle), - who, - messages, - ); - } + let messages = messages + .into_iter() + .filter_map(|msg| { + if self.registered_notif_protocols.contains(&msg.engine_id) { + Some((msg.engine_id, From::from(msg.data))) + } else { + warn!(target: "sync", "Received message on non-registered protocol: {:?}", msg.engine_id); + None + } + }) + .collect::>(); + + return if !messages.is_empty() { + CustomMessageOutcome::NotifMessages { + remote: who.clone(), + messages, + } + } else { + CustomMessageOutcome::None + }; + }, GenericMessage::ChainSpecific(msg) => self.specialization.on_message( &mut ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle), who, @@ -682,14 +699,6 @@ impl, H: ExHashT> Protocol { ); } - /// Locks `self` and returns a context plus the `ConsensusGossip` struct. - pub fn consensus_gossip_lock<'a>( - &'a mut self, - ) -> (impl Context + 'a, &'a mut ConsensusGossip) { - let context = ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle); - (context, &mut self.consensus_gossip) - } - /// Locks `self` and returns a context plus the network specialization. pub fn specialization_lock<'a>( &'a mut self, @@ -698,26 +707,6 @@ impl, H: ExHashT> Protocol { (context, &mut self.specialization) } - /// Gossip a consensus message to the network. - pub fn gossip_consensus_message( - &mut self, - topic: B::Hash, - engine_id: ConsensusEngineId, - message: Vec, - recipient: GossipMessageRecipient, - ) { - let mut context = ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle); - let message = ConsensusMessage { data: message, engine_id }; - match recipient { - GossipMessageRecipient::BroadcastToAll => - self.consensus_gossip.multicast(&mut context, topic, message, true), - GossipMessageRecipient::BroadcastNew => - self.consensus_gossip.multicast(&mut context, topic, message, false), - GossipMessageRecipient::Peer(who) => - self.send_message(&who, GenericMessage::Consensus(message)), - } - } - /// Called when a new peer is connected pub fn on_peer_connected(&mut self, who: PeerId) { trace!(target: "sync", "Connecting {}", who); @@ -738,11 +727,8 @@ impl, H: ExHashT> Protocol { self.handshaking_peers.remove(&peer); self.context_data.peers.remove(&peer) }; - if let Some(peer_data) = removed { + if let Some(_peer_data) = removed { let mut context = ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle); - if peer_data.info.protocol_version > 2 { - self.consensus_gossip.peer_disconnected(&mut context, peer.clone()); - } self.sync.peer_disconnected(peer.clone()); self.specialization.on_disconnect(&mut context, peer.clone()); self.light_dispatch.on_disconnect(LightDispatchIn { @@ -905,9 +891,6 @@ impl, H: ExHashT> Protocol { /// /// > **Note**: This method normally doesn't have to be called except for testing purposes. pub fn tick(&mut self) { - self.consensus_gossip.tick( - &mut ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle) - ); self.maintain_peers(); self.light_dispatch.maintain_peers(LightDispatchIn { behaviour: &mut self.behaviour, @@ -960,7 +943,7 @@ impl, H: ExHashT> Protocol { /// Called by peer to report status fn on_status_message(&mut self, who: PeerId, status: message::Status) { trace!(target: "sync", "New peer {} {:?}", who, status); - let protocol_version = { + let _protocol_version = { if self.context_data.peers.contains_key(&who) { log!( target: "sync", @@ -1066,12 +1049,56 @@ impl, H: ExHashT> Protocol { } } let mut context = ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle); - if protocol_version > 2 { - self.consensus_gossip.new_peer(&mut context, who.clone(), status.roles); - } self.specialization.on_connect(&mut context, who, status); } + /// Send a notification to the given peer we're connected to. + /// + /// Doesn't do anything if we don't have a notifications substream for that protocol with that + /// peer. + pub fn write_notification( + &mut self, + target: PeerId, + engine_id: ConsensusEngineId, + message: impl Into> + ) { + if !self.registered_notif_protocols.contains(&engine_id) { + error!( + target: "sub-libp2p", + "Sending a notification with a protocol that wasn't registered: {:?}", + engine_id + ); + } + + self.send_message(&target, GenericMessage::Consensus(ConsensusMessage { + engine_id, + data: message.into(), + })); + } + + /// Registers a new notifications protocol. + /// + /// You are very strongly encouraged to call this method very early on. Any connection open + /// will retain the protocols that were registered then, and not any new one. + pub fn register_notifications_protocol( + &mut self, + engine_id: ConsensusEngineId, + ) -> Vec { + if !self.registered_notif_protocols.insert(engine_id) { + error!(target: "sub-libp2p", "Notifications protocol already registered: {:?}", engine_id); + } + + // Registering a protocol while we already have open connections isn't great, but for now + // we handle it by notifying that we opened channels with everyone. + self.behaviour.open_peers() + .map(|peer| + event::Event::NotifOpened { + remote: peer.clone(), + engine_id, + }) + .collect() + } + /// Called when peer sends us new extrinsics fn on_extrinsics( &mut self, @@ -1741,6 +1768,12 @@ pub enum CustomMessageOutcome { BlockImport(BlockOrigin, Vec>), JustificationImport(Origin, B::Hash, NumberFor, Justification), FinalityProofImport(Origin, B::Hash, NumberFor, Vec), + /// Notifications protocols have been opened with a remote. + NotifOpened { remote: PeerId, protocols: Vec }, + /// Notifications protocols have been closed with a remote. + NotifClosed { remote: PeerId, protocols: Vec }, + /// Messages have been received on one or more notifications protocols. + NotifMessages { remote: PeerId, messages: Vec<(ConsensusEngineId, Bytes)> }, None, } @@ -1870,12 +1903,20 @@ Protocol { version <= CURRENT_VERSION as u8 && version >= MIN_VERSION as u8 ); - self.on_peer_connected(peer_id); - CustomMessageOutcome::None + self.on_peer_connected(peer_id.clone()); + // Notify all the notification protocols as open. + CustomMessageOutcome::NotifOpened { + remote: peer_id, + protocols: self.registered_notif_protocols.iter().cloned().collect(), + } } LegacyProtoOut::CustomProtocolClosed { peer_id, .. } => { - self.on_peer_disconnected(peer_id); - CustomMessageOutcome::None + self.on_peer_disconnected(peer_id.clone()); + // Notify all the notification protocols as closed. + CustomMessageOutcome::NotifClosed { + remote: peer_id, + protocols: self.registered_notif_protocols.iter().cloned().collect(), + } }, LegacyProtoOut::CustomMessage { peer_id, message } => self.on_custom_message(peer_id, message), diff --git a/client/network/src/protocol/event.rs b/client/network/src/protocol/event.rs index c8bee5588c704..d2da45cc7370a 100644 --- a/client/network/src/protocol/event.rs +++ b/client/network/src/protocol/event.rs @@ -17,10 +17,14 @@ //! Network event types. These are are not the part of the protocol, but rather //! events that happen on the network like DHT get/put results received. +use bytes::Bytes; +use libp2p::core::PeerId; use libp2p::kad::record::Key; +use sr_primitives::ConsensusEngineId; /// Events generated by DHT as a response to get_value and put_value requests. #[derive(Debug, Clone)] +#[must_use] pub enum DhtEvent { /// The value was found. ValueFound(Vec<(Key, Vec)>), @@ -37,7 +41,35 @@ pub enum DhtEvent { /// Type for events generated by networking layer. #[derive(Debug, Clone)] +#[must_use] pub enum Event { /// Event generated by a DHT. Dht(DhtEvent), + + /// Opened a substream with the given node with the given notifications protocol. + /// + /// The protocol is always one of the notification protocols that have been registered. + NotifOpened { + /// Node we opened the substream with. + remote: PeerId, + /// The concerned protocol. Each protocol uses a different substream. + engine_id: ConsensusEngineId, + }, + + /// Closed a substream with the given node. Always matches a corresponding previous + /// `NotifOpened` message. + NotifClosed { + /// Node we closed the substream with. + remote: PeerId, + /// The concerned protocol. Each protocol uses a different substream. + engine_id: ConsensusEngineId, + }, + + /// Received one or more messages from the given node using the given protocol. + NotifMessages { + /// Node we received the message from. + remote: PeerId, + /// Concerned protocol and associated message. + messages: Vec<(ConsensusEngineId, Bytes)>, + }, } diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 437e978f4bd47..a9b7bdc1b64fb 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -28,6 +28,7 @@ use std::{collections::{HashMap, HashSet}, fs, marker::PhantomData, io, path::Path}; use std::sync::{Arc, atomic::{AtomicBool, AtomicUsize, Ordering}}; +use codec::Encode; use consensus::import_queue::{ImportQueue, Link}; use consensus::import_queue::{BlockImportResult, BlockImportError}; use futures::{prelude::*, sync::mpsc}; @@ -45,8 +46,7 @@ use crate::{NetworkState, NetworkStateNotConnectedPeer, NetworkStatePeer}; use crate::{transport, config::NonReservedPeerMode}; use crate::config::{Params, TransportConfig}; use crate::error::Error; -use crate::protocol::{self, Protocol, Context, CustomMessageOutcome, PeerInfo}; -use crate::protocol::consensus_gossip::{ConsensusGossip, MessageRecipient as GossipMessageRecipient}; +use crate::protocol::{self, Protocol, Context, PeerInfo}; use crate::protocol::{event::Event, light_dispatch::{AlwaysBadChecker, RequestData}}; use crate::protocol::specialization::NetworkSpecialization; use crate::protocol::sync::SyncState; @@ -276,6 +276,7 @@ impl, H: ExHashT> NetworkWorker import_queue: params.import_queue, from_worker, light_client_rqs: params.on_demand.and_then(|od| od.extract_receiver()), + event_streams: Vec::new(), }) } @@ -416,6 +417,55 @@ impl, H: ExHashT> NetworkServic self.local_peer_id.clone() } + /// Writes a message on an open notifications channel. Has no effect if the notifications + /// channel with this protocol name is closed. + /// + /// > **Note**: The reason why this is a no-op in the situation where we have no channel is + /// > that we don't guarantee message delivery anyway. Networking issues can cause + /// > connections to drop at any time, and higher-level logic shouldn't differentiate + /// > between the remote voluntarily closing a substream or a network error + /// > preventing the message from being delivered. + /// + /// The protocol must have been registered with `register_notifications_protocol`. + /// + pub fn write_notification(&self, target: PeerId, engine_id: ConsensusEngineId, message: impl Encode) { + let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::WriteNotification { + target, + engine_id, + message: message.encode(), + }); + } + + /// Returns a stream containing the events that happen on the network. + /// + /// If this method is called multiple times, the events are duplicated. + /// + /// The stream never ends (unless the `NetworkWorker` gets shut down). + pub fn event_stream(&self) -> impl Stream { + // Note: when transitioning to stable futures, remove the `Error` entirely + let (tx, rx) = mpsc::unbounded(); + let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::EventStream(tx)); + rx + } + + /// Registers a new notifications protocol. + /// + /// After that, you can call `write_notifications`. + /// + /// Please call `event_stream` before registering a protocol, otherwise you may miss events + /// about the protocol that you have registered. + /// + /// You are very strongly encouraged to call this method very early on. Any connection open + /// will retain the protocols that were registered then, and not any new one. + pub fn register_notifications_protocol( + &self, + engine_id: ConsensusEngineId, + ) { + let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::RegisterNotifProtocol { + engine_id, + }); + } + /// You must call this when new transactons are imported by the transaction pool. /// /// The latest transactions will be fetched from the `TransactionPool` that was passed at @@ -432,27 +482,19 @@ impl, H: ExHashT> NetworkServic let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::AnnounceBlock(hash, data)); } - /// Send a consensus message through the gossip - pub fn gossip_consensus_message( - &self, - topic: B::Hash, - engine_id: ConsensusEngineId, - message: Vec, - recipient: GossipMessageRecipient, - ) { - let _ = self - .to_worker - .unbounded_send(ServerToWorkerMsg::GossipConsensusMessage( - topic, engine_id, message, recipient, - )); - } - /// Report a given peer as either beneficial (+) or costly (-) according to the /// given scalar. pub fn report_peer(&self, who: PeerId, cost_benefit: i32) { self.peerset.report_peer(who, cost_benefit); } + /// Disconnect from a node as soon as possible. + /// + /// This triggers the same effects as if the connection had closed itself spontaneously. + pub fn disconnect_peer(&self, who: PeerId) { + let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::DisconnectPeer(who)); + } + /// Request a justification for the given block from the network. /// /// On success, the justification will be passed to the import queue that was part at @@ -472,15 +514,6 @@ impl, H: ExHashT> NetworkServic .unbounded_send(ServerToWorkerMsg::ExecuteWithSpec(Box::new(f))); } - /// Execute a closure with the consensus gossip. - pub fn with_gossip(&self, f: F) - where F: FnOnce(&mut ConsensusGossip, &mut dyn Context) + Send + 'static - { - let _ = self - .to_worker - .unbounded_send(ServerToWorkerMsg::ExecuteWithGossip(Box::new(f))); - } - /// Are we in the process of downloading the chain? pub fn is_major_syncing(&self) -> bool { self.is_major_syncing.load(Ordering::Relaxed) @@ -630,12 +663,20 @@ enum ServerToWorkerMsg> { RequestJustification(B::Hash, NumberFor), AnnounceBlock(B::Hash, Vec), ExecuteWithSpec(Box) + Send>), - ExecuteWithGossip(Box, &mut dyn Context) + Send>), - GossipConsensusMessage(B::Hash, ConsensusEngineId, Vec, GossipMessageRecipient), GetValue(record::Key), PutValue(record::Key, Vec), AddKnownAddress(PeerId, Multiaddr), SyncFork(Vec, B::Hash, NumberFor), + EventStream(mpsc::UnboundedSender), + WriteNotification { + message: Vec, + engine_id: ConsensusEngineId, + target: PeerId, + }, + RegisterNotifProtocol { + engine_id: ConsensusEngineId, + }, + DisconnectPeer(PeerId), } /// Main network worker. Must be polled in order for the network to advance. @@ -659,10 +700,12 @@ pub struct NetworkWorker, H: Ex from_worker: mpsc::UnboundedReceiver>, /// Receiver for queries from the light client that must be processed. light_client_rqs: Option>>, + /// Senders for events that happen on the network. + event_streams: Vec>, } impl, H: ExHashT> Stream for NetworkWorker { - type Item = Event; + type Item = (); type Error = io::Error; fn poll(&mut self) -> Poll, Self::Error> { @@ -695,13 +738,6 @@ impl, H: ExHashT> Stream for Ne let (mut context, spec) = protocol.specialization_lock(); task(spec, &mut context); }, - ServerToWorkerMsg::ExecuteWithGossip(task) => { - let protocol = self.network_service.user_protocol_mut(); - let (mut context, gossip) = protocol.consensus_gossip_lock(); - task(gossip, &mut context); - } - ServerToWorkerMsg::GossipConsensusMessage(topic, engine_id, message, recipient) => - self.network_service.user_protocol_mut().gossip_consensus_message(topic, engine_id, message, recipient), ServerToWorkerMsg::AnnounceBlock(hash, data) => self.network_service.user_protocol_mut().announce_block(hash, data), ServerToWorkerMsg::RequestJustification(hash, number) => @@ -716,6 +752,18 @@ impl, H: ExHashT> Stream for Ne self.network_service.add_known_address(peer_id, addr), ServerToWorkerMsg::SyncFork(peer_ids, hash, number) => self.network_service.user_protocol_mut().set_sync_fork_request(peer_ids, &hash, number), + ServerToWorkerMsg::EventStream(sender) => + self.event_streams.push(sender), + ServerToWorkerMsg::WriteNotification { message, engine_id, target } => + self.network_service.user_protocol_mut().write_notification(target, engine_id, message), + ServerToWorkerMsg::RegisterNotifProtocol { engine_id } => { + let events = self.network_service.user_protocol_mut().register_notifications_protocol(engine_id); + for event in events { + self.event_streams.retain(|sender| sender.unbounded_send(event.clone()).is_ok()); + } + }, + ServerToWorkerMsg::DisconnectPeer(who) => + self.network_service.user_protocol_mut().disconnect_peer(&who), } } @@ -723,27 +771,23 @@ impl, H: ExHashT> Stream for Ne // Process the next action coming from the network. let poll_value = self.network_service.poll(); - let outcome = match poll_value { + match poll_value { Ok(Async::NotReady) => break, - Ok(Async::Ready(Some(BehaviourOut::SubstrateAction(outcome)))) => outcome, - Ok(Async::Ready(Some(BehaviourOut::Dht(ev)))) => - return Ok(Async::Ready(Some(Event::Dht(ev)))), - Ok(Async::Ready(None)) => CustomMessageOutcome::None, + Ok(Async::Ready(Some(BehaviourOut::BlockImport(origin, blocks)))) => + self.import_queue.import_blocks(origin, blocks), + Ok(Async::Ready(Some(BehaviourOut::JustificationImport(origin, hash, nb, justification)))) => + self.import_queue.import_justification(origin, hash, nb, justification), + Ok(Async::Ready(Some(BehaviourOut::FinalityProofImport(origin, hash, nb, proof)))) => + self.import_queue.import_finality_proof(origin, hash, nb, proof), + Ok(Async::Ready(Some(BehaviourOut::Event(ev)))) => { + self.event_streams.retain(|sender| sender.unbounded_send(ev.clone()).is_ok()); + }, + Ok(Async::Ready(None)) => {}, Err(err) => { error!(target: "sync", "Error in the network: {:?}", err); return Err(err) } }; - - match outcome { - CustomMessageOutcome::BlockImport(origin, blocks) => - self.import_queue.import_blocks(origin, blocks), - CustomMessageOutcome::JustificationImport(origin, hash, nb, justification) => - self.import_queue.import_justification(origin, hash, nb, justification), - CustomMessageOutcome::FinalityProofImport(origin, hash, nb, proof) => - self.import_queue.import_finality_proof(origin, hash, nb, proof), - CustomMessageOutcome::None => {} - } } // Update the variables shared with the `NetworkService`. From f72f42411cff22b7a0620754e21a5d3252848c3b Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 29 Nov 2019 12:18:04 +0100 Subject: [PATCH 02/21] Finish porting GRANDPA tests --- .../src/communication/tests.rs | 93 +++++++------------ 1 file changed, 33 insertions(+), 60 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index cd032d289bfb3..72a8c70baf998 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -18,7 +18,7 @@ use futures::sync::mpsc; use futures::prelude::*; -use network::{PeerId, test::{Block, Hash}}; +use network::{Event as NetworkEvent, PeerId, test::{Block, Hash}}; use network_gossip::Validator; use tokio::runtime::current_thread; use std::sync::Arc; @@ -32,10 +32,8 @@ use super::gossip::{self, GossipValidator}; use super::{AuthorityId, VoterSet, Round, SetId}; enum Event { - MessagesFor(Hash, mpsc::UnboundedSender), - RegisterValidator(Arc>), - GossipMessage(Hash, Vec, bool), - SendMessage(Vec, Vec), + EventStream(mpsc::UnboundedSender), + WriteNotification(network::PeerId, Vec), Report(network::PeerId, i32), Announce(Hash), } @@ -46,8 +44,10 @@ struct TestNetwork { } impl network_gossip::Network for TestNetwork { - fn event_stream(&self) -> Box + Send> { + fn event_stream(&self) + -> Box + Send> { let (tx, rx) = mpsc::unbounded(); + let _ = self.sender.unbounded_send(Event::EventStream(tx)); Box::new(rx) } @@ -57,52 +57,22 @@ impl network_gossip::Network for TestNetwork { fn disconnect_peer(&self, _: PeerId) {} - fn write_notification(&self, who: PeerId, engine_id: ConsensusEngineId, message: Vec) { - + fn write_notification(&self, who: PeerId, _: ConsensusEngineId, message: Vec) { + let _ = self.sender.unbounded_send(Event::WriteNotification(who, message)); } fn register_notifications_protocol(&self, _: ConsensusEngineId) {} - /*/// Get a stream of messages for a specific gossip topic. - fn messages_for(&self, topic: Hash) -> Self::In { - let (tx, rx) = mpsc::unbounded(); - let _ = self.sender.unbounded_send(Event::MessagesFor(topic, tx)); - - rx - } - - /// Register a gossip validator. - fn register_validator(&self, validator: Arc>) { - let _ = self.sender.unbounded_send(Event::RegisterValidator(validator)); - } - - /// Gossip a message out to all connected peers. - /// - /// Force causes it to be sent to all peers, even if they've seen it already. - /// Only should be used in case of consensus stall. - fn gossip_message(&self, topic: Hash, data: Vec, force: bool) { - let _ = self.sender.unbounded_send(Event::GossipMessage(topic, data, force)); - } - - /// Send a message to a bunch of specific peers, even if they've seen it already. - fn send_message(&self, who: Vec, data: Vec) { - let _ = self.sender.unbounded_send(Event::SendMessage(who, data)); - } - - /// Register a message with the gossip service, it isn't broadcast right - /// away to any peers, but may be sent to new peers joining or when asked to - /// broadcast the topic. Useful to register previous messages on node - /// startup. - fn register_gossip_message(&self, _topic: Hash, _data: Vec) { - // NOTE: only required to restore previous state on startup - // not required for tests currently - }*/ - fn announce(&self, block: Hash, _associated_data: Vec) { let _ = self.sender.unbounded_send(Event::Announce(block)); } - fn set_sync_fork_request(&self, _peers: Vec, _hash: Hash, _number: NumberFor) {} + fn set_sync_fork_request( + &self, + _peers: Vec, + _hash: Hash, + _number: NumberFor + ) {} } impl network_gossip::ValidatorContext for TestNetwork { @@ -111,7 +81,12 @@ impl network_gossip::ValidatorContext for TestNetwork { fn broadcast_message(&mut self, _: Hash, _: Vec, _: bool) { } fn send_message(&mut self, who: &network::PeerId, data: Vec) { - >::send_message(self, vec![who.clone()], data); + >::write_notification( + self, + who.clone(), + Default::default(), + data + ); } fn send_topic(&mut self, _: &network::PeerId, _: Hash, _: bool) { } @@ -294,11 +269,10 @@ fn good_commit_leads_to_relay() { // send a message. let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { - Event::MessagesFor(topic, sender) => { - if topic != global_topic { return false } - let _ = sender.unbounded_send(network_gossip::TopicNotification { - message: commit_to_send.clone(), - sender: Some(sender_id.clone()), + Event::EventStream(sender) => { + let _ = sender.unbounded_send(NetworkEvent::NotifMessages { + remote: sender_id.clone(), + messages: vec![(Default::default(), commit_to_send.clone().into())], }); true @@ -322,8 +296,8 @@ fn good_commit_leads_to_relay() { // a repropagation event coming from the network. send_message.join(handle_commit).and_then(move |(tester, ())| { tester.filter_network_events(move |event| match event { - Event::GossipMessage(topic, data, false) => { - if topic == global_topic && data == encoded_commit { + Event::WriteNotification(_, data) => { + if data == encoded_commit { true } else { panic!("Trying to gossip something strange") @@ -409,11 +383,10 @@ fn bad_commit_leads_to_report() { // send a message. let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { - Event::MessagesFor(topic, sender) => { - if topic != global_topic { return false } - let _ = sender.unbounded_send(network_gossip::TopicNotification { - message: commit_to_send.clone(), - sender: Some(sender_id.clone()), + Event::EventStream(sender) => { + let _ = sender.unbounded_send(NetworkEvent::NotifMessages { + remote: sender_id.clone(), + messages: vec![(Default::default(), commit_to_send.clone().into())], }); true @@ -485,10 +458,10 @@ fn peer_with_higher_view_leads_to_catch_up_request() { // a catch up request should be sent to the peer for round - 1 tester.filter_network_events(move |event| match event { - Event::SendMessage(peers, message) => { + Event::WriteNotification(peer, message) => { assert_eq!( - peers, - vec![id.clone()], + peer, + id, ); assert_eq!( From 64d2d8e9e13c28493c9418fab8837f52b5ad66cc Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 2 Dec 2019 10:12:27 +0100 Subject: [PATCH 03/21] Try put correct engine ID --- client/finality-grandpa/src/communication/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 72a8c70baf998..396ba7010c6e3 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -27,7 +27,7 @@ use codec::Encode; use sr_primitives::{ConsensusEngineId, traits::NumberFor}; use crate::environment::SharedVoterSetState; -use fg_primitives::AuthorityList; +use fg_primitives::{AuthorityList, GRANDPA_ENGINE_ID}; use super::gossip::{self, GossipValidator}; use super::{AuthorityId, VoterSet, Round, SetId}; @@ -84,7 +84,7 @@ impl network_gossip::ValidatorContext for TestNetwork { >::write_notification( self, who.clone(), - Default::default(), + GRANDPA_ENGINE_ID, data ); } @@ -272,7 +272,7 @@ fn good_commit_leads_to_relay() { Event::EventStream(sender) => { let _ = sender.unbounded_send(NetworkEvent::NotifMessages { remote: sender_id.clone(), - messages: vec![(Default::default(), commit_to_send.clone().into())], + messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], }); true @@ -386,7 +386,7 @@ fn bad_commit_leads_to_report() { Event::EventStream(sender) => { let _ = sender.unbounded_send(NetworkEvent::NotifMessages { remote: sender_id.clone(), - messages: vec![(Default::default(), commit_to_send.clone().into())], + messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], }); true From 5747b8ced1d0dd2c29ed8d29dac3ba9ddd0bf1b5 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 2 Dec 2019 11:42:58 +0100 Subject: [PATCH 04/21] Fix messages encoding --- client/network/src/service.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index a9b7bdc1b64fb..98f2b53d74f2f 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -28,7 +28,6 @@ use std::{collections::{HashMap, HashSet}, fs, marker::PhantomData, io, path::Path}; use std::sync::{Arc, atomic::{AtomicBool, AtomicUsize, Ordering}}; -use codec::Encode; use consensus::import_queue::{ImportQueue, Link}; use consensus::import_queue::{BlockImportResult, BlockImportError}; use futures::{prelude::*, sync::mpsc}; @@ -428,11 +427,11 @@ impl, H: ExHashT> NetworkServic /// /// The protocol must have been registered with `register_notifications_protocol`. /// - pub fn write_notification(&self, target: PeerId, engine_id: ConsensusEngineId, message: impl Encode) { + pub fn write_notification(&self, target: PeerId, engine_id: ConsensusEngineId, message: Vec) { let _ = self.to_worker.unbounded_send(ServerToWorkerMsg::WriteNotification { target, engine_id, - message: message.encode(), + message, }); } From be5c07dd180c68be88dee0fa150e90e1ac837722 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 2 Dec 2019 14:41:00 +0100 Subject: [PATCH 05/21] Fix communication tests --- .../src/communication/tests.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 396ba7010c6e3..6e790a3fcf3f1 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -270,6 +270,10 @@ fn good_commit_leads_to_relay() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { + let _ = sender.unbounded_send(NetworkEvent::NotifOpened { + remote: sender_id.clone(), + engine_id: GRANDPA_ENGINE_ID, + }); let _ = sender.unbounded_send(NetworkEvent::NotifMessages { remote: sender_id.clone(), messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], @@ -297,11 +301,7 @@ fn good_commit_leads_to_relay() { send_message.join(handle_commit).and_then(move |(tester, ())| { tester.filter_network_events(move |event| match event { Event::WriteNotification(_, data) => { - if data == encoded_commit { - true - } else { - panic!("Trying to gossip something strange") - } + data == encoded_commit } _ => false, }) @@ -310,11 +310,12 @@ fn good_commit_leads_to_relay() { .map(|_| ()) }); - current_thread::block_on_all(test).unwrap(); + current_thread::Runtime::new().unwrap().block_on(test).unwrap(); } #[test] fn bad_commit_leads_to_report() { + env_logger::init(); let private = [Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let public = make_ids(&private[..]); let voter_set = Arc::new(public.iter().cloned().collect::>()); @@ -384,6 +385,10 @@ fn bad_commit_leads_to_report() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { + let _ = sender.unbounded_send(NetworkEvent::NotifOpened { + remote: sender_id.clone(), + engine_id: GRANDPA_ENGINE_ID, + }); let _ = sender.unbounded_send(NetworkEvent::NotifMessages { remote: sender_id.clone(), messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], @@ -411,11 +416,7 @@ fn bad_commit_leads_to_report() { send_message.join(handle_commit).and_then(move |(tester, ())| { tester.filter_network_events(move |event| match event { Event::Report(who, cost_benefit) => { - if who == id && cost_benefit == super::cost::INVALID_COMMIT { - true - } else { - panic!("reported unknown peer or unexpected cost"); - } + who == id && cost_benefit == super::cost::INVALID_COMMIT } _ => false, }) @@ -424,7 +425,7 @@ fn bad_commit_leads_to_report() { .map(|_| ()) }); - current_thread::block_on_all(test).unwrap(); + current_thread::Runtime::new().unwrap().block_on(test).unwrap(); } #[test] @@ -482,5 +483,5 @@ fn peer_with_higher_view_leads_to_catch_up_request() { .map(|_| ()) }); - current_thread::block_on_all(test).unwrap(); + current_thread::Runtime::new().unwrap().block_on(test).unwrap(); } From 64629da1781d8cf136196baafd14bd91729b19be Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 2 Dec 2019 17:23:43 +0100 Subject: [PATCH 06/21] Use a threads pool to spawn stuff --- bin/node-template/src/service.rs | 1 + bin/node/cli/src/service.rs | 1 + .../finality-grandpa/src/communication/mod.rs | 25 ++++------ .../src/communication/tests.rs | 12 +++-- client/finality-grandpa/src/lib.rs | 16 +++++-- client/finality-grandpa/src/observer.rs | 5 +- client/finality-grandpa/src/tests.rs | 48 +++++++++++++++---- client/network-gossip/src/bridge.rs | 17 +++++-- client/network-gossip/src/lib.rs | 5 +- 9 files changed, 89 insertions(+), 41 deletions(-) diff --git a/bin/node-template/src/service.rs b/bin/node-template/src/service.rs index 6fd4daed69c8a..267d34500d7bb 100644 --- a/bin/node-template/src/service.rs +++ b/bin/node-template/src/service.rs @@ -170,6 +170,7 @@ pub fn new_full(config: Configuration NetworkBridge { service: N, config: crate::Config, set_state: crate::environment::SharedVoterSetState, + executor: &impl futures03::task::Spawn, on_exit: impl Future + Clone + Send + 'static, ) -> ( Self, impl Future + Send + 'static, ) { - let (validator, report_stream) = GossipValidator::new( config, set_state.clone(), ); let validator = Arc::new(validator); - let service = GossipEngine::new(service, GRANDPA_ENGINE_ID, validator.clone()); + let service = GossipEngine::new(service, executor, GRANDPA_ENGINE_ID, validator.clone()); { // register all previous votes with the gossip service so that they're @@ -182,18 +180,13 @@ impl NetworkBridge { let bridge = NetworkBridge { service, validator, neighbor_sender }; - let startup_work = futures::future::lazy(move || { - // lazily spawn these jobs onto their own tasks. the lazy future has access - // to tokio globals, which aren't available outside. - let mut executor = tokio_executor::DefaultExecutor::current(); - executor.spawn(Box::new(rebroadcast_job.select(on_exit.clone()).then(|_| Ok(())))) - .expect("failed to spawn grandpa rebroadcast job task"); - executor.spawn(Box::new(reporting_job.select(on_exit.clone()).then(|_| Ok(())))) - .expect("failed to spawn grandpa reporting job task"); - Ok(()) - }); + let executor = Compat::new(executor); + executor.execute(Box::new(rebroadcast_job.select(on_exit.clone()).then(|_| Ok(())))) + .expect("failed to spawn grandpa rebroadcast job task"); + executor.execute(Box::new(reporting_job.select(on_exit.clone()).then(|_| Ok(())))) + .expect("failed to spawn grandpa reporting job task"); - (bridge, startup_work) + (bridge, futures::future::ok(())) } /// Note the beginning of a new round to the `GossipValidator`. diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 6e790a3fcf3f1..b35fde4c1680f 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -147,7 +147,7 @@ fn voter_set_state() -> SharedVoterSetState { } // needs to run in a tokio runtime. -fn make_test_network() -> ( +fn make_test_network(executor: &impl futures03::task::Spawn) -> ( impl Future, TestNetwork, ) { @@ -170,6 +170,7 @@ fn make_test_network() -> ( net.clone(), config(), voter_set_state(), + executor, Exit, ); @@ -244,7 +245,8 @@ fn good_commit_leads_to_relay() { let id = network::PeerId::random(); let global_topic = super::global_topic::(set_id); - let test = make_test_network().0 + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); + let test = make_test_network(&threads_pool).0 .and_then(move |tester| { // register a peer. tester.gossip_validator.new_peer(&mut NoopContext, &id, network::config::Roles::FULL); @@ -359,7 +361,8 @@ fn bad_commit_leads_to_report() { let id = network::PeerId::random(); let global_topic = super::global_topic::(set_id); - let test = make_test_network().0 + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); + let test = make_test_network(&threads_pool).0 .and_then(move |tester| { // register a peer. tester.gossip_validator.new_peer(&mut NoopContext, &id, network::config::Roles::FULL); @@ -432,7 +435,8 @@ fn bad_commit_leads_to_report() { fn peer_with_higher_view_leads_to_catch_up_request() { let id = network::PeerId::random(); - let (tester, mut net) = make_test_network(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); + let (tester, mut net) = make_test_network(&threads_pool); let test = tester .and_then(move |tester| { // register a peer with authority role. diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 86d239990b582..b7525196ad0b3 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -521,7 +521,7 @@ fn register_finality_tracker_inherent_data_provider, N, RA, SC, VR, X> { +pub struct GrandpaParams, N, RA, SC, VR, X, Sp> { /// Configuration for the GRANDPA service. pub config: Config, /// A link to the block import worker. @@ -536,12 +536,14 @@ pub struct GrandpaParams, N, RA, SC, VR, X> { pub telemetry_on_connect: Option>, /// A voting rule used to potentially restrict target votes. pub voting_rule: VR, + /// How to spawn background tasks. + pub executor: Sp, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a /// block import worker that has already been instantiated with `block_import`. -pub fn run_grandpa_voter, N, RA, SC, VR, X>( - grandpa_params: GrandpaParams, +pub fn run_grandpa_voter, N, RA, SC, VR, X, Sp>( + grandpa_params: GrandpaParams, ) -> sp_blockchain::Result + Send + 'static> where Block::Hash: Ord, B: Backend + 'static, @@ -553,6 +555,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X>( DigestFor: Encode, RA: Send + Sync + 'static, X: Future + Clone + Send + 'static, + Sp: futures03::task::Spawn + 'static, { let GrandpaParams { config, @@ -562,6 +565,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X>( on_exit, telemetry_on_connect, voting_rule, + executor, } = grandpa_params; let LinkHalf { @@ -575,6 +579,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X>( network, config.clone(), persistent_data.set_state.clone(), + &executor, on_exit.clone(), ); @@ -869,8 +874,8 @@ where } #[deprecated(since = "1.1.0", note = "Please switch to run_grandpa_voter.")] -pub fn run_grandpa, N, RA, SC, VR, X>( - grandpa_params: GrandpaParams, +pub fn run_grandpa, N, RA, SC, VR, X, Sp>( + grandpa_params: GrandpaParams, ) -> ::sp_blockchain::Result + Send + 'static> where Block::Hash: Ord, B: Backend + 'static, @@ -882,6 +887,7 @@ pub fn run_grandpa, N, RA, SC, VR, X>( RA: Send + Sync + 'static, VR: VotingRule> + Clone + 'static, X: Future + Clone + Send + 'static, + Sp: futures03::task::Spawn + 'static, { run_grandpa_voter(grandpa_params) } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 19f8b0243332c..fa70c4a051110 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -151,11 +151,12 @@ fn grandpa_observer, RA, S, F>( /// listening for and validating GRANDPA commits instead of following the full /// protocol. Provide configuration and a link to a block import worker that has /// already been instantiated with `block_import`. -pub fn run_grandpa_observer, N, RA, SC>( +pub fn run_grandpa_observer, N, RA, SC, Sp>( config: Config, link: LinkHalf, network: N, on_exit: impl Future + Clone + Send + 'static, + executor: Sp, ) -> ::sp_blockchain::Result + Send + 'static> where B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, @@ -163,6 +164,7 @@ pub fn run_grandpa_observer, N, RA, SC>( SC: SelectChain + 'static, NumberFor: BlockNumberOps, RA: Send + Sync + 'static, + Sp: futures03::task::Spawn + 'static, { let LinkHalf { client, @@ -175,6 +177,7 @@ pub fn run_grandpa_observer, N, RA, SC>( network, config.clone(), persistent_data.set_state.clone(), + &executor, on_exit.clone(), ); diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 1226176456458..3f7d2a41f25af 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -338,6 +338,7 @@ fn create_keystore(authority: Ed25519Keyring) -> (KeyStorePtr, tempfile::TempDir // the voters are spawned but before blocking on them. fn run_to_completion_with( runtime: &mut current_thread::Runtime, + threads_pool: &futures03::executor::ThreadPool, blocks: u64, net: Arc>, peers: &[Ed25519Keyring], @@ -405,6 +406,7 @@ fn run_to_completion_with( on_exit: Exit, telemetry_on_connect: None, voting_rule: (), + executor: threads_pool.clone(), }; let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); @@ -427,11 +429,12 @@ fn run_to_completion_with( fn run_to_completion( runtime: &mut current_thread::Runtime, + threads_pool: &futures03::executor::ThreadPool, blocks: u64, net: Arc>, peers: &[Ed25519Keyring] ) -> u64 { - run_to_completion_with(runtime, blocks, net, peers, |_| None) + run_to_completion_with(runtime, threads_pool, blocks, net, peers, |_| None) } fn add_scheduled_change(block: &mut Block, change: ScheduledChange) { @@ -456,6 +459,7 @@ fn add_forced_change( fn finalize_3_voters_no_observers() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(peers); @@ -469,7 +473,7 @@ fn finalize_3_voters_no_observers() { } let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 20, net.clone(), peers); + run_to_completion(&mut runtime, &threads_pool, 20, net.clone(), peers); // normally there's no justification for finalized blocks assert!( @@ -481,6 +485,7 @@ fn finalize_3_voters_no_observers() { #[test] fn finalize_3_voters_1_full_observer() { let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(peers); @@ -541,6 +546,7 @@ fn finalize_3_voters_1_full_observer() { on_exit: Exit, telemetry_on_connect: None, voting_rule: (), + executor: threads_pool.clone(), }; voters.push(run_grandpa_voter(grandpa_params).expect("all in order with client and network")); @@ -588,6 +594,7 @@ fn transition_3_voters_twice_1_full_observer() { let net = Arc::new(Mutex::new(GrandpaTestNet::new(api, 8))); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); net.lock().peer(0).push_blocks(1, false); net.lock().block_until_sync(&mut runtime); @@ -692,6 +699,7 @@ fn transition_3_voters_twice_1_full_observer() { assert_eq!(set.pending_changes().count(), 0); }) ); + let grandpa_params = GrandpaParams { config: Config { gossip_duration: TEST_GOSSIP_DURATION, @@ -707,6 +715,7 @@ fn transition_3_voters_twice_1_full_observer() { on_exit: Exit, telemetry_on_connect: None, voting_rule: (), + executor: threads_pool.clone(), }; let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); @@ -725,6 +734,7 @@ fn transition_3_voters_twice_1_full_observer() { #[test] fn justification_is_emitted_when_consensus_data_changes() { let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let mut net = GrandpaTestNet::new(TestApi::new(make_ids(peers)), 3); @@ -733,7 +743,7 @@ fn justification_is_emitted_when_consensus_data_changes() { net.peer(0).push_authorities_change_block(new_authorities); net.block_until_sync(&mut runtime); let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 1, net.clone(), peers); + run_to_completion(&mut runtime, &threads_pool, 1, net.clone(), peers); // ... and check that there's justification for block#1 assert!(net.lock().peer(0).client().justification(&BlockId::Number(1)).unwrap().is_some(), @@ -743,6 +753,7 @@ fn justification_is_emitted_when_consensus_data_changes() { #[test] fn justification_is_generated_periodically() { let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(peers); @@ -751,7 +762,7 @@ fn justification_is_generated_periodically() { net.block_until_sync(&mut runtime); let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 32, net.clone(), peers); + run_to_completion(&mut runtime, &threads_pool, 32, net.clone(), peers); // when block#32 (justification_period) is finalized, justification // is required => generated @@ -782,6 +793,7 @@ fn consensus_changes_works() { #[test] fn sync_justifications_on_change_blocks() { let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers_a = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let peers_b = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob]; let voters = make_ids(peers_b); @@ -813,7 +825,7 @@ fn sync_justifications_on_change_blocks() { } let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 25, net.clone(), peers_a); + run_to_completion(&mut runtime, &threads_pool, 25, net.clone(), peers_a); // the first 3 peers are grandpa voters and therefore have already finalized // block 21 and stored a justification @@ -836,6 +848,7 @@ fn sync_justifications_on_change_blocks() { fn finalizes_multiple_pending_changes_in_order() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers_a = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let peers_b = &[Ed25519Keyring::Dave, Ed25519Keyring::Eve, Ed25519Keyring::Ferdie]; @@ -889,13 +902,14 @@ fn finalizes_multiple_pending_changes_in_order() { } let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 30, net.clone(), all_peers); + run_to_completion(&mut runtime, &threads_pool, 30, net.clone(), all_peers); } #[test] fn force_change_to_new_set() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); // two of these guys are offline. let genesis_authorities = &[ Ed25519Keyring::Alice, @@ -946,7 +960,7 @@ fn force_change_to_new_set() { // it will only finalize if the forced transition happens. // we add_blocks after the voters are spawned because otherwise // the link-halfs have the wrong AuthoritySet - run_to_completion(&mut runtime, 25, net, peers_a); + run_to_completion(&mut runtime, &threads_pool, 25, net, peers_a); } #[test] @@ -1064,6 +1078,7 @@ fn voter_persists_its_votes() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); // we have two authorities but we'll only be running the voter for alice // we are going to be listening for the prevotes it casts @@ -1102,6 +1117,7 @@ fn voter_persists_its_votes() { net: Arc>, client: PeersClient, keystore: KeyStorePtr, + threads_pool: futures03::executor::ThreadPool, } impl Future for ResettableVoter { @@ -1137,6 +1153,7 @@ fn voter_persists_its_votes() { on_exit: Exit, telemetry_on_connect: None, voting_rule: VotingRulesBuilder::default().build(), + executor: self.threads_pool.clone(), }; let voter = run_grandpa_voter(grandpa_params) @@ -1168,6 +1185,7 @@ fn voter_persists_its_votes() { net: net.clone(), client: client.clone(), keystore, + threads_pool: threads_pool.clone(), }); } @@ -1200,6 +1218,7 @@ fn voter_persists_its_votes() { net.lock().peers[1].network_service().clone(), config.clone(), set_state, + &threads_pool, Exit, ); runtime.block_on(routing_work).unwrap(); @@ -1307,6 +1326,7 @@ fn voter_persists_its_votes() { fn finalize_3_voters_1_light_observer() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let authorities = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(authorities); @@ -1327,7 +1347,7 @@ fn finalize_3_voters_1_light_observer() { .take_while(|n| Ok(n.header.number() < &20)) .collect(); - run_to_completion_with(&mut runtime, 20, net.clone(), authorities, |executor| { + run_to_completion_with(&mut runtime, &threads_pool, 20, net.clone(), authorities, |executor| { executor.spawn( run_grandpa_observer( Config { @@ -1341,6 +1361,7 @@ fn finalize_3_voters_1_light_observer() { link, net.lock().peers[3].network_service().clone(), Exit, + threads_pool.clone(), ).unwrap() ).unwrap(); @@ -1352,6 +1373,7 @@ fn finalize_3_voters_1_light_observer() { fn finality_proof_is_fetched_by_light_client_when_consensus_data_changes() { let _ = ::env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice]; let mut net = GrandpaTestNet::new(TestApi::new(make_ids(peers)), 1); @@ -1361,7 +1383,7 @@ fn finality_proof_is_fetched_by_light_client_when_consensus_data_changes() { // && instead fetches finality proof for block #1 net.peer(0).push_authorities_change_block(vec![babe_primitives::AuthorityId::from_slice(&[42; 32])]); let net = Arc::new(Mutex::new(net)); - run_to_completion(&mut runtime, 1, net.clone(), peers); + run_to_completion(&mut runtime, &threads_pool, 1, net.clone(), peers); net.lock().block_until_sync(&mut runtime); // check that the block#1 is finalized on light client @@ -1382,6 +1404,7 @@ fn empty_finality_proof_is_returned_to_light_client_when_authority_set_is_differ let _ = ::env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); // two of these guys are offline. let genesis_authorities = if FORCE_CHANGE { @@ -1429,7 +1452,7 @@ fn empty_finality_proof_is_returned_to_light_client_when_authority_set_is_differ net.lock().block_until_sync(&mut runtime); // finalize block #11 on full clients - run_to_completion(&mut runtime, 11, net.clone(), peers_a); + run_to_completion(&mut runtime, &threads_pool, 11, net.clone(), peers_a); // request finalization by light client net.lock().add_light_peer(&GrandpaTestNet::default_config()); @@ -1446,6 +1469,7 @@ fn empty_finality_proof_is_returned_to_light_client_when_authority_set_is_differ fn voter_catches_up_to_latest_round_when_behind() { let _ = env_logger::try_init(); let mut runtime = current_thread::Runtime::new().unwrap(); + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob]; let voters = make_ids(peers); @@ -1473,6 +1497,7 @@ fn voter_catches_up_to_latest_round_when_behind() { on_exit: Exit, telemetry_on_connect: None, voting_rule: (), + executor: threads_pool.clone(), }; Box::new(run_grandpa_voter(grandpa_params).expect("all in order with client and network")) @@ -1560,6 +1585,8 @@ fn grandpa_environment_respects_voting_rules() { use grandpa::Chain; use network::test::TestClient; + let threads_pool = futures03::executor::ThreadPool::new().unwrap(); + let peers = &[Ed25519Keyring::Alice]; let voters = make_ids(peers); @@ -1590,6 +1617,7 @@ fn grandpa_environment_respects_voting_rules() { network_service.clone(), config.clone(), set_state.clone(), + &threads_pool, Exit, ); diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 4c09173ac027c..630229c8a0233 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -21,7 +21,7 @@ use network::Context; use network::message::generic::ConsensusMessage; use network::{Event, config::Roles}; -use futures::{prelude::*, channel::mpsc, compat::Compat01As03}; +use futures::{prelude::*, channel::mpsc, compat::Compat01As03, task::SpawnExt as _}; use libp2p::PeerId; use parking_lot::Mutex; use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; @@ -42,6 +42,7 @@ impl GossipEngine { /// Create a new instance. pub fn new + Send + Clone + 'static>( network: N, + executor: &impl futures::task::Spawn, engine_id: ConsensusEngineId, validator: Arc>, ) -> Self where B: 'static { @@ -71,7 +72,7 @@ impl GossipEngine { engine_id, }; - async_std::task::spawn({ + let res = executor.spawn({ let inner = Arc::downgrade(&inner); async move { loop { @@ -87,7 +88,12 @@ impl GossipEngine { } }); - async_std::task::spawn(async move { + // Note: we consider the chances of an error to spawn a background task almost null. + if res.is_err() { + log::error!(target: "gossip", "Failed to spawn background task"); + } + + let res = executor.spawn(async move { let mut stream = Compat01As03::new(event_stream); while let Some(Ok(event)) = stream.next().await { match event { @@ -126,6 +132,11 @@ impl GossipEngine { } }); + // Note: we consider the chances of an error to spawn a background task almost null. + if res.is_err() { + log::error!(target: "gossip", "Failed to spawn background task"); + } + gossip_engine } diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index a2eb1507c113b..8cea4027a9faf 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -15,8 +15,9 @@ // along with Substrate. If not, see . pub use self::bridge::GossipEngine; -// TODO: remove -pub use self::state_machine::*; +pub use self::state_machine::{TopicNotification, MessageRecipient, MessageIntent}; +pub use self::state_machine::{Validator, ValidatorContext, ValidationResult}; +pub use self::state_machine::DiscardAll; use network::{specialization::NetworkSpecialization, Event, ExHashT, NetworkService, PeerId}; use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; From 76d9bf30a596e31bedf57614612a53214e78a9ee Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 2 Dec 2019 17:43:15 +0100 Subject: [PATCH 07/21] Fix compilation everywhere --- bin/node-template/src/service.rs | 1 + bin/node/cli/src/service.rs | 1 + client/network/src/service.rs | 6 ++-- client/service/src/lib.rs | 47 +++++++++++++++++++++++--------- 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/bin/node-template/src/service.rs b/bin/node-template/src/service.rs index 267d34500d7bb..41e6833436f66 100644 --- a/bin/node-template/src/service.rs +++ b/bin/node-template/src/service.rs @@ -158,6 +158,7 @@ pub fn new_full(config: Configuration { diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index ad0128ed05732..bf9ae303da1e1 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -228,6 +228,7 @@ macro_rules! new_full { grandpa_link, service.network(), service.on_exit(), + service.spawn_task_handle(), )?); }, (true, false) => { diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 98f2b53d74f2f..dc57663cb113b 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -703,11 +703,11 @@ pub struct NetworkWorker, H: Ex event_streams: Vec>, } -impl, H: ExHashT> Stream for NetworkWorker { +impl, H: ExHashT> Future for NetworkWorker { type Item = (); type Error = io::Error; - fn poll(&mut self) -> Poll, Self::Error> { + fn poll(&mut self) -> Poll { // Poll the import queue for actions to perform. let _ = futures03::future::poll_fn(|cx| { self.import_queue.poll_actions(cx, &mut NetworkLink { @@ -727,7 +727,7 @@ impl, H: ExHashT> Stream for Ne // Process the next message coming from the `NetworkService`. let msg = match self.from_worker.poll() { Ok(Async::Ready(Some(msg))) => msg, - Ok(Async::Ready(None)) | Err(_) => return Ok(Async::Ready(None)), + Ok(Async::Ready(None)) | Err(_) => return Ok(Async::Ready(())), Ok(Async::NotReady) => break, }; diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 2115fd50a7167..e0c2e49d1d2e9 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -130,6 +130,14 @@ impl Executor + Send>> for SpawnTaskHandle } } +impl futures03::task::Spawn for SpawnTaskHandle { + fn spawn_obj(&self, future: futures03::task::FutureObj<'static, ()>) + -> Result<(), futures03::task::SpawnError> { + self.execute(Box::new(futures03::compat::Compat::new(future.unit_error()))) + .map_err(|_| futures03::task::SpawnError::shutdown()) + } +} + /// Abstraction over a Substrate service. pub trait AbstractService: 'static + Future + Executor + Send>> + Send { @@ -372,6 +380,9 @@ fn build_network_future< let mut finality_notification_stream = client.finality_notification_stream().fuse() .map(|v| Ok::<_, ()>(v)).compat(); + // Initializing a stream in order to obtain DHT events from the network. + let mut event_stream = network.service().event_stream(); + futures::future::poll_fn(move || { let before_polling = Instant::now(); @@ -448,22 +459,32 @@ fn build_network_future< (status, state) }); + // Processing DHT events. + while let Ok(Async::Ready(Some(event))) = event_stream.poll() { + match event { + Event::Dht(event) => { + // Given that client/authority-discovery is the only upper stack consumer of Dht events at the moment, all Dht + // events are being passed on to the authority-discovery module. In the future there might be multiple + // consumers of these events. In that case this would need to be refactored to properly dispatch the events, + // e.g. via a subscriber model. + if let Some(Err(e)) = dht_event_tx.as_ref().map(|c| c.clone().try_send(event)) { + if e.is_full() { + warn!(target: "service", "Dht event channel to authority discovery is full, dropping event."); + } else if e.is_disconnected() { + warn!(target: "service", "Dht event channel to authority discovery is disconnected, dropping event."); + } + } + } + _ => {} + } + } + // Main network polling. - while let Ok(Async::Ready(Some(Event::Dht(event)))) = network.poll().map_err(|err| { + if let Ok(Async::Ready(())) = network.poll().map_err(|err| { warn!(target: "service", "Error in network: {:?}", err); }) { - // Given that client/authority-discovery is the only upper stack consumer of Dht events at the moment, all Dht - // events are being passed on to the authority-discovery module. In the future there might be multiple - // consumers of these events. In that case this would need to be refactored to properly dispatch the events, - // e.g. via a subscriber model. - if let Some(Err(e)) = dht_event_tx.as_ref().map(|c| c.clone().try_send(event)) { - if e.is_full() { - warn!(target: "service", "Dht event channel to authority discovery is full, dropping event."); - } else if e.is_disconnected() { - warn!(target: "service", "Dht event channel to authority discovery is disconnected, dropping event."); - } - } - }; + return Ok(Async::Ready(())); + } // Now some diagnostic for performances. let polling_dur = before_polling.elapsed(); From 0dbccc6ed1dfe3d83e631f9f738f70cf51a3614e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 3 Dec 2019 11:36:43 +0100 Subject: [PATCH 08/21] Fix bad merge conflict --- client/network-gossip/src/bridge.rs | 2 +- client/network-gossip/src/lib.rs | 2 +- client/network/src/behaviour.rs | 6 +----- client/network/src/protocol/event.rs | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 630229c8a0233..be74095435014 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -24,7 +24,7 @@ use network::{Event, config::Roles}; use futures::{prelude::*, channel::mpsc, compat::Compat01As03, task::SpawnExt as _}; use libp2p::PeerId; use parking_lot::Mutex; -use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use sp_runtime::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; use std::{sync::Arc, time::Duration}; pub struct GossipEngine { diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 8cea4027a9faf..cbc015fc7201f 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -20,7 +20,7 @@ pub use self::state_machine::{Validator, ValidatorContext, ValidationResult}; pub use self::state_machine::DiscardAll; use network::{specialization::NetworkSpecialization, Event, ExHashT, NetworkService, PeerId}; -use sr_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use sp_runtime::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; use std::sync::Arc; mod bridge; diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index 1cfec5f257bb8..b7b04e1eceedd 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -28,11 +28,7 @@ use libp2p::kad::record; use libp2p::swarm::{NetworkBehaviourAction, NetworkBehaviourEventProcess}; use libp2p::core::{nodes::Substream, muxing::StreamMuxerBox}; use log::{debug, warn}; -<<<<<<< HEAD -use sr_primitives::{traits::{Block as BlockT, NumberFor}, Justification}; -======= -use sp_runtime::traits::Block as BlockT; ->>>>>>> upstream/master +use sp_runtime::{traits::{Block as BlockT, NumberFor}, Justification}; use std::iter; use void; diff --git a/client/network/src/protocol/event.rs b/client/network/src/protocol/event.rs index d2da45cc7370a..68e13fbb190b8 100644 --- a/client/network/src/protocol/event.rs +++ b/client/network/src/protocol/event.rs @@ -20,7 +20,7 @@ use bytes::Bytes; use libp2p::core::PeerId; use libp2p::kad::record::Key; -use sr_primitives::ConsensusEngineId; +use sp_runtime::ConsensusEngineId; /// Events generated by DHT as a response to get_value and put_value requests. #[derive(Debug, Clone)] From 7134aa465c248a40b60cbfbfea27477cbaed71a8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 3 Dec 2019 15:47:29 +0100 Subject: [PATCH 09/21] Remove dependency on async-std --- Cargo.lock | 1 - client/network-gossip/Cargo.toml | 1 - client/network-gossip/src/bridge.rs | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cf92b5c405bad..b72a6e72fcb19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5339,7 +5339,6 @@ dependencies = [ name = "sc-network-gossip" version = "2.0.0" dependencies = [ - "async-std 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "futures-timer 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/client/network-gossip/Cargo.toml b/client/network-gossip/Cargo.toml index 4980e537b59eb..7f0e9184e0eed 100644 --- a/client/network-gossip/Cargo.toml +++ b/client/network-gossip/Cargo.toml @@ -7,7 +7,6 @@ authors = ["Parity Technologies "] edition = "2018" [dependencies] -async-std = "1.0" log = "0.4.8" futures01 = { package = "futures", version = "0.1.29" } futures = { version = "0.3.1", features = ["compat"] } diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 6da6238e31ed4..8739ed3bc1fca 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -76,7 +76,7 @@ impl GossipEngine { let inner = Arc::downgrade(&inner); async move { loop { - async_std::task::sleep(Duration::from_millis(1100)).await; + let _ = futures_timer::Delay::new(Duration::from_millis(1100)).await; if let Some(inner) = inner.upgrade() { let mut inner = inner.lock(); let inner = &mut *inner; From cdca5006b4430c34d4917001f60785d469b65446 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 4 Dec 2019 11:22:00 +0100 Subject: [PATCH 10/21] Apply suggestions from code review Co-Authored-By: Robert Habermeier --- client/finality-grandpa/src/communication/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index ea363b73109b4..dd6f677197436 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -71,7 +71,7 @@ impl network_gossip::Network for TestNetwork { &self, _peers: Vec, _hash: Hash, - _number: NumberFor + _number: NumberFor, ) {} } @@ -85,7 +85,7 @@ impl network_gossip::ValidatorContext for TestNetwork { self, who.clone(), GRANDPA_ENGINE_ID, - data + data, ); } From d1cc54284c08a2de735d007bceaad2b6ec6ad6a3 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 4 Dec 2019 11:28:51 +0100 Subject: [PATCH 11/21] More suggestions --- .../finality-grandpa/src/communication/tests.rs | 8 ++++---- client/network-gossip/src/bridge.rs | 12 +++++++----- client/network/src/behaviour.rs | 12 ++++++------ client/network/src/protocol.rs | 16 ++++++++-------- client/network/src/protocol/event.rs | 8 ++++---- 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index dd6f677197436..572d3ec7eda8b 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -272,11 +272,11 @@ fn good_commit_leads_to_relay() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { - let _ = sender.unbounded_send(NetworkEvent::NotifOpened { + let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, }); - let _ = sender.unbounded_send(NetworkEvent::NotifMessages { + let _ = sender.unbounded_send(NetworkEvent::NotificationsReceived { remote: sender_id.clone(), messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], }); @@ -388,11 +388,11 @@ fn bad_commit_leads_to_report() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { - let _ = sender.unbounded_send(NetworkEvent::NotifOpened { + let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, }); - let _ = sender.unbounded_send(NetworkEvent::NotifMessages { + let _ = sender.unbounded_send(NetworkEvent::NotificationsReceived { remote: sender_id.clone(), messages: vec![(GRANDPA_ENGINE_ID, commit_to_send.clone().into())], }); diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 8739ed3bc1fca..575ead3a9d614 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -82,6 +82,8 @@ impl GossipEngine { let inner = &mut *inner; inner.state_machine.tick(&mut *inner.context); } else { + // We reach this branch if the `Arc` has no reference + // left. We can now let the task end. break; } } @@ -97,7 +99,7 @@ impl GossipEngine { let mut stream = Compat01As03::new(event_stream); while let Some(Ok(event)) = stream.next().await { match event { - Event::NotifOpened { remote, engine_id: msg_engine_id } => { + Event::NotificationsStreamOpened { remote, engine_id: msg_engine_id } => { if msg_engine_id != engine_id { continue; } @@ -106,7 +108,7 @@ impl GossipEngine { // TODO: for now we hard-code the roles to FULL; fix that inner.state_machine.new_peer(&mut *inner.context, remote, Roles::FULL); } - Event::NotifClosed { remote, engine_id: msg_engine_id } => { + Event::NotificationsStreamClosed { remote, engine_id: msg_engine_id } => { if msg_engine_id != engine_id { continue; } @@ -114,7 +116,7 @@ impl GossipEngine { let inner = &mut *inner; inner.state_machine.peer_disconnected(&mut *inner.context, remote); }, - Event::NotifMessages { remote, messages } => { + Event::NotificationsReceived { remote, messages } => { let mut inner = inner.lock(); let inner = &mut *inner; inner.state_machine.on_incoming( @@ -150,7 +152,7 @@ impl GossipEngine { } /// Registers a message without propagating it to any peers. The message - /// becomes available to new peers or when the service is asked to GossipEngine + /// becomes available to new peers or when the service is asked to gossip /// the message's topic. No validation is performed on the message, if the /// message is already expired it should be dropped on the next garbage /// collection. @@ -174,7 +176,7 @@ impl GossipEngine { inner.state_machine.broadcast_topic(&mut *inner.context, topic, force); } - /// Get data of valid, incoming messages for a topic (but might have expired meanwhile) + /// Get data of valid, incoming messages for a topic (but might have expired meanwhile). pub fn messages_for(&self, topic: B::Hash) -> mpsc::UnboundedReceiver { diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index b7b04e1eceedd..998c79d23804c 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -137,22 +137,22 @@ Behaviour { self.events.push(BehaviourOut::JustificationImport(origin, hash, nb, justification)), CustomMessageOutcome::FinalityProofImport(origin, hash, nb, proof) => self.events.push(BehaviourOut::FinalityProofImport(origin, hash, nb, proof)), - CustomMessageOutcome::NotifOpened { remote, protocols } => + CustomMessageOutcome::NotificationsStreamOpened { remote, protocols } => for engine_id in protocols { - self.events.push(BehaviourOut::Event(Event::NotifOpened { + self.events.push(BehaviourOut::Event(Event::NotificationsStreamOpened { remote: remote.clone(), engine_id, })); }, - CustomMessageOutcome::NotifClosed { remote, protocols } => + CustomMessageOutcome::NotificationsStreamClosed { remote, protocols } => for engine_id in protocols { - self.events.push(BehaviourOut::Event(Event::NotifClosed { + self.events.push(BehaviourOut::Event(Event::NotificationsStreamClosed { remote: remote.clone(), engine_id, })); }, - CustomMessageOutcome::NotifMessages { remote, messages } => { - let ev = Event::NotifMessages { remote, messages }; + CustomMessageOutcome::NotificationsReceived { remote, messages } => { + let ev = Event::NotificationsReceived { remote, messages }; self.events.push(BehaviourOut::Event(ev)); }, CustomMessageOutcome::None => {} diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 014c95e21a8d8..49380fbb93edf 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -655,7 +655,7 @@ impl, H: ExHashT> Protocol { self.on_remote_read_child_request(who, request), GenericMessage::Consensus(msg) => return if self.registered_notif_protocols.contains(&msg.engine_id) { - CustomMessageOutcome::NotifMessages { + CustomMessageOutcome::NotificationsReceived { remote: who.clone(), messages: vec![(msg.engine_id, From::from(msg.data))], } @@ -677,7 +677,7 @@ impl, H: ExHashT> Protocol { .collect::>(); return if !messages.is_empty() { - CustomMessageOutcome::NotifMessages { + CustomMessageOutcome::NotificationsReceived { remote: who.clone(), messages, } @@ -1107,7 +1107,7 @@ impl, H: ExHashT> Protocol { // we handle it by notifying that we opened channels with everyone. self.behaviour.open_peers() .map(|peer| - event::Event::NotifOpened { + event::Event::NotificationsStreamOpened { remote: peer.clone(), engine_id, }) @@ -1784,11 +1784,11 @@ pub enum CustomMessageOutcome { JustificationImport(Origin, B::Hash, NumberFor, Justification), FinalityProofImport(Origin, B::Hash, NumberFor, Vec), /// Notifications protocols have been opened with a remote. - NotifOpened { remote: PeerId, protocols: Vec }, + NotificationsStreamOpened { remote: PeerId, protocols: Vec }, /// Notifications protocols have been closed with a remote. - NotifClosed { remote: PeerId, protocols: Vec }, + NotificationsStreamClosed { remote: PeerId, protocols: Vec }, /// Messages have been received on one or more notifications protocols. - NotifMessages { remote: PeerId, messages: Vec<(ConsensusEngineId, Bytes)> }, + NotificationsReceived { remote: PeerId, messages: Vec<(ConsensusEngineId, Bytes)> }, None, } @@ -1920,7 +1920,7 @@ Protocol { ); self.on_peer_connected(peer_id.clone()); // Notify all the notification protocols as open. - CustomMessageOutcome::NotifOpened { + CustomMessageOutcome::NotificationsStreamOpened { remote: peer_id, protocols: self.registered_notif_protocols.iter().cloned().collect(), } @@ -1928,7 +1928,7 @@ Protocol { LegacyProtoOut::CustomProtocolClosed { peer_id, .. } => { self.on_peer_disconnected(peer_id.clone()); // Notify all the notification protocols as closed. - CustomMessageOutcome::NotifClosed { + CustomMessageOutcome::NotificationsStreamClosed { remote: peer_id, protocols: self.registered_notif_protocols.iter().cloned().collect(), } diff --git a/client/network/src/protocol/event.rs b/client/network/src/protocol/event.rs index 68e13fbb190b8..9b2d0c8adf1d4 100644 --- a/client/network/src/protocol/event.rs +++ b/client/network/src/protocol/event.rs @@ -49,7 +49,7 @@ pub enum Event { /// Opened a substream with the given node with the given notifications protocol. /// /// The protocol is always one of the notification protocols that have been registered. - NotifOpened { + NotificationsStreamOpened { /// Node we opened the substream with. remote: PeerId, /// The concerned protocol. Each protocol uses a different substream. @@ -57,8 +57,8 @@ pub enum Event { }, /// Closed a substream with the given node. Always matches a corresponding previous - /// `NotifOpened` message. - NotifClosed { + /// `NotificationsStreamOpened` message. + NotificationsStreamClosed { /// Node we closed the substream with. remote: PeerId, /// The concerned protocol. Each protocol uses a different substream. @@ -66,7 +66,7 @@ pub enum Event { }, /// Received one or more messages from the given node using the given protocol. - NotifMessages { + NotificationsReceived { /// Node we received the message from. remote: PeerId, /// Concerned protocol and associated message. From 37f7b7c951c2b8163e9122188a0ceb9c1d667f67 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 4 Dec 2019 12:29:41 +0100 Subject: [PATCH 12/21] Remove network startup GP future --- client/finality-grandpa/src/communication/mod.rs | 7 ++----- client/finality-grandpa/src/communication/tests.rs | 4 ++-- client/finality-grandpa/src/lib.rs | 4 +--- client/finality-grandpa/src/observer.rs | 4 +--- client/finality-grandpa/src/tests.rs | 5 ++--- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index f5b7effb09b95..7282a2a8e93aa 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -128,10 +128,7 @@ impl NetworkBridge { set_state: crate::environment::SharedVoterSetState, executor: &impl futures03::task::Spawn, on_exit: impl Future + Clone + Send + 'static, - ) -> ( - Self, - impl Future + Send + 'static, - ) { + ) -> Self { let (validator, report_stream) = GossipValidator::new( config, set_state.clone(), @@ -189,7 +186,7 @@ impl NetworkBridge { executor.execute(Box::new(reporting_job.select(on_exit.clone()).then(|_| Ok(())))) .expect("failed to spawn grandpa reporting job task"); - (bridge, futures::future::ok(())) + bridge } /// Note the beginning of a new round to the `GossipValidator`. diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 572d3ec7eda8b..92457f46fc167 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -166,7 +166,7 @@ fn make_test_network(executor: &impl futures03::task::Spawn) -> ( } } - let (bridge, startup_work) = super::NetworkBridge::new( + let bridge = super::NetworkBridge::new( net.clone(), config(), voter_set_state(), @@ -175,7 +175,7 @@ fn make_test_network(executor: &impl futures03::task::Spawn) -> ( ); ( - startup_work.map(move |()| Tester { + futures::future::ok(Tester { gossip_validator: bridge.validator.clone(), net_handle: bridge, events: rx, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 7b7710db292ab..9f411f0c0c834 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -575,7 +575,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X, Sp>( voter_commands_rx, } = link; - let (network, network_startup) = NetworkBridge::new( + let network = NetworkBridge::new( network, config.clone(), persistent_data.set_state.clone(), @@ -630,8 +630,6 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X, Sp>( telemetry!(CONSENSUS_WARN; "afg.voter_failed"; "e" => ?e); }); - let voter_work = network_startup.and_then(move |()| voter_work); - // Make sure that `telemetry_task` doesn't accidentally finish and kill grandpa. let telemetry_task = telemetry_task .then(|_| futures::future::empty::<(), ()>()); diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 96a020f410828..91cb263d8abba 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -173,7 +173,7 @@ pub fn run_grandpa_observer, N, RA, SC, Sp>( voter_commands_rx, } = link; - let (network, network_startup) = NetworkBridge::new( + let network = NetworkBridge::new( network, config.clone(), persistent_data.set_state.clone(), @@ -195,8 +195,6 @@ pub fn run_grandpa_observer, N, RA, SC, Sp>( warn!("GRANDPA Observer failed: {:?}", e); }); - let observer_work = network_startup.and_then(move |()| observer_work); - Ok(observer_work.select(on_exit).map(|_| ()).map_err(|_| ())) } diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 9ef1f40214723..311dae7e0b893 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1214,14 +1214,13 @@ fn voter_persists_its_votes() { set_state }; - let (network, routing_work) = communication::NetworkBridge::new( + let network = communication::NetworkBridge::new( net.lock().peers[1].network_service().clone(), config.clone(), set_state, &threads_pool, Exit, ); - runtime.block_on(routing_work).unwrap(); let (round_rx, round_tx) = network.round_communication( communication::Round(1), @@ -1613,7 +1612,7 @@ fn grandpa_environment_respects_voting_rules() { observer_enabled: true, }; - let (network, _) = NetworkBridge::new( + let network = NetworkBridge::new( network_service.clone(), config.clone(), set_state.clone(), From 00b82a5c12f31730d97de1a9c981d39324077b21 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 4 Dec 2019 14:11:26 +0100 Subject: [PATCH 13/21] Update to futures_timer --- Cargo.lock | 3 +-- client/finality-grandpa/Cargo.toml | 3 +-- .../src/communication/periodic.rs | 7 +++--- client/finality-grandpa/src/environment.rs | 18 +++++++-------- client/finality-grandpa/src/lib.rs | 4 ++-- client/finality-grandpa/src/tests.rs | 10 ++++++++- client/finality-grandpa/src/until_imported.rs | 22 +++++++++++-------- 7 files changed, 38 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b72a6e72fcb19..856708102ca02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5239,6 +5239,7 @@ dependencies = [ "fork-tree 2.0.0", "futures 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)", "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", + "futures-timer 2.0.2 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "parity-scale-codec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", "parking_lot 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -5264,8 +5265,6 @@ dependencies = [ "substrate-test-runtime-client 2.0.0", "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "tokio 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", - "tokio-executor 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", - "tokio-timer 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index d940bd67d9cdf..f719414ba551b 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -8,10 +8,9 @@ edition = "2018" fork-tree = { path = "../../utils/fork-tree" } futures = "0.1.29" futures03 = { package = "futures", version = "0.3.1", features = ["compat"] } +futures-timer = "2.0.2" log = "0.4.8" parking_lot = "0.9.0" -tokio-executor = "0.1.8" -tokio-timer = "0.2.11" rand = "0.7.2" codec = { package = "parity-scale-codec", version = "1.0.0", features = ["derive"] } sp-runtime = { path = "../../primitives/sr-primitives" } diff --git a/client/finality-grandpa/src/communication/periodic.rs b/client/finality-grandpa/src/communication/periodic.rs index b52dc42b0b664..3f9cc0dd8eb8c 100644 --- a/client/finality-grandpa/src/communication/periodic.rs +++ b/client/finality-grandpa/src/communication/periodic.rs @@ -21,8 +21,9 @@ use std::time::{Instant, Duration}; use codec::Encode; use futures::prelude::*; use futures::sync::mpsc; +use futures_timer::Delay; +use futures03::future::{FutureExt as _, TryFutureExt as _}; use log::{debug, warn}; -use tokio_timer::Delay; use network::PeerId; use network_gossip::GossipEngine; @@ -67,7 +68,7 @@ pub(super) fn neighbor_packet_worker(net: GossipEngine) -> ( { let mut last = None; let (tx, mut rx) = mpsc::unbounded::<(Vec, NeighborPacket>)>(); - let mut delay = Delay::new(rebroadcast_instant()); + let mut delay = Delay::new(REBROADCAST_AFTER); let work = futures::future::poll_fn(move || { loop { @@ -88,7 +89,7 @@ pub(super) fn neighbor_packet_worker(net: GossipEngine) -> ( // has to be done in a loop because it needs to be polled after // re-scheduling. loop { - match delay.poll() { + match (&mut delay).unit_error().compat().poll() { Err(e) => { warn!(target: "afg", "Could not rebroadcast neighbor packets: {:?}", e); delay.reset(rebroadcast_instant()); diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 19637277c6932..a25266848137d 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -17,12 +17,13 @@ use std::collections::BTreeMap; use std::iter::FromIterator; use std::sync::Arc; -use std::time::{Duration, Instant}; +use std::time::Duration; use log::{debug, warn, info}; use codec::{Decode, Encode}; use futures::prelude::*; -use tokio_timer::Delay; +use futures03::future::{FutureExt as _, TryFutureExt as _}; +use futures_timer::Delay; use parking_lot::RwLock; use sp_blockchain::{HeaderBackend, Error as ClientError}; @@ -585,9 +586,8 @@ where &self, round: RoundNumber, ) -> voter::RoundData { - let now = Instant::now(); - let prevote_timer = Delay::new(now + self.config.gossip_duration * 2); - let precommit_timer = Delay::new(now + self.config.gossip_duration * 4); + let prevote_timer = Delay::new(self.config.gossip_duration * 2); + let precommit_timer = Delay::new(self.config.gossip_duration * 4); let local_key = crate::is_voter(&self.voters, &self.config.keystore); @@ -625,8 +625,8 @@ where voter::RoundData { voter_id: local_key.map(|pair| pair.public()), - prevote_timer: Box::new(prevote_timer.map_err(|e| Error::Timer(e).into())), - precommit_timer: Box::new(precommit_timer.map_err(|e| Error::Timer(e).into())), + prevote_timer: Box::new(prevote_timer.map(Ok).compat()), + precommit_timer: Box::new(precommit_timer.map(Ok).compat()), incoming, outgoing, } @@ -900,9 +900,7 @@ where //random between 0-1 seconds. let delay: u64 = thread_rng().gen_range(0, 1000); - Box::new(Delay::new( - Instant::now() + Duration::from_millis(delay) - ).map_err(|e| Error::Timer(e).into())) + Box::new(Delay::new(Duration::from_millis(delay)).map(Ok).compat()) } fn prevote_equivocation( diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 9f411f0c0c834..5cc2e63c0d8f3 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -73,7 +73,7 @@ use sp_finality_tracker; use grandpa::Error as GrandpaError; use grandpa::{voter, BlockNumberOps, voter_set::VoterSet}; -use std::fmt; +use std::{fmt, io}; use std::sync::Arc; use std::time::Duration; @@ -230,7 +230,7 @@ pub enum Error { /// An invariant has been violated (e.g. not finalizing pending change blocks in-order) Safety(String), /// A timer failed to fire. - Timer(tokio_timer::Error), + Timer(io::Error), } impl From for Error { diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 311dae7e0b893..67f98982d13db 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -22,6 +22,7 @@ use network::test::{Block, DummySpecialization, Hash, TestNetFactory, Peer, Peer use network::test::{PassThroughVerifier}; use network::config::{ProtocolConfig, Roles, BoxFinalityProofRequestBuilder}; use parking_lot::Mutex; +use futures_timer::Delay; use futures03::{StreamExt as _, TryStreamExt as _}; use tokio::runtime::current_thread; use keyring::Ed25519Keyring; @@ -1255,7 +1256,14 @@ fn voter_persists_its_votes() { let net = net.clone(); let voter_tx = voter_tx.clone(); let round_tx = round_tx.clone(); - future::Either::A(tokio_timer::Interval::new_interval(Duration::from_millis(200)) + + let interval = futures03::stream::unfold(Delay::new(Duration::from_millis(200)), |delay| + Box::pin(async move { + delay.await; + Some(((), Delay::new(Duration::from_millis(200)))) + })).map(Ok::<_, ()>).compat(); + + future::Either::A(interval .take_while(move |_| { Ok(net2.lock().peer(1).client().info().chain.best_number != 40) }) diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index 7e209e13b8ea2..c843547a7bbbe 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -32,11 +32,11 @@ use log::{debug, warn}; use client_api::{BlockImportNotification, ImportNotifications}; use futures::prelude::*; use futures::stream::Fuse; +use futures_timer::Delay; use futures03::{StreamExt as _, TryStreamExt as _}; use grandpa::voter; use parking_lot::Mutex; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; -use tokio_timer::Interval; use std::collections::{HashMap, VecDeque}; use std::sync::{atomic::{AtomicUsize, Ordering}, Arc}; @@ -76,7 +76,7 @@ pub(crate) struct UntilImported, ready: VecDeque, - check_pending: Interval, + check_pending: Box + Send>, /// Mapping block hashes to their block number, the point in time it was /// first encountered (Instant) and a list of GRANDPA messages referencing /// the block hash. @@ -104,9 +104,13 @@ impl UntilImported _>(|v| Ok::<_, ()>(v)).compat(); @@ -116,7 +120,7 @@ impl UntilImported panic!("neither should have had error"), Ok(Either::A(_)) => panic!("timeout should have fired first"), @@ -929,7 +933,7 @@ mod tests { // the `until_imported` stream doesn't request the blocks immediately, // but it should request them after a small timeout - let timeout = Delay::new(Instant::now() + Duration::from_secs(60)); + let timeout = Delay::new(Duration::from_secs(60)).unit_error().compat(); let test = assert.select2(timeout).map(|res| match res { Either::A(_) => {}, Either::B(_) => panic!("timed out waiting for block sync request"), From 9e31024412c059335bd3c3b35ed4691cda4655db Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 4 Dec 2019 17:09:42 +0100 Subject: [PATCH 14/21] adjust wait_when_behind test --- client/finality-grandpa/src/communication/gossip.rs | 13 +++++-------- client/finality-grandpa/src/tests.rs | 8 ++++++-- client/finality-grandpa/src/voting_rule.rs | 3 ++- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index a29af013be1b3..5142c5e612253 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -109,7 +109,6 @@ const CATCH_UP_THRESHOLD: u64 = 2; const PROPAGATION_ALL: u32 = 4; //in rounds; const PROPAGATION_ALL_AUTHORITIES: u32 = 2; //in rounds; -const PROPAGATION_SOME_NON_AUTHORITIES: u32 = 3; //in rounds; const ROUND_DURATION: u32 = 4; // measured in gossip durations const MIN_LUCKY: usize = 5; @@ -1097,14 +1096,12 @@ impl Inner { } else { // the node is not an authority so we apply stricter filters if round_elapsed >= round_duration * PROPAGATION_ALL { - // if we waited for 3 (or more) rounds + // if we waited for enough // then it is allowed to be sent to all peers. true - } else if round_elapsed >= round_duration * PROPAGATION_SOME_NON_AUTHORITIES { + } else { // otherwise we only send it to `sqrt(non-authorities)`. self.peers.lucky_peers.contains(who) - } else { - false } } } @@ -1146,11 +1143,11 @@ impl Inner { } else { let non_authorities = self.peers.non_authorities(); - // the target node is not an authority, on the first and second - // round duration we start by sending the message to only + // the target node is not an authority. on the first few + // round durations we start by sending the message to only // `sqrt(non_authorities)` (if we're connected to at least // `MIN_LUCKY`). - if round_elapsed < round_duration * PROPAGATION_SOME_NON_AUTHORITIES + if round_elapsed < round_duration * PROPAGATION_ALL && non_authorities > MIN_LUCKY { self.peers.lucky_peers.contains(who) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 67f98982d13db..4b3717e718ac3 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1503,7 +1503,7 @@ fn voter_catches_up_to_latest_round_when_behind() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, - voting_rule: (), + voting_rule: voting_rule::ThreeQuartersOfTheUnfinalizedChain, executor: threads_pool.clone(), }; @@ -1523,11 +1523,16 @@ fn voter_catches_up_to_latest_round_when_behind() { ) }; + let set_state = link.persistent_data.set_state.clone(); + finality_notifications.push( client.finality_notification_stream() .map(|v| Ok::<_, ()>(v)).compat() .take_while(|n| Ok(n.header.number() < &50)) .for_each(move |_| Ok(())) + .map(move |_| + assert!(set_state.read().last_completed_round().number >= 4) + ) ); let (keystore, keystore_path) = create_keystore(*key); @@ -1541,7 +1546,6 @@ fn voter_catches_up_to_latest_round_when_behind() { // wait for them to finalize block 50. since they'll vote on 3/4 of the // unfinalized chain it will take at least 4 rounds to do it. let wait_for_finality = ::futures::future::join_all(finality_notifications) - .map(|_| ()) .map_err(|_| ()); // spawn a new voter, it should be behind by at least 4 rounds and should be diff --git a/client/finality-grandpa/src/voting_rule.rs b/client/finality-grandpa/src/voting_rule.rs index cfa28d03fedef..bf3bfbf3216fe 100644 --- a/client/finality-grandpa/src/voting_rule.rs +++ b/client/finality-grandpa/src/voting_rule.rs @@ -69,7 +69,7 @@ impl VotingRule for () where /// A custom voting rule that guarantees that our vote is always behind the best /// block, in the best case exactly one block behind it. -#[derive(Clone)] +#[derive(Clone, Copy, Debug)] pub struct BeforeBestBlock; impl VotingRule for BeforeBestBlock where Block: BlockT, @@ -100,6 +100,7 @@ impl VotingRule for BeforeBestBlock where /// A custom voting rule that limits votes towards 3/4 of the unfinalized chain, /// using the given `base` and `best_target` to figure where the 3/4 target /// should fall. +#[derive(Clone, Copy, Debug)] pub struct ThreeQuartersOfTheUnfinalizedChain; impl VotingRule for ThreeQuartersOfTheUnfinalizedChain where From eac9548345a060edc79d3e68a9bfb948e686619d Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Thu, 5 Dec 2019 10:53:47 +0100 Subject: [PATCH 15/21] Pass correct Roles after handshake --- .../src/communication/tests.rs | 4 +- client/network-gossip/src/bridge.rs | 7 ++-- client/network/src/behaviour.rs | 3 +- client/network/src/protocol.rs | 41 +++++++++++-------- client/network/src/protocol/event.rs | 3 ++ 5 files changed, 34 insertions(+), 24 deletions(-) diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 92457f46fc167..f121b46f0663a 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -18,7 +18,7 @@ use futures::sync::mpsc; use futures::prelude::*; -use network::{Event as NetworkEvent, PeerId, test::{Block, Hash}}; +use network::{Event as NetworkEvent, PeerId, config::Roles, test::{Block, Hash}}; use network_gossip::Validator; use tokio::runtime::current_thread; use std::sync::Arc; @@ -275,6 +275,7 @@ fn good_commit_leads_to_relay() { let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, + roles: Roles::FULL, }); let _ = sender.unbounded_send(NetworkEvent::NotificationsReceived { remote: sender_id.clone(), @@ -391,6 +392,7 @@ fn bad_commit_leads_to_report() { let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, + roles: Roles::FULL, }); let _ = sender.unbounded_send(NetworkEvent::NotificationsReceived { remote: sender_id.clone(), diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 575ead3a9d614..3dc6bddc3273b 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -19,7 +19,7 @@ use crate::state_machine::{ConsensusGossip, Validator, TopicNotification}; use network::Context; use network::message::generic::ConsensusMessage; -use network::{Event, config::Roles, ReputationChange}; +use network::{Event, ReputationChange}; use futures::{prelude::*, channel::mpsc, compat::Compat01As03, task::SpawnExt as _}; use libp2p::PeerId; @@ -99,14 +99,13 @@ impl GossipEngine { let mut stream = Compat01As03::new(event_stream); while let Some(Ok(event)) = stream.next().await { match event { - Event::NotificationsStreamOpened { remote, engine_id: msg_engine_id } => { + Event::NotificationsStreamOpened { remote, engine_id: msg_engine_id, roles } => { if msg_engine_id != engine_id { continue; } let mut inner = inner.lock(); let inner = &mut *inner; - // TODO: for now we hard-code the roles to FULL; fix that - inner.state_machine.new_peer(&mut *inner.context, remote, Roles::FULL); + inner.state_machine.new_peer(&mut *inner.context, remote, roles); } Event::NotificationsStreamClosed { remote, engine_id: msg_engine_id } => { if msg_engine_id != engine_id { diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index 998c79d23804c..9060fd6585d12 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -137,11 +137,12 @@ Behaviour { self.events.push(BehaviourOut::JustificationImport(origin, hash, nb, justification)), CustomMessageOutcome::FinalityProofImport(origin, hash, nb, proof) => self.events.push(BehaviourOut::FinalityProofImport(origin, hash, nb, proof)), - CustomMessageOutcome::NotificationsStreamOpened { remote, protocols } => + CustomMessageOutcome::NotificationsStreamOpened { remote, protocols, roles } => for engine_id in protocols { self.events.push(BehaviourOut::Event(Event::NotificationsStreamOpened { remote: remote.clone(), engine_id, + roles, })); }, CustomMessageOutcome::NotificationsStreamClosed { remote, protocols } => diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 49380fbb93edf..50437c8b83631 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -611,7 +611,7 @@ impl, H: ExHashT> Protocol { stats.count_in += 1; match message { - GenericMessage::Status(s) => self.on_status_message(who, s), + GenericMessage::Status(s) => return self.on_status_message(who, s), GenericMessage::BlockRequest(r) => self.on_block_request(who, r), GenericMessage::BlockResponse(r) => { // Note, this is safe because only `ordinary bodies` and `remote bodies` are received in this matter. @@ -956,7 +956,7 @@ impl, H: ExHashT> Protocol { } /// Called by peer to report status - fn on_status_message(&mut self, who: PeerId, status: message::Status) { + fn on_status_message(&mut self, who: PeerId, status: message::Status) -> CustomMessageOutcome { trace!(target: "sync", "New peer {} {:?}", who, status); let _protocol_version = { if self.context_data.peers.contains_key(&who) { @@ -966,7 +966,7 @@ impl, H: ExHashT> Protocol { "Unexpected status packet from {}", who ); self.peerset_handle.report_peer(who, rep::UNEXPECTED_STATUS); - return; + return CustomMessageOutcome::None; } if status.genesis_hash != self.genesis_hash { log!( @@ -977,7 +977,7 @@ impl, H: ExHashT> Protocol { ); self.peerset_handle.report_peer(who.clone(), rep::GENESIS_MISMATCH); self.behaviour.disconnect_peer(&who); - return; + return CustomMessageOutcome::None; } if status.version < MIN_VERSION && CURRENT_VERSION < status.min_supported_version { log!( @@ -987,7 +987,7 @@ impl, H: ExHashT> Protocol { ); self.peerset_handle.report_peer(who.clone(), rep::BAD_PROTOCOL); self.behaviour.disconnect_peer(&who); - return; + return CustomMessageOutcome::None; } if self.config.roles.is_light() { @@ -996,7 +996,7 @@ impl, H: ExHashT> Protocol { debug!(target: "sync", "Peer {} is unable to serve light requests", who); self.peerset_handle.report_peer(who.clone(), rep::BAD_ROLE); self.behaviour.disconnect_peer(&who); - return; + return CustomMessageOutcome::None; } // we don't interested in peers that are far behind us @@ -1013,7 +1013,7 @@ impl, H: ExHashT> Protocol { debug!(target: "sync", "Peer {} is far behind us and will unable to serve light requests", who); self.peerset_handle.report_peer(who.clone(), rep::PEER_BEHIND_US_LIGHT); self.behaviour.disconnect_peer(&who); - return; + return CustomMessageOutcome::None; } } @@ -1028,7 +1028,7 @@ impl, H: ExHashT> Protocol { }, None => { error!(target: "sync", "Received status from previously unconnected node {}", who); - return; + return CustomMessageOutcome::None; }, }; @@ -1063,8 +1063,16 @@ impl, H: ExHashT> Protocol { } } } + let mut context = ProtocolContext::new(&mut self.context_data, &mut self.behaviour, &self.peerset_handle); - self.specialization.on_connect(&mut context, who, status); + self.specialization.on_connect(&mut context, who.clone(), status); + + // Notify all the notification protocols as open. + CustomMessageOutcome::NotificationsStreamOpened { + remote: who, + protocols: self.registered_notif_protocols.iter().cloned().collect(), + roles: info.roles, + } } /// Send a notification to the given peer we're connected to. @@ -1105,11 +1113,12 @@ impl, H: ExHashT> Protocol { // Registering a protocol while we already have open connections isn't great, but for now // we handle it by notifying that we opened channels with everyone. - self.behaviour.open_peers() - .map(|peer| + self.context_data.peers.iter() + .map(|(peer_id, peer)| event::Event::NotificationsStreamOpened { - remote: peer.clone(), + remote: peer_id.clone(), engine_id, + roles: peer.info.roles, }) .collect() } @@ -1784,7 +1793,7 @@ pub enum CustomMessageOutcome { JustificationImport(Origin, B::Hash, NumberFor, Justification), FinalityProofImport(Origin, B::Hash, NumberFor, Vec), /// Notifications protocols have been opened with a remote. - NotificationsStreamOpened { remote: PeerId, protocols: Vec }, + NotificationsStreamOpened { remote: PeerId, protocols: Vec, roles: Roles }, /// Notifications protocols have been closed with a remote. NotificationsStreamClosed { remote: PeerId, protocols: Vec }, /// Messages have been received on one or more notifications protocols. @@ -1919,11 +1928,7 @@ Protocol { && version >= MIN_VERSION as u8 ); self.on_peer_connected(peer_id.clone()); - // Notify all the notification protocols as open. - CustomMessageOutcome::NotificationsStreamOpened { - remote: peer_id, - protocols: self.registered_notif_protocols.iter().cloned().collect(), - } + CustomMessageOutcome::None } LegacyProtoOut::CustomProtocolClosed { peer_id, .. } => { self.on_peer_disconnected(peer_id.clone()); diff --git a/client/network/src/protocol/event.rs b/client/network/src/protocol/event.rs index 9b2d0c8adf1d4..961f898e04d5f 100644 --- a/client/network/src/protocol/event.rs +++ b/client/network/src/protocol/event.rs @@ -17,6 +17,7 @@ //! Network event types. These are are not the part of the protocol, but rather //! events that happen on the network like DHT get/put results received. +use crate::config::Roles; use bytes::Bytes; use libp2p::core::PeerId; use libp2p::kad::record::Key; @@ -54,6 +55,8 @@ pub enum Event { remote: PeerId, /// The concerned protocol. Each protocol uses a different substream. engine_id: ConsensusEngineId, + /// Roles that the remote . + roles: Roles, }, /// Closed a substream with the given node. Always matches a corresponding previous From 715462ba9ab6c600678a087d6ac2ea1569802a9e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 6 Dec 2019 12:09:48 +0100 Subject: [PATCH 16/21] Revert "adjust wait_when_behind test" This reverts commit 9e31024412c059335bd3c3b35ed4691cda4655db. --- client/finality-grandpa/src/communication/gossip.rs | 13 ++++++++----- client/finality-grandpa/src/tests.rs | 8 ++------ client/finality-grandpa/src/voting_rule.rs | 3 +-- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index 5142c5e612253..a29af013be1b3 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -109,6 +109,7 @@ const CATCH_UP_THRESHOLD: u64 = 2; const PROPAGATION_ALL: u32 = 4; //in rounds; const PROPAGATION_ALL_AUTHORITIES: u32 = 2; //in rounds; +const PROPAGATION_SOME_NON_AUTHORITIES: u32 = 3; //in rounds; const ROUND_DURATION: u32 = 4; // measured in gossip durations const MIN_LUCKY: usize = 5; @@ -1096,12 +1097,14 @@ impl Inner { } else { // the node is not an authority so we apply stricter filters if round_elapsed >= round_duration * PROPAGATION_ALL { - // if we waited for enough + // if we waited for 3 (or more) rounds // then it is allowed to be sent to all peers. true - } else { + } else if round_elapsed >= round_duration * PROPAGATION_SOME_NON_AUTHORITIES { // otherwise we only send it to `sqrt(non-authorities)`. self.peers.lucky_peers.contains(who) + } else { + false } } } @@ -1143,11 +1146,11 @@ impl Inner { } else { let non_authorities = self.peers.non_authorities(); - // the target node is not an authority. on the first few - // round durations we start by sending the message to only + // the target node is not an authority, on the first and second + // round duration we start by sending the message to only // `sqrt(non_authorities)` (if we're connected to at least // `MIN_LUCKY`). - if round_elapsed < round_duration * PROPAGATION_ALL + if round_elapsed < round_duration * PROPAGATION_SOME_NON_AUTHORITIES && non_authorities > MIN_LUCKY { self.peers.lucky_peers.contains(who) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index da39d1e3714c9..499640c7611cd 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1503,7 +1503,7 @@ fn voter_catches_up_to_latest_round_when_behind() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, - voting_rule: voting_rule::ThreeQuartersOfTheUnfinalizedChain, + voting_rule: (), executor: threads_pool.clone(), }; @@ -1523,16 +1523,11 @@ fn voter_catches_up_to_latest_round_when_behind() { ) }; - let set_state = link.persistent_data.set_state.clone(); - finality_notifications.push( client.finality_notification_stream() .map(|v| Ok::<_, ()>(v)).compat() .take_while(|n| Ok(n.header.number() < &50)) .for_each(move |_| Ok(())) - .map(move |_| - assert!(set_state.read().last_completed_round().number >= 4) - ) ); let (keystore, keystore_path) = create_keystore(*key); @@ -1546,6 +1541,7 @@ fn voter_catches_up_to_latest_round_when_behind() { // wait for them to finalize block 50. since they'll vote on 3/4 of the // unfinalized chain it will take at least 4 rounds to do it. let wait_for_finality = ::futures::future::join_all(finality_notifications) + .map(|_| ()) .map_err(|_| ()); // spawn a new voter, it should be behind by at least 4 rounds and should be diff --git a/client/finality-grandpa/src/voting_rule.rs b/client/finality-grandpa/src/voting_rule.rs index bf3bfbf3216fe..cfa28d03fedef 100644 --- a/client/finality-grandpa/src/voting_rule.rs +++ b/client/finality-grandpa/src/voting_rule.rs @@ -69,7 +69,7 @@ impl VotingRule for () where /// A custom voting rule that guarantees that our vote is always behind the best /// block, in the best case exactly one block behind it. -#[derive(Clone, Copy, Debug)] +#[derive(Clone)] pub struct BeforeBestBlock; impl VotingRule for BeforeBestBlock where Block: BlockT, @@ -100,7 +100,6 @@ impl VotingRule for BeforeBestBlock where /// A custom voting rule that limits votes towards 3/4 of the unfinalized chain, /// using the given `base` and `best_target` to figure where the 3/4 target /// should fall. -#[derive(Clone, Copy, Debug)] pub struct ThreeQuartersOfTheUnfinalizedChain; impl VotingRule for ThreeQuartersOfTheUnfinalizedChain where From 1f0a5581e4f0392ef50311ea129ad6a2d9032a5c Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 6 Dec 2019 12:52:17 +0100 Subject: [PATCH 17/21] Crate root documentation --- client/network-gossip/src/bridge.rs | 3 +- client/network-gossip/src/lib.rs | 40 ++++++++++++++++++++++ client/network-gossip/src/state_machine.rs | 29 ---------------- 3 files changed, 42 insertions(+), 30 deletions(-) diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 3dc6bddc3273b..8a115d801d1a6 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -27,6 +27,8 @@ use parking_lot::Mutex; use sp_runtime::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; use std::{sync::Arc, time::Duration}; +/// Wraps around an implementation of the `Network` crate and provides gossiping capabilities on +/// top of it. pub struct GossipEngine { inner: Arc>>, engine_id: ConsensusEngineId, @@ -270,7 +272,6 @@ impl> Context for ContextOverService { } fn send_consensus(&mut self, who: PeerId, messages: Vec) { - // TODO: send batch for message in messages { self.network.write_notification(who.clone(), message.engine_id, message.data); } diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 6ea949502b4e6..7bbaa5c6b6f66 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -14,6 +14,46 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . +//! Polite gossiping. +//! +//! This crate provides gossiping capabilities on top of a network. +//! +//! Gossip messages are separated by two categories: "topics" and consensus engine ID. +//! The consensus engine ID is sent over the wire with the message, while the topic is not, +//! with the expectation that the topic can be derived implicitly from the content of the +//! message, assuming it is valid. +//! +//! Topics are a single 32-byte tag associated with a message, used to group those messages +//! in an opaque way. Consensus code can invoke `broadcast_topic` to attempt to send all messages +//! under a single topic to all peers who don't have them yet, and `send_topic` to +//! send all messages under a single topic to a specific peer. +//! +//! # Usage +//! +//! - Implement the `Network` trait, representing the low-level networking primitives. It is +//! already implemented on `sc_network::NetworkService`. +//! - Implement the `Validator` trait. See the section below. +//! - Decide on a `ConsensusEngineId`. Each gossiping protocol should have a different one. +//! - Build a `GossipEngine` using these three elements. +//! - Use the methods of the `GossipEngine` in order to send out messages and receive incoming +//! messages. +//! +//! # What is a validator? +//! +//! The primary role of a `Validator` is to process incoming messages from peers, and decide +//! whether to discard them or process them. It also decides whether to re-broadcast the message. +//! +//! The secondary role of the `Validator` is to check if a message is allowed to be sent to a given +//! peer. All messages, before being sent, will be checked against this filter. +//! This enables the validator to use information it's aware of about connected peers to decide +//! whether to send messages to them at any given moment in time - In particular, to wait until +//! peers can accept and process the message before sending it. +//! +//! Lastly, the fact that gossip validators can decide not to rebroadcast messages +//! opens the door for neighbor status packets to be baked into the gossip protocol. +//! These status packets will typically contain light pieces of information +//! used to inform peers of a current view of protocol state. + pub use self::bridge::GossipEngine; pub use self::state_machine::{TopicNotification, MessageRecipient, MessageIntent}; pub use self::state_machine::{Validator, ValidatorContext, ValidationResult}; diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 68307f986d7c6..384ef2712b87f 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -14,35 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -//! Utility for gossip of network messages between nodes. -//! Handles chain-specific and standard BFT messages. -//! -//! Gossip messages are separated by two categories: "topics" and consensus engine ID. -//! The consensus engine ID is sent over the wire with the message, while the topic is not, -//! with the expectation that the topic can be derived implicitly from the content of the -//! message, assuming it is valid. -//! -//! Topics are a single 32-byte tag associated with a message, used to group those messages -//! in an opaque way. Consensus code can invoke `broadcast_topic` to attempt to send all messages -//! under a single topic to all peers who don't have them yet, and `send_topic` to -//! send all messages under a single topic to a specific peer. -//! -//! Each consensus engine ID must have an associated, -//! registered `Validator` for all gossip messages. The primary role of this `Validator` is -//! to process incoming messages from peers, and decide whether to discard them or process -//! them. It also decides whether to re-broadcast the message. -//! -//! The secondary role of the `Validator` is to check if a message is allowed to be sent to a given -//! peer. All messages, before being sent, will be checked against this filter. -//! This enables the validator to use information it's aware of about connected peers to decide -//! whether to send messages to them at any given moment in time - In particular, to wait until -//! peers can accept and process the message before sending it. -//! -//! Lastly, the fact that gossip validators can decide not to rebroadcast messages -//! opens the door for neighbor status packets to be baked into the gossip protocol. -//! These status packets will typically contain light pieces of information -//! used to inform peers of a current view of protocol state. - use std::collections::{HashMap, HashSet, hash_map::Entry}; use std::sync::Arc; use std::iter; From e6a06adae4e4f9dccfb4a361e2abb91a673a5a5e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Fri, 6 Dec 2019 16:10:11 +0100 Subject: [PATCH 18/21] Remove MessageRecipient --- client/network-gossip/src/lib.rs | 2 +- client/network-gossip/src/state_machine.rs | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 7bbaa5c6b6f66..6decda05c5142 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -55,7 +55,7 @@ //! used to inform peers of a current view of protocol state. pub use self::bridge::GossipEngine; -pub use self::state_machine::{TopicNotification, MessageRecipient, MessageIntent}; +pub use self::state_machine::{TopicNotification, MessageIntent}; pub use self::state_machine::{Validator, ValidatorContext, ValidationResult}; pub use self::state_machine::DiscardAll; diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 384ef2712b87f..48854fc2a8b82 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -67,16 +67,6 @@ struct MessageEntry { sender: Option, } -/// Consensus message destination. -pub enum MessageRecipient { - /// Send to all peers. - BroadcastToAll, - /// Send to peers that don't have that message already. - BroadcastNew, - /// Send to specific peer. - Peer(PeerId), -} - /// The reason for sending out the message. #[derive(Eq, PartialEq, Copy, Clone)] #[cfg_attr(test, derive(Debug))] From 76437bcecc194f667b4109d723ef4eaf7a6116da Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Mon, 9 Dec 2019 17:12:41 +0100 Subject: [PATCH 19/21] Address concerns --- .../finality-grandpa/src/communication/mod.rs | 54 +++++++++---------- .../src/communication/tests.rs | 4 +- client/network-gossip/src/bridge.rs | 2 +- client/network/src/behaviour.rs | 4 +- client/network/src/protocol.rs | 10 ++-- client/network/src/protocol/event.rs | 4 +- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 9b00eaf198263..d15f98326087c 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -112,18 +112,18 @@ pub(crate) fn register_dummy_protocol>(network: N) { /// Bridge between the underlying network service, gossiping consensus messages and Grandpa pub(crate) struct NetworkBridge { - service: GossipEngine, + gossip_engine: GossipEngine, validator: Arc>, neighbor_sender: periodic::NeighborPacketSender, } impl NetworkBridge { /// Create a new NetworkBridge to the given NetworkService. Returns the service - /// handle and a future that must be polled to completion to finish startup. + /// handle. /// On creation it will register previous rounds' votes with the gossip /// service taken from the VoterSetState. pub(crate) fn new + Clone + Send + 'static>( - service: N, + gossip_engine: N, config: crate::Config, set_state: crate::environment::SharedVoterSetState, executor: &impl futures03::task::Spawn, @@ -135,7 +135,7 @@ impl NetworkBridge { ); let validator = Arc::new(validator); - let service = GossipEngine::new(service, executor, GRANDPA_ENGINE_ID, validator.clone()); + let gossip_engine = GossipEngine::new(gossip_engine, executor, GRANDPA_ENGINE_ID, validator.clone()); { // register all previous votes with the gossip service so that they're @@ -159,7 +159,7 @@ impl NetworkBridge { } ); - service.register_gossip_message( + gossip_engine.register_gossip_message( topic, message.encode(), ); @@ -175,10 +175,10 @@ impl NetworkBridge { } } - let (rebroadcast_job, neighbor_sender) = periodic::neighbor_packet_worker(service.clone()); - let reporting_job = report_stream.consume(service.clone()); + let (rebroadcast_job, neighbor_sender) = periodic::neighbor_packet_worker(gossip_engine.clone()); + let reporting_job = report_stream.consume(gossip_engine.clone()); - let bridge = NetworkBridge { service, validator, neighbor_sender }; + let bridge = NetworkBridge { gossip_engine, validator, neighbor_sender }; let executor = Compat::new(executor); executor.execute(Box::new(rebroadcast_job.select(on_exit.clone().map(Ok).compat()).then(|_| Ok(())))) @@ -238,7 +238,7 @@ impl NetworkBridge { }); let topic = round_topic::(round.0, set_id.0); - let incoming = Compat::new(self.service.messages_for(topic) + let incoming = Compat::new(self.gossip_engine.messages_for(topic) .map(|item| Ok::<_, ()>(item))) .filter_map(|notification| { let decoded = GossipMessage::::decode(&mut ¬ification.message[..]); @@ -295,7 +295,7 @@ impl NetworkBridge { let outgoing = OutgoingMessages:: { round: round.0, set_id: set_id.0, - network: self.service.clone(), + network: self.gossip_engine.clone(), locals, sender: tx, has_voted, @@ -329,7 +329,7 @@ impl NetworkBridge { |to, neighbor| self.neighbor_sender.send(to, neighbor), ); - let service = self.service.clone(); + let service = self.gossip_engine.clone(); let topic = global_topic::(set_id.0); let incoming = incoming_global( service, @@ -340,7 +340,7 @@ impl NetworkBridge { ); let outgoing = CommitsOut::::new( - self.service.clone(), + self.gossip_engine.clone(), set_id.0, is_voter, self.validator.clone(), @@ -362,12 +362,12 @@ impl NetworkBridge { /// should make a best effort to fetch the block from any peers it is /// connected to (NOTE: this assumption will change in the future #3629). pub(crate) fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - self.service.set_sync_fork_request(peers, hash, number) + self.gossip_engine.set_sync_fork_request(peers, hash, number) } } fn incoming_global( - mut service: GossipEngine, + mut gossip_engine: GossipEngine, topic: B::Hash, voters: Arc>, gossip_validator: Arc>, @@ -376,7 +376,7 @@ fn incoming_global( let process_commit = move | msg: FullCommitMessage, mut notification: network_gossip::TopicNotification, - service: &mut GossipEngine, + gossip_engine: &mut GossipEngine, gossip_validator: &Arc>, voters: &VoterSet, | { @@ -398,7 +398,7 @@ fn incoming_global( msg.set_id, ) { if let Some(who) = notification.sender { - service.report(who, cost); + gossip_engine.report(who, cost); } return None; @@ -408,7 +408,7 @@ fn incoming_global( let commit = msg.message; let finalized_number = commit.target_number; let gossip_validator = gossip_validator.clone(); - let service = service.clone(); + let gossip_engine = gossip_engine.clone(); let neighbor_sender = neighbor_sender.clone(); let cb = move |outcome| match outcome { voter::CommitProcessingOutcome::Good(_) => { @@ -420,12 +420,12 @@ fn incoming_global( |to, neighbor| neighbor_sender.send(to, neighbor), ); - service.gossip_message(topic, notification.message.clone(), false); + gossip_engine.gossip_message(topic, notification.message.clone(), false); } voter::CommitProcessingOutcome::Bad(_) => { // report peer and do not gossip. if let Some(who) = notification.sender.take() { - service.report(who, cost::INVALID_COMMIT); + gossip_engine.report(who, cost::INVALID_COMMIT); } } }; @@ -438,12 +438,12 @@ fn incoming_global( let process_catch_up = move | msg: FullCatchUpMessage, mut notification: network_gossip::TopicNotification, - service: &mut GossipEngine, + gossip_engine: &mut GossipEngine, gossip_validator: &Arc>, voters: &VoterSet, | { let gossip_validator = gossip_validator.clone(); - let service = service.clone(); + let gossip_engine = gossip_engine.clone(); if let Err(cost) = check_catch_up::( &msg.message, @@ -451,7 +451,7 @@ fn incoming_global( msg.set_id, ) { if let Some(who) = notification.sender { - service.report(who, cost); + gossip_engine.report(who, cost); } return None; @@ -461,7 +461,7 @@ fn incoming_global( if let voter::CatchUpProcessingOutcome::Bad(_) = outcome { // report peer if let Some(who) = notification.sender.take() { - service.report(who, cost::INVALID_CATCH_UP); + gossip_engine.report(who, cost::INVALID_CATCH_UP); } } @@ -473,7 +473,7 @@ fn incoming_global( Some(voter::CommunicationIn::CatchUp(msg.message, cb)) }; - Compat::new(service.messages_for(topic) + Compat::new(gossip_engine.messages_for(topic) .map(|m| Ok::<_, ()>(m))) .filter_map(|notification| { // this could be optimized by decoding piecewise. @@ -486,9 +486,9 @@ fn incoming_global( .filter_map(move |(notification, msg)| { match msg { GossipMessage::Commit(msg) => - process_commit(msg, notification, &mut service, &gossip_validator, &*voters), + process_commit(msg, notification, &mut gossip_engine, &gossip_validator, &*voters), GossipMessage::CatchUp(msg) => - process_catch_up(msg, notification, &mut service, &gossip_validator, &*voters), + process_catch_up(msg, notification, &mut gossip_engine, &gossip_validator, &*voters), _ => { debug!(target: "afg", "Skipping unknown message type"); return None; @@ -501,7 +501,7 @@ fn incoming_global( impl Clone for NetworkBridge { fn clone(&self) -> Self { NetworkBridge { - service: self.service.clone(), + gossip_engine: self.gossip_engine.clone(), validator: Arc::clone(&self.validator), neighbor_sender: self.neighbor_sender.clone(), } diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index a8b640542ecb9..8f4fcbbc8b365 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -271,7 +271,7 @@ fn good_commit_leads_to_relay() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { - let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { + let _ = sender.unbounded_send(NetworkEvent::NotificationStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, roles: Roles::FULL, @@ -388,7 +388,7 @@ fn bad_commit_leads_to_report() { let sender_id = id.clone(); let send_message = tester.filter_network_events(move |event| match event { Event::EventStream(sender) => { - let _ = sender.unbounded_send(NetworkEvent::NotificationsStreamOpened { + let _ = sender.unbounded_send(NetworkEvent::NotificationStreamOpened { remote: sender_id.clone(), engine_id: GRANDPA_ENGINE_ID, roles: Roles::FULL, diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 8a115d801d1a6..28f0e3f9b446c 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -101,7 +101,7 @@ impl GossipEngine { let mut stream = Compat01As03::new(event_stream); while let Some(Ok(event)) = stream.next().await { match event { - Event::NotificationsStreamOpened { remote, engine_id: msg_engine_id, roles } => { + Event::NotificationStreamOpened { remote, engine_id: msg_engine_id, roles } => { if msg_engine_id != engine_id { continue; } diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index 9060fd6585d12..ae00c717570ee 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -137,9 +137,9 @@ Behaviour { self.events.push(BehaviourOut::JustificationImport(origin, hash, nb, justification)), CustomMessageOutcome::FinalityProofImport(origin, hash, nb, proof) => self.events.push(BehaviourOut::FinalityProofImport(origin, hash, nb, proof)), - CustomMessageOutcome::NotificationsStreamOpened { remote, protocols, roles } => + CustomMessageOutcome::NotificationStreamOpened { remote, protocols, roles } => for engine_id in protocols { - self.events.push(BehaviourOut::Event(Event::NotificationsStreamOpened { + self.events.push(BehaviourOut::Event(Event::NotificationStreamOpened { remote: remote.clone(), engine_id, roles, diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 33c25cd7d9f88..87dd1be4ec3ef 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -1070,7 +1070,7 @@ impl, H: ExHashT> Protocol { self.specialization.on_connect(&mut context, who.clone(), status); // Notify all the notification protocols as open. - CustomMessageOutcome::NotificationsStreamOpened { + CustomMessageOutcome::NotificationStreamOpened { remote: who, protocols: self.registered_notif_protocols.iter().cloned().collect(), roles: info.roles, @@ -1117,7 +1117,7 @@ impl, H: ExHashT> Protocol { // we handle it by notifying that we opened channels with everyone. self.context_data.peers.iter() .map(|(peer_id, peer)| - event::Event::NotificationsStreamOpened { + event::Event::NotificationStreamOpened { remote: peer_id.clone(), engine_id, roles: peer.info.roles, @@ -1794,9 +1794,9 @@ pub enum CustomMessageOutcome { BlockImport(BlockOrigin, Vec>), JustificationImport(Origin, B::Hash, NumberFor, Justification), FinalityProofImport(Origin, B::Hash, NumberFor, Vec), - /// Notifications protocols have been opened with a remote. - NotificationsStreamOpened { remote: PeerId, protocols: Vec, roles: Roles }, - /// Notifications protocols have been closed with a remote. + /// Notification protocols have been opened with a remote. + NotificationStreamOpened { remote: PeerId, protocols: Vec, roles: Roles }, + /// Notification protocols have been closed with a remote. NotificationsStreamClosed { remote: PeerId, protocols: Vec }, /// Messages have been received on one or more notifications protocols. NotificationsReceived { remote: PeerId, messages: Vec<(ConsensusEngineId, Bytes)> }, diff --git a/client/network/src/protocol/event.rs b/client/network/src/protocol/event.rs index 961f898e04d5f..98aad8c76c804 100644 --- a/client/network/src/protocol/event.rs +++ b/client/network/src/protocol/event.rs @@ -50,7 +50,7 @@ pub enum Event { /// Opened a substream with the given node with the given notifications protocol. /// /// The protocol is always one of the notification protocols that have been registered. - NotificationsStreamOpened { + NotificationStreamOpened { /// Node we opened the substream with. remote: PeerId, /// The concerned protocol. Each protocol uses a different substream. @@ -60,7 +60,7 @@ pub enum Event { }, /// Closed a substream with the given node. Always matches a corresponding previous - /// `NotificationsStreamOpened` message. + /// `NotificationStreamOpened` message. NotificationsStreamClosed { /// Node we closed the substream with. remote: PeerId, From a0b3c1c3f6c0859459395706cd2c17a923cca151 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 10 Dec 2019 15:35:33 +0100 Subject: [PATCH 20/21] Fix more concerns --- client/finality-grandpa/src/communication/mod.rs | 9 ++------- client/finality-grandpa/src/lib.rs | 5 ++++- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index d15f98326087c..e535f8577641b 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -105,11 +105,6 @@ pub(crate) fn global_topic(set_id: SetIdNumber) -> B::Hash { <::Hashing as HashT>::hash(format!("{}-GLOBAL", set_id).as_bytes()) } -/// Registers the notifications protocol towards the network. -pub(crate) fn register_dummy_protocol>(network: N) { - network.register_notifications_protocol(GRANDPA_ENGINE_ID); -} - /// Bridge between the underlying network service, gossiping consensus messages and Grandpa pub(crate) struct NetworkBridge { gossip_engine: GossipEngine, @@ -123,7 +118,7 @@ impl NetworkBridge { /// On creation it will register previous rounds' votes with the gossip /// service taken from the VoterSetState. pub(crate) fn new + Clone + Send + 'static>( - gossip_engine: N, + service: N, config: crate::Config, set_state: crate::environment::SharedVoterSetState, executor: &impl futures03::task::Spawn, @@ -135,7 +130,7 @@ impl NetworkBridge { ); let validator = Arc::new(validator); - let gossip_engine = GossipEngine::new(gossip_engine, executor, GRANDPA_ENGINE_ID, validator.clone()); + let gossip_engine = GossipEngine::new(service, executor, GRANDPA_ENGINE_ID, validator.clone()); { // register all previous votes with the gossip service so that they're diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 6308fe2d23b2b..82c04006127f3 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -913,7 +913,10 @@ pub fn setup_disabled_grandpa, RA, N>( inherent_data_providers, )?; - communication::register_dummy_protocol(network); + // We register the GRANDPA protocol so that we don't consider it an anomaly + // to receive GRANDPA messages on the network. We don't process the + // messages. + network.register_notifications_protocol(communication::GRANDPA_ENGINE_ID); Ok(()) } From 782204215228de01ac062742f9af1f38407a33d4 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 11 Dec 2019 10:34:48 +0100 Subject: [PATCH 21/21] client: Do not set fork sync request via network-gossip The finality-grandpa module needs two sets of functionalities from the network: 1. Everything gossip related, e.g. event_stream, write_notification, ... 2. The ability to set a fork sync request for a specific block hash. Instead of embedding (2) inside of (1) this patch extracts (2) from (1) having finality-grandpa depend on a `Network` that fulfills the `network_gossip::Network` trait and that can set block sync requests. On the one hand this improves the overall structure splitting things that don't logically belong together. On the other hand it does reintroduce a lot of trait bounds within finality-grandpa. --- .../finality-grandpa/src/communication/mod.rs | 51 +++++++++++++++---- .../src/communication/tests.rs | 4 +- client/finality-grandpa/src/environment.rs | 17 ++++--- client/finality-grandpa/src/lib.rs | 29 ++++++----- client/finality-grandpa/src/observer.rs | 18 ++++--- client/network-gossip/src/bridge.rs | 20 +------- client/network-gossip/src/lib.rs | 17 +------ 7 files changed, 82 insertions(+), 74 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index e535f8577641b..296ab22bdf600 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -34,8 +34,8 @@ use futures03::{compat::Compat, stream::StreamExt, future::FutureExt as _, futur use grandpa::Message::{Prevote, Precommit, PrimaryPropose}; use grandpa::{voter, voter_set::VoterSet}; use log::{debug, trace}; -use network::ReputationChange; -use network_gossip::{GossipEngine, Network}; +use network::{NetworkService, ReputationChange}; +use network_gossip::{GossipEngine, Network as GossipNetwork}; use codec::{Encode, Decode}; use primitives::Pair; use sp_runtime::traits::{Block as BlockT, Hash as HashT, Header as HeaderT, NumberFor}; @@ -95,6 +95,30 @@ mod benefit { pub(super) const PER_EQUIVOCATION: i32 = 10; } +/// A handle to the network. +/// +/// Something that provides both the capabilities needed for the `gossip_network::Network` trait as +/// well as the ability to set a fork sync request for a particular block. +pub trait Network: GossipNetwork + Clone + Send + 'static { + /// Notifies the sync service to try and sync the given block from the given + /// peers. + /// + /// If the given vector of peers is empty then the underlying implementation + /// should make a best effort to fetch the block from any peers it is + /// connected to (NOTE: this assumption will change in the future #3629). + fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor); +} + +impl Network for Arc> where + B: BlockT, + S: network::specialization::NetworkSpecialization, + H: network::ExHashT, +{ + fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { + NetworkService::set_sync_fork_request(self, peers, hash, number) + } +} + /// Create a unique topic for a round and set-id combo. pub(crate) fn round_topic(round: RoundNumber, set_id: SetIdNumber) -> B::Hash { <::Hashing as HashT>::hash(format!("{}-{}", set_id, round).as_bytes()) @@ -106,18 +130,19 @@ pub(crate) fn global_topic(set_id: SetIdNumber) -> B::Hash { } /// Bridge between the underlying network service, gossiping consensus messages and Grandpa -pub(crate) struct NetworkBridge { +pub(crate) struct NetworkBridge> { + service: N, gossip_engine: GossipEngine, validator: Arc>, neighbor_sender: periodic::NeighborPacketSender, } -impl NetworkBridge { +impl> NetworkBridge { /// Create a new NetworkBridge to the given NetworkService. Returns the service /// handle. /// On creation it will register previous rounds' votes with the gossip /// service taken from the VoterSetState. - pub(crate) fn new + Clone + Send + 'static>( + pub(crate) fn new( service: N, config: crate::Config, set_state: crate::environment::SharedVoterSetState, @@ -130,7 +155,7 @@ impl NetworkBridge { ); let validator = Arc::new(validator); - let gossip_engine = GossipEngine::new(service, executor, GRANDPA_ENGINE_ID, validator.clone()); + let gossip_engine = GossipEngine::new(service.clone(), executor, GRANDPA_ENGINE_ID, validator.clone()); { // register all previous votes with the gossip service so that they're @@ -173,7 +198,7 @@ impl NetworkBridge { let (rebroadcast_job, neighbor_sender) = periodic::neighbor_packet_worker(gossip_engine.clone()); let reporting_job = report_stream.consume(gossip_engine.clone()); - let bridge = NetworkBridge { gossip_engine, validator, neighbor_sender }; + let bridge = NetworkBridge { service, gossip_engine, validator, neighbor_sender }; let executor = Compat::new(executor); executor.execute(Box::new(rebroadcast_job.select(on_exit.clone().map(Ok).compat()).then(|_| Ok(())))) @@ -356,8 +381,13 @@ impl NetworkBridge { /// If the given vector of peers is empty then the underlying implementation /// should make a best effort to fetch the block from any peers it is /// connected to (NOTE: this assumption will change in the future #3629). - pub(crate) fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - self.gossip_engine.set_sync_fork_request(peers, hash, number) + pub(crate) fn set_sync_fork_request( + &self, + peers: Vec, + hash: B::Hash, + number: NumberFor + ) { + Network::set_sync_fork_request(&self.service, peers, hash, number) } } @@ -493,9 +523,10 @@ fn incoming_global( .map_err(|()| Error::Network(format!("Failed to receive message on unbounded stream"))) } -impl Clone for NetworkBridge { +impl> Clone for NetworkBridge { fn clone(&self) -> Self { NetworkBridge { + service: self.service.clone(), gossip_engine: self.gossip_engine.clone(), validator: Arc::clone(&self.validator), neighbor_sender: self.neighbor_sender.clone(), diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 8f4fcbbc8b365..d817abf30306a 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -66,7 +66,9 @@ impl network_gossip::Network for TestNetwork { fn announce(&self, block: Hash, _associated_data: Vec) { let _ = self.sender.unbounded_send(Event::Announce(block)); } +} +impl super::Network for TestNetwork { fn set_sync_fork_request( &self, _peers: Vec, @@ -93,7 +95,7 @@ impl network_gossip::ValidatorContext for TestNetwork { } struct Tester { - net_handle: super::NetworkBridge, + net_handle: super::NetworkBridge, gossip_validator: Arc>, events: mpsc::UnboundedReceiver, } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index a25266848137d..f450f5ab6326a 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -56,6 +56,7 @@ use crate::{ use consensus_common::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; +use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; @@ -376,20 +377,20 @@ impl SharedVoterSetState { } /// The environment we run GRANDPA in. -pub(crate) struct Environment { +pub(crate) struct Environment, RA, SC, VR> { pub(crate) client: Arc>, pub(crate) select_chain: SC, pub(crate) voters: Arc>, pub(crate) config: Config, pub(crate) authority_set: SharedAuthoritySet>, pub(crate) consensus_changes: SharedConsensusChanges>, - pub(crate) network: crate::communication::NetworkBridge, + pub(crate) network: crate::communication::NetworkBridge, pub(crate) set_id: SetId, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, } -impl Environment { +impl, RA, SC, VR> Environment { /// Updates the voter set state using the given closure. The write lock is /// held during evaluation of the closure and the environment's voter set /// state is set to its result if successful. @@ -405,13 +406,14 @@ impl Environment { } } -impl, B, E, RA, SC, VR> +impl, B, E, N, RA, SC, VR> grandpa::Chain> -for Environment +for Environment where Block: 'static, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, + N: NetworkT + 'static + Send, SC: SelectChain + 'static, VR: VotingRule>, RA: Send + Sync, @@ -554,13 +556,14 @@ pub(crate) fn ancestry, E, RA>( Ok(tree_route.retracted().iter().skip(1).map(|e| e.hash).collect()) } -impl, RA, SC, VR> +impl, N, RA, SC, VR> voter::Environment> -for Environment +for Environment where Block: 'static, B: Backend + 'static, E: CallExecutor + 'static + Send + Sync, + N: NetworkT + 'static + Send, RA: 'static + Send + Sync, SC: SelectChain + 'static, VR: VotingRule>, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 82c04006127f3..800dc27cad3dd 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -90,7 +90,6 @@ mod observer; mod until_imported; mod voting_rule; -pub use network_gossip::Network; pub use finality_proof::FinalityProofProvider; pub use justification::GrandpaJustification; pub use light_import::light_block_import; @@ -103,7 +102,7 @@ use aux_schema::PersistentData; use environment::{Environment, VoterSetState}; use import::GrandpaBlockImport; use until_imported::UntilGlobalMessageBlocksImported; -use communication::NetworkBridge; +use communication::{NetworkBridge, Network as NetworkT}; use fg_primitives::{AuthorityList, AuthorityPair, AuthoritySignature, SetId}; // Re-export these two because it's just so damn convenient. @@ -276,8 +275,9 @@ pub(crate) trait BlockSyncRequester { fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor); } -impl BlockSyncRequester for NetworkBridge where +impl BlockSyncRequester for NetworkBridge where Block: BlockT, + Network: NetworkT, { fn set_sync_fork_request(&self, peers: Vec, hash: Block::Hash, number: NumberFor) { NetworkBridge::set_sync_fork_request(self, peers, hash, number) @@ -446,11 +446,11 @@ where )) } -fn global_communication, B, E, RA>( +fn global_communication, B, E, N, RA>( set_id: SetId, voters: &Arc>, client: &Arc>, - network: &NetworkBridge, + network: &NetworkBridge, keystore: &Option, ) -> ( impl Stream< @@ -464,6 +464,7 @@ fn global_communication, B, E, RA>( ) where B: Backend, E: CallExecutor + Send + Sync, + N: NetworkT, RA: Send + Sync, NumberFor: BlockNumberOps, { @@ -548,7 +549,7 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X, Sp>( Block::Hash: Ord, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Clone + 'static, + N: NetworkT + Send + Sync + Clone + 'static, SC: SelectChain + 'static, VR: VotingRule> + Clone + 'static, NumberFor: BlockNumberOps, @@ -641,15 +642,16 @@ pub fn run_grandpa_voter, N, RA, SC, VR, X, Sp>( /// Future that powers the voter. #[must_use] -struct VoterWork { +struct VoterWork, RA, SC, VR> { voter: Box>> + Send>, - env: Arc>, + env: Arc>, voter_commands_rx: mpsc::UnboundedReceiver>>, } -impl VoterWork +impl VoterWork where Block: BlockT, + N: NetworkT + Sync, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -660,7 +662,7 @@ where fn new( client: Arc>, config: Config, - network: NetworkBridge, + network: NetworkBridge, select_chain: SC, voting_rule: VR, persistent_data: PersistentData, @@ -821,9 +823,10 @@ where } } -impl Future for VoterWork +impl Future for VoterWork where Block: BlockT, + N: NetworkT + Sync, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -880,7 +883,7 @@ pub fn run_grandpa, N, RA, SC, VR, X, Sp>( Block::Hash: Ord, B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Clone + 'static, + N: NetworkT + Send + Sync + Clone + 'static, SC: SelectChain + 'static, NumberFor: BlockNumberOps, DigestFor: Encode, @@ -906,7 +909,7 @@ pub fn setup_disabled_grandpa, RA, N>( B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, RA: Send + Sync + 'static, - N: Network + Send + Clone + 'static, + N: NetworkT + Send + Clone + 'static, { register_finality_tracker_inherent_data_provider( client, diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 4681c12753178..e1b79297b213d 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -32,10 +32,10 @@ use primitives::{H256, Blake2Hasher}; use crate::{ global_communication, CommandOrError, CommunicationIn, Config, environment, - LinkHalf, Network, Error, aux_schema::PersistentData, VoterCommand, VoterSetState, + LinkHalf, Error, aux_schema::PersistentData, VoterCommand, VoterSetState, }; use crate::authorities::SharedAuthoritySet; -use crate::communication::NetworkBridge; +use crate::communication::{Network as NetworkT, NetworkBridge}; use crate::consensus_changes::SharedConsensusChanges; use fg_primitives::AuthorityId; @@ -160,7 +160,7 @@ pub fn run_grandpa_observer, N, RA, SC, Sp>( ) -> ::sp_blockchain::Result + Send + 'static> where B: Backend + 'static, E: CallExecutor + Send + Sync + 'static, - N: Network + Send + Clone + 'static, + N: NetworkT + Send + Clone + 'static, SC: SelectChain + 'static, NumberFor: BlockNumberOps, RA: Send + Sync + 'static, @@ -202,18 +202,19 @@ pub fn run_grandpa_observer, N, RA, SC, Sp>( /// Future that powers the observer. #[must_use] -struct ObserverWork, E, Backend, RA> { +struct ObserverWork, N: NetworkT, E, Backend, RA> { observer: Box>> + Send>, client: Arc>, - network: NetworkBridge, + network: NetworkBridge, persistent_data: PersistentData, keystore: Option, voter_commands_rx: mpsc::UnboundedReceiver>>, } -impl ObserverWork +impl ObserverWork where B: BlockT, + N: NetworkT, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, @@ -221,7 +222,7 @@ where { fn new( client: Arc>, - network: NetworkBridge, + network: NetworkBridge, persistent_data: PersistentData, keystore: Option, voter_commands_rx: mpsc::UnboundedReceiver>>, @@ -325,9 +326,10 @@ where } } -impl Future for ObserverWork +impl Future for ObserverWork where B: BlockT, + N: NetworkT, NumberFor: BlockNumberOps, RA: 'static + Send + Sync, E: CallExecutor + Send + Sync + 'static, diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 28f0e3f9b446c..c950a692002d5 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -24,7 +24,7 @@ use network::{Event, ReputationChange}; use futures::{prelude::*, channel::mpsc, compat::Compat01As03, task::SpawnExt as _}; use libp2p::PeerId; use parking_lot::Mutex; -use sp_runtime::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use sp_runtime::{traits::Block as BlockT, ConsensusEngineId}; use std::{sync::Arc, time::Duration}; /// Wraps around an implementation of the `Network` crate and provides gossiping capabilities on @@ -234,19 +234,6 @@ impl GossipEngine { pub fn announce(&self, block: B::Hash, associated_data: Vec) { self.inner.lock().context_ext.announce(block, associated_data); } - - /// Notifies the sync service to try and sync the given block from the given - /// peers. - /// - /// If the given vector of peers is empty then the underlying implementation - /// should make a best effort to fetch the block from any peers it is - /// connected to (NOTE: this assumption will change in the future #3629). - /// - /// Note: this method isn't strictly related to gossiping and should eventually be moved - /// somewhere else. - pub fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - self.inner.lock().context_ext.set_sync_fork_request(peers, hash, number); - } } impl Clone for GossipEngine { @@ -287,15 +274,10 @@ impl> Context for ContextOverService { trait ContextExt { fn announce(&self, block: B::Hash, associated_data: Vec); - fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor); } impl> ContextExt for ContextOverService { fn announce(&self, block: B::Hash, associated_data: Vec) { Network::announce(&self.network, block, associated_data) } - - fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - Network::set_sync_fork_request(&self.network, peers, hash, number) - } } diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 6decda05c5142..97909a5668165 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -60,7 +60,7 @@ pub use self::state_machine::{Validator, ValidatorContext, ValidationResult}; pub use self::state_machine::DiscardAll; use network::{specialization::NetworkSpecialization, Event, ExHashT, NetworkService, PeerId, ReputationChange}; -use sp_runtime::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId}; +use sp_runtime::{traits::Block as BlockT, ConsensusEngineId}; use std::sync::Arc; mod bridge; @@ -93,17 +93,6 @@ pub trait Network { /// Note: this method isn't strictly related to gossiping and should eventually be moved /// somewhere else. fn announce(&self, block: B::Hash, associated_data: Vec); - - /// Notifies the sync service to try and sync the given block from the given - /// peers. - /// - /// If the given vector of peers is empty then the underlying implementation - /// should make a best effort to fetch the block from any peers it is - /// connected to (NOTE: this assumption will change in the future #3629). - /// - /// Note: this method isn't strictly related to gossiping and should eventually be moved - /// somewhere else. - fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor); } impl, H: ExHashT> Network for Arc> { @@ -133,8 +122,4 @@ impl, H: ExHashT> Network for Arc) { NetworkService::announce_block(self, block, associated_data) } - - fn set_sync_fork_request(&self, peers: Vec, hash: B::Hash, number: NumberFor) { - NetworkService::set_sync_fork_request(self, peers, hash, number) - } }