Companion PR for refactoring priority groups (#2095)

* Companion PR for refactoring priority groups

* Fix non reserved node

* Try fix tests

* Missing import

* Fix warning

* Change protocols order

* Fix test

* Renames

* Update syn dependency to make it compile again after merging master

* "Update Substrate"

Co-authored-by: parity-processbot <>

polkadot/node/network/bridge/src/lib.rs

@@ -80,12 +80,28 @@ pub enum WireMessage<M> {
 	ViewUpdate(View),
 }
 
-/// Information about the notifications protocol. Should be used during network configuration
-/// or shortly after startup to register the protocol with the network service.
-pub fn notifications_protocol_info() -> Vec<std::borrow::Cow<'static, str>> {
+/// Information about the extra peers set. Should be used during network configuration
+/// to register the protocol with the network service.
+pub fn peers_sets_info() -> Vec<sc_network::config::NonDefaultSetConfig> {
 	vec![
-		VALIDATION_PROTOCOL_NAME.into(),
-		COLLATION_PROTOCOL_NAME.into(),
+		sc_network::config::NonDefaultSetConfig {
+			notifications_protocol: VALIDATION_PROTOCOL_NAME.into(),
+			set_config: sc_network::config::SetConfig {
+				in_peers: 25,
+				out_peers: 0,
+				reserved_nodes: Vec::new(),
+				non_reserved_mode: sc_network::config::NonReservedPeerMode::Accept,
+			},
+		},
+		sc_network::config::NonDefaultSetConfig {
+			notifications_protocol: COLLATION_PROTOCOL_NAME.into(),
+			set_config: sc_network::config::SetConfig {
+				in_peers: 25,
+				out_peers: 0,
+				reserved_nodes: Vec::new(),
+				non_reserved_mode: sc_network::config::NonReservedPeerMode::Accept,
+			},
+		}
 	]
 }
 
@@ -202,7 +218,7 @@ impl<N, AD> NetworkBridge<N, AD> {
 	/// Create a new network bridge subsystem with underlying network service and authority discovery service.
 	///
 	/// This assumes that the network service has had the notifications protocol for the network
-	/// bridge already registered. See [`notifications_protocol_info`](notifications_protocol_info).
+	/// bridge already registered. See [`peers_sets_info`](peers_sets_info).
 	pub fn new(network_service: N, authority_discovery_service: AD) -> Self {
 		NetworkBridge {
 			network_service,
@@ -300,7 +316,9 @@ fn action_from_network_message(event: Option<NetworkEvent>) -> Action {
 			tracing::info!(target: LOG_TARGET, "Shutting down Network Bridge: underlying event stream concluded");
 			Action::Abort
 		}
-		Some(NetworkEvent::Dht(_)) => Action::Nop,
+		Some(NetworkEvent::Dht(_)) |
+		Some(NetworkEvent::SyncConnected { .. }) |
+		Some(NetworkEvent::SyncDisconnected { .. }) => Action::Nop,
 		Some(NetworkEvent::NotificationStreamOpened { remote, protocol, role }) => {
 			let role = role.into();
 			match protocol {
@@ -755,6 +773,7 @@ mod tests {
 	use futures::channel::mpsc;
 	use futures::executor;
 
+	use std::borrow::Cow;
 	use std::sync::Arc;
 	use std::collections::HashSet;
 	use async_trait::async_trait;
@@ -829,11 +848,11 @@ mod tests {
 
 	#[async_trait]
 	impl validator_discovery::Network for TestNetwork {
-		async fn add_to_priority_group(&mut self, _group_id: String, _multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+		async fn add_peers_to_reserved_set(&mut self, _protocol: Cow<'static, str>, _: HashSet<Multiaddr>) -> Result<(), String> {
 			Ok(())
 		}
 
-		async fn remove_from_priority_group(&mut self, _group_id: String, _multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+		async fn remove_peers_from_reserved_set(&mut self, _protocol: Cow<'static, str>, _: HashSet<Multiaddr>) -> Result<(), String> {
 			Ok(())
 		}
 	}

polkadot/node/network/bridge/src/validator_discovery.rs

@@ -17,6 +17,7 @@
 //! A validator discovery service for the Network Bridge.
 
 use core::marker::PhantomData;
+use std::borrow::Cow;
 use std::collections::{HashSet, HashMap, hash_map};
 use std::sync::Arc;
 
@@ -28,16 +29,15 @@ use sc_authority_discovery::Service as AuthorityDiscoveryService;
 use polkadot_node_network_protocol::PeerId;
 use polkadot_primitives::v1::{AuthorityDiscoveryId, Block, Hash};
 
-const PRIORITY_GROUP: &'static str = "parachain_validators";
 const LOG_TARGET: &str = "validator_discovery";
 
 /// An abstraction over networking for the purposes of validator discovery service.
 #[async_trait]
 pub trait Network: Send + 'static {
 	/// Ask the network to connect to these nodes and not disconnect from them until removed from the priority group.
-	async fn add_to_priority_group(&mut self, group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String>;
+	async fn add_peers_to_reserved_set(&mut self, protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String>;
 	/// Remove the peers from the priority group.
-	async fn remove_from_priority_group(&mut self, group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String>;
+	async fn remove_peers_from_reserved_set(&mut self, protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String>;
 }
 
 /// An abstraction over the authority discovery service.
@@ -51,12 +51,12 @@ pub trait AuthorityDiscovery: Send + 'static {
 
 #[async_trait]
 impl Network for Arc<sc_network::NetworkService<Block, Hash>> {
-	async fn add_to_priority_group(&mut self, group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
-		sc_network::NetworkService::add_to_priority_group(&**self, group_id, multiaddresses).await
+	async fn add_peers_to_reserved_set(&mut self, protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+		sc_network::NetworkService::add_peers_to_reserved_set(&**self, protocol, multiaddresses)
 	}
 
-	async fn remove_from_priority_group(&mut self, group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
-		sc_network::NetworkService::remove_from_priority_group(&**self, group_id, multiaddresses).await
+	async fn remove_peers_from_reserved_set(&mut self, protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+		sc_network::NetworkService::remove_peers_from_reserved_set(&**self, protocol, multiaddresses)
 	}
 }
 
@@ -274,15 +274,28 @@ impl<N: Network, AD: AuthorityDiscovery> Service<N, AD> {
 		}
 
 		// ask the network to connect to these nodes and not disconnect
-		// from them until removed from the priority group
-		if let Err(e) = network_service.add_to_priority_group(
-			PRIORITY_GROUP.to_owned(),
+		// from them until removed from the set
+		if let Err(e) = network_service.add_peers_to_reserved_set(
+			super::COLLATION_PROTOCOL_NAME.into(),
+			multiaddr_to_add.clone(),
+		).await {
+			tracing::warn!(target: LOG_TARGET, err = ?e, "AuthorityDiscoveryService returned an invalid multiaddress");
+		}
+		if let Err(e) = network_service.add_peers_to_reserved_set(
+			super::VALIDATION_PROTOCOL_NAME.into(),
 			multiaddr_to_add,
 		).await {
 			tracing::warn!(target: LOG_TARGET, err = ?e, "AuthorityDiscoveryService returned an invalid multiaddress");
 		}
 		// the addresses are known to be valid
-		let _ = network_service.remove_from_priority_group(PRIORITY_GROUP.to_owned(), multiaddr_to_remove).await;
+		let _ = network_service.remove_peers_from_reserved_set(
+			super::COLLATION_PROTOCOL_NAME.into(),
+			multiaddr_to_remove.clone()
+		).await;
+		let _ = network_service.remove_peers_from_reserved_set(
+			super::VALIDATION_PROTOCOL_NAME.into(),
+			multiaddr_to_remove
+		).await;
 
 		let pending = validator_ids.iter()
 			.cloned()
@@ -339,7 +352,7 @@ mod tests {
 
 	#[derive(Default)]
 	struct TestNetwork {
-		priority_group: HashSet<Multiaddr>,
+		peers_set: HashSet<Multiaddr>,
 	}
 
 	#[derive(Default)]
@@ -367,13 +380,13 @@ mod tests {
 
 	#[async_trait]
 	impl Network for TestNetwork {
-		async fn add_to_priority_group(&mut self, _group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
-			self.priority_group.extend(multiaddresses.into_iter());
+		async fn add_peers_to_reserved_set(&mut self, _protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+			self.peers_set.extend(multiaddresses.into_iter());
 			Ok(())
 		}
 
-		async fn remove_from_priority_group(&mut self, _group_id: String, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
-			self.priority_group.retain(|elem| !multiaddresses.contains(elem));
+		async fn remove_peers_from_reserved_set(&mut self, _protocol: Cow<'static, str>, multiaddresses: HashSet<Multiaddr>) -> Result<(), String> {
+			self.peers_set.retain(|elem| !multiaddresses.contains(elem));
 			Ok(())
 		}
 	}
@@ -570,7 +583,7 @@ mod tests {
 
 			let _ = receiver.next().await.unwrap();
 			assert_eq!(service.non_revoked_discovery_requests.len(), 1);
-			assert_eq!(ns.priority_group.len(), 2);
+			assert_eq!(ns.peers_set.len(), 2);
 
 			// revoke the second request
 			drop(receiver);
@@ -586,7 +599,7 @@ mod tests {
 
 			let _ = receiver.next().await.unwrap();
 			assert_eq!(service.non_revoked_discovery_requests.len(), 1);
-			assert_eq!(ns.priority_group.len(), 1);
+			assert_eq!(ns.peers_set.len(), 1);
 		});
 	}
 

polkadot/node/service/src/lib.rs

@@ -562,9 +562,12 @@ pub fn new_full<RuntimeApi, Executor>(
 
 	let shared_voter_state = rpc_setup;
 
+	// Note: GrandPa is pushed before the Polkadot-specific protocols. This doesn't change
+	// anything in terms of behaviour, but makes the logs more consistent with the other
+	// Substrate nodes.
+	config.network.extra_sets.push(grandpa::grandpa_peers_set_config());
 	#[cfg(feature = "real-overseer")]
-	config.network.notifications_protocols.extend(polkadot_network_bridge::notifications_protocol_info());
-	config.network.notifications_protocols.push(grandpa::GRANDPA_PROTOCOL_NAME.into());
+	config.network.extra_sets.extend(polkadot_network_bridge::peers_sets_info());
 
 	let (network, network_status_sinks, system_rpc_tx, network_starter) =
 		service::build_network(service::BuildNetworkParams {