From 5eadb3c5ce9e9a60615ae0a06ace6928ee644c2e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 14:09:33 +0100 Subject: [PATCH 01/12] Remove sc_network::NetworkService::register_notifications_protocol --- bin/node-template/node/src/service.rs | 10 +++--- bin/node/cli/src/service.rs | 8 +++-- .../finality-grandpa/src/communication/mod.rs | 2 ++ .../src/communication/tests.rs | 2 -- client/finality-grandpa/src/lib.rs | 25 +++------------ client/network-gossip/src/bridge.rs | 6 ---- client/network-gossip/src/lib.rs | 15 --------- client/network-gossip/src/state_machine.rs | 2 -- client/network/README.md | 6 ++-- client/network/src/lib.rs | 5 +-- client/network/src/service.rs | 31 ++----------------- 11 files changed, 27 insertions(+), 85 deletions(-) diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index e32ba740504bc..e6d8cc0de6727 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -79,13 +79,15 @@ pub fn new_partial(config: &Configuration) -> Result Result { +pub fn new_full(mut config: Configuration) -> Result { let sc_service::PartialComponents { client, backend, mut task_manager, import_queue, keystore_container, select_chain, transaction_pool, inherent_data_providers, other: (block_import, grandpa_link), } = new_partial(&config)?; + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { config: &config, @@ -210,8 +212,6 @@ pub fn new_full(config: Configuration) -> Result { "grandpa-voter", sc_finality_grandpa::run_grandpa_voter(grandpa_config)? ); - } else { - sc_finality_grandpa::setup_disabled_grandpa(network)?; } network_starter.start_network(); @@ -219,10 +219,12 @@ pub fn new_full(config: Configuration) -> Result { } /// Builds a new service for a light client. -pub fn new_light(config: Configuration) -> Result { +pub fn new_light(mut config: Configuration) -> Result { let (client, backend, keystore_container, mut task_manager, on_demand) = sc_service::new_light_parts::(&config)?; + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + let select_chain = sc_consensus::LongestChain::new(backend.clone()); let transaction_pool = Arc::new(sc_transaction_pool::BasicPool::new_light( diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 9d7c9bb1b7a66..3f3fcf9f8d75f 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -164,7 +164,7 @@ pub struct NewFullBase { /// Creates a full service from the configuration. pub fn new_full_base( - config: Configuration, + mut config: Configuration, with_startup_data: impl FnOnce( &sc_consensus_babe::BabeBlockImport, &sc_consensus_babe::BabeLink, @@ -178,6 +178,8 @@ pub fn new_full_base( let shared_voter_state = rpc_setup; + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { config: &config, @@ -338,7 +340,7 @@ pub fn new_full(config: Configuration) }) } -pub fn new_light_base(config: Configuration) -> Result<( +pub fn new_light_base(mut config: Configuration) -> Result<( TaskManager, RpcHandlers, Arc, Arc::Hash>>, Arc>> @@ -346,6 +348,8 @@ pub fn new_light_base(config: Configuration) -> Result<( let (client, backend, keystore_container, mut task_manager, on_demand) = sc_service::new_light_parts::(&config)?; + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + let select_chain = sc_consensus::LongestChain::new(backend.clone()); let transaction_pool = Arc::new(sc_transaction_pool::BasicPool::new_light( diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 038d82a8cdc3b..29fe8bc7471a0 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -68,6 +68,8 @@ mod periodic; #[cfg(test)] pub(crate) mod tests; +/// Name of the notifications protocol used by Grandpa. Must be registered towards the networking +/// in order for Grandpa to properly function. pub const GRANDPA_PROTOCOL_NAME: &'static str = "/paritytech/grandpa/1"; // cost scalars for reporting peers. diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index e1685256f7b8d..27a394a062bc8 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -62,8 +62,6 @@ impl sc_network_gossip::Network for TestNetwork { let _ = self.sender.unbounded_send(Event::WriteNotification(who, message)); } - fn register_notifications_protocol(&self, _: Cow<'static, str>) {} - fn announce(&self, block: Hash, _associated_data: Vec) { let _ = self.sender.unbounded_send(Event::Announce(block)); } diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index c5f89717a64d7..ced101b8c8562 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -122,6 +122,7 @@ mod until_imported; mod voting_rule; pub use authorities::{SharedAuthoritySet, AuthoritySet}; +pub use communication::GRANDPA_PROTOCOL_NAME; pub use finality_proof::{FinalityProofFragment, FinalityProofProvider, StorageAndProofProvider}; pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream}; pub use import::GrandpaBlockImport; @@ -652,6 +653,10 @@ pub struct GrandpaParams { /// A link to the block import worker. pub link: LinkHalf, /// The Network instance. + /// + /// It is assumed that this network will feed us Grandpa notifications. When using the + /// `sc_network` crate, it is assumed that the Grandpa notifications protocol has been passed + /// to the configuration of the networking. pub network: N, /// If supplied, can be used to hook on telemetry connection established events. pub telemetry_on_connect: Option>, @@ -1065,26 +1070,6 @@ where } } -/// When GRANDPA is not initialized we still need to register the finality -/// tracker inherent provider which might be expected by the runtime for block -/// authoring. Additionally, we register a gossip message validator that -/// discards all GRANDPA messages (otherwise, we end up banning nodes that send -/// us a `Neighbor` message, since there is no registered gossip validator for -/// the engine id defined in the message.) -pub fn setup_disabled_grandpa(network: N) -> Result<(), sp_consensus::Error> -where - N: NetworkT + Send + Clone + 'static, -{ - // 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( - From::from(communication::GRANDPA_PROTOCOL_NAME), - ); - - Ok(()) -} - /// Checks if this node has any available keys in the keystore for any authority id in the given /// voter set. Returns the authority id for which keys are available, or `None` if no keys are /// available. diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 98ada69590f1c..4deaad6d748fd 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -72,11 +72,7 @@ impl GossipEngine { validator: Arc>, ) -> Self where B: 'static { let protocol = protocol.into(); - - // We grab the event stream before registering the notifications protocol, otherwise we - // might miss events. let network_event_stream = network.event_stream(); - network.register_notifications_protocol(protocol.clone()); GossipEngine { state_machine: ConsensusGossip::new(validator, protocol.clone()), @@ -335,8 +331,6 @@ mod tests { unimplemented!(); } - fn register_notifications_protocol(&self, _: Cow<'static, str>) {} - fn announce(&self, _: B::Hash, _: Vec) { unimplemented!(); } diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 09e946d1a1ea9..2b333610223e2 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -81,14 +81,6 @@ pub trait Network { /// Send a notification to a peer. fn write_notification(&self, who: PeerId, protocol: Cow<'static, str>, message: Vec); - /// Registers a notifications protocol. - /// - /// See the documentation of [`NetworkService:register_notifications_protocol`] for more information. - fn register_notifications_protocol( - &self, - protocol: Cow<'static, str>, - ); - /// 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 @@ -113,13 +105,6 @@ impl Network for Arc> { NetworkService::write_notification(self, who, protocol, message) } - fn register_notifications_protocol( - &self, - protocol: Cow<'static, str>, - ) { - NetworkService::register_notifications_protocol(self, protocol) - } - fn announce(&self, block: B::Hash, associated_data: Vec) { NetworkService::announce_block(self, block, associated_data) } diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 8bd6d9df01911..88f9d48375dec 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -503,8 +503,6 @@ mod tests { unimplemented!(); } - fn register_notifications_protocol(&self, _: Cow<'static, str>) {} - fn announce(&self, _: B::Hash, _: Vec) { unimplemented!(); } diff --git a/client/network/README.md b/client/network/README.md index e0bd691043bee..914720f53e2a9 100644 --- a/client/network/README.md +++ b/client/network/README.md @@ -120,8 +120,8 @@ bytes. block announces are pushed to other nodes. The handshake is empty on both sides. The message format is a SCALE-encoded tuple containing a block header followed with an opaque list of bytes containing some data associated with this block announcement, e.g. a candidate message. -- Notifications protocols that are registered using the `register_notifications_protocol` -method. For example: `/paritytech/grandpa/1`. See below for more information. +- Notifications protocols that are registered using `NetworkConfiguration::notifications_protocols`. +For example: `/paritytech/grandpa/1`. See below for more information. ## The legacy Substrate substream @@ -223,4 +223,4 @@ dispatching a background task with the [`NetworkWorker`]. More precise usage details are still being worked on and will likely change in the future. -License: GPL-3.0-or-later WITH Classpath-exception-2.0 \ No newline at end of file +License: GPL-3.0-or-later WITH Classpath-exception-2.0 diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index 91ea49bce76c5..fb65c754d79a2 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -141,8 +141,9 @@ //! block announces are pushed to other nodes. The handshake is empty on both sides. The message //! format is a SCALE-encoded tuple containing a block header followed with an opaque list of //! bytes containing some data associated with this block announcement, e.g. a candidate message. -//! - Notifications protocols that are registered using the `register_notifications_protocol` -//! method. For example: `/paritytech/grandpa/1`. See below for more information. +//! - Notifications protocols that are registered using +//! `NetworkConfiguration::notifications_protocols`. For example: `/paritytech/grandpa/1`. See +//! below for more information. //! //! ## The legacy Substrate substream //! diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 8c0dbd7eec6af..b6f162affd671 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -656,7 +656,7 @@ impl NetworkService { /// > 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` or + /// The protocol must have been registered with /// [`NetworkConfiguration::notifications_protocols`](crate::config::NetworkConfiguration::notifications_protocols). /// pub fn write_notification(&self, target: PeerId, protocol: Cow<'static, str>, message: Vec) { @@ -717,7 +717,7 @@ impl NetworkService { /// return an error. It is however possible for the entire connection to be abruptly closed, /// in which case enqueued notifications will be lost. /// - /// The protocol must have been registered with `register_notifications_protocol` or + /// The protocol must have been registered with /// [`NetworkConfiguration::notifications_protocols`](crate::config::NetworkConfiguration::notifications_protocols). /// /// # Usage @@ -844,28 +844,6 @@ impl NetworkService { } } - /// Registers a new notifications protocol. - /// - /// After a protocol has been registered, you can call `write_notifications`. - /// - /// **Important**: This method is a work-around, and you are instead strongly encouraged to - /// pass the protocol in the `NetworkConfiguration::notifications_protocols` list instead. - /// If you have no other choice but to use this method, you are very strongly encouraged to - /// call it very early on. Any connection open will retain the protocols that were registered - /// then, and not any new one. - /// - /// Please call `event_stream` before registering a protocol, otherwise you may miss events - /// about the protocol that you have registered. - // TODO: remove this method after https://github.com/paritytech/substrate/issues/6827 - pub fn register_notifications_protocol( - &self, - protocol_name: impl Into>, - ) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::RegisterNotifProtocol { - protocol_name: protocol_name.into(), - }); - } - /// You may call this when new transactons are imported by the transaction pool. /// /// All transactions will be fetched from the `TransactionPool` that was passed at @@ -1222,9 +1200,6 @@ enum ServiceToWorkerMsg { request: Vec, pending_response: oneshot::Sender, RequestFailure>>, }, - RegisterNotifProtocol { - protocol_name: Cow<'static, str>, - }, DisconnectPeer(PeerId), NewBestBlockImported(B::Hash, NumberFor), } @@ -1359,8 +1334,6 @@ impl Future for NetworkWorker { }, } }, - ServiceToWorkerMsg::RegisterNotifProtocol { protocol_name } => - this.network_service.register_notifications_protocol(protocol_name), ServiceToWorkerMsg::DisconnectPeer(who) => this.network_service.user_protocol_mut().disconnect_peer(&who), ServiceToWorkerMsg::NewBestBlockImported(hash, number) => From 85bbc8fb2c85ad2bee07e8c81820477b2cafb0c6 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 14:37:27 +0100 Subject: [PATCH 02/12] Missing calls to .into() --- bin/node-template/node/src/service.rs | 4 ++-- bin/node/cli/src/service.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index e6d8cc0de6727..1fa1a372a05d3 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -86,7 +86,7 @@ pub fn new_full(mut config: Configuration) -> Result other: (block_import, grandpa_link), } = new_partial(&config)?; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { @@ -223,7 +223,7 @@ pub fn new_light(mut config: Configuration) -> Result let (client, backend, keystore_container, mut task_manager, on_demand) = sc_service::new_light_parts::(&config)?; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); let select_chain = sc_consensus::LongestChain::new(backend.clone()); diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 3f3fcf9f8d75f..e4068b7c7cf39 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -178,7 +178,7 @@ pub fn new_full_base( let shared_voter_state = rpc_setup; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { @@ -348,7 +348,7 @@ pub fn new_light_base(mut config: Configuration) -> Result<( let (client, backend, keystore_container, mut task_manager, on_demand) = sc_service::new_light_parts::(&config)?; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME); + config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); let select_chain = sc_consensus::LongestChain::new(backend.clone()); From 83425d22f2aa3ca60d8acec20a321ef069a6e36e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 14:39:08 +0100 Subject: [PATCH 03/12] Wrong crate name --- bin/node/cli/src/service.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index e4068b7c7cf39..5eb8e35e69ec5 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -178,7 +178,7 @@ pub fn new_full_base( let shared_voter_state = rpc_setup; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); + config.network.notifications_protocols.push(grandpa::GRANDPA_PROTOCOL_NAME.into()); let (network, network_status_sinks, system_rpc_tx, network_starter) = sc_service::build_network(sc_service::BuildNetworkParams { @@ -317,8 +317,6 @@ pub fn new_full_base( "grandpa-voter", grandpa::run_grandpa_voter(grandpa_config)? ); - } else { - grandpa::setup_disabled_grandpa(network.clone())?; } network_starter.start_network(); @@ -348,7 +346,7 @@ pub fn new_light_base(mut config: Configuration) -> Result<( let (client, backend, keystore_container, mut task_manager, on_demand) = sc_service::new_light_parts::(&config)?; - config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into()); + config.network.notifications_protocols.push(grandpa::GRANDPA_PROTOCOL_NAME.into()); let select_chain = sc_consensus::LongestChain::new(backend.clone()); From e0b4c2fe19a2628f14ac1b96aefb96d0a9f45500 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 17:57:20 +0100 Subject: [PATCH 04/12] [WIP] Fix Grandpa tests --- client/finality-grandpa/src/tests.rs | 230 +++++++++++---------------- 1 file changed, 96 insertions(+), 134 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index ef8168e84f66c..9ce8096e2191d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -269,6 +269,52 @@ fn block_until_complete(future: impl Future + Unpin, net: &Arc impl Future { + let voters = stream::FuturesUnordered::new(); + + for (peer_id, key) in peers.iter().enumerate() { + let (keystore, _) = create_keystore(*key); + + let (net_service, link) = { + // temporary needed for some reason + let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); + ( + net.peers[peer_id].network_service().clone(), + link, + ) + }; + + let grandpa_params = GrandpaParams { + config: Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: Some(keystore), + name: Some(format!("peer#{}", peer_id)), + is_authority: true, + observer_enabled: true, + }, + link, + network: net_service, + telemetry_on_connect: None, + voting_rule: (), + prometheus_registry: None, + shared_voter_state: SharedVoterState::empty(), + }; + let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); + + fn assert_send(_: &T) { } + assert_send(&voter); + + voters.push(voter); + } + + voters.for_each(|_| async move {}) +} + // run the voters to completion. provide a closure to be invoked after // the voters are spawned but before blocking on them. fn run_to_completion_with( @@ -288,22 +334,9 @@ fn run_to_completion_with( wait_for.push(f); }; - let mut keystore_paths = Vec::new(); - for (peer_id, key) in peers.iter().enumerate() { - let (keystore, keystore_path) = create_keystore(*key); - keystore_paths.push(keystore_path); - + for (peer_id, _) in peers.iter().enumerate() { let highest_finalized = highest_finalized.clone(); - let (client, net_service, link) = { - let net = net.lock(); - // temporary needed for some reason - let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); - ( - net.peers[peer_id].client().clone(), - net.peers[peer_id].network_service().clone(), - link, - ) - }; + let client = net.lock().peers[peer_id].client().clone(); wait_for.push( Box::pin( @@ -319,30 +352,6 @@ fn run_to_completion_with( .map(|_| ()) ) ); - - fn assert_send(_: &T) { } - - let grandpa_params = GrandpaParams { - config: Config { - gossip_duration: TEST_GOSSIP_DURATION, - justification_period: 32, - keystore: Some(keystore), - name: Some(format!("peer#{}", peer_id)), - is_authority: true, - observer_enabled: true, - }, - link: link, - network: net_service, - telemetry_on_connect: None, - voting_rule: (), - prometheus_registry: None, - shared_voter_state: SharedVoterState::empty(), - }; - let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); - - assert_send(&voter); - - runtime.spawn(voter); } // wait for all finalized on each. @@ -388,6 +397,7 @@ fn finalize_3_voters_no_observers() { let voters = make_ids(peers); let mut net = GrandpaTestNet::new(TestApi::new(voters), 3); + runtime.spawn(initialize_grandpa(&mut net, peers)); net.peer(0).push_blocks(20, false); net.block_until_sync(); @@ -406,75 +416,33 @@ fn finalize_3_voters_no_observers() { ); } -#[test] +/*#[test] fn finalize_3_voters_1_full_observer() { let mut runtime = Runtime::new().unwrap(); let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(peers); - let mut net = GrandpaTestNet::new(TestApi::new(voters), 4); - net.peer(0).push_blocks(20, false); - net.block_until_sync(); - - let net = Arc::new(Mutex::new(net)); - let mut finality_notifications = Vec::new(); - let all_peers = peers.iter() .cloned() .map(Some) - .chain(std::iter::once(None)); + .chain(std::iter::once(None)) + .collect::>(); - let mut keystore_paths = Vec::new(); + let mut net = GrandpaTestNet::new(TestApi::new(voters), 4); + runtime.spawn(initialize_grandpa(&mut net, &all_peers)); + net.peer(0).push_blocks(20, false); - let mut voters = Vec::new(); + let net = Arc::new(Mutex::new(net)); + let mut finality_notifications = Vec::new(); - for (peer_id, local_key) in all_peers.enumerate() { - let (client, net_service, link) = { - let net = net.lock(); - let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); - ( - net.peers[peer_id].client().clone(), - net.peers[peer_id].network_service().clone(), - link, - ) - }; + for (peer_id, _) in all_peers.iter().enumerate() { + let client = net.lock().peers[peer_id].client().clone(); finality_notifications.push( client.finality_notification_stream() .take_while(|n| future::ready(n.header.number() < &20)) .for_each(move |_| future::ready(())) ); - - let keystore = if let Some(local_key) = local_key { - let (keystore, keystore_path) = create_keystore(local_key); - keystore_paths.push(keystore_path); - Some(keystore) - } else { - None - }; - - let grandpa_params = GrandpaParams { - config: Config { - gossip_duration: TEST_GOSSIP_DURATION, - justification_period: 32, - keystore, - name: Some(format!("peer#{}", peer_id)), - is_authority: true, - observer_enabled: true, - }, - link: link, - network: net_service, - telemetry_on_connect: None, - voting_rule: (), - prometheus_registry: None, - shared_voter_state: SharedVoterState::empty(), - }; - - voters.push(run_grandpa_voter(grandpa_params).expect("all in order with client and network")); - } - - for voter in voters { - runtime.spawn(voter); } // wait for all finalized on each. @@ -482,9 +450,9 @@ fn finalize_3_voters_1_full_observer() { .map(|_| ()); block_until_complete(wait_for, &net, &mut runtime); -} +}*/ -#[test] +/*#[test] fn transition_3_voters_twice_1_full_observer() { sp_tracing::try_init_simple(); let peers_a = &[ @@ -641,7 +609,7 @@ fn transition_3_voters_twice_1_full_observer() { let wait_for = ::futures::future::join_all(finality_notifications); block_until_complete(wait_for, &net, &mut runtime); -} +}*/ #[test] fn justification_is_generated_periodically() { @@ -650,6 +618,7 @@ fn justification_is_generated_periodically() { let voters = make_ids(peers); let mut net = GrandpaTestNet::new(TestApi::new(voters), 3); + runtime.spawn(initialize_grandpa(&mut net, peers)); net.peer(0).push_blocks(32, false); net.block_until_sync(); @@ -673,6 +642,7 @@ fn sync_justifications_on_change_blocks() { // 4 peers, 3 of them are authorities and participate in grandpa let api = TestApi::new(voters); let mut net = GrandpaTestNet::new(api, 4); + let voters = initialize_grandpa(&mut net, peers_a); // add 20 blocks net.peer(0).push_blocks(20, false); @@ -697,6 +667,7 @@ fn sync_justifications_on_change_blocks() { } let net = Arc::new(Mutex::new(net)); + runtime.spawn(voters); run_to_completion(&mut runtime, 25, net.clone(), peers_a); // the first 3 peers are grandpa voters and therefore have already finalized @@ -734,6 +705,7 @@ fn finalizes_multiple_pending_changes_in_order() { // 6 peers, 3 of them are authorities and participate in grandpa from genesis let api = TestApi::new(genesis_voters); let mut net = GrandpaTestNet::new(api, 6); + runtime.spawn(initialize_grandpa(&mut net, all_peers)); // add 20 blocks net.peer(0).push_blocks(20, false); @@ -776,7 +748,7 @@ fn finalizes_multiple_pending_changes_in_order() { run_to_completion(&mut runtime, 30, net.clone(), all_peers); } -#[test] +/*#[test] fn force_change_to_new_set() { sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); @@ -792,7 +764,8 @@ fn force_change_to_new_set() { let api = TestApi::new(make_ids(genesis_authorities)); let voters = make_ids(peers_a); - let net = GrandpaTestNet::new(api, 3); + let mut net = GrandpaTestNet::new(api, 3); + runtime.spawn(initialize_grandpa(&mut net, peers_a)); let net = Arc::new(Mutex::new(net)); net.lock().peer(0).generate_blocks(1, BlockOrigin::File, |builder| { @@ -831,7 +804,7 @@ fn force_change_to_new_set() { // we add_blocks after the voters are spawned because otherwise // the link-halves have the wrong AuthoritySet run_to_completion(&mut runtime, 25, net, peers_a); -} +}*/ #[test] fn allows_reimporting_change_blocks() { @@ -933,7 +906,7 @@ fn test_bad_justification() { ); } -#[test] +/*#[test] fn voter_persists_its_votes() { use std::sync::atomic::{AtomicUsize, Ordering}; use futures::future; @@ -1190,7 +1163,7 @@ fn voter_persists_its_votes() { } block_until_complete(exit_rx.into_future(), &net, &mut runtime); -} +}*/ #[test] fn finalize_3_voters_1_light_observer() { @@ -1200,6 +1173,19 @@ fn finalize_3_voters_1_light_observer() { let voters = make_ids(authorities); let mut net = GrandpaTestNet::new(TestApi::new(voters), 4); + let voters = initialize_grandpa(&mut net, authorities); + let observer = observer::run_grandpa_observer( + Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: None, + name: Some("observer".to_string()), + is_authority: false, + observer_enabled: true, + }, + net.peers[3].data.lock().take().expect("link initialized at startup; qed"), + net.peers[3].network_service().clone(), + ).unwrap(); net.peer(0).push_blocks(20, false); net.block_until_sync(); @@ -1209,32 +1195,10 @@ fn finalize_3_voters_1_light_observer() { } let net = Arc::new(Mutex::new(net)); - let link = net.lock().peer(3).data.lock().take().expect("link initialized on startup; qed"); - - let finality_notifications = net.lock().peer(3).client().finality_notification_stream() - .take_while(|n| { - future::ready(n.header.number() < &20) - }) - .collect::>(); - - run_to_completion_with(&mut runtime, 20, net.clone(), authorities, |executor| { - executor.spawn( - observer::run_grandpa_observer( - Config { - gossip_duration: TEST_GOSSIP_DURATION, - justification_period: 32, - keystore: None, - name: Some("observer".to_string()), - is_authority: false, - observer_enabled: true, - }, - link, - net.lock().peers[3].network_service().clone(), - ).unwrap() - ); - Some(Box::pin(finality_notifications.map(|_| ()))) - }); + runtime.spawn(voters); + runtime.spawn(observer); + run_to_completion(&mut runtime, 20, net.clone(), authorities); } #[test] @@ -1245,9 +1209,7 @@ fn voter_catches_up_to_latest_round_when_behind() { let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob]; let voters = make_ids(peers); - let mut net = GrandpaTestNet::new(TestApi::new(voters), 3); - net.peer(0).push_blocks(50, false); - net.block_until_sync(); + let net = GrandpaTestNet::new(TestApi::new(voters), 2); let net = Arc::new(Mutex::new(net)); let mut finality_notifications = Vec::new(); @@ -1300,6 +1262,9 @@ fn voter_catches_up_to_latest_round_when_behind() { runtime.spawn(voter); } + net.lock().peer(0).push_blocks(50, false); + net.lock().block_until_sync(); + // 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); @@ -1311,18 +1276,15 @@ fn voter_catches_up_to_latest_round_when_behind() { let runtime = runtime.handle().clone(); wait_for_finality.then(move |_| { - let peer_id = 2; + net.lock().add_full_peer(); + let link = { let net = net.lock(); - let mut link = net.peers[peer_id].data.lock(); + let mut link = net.peers[2].data.lock(); link.take().expect("link initialized at startup; qed") }; - let set_state = link.persistent_data.set_state.clone(); - - let voter = voter(None, peer_id, link, net); - - runtime.spawn(voter); + runtime.spawn(voter(None, 2, link, net.clone())); let start_time = std::time::Instant::now(); let timeout = Duration::from_secs(5 * 60); From d2783df302754cbdc80e1f29934d77af4a89dee4 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 18:02:42 +0100 Subject: [PATCH 05/12] One more passing --- client/finality-grandpa/src/tests.rs | 34 ++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 9ce8096e2191d..ca292e1ac2822 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -416,7 +416,7 @@ fn finalize_3_voters_no_observers() { ); } -/*#[test] +#[test] fn finalize_3_voters_1_full_observer() { let mut runtime = Runtime::new().unwrap(); @@ -430,13 +430,39 @@ fn finalize_3_voters_1_full_observer() { .collect::>(); let mut net = GrandpaTestNet::new(TestApi::new(voters), 4); - runtime.spawn(initialize_grandpa(&mut net, &all_peers)); + runtime.spawn(initialize_grandpa(&mut net, peers)); + + runtime.spawn({ + let peer_id = 3; + let net_service = net.peers[peer_id].network_service().clone(); + let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); + + let grandpa_params = GrandpaParams { + config: Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: None, + name: Some(format!("peer#{}", peer_id)), + is_authority: true, + observer_enabled: true, + }, + link: link, + network: net_service, + telemetry_on_connect: None, + voting_rule: (), + prometheus_registry: None, + shared_voter_state: SharedVoterState::empty(), + }; + + run_grandpa_voter(grandpa_params).expect("all in order with client and network") + }); + net.peer(0).push_blocks(20, false); let net = Arc::new(Mutex::new(net)); let mut finality_notifications = Vec::new(); - for (peer_id, _) in all_peers.iter().enumerate() { + for peer_id in 0..4 { let client = net.lock().peers[peer_id].client().clone(); finality_notifications.push( client.finality_notification_stream() @@ -450,7 +476,7 @@ fn finalize_3_voters_1_full_observer() { .map(|_| ()); block_until_complete(wait_for, &net, &mut runtime); -}*/ +} /*#[test] fn transition_3_voters_twice_1_full_observer() { From 5ec0c26bb0e3dff8c4d1bcd6cf03733df4fc5ce3 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 18:19:12 +0100 Subject: [PATCH 06/12] One more. Two to go. --- client/finality-grandpa/src/tests.rs | 93 +++++++++++++++------------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index ca292e1ac2822..3852a9879ad0d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -478,7 +478,7 @@ fn finalize_3_voters_1_full_observer() { block_until_complete(wait_for, &net, &mut runtime); } -/*#[test] +#[test] fn transition_3_voters_twice_1_full_observer() { sp_tracing::try_init_simple(); let peers_a = &[ @@ -501,6 +501,13 @@ fn transition_3_voters_twice_1_full_observer() { let observer = &[Ed25519Keyring::One]; + let all_peers = peers_a.iter() + .chain(peers_b) + .chain(peers_c) + .chain(observer) + .cloned() + .collect::>(); // deduplicate + let genesis_voters = make_ids(peers_a); let api = TestApi::new(genesis_voters); @@ -508,6 +515,41 @@ fn transition_3_voters_twice_1_full_observer() { let mut runtime = Runtime::new().unwrap(); + let mut keystore_paths = Vec::new(); + let mut voters = Vec::new(); + for (peer_id, local_key) in all_peers.clone().into_iter().enumerate() { + let (keystore, keystore_path) = create_keystore(local_key); + keystore_paths.push(keystore_path); + + let (net_service, link) = { + let net = net.lock(); + let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); + ( + net.peers[peer_id].network_service().clone(), + link, + ) + }; + + let grandpa_params = GrandpaParams { + config: Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: Some(keystore), + name: Some(format!("peer#{}", peer_id)), + is_authority: true, + observer_enabled: true, + }, + link, + network: net_service, + telemetry_on_connect: None, + voting_rule: (), + prometheus_registry: None, + shared_voter_state: SharedVoterState::empty(), + }; + + voters.push(run_grandpa_voter(grandpa_params).expect("all in order with client and network")); + } + net.lock().peer(0).push_blocks(1, false); net.lock().block_until_sync(); @@ -573,30 +615,13 @@ fn transition_3_voters_twice_1_full_observer() { } let mut finality_notifications = Vec::new(); - let all_peers = peers_a.iter() - .chain(peers_b) - .chain(peers_c) - .chain(observer) - .cloned() - .collect::>() // deduplicate - .into_iter() - .enumerate(); - let mut keystore_paths = Vec::new(); - for (peer_id, local_key) in all_peers { - let (keystore, keystore_path) = create_keystore(local_key); - keystore_paths.push(keystore_path); - - let (client, net_service, link) = { - let net = net.lock(); - let link = net.peers[peer_id].data.lock().take().expect("link initialized at startup; qed"); - ( - net.peers[peer_id].client().clone(), - net.peers[peer_id].network_service().clone(), - link, - ) - }; + for voter in voters { + runtime.spawn(voter); + } + for (peer_id, _) in all_peers.into_iter().enumerate() { + let client = net.lock().peers[peer_id].client().clone(); finality_notifications.push( client.finality_notification_stream() .take_while(|n| future::ready(n.header.number() < &30)) @@ -609,33 +634,13 @@ 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, - justification_period: 32, - keystore: Some(keystore), - name: Some(format!("peer#{}", peer_id)), - is_authority: true, - observer_enabled: true, - }, - link: link, - network: net_service, - telemetry_on_connect: None, - voting_rule: (), - prometheus_registry: None, - shared_voter_state: SharedVoterState::empty(), - }; - let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); - - runtime.spawn(voter); } // wait for all finalized on each. let wait_for = ::futures::future::join_all(finality_notifications); block_until_complete(wait_for, &net, &mut runtime); -}*/ +} #[test] fn justification_is_generated_periodically() { From e1fff657642d95f1097dc79767c836ec65189c9e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 18:21:13 +0100 Subject: [PATCH 07/12] =?UTF-8?q?This=20one=20was=20actually=20already=20p?= =?UTF-8?q?assing=20=F0=9F=8E=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- client/finality-grandpa/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 3852a9879ad0d..9cf1cc4a87598 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -779,7 +779,7 @@ fn finalizes_multiple_pending_changes_in_order() { run_to_completion(&mut runtime, 30, net.clone(), all_peers); } -/*#[test] +#[test] fn force_change_to_new_set() { sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); @@ -835,7 +835,7 @@ fn force_change_to_new_set() { // we add_blocks after the voters are spawned because otherwise // the link-halves have the wrong AuthoritySet run_to_completion(&mut runtime, 25, net, peers_a); -}*/ +} #[test] fn allows_reimporting_change_blocks() { From 04a1519ab129a7f9b0ee3105b48be02b49b87e0e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 1 Dec 2020 18:58:14 +0100 Subject: [PATCH 08/12] Last one compiles --- client/finality-grandpa/src/tests.rs | 168 +++++++-------------------- 1 file changed, 42 insertions(+), 126 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 9cf1cc4a87598..3a32965f709ab 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -423,12 +423,6 @@ fn finalize_3_voters_1_full_observer() { let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let voters = make_ids(peers); - let all_peers = peers.iter() - .cloned() - .map(Some) - .chain(std::iter::once(None)) - .collect::>(); - let mut net = GrandpaTestNet::new(TestApi::new(voters), 4); runtime.spawn(initialize_grandpa(&mut net, peers)); @@ -937,14 +931,14 @@ fn test_bad_justification() { ); } -/*#[test] +#[test] fn voter_persists_its_votes() { use std::sync::atomic::{AtomicUsize, Ordering}; use futures::future; - use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); + let mut keystore_paths = Vec::new(); // 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 @@ -952,153 +946,71 @@ fn voter_persists_its_votes() { let voters = make_ids(peers); // alice has a chain with 20 blocks - let mut net = GrandpaTestNet::new(TestApi::new(voters.clone()), 2); - net.peer(0).push_blocks(20, false); - net.block_until_sync(); - - assert_eq!(net.peer(0).client().info().best_number, 20, - "Peer #{} failed to sync", 0); - - - let peer = net.peer(0); - let client = peer.client().clone(); - let net = Arc::new(Mutex::new(net)); - - // channel between the voter and the main controller. - // sending a message on the `voter_tx` restarts the voter. - let (voter_tx, voter_rx) = tracing_unbounded::<()>(""); - - let mut keystore_paths = Vec::new(); - - // startup a grandpa voter for alice but also listen for messages on a - // channel. whenever a message is received the voter is restarted. when the - // sender is dropped the voter is stopped. - { - let (keystore, keystore_path) = create_keystore(peers[0]); - keystore_paths.push(keystore_path); - - struct ResettableVoter { - voter: Pin + Send + Unpin>>, - voter_rx: TracingUnboundedReceiver<()>, - net: Arc>, - client: PeersClient, - keystore: SyncCryptoStorePtr, - } - - impl Future for ResettableVoter { - type Output = (); - - fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll { - let this = Pin::into_inner(self); - - if let Poll::Ready(()) = Pin::new(&mut this.voter).poll(cx) { - panic!("error in the voter"); - } - - match Pin::new(&mut this.voter_rx).poll_next(cx) { - Poll::Pending => return Poll::Pending, - Poll::Ready(None) => return Poll::Ready(()), - Poll::Ready(Some(())) => { - let (_block_import, _, link) = - this.net.lock() - .make_block_import::< - TransactionFor - >(this.client.clone()); - let link = link.lock().take().unwrap(); - - let grandpa_params = GrandpaParams { - config: Config { - gossip_duration: TEST_GOSSIP_DURATION, - justification_period: 32, - keystore: Some(this.keystore.clone()), - name: Some(format!("peer#{}", 0)), - is_authority: true, - observer_enabled: true, - }, - link, - network: this.net.lock().peers[0].network_service().clone(), - telemetry_on_connect: None, - voting_rule: VotingRulesBuilder::default().build(), - prometheus_registry: None, - shared_voter_state: SharedVoterState::empty(), - }; - - let voter = run_grandpa_voter(grandpa_params) - .expect("all in order with client and network") - .map(move |r| { - // we need to keep the block_import alive since it owns the - // sender for the voter commands channel, if that gets dropped - // then the voter will stop - drop(_block_import); - r - }); - - this.voter = Box::pin(voter); - // notify current task in order to poll the voter - cx.waker().wake_by_ref(); - } - }; - - Poll::Pending - } - } - - // we create a "dummy" voter by setting it to `pending` and triggering the `tx`. - // this way, the `ResettableVoter` will reset its `voter` field to a value ASAP. - voter_tx.unbounded_send(()).unwrap(); - runtime.spawn(ResettableVoter { - voter: Box::pin(futures::future::pending()), - voter_rx, - net: net.clone(), - client: client.clone(), - keystore, - }); - } - - let (exit_tx, exit_rx) = futures::channel::oneshot::channel::<()>(); + let mut net = GrandpaTestNet::new(TestApi::new(voters.clone()), 3); // create the communication layer for bob, but don't start any // voter. instead we'll listen for the prevote that alice casts // and cast our own manually - { + let bob_keystore = { let (keystore, keystore_path) = create_keystore(peers[1]); keystore_paths.push(keystore_path); - + keystore + }; + let bob_network = { let config = Config { gossip_duration: TEST_GOSSIP_DURATION, justification_period: 32, - keystore: Some(keystore.clone()), + keystore: Some(bob_keystore.clone()), name: Some(format!("peer#{}", 1)), is_authority: true, observer_enabled: true, }; let set_state = { - let (_, _, link) = net.lock() + let bob_client = net.peer(1).client().clone(); + let (_, _, link) = net .make_block_import::< TransactionFor - >(client); + >(bob_client); let LinkHalf { persistent_data, .. } = link.lock().take().unwrap(); let PersistentData { set_state, .. } = persistent_data; set_state }; - let network = communication::NetworkBridge::new( - net.lock().peers[1].network_service().clone(), + communication::NetworkBridge::new( + net.peers[1].network_service().clone(), config.clone(), set_state, None, - ); + ) + }; - let (round_rx, round_tx) = network.round_communication( - Some((peers[1].public().into(), keystore).into()), + net.peer(0).push_blocks(20, false); + net.block_until_sync(); + + assert_eq!(net.peer(0).client().info().best_number, 20, + "Peer #{} failed to sync", 0); + + // spawn two voters for alice. + // half-way through the test, we stop one and start the other. + let (alice_voter1, abort) = future::abortable(initialize_grandpa(&mut net, &peers[..1])); + let alice_voter2 = Arc::new(Mutex::new(Some(initialize_grandpa(&mut net, &peers[..1])))); + runtime.spawn(alice_voter1); + + let net = Arc::new(Mutex::new(net)); + + let (exit_tx, exit_rx) = futures::channel::oneshot::channel::<()>(); + + { + let (round_rx, round_tx) = bob_network.round_communication( + Some((peers[1].public().into(), bob_keystore).into()), communication::Round(1), communication::SetId(0), Arc::new(VoterSet::new(voters).unwrap()), HasVoted::No, ); - runtime.spawn(network); + runtime.spawn(bob_network); let round_tx = Arc::new(Mutex::new(round_tx)); let exit_tx = Arc::new(Mutex::new(Some(exit_tx))); @@ -1106,13 +1018,16 @@ fn voter_persists_its_votes() { let net = net.clone(); let state = Arc::new(AtomicUsize::new(0)); + let runtime_handle = runtime.handle().clone(); runtime.spawn(round_rx.for_each(move |signed| { let net2 = net.clone(); let net = net.clone(); - let voter_tx = voter_tx.clone(); + let abort = abort.clone(); let round_tx = round_tx.clone(); let state = state.clone(); let exit_tx = exit_tx.clone(); + let runtime_handle = runtime_handle.clone(); + let alice_voter2 = alice_voter2.clone(); async move { if state.compare_and_swap(0, 1, Ordering::SeqCst) == 0 { @@ -1147,7 +1062,8 @@ fn voter_persists_its_votes() { net.lock().peer(0).client().as_full().unwrap().hash(30).unwrap().unwrap(); // we restart alice's voter - voter_tx.unbounded_send(()).unwrap(); + abort.abort(); + runtime_handle.spawn(alice_voter2.lock().take().unwrap()); // and we push our own prevote for block 30 let prevote = finality_grandpa::Prevote { @@ -1194,7 +1110,7 @@ fn voter_persists_its_votes() { } block_until_complete(exit_rx.into_future(), &net, &mut runtime); -}*/ +} #[test] fn finalize_3_voters_1_light_observer() { From 075aa5ed69c48e82838becd5e94f036009f95d28 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 2 Dec 2020 10:53:16 +0100 Subject: [PATCH 09/12] Progress --- client/finality-grandpa/src/tests.rs | 46 +++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 3a32965f709ab..0a3c25af68d73 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -946,7 +946,7 @@ fn voter_persists_its_votes() { let voters = make_ids(peers); // alice has a chain with 20 blocks - let mut net = GrandpaTestNet::new(TestApi::new(voters.clone()), 3); + let mut net = GrandpaTestNet::new(TestApi::new(voters.clone()), 2); // create the communication layer for bob, but don't start any // voter. instead we'll listen for the prevote that alice casts @@ -985,18 +985,48 @@ fn voter_persists_its_votes() { ) }; + // spawn two voters for alice. + // half-way through the test, we stop one and start the other. + let (alice_voter1, abort) = future::abortable(initialize_grandpa(&mut net, &peers[..1])); + let alice_voter2 = Arc::new(Mutex::new(Some({ + let (keystore, _) = create_keystore(peers[0]); + + let net_service = net.peers[0].network_service().clone(); + + let alice_client = net.peer(0).client().clone(); + let (_, _, link) = net + .make_block_import::< + TransactionFor + >(alice_client); + let link = link.lock().take().unwrap(); + + let grandpa_params = GrandpaParams { + config: Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: Some(keystore), + name: Some(format!("peer#{}", 0)), + is_authority: true, + observer_enabled: true, + }, + link, + network: net_service, + telemetry_on_connect: None, + voting_rule: (), + prometheus_registry: None, + shared_voter_state: SharedVoterState::empty(), + }; + run_grandpa_voter(grandpa_params).expect("all in order with client and network") + }))); + runtime.spawn(alice_voter1); + + net.peer(0).push_blocks(20, false); net.block_until_sync(); assert_eq!(net.peer(0).client().info().best_number, 20, "Peer #{} failed to sync", 0); - // spawn two voters for alice. - // half-way through the test, we stop one and start the other. - let (alice_voter1, abort) = future::abortable(initialize_grandpa(&mut net, &peers[..1])); - let alice_voter2 = Arc::new(Mutex::new(Some(initialize_grandpa(&mut net, &peers[..1])))); - runtime.spawn(alice_voter1); - let net = Arc::new(Mutex::new(net)); let (exit_tx, exit_rx) = futures::channel::oneshot::channel::<()>(); @@ -1039,7 +1069,7 @@ fn voter_persists_its_votes() { // its chain has 20 blocks and the voter targets 3/4 of the // unfinalized chain, so the vote should be for block 15 - assert!(prevote.target_number == 15); + assert_eq!(prevote.target_number, 15); // we push 20 more blocks to alice's chain net.lock().peer(0).push_blocks(20, false); From aa1583e28d87451f0ce8a6edd63b17b872494c15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 2 Dec 2020 11:06:53 +0000 Subject: [PATCH 10/12] grandpa: fix voter_persists_its_votes test --- client/finality-grandpa/src/tests.rs | 42 ++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 0a3c25af68d73..a9d67454af378 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -300,7 +300,7 @@ fn initialize_grandpa( link, network: net_service, telemetry_on_connect: None, - voting_rule: (), + voting_rule: VotingRulesBuilder::default().build(), prometheus_registry: None, shared_voter_state: SharedVoterState::empty(), }; @@ -988,13 +988,23 @@ fn voter_persists_its_votes() { // spawn two voters for alice. // half-way through the test, we stop one and start the other. let (alice_voter1, abort) = future::abortable(initialize_grandpa(&mut net, &peers[..1])); - let alice_voter2 = Arc::new(Mutex::new(Some({ + fn alice_voter2( + peers: &[Ed25519Keyring], + net: Arc>, + ) -> impl Future + Unpin + Send + 'static { let (keystore, _) = create_keystore(peers[0]); - - let net_service = net.peers[0].network_service().clone(); - + let mut net = net.lock(); + + // we add a new peer to the test network and we'll use + // the network service of this new peer + net.add_full_peer(); + let net_service = net.peers[2].network_service().clone(); + // but we'll reuse the client from the first peer (alice_voter1) + // since we want to share the same database, so that we can + // read the persisted state after aborting alice_voter1. let alice_client = net.peer(0).client().clone(); - let (_, _, link) = net + + let (_block_import, _, link) = net .make_block_import::< TransactionFor >(alice_client); @@ -1012,14 +1022,23 @@ fn voter_persists_its_votes() { link, network: net_service, telemetry_on_connect: None, - voting_rule: (), + voting_rule: VotingRulesBuilder::default().build(), prometheus_registry: None, shared_voter_state: SharedVoterState::empty(), }; - run_grandpa_voter(grandpa_params).expect("all in order with client and network") - }))); - runtime.spawn(alice_voter1); + run_grandpa_voter(grandpa_params) + .expect("all in order with client and network") + .map(move |r| { + // we need to keep the block_import alive since it owns the + // sender for the voter commands channel, if that gets dropped + // then the voter will stop + drop(_block_import); + r + }) + }; + + runtime.spawn(alice_voter1); net.peer(0).push_blocks(20, false); net.block_until_sync(); @@ -1057,7 +1076,6 @@ fn voter_persists_its_votes() { let state = state.clone(); let exit_tx = exit_tx.clone(); let runtime_handle = runtime_handle.clone(); - let alice_voter2 = alice_voter2.clone(); async move { if state.compare_and_swap(0, 1, Ordering::SeqCst) == 0 { @@ -1093,7 +1111,7 @@ fn voter_persists_its_votes() { // we restart alice's voter abort.abort(); - runtime_handle.spawn(alice_voter2.lock().take().unwrap()); + runtime_handle.spawn(alice_voter2(peers, net.clone())); // and we push our own prevote for block 30 let prevote = finality_grandpa::Prevote { From 1557ed824c5b4160cc5974df41e610fa8474fbc9 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 2 Dec 2020 13:22:44 +0100 Subject: [PATCH 11/12] Restore other tests --- client/finality-grandpa/src/tests.rs | 35 ++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index a9d67454af378..e3561e518e44d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -300,7 +300,7 @@ fn initialize_grandpa( link, network: net_service, telemetry_on_connect: None, - voting_rule: VotingRulesBuilder::default().build(), + voting_rule: (), prometheus_registry: None, shared_voter_state: SharedVoterState::empty(), }; @@ -987,7 +987,38 @@ fn voter_persists_its_votes() { // spawn two voters for alice. // half-way through the test, we stop one and start the other. - let (alice_voter1, abort) = future::abortable(initialize_grandpa(&mut net, &peers[..1])); + let (alice_voter1, abort) = future::abortable({ + let (keystore, _) = create_keystore(peers[0]); + + let (net_service, link) = { + // temporary needed for some reason + let link = net.peers[0].data.lock().take().expect("link initialized at startup; qed"); + ( + net.peers[0].network_service().clone(), + link, + ) + }; + + let grandpa_params = GrandpaParams { + config: Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: Some(keystore), + name: Some(format!("peer#{}", 0)), + is_authority: true, + observer_enabled: true, + }, + link, + network: net_service, + telemetry_on_connect: None, + voting_rule: VotingRulesBuilder::default().build(), + prometheus_registry: None, + shared_voter_state: SharedVoterState::empty(), + }; + + run_grandpa_voter(grandpa_params).expect("all in order with client and network") + }); + fn alice_voter2( peers: &[Ed25519Keyring], net: Arc>, From 9cd559b49d7b9135b0b02d0b77ce5f14793c4618 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 2 Dec 2020 13:45:23 +0100 Subject: [PATCH 12/12] Try spawn future later --- client/finality-grandpa/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index e3561e518e44d..452b30941de5c 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -790,7 +790,7 @@ fn force_change_to_new_set() { let voters = make_ids(peers_a); let mut net = GrandpaTestNet::new(api, 3); - runtime.spawn(initialize_grandpa(&mut net, peers_a)); + let voters_future = initialize_grandpa(&mut net, peers_a); let net = Arc::new(Mutex::new(net)); net.lock().peer(0).generate_blocks(1, BlockOrigin::File, |builder| { @@ -828,6 +828,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-halves have the wrong AuthoritySet + runtime.spawn(voters_future); run_to_completion(&mut runtime, 25, net, peers_a); }