Change the import queue traits to take &mut self instead of &self

[tomaka]

Note: On ice because has to be based on top of #3049 in order to fix the tests. There's some overlap between this PR and #3049 and I'd rather wait for #3049 instead of fixing merge conflicts. However, the PR itself is ready for review.

The BlockImport, JustificationImport, FinalityProofImport and FinalityProofRequestBuilder traits have been modified to accept &mut self instead of &self.
Additionally, they no longer get passed to the import queue by Arc but by Box.
The only culprit is Aura, where I had to use an Arc<Mutex<>> instead of a Box. Further clean ups should make it possible to remove this Arc<Mutex<>>.

This PR doesn't achieve much. It is just some refactoring/cleanup. If there's no good reason to accept &self, we should instead accept &mut self.

Here's an untested Polkadot patch, but that I'm confident should work:

diff --git a/service/src/lib.rs b/service/src/lib.rs
index 73e9d162..62b02e20 100644
--- a/service/src/lib.rs
+++ b/service/src/lib.rs
@@ -57,7 +57,7 @@ pub struct CustomConfiguration {
 	// FIXME: rather than putting this on the config, let's have an actual intermediate setup state
 	// https://github.com/paritytech/substrate/issues/1134
 	pub grandpa_import_setup: Option<(
-		Arc<grandpa::BlockImportForService<Factory>>,
+		grandpa::BlockImportForService<Factory>,
 		grandpa::LinkHalfForService<Factory>
 	)>,
 
@@ -320,14 +320,13 @@ service::construct_service_factory! {
 					grandpa::block_import::<_, _, _, RuntimeApi, FullClient<Self>, _>(
 						client.clone(), client.clone(), select_chain
 					)?;
-				let block_import = Arc::new(block_import);
 				let justification_import = block_import.clone();
 
 				config.custom.grandpa_import_setup = Some((block_import.clone(), link_half));
 				import_queue::<_, _, ed25519::Pair>(
 					slot_duration,
-					block_import,
-					Some(justification_import),
+					Box::new(block_import),
+					Some(Box::new(justification_import)),
 					None,
 					None,
 					client,
@@ -346,15 +345,14 @@ service::construct_service_factory! {
 				let block_import = grandpa::light_block_import::<_, _, _, RuntimeApi, LightClient<Self>>(
 					client.clone(), Arc::new(fetch_checker), client.clone()
 				)?;
-				let block_import = Arc::new(block_import);
 				let finality_proof_import = block_import.clone();
 				let finality_proof_request_builder = finality_proof_import.create_finality_proof_request_builder();
 
 				import_queue::<_, _, ed25519::Pair>(
 					SlotDuration::get_or_compute(&*client)?,
-					block_import,
+					Box::new(block_import),
 					None,
-					Some(finality_proof_import),
+					Some(Box::new(finality_proof_import)),
 					Some(finality_proof_request_builder),
 					client,
 					config.custom.inherent_data_providers.clone(),

[arkpar]

As a general rule I'd prefer to keep synchronization as low as possible. This allows for a more fine grained locking and less contention. E.g. in this case import_block implementation currently may be more efficient as it might only lock parts of the Client that are required to import the block internally. After changing to &mut self the called is required to lock the whole thing, potentially blocking on other operations that require access to the Client

[tomaka]

After changing to &mut self the called is required to lock the whole thing, potentially blocking on other operations that require access to the Client

What I did in this PR is implement the ImportBlock trait on Arc<Client> and &Client instead of just Client.

Before this PR, the import queue was manipulating an Arc<Arc<Client>>, and passing an &Arc<Client> when doing a block import.
After this PR, the import queue is manipulating a Box<Arc<Client>>, and passing a &mut Arc<Client> when doing an import. We don't actually have to lock anything from the outside, and in general, the direction of my refactoring is to remove mutexes. The exception, as I mentioned, is unfortunately Aura.
The behaviour in Client itself is still exactly the same.

The next goal (if possible) would be to make the import queue generic over its parameters instead of using Box, so that it would directly manipulate Arc<Client>.