Remove sc_network::NetworkService::register_notifications_protocol and partially refactor Grandpa tests (paritytech#7646)

* Remove sc_network::NetworkService::register_notifications_protocol

* Missing calls to .into()

* Wrong crate name

* [WIP] Fix Grandpa tests

* One more passing

* One more. Two to go.

* This one was actually already passing 🎉

* Last one compiles

* Progress

* grandpa: fix voter_persists_its_votes test

* Restore other tests

* Try spawn future later

Co-authored-by: André Silva <[email protected]>

Author: 2 people

bin/node-template/node/src/service.rs

@@ -79,13 +79,15 @@ pub fn new_partial(config: &Configuration) -> Result<sc_service::PartialComponen
 }
 
 /// Builds a new service for a full client.
-pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
+pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError> {
 	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.into());
+
 	let (network, network_status_sinks, system_rpc_tx, network_starter) =
 		sc_service::build_network(sc_service::BuildNetworkParams {
 			config: &config,
@@ -210,19 +212,19 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
 			"grandpa-voter",
 			sc_finality_grandpa::run_grandpa_voter(grandpa_config)?
 		);
-	} else {
-		sc_finality_grandpa::setup_disabled_grandpa(network)?;
 	}
 
 	network_starter.start_network();
 	Ok(task_manager)
 }
 
 /// Builds a new service for a light client.
-pub fn new_light(config: Configuration) -> Result<TaskManager, ServiceError> {
+pub fn new_light(mut config: Configuration) -> Result<TaskManager, ServiceError> {
 	let (client, backend, keystore_container, mut task_manager, on_demand) =
 		sc_service::new_light_parts::<Block, RuntimeApi, Executor>(&config)?;
 
+	config.network.notifications_protocols.push(sc_finality_grandpa::GRANDPA_PROTOCOL_NAME.into());
+
 	let select_chain = sc_consensus::LongestChain::new(backend.clone());
 
 	let transaction_pool = Arc::new(sc_transaction_pool::BasicPool::new_light(

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<Block, FullClient, FullGrandpaBlockImport>,
 		&sc_consensus_babe::BabeLink<Block>,
@@ -178,6 +178,8 @@ pub fn new_full_base(
 
 	let shared_voter_state = rpc_setup;
 
+	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 {
 			config: &config,
@@ -315,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();
@@ -338,14 +338,16 @@ 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<LightClient>,
 	Arc<NetworkService<Block, <Block as BlockT>::Hash>>,
 	Arc<sc_transaction_pool::LightPool<Block, LightClient, sc_network::config::OnDemand<Block>>>
 ), ServiceError> {
 	let (client, backend, keystore_container, mut task_manager, on_demand) =
 		sc_service::new_light_parts::<Block, RuntimeApi, Executor>(&config)?;
 
+	config.network.notifications_protocols.push(grandpa::GRANDPA_PROTOCOL_NAME.into());
+
 	let select_chain = sc_consensus::LongestChain::new(backend.clone());
 
 	let transaction_pool = Arc::new(sc_transaction_pool::BasicPool::new_light(

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.

client/finality-grandpa/src/communication/tests.rs

@@ -62,8 +62,6 @@ impl sc_network_gossip::Network<Block> 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<u8>) {
 		let _ = self.sender.unbounded_send(Event::Announce(block));
 	}

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<Block: BlockT, C, N, SC, VR> {
 	/// A link to the block import worker.
 	pub link: LinkHalf<Block, C, SC>,
 	/// 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<TracingUnboundedReceiver<()>>,
@@ -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<Block: BlockT, N>(network: N) -> Result<(), sp_consensus::Error>
-where
-	N: NetworkT<Block> + 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.