Change the import queue traits to take &mut self instead of &self (#3058)

* SharedFinalityProofRequestBuilder -> BoxFinalityProofRequestBuilder

* SharedThings -> BoxThings

* Fix tests

* build_request_data now takes &mut self

* The other traits now also take &mut self

* More or less fix tests

* Fix tests

* Fix more tests

* Moar tests

* Don't call make_block_import multiple time

* Fix doctest

substrate/core/finality-grandpa/src/import.rs

@@ -63,6 +63,21 @@ pub struct GrandpaBlockImport<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> {
 	api: Arc<PRA>,
 }
 
+impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC: Clone> Clone for
+	GrandpaBlockImport<B, E, Block, RA, PRA, SC>
+{
+	fn clone(&self) -> Self {
+		GrandpaBlockImport {
+			inner: self.inner.clone(),
+			select_chain: self.select_chain.clone(),
+			authority_set: self.authority_set.clone(),
+			send_voter_commands: self.send_voter_commands.clone(),
+			consensus_changes: self.consensus_changes.clone(),
+			api: self.api.clone(),
+		}
+	}
+}
+
 impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> JustificationImport<Block>
 	for GrandpaBlockImport<B, E, Block, RA, PRA, SC> where
 		NumberFor<Block>: grandpa::BlockNumberOps,
@@ -76,7 +91,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> JustificationImport<Block>
 {
 	type Error = ConsensusError;
 
-	fn on_start(&self) -> Vec<(Block::Hash, NumberFor<Block>)> {
+	fn on_start(&mut self) -> Vec<(Block::Hash, NumberFor<Block>)> {
 		let mut out = Vec::new();
 		let chain_info = self.inner.info().chain;
 
@@ -106,7 +121,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> JustificationImport<Block>
 	}
 
 	fn import_justification(
-		&self,
+		&mut self,
 		hash: Block::Hash,
 		number: NumberFor<Block>,
 		justification: Justification,
@@ -390,7 +405,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> BlockImport<Block>
 {
 	type Error = ConsensusError;
 
-	fn import_block(&self, mut block: ImportBlock<Block>, new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>)
+	fn import_block(&mut self, mut block: ImportBlock<Block>, new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>)
 		-> Result<ImportResult, Self::Error>
 	{
 		let hash = block.post_header().hash();
@@ -410,7 +425,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> BlockImport<Block>
 		// we don't want to finalize on `inner.import_block`
 		let mut justification = block.justification.take();
 		let enacts_consensus_change = !new_cache.is_empty();
-		let import_result = self.inner.import_block(block, new_cache);
+		let import_result = (&*self.inner).import_block(block, new_cache);
 
 		let mut imported_aux = {
 			match import_result {
@@ -504,7 +519,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA, SC> BlockImport<Block>
 	}
 
 	fn check_block(
-		&self,
+		&mut self,
 		hash: Block::Hash,
 		parent_hash: Block::Hash,
 	) -> Result<ImportResult, Self::Error> {
@@ -548,7 +563,7 @@ where
 	/// If `enacts_change` is set to true, then finalizing this block *must*
 	/// enact an authority set change, the function will panic otherwise.
 	fn import_justification(
-		&self,
+		&mut self,
 		hash: Block::Hash,
 		number: NumberFor<Block>,
 		justification: Justification,

substrate/core/finality-grandpa/src/light_import.rs

@@ -27,7 +27,7 @@ use client::{
 };
 use parity_codec::{Encode, Decode};
 use consensus_common::{
-	import_queue::{Verifier, SharedFinalityProofRequestBuilder}, well_known_cache_keys,
+	import_queue::{Verifier, BoxFinalityProofRequestBuilder}, well_known_cache_keys,
 	BlockOrigin, BlockImport, FinalityProofImport, ImportBlock, ImportResult, ImportedAux,
 	Error as ConsensusError, FinalityProofRequestBuilder,
 };
@@ -84,6 +84,16 @@ pub struct GrandpaLightBlockImport<B, E, Block: BlockT<Hash=H256>, RA> {
 	data: Arc<RwLock<LightImportData<Block>>>,
 }
 
+impl<B, E, Block: BlockT<Hash=H256>, RA> Clone for GrandpaLightBlockImport<B, E, Block, RA> {
+	fn clone(&self) -> Self {
+		GrandpaLightBlockImport {
+			client: self.client.clone(),
+			authority_set_provider: self.authority_set_provider.clone(),
+			data: self.data.clone(),
+		}
+	}
+}
+
 /// Mutable data of light block importer.
 struct LightImportData<Block: BlockT<Hash=H256>> {
 	last_finalized: Block::Hash,
@@ -100,8 +110,8 @@ struct LightAuthoritySet {
 
 impl<B, E, Block: BlockT<Hash=H256>, RA> GrandpaLightBlockImport<B, E, Block, RA> {
 	/// Create finality proof request builder.
-	pub fn create_finality_proof_request_builder(&self) -> SharedFinalityProofRequestBuilder<Block> {
-		Arc::new(GrandpaFinalityProofRequestBuilder(self.data.clone())) as _
+	pub fn create_finality_proof_request_builder(&self) -> BoxFinalityProofRequestBuilder<Block> {
+		Box::new(GrandpaFinalityProofRequestBuilder(self.data.clone())) as _
 	}
 }
 
@@ -116,7 +126,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA> BlockImport<Block>
 	type Error = ConsensusError;
 
 	fn import_block(
-		&self,
+		&mut self,
 		block: ImportBlock<Block>,
 		new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>,
 	) -> Result<ImportResult, Self::Error> {
@@ -126,7 +136,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA> BlockImport<Block>
 	}
 
 	fn check_block(
-		&self,
+		&mut self,
 		hash: Block::Hash,
 		parent_hash: Block::Hash,
 	) -> Result<ImportResult, Self::Error> {
@@ -144,7 +154,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA> FinalityProofImport<Block>
 {
 	type Error = ConsensusError;
 
-	fn on_start(&self) -> Vec<(Block::Hash, NumberFor<Block>)> {
+	fn on_start(&mut self) -> Vec<(Block::Hash, NumberFor<Block>)> {
 		let mut out = Vec::new();
 		let chain_info = self.client.info().chain;
 
@@ -159,7 +169,7 @@ impl<B, E, Block: BlockT<Hash=H256>, RA> FinalityProofImport<Block>
 	}
 
 	fn import_finality_proof(
-		&self,
+		&mut self,
 		hash: Block::Hash,
 		number: NumberFor<Block>,
 		finality_proof: Vec<u8>,
@@ -206,7 +216,7 @@ impl LightAuthoritySet {
 struct GrandpaFinalityProofRequestBuilder<B: BlockT<Hash=H256>>(Arc<RwLock<LightImportData<B>>>);
 
 impl<B: BlockT<Hash=H256>> FinalityProofRequestBuilder<B> for GrandpaFinalityProofRequestBuilder<B> {
-	fn build_request_data(&self, _hash: &B::Hash) -> Vec<u8> {
+	fn build_request_data(&mut self, _hash: &B::Hash) -> Vec<u8> {
 		let data = self.0.read();
 		make_finality_proof_request(
 			data.last_finalized,
@@ -217,7 +227,7 @@ impl<B: BlockT<Hash=H256>> FinalityProofRequestBuilder<B> for GrandpaFinalityPro
 
 /// Try to import new block.
 fn do_import_block<B, E, Block: BlockT<Hash=H256>, RA, J>(
-	client: &Client<B, E, Block, RA>,
+	mut client: &Client<B, E, Block, RA>,
 	data: &mut LightImportData<Block>,
 	mut block: ImportBlock<Block>,
 	new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>,
@@ -236,7 +246,7 @@ fn do_import_block<B, E, Block: BlockT<Hash=H256>, RA, J>(
 	// we don't want to finalize on `inner.import_block`
 	let justification = block.justification.take();
 	let enacts_consensus_change = !new_cache.is_empty();
-	let import_result = client.import_block(block, new_cache);
+	let import_result = BlockImport::import_block(&mut client, block, new_cache);
 
 	let mut imported_aux = match import_result {
 		Ok(ImportResult::Imported(aux)) => aux,
@@ -537,6 +547,19 @@ pub mod tests {
 		pub GrandpaLightBlockImport<B, E, Block, RA>
 	);
 
+	impl<B, E, Block: BlockT<Hash=H256>, RA> Clone
+		for NoJustificationsImport<B, E, Block, RA> where
+			NumberFor<Block>: grandpa::BlockNumberOps,
+			B: Backend<Block, Blake2Hasher> + 'static,
+			E: CallExecutor<Block, Blake2Hasher> + 'static + Clone + Send + Sync,
+			DigestFor<Block>: Encode,
+			RA: Send + Sync,
+	{
+		fn clone(&self) -> Self {
+			NoJustificationsImport(self.0.clone())
+		}
+	}
+
 	impl<B, E, Block: BlockT<Hash=H256>, RA> BlockImport<Block>
 		for NoJustificationsImport<B, E, Block, RA> where
 			NumberFor<Block>: grandpa::BlockNumberOps,
@@ -548,7 +571,7 @@ pub mod tests {
 		type Error = ConsensusError;
 
 		fn import_block(
-			&self,
+			&mut self,
 			mut block: ImportBlock<Block>,
 			new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>,
 		) -> Result<ImportResult, Self::Error> {
@@ -557,7 +580,7 @@ pub mod tests {
 		}
 
 		fn check_block(
-			&self,
+			&mut self,
 			hash: Block::Hash,
 			parent_hash: Block::Hash,
 		) -> Result<ImportResult, Self::Error> {
@@ -575,12 +598,12 @@ pub mod tests {
 	{
 		type Error = ConsensusError;
 
-		fn on_start(&self) -> Vec<(Block::Hash, NumberFor<Block>)> {
+		fn on_start(&mut self) -> Vec<(Block::Hash, NumberFor<Block>)> {
 			self.0.on_start()
 		}
 
 		fn import_finality_proof(
-			&self,
+			&mut self,
 			hash: Block::Hash,
 			number: NumberFor<Block>,
 			finality_proof: Vec<u8>,

substrate/core/finality-grandpa/src/tests.rs

@@ -30,8 +30,8 @@ use client::{
 };
 use test_client::{self, runtime::BlockNumber};
 use consensus_common::{BlockOrigin, ForkChoiceStrategy, ImportedAux, ImportBlock, ImportResult};
-use consensus_common::import_queue::{SharedBlockImport, SharedJustificationImport, SharedFinalityProofImport,
-	SharedFinalityProofRequestBuilder,
+use consensus_common::import_queue::{BoxBlockImport, BoxJustificationImport, BoxFinalityProofImport,
+	BoxFinalityProofRequestBuilder,
 };
 use std::collections::{HashMap, HashSet};
 use std::result;
@@ -106,10 +106,10 @@ impl TestNetFactory for GrandpaTestNet {
 
 	fn make_block_import(&self, client: PeersClient)
 		-> (
-			SharedBlockImport<Block>,
-			Option<SharedJustificationImport<Block>>,
-			Option<SharedFinalityProofImport<Block>>,
-			Option<SharedFinalityProofRequestBuilder<Block>>,
+			BoxBlockImport<Block>,
+			Option<BoxJustificationImport<Block>>,
+			Option<BoxFinalityProofImport<Block>>,
+			Option<BoxFinalityProofRequestBuilder<Block>>,
 			PeerData,
 		)
 	{
@@ -124,8 +124,9 @@ impl TestNetFactory for GrandpaTestNet {
 					Arc::new(self.test_config.clone()),
 					select_chain,
 				).expect("Could not create block import for fresh peer.");
-				let shared_import = Arc::new(import);
-				(shared_import.clone(), Some(shared_import), None, None, Mutex::new(Some(link)))
+				let justification_import = Box::new(import.clone());
+				let block_import = Box::new(import);
+				(block_import, Some(justification_import), None, None, Mutex::new(Some(link)))
 			},
 			PeersClient::Light(ref client) => {
 				use crate::light_import::tests::light_block_import_without_justifications;
@@ -139,8 +140,9 @@ impl TestNetFactory for GrandpaTestNet {
 					Arc::new(self.test_config.clone())
 				).expect("Could not create block import for fresh peer.");
 				let finality_proof_req_builder = import.0.create_finality_proof_request_builder();
-				let shared_import = Arc::new(import);
-				(shared_import.clone(), None, Some(shared_import), Some(finality_proof_req_builder), Mutex::new(None))
+				let proof_import = Box::new(import.clone());
+				let block_import = Box::new(import);
+				(block_import, None, Some(proof_import), Some(finality_proof_req_builder), Mutex::new(None))
 			},
 		}
 	}
@@ -952,7 +954,7 @@ fn allows_reimporting_change_blocks() {
 	let mut net = GrandpaTestNet::new(api.clone(), 3);
 
 	let client = net.peer(0).client().clone();
-	let (block_import, ..) = net.make_block_import(client.clone());
+	let (mut block_import, ..) = net.make_block_import(client.clone());
 
 	let full_client = client.as_full().unwrap();
 	let builder = full_client.new_block_at(&BlockId::Number(0), Default::default()).unwrap();
@@ -1001,7 +1003,7 @@ fn test_bad_justification() {
 	let mut net = GrandpaTestNet::new(api.clone(), 3);
 
 	let client = net.peer(0).client().clone();
-	let (block_import, ..) = net.make_block_import(client.clone());
+	let (mut block_import, ..) = net.make_block_import(client.clone());
 
 	let full_client = client.as_full().expect("only full clients are used in test");
 	let builder = full_client.new_block_at(&BlockId::Number(0), Default::default()).unwrap();
@@ -1093,15 +1095,16 @@ fn voter_persists_its_votes() {
 				on_exit: Exit,
 				telemetry_on_connect: None,
 			};
-			let mut voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network");
 
-			let voter = future::poll_fn(move || {
-				// we need to keep the block_import alive since it owns the
-				// sender for the voter commands channel, if that gets dropped
-				// then the voter will stop
-				let _block_import = _block_import.clone();
-				voter.poll()
-			});
+			let voter = run_grandpa_voter(grandpa_params)
+				.expect("all in order with client and network")
+				.then(move |r| {
+					// we need to keep the block_import alive since it owns the
+					// sender for the voter commands channel, if that gets dropped
+					// then the voter will stop
+					drop(_block_import);
+					r
+				});
 
 			voter.select2(rx.into_future()).then(|res| match res {
 				Ok(future::Either::A(x)) => {