client/network: Pass protocol config instead of protocol name

client/network/src/behaviour.rs

@@ -165,12 +165,18 @@ impl<B: BlockT, H: ExHashT> Behaviour<B, H> {
 		local_public_key: PublicKey,
 		light_client_handler: light_client_handler::LightClientHandler<B>,
 		disco_config: DiscoveryConfig,
-		request_response_protocols: Vec<request_responses::ProtocolConfig>,
+		// TODO: Document that these are all others apart from block and finality.
+		mut request_response_protocols: Vec<request_responses::ProtocolConfig>,
 
 		// TODO: Not so fancy, see definition.
-		block_request_protocol_name: String,
-		finality_request_protocol_name: String,
+		block_request_protocol_config: request_responses::ProtocolConfig,
+		finality_request_protocol_config: request_responses::ProtocolConfig,
 	) -> Result<Self, request_responses::RegisterError> {
+		let block_request_protocol_name = block_request_protocol_config.name.to_string();
+		let finality_request_protocol_name = finality_request_protocol_config.name.to_string();
+		request_response_protocols.push(block_request_protocol_config);
+		request_response_protocols.push(finality_request_protocol_config);
+
 		Ok(Behaviour {
 			substrate,
 			peer_info: peer_info::PeerInfoBehaviour::new(user_agent, local_public_key),

client/network/src/block_request_handler.rs

@@ -38,8 +38,19 @@ const LOG_TARGET: &str = "block-request-handler";
 const MAX_BLOCKS_IN_RESPONSE: usize = 128;
 const MAX_BODY_BYTES: usize = 8 * 1024 * 1024;
 
+// TODO: Document that this is for clients not reponding only.
+pub fn generate_protocol_config(protocol_id: ProtocolId) -> ProtocolConfig {
+	ProtocolConfig {
+		name: generate_protocol_name(protocol_id).into(),
+		max_request_size: 1024 * 1024,
+		max_response_size: 16 * 1024 * 1024,
+		request_timeout: Duration::from_secs(40),
+		inbound_queue: None,
+	}
+}
+
 /// Generate the block protocol name from chain specific protocol identifier.
-pub fn generate_protocol_name(protocol_id: ProtocolId) -> String {
+fn generate_protocol_name(protocol_id: ProtocolId) -> String {
 	let mut s = String::new();
 	s.push_str("/");
 	s.push_str(protocol_id.as_ref());
@@ -59,13 +70,8 @@ impl <B: BlockT> BlockRequestHandler<B> {
 		// TODO: Likeley we want to allow more than 0 buffered requests. Rethink this value.
 		let (tx, request_receiver) = mpsc::channel(0);
 
-		let protocol_config = ProtocolConfig {
-			name: generate_protocol_name(protocol_id).into(),
-			max_request_size: 1024 * 1024,
-			max_response_size: 16 * 1024 * 1024,
-			request_timeout: Duration::from_secs(40),
-			inbound_queue: Some(tx),
-		};
+		let mut protocol_config = generate_protocol_config(protocol_id);
+		protocol_config.inbound_queue = Some(tx);
 
 		(Self { client, request_receiver }, protocol_config)
 	}

client/network/src/config.rs

@@ -100,6 +100,10 @@ pub struct Params<B: BlockT, H: ExHashT> {
 
 	/// Registry for recording prometheus metrics to.
 	pub metrics_registry: Option<Registry>,
+
+	// TODO: Dcoument that you can only get these by creating a Protocol config or creating a whole handler.
+	pub block_request_protocol_config: RequestResponseConfig,
+	pub finality_request_protocol_config: RequestResponseConfig,
 }
 
 /// Role of the local node.

client/network/src/finality_request_handler.rs

@@ -32,8 +32,20 @@ use std::time::Duration;
 
 const LOG_TARGET: &str = "finality-request-handler";
 
+// TODO: Document that this is for clients not reponding only.
+pub fn generate_protocol_config(protocol_id: ProtocolId) -> ProtocolConfig {
+	ProtocolConfig {
+		name: generate_protocol_name(protocol_id).into(),
+		max_request_size: 1024 * 1024,
+		max_response_size: 1024 * 1024,
+		// TODO: What is a sane value here?
+		request_timeout: Duration::from_secs(10),
+		inbound_queue: None,
+	}
+}
+
 /// Generate the finality proof protocol name from chain specific protocol identifier.
-pub fn generate_protocol_name(protocol_id: ProtocolId) -> String {
+fn generate_protocol_name(protocol_id: ProtocolId) -> String {
 	let mut s = String::new();
 	s.push_str("/");
 	s.push_str(protocol_id.as_ref());
@@ -56,14 +68,8 @@ impl<B: BlockT> FinalityRequestHandler<B> {
 		// TODO: Likeley we want to allow more than 0 buffered requests. Rethink this value.
 		let (tx, rx) = mpsc::channel(0);
 
-		let protocol_config = ProtocolConfig {
-			name: generate_protocol_name(protocol_id).into(),
-			max_request_size: 1024 * 1024,
-			max_response_size: 1024 * 1024,
-			// TODO: What is a sane value here?
-			request_timeout: Duration::from_secs(10),
-			inbound_queue: Some(tx),
-		};
+		let mut protocol_config = generate_protocol_config(protocol_id);
+		protocol_config.inbound_queue = Some(tx);
 
 		let handler = FinalityRequestHandler {
 			proof_provider,

client/network/src/gossip/tests.rs

@@ -33,7 +33,7 @@ type TestNetworkService = NetworkService<
 ///
 /// > **Note**: We return the events stream in order to not possibly lose events between the
 /// >			construction of the service and the moment the events stream is grabbed.
-fn build_test_full_node(config: config::NetworkConfiguration)
+fn build_test_full_node(mut network_config: config::NetworkConfiguration)
 	-> (Arc<TestNetworkService>, impl Stream<Item = Event>)
 {
 	let client = Arc::new(
@@ -91,10 +91,29 @@ fn build_test_full_node(config: config::NetworkConfiguration)
 		None,
 	));
 
+	let protocol_id = config::ProtocolId::from("/test-protocol-name");
+
+	// Add block request handler.
+	let block_request_protocol_name = {
+		let (handler, protocol_config) = crate::block_request_handler::BlockRequestHandler::new(protocol_id.clone(), client.clone());
+		let name = protocol_config.name.to_string();
+		network_config.request_response_protocols.push(protocol_config);
+		async_std::task::spawn(handler.run().boxed());
+		name
+	};
+
+	// Add finality protocol.
+	let finality_request_protocol_name = {
+		let protocol_config = crate::finality_request_handler::generate_protocol_config(protocol_id.clone());
+		let name = protocol_config.name.to_string();
+		network_config.request_response_protocols.push(protocol_config);
+		name
+	};
+
 	let worker = NetworkWorker::new(config::Params {
 		role: config::Role::Full,
 		executor: None,
-		network_config: config,
+		network_config,
 		chain: client.clone(),
 		finality_proof_request_builder: None,
 		on_demand: None,
@@ -105,6 +124,8 @@ fn build_test_full_node(config: config::NetworkConfiguration)
 			sp_consensus::block_validation::DefaultBlockAnnounceValidator,
 		),
 		metrics_registry: None,
+		block_request_protocol_name,
+		finality_request_protocol_name,
 	})
 	.unwrap();
 

client/network/src/service.rs

@@ -309,8 +309,8 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
 					light_client_handler,
 					discovery_config,
 					params.network_config.request_response_protocols,
-					crate::block_request_handler::generate_protocol_name(params.protocol_id.clone()),
-					crate::finality_request_handler::generate_protocol_name(params.protocol_id.clone()),
+					params.block_request_protocol_config,
+					params.finality_request_protocol_config,
 				);
 
 				match result {

client/network/src/service/tests.rs

@@ -92,20 +92,41 @@ fn build_test_full_node(config: config::NetworkConfiguration)
 		None,
 	));
 
+	let protocol_id = config::ProtocolId::from("/test-protocol-name");
+
+	// Add block request handler.
+	let block_request_protocol_name = {
+		let (handler, protocol_config) = crate::block_request_handler::BlockRequestHandler::new(protocol_id.clone(), client.clone());
+		let name = protocol_config.name.to_string();
+		network_config.request_response_protocols.push(protocol_config);
+		async_std::task::spawn(handler.run().boxed());
+		name
+	};
+
+	// Add finality protocol.
+	let finality_request_protocol_name = {
+		let protocol_config = crate::finality_request_handler::generate_protocol_config(protocol_id.clone());
+		let name = protocol_config.name.to_string();
+		network_config.request_response_protocols.push(protocol_config);
+		name
+	};
+
 	let worker = NetworkWorker::new(config::Params {
 		role: config::Role::Full,
 		executor: None,
-		network_config: config,
+		network_config,
 		chain: client.clone(),
 		finality_proof_request_builder: None,
 		on_demand: None,
 		transaction_pool: Arc::new(crate::config::EmptyTransactionPool),
-		protocol_id: config::ProtocolId::from("/test-protocol-name"),
+		protocol_id,
 		import_queue,
 		block_announce_validator: Box::new(
 			sp_consensus::block_validation::DefaultBlockAnnounceValidator,
 		),
 		metrics_registry: None,
+		block_request_protocol_name,
+		finality_request_protocol_name,
 	})
 	.unwrap();
 

client/network/test/Cargo.toml

@@ -13,6 +13,7 @@ repository = "https://github.com/paritytech/substrate/"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+async-std = "1.6.5"
 sc-network = { version = "0.8.0", path = "../" }
 log = "0.4.8"
 parking_lot = "0.10.0"

client/network/test/src/lib.rs

@@ -30,6 +30,8 @@ use std::{
 use libp2p::build_multiaddr;
 use log::trace;
 use sc_network::config::FinalityProofProvider;
+use sc_network::block_request_handler::BlockRequestHandler;
+use sc_network::finality_request_handler::FinalityRequestHandler;
 use sp_blockchain::{
 	HeaderBackend, Result as ClientResult,
 	well_known_cache_keys::{self, Id as CacheKeyId},
@@ -50,6 +52,7 @@ use sp_consensus::block_import::{BlockImport, ImportResult};
 use sp_consensus::Error as ConsensusError;
 use sp_consensus::{BlockOrigin, ForkChoiceStrategy, BlockImportParams, BlockCheckParams, JustificationImport};
 use futures::prelude::*;
+use futures::future::BoxFuture;
 use sc_network::{NetworkWorker, NetworkService, config::ProtocolId};
 use sc_network::config::{NetworkConfiguration, TransportConfig, BoxFinalityProofRequestBuilder};
 use libp2p::PeerId;
@@ -670,22 +673,42 @@ pub trait TestNetFactory: Sized {
 		network_config.allow_non_globals_in_dht = true;
 		network_config.notifications_protocols = config.notifications_protocols;
 
+		let protocol_id = ProtocolId::from("test-protocol-name");
+
+		// Add block request handler.
+		let block_request_protocol_config = {
+			let (handler, protocol_config) = BlockRequestHandler::new(protocol_id.clone(), client.clone());
+			self.spawn_task(handler.run().boxed());
+			protocol_config
+		};
+
+		// Add finality request handler.
+		let finality_request_protocol_config = match self.make_finality_proof_provider(PeersClient::Full(client.clone(), backend.clone())) {
+			Some(provider) => {
+				let (handler, protocol_config) = FinalityRequestHandler::new(protocol_id.clone(), provider);
+				self.spawn_task(handler.run().boxed());
+				protocol_config
+			},
+			None => {
+				sc_network::finality_request_handler::generate_protocol_config(protocol_id.clone())
+			}
+		};
+
 		let network = NetworkWorker::new(sc_network::config::Params {
 			role: Role::Full,
 			executor: None,
 			network_config,
 			chain: client.clone(),
-			finality_proof_provider: self.make_finality_proof_provider(
-				PeersClient::Full(client.clone(), backend.clone()),
-			),
 			finality_proof_request_builder,
 			on_demand: None,
 			transaction_pool: Arc::new(EmptyTransactionPool),
-			protocol_id: ProtocolId::from("test-protocol-name"),
+			protocol_id,
 			import_queue,
 			block_announce_validator: config.block_announce_validator
 				.unwrap_or_else(|| Box::new(DefaultBlockAnnounceValidator)),
 			metrics_registry: None,
+			block_request_protocol_config,
+			finality_request_protocol_config,
 		}).unwrap();
 
 		self.mut_peers(|peers| {
@@ -750,21 +773,28 @@ pub trait TestNetFactory: Sized {
 		network_config.listen_addresses = vec![listen_addr.clone()];
 		network_config.allow_non_globals_in_dht = true;
 
+		let protocol_id = ProtocolId::from("test-protocol-name");
+
+		// Add block request handler.
+		let block_request_protocol_config = sc_network::block_request_handler::generate_protocol_config(protocol_id.clone());
+
+		// Add finality request handler.
+		let finality_request_protocol_config = sc_network::finality_request_handler::generate_protocol_config(protocol_id.clone());
+
 		let network = NetworkWorker::new(sc_network::config::Params {
 			role: Role::Light,
 			executor: None,
 			network_config,
 			chain: client.clone(),
-			finality_proof_provider: self.make_finality_proof_provider(
-				PeersClient::Light(client.clone(), backend.clone())
-			),
 			finality_proof_request_builder,
 			on_demand: None,
 			transaction_pool: Arc::new(EmptyTransactionPool),
-			protocol_id: ProtocolId::from("test-protocol-name"),
+			protocol_id,
 			import_queue,
 			block_announce_validator: Box::new(DefaultBlockAnnounceValidator),
 			metrics_registry: None,
+			block_request_protocol_config,
+			finality_request_protocol_config,
 		}).unwrap();
 
 		self.mut_peers(|peers| {
@@ -789,6 +819,11 @@ pub trait TestNetFactory: Sized {
 		});
 	}
 
+	// TODO: Not ideal. Can we do better?
+	fn spawn_task(&self, f: BoxFuture<'static, ()>) {
+		async_std::task::spawn(f);
+	}
+
 	/// Polls the testnet until all nodes are in sync.
 	///
 	/// Must be executed in a task context.

client/service/src/builder.rs

@@ -871,19 +871,24 @@ pub fn build_network<TBl, TExPool, TImpQu, TCl>(
 		Box::new(DefaultBlockAnnounceValidator)
 	};
 
-	let mut network_config = config.network.clone();
-
-	if let Some(proof_provider) = finality_proof_provider {
-		let (handler, protocol_config) = sc_network::finality_request_handler::FinalityRequestHandler::new(protocol_id.clone(), proof_provider);
-		network_config.request_response_protocols.push(protocol_config);
-		spawn_handle.spawn("finality_request_handler", handler.run());
-	}
-
-	{
+	let block_request_protocol_config = {
+		// TODO: Only do this if we are not a light client.
 		let (handler, protocol_config) = sc_network::block_request_handler::BlockRequestHandler::new(protocol_id.clone(), client.clone());
-		network_config.request_response_protocols.push(protocol_config);
 		spawn_handle.spawn("block_request_handler", handler.run());
-	}
+		protocol_config
+	};
+
+	let finality_request_protocol_config = match finality_proof_provider {
+		Some(provider) => {
+			// TODO: Only do this if we are not a light client.
+			let (handler, protocol_config) = sc_network::finality_request_handler::FinalityRequestHandler::new(protocol_id.clone(), provider);
+			spawn_handle.spawn("finality_request_handler", handler.run());
+			protocol_config
+		},
+		None => {
+			sc_network::finality_request_handler::generate_protocol_config(protocol_id.clone())
+		}
+	};
 
 	let network_params = sc_network::config::Params {
 		role: config.role.clone(),
@@ -893,15 +898,17 @@ pub fn build_network<TBl, TExPool, TImpQu, TCl>(
 				spawn_handle.spawn("libp2p-node", fut);
 			}))
 		},
-		network_config,
+		network_config: config.network.clone(),
 		chain: client.clone(),
 		finality_proof_request_builder,
 		on_demand: on_demand,
 		transaction_pool: transaction_pool_adapter as _,
 		import_queue: Box::new(import_queue),
 		protocol_id,
 		block_announce_validator,
-		metrics_registry: config.prometheus_config.as_ref().map(|config| config.registry.clone())
+		metrics_registry: config.prometheus_config.as_ref().map(|config| config.registry.clone()),
+		block_request_protocol_config,
+		finality_request_protocol_config,
 	};
 
 	let has_bootnodes = !network_params.network_config.boot_nodes.is_empty();