Move import queue out of sc-network (#12764)

* Move import queue out of `sc-network`

Add supplementary asynchronous API for the import queue which means
it can be run as an independent task and communicated with through
the `ImportQueueService`.

This commit removes removes block and justification imports from
`sc-network` and provides `ChainSync` with a handle to import queue so
it can import blocks and justifications. Polling of the import queue is
moved complete out of `sc-network` and `sc_consensus::Link` is
implemented for `ChainSyncInterfaceHandled` so the import queue
can still influence the syncing process.

* Fix tests

* Apply review comments

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>

* Update client/network/sync/src/lib.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

Co-authored-by: Bastian Köcher <git@kchr.de>

substrate/client/network/src/service/metrics.rs

@@ -53,8 +53,6 @@ pub struct Metrics {
 	pub connections_opened_total: CounterVec<U64>,
 	pub distinct_peers_connections_closed_total: Counter<U64>,
 	pub distinct_peers_connections_opened_total: Counter<U64>,
-	pub import_queue_blocks_submitted: Counter<U64>,
-	pub import_queue_justifications_submitted: Counter<U64>,
 	pub incoming_connections_errors_total: CounterVec<U64>,
 	pub incoming_connections_total: Counter<U64>,
 	pub issued_light_requests: Counter<U64>,
@@ -103,14 +101,6 @@ impl Metrics {
 					"substrate_sub_libp2p_distinct_peers_connections_opened_total",
 					"Total number of connections opened with distinct peers"
 			)?, registry)?,
-			import_queue_blocks_submitted: prometheus::register(Counter::new(
-				"substrate_import_queue_blocks_submitted",
-				"Number of blocks submitted to the import queue.",
-			)?, registry)?,
-			import_queue_justifications_submitted: prometheus::register(Counter::new(
-				"substrate_import_queue_justifications_submitted",
-				"Number of justifications submitted to the import queue.",
-			)?, registry)?,
 			incoming_connections_errors_total: prometheus::register(CounterVec::new(
 				Opts::new(
 					"substrate_sub_libp2p_incoming_connections_handshake_errors_total",

substrate/client/network/src/service/tests/chain_sync.rs

@@ -86,27 +86,26 @@ async fn normal_network_poll_no_peers() {
 
 #[tokio::test]
 async fn request_justification() {
-	// build `ChainSyncInterface` provider and set no expecations for it (i.e., it cannot be
-	// called)
-	let chain_sync_service =
-		Box::new(MockChainSyncInterface::<substrate_test_runtime_client::runtime::Block>::new());
-
-	// build `ChainSync` and verify that call to `request_justification()` is made
-	let mut chain_sync =
-		Box::new(MockChainSync::<substrate_test_runtime_client::runtime::Block>::new());
-
 	let hash = H256::random();
 	let number = 1337u64;
 
-	chain_sync
-		.expect_request_justification()
+	// build `ChainSyncInterface` provider and and expect
+	// `JustificationSyncLink::request_justification() to be called once
+	let mut chain_sync_service =
+		Box::new(MockChainSyncInterface::<substrate_test_runtime_client::runtime::Block>::new());
+
+	chain_sync_service
+		.expect_justification_sync_link_request_justification()
 		.withf(move |in_hash, in_number| &hash == in_hash && &number == in_number)
 		.once()
 		.returning(|_, _| ());
 
+	// build `ChainSync` and set default expecations for it
+	let mut chain_sync = MockChainSync::<substrate_test_runtime_client::runtime::Block>::new();
+
 	set_default_expecations_no_peers(&mut chain_sync);
 	let mut network = TestNetworkBuilder::new(Handle::current())
-		.with_chain_sync((chain_sync, chain_sync_service))
+		.with_chain_sync((Box::new(chain_sync), chain_sync_service))
 		.build();
 
 	// send "request justifiction" message and poll the network
@@ -121,17 +120,20 @@ async fn request_justification() {
 
 #[tokio::test]
 async fn clear_justification_requests() {
-	// build `ChainSyncInterface` provider and set no expecations for it (i.e., it cannot be
-	// called)
-	let chain_sync_service =
+	// build `ChainSyncInterface` provider and expect
+	// `JustificationSyncLink::clear_justification_requests()` to be called
+	let mut chain_sync_service =
 		Box::new(MockChainSyncInterface::<substrate_test_runtime_client::runtime::Block>::new());
 
-	// build `ChainSync` and verify that call to `clear_justification_requests()` is made
+	chain_sync_service
+		.expect_justification_sync_link_clear_justification_requests()
+		.once()
+		.returning(|| ());
+
+	// build `ChainSync` and set default expecations for it
 	let mut chain_sync =
 		Box::new(MockChainSync::<substrate_test_runtime_client::runtime::Block>::new());
 
-	chain_sync.expect_clear_justification_requests().once().returning(|| ());
-
 	set_default_expecations_no_peers(&mut chain_sync);
 	let mut network = TestNetworkBuilder::new(Handle::current())
 		.with_chain_sync((chain_sync, chain_sync_service))
@@ -235,19 +237,13 @@ async fn on_block_finalized() {
 // and verify that connection to the peer is closed
 #[tokio::test]
 async fn invalid_justification_imported() {
-	struct DummyImportQueue(
-		Arc<
-			RwLock<
-				Option<(
-					PeerId,
-					substrate_test_runtime_client::runtime::Hash,
-					sp_runtime::traits::NumberFor<substrate_test_runtime_client::runtime::Block>,
-				)>,
-			>,
-		>,
-	);
+	struct DummyImportQueueHandle;
 
-	impl sc_consensus::ImportQueue<substrate_test_runtime_client::runtime::Block> for DummyImportQueue {
+	impl
+		sc_consensus::import_queue::ImportQueueService<
+			substrate_test_runtime_client::runtime::Block,
+		> for DummyImportQueueHandle
+	{
 		fn import_blocks(
 			&mut self,
 			_origin: sp_consensus::BlockOrigin,
@@ -265,7 +261,23 @@ async fn invalid_justification_imported() {
 			_justifications: sp_runtime::Justifications,
 		) {
 		}
+	}
 
+	struct DummyImportQueue(
+		Arc<
+			RwLock<
+				Option<(
+					PeerId,
+					substrate_test_runtime_client::runtime::Hash,
+					sp_runtime::traits::NumberFor<substrate_test_runtime_client::runtime::Block>,
+				)>,
+			>,
+		>,
+		DummyImportQueueHandle,
+	);
+
+	#[async_trait::async_trait]
+	impl sc_consensus::ImportQueue<substrate_test_runtime_client::runtime::Block> for DummyImportQueue {
 		fn poll_actions(
 			&mut self,
 			_cx: &mut futures::task::Context,
@@ -275,13 +287,40 @@ async fn invalid_justification_imported() {
 				link.justification_imported(peer, &hash, number, false);
 			}
 		}
+
+		fn service(
+			&self,
+		) -> Box<
+			dyn sc_consensus::import_queue::ImportQueueService<
+				substrate_test_runtime_client::runtime::Block,
+			>,
+		> {
+			Box::new(DummyImportQueueHandle {})
+		}
+
+		fn service_ref(
+			&mut self,
+		) -> &mut dyn sc_consensus::import_queue::ImportQueueService<
+			substrate_test_runtime_client::runtime::Block,
+		> {
+			&mut self.1
+		}
+
+		async fn run(
+			self,
+			_link: Box<dyn sc_consensus::Link<substrate_test_runtime_client::runtime::Block>>,
+		) {
+		}
 	}
 
 	let justification_info = Arc::new(RwLock::new(None));
 	let listen_addr = config::build_multiaddr![Memory(rand::random::<u64>())];
 
 	let (service1, mut event_stream1) = TestNetworkBuilder::new(Handle::current())
-		.with_import_queue(Box::new(DummyImportQueue(justification_info.clone())))
+		.with_import_queue(Box::new(DummyImportQueue(
+			justification_info.clone(),
+			DummyImportQueueHandle {},
+		)))
 		.with_listen_addresses(vec![listen_addr.clone()])
 		.build()
 		.start_network();
@@ -331,6 +370,7 @@ async fn disconnect_peer_using_chain_sync_handle() {
 	let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0);
 	let listen_addr = config::build_multiaddr![Memory(rand::random::<u64>())];
 
+	let import_queue = Box::new(sc_consensus::import_queue::mock::MockImportQueueHandle::new());
 	let (chain_sync_network_provider, chain_sync_network_handle) =
 		sc_network_sync::service::network::NetworkServiceProvider::new();
 	let handle_clone = chain_sync_network_handle.clone();
@@ -344,7 +384,9 @@ async fn disconnect_peer_using_chain_sync_handle() {
 		Box::new(sp_consensus::block_validation::DefaultBlockAnnounceValidator),
 		1u32,
 		None,
+		None,
 		chain_sync_network_handle.clone(),
+		import_queue,
 		ProtocolName::from("block-request"),
 		ProtocolName::from("state-request"),
 		None,
@@ -353,7 +395,7 @@ async fn disconnect_peer_using_chain_sync_handle() {
 
 	let (node1, mut event_stream1) = TestNetworkBuilder::new(Handle::current())
 		.with_listen_addresses(vec![listen_addr.clone()])
-		.with_chain_sync((Box::new(chain_sync), chain_sync_service))
+		.with_chain_sync((Box::new(chain_sync), Box::new(chain_sync_service)))
 		.with_chain_sync_network((chain_sync_network_provider, chain_sync_network_handle))
 		.with_client(client.clone())
 		.build()

substrate/client/network/src/service/tests/mod.rs

@@ -21,7 +21,7 @@ use crate::{config, ChainSyncInterface, NetworkService, NetworkWorker};
 use futures::prelude::*;
 use libp2p::Multiaddr;
 use sc_client_api::{BlockBackend, HeaderBackend};
-use sc_consensus::ImportQueue;
+use sc_consensus::{ImportQueue, Link};
 use sc_network_common::{
 	config::{
 		NonDefaultSetConfig, NonReservedPeerMode, NotificationHandshake, ProtocolId, SetConfig,
@@ -93,6 +93,7 @@ impl TestNetwork {
 
 struct TestNetworkBuilder {
 	import_queue: Option<Box<dyn ImportQueue<TestBlock>>>,
+	link: Option<Box<dyn Link<TestBlock>>>,
 	client: Option<Arc<substrate_test_runtime_client::TestClient>>,
 	listen_addresses: Vec<Multiaddr>,
 	set_config: Option<SetConfig>,
@@ -106,6 +107,7 @@ impl TestNetworkBuilder {
 	pub fn new(rt_handle: Handle) -> Self {
 		Self {
 			import_queue: None,
+			link: None,
 			client: None,
 			listen_addresses: Vec::new(),
 			set_config: None,
@@ -212,13 +214,14 @@ impl TestNetworkBuilder {
 			}
 		}
 
-		let import_queue = self.import_queue.unwrap_or(Box::new(sc_consensus::BasicQueue::new(
-			PassThroughVerifier(false),
-			Box::new(client.clone()),
-			None,
-			&sp_core::testing::TaskExecutor::new(),
-			None,
-		)));
+		let mut import_queue =
+			self.import_queue.unwrap_or(Box::new(sc_consensus::BasicQueue::new(
+				PassThroughVerifier(false),
+				Box::new(client.clone()),
+				None,
+				&sp_core::testing::TaskExecutor::new(),
+				None,
+			)));
 
 		let protocol_id = ProtocolId::from("test-protocol-name");
 		let fork_id = Some(String::from("test-fork-id"));
@@ -289,15 +292,23 @@ impl TestNetworkBuilder {
 				Box::new(sp_consensus::block_validation::DefaultBlockAnnounceValidator),
 				network_config.max_parallel_downloads,
 				None,
+				None,
 				chain_sync_network_handle,
+				import_queue.service(),
 				block_request_protocol_config.name.clone(),
 				state_request_protocol_config.name.clone(),
 				None,
 			)
 			.unwrap();
 
-			(Box::new(chain_sync), chain_sync_service)
+			if let None = self.link {
+				self.link = Some(Box::new(chain_sync_service.clone()));
+			}
+			(Box::new(chain_sync), Box::new(chain_sync_service))
 		});
+		let mut link = self
+			.link
+			.unwrap_or(Box::new(sc_network_sync::service::mock::MockChainSyncInterface::new()));
 
 		let handle = self.rt_handle.clone();
 		let executor = move |f| {
@@ -316,7 +327,6 @@ impl TestNetworkBuilder {
 			chain: client.clone(),
 			protocol_id,
 			fork_id,
-			import_queue,
 			chain_sync,
 			chain_sync_service,
 			metrics_registry: None,
@@ -333,6 +343,16 @@ impl TestNetworkBuilder {
 		self.rt_handle.spawn(async move {
 			let _ = chain_sync_network_provider.run(service).await;
 		});
+		self.rt_handle.spawn(async move {
+			loop {
+				futures::future::poll_fn(|cx| {
+					import_queue.poll_actions(cx, &mut *link);
+					std::task::Poll::Ready(())
+				})
+				.await;
+				tokio::time::sleep(std::time::Duration::from_millis(250)).await;
+			}
+		});
 
 		TestNetwork::new(worker, self.rt_handle)
 	}