BlockId removal: refactor: Finalizer (paritytech#12528)

* BlockId removal: refactor: Finalizer

It changes the arguments of methods of `Finalizer` trait from:
block: `BlockId<Block>` to: hash: `&Block::Hash`

This PR is part of BlockId::Number refactoring analysis (paritytech/substrate#11292)

* minor corrections

* failing test corrected

* minor rework

Author: michalkucharczyk

client/api/src/backend.rs

@@ -252,7 +252,7 @@ pub trait Finalizer<Block: BlockT, B: Backend<Block>> {
 	fn apply_finality(
 		&self,
 		operation: &mut ClientImportOperation<Block, B>,
-		id: BlockId<Block>,
+		block: &Block::Hash,
 		justification: Option<Justification>,
 		notify: bool,
 	) -> sp_blockchain::Result<()>;
@@ -272,7 +272,7 @@ pub trait Finalizer<Block: BlockT, B: Backend<Block>> {
 	/// while performing major synchronization work.
 	fn finalize_block(
 		&self,
-		id: BlockId<Block>,
+		block: &Block::Hash,
 		justification: Option<Justification>,
 		notify: bool,
 	) -> sp_blockchain::Result<()>;

client/beefy/src/tests.rs

@@ -509,14 +509,10 @@ fn finalize_block_and_wait_for_beefy(
 	let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone());
 
 	for block in finalize_targets {
-		let finalize = BlockId::number(*block);
 		peers.clone().for_each(|(index, _)| {
-			net.lock()
-				.peer(index)
-				.client()
-				.as_client()
-				.finalize_block(finalize, None)
-				.unwrap();
+			let client = net.lock().peer(index).client().as_client();
+			let finalize = client.expect_block_hash_from_id(&BlockId::number(*block)).unwrap();
+			client.finalize_block(&finalize, None).unwrap();
 		})
 	}
 
@@ -604,17 +600,23 @@ fn lagging_validators() {
 	);
 
 	// Alice finalizes #25, Bob lags behind
-	let finalize = BlockId::number(25);
+	let finalize = net
+		.lock()
+		.peer(0)
+		.client()
+		.as_client()
+		.expect_block_hash_from_id(&BlockId::number(25))
+		.unwrap();
 	let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone());
-	net.lock().peer(0).client().as_client().finalize_block(finalize, None).unwrap();
+	net.lock().peer(0).client().as_client().finalize_block(&finalize, None).unwrap();
 	// verify nothing gets finalized by BEEFY
 	let timeout = Some(Duration::from_millis(250));
 	streams_empty_after_timeout(best_blocks, &net, &mut runtime, timeout);
 	streams_empty_after_timeout(versioned_finality_proof, &net, &mut runtime, None);
 
 	// Bob catches up and also finalizes #25
 	let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone());
-	net.lock().peer(1).client().as_client().finalize_block(finalize, None).unwrap();
+	net.lock().peer(1).client().as_client().finalize_block(&finalize, None).unwrap();
 	// expected beefy finalizes block #17 from diff-power-of-two
 	wait_for_best_beefy_blocks(best_blocks, &net, &mut runtime, &[23, 24, 25]);
 	wait_for_beefy_signed_commitments(versioned_finality_proof, &net, &mut runtime, &[23, 24, 25]);
@@ -628,16 +630,22 @@ fn lagging_validators() {
 
 	// Alice finalizes session-boundary mandatory block #60, Bob lags behind
 	let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone());
-	let finalize = BlockId::number(60);
-	net.lock().peer(0).client().as_client().finalize_block(finalize, None).unwrap();
+	let finalize = net
+		.lock()
+		.peer(0)
+		.client()
+		.as_client()
+		.expect_block_hash_from_id(&BlockId::number(60))
+		.unwrap();
+	net.lock().peer(0).client().as_client().finalize_block(&finalize, None).unwrap();
 	// verify nothing gets finalized by BEEFY
 	let timeout = Some(Duration::from_millis(250));
 	streams_empty_after_timeout(best_blocks, &net, &mut runtime, timeout);
 	streams_empty_after_timeout(versioned_finality_proof, &net, &mut runtime, None);
 
 	// Bob catches up and also finalizes #60 (and should have buffered Alice's vote on #60)
 	let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers);
-	net.lock().peer(1).client().as_client().finalize_block(finalize, None).unwrap();
+	net.lock().peer(1).client().as_client().finalize_block(&finalize, None).unwrap();
 	// verify beefy skips intermediary votes, and successfully finalizes mandatory block #60
 	wait_for_best_beefy_blocks(best_blocks, &net, &mut runtime, &[60]);
 	wait_for_beefy_signed_commitments(versioned_finality_proof, &net, &mut runtime, &[60]);
@@ -681,24 +689,16 @@ fn correct_beefy_payload() {
 		get_beefy_streams(&mut net.lock(), [(0, BeefyKeyring::Alice)].into_iter());
 
 	// now 2 good validators and 1 bad one are voting
-	net.lock()
+	let hashof11 = net
+		.lock()
 		.peer(0)
 		.client()
 		.as_client()
-		.finalize_block(BlockId::number(11), None)
-		.unwrap();
-	net.lock()
-		.peer(1)
-		.client()
-		.as_client()
-		.finalize_block(BlockId::number(11), None)
-		.unwrap();
-	net.lock()
-		.peer(3)
-		.client()
-		.as_client()
-		.finalize_block(BlockId::number(11), None)
+		.expect_block_hash_from_id(&BlockId::number(11))
 		.unwrap();
+	net.lock().peer(0).client().as_client().finalize_block(&hashof11, None).unwrap();
+	net.lock().peer(1).client().as_client().finalize_block(&hashof11, None).unwrap();
+	net.lock().peer(3).client().as_client().finalize_block(&hashof11, None).unwrap();
 
 	// verify consensus is _not_ reached
 	let timeout = Some(Duration::from_millis(250));
@@ -708,12 +708,7 @@ fn correct_beefy_payload() {
 	// 3rd good validator catches up and votes as well
 	let (best_blocks, versioned_finality_proof) =
 		get_beefy_streams(&mut net.lock(), [(0, BeefyKeyring::Alice)].into_iter());
-	net.lock()
-		.peer(2)
-		.client()
-		.as_client()
-		.finalize_block(BlockId::number(11), None)
-		.unwrap();
+	net.lock().peer(2).client().as_client().finalize_block(&hashof11, None).unwrap();
 
 	// verify consensus is reached
 	wait_for_best_beefy_blocks(best_blocks, &net, &mut runtime, &[11]);
@@ -923,12 +918,9 @@ fn on_demand_beefy_justification_sync() {
 
 	let (dave_best_blocks, _) =
 		get_beefy_streams(&mut net.lock(), [(dave_index, BeefyKeyring::Dave)].into_iter());
-	net.lock()
-		.peer(dave_index)
-		.client()
-		.as_client()
-		.finalize_block(BlockId::number(1), None)
-		.unwrap();
+	let client = net.lock().peer(dave_index).client().as_client();
+	let hashof1 = client.expect_block_hash_from_id(&BlockId::number(1)).unwrap();
+	client.finalize_block(&hashof1, None).unwrap();
 	// Give Dave task some cpu cycles to process the finality notification,
 	run_for(Duration::from_millis(100), &net, &mut runtime);
 	// freshly spun up Dave now needs to listen for gossip to figure out the state of his peers.

client/beefy/src/worker.rs

@@ -1506,11 +1506,9 @@ pub(crate) mod tests {
 		// push 15 blocks with `AuthorityChange` digests every 10 blocks
 		net.generate_blocks_and_sync(15, 10, &validator_set, false);
 		// finalize 13 without justifications
-		net.peer(0)
-			.client()
-			.as_client()
-			.finalize_block(BlockId::number(13), None)
-			.unwrap();
+		let hashof13 =
+			backend.blockchain().expect_block_hash_from_id(&BlockId::Number(13)).unwrap();
+		net.peer(0).client().as_client().finalize_block(&hashof13, None).unwrap();
 
 		// Test initialization at session boundary.
 		{

client/consensus/babe/src/tests.rs

@@ -820,7 +820,7 @@ fn revert_not_allowed_for_finalized() {
 	let canon = propose_and_import_blocks_wrap(BlockId::Number(0), 3);
 
 	// Finalize best block
-	client.finalize_block(BlockId::Hash(canon[2]), None, false).unwrap();
+	client.finalize_block(&canon[2], None, false).unwrap();
 
 	// Revert canon chain to last finalized block
 	revert(client.clone(), backend, 100).expect("revert should work for baked test scenario");
@@ -882,7 +882,7 @@ fn importing_epoch_change_block_prunes_tree() {
 
 	// We finalize block #13 from the canon chain, so on the next epoch
 	// change the tree should be pruned, to not contain F (#7).
-	client.finalize_block(BlockId::Hash(canon_hashes[12]), None, false).unwrap();
+	client.finalize_block(&canon_hashes[12], None, false).unwrap();
 	propose_and_import_blocks_wrap(BlockId::Hash(client.chain_info().best_hash), 7);
 
 	// at this point no hashes from the first fork must exist on the tree
@@ -909,7 +909,7 @@ fn importing_epoch_change_block_prunes_tree() {
 		.any(|h| fork_3.contains(h)),);
 
 	// finalizing block #25 from the canon chain should prune out the second fork
-	client.finalize_block(BlockId::Hash(canon_hashes[24]), None, false).unwrap();
+	client.finalize_block(&canon_hashes[24], None, false).unwrap();
 	propose_and_import_blocks_wrap(BlockId::Hash(client.chain_info().best_hash), 8);
 
 	// at this point no hashes from the second fork must exist on the tree
@@ -1049,7 +1049,7 @@ fn obsolete_blocks_aux_data_cleanup() {
 	assert!(aux_data_check(&fork3_hashes, true));
 
 	// Finalize A3
-	client.finalize_block(BlockId::Number(3), None, true).unwrap();
+	client.finalize_block(&fork1_hashes[2], None, true).unwrap();
 
 	// Wiped: A1, A2
 	assert!(aux_data_check(&fork1_hashes[..2], false));
@@ -1060,7 +1060,7 @@ fn obsolete_blocks_aux_data_cleanup() {
 	// Present C4, C5
 	assert!(aux_data_check(&fork3_hashes, true));
 
-	client.finalize_block(BlockId::Number(4), None, true).unwrap();
+	client.finalize_block(&fork1_hashes[3], None, true).unwrap();
 
 	// Wiped: A3
 	assert!(aux_data_check(&fork1_hashes[2..3], false));

client/consensus/manual-seal/src/finalize_block.rs

@@ -20,7 +20,7 @@
 
 use crate::rpc;
 use sc_client_api::backend::{Backend as ClientBackend, Finalizer};
-use sp_runtime::{generic::BlockId, traits::Block as BlockT, Justification};
+use sp_runtime::{traits::Block as BlockT, Justification};
 use std::{marker::PhantomData, sync::Arc};
 
 /// params for block finalization.
@@ -46,7 +46,7 @@ where
 {
 	let FinalizeBlockParams { hash, mut sender, justification, finalizer, .. } = params;
 
-	match finalizer.finalize_block(BlockId::Hash(hash), justification, true) {
+	match finalizer.finalize_block(&hash, justification, true) {
 		Err(e) => {
 			log::warn!("Failed to finalize block {}", e);
 			rpc::send_result(&mut sender, Err(e.into()))

client/finality-grandpa/src/environment.rs

@@ -1352,7 +1352,7 @@ where
 		// ideally some handle to a synchronization oracle would be used
 		// to avoid unconditionally notifying.
 		client
-			.apply_finality(import_op, BlockId::Hash(hash), persisted_justification, true)
+			.apply_finality(import_op, &hash, persisted_justification, true)
 			.map_err(|e| {
 				warn!(target: "afg", "Error applying finality to block {:?}: {}", (hash, number), e);
 				e

client/finality-grandpa/src/finality_proof.rs

@@ -309,7 +309,8 @@ mod tests {
 		}
 
 		for block in to_finalize {
-			client.finalize_block(BlockId::Number(*block), None).unwrap();
+			let hash = blocks[*block as usize - 1].hash();
+			client.finalize_block(&hash, None).unwrap();
 		}
 		(client, backend, blocks)
 	}
@@ -489,7 +490,7 @@ mod tests {
 		let grandpa_just8 = GrandpaJustification::from_commit(&client, round, commit).unwrap();
 
 		client
-			.finalize_block(BlockId::Number(8), Some((ID, grandpa_just8.encode().clone())))
+			.finalize_block(&block8.hash(), Some((ID, grandpa_just8.encode().clone())))
 			.unwrap();
 
 		// Authority set change at block 8, so the justification stored there will be used in the

client/finality-grandpa/src/tests.rs

@@ -1457,7 +1457,12 @@ fn grandpa_environment_respects_voting_rules() {
 	);
 
 	// we finalize block 19 with block 21 being the best block
-	peer.client().finalize_block(BlockId::Number(19), None, false).unwrap();
+	let hashof19 = peer
+		.client()
+		.as_client()
+		.expect_block_hash_from_id(&BlockId::Number(19))
+		.unwrap();
+	peer.client().finalize_block(&hashof19, None, false).unwrap();
 
 	// the 3/4 environment should propose block 21 for voting
 	assert_eq!(
@@ -1479,7 +1484,12 @@ fn grandpa_environment_respects_voting_rules() {
 	);
 
 	// we finalize block 21 with block 21 being the best block
-	peer.client().finalize_block(BlockId::Number(21), None, false).unwrap();
+	let hashof21 = peer
+		.client()
+		.as_client()
+		.expect_block_hash_from_id(&BlockId::Number(21))
+		.unwrap();
+	peer.client().finalize_block(&hashof21, None, false).unwrap();
 
 	// even though the default environment will always try to not vote on the
 	// best block, there's a hard rule that we can't cast any votes lower than

client/finality-grandpa/src/warp_proof.rs

@@ -327,7 +327,7 @@ mod tests {
 	use sp_consensus::BlockOrigin;
 	use sp_finality_grandpa::GRANDPA_ENGINE_ID;
 	use sp_keyring::Ed25519Keyring;
-	use sp_runtime::{generic::BlockId, traits::Header as _};
+	use sp_runtime::traits::Header as _;
 	use std::sync::Arc;
 	use substrate_test_runtime_client::{
 		ClientBlockImportExt, ClientExt, DefaultTestClientBuilderExt, TestClientBuilder,
@@ -412,10 +412,7 @@ mod tests {
 				let justification = GrandpaJustification::from_commit(&client, 42, commit).unwrap();
 
 				client
-					.finalize_block(
-						BlockId::Hash(target_hash),
-						Some((GRANDPA_ENGINE_ID, justification.encode())),
-					)
+					.finalize_block(&target_hash, Some((GRANDPA_ENGINE_ID, justification.encode())))
 					.unwrap();
 
 				authority_set_changes.push((current_set_id, n));

client/network/sync/src/lib.rs

@@ -3172,9 +3172,7 @@ mod test {
 
 		let finalized_block = blocks[MAX_BLOCKS_TO_LOOK_BACKWARDS as usize * 2 - 1].clone();
 		let just = (*b"TEST", Vec::new());
-		client
-			.finalize_block(BlockId::Hash(finalized_block.hash()), Some(just))
-			.unwrap();
+		client.finalize_block(&finalized_block.hash(), Some(just)).unwrap();
 		sync.update_chain_info(&info.best_hash, info.best_number);
 
 		let peer_id1 = PeerId::random();
@@ -3303,9 +3301,7 @@ mod test {
 
 		let finalized_block = blocks[MAX_BLOCKS_TO_LOOK_BACKWARDS as usize * 2 - 1].clone();
 		let just = (*b"TEST", Vec::new());
-		client
-			.finalize_block(BlockId::Hash(finalized_block.hash()), Some(just))
-			.unwrap();
+		client.finalize_block(&finalized_block.hash(), Some(just)).unwrap();
 		sync.update_chain_info(&info.best_hash, info.best_number);
 
 		let peer_id1 = PeerId::random();