BlockId removal: refactor: Backend::append_justification (#12551)

* BlockId removal: refactor: Backend::append_justification

It changes the arguments of `Backend::append_justification`
from: block: `BlockId<Block>` to: hash: `&Block::Hash`

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

* Error message improved

Co-authored-by: Adrian Catangiu <adrian@parity.io>

* single error message in beefy::finalize

* println removed

Co-authored-by: Adrian Catangiu <adrian@parity.io>

substrate/client/api/src/backend.rs

@@ -27,7 +27,6 @@ use sp_blockchain;
 use sp_consensus::BlockOrigin;
 use sp_core::offchain::OffchainStorage;
 use sp_runtime::{
-	generic::BlockId,
 	traits::{Block as BlockT, HashFor, NumberFor},
 	Justification, Justifications, StateVersion, Storage,
 };
@@ -485,12 +484,12 @@ pub trait Backend<Block: BlockT>: AuxStore + Send + Sync {
 		justification: Option<Justification>,
 	) -> sp_blockchain::Result<()>;
 
-	/// Append justification to the block with the given Id.
+	/// Append justification to the block with the given `hash`.
 	///
 	/// This should only be called for blocks that are already finalized.
 	fn append_justification(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Justification,
 	) -> sp_blockchain::Result<()>;
 

substrate/client/api/src/in_mem.rs

@@ -295,10 +295,9 @@ impl<Block: BlockT> Blockchain<Block> {
 
 	fn append_justification(
 		&self,
-		id: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Justification,
 	) -> sp_blockchain::Result<()> {
-		let hash = self.expect_block_hash_from_id(&id)?;
 		let mut storage = self.storage.write();
 
 		let block = storage
@@ -745,10 +744,10 @@ where
 
 	fn append_justification(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Justification,
 	) -> sp_blockchain::Result<()> {
-		self.blockchain.append_justification(block, justification)
+		self.blockchain.append_justification(hash, justification)
 	}
 
 	fn blockchain(&self) -> &Self::Blockchain {
@@ -865,26 +864,27 @@ mod tests {
 	fn append_and_retrieve_justifications() {
 		let blockchain = test_blockchain();
 		let last_finalized = blockchain.last_finalized().unwrap();
-		let block = BlockId::Hash(last_finalized);
 
-		blockchain.append_justification(block, (ID2, vec![4])).unwrap();
+		blockchain.append_justification(&last_finalized, (ID2, vec![4])).unwrap();
 		let justifications = {
 			let mut just = Justifications::from((ID1, vec![3]));
 			just.append((ID2, vec![4]));
 			just
 		};
-		assert_eq!(blockchain.justifications(block).unwrap(), Some(justifications));
+		assert_eq!(
+			blockchain.justifications(BlockId::Hash(last_finalized)).unwrap(),
+			Some(justifications)
+		);
 	}
 
 	#[test]
 	fn store_duplicate_justifications_is_forbidden() {
 		let blockchain = test_blockchain();
 		let last_finalized = blockchain.last_finalized().unwrap();
-		let block = BlockId::Hash(last_finalized);
 
-		blockchain.append_justification(block, (ID2, vec![0])).unwrap();
+		blockchain.append_justification(&last_finalized, (ID2, vec![0])).unwrap();
 		assert!(matches!(
-			blockchain.append_justification(block, (ID2, vec![1])),
+			blockchain.append_justification(&last_finalized, (ID2, vec![1])),
 			Err(sp_blockchain::Error::BadJustification(_)),
 		));
 	}

substrate/client/beefy/src/worker.rs

@@ -500,20 +500,22 @@ where
 
 			self.on_demand_justifications.cancel_requests_older_than(block_num);
 
-			if let Err(e) = self.backend.append_justification(
+			if let Err(e) = self
-				BlockId::Number(block_num),
+				.backend
-				(BEEFY_ENGINE_ID, finality_proof.encode()),
+				.blockchain()
-			) {
+				.expect_block_hash_from_id(&BlockId::Number(block_num))
+				.and_then(|hash| {
+					self.links
+						.to_rpc_best_block_sender
+						.notify(|| Ok::<_, ()>(hash))
+						.expect("forwards closure result; the closure always returns Ok; qed.");
+
+					self.backend
+						.append_justification(&hash, (BEEFY_ENGINE_ID, finality_proof.encode()))
+				}) {
 				error!(target: "beefy", "🥩 Error {:?} on appending justification: {:?}", e, finality_proof);
 			}
 
-			self.backend.blockchain().hash(block_num).ok().flatten().map(|hash| {
-				self.links
-					.to_rpc_best_block_sender
-					.notify(|| Ok::<_, ()>(hash))
-					.expect("forwards closure result; the closure always returns Ok; qed.")
-			});
-
 			self.links
 				.to_rpc_justif_sender
 				.notify(|| Ok::<_, ()>(finality_proof))
@@ -1546,8 +1548,10 @@ pub(crate) mod tests {
 				commitment,
 				signatures: vec![None],
 			});
+			let hashof10 =
+				backend.blockchain().expect_block_hash_from_id(&BlockId::Number(10)).unwrap();
 			backend
-				.append_justification(BlockId::Number(10), (BEEFY_ENGINE_ID, justif.encode()))
+				.append_justification(&hashof10, (BEEFY_ENGINE_ID, justif.encode()))
 				.unwrap();
 
 			// initialize voter at block 13, expect rounds initialized at last beefy finalized 10
@@ -1580,8 +1584,10 @@ pub(crate) mod tests {
 				commitment,
 				signatures: vec![None],
 			});
+			let hashof12 =
+				backend.blockchain().expect_block_hash_from_id(&BlockId::Number(12)).unwrap();
 			backend
-				.append_justification(BlockId::Number(12), (BEEFY_ENGINE_ID, justif.encode()))
+				.append_justification(&hashof12, (BEEFY_ENGINE_ID, justif.encode()))
 				.unwrap();
 
 			// initialize voter at block 13, expect rounds initialized at last beefy finalized 12

substrate/client/db/src/lib.rs

@@ -2013,12 +2013,11 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 
 	fn append_justification(
 		&self,
-		block: BlockId<Block>,
+		hash: &Block::Hash,
 		justification: Justification,
 	) -> ClientResult<()> {
 		let mut transaction: Transaction<DbHash> = Transaction::new();
-		let hash = self.blockchain.expect_block_hash_from_id(&block)?;
+		let header = self.blockchain.expect_header(BlockId::Hash(*hash))?;
-		let header = self.blockchain.expect_header(block)?;
 		let number = *header.number();
 
 		// Check if the block is finalized first.
@@ -2027,13 +2026,13 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 
 		// We can do a quick check first, before doing a proper but more expensive check
 		if number > self.blockchain.info().finalized_number ||
-			(hash != last_finalized && !is_descendent_of(&hash, &last_finalized)?)
+			(*hash != last_finalized && !is_descendent_of(hash, &last_finalized)?)
 		{
 			return Err(ClientError::NotInFinalizedChain)
 		}
 
 		let justifications = if let Some(mut stored_justifications) =
-			self.blockchain.justifications(block)?
+			self.blockchain.justifications(BlockId::Hash(*hash))?
 		{
 			if !stored_justifications.append(justification) {
 				return Err(ClientError::BadJustification("Duplicate consensus engine ID".into()))
@@ -2045,7 +2044,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 
 		transaction.set_from_vec(
 			columns::JUSTIFICATIONS,
-			&utils::number_and_hash_to_lookup_key(number, hash)?,
+			&utils::number_and_hash_to_lookup_key(number, *hash)?,
 			justifications.encode(),
 		);
 
@@ -3029,11 +3028,11 @@ pub(crate) mod tests {
 		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();
+		backend.append_justification(&block1, just1.clone()).unwrap();
 
 		let just2 = (CONS1_ENGINE_ID, vec![6, 7]);
 		assert!(matches!(
-			backend.append_justification(BlockId::Number(1), just2),
+			backend.append_justification(&block1, just2),
 			Err(ClientError::BadJustification(_))
 		));