Replace unnecessary &mut self with &self in `BlockImport::import_…

…block()` (paritytech#5339)

There was no need for it to be `&mut self` since block import can happen
concurrently for different blocks and in many cases it was `&mut Arc<dyn
BlockImport>` anyway 🤷‍♂️

Similar in nature to
paritytech#4844

cumulus/client/consensus/common/src/lib.rs

@@ -185,7 +185,7 @@ where
 	}
 
 	async fn import_block(
-		&mut self,
+		&self,
 		mut params: sc_consensus::BlockImportParams<Block>,
 	) -> Result<sc_consensus::ImportResult, Self::Error> {
 		// Blocks are stored within the backend by using POST hash.

cumulus/client/consensus/common/src/parachain_consensus.rs

@@ -433,11 +433,8 @@ async fn handle_new_best_parachain_head<Block, P>(
 	}
 }
 
-async fn import_block_as_new_best<Block, P>(
-	hash: Block::Hash,
-	header: Block::Header,
-	mut parachain: &P,
-) where
+async fn import_block_as_new_best<Block, P>(hash: Block::Hash, header: Block::Header, parachain: &P)
+where
 	Block: BlockT,
 	P: UsageProvider<Block> + Send + Sync + BlockBackend<Block>,
 	for<'a> &'a P: BlockImport<Block>,

cumulus/client/consensus/common/src/tests.rs

@@ -321,7 +321,7 @@ fn build_block<B: InitBlockBuilder>(
 }
 
 async fn import_block<I: BlockImport<Block>>(
-	importer: &mut I,
+	importer: &I,
 	block: Block,
 	origin: BlockOrigin,
 	import_as_best: bool,
@@ -568,7 +568,7 @@ fn follow_finalized_does_not_stop_on_unknown_block() {
 fn follow_new_best_sets_best_after_it_is_imported() {
 	sp_tracing::try_init_simple();
 
-	let mut client = Arc::new(TestClientBuilder::default().build());
+	let client = Arc::new(TestClientBuilder::default().build());
 
 	let block = build_and_import_block(client.clone(), false);
 

cumulus/client/network/src/tests.rs

@@ -612,7 +612,7 @@ fn relay_parent_not_imported_when_block_announce_is_processed() {
 	block_on(async move {
 		let (mut validator, api) = make_validator_and_api();
 
-		let mut client = api.relay_client.clone();
+		let client = api.relay_client.clone();
 		let block = client.init_polkadot_block_builder().build().expect("Build new block").block;
 
 		let (signal, header) = make_gossip_message_and_header(api, block.hash(), 0).await;

cumulus/client/relay-chain-inprocess-interface/src/lib.rs

@@ -423,7 +423,7 @@ mod tests {
 
 	#[test]
 	fn returns_directly_for_available_block() {
-		let (mut client, block, relay_chain_interface) = build_client_backend_and_block();
+		let (client, block, relay_chain_interface) = build_client_backend_and_block();
 		let hash = block.hash();
 
 		block_on(client.import(BlockOrigin::Own, block)).expect("Imports the block");
@@ -439,7 +439,7 @@ mod tests {
 
 	#[test]
 	fn resolve_after_block_import_notification_was_received() {
-		let (mut client, block, relay_chain_interface) = build_client_backend_and_block();
+		let (client, block, relay_chain_interface) = build_client_backend_and_block();
 		let hash = block.hash();
 
 		block_on(async move {
@@ -468,7 +468,7 @@ mod tests {
 
 	#[test]
 	fn do_not_resolve_after_different_block_import_notification_was_received() {
-		let (mut client, block, relay_chain_interface) = build_client_backend_and_block();
+		let (client, block, relay_chain_interface) = build_client_backend_and_block();
 		let hash = block.hash();
 
 		let ext = construct_transfer_extrinsic(

cumulus/pallets/parachain-system/src/validate_block/tests.rs

@@ -312,7 +312,7 @@ fn validation_params_and_memory_optimized_validation_params_encode_and_decode()
 fn validate_block_works_with_child_tries() {
 	sp_tracing::try_init_simple();
 
-	let (mut client, parent_head) = create_test_client();
+	let (client, parent_head) = create_test_client();
 	let TestBlockData { block, .. } = build_block_with_witness(
 		&client,
 		vec![generate_extrinsic(&client, Charlie, TestPalletCall::read_and_write_child_tries {})],

cumulus/test/service/benches/validate_block_glutton.rs

@@ -43,7 +43,7 @@ use sp_runtime::traits::Header as HeaderT;
 use cumulus_test_service::bench_utils as utils;
 
 async fn import_block(
-	mut client: &cumulus_test_client::Client,
+	client: &cumulus_test_client::Client,
 	built: cumulus_test_runtime::Block,
 	import_existing: bool,
 ) {

cumulus/test/service/src/bench_utils.rs

@@ -111,7 +111,7 @@ pub fn extrinsic_set_validation_data(
 }
 
 /// Import block into the given client and make sure the import was successful
-pub async fn import_block(mut client: &TestClient, block: &NodeBlock, import_existing: bool) {
+pub async fn import_block(client: &TestClient, block: &NodeBlock, import_existing: bool) {
 	let mut params = BlockImportParams::new(BlockOrigin::File, block.header.clone());
 	params.body = Some(block.extrinsics.clone());
 	params.state_action = StateAction::Execute;

polkadot/node/test/client/src/lib.rs

@@ -102,7 +102,7 @@ mod tests {
 
 	#[test]
 	fn ensure_test_client_can_build_and_import_block() {
-		let mut client = TestClientBuilder::new().build();
+		let client = TestClientBuilder::new().build();
 
 		let block_builder = client.init_polkadot_block_builder();
 		let block = block_builder.build().expect("Finalizes the block").block;
@@ -113,7 +113,7 @@ mod tests {
 
 	#[test]
 	fn ensure_test_client_can_push_extrinsic() {
-		let mut client = TestClientBuilder::new().build();
+		let client = TestClientBuilder::new().build();
 
 		let transfer = construct_transfer_extrinsic(
 			&client,

polkadot/xcm/xcm-executor/integration-tests/src/lib.rs

@@ -34,7 +34,7 @@ use xcm_executor::traits::WeightBounds;
 #[test]
 fn basic_buy_fees_message_executes() {
 	sp_tracing::try_init_simple();
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 
 	let msg = Xcm(vec![
 		WithdrawAsset((Parent, 100).into()),
@@ -75,7 +75,7 @@ fn basic_buy_fees_message_executes() {
 #[test]
 fn transact_recursion_limit_works() {
 	sp_tracing::try_init_simple();
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 
 	let base_xcm = |call: polkadot_test_runtime::RuntimeCall| {
 		Xcm(vec![
@@ -174,7 +174,7 @@ fn query_response_fires() {
 	use polkadot_test_runtime::RuntimeEvent::TestNotifier;
 
 	sp_tracing::try_init_simple();
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 
 	let mut block_builder = client.init_polkadot_block_builder();
 
@@ -256,7 +256,7 @@ fn query_response_elicits_handler() {
 	use polkadot_test_runtime::RuntimeEvent::TestNotifier;
 
 	sp_tracing::try_init_simple();
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 
 	let mut block_builder = client.init_polkadot_block_builder();
 
@@ -332,7 +332,7 @@ fn query_response_elicits_handler() {
 #[test]
 fn deposit_reserve_asset_works_for_any_xcm_sender() {
 	sp_tracing::try_init_simple();
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 
 	// Init values for the simulated origin Parachain
 	let amount_to_send: u128 = 1_000_000_000_000;

prdoc/pr_5339.prdoc

@@ -0,0 +1,45 @@
+title: Replace unnecessary `&mut self` with `&self` in `BlockImport::import_block()`
+
+doc:
+  - audience: Node Dev
+    description: |
+      Simplifies block import API to match intended design where independent blocks can technically be imported
+      concurrently and in practice was called through `Arc` anyway
+
+crates:
+  - name: cumulus-client-consensus-common
+    bump: major
+  - name: cumulus-client-network
+    bump: none
+  - name: cumulus-relay-chain-inprocess-interface
+    bump: none
+  - name: cumulus-pallet-parachain-system
+    bump: none
+  - name: sc-basic-authorship
+    bump: patch
+  - name: sc-consensus-babe
+    bump: major
+  - name: sc-consensus-beefy
+    bump: major
+  - name: sc-consensus
+    bump: major
+  - name: sc-consensus-grandpa
+    bump: major
+  - name: sc-consensus-pow
+    bump: major
+  - name: mmr-gadget
+    bump: none
+  - name: sc-network
+    bump: none
+  - name: sc-network-sync
+    bump: none
+  - name: sc-offchain
+    bump: none
+  - name: sc-rpc-spec-v2
+    bump: none
+  - name: sc-rpc
+    bump: none
+  - name: sc-service
+    bump: major
+  - name: sc-transaction-pool
+    bump: none

substrate/bin/node/cli/benches/block_production.rs

@@ -124,7 +124,7 @@ fn extrinsic_set_time(now: u64) -> OpaqueExtrinsic {
 	.into()
 }
 
-fn import_block(mut client: &FullClient, built: BuiltBlock<node_primitives::Block>) {
+fn import_block(client: &FullClient, built: BuiltBlock<node_primitives::Block>) {
 	let mut params = BlockImportParams::new(BlockOrigin::File, built.block.header);
 	params.state_action =
 		StateAction::ApplyChanges(sc_consensus::StorageChanges::Changes(built.storage_changes));

substrate/bin/node/cli/tests/basic.rs

@@ -850,7 +850,7 @@ fn should_import_block_with_test_client() {
 		sp_consensus::BlockOrigin, ClientBlockImportExt, TestClientBuilder, TestClientBuilderExt,
 	};
 
-	let mut client = TestClientBuilder::new().build();
+	let client = TestClientBuilder::new().build();
 	let block1 = changes_trie_block();
 	let block_data = block1.0;
 	let block = node_primitives::Block::decode(&mut &block_data[..]).unwrap();

substrate/client/basic-authorship/src/basic_authorship.rs

@@ -852,7 +852,7 @@ mod tests {
 			block
 		};
 
-		let import_and_maintain = |mut client: Arc<TestClient>, block: TestBlock| {
+		let import_and_maintain = |client: Arc<TestClient>, block: TestBlock| {
 			let hash = block.hash();
 			block_on(client.import(BlockOrigin::Own, block)).unwrap();
 			block_on(txpool.maintain(chain_event(

substrate/client/consensus/babe/src/lib.rs

@@ -1342,7 +1342,7 @@ where
 	// This function makes multiple transactions to the DB. If one of them fails we may
 	// end up in an inconsistent state and have to resync.
 	async fn import_state(
-		&mut self,
+		&self,
 		mut block: BlockImportParams<Block>,
 	) -> Result<ImportResult, ConsensusError> {
 		let hash = block.post_hash();
@@ -1405,7 +1405,7 @@ where
 	type Error = ConsensusError;
 
 	async fn import_block(
-		&mut self,
+		&self,
 		mut block: BlockImportParams<Block>,
 	) -> Result<ImportResult, Self::Error> {
 		let hash = block.post_hash();

substrate/client/consensus/babe/src/tests.rs

@@ -150,7 +150,7 @@ where
 	type Error = BI::Error;
 
 	async fn import_block(
-		&mut self,
+		&self,
 		block: BlockImportParams<TestBlock>,
 	) -> Result<ImportResult, Self::Error> {
 		Ok(self.0.import_block(block).await.expect("importing block failed"))

substrate/client/consensus/beefy/src/import.rs

@@ -132,7 +132,7 @@ where
 	type Error = ConsensusError;
 
 	async fn import_block(
-		&mut self,
+		&self,
 		mut block: BlockImportParams<Block>,
 	) -> Result<ImportResult, Self::Error> {
 		let hash = block.post_hash();

substrate/client/consensus/beefy/src/tests.rs

@@ -790,7 +790,7 @@ async fn beefy_importing_justifications() {
 
 	let client = net.peer(0).client().clone();
 	let full_client = client.as_client();
-	let (mut block_import, _, peer_data) = net.make_block_import(client.clone());
+	let (block_import, _, peer_data) = net.make_block_import(client.clone());
 	let PeerData { beefy_voter_links, .. } = peer_data;
 	let justif_stream = beefy_voter_links.lock().take().unwrap().from_block_import_justif_stream;
 	let mut justif_recv = justif_stream.subscribe(100_000);

substrate/client/consensus/common/src/block_import.rs

@@ -310,10 +310,7 @@ pub trait BlockImport<B: BlockT> {
 	async fn check_block(&self, block: BlockCheckParams<B>) -> Result<ImportResult, Self::Error>;
 
 	/// Import a block.
-	async fn import_block(
-		&mut self,
-		block: BlockImportParams<B>,
-	) -> Result<ImportResult, Self::Error>;
+	async fn import_block(&self, block: BlockImportParams<B>) -> Result<ImportResult, Self::Error>;
 }
 
 #[async_trait::async_trait]
@@ -326,10 +323,7 @@ impl<B: BlockT> BlockImport<B> for crate::import_queue::BoxBlockImport<B> {
 	}
 
 	/// Import a block.
-	async fn import_block(
-		&mut self,
-		block: BlockImportParams<B>,
-	) -> Result<ImportResult, Self::Error> {
+	async fn import_block(&self, block: BlockImportParams<B>) -> Result<ImportResult, Self::Error> {
 		(**self).import_block(block).await
 	}
 }
@@ -346,10 +340,7 @@ where
 		(&**self).check_block(block).await
 	}
 
-	async fn import_block(
-		&mut self,
-		block: BlockImportParams<B>,
-	) -> Result<ImportResult, Self::Error> {
+	async fn import_block(&self, block: BlockImportParams<B>) -> Result<ImportResult, Self::Error> {
 		(&**self).import_block(block).await
 	}
 }

substrate/client/consensus/common/src/import_queue/basic_queue.rs

@@ -525,7 +525,7 @@ mod tests {
 		}
 
 		async fn import_block(
-			&mut self,
+			&self,
 			_block: BlockImportParams<Block>,
 		) -> Result<ImportResult, Self::Error> {
 			Ok(ImportResult::imported(true))

substrate/client/consensus/grandpa/src/finality_proof.rs

@@ -319,7 +319,7 @@ mod tests {
 	) -> (Arc<TestClient>, Arc<TestBackend>, Vec<Block>) {
 		let builder = TestClientBuilder::new();
 		let backend = builder.backend();
-		let mut client = Arc::new(builder.build());
+		let client = Arc::new(builder.build());
 
 		let mut blocks = Vec::new();
 		for _ in 0..number_of_blocks {