Rewrite the BasiQueue using channels (#1327)

* use channels to implement basic import queue

* async justification import

* better conditional for is_done in tests

* reword the test for presence of link

* fix conditional

* trace instead of panic when no link present

* reword expectations when sending to importers

* fix

* debug justification import error

* update expectations

* use NumberFor

* nits

* add general description

* move error handling into closure

substrate/core/network/src/test/block_import.rs

@@ -16,55 +16,15 @@
 
 //! Testing block import logic.
 
-use consensus::import_queue::{import_single_block, process_import_result};
-use consensus::import_queue::{AsyncImportQueueData, BasicQueue, BlockImportError, BlockImportResult};
+use consensus::import_queue::{import_single_block, BasicQueue, BlockImportError, BlockImportResult};
 use test_client::{self, TestClient};
 use test_client::runtime::{Block, Hash};
 use runtime_primitives::generic::BlockId;
-use runtime_primitives::traits::NumberFor;
-use std::cell::Cell;
 use super::*;
 
-struct TestLink {
-	imported: Cell<usize>,
-	maintains: Cell<usize>,
-	disconnects: Cell<usize>,
-	restarts: Cell<usize>,
-}
+struct TestLink {}
 
-impl TestLink {
-	fn new() -> TestLink {
-		TestLink {
-			imported: Cell::new(0),
-			maintains: Cell::new(0),
-			disconnects: Cell::new(0),
-			restarts: Cell::new(0),
-		}
-	}
-
-	fn total(&self) -> usize {
-		self.imported.get() + self.maintains.get() + self.disconnects.get() + self.restarts.get()
-	}
-}
-
-impl Link<Block> for TestLink {
-	fn block_imported(&self, _hash: &Hash, _number: NumberFor<Block>) {
-		self.imported.set(self.imported.get() + 1);
-	}
-	fn maintain_sync(&self) {
-		self.maintains.set(self.maintains.get() + 1);
-	}
-	fn useless_peer(&self, _: NodeIndex, _: &str) {
-		self.disconnects.set(self.disconnects.get() + 1);
-	}
-	fn note_useless_and_restart_sync(&self, id: NodeIndex, r: &str) {
-		self.useless_peer(id, r);
-		self.restart();
-	}
-	fn restart(&self) {
-		self.restarts.set(self.restarts.get() + 1);
-	}
-}
+impl Link<Block> for TestLink {}
 
 fn prepare_good_block() -> (client::Client<test_client::Backend, test_client::Executor, Block, test_client::runtime::RuntimeApi>, Hash, u64, IncomingBlock<Block>) {
 	let client = test_client::new();
@@ -85,19 +45,19 @@ fn prepare_good_block() -> (client::Client<test_client::Backend, test_client::Ex
 
 #[test]
 fn import_single_good_block_works() {
-	let (_, hash, number, block) = prepare_good_block();
+	let (_, _hash, number, block) = prepare_good_block();
 	assert_eq!(
 		import_single_block(&test_client::new(), BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))),
-		Ok(BlockImportResult::ImportedUnknown(hash, number))
+		Ok(BlockImportResult::ImportedUnknown(number))
 	);
 }
 
 #[test]
 fn import_single_good_known_block_is_ignored() {
-	let (client, hash, number, block) = prepare_good_block();
+	let (client, _hash, number, block) = prepare_good_block();
 	assert_eq!(
 		import_single_block(&client, BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))),
-		Ok(BlockImportResult::ImportedKnown(hash, number))
+		Ok(BlockImportResult::ImportedKnown(number))
 	);
 }
 
@@ -111,68 +71,13 @@ fn import_single_good_block_without_header_fails() {
 	);
 }
 
-#[test]
-fn process_import_result_works() {
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Ok(BlockImportResult::ImportedKnown(Default::default(), 0))), 1);
-	assert_eq!(link.total(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Ok(BlockImportResult::ImportedKnown(Default::default(), 0))), 1);
-	assert_eq!(link.total(), 1);
-	assert_eq!(link.imported.get(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Ok(BlockImportResult::ImportedUnknown(Default::default(), 0))), 1);
-	assert_eq!(link.total(), 1);
-	assert_eq!(link.imported.get(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Err(BlockImportError::IncompleteHeader(Some(0)))), 0);
-	assert_eq!(link.total(), 2);
-	assert_eq!(link.disconnects.get(), 1);
-	assert_eq!(link.restarts.get(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Err(BlockImportError::UnknownParent)), 0);
-	assert_eq!(link.total(), 1);
-	assert_eq!(link.restarts.get(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Err(BlockImportError::Error)), 0);
-	assert_eq!(link.total(), 1);
-	assert_eq!(link.restarts.get(), 1);
-
-	let link = TestLink::new();
-	assert_eq!(process_import_result::<Block>(&link, Err(BlockImportError::VerificationFailed(Some(0), String::new()))), 0);
-	assert_eq!(link.total(), 2);
-	assert_eq!(link.restarts.get(), 1);
-	assert_eq!(link.disconnects.get(), 1);
-}
-
-#[test]
-fn import_many_blocks_stops_when_stopping() {
-	let (_, _, _, block) = prepare_good_block();
-	let qdata = AsyncImportQueueData::new();
-	let verifier = Arc::new(PassThroughVerifier(true));
-	qdata.stop();
-	let client = test_client::new();
-	assert!(!import_many_blocks(
-		&client,
-		&mut TestLink::new(),
-		Some(&qdata),
-		(BlockOrigin::File, vec![block.clone(), block]),
-		verifier
-	));
-}
-
 #[test]
 fn async_import_queue_drops() {
 	// Perform this test multiple times since it exhibits non-deterministic behavior.
 	for _ in 0..100 {
 		let verifier = Arc::new(PassThroughVerifier(true));
 		let queue = BasicQueue::new(verifier, Arc::new(test_client::new()), None);
-		queue.start(TestLink::new()).unwrap();
+		queue.start(Box::new(TestLink{})).unwrap();
 		drop(queue);
 	}
 }

substrate/core/network/src/test/mod.rs

@@ -23,13 +23,14 @@ mod sync;
 
 use std::collections::{HashMap, HashSet};
 use std::sync::Arc;
+use std::thread;
 use std::time::Duration;
 
 use log::trace;
 use client;
 use client::block_builder::BlockBuilder;
 use crate::config::ProtocolConfig;
-use consensus::import_queue::{import_many_blocks, ImportQueue, ImportQueueStatus, IncomingBlock};
+use consensus::import_queue::{BasicQueue, ImportQueue, IncomingBlock};
 use consensus::import_queue::{Link, SharedBlockImport, SharedJustificationImport, Verifier};
 use consensus::{Error as ConsensusError, ErrorKind as ConsensusErrorKind};
 use consensus::{BlockOrigin, ForkChoiceStrategy, ImportBlock, JustificationImport};
@@ -45,7 +46,7 @@ use parking_lot::Mutex;
 use primitives::{H256, Ed25519AuthorityId};
 use crate::protocol::{Context, Protocol, ProtocolMsg, ProtocolStatus};
 use runtime_primitives::generic::BlockId;
-use runtime_primitives::traits::{AuthorityIdFor, Block as BlockT, Digest, DigestItem, Header, Zero, NumberFor};
+use runtime_primitives::traits::{AuthorityIdFor, Block as BlockT, Digest, DigestItem, Header, NumberFor};
 use runtime_primitives::Justification;
 use crate::service::{network_channel, NetworkChan, NetworkLink, NetworkMsg, NetworkPort, TransactionPool};
 use crate::specialization::NetworkSpecialization;
@@ -54,34 +55,6 @@ use test_client;
 pub use test_client::runtime::{Block, Extrinsic, Hash, Transfer};
 pub use test_client::TestClient;
 
-#[cfg(any(test, feature = "test-helpers"))]
-use std::cell::RefCell;
-
-#[cfg(any(test, feature = "test-helpers"))]
-struct ImportCB<B: BlockT>(RefCell<Option<Box<dyn Fn(BlockOrigin, Vec<IncomingBlock<B>>) -> bool>>>);
-
-#[cfg(any(test, feature = "test-helpers"))]
-impl<B: BlockT> ImportCB<B> {
-	fn new() -> Self {
-		ImportCB(RefCell::new(None))
-	}
-	fn set<F>(&self, cb: Box<F>)
-	where F: 'static + Fn(BlockOrigin, Vec<IncomingBlock<B>>) -> bool,
-	{
-		*self.0.borrow_mut() = Some(cb);
-	}
-	fn call(&self, origin: BlockOrigin, data: Vec<IncomingBlock<B>>) -> bool {
-		let b = self.0.borrow();
-		b.as_ref().expect("The Callback has been set before. qed.")(origin, data)
-	}
-}
-
-#[cfg(any(test, feature = "test-helpers"))]
-unsafe impl<B: BlockT> Send for ImportCB<B> {}
-#[cfg(any(test, feature = "test-helpers"))]
-unsafe impl<B: BlockT> Sync for ImportCB<B> {}
-
-
 #[cfg(any(test, feature = "test-helpers"))]
 /// A Verifier that accepts all blocks and passes them on with the configured
 /// finality to be imported.
@@ -114,101 +87,10 @@ impl<B: BlockT> Verifier<B> for PassThroughVerifier {
 }
 
 /// A link implementation that does nothing.
-pub struct NoopLink;
+pub struct NoopLink { }
 
 impl<B: BlockT> Link<B> for NoopLink { }
 
-/// Blocks import queue that is importing blocks in the same thread.
-/// The boolean value indicates whether blocks should be imported without instant finality.
-#[cfg(any(test, feature = "test-helpers"))]
-pub struct SyncImportQueue<B: BlockT, V: Verifier<B>> {
-	verifier: Arc<V>,
-	link: ImportCB<B>,
-	block_import: SharedBlockImport<B>,
-	justification_import: Option<SharedJustificationImport<B>>,
-}
-
-#[cfg(any(test, feature = "test-helpers"))]
-impl<B: 'static + BlockT, V: 'static + Verifier<B>> SyncImportQueue<B, V> {
-	/// Create a new SyncImportQueue wrapping the given Verifier and block import
-	/// handle.
-	pub fn new(verifier: Arc<V>, block_import: SharedBlockImport<B>, justification_import: Option<SharedJustificationImport<B>>) -> Self {
-		let queue = SyncImportQueue {
-			verifier,
-			link: ImportCB::new(),
-			block_import,
-			justification_import,
-		};
-
-		let v = queue.verifier.clone();
-		let import_handle = queue.block_import.clone();
-		queue.link.set(Box::new(move |origin, new_blocks| {
-			let verifier = v.clone();
-			import_many_blocks(
-				&*import_handle,
-				&NoopLink,
-				None,
-				(origin, new_blocks),
-				verifier,
-			)
-		}));
-
-		queue
-	}
-}
-
-#[cfg(any(test, feature = "test-helpers"))]
-impl<B: 'static + BlockT, V: 'static + Verifier<B>> ImportQueue<B> for SyncImportQueue<B, V>
-{
-	fn start<L: 'static + Link<B>>(
-		&self,
-		link: L,
-	) -> Result<(), std::io::Error> {
-		let v = self.verifier.clone();
-		let import_handle = self.block_import.clone();
-		self.link.set(Box::new(move |origin, new_blocks| {
-			let verifier = v.clone();
-			import_many_blocks(
-				&*import_handle,
-				&link,
-				None,
-				(origin, new_blocks),
-				verifier
-			)
-		}));
-		Ok(())
-	}
-	fn clear(&self) { }
-
-	fn stop(&self) { }
-
-	fn status(&self) -> ImportQueueStatus<B> {
-		ImportQueueStatus {
-			importing_count: 0,
-			best_importing_number: Zero::zero(),
-		}
-	}
-
-	fn is_importing(&self, _hash: &B::Hash) -> bool {
-		false
-	}
-
-	fn import_blocks(&self, origin: BlockOrigin, blocks: Vec<IncomingBlock<B>>) {
-		self.link.call(origin, blocks);
-	}
-
-	fn import_justification(
-		&self,
-		hash: B::Hash,
-		number: NumberFor<B>,
-		justification: Justification,
-	) -> bool {
-		self.justification_import.as_ref().map(|justification_import| {
-			justification_import.import_justification(hash, number, justification).is_ok()
-		}).unwrap_or(false)
-	}
-}
-
 /// The test specialization.
 pub struct DummySpecialization { }
 
@@ -234,19 +116,20 @@ impl NetworkSpecialization<Block> for DummySpecialization {
 
 pub type PeersClient = client::Client<test_client::Backend, test_client::Executor, Block, test_client::runtime::RuntimeApi>;
 
-pub struct Peer<V: 'static + Verifier<Block>, D> {
+pub struct Peer<D> {
 	client: Arc<PeersClient>,
 	pub protocol_sender: Sender<ProtocolMsg<Block, DummySpecialization>>,
+
 	network_port: Mutex<NetworkPort<Block>>,
-	import_queue: Arc<SyncImportQueue<Block, V>>,
+	pub import_queue: Box<ImportQueue<Block>>,
 	network_sender: NetworkChan<Block>,
 	pub data: D,
 }
 
-impl<V: 'static + Verifier<Block>, D> Peer<V, D> {
+impl<D> Peer<D> {
 	fn new(
 		client: Arc<PeersClient>,
-		import_queue: Arc<SyncImportQueue<Block, V>>,
+		import_queue: Box<ImportQueue<Block>>,
 		protocol_sender: Sender<ProtocolMsg<Block, DummySpecialization>>,
 		network_sender: NetworkChan<Block>,
 		network_port: NetworkPort<Block>,
@@ -276,7 +159,7 @@ impl<V: 'static + Verifier<Block>, D> Peer<V, D> {
 			network_sender: self.network_sender.clone(),
 		};
 
-		self.import_queue.start(network_link).expect("Test ImportQueue always starts");
+		self.import_queue.start(Box::new(network_link)).expect("Test ImportQueue always starts");
 		let _ = self
 			.protocol_sender
 			.send(ProtocolMsg::BlockImported(info.chain.best_hash, header));
@@ -327,7 +210,8 @@ impl<V: 'static + Verifier<Block>, D> Peer<V, D> {
 
 	/// Whether this peer is done syncing (has no messages to send).
 	fn is_done(&self) -> bool {
-		self.network_port.lock().receiver().is_empty()
+		self.import_queue.status().importing_count == 0 &&
+			self.network_port.lock().receiver().is_empty()
 	}
 
 	/// Execute a "sync step". This is called for each peer after it sends a packet.
@@ -437,8 +321,6 @@ impl<V: 'static + Verifier<Block>, D> Peer<V, D> {
 			let header = block.header.clone();
 			at = BlockId::Hash(hash);
 
-			// NOTE: if we use a non-synchronous queue in the test-net in the future,
-			// this may not work.
 			self.import_queue.import_blocks(
 				origin,
 				vec![IncomingBlock {
@@ -447,8 +329,12 @@ impl<V: 'static + Verifier<Block>, D> Peer<V, D> {
 					header: Some(header),
 					body: Some(block.extrinsics),
 					justification: None,
-				}
-			]);
+				}],
+			);
+			// Simulate a synchronous import.
+			while self.import_queue.status().importing_count > 0 {
+				thread::sleep(Duration::from_millis(20));
+			}
 		}
 	}
 
@@ -516,9 +402,9 @@ pub trait TestNetFactory: Sized {
 	fn make_verifier(&self, client: Arc<PeersClient>, config: &ProtocolConfig) -> Arc<Self::Verifier>;
 
 	/// Get reference to peer.
-	fn peer(&self, i: usize) -> &Peer<Self::Verifier, Self::PeerData>;
-	fn peers(&self) -> &Vec<Arc<Peer<Self::Verifier, Self::PeerData>>>;
-	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<Self::Verifier, Self::PeerData>>>)>(&mut self, closure: F);
+	fn peer(&self, i: usize) -> &Peer<Self::PeerData>;
+	fn peers(&self) -> &Vec<Arc<Peer<Self::PeerData>>>;
+	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<Self::PeerData>>>)>(&mut self, closure: F);
 
 	fn started(&self) -> bool;
 	fn set_started(&mut self, now: bool);
@@ -553,8 +439,8 @@ pub trait TestNetFactory: Sized {
 		let (block_import, justification_import, data) = self.make_block_import(client.clone());
 		let (network_sender, network_port) = network_channel(ProtocolId::default());
 
-		let import_queue = Arc::new(SyncImportQueue::new(verifier, block_import, justification_import));
-		let specialization = DummySpecialization { };
+		let import_queue = Box::new(BasicQueue::new(verifier, block_import, justification_import));
+		let specialization = DummySpecialization {};
 		let protocol_sender = Protocol::new(
 			network_sender.clone(),
 			config.clone(),
@@ -727,8 +613,8 @@ pub trait TestNetFactory: Sized {
 }
 
 pub struct TestNet {
-	peers: Vec<Arc<Peer<PassThroughVerifier, ()>>>,
-	started: bool
+	peers: Vec<Arc<Peer<()>>>,
+	started: bool,
 }
 
 impl TestNetFactory for TestNet {
@@ -749,15 +635,15 @@ impl TestNetFactory for TestNet {
 		Arc::new(PassThroughVerifier(false))
 	}
 
-	fn peer(&self, i: usize) -> &Peer<Self::Verifier, ()> {
+	fn peer(&self, i: usize) -> &Peer<()> {
 		&self.peers[i]
 	}
 
-	fn peers(&self) -> &Vec<Arc<Peer<Self::Verifier, ()>>> {
+	fn peers(&self) -> &Vec<Arc<Peer<()>>> {
 		&self.peers
 	}
 
-	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<Self::Verifier, ()>>>)>(&mut self, closure: F ) {
+	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<()>>>)>(&mut self, closure: F) {
 		closure(&mut self.peers);
 	}
 
@@ -802,15 +688,15 @@ impl TestNetFactory for JustificationTestNet {
 		self.0.make_verifier(client, config)
 	}
 
-	fn peer(&self, i: usize) -> &Peer<Self::Verifier, ()> {
+	fn peer(&self, i: usize) -> &Peer<Self::PeerData> {
 		self.0.peer(i)
 	}
 
-	fn peers(&self) -> &Vec<Arc<Peer<Self::Verifier, ()>>> {
+	fn peers(&self) -> &Vec<Arc<Peer<Self::PeerData>>> {
 		self.0.peers()
 	}
 
-	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<Self::Verifier, ()>>>)>(&mut self, closure: F ) {
+	fn mut_peers<F: Fn(&mut Vec<Arc<Peer<Self::PeerData>>>)>(&mut self, closure: F ) {
 		self.0.mut_peers(closure)
 	}
 

substrate/core/network/src/test/sync.rs

@@ -20,7 +20,6 @@ use crate::config::Roles;
 use consensus::BlockOrigin;
 use network_libp2p::NodeIndex;
 use crate::sync::SyncState;
-use std::{thread, time};
 use std::collections::HashSet;
 use super::*;
 
@@ -55,11 +54,6 @@ fn sync_long_chain_works() {
 	let mut net = TestNet::new(2);
 	net.peer(1).push_blocks(500, false);
 	net.sync();
-	// Wait for peer 0 to import blocks received over the network.
-	thread::sleep(time::Duration::from_millis(1000));
-	net.sync();
-	// Wait for peers to get up to speed.
-	thread::sleep(time::Duration::from_millis(1000));
 	assert!(net.peer(0).client.backend().as_in_memory().blockchain()
 		.equals_to(net.peer(1).client.backend().as_in_memory().blockchain()));
 }
@@ -149,6 +143,7 @@ fn own_blocks_are_announced() {
 	let header = net.peer(0).client().header(&BlockId::Number(1)).unwrap().unwrap();
 	net.peer(0).on_block_imported(header.hash(), &header);
 	net.sync();
+
 	assert_eq!(net.peer(0).client.backend().blockchain().info().unwrap().best_number, 1);
 	assert_eq!(net.peer(1).client.backend().blockchain().info().unwrap().best_number, 1);
 	let peer0_chain = net.peer(0).client.backend().as_in_memory().blockchain().clone();