BlockId removal: refactor: BlockImportOperation+Bknd::finalize_block (#12535)

* BlockId removal: refactor: BlockImportOperation+Bknd::finalize_block

It changes the arguments of methods of `BlockImportOperation` trait
from: block: `BlockId<Block>` to: hash: `&Block::Hash`
`Backend::finalize_block` was also changed.

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

* Review suggestion applied

thx to @davxy

* trigger CI job

substrate/client/api/src/backend.rs

@@ -216,13 +216,13 @@ pub trait BlockImportOperation<Block: BlockT> {
 	/// Mark a block as finalized.
 	fn mark_finalized(
 		&mut self,
-		id: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()>;
 
 	/// Mark a block as new head. If both block import and set head are specified, set head
 	/// overrides block import's best block rule.
-	fn mark_head(&mut self, id: BlockId<Block>) -> sp_blockchain::Result<()>;
+	fn mark_head(&mut self, hash: &Block::Hash) -> sp_blockchain::Result<()>;
 
 	/// Add a transaction index operation.
 	fn update_transaction_index(&mut self, index: Vec<IndexOperation>)
@@ -476,12 +476,12 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
 		transaction: Self::BlockImportOperation,
 	) -> sp_blockchain::Result<()>;
 
-	/// Finalize block with given Id.
+	/// Finalize block with given `hash`.
 	///
 	/// This should only be called if the parent of the given block has been finalized.
 	fn finalize_block(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()>;
 

substrate/client/api/src/in_mem.rs

@@ -223,10 +223,10 @@ impl<Block: BlockT> Blockchain<Block> {
 	}
 
 	/// Set an existing block as head.
-	pub fn set_head(&self, id: BlockId<Block>) -> sp_blockchain::Result<()> {
+	pub fn set_head(&self, hash: Block::Hash) -> sp_blockchain::Result<()> {
 		let header = self
-			.header(id)?
-			.ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", id)))?;
+			.header(BlockId::Hash(hash))?
+			.ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", hash)))?;
 
 		self.apply_head(&header)
 	}
@@ -271,21 +271,16 @@ impl<Block: BlockT> Blockchain<Block> {
 
 	fn finalize_header(
 		&self,
-		id: BlockId<Block>,
+		block: &Block::Hash,
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()> {
-		let hash = match self.header(id)? {
-			Some(h) => h.hash(),
-			None => return Err(sp_blockchain::Error::UnknownBlock(format!("{}", id))),
-		};
-
 		let mut storage = self.storage.write();
-		storage.finalized_hash = hash;
+		storage.finalized_hash = *block;
 
 		if justification.is_some() {
 			let block = storage
 				.blocks
-				.get_mut(&hash)
+				.get_mut(block)
 				.expect("hash was fetched from a block in the db; qed");
 
 			let block_justifications = match block {
@@ -500,8 +495,8 @@ pub struct BlockImportOperation<Block: BlockT> {
 	new_state:
 		Option<<InMemoryBackend<HashFor<Block>> as StateBackend<HashFor<Block>>>::Transaction>,
 	aux: Vec<(Vec<u8>, Option<Vec<u8>>)>,
-	finalized_blocks: Vec<(BlockId<Block>, Option<Justification>)>,
-	set_head: Option<BlockId<Block>>,
+	finalized_blocks: Vec<(Block::Hash, Option<Justification>)>,
+	set_head: Option<Block::Hash>,
 }
 
 impl<Block: BlockT> BlockImportOperation<Block>
@@ -605,16 +600,16 @@ where
 
 	fn mark_finalized(
 		&mut self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()> {
-		self.finalized_blocks.push((block, justification));
+		self.finalized_blocks.push((*hash, justification));
 		Ok(())
 	}
 
-	fn mark_head(&mut self, block: BlockId<Block>) -> sp_blockchain::Result<()> {
+	fn mark_head(&mut self, hash: &Block::Hash) -> sp_blockchain::Result<()> {
 		assert!(self.pending_block.is_none(), "Only one set block per operation is allowed");
-		self.set_head = Some(block);
+		self.set_head = Some(*hash);
 		Ok(())
 	}
 
@@ -710,7 +705,7 @@ where
 	fn commit_operation(&self, operation: Self::BlockImportOperation) -> sp_blockchain::Result<()> {
 		if !operation.finalized_blocks.is_empty() {
 			for (block, justification) in operation.finalized_blocks {
-				self.blockchain.finalize_header(block, justification)?;
+				self.blockchain.finalize_header(&block, justification)?;
 			}
 		}
 
@@ -743,10 +738,10 @@ where
 
 	fn finalize_block(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()> {
-		self.blockchain.finalize_header(block, justification)
+		self.blockchain.finalize_header(hash, justification)
 	}
 
 	fn append_justification(

substrate/client/beefy/src/worker.rs

@@ -1371,8 +1371,10 @@ pub(crate) mod tests {
 		let mut best_block_stream = best_block_streams.drain(..).next().unwrap();
 		net.peer(0).push_blocks(2, false);
 		// finalize 1 and 2 without justifications
-		backend.finalize_block(BlockId::number(1), None).unwrap();
-		backend.finalize_block(BlockId::number(2), None).unwrap();
+		let hashof1 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(1)).unwrap();
+		let hashof2 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(2)).unwrap();
+		backend.finalize_block(&hashof1, None).unwrap();
+		backend.finalize_block(&hashof2, None).unwrap();
 
 		let justif = create_finality_proof(2);
 		// create new session at block #2

substrate/client/db/src/lib.rs

@@ -756,8 +756,8 @@ pub struct BlockImportOperation<Block: BlockT> {
 	offchain_storage_updates: OffchainChangesCollection,
 	pending_block: Option<PendingBlock<Block>>,
 	aux_ops: Vec<(Vec<u8>, Option<Vec<u8>>)>,
-	finalized_blocks: Vec<(BlockId<Block>, Option<Justification>)>,
-	set_head: Option<BlockId<Block>>,
+	finalized_blocks: Vec<(Block::Hash, Option<Justification>)>,
+	set_head: Option<Block::Hash>,
 	commit_state: bool,
 	index_ops: Vec<IndexOperation>,
 }
@@ -897,16 +897,16 @@ impl<Block: BlockT> sc_client_api::backend::BlockImportOperation<Block>
 
 	fn mark_finalized(
 		&mut self,
-		block: BlockId<Block>,
+		block: &Block::Hash,
 		justification: Option<Justification>,
 	) -> ClientResult<()> {
-		self.finalized_blocks.push((block, justification));
+		self.finalized_blocks.push((*block, justification));
 		Ok(())
 	}
 
-	fn mark_head(&mut self, block: BlockId<Block>) -> ClientResult<()> {
+	fn mark_head(&mut self, hash: &Block::Hash) -> ClientResult<()> {
 		assert!(self.set_head.is_none(), "Only one set head per operation is allowed");
-		self.set_head = Some(block);
+		self.set_head = Some(*hash);
 		Ok(())
 	}
 
@@ -1351,8 +1351,7 @@ impl<Block: BlockT> Backend<Block> {
 			(meta.best_number, meta.finalized_hash, meta.finalized_number, meta.block_gap)
 		};
 
-		for (block, justification) in operation.finalized_blocks {
-			let block_hash = self.blockchain.expect_block_hash_from_id(&block)?;
+		for (block_hash, justification) in operation.finalized_blocks {
 			let block_header = self.blockchain.expect_header(BlockId::Hash(block_hash))?;
 			meta_updates.push(self.finalize_block_with_transaction(
 				&mut transaction,
@@ -1624,9 +1623,10 @@ impl<Block: BlockT> Backend<Block> {
 		};
 
 		if let Some(set_head) = operation.set_head {
-			if let Some(header) =
-				sc_client_api::blockchain::HeaderBackend::header(&self.blockchain, set_head)?
-			{
+			if let Some(header) = sc_client_api::blockchain::HeaderBackend::header(
+				&self.blockchain,
+				BlockId::Hash(set_head),
+			)? {
 				let number = header.number();
 				let hash = header.hash();
 
@@ -1992,17 +1992,16 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 
 	fn finalize_block(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Option<Justification>,
 	) -> ClientResult<()> {
 		let mut transaction = Transaction::new();
-		let hash = self.blockchain.expect_block_hash_from_id(&block)?;
-		let header = self.blockchain.expect_header(block)?;
+		let header = self.blockchain.expect_header(BlockId::Hash(*hash))?;
 		let mut displaced = None;
 
 		let m = self.finalize_block_with_transaction(
 			&mut transaction,
-			&hash,
+			hash,
 			&header,
 			None,
 			justification,
@@ -2605,13 +2604,12 @@ pub(crate) mod tests {
 			header.state_root = root.into();
 
 			op.update_storage(storage, Vec::new()).unwrap();
-			op.set_block_data(header, Some(vec![]), None, None, NewBlockState::Best)
+			op.set_block_data(header.clone(), Some(vec![]), None, None, NewBlockState::Best)
 				.unwrap();
 
 			db.commit_operation(op).unwrap();
 
-			let hash = db.blockchain().expect_block_hash_from_id(&BlockId::Number(1)).unwrap();
-			let state = db.state_at(&hash).unwrap();
+			let state = db.state_at(&header.hash()).unwrap();
 
 			assert_eq!(state.storage(&[1, 3, 5]).unwrap(), None);
 			assert_eq!(state.storage(&[1, 2, 3]).unwrap(), Some(vec![9, 9, 9]));
@@ -2665,7 +2663,7 @@ pub(crate) mod tests {
 			hash
 		};
 
-		let hash = {
+		let hashof1 = {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Number(0)).unwrap();
 			let mut header = Header {
@@ -2702,12 +2700,12 @@ pub(crate) mod tests {
 			hash
 		};
 
-		let hash = {
+		let hashof2 = {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Number(1)).unwrap();
 			let mut header = Header {
 				number: 2,
-				parent_hash: hash,
+				parent_hash: hashof1,
 				state_root: Default::default(),
 				digest: Default::default(),
 				extrinsics_root: Default::default(),
@@ -2736,12 +2734,12 @@ pub(crate) mod tests {
 			hash
 		};
 
-		{
+		let hashof3 = {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Number(2)).unwrap();
 			let mut header = Header {
 				number: 3,
-				parent_hash: hash,
+				parent_hash: hashof2,
 				state_root: Default::default(),
 				digest: Default::default(),
 				extrinsics_root: Default::default(),
@@ -2754,6 +2752,7 @@ pub(crate) mod tests {
 				.storage_root(storage.iter().cloned().map(|(x, y)| (x, Some(y))), state_version)
 				.0
 				.into();
+			let hash = header.hash();
 
 			op.set_block_data(header, Some(vec![]), None, None, NewBlockState::Best)
 				.unwrap();
@@ -2764,11 +2763,12 @@ pub(crate) mod tests {
 				.db
 				.get(columns::STATE, &sp_trie::prefixed_key::<BlakeTwo256>(&key, EMPTY_PREFIX))
 				.is_none());
-		}
+			hash
+		};
 
-		backend.finalize_block(BlockId::Number(1), None).unwrap();
-		backend.finalize_block(BlockId::Number(2), None).unwrap();
-		backend.finalize_block(BlockId::Number(3), None).unwrap();
+		backend.finalize_block(&hashof1, None).unwrap();
+		backend.finalize_block(&hashof2, None).unwrap();
+		backend.finalize_block(&hashof3, None).unwrap();
 		assert!(backend
 			.storage
 			.db
@@ -2991,8 +2991,8 @@ pub(crate) mod tests {
 			vec![block2_a, block2_b, block2_c, block1_c]
 		);
 
-		backend.finalize_block(BlockId::hash(block1_a), None).unwrap();
-		backend.finalize_block(BlockId::hash(block2_a), None).unwrap();
+		backend.finalize_block(&block1_a, None).unwrap();
+		backend.finalize_block(&block2_a, None).unwrap();
 
 		// leaves at same height stay. Leaves at lower heights pruned.
 		assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a, block2_b, block2_c]);
@@ -3016,10 +3016,10 @@ pub(crate) mod tests {
 		let backend = Backend::<Block>::new_test(10, 10);
 
 		let block0 = insert_header(&backend, 0, Default::default(), None, Default::default());
-		let _ = insert_header(&backend, 1, block0, None, Default::default());
+		let block1 = insert_header(&backend, 1, block0, None, Default::default());
 
 		let justification = Some((CONS0_ENGINE_ID, vec![1, 2, 3]));
-		backend.finalize_block(BlockId::Number(1), justification.clone()).unwrap();
+		backend.finalize_block(&block1, justification.clone()).unwrap();
 
 		assert_eq!(
 			backend.blockchain().justifications(BlockId::Number(1)).unwrap(),
@@ -3034,10 +3034,10 @@ pub(crate) mod tests {
 		let backend = Backend::<Block>::new_test(10, 10);
 
 		let block0 = insert_header(&backend, 0, Default::default(), None, Default::default());
-		let _ = insert_header(&backend, 1, block0, None, Default::default());
+		let block1 = insert_header(&backend, 1, block0, None, Default::default());
 
 		let just0 = (CONS0_ENGINE_ID, vec![1, 2, 3]);
-		backend.finalize_block(BlockId::Number(1), Some(just0.clone().into())).unwrap();
+		backend.finalize_block(&block1, Some(just0.clone().into())).unwrap();
 
 		let just1 = (CONS1_ENGINE_ID, vec![4, 5]);
 		backend.append_justification(BlockId::Number(1), just1.clone()).unwrap();
@@ -3071,15 +3071,15 @@ pub(crate) mod tests {
 		{
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(block0)).unwrap();
-			op.mark_finalized(BlockId::Hash(block1), None).unwrap();
-			op.mark_finalized(BlockId::Hash(block2), None).unwrap();
+			op.mark_finalized(&block1, None).unwrap();
+			op.mark_finalized(&block2, None).unwrap();
 			backend.commit_operation(op).unwrap();
 		}
 		{
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(block2)).unwrap();
-			op.mark_finalized(BlockId::Hash(block3), None).unwrap();
-			op.mark_finalized(BlockId::Hash(block4), None).unwrap();
+			op.mark_finalized(&block3, None).unwrap();
+			op.mark_finalized(&block4, None).unwrap();
 			backend.commit_operation(op).unwrap();
 		}
 	}
@@ -3181,7 +3181,7 @@ pub(crate) mod tests {
 		{
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(block0)).unwrap();
-			op.mark_finalized(BlockId::Hash(block2), None).unwrap();
+			op.mark_finalized(&block2, None).unwrap();
 			backend.commit_operation(op).unwrap_err();
 		}
 	}
@@ -3210,7 +3210,7 @@ pub(crate) mod tests {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
 			for i in 1..5 {
-				op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap();
+				op.mark_finalized(&blocks[i], None).unwrap();
 			}
 			backend.commit_operation(op).unwrap();
 		}
@@ -3245,7 +3245,7 @@ pub(crate) mod tests {
 		let mut op = backend.begin_operation().unwrap();
 		backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
 		for i in 1..3 {
-			op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap();
+			op.mark_finalized(&blocks[i], None).unwrap();
 		}
 		backend.commit_operation(op).unwrap();
 
@@ -3301,7 +3301,7 @@ pub(crate) mod tests {
 
 		let mut op = backend.begin_operation().unwrap();
 		backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
-		op.mark_head(BlockId::Hash(blocks[4])).unwrap();
+		op.mark_head(&blocks[4]).unwrap();
 		backend.commit_operation(op).unwrap();
 
 		let bc = backend.blockchain();
@@ -3310,7 +3310,7 @@ pub(crate) mod tests {
 		for i in 1..5 {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(blocks[i])).unwrap();
-			op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap();
+			op.mark_finalized(&blocks[i], None).unwrap();
 			backend.commit_operation(op).unwrap();
 		}
 
@@ -3370,13 +3370,13 @@ pub(crate) mod tests {
 		.unwrap();
 		let mut op = backend.begin_operation().unwrap();
 		backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
-		op.mark_head(BlockId::Hash(blocks[4])).unwrap();
+		op.mark_head(&blocks[4]).unwrap();
 		backend.commit_operation(op).unwrap();
 
 		for i in 1..5 {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
-			op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap();
+			op.mark_finalized(&blocks[i], None).unwrap();
 			backend.commit_operation(op).unwrap();
 		}
 
@@ -3423,8 +3423,9 @@ pub(crate) mod tests {
 		assert_eq!(bc.indexed_transaction(&x1_hash).unwrap().unwrap(), &x1[1..]);
 
 		// Push one more blocks and make sure block is pruned and transaction index is cleared.
-		insert_block(&backend, 1, hash, None, Default::default(), vec![], None).unwrap();
-		backend.finalize_block(BlockId::Number(1), None).unwrap();
+		let block1 =
+			insert_block(&backend, 1, hash, None, Default::default(), vec![], None).unwrap();
+		backend.finalize_block(&block1, None).unwrap();
 		assert_eq!(bc.body(BlockId::Number(0)).unwrap(), None);
 		assert_eq!(bc.indexed_transaction(&x0_hash).unwrap(), None);
 		assert_eq!(bc.indexed_transaction(&x1_hash).unwrap(), None);
@@ -3501,7 +3502,7 @@ pub(crate) mod tests {
 		for i in 1..10 {
 			let mut op = backend.begin_operation().unwrap();
 			backend.begin_state_operation(&mut op, BlockId::Hash(blocks[4])).unwrap();
-			op.mark_finalized(BlockId::Hash(blocks[i]), None).unwrap();
+			op.mark_finalized(&blocks[i], None).unwrap();
 			backend.commit_operation(op).unwrap();
 			let bc = backend.blockchain();
 			if i < 6 {
@@ -3676,7 +3677,7 @@ pub(crate) mod tests {
 
 		let block1_a = insert_header(&backend, 1, block0, None, Default::default());
 		let block2_a = insert_header(&backend, 2, block1_a, None, Default::default());
-		backend.finalize_block(BlockId::hash(block1_a), None).unwrap();
+		backend.finalize_block(&block1_a, None).unwrap();
 		assert_eq!(backend.blockchain().leaves().unwrap(), vec![block2_a]);
 
 		// Insert a fork prior to finalization point. Leave should not be created.

substrate/client/service/src/client/client.rs

@@ -878,17 +878,17 @@ where
 			// plugable we cannot make a better choice here. usages that need
 			// an accurate "best" block need to go through `SelectChain`
 			// instead.
-			operation.op.mark_head(BlockId::Hash(block))?;
+			operation.op.mark_head(&block)?;
 		}
 
 		let enacted = route_from_finalized.enacted();
 		assert!(enacted.len() > 0);
 		for finalize_new in &enacted[..enacted.len() - 1] {
-			operation.op.mark_finalized(BlockId::Hash(finalize_new.hash), None)?;
+			operation.op.mark_finalized(&finalize_new.hash, None)?;
 		}
 
 		assert_eq!(enacted.last().map(|e| e.hash), Some(block));
-		operation.op.mark_finalized(BlockId::Hash(block), justification)?;
+		operation.op.mark_finalized(&block, justification)?;
 
 		if notify {
 			let finalized =