BlockId removal: refactor: Backend::justifications (#12602)

* BlockId removal: refactor: Backend::justifications

It changes the arguments of `Backend::justifications` method from: `BlockId<Block>` to: `&Block::Hash`

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

* trigger CI job

* trigger CI job

* bug fix

* match -> if

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

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

substrate/client/api/src/client.rs

@@ -131,7 +131,7 @@ pub trait BlockBackend<Block: BlockT> {
 		-> sp_blockchain::Result<sp_consensus::BlockStatus>;
 
 	/// Get block justifications for the block with the given id.
-	fn justifications(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<Justifications>>;
+	fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result<Option<Justifications>>;
 
 	/// Get block hash by number.
 	fn block_hash(&self, number: NumberFor<Block>) -> sp_blockchain::Result<Option<Block::Hash>>;

substrate/client/api/src/in_mem.rs

@@ -415,10 +415,8 @@ impl<Block: BlockT> blockchain::Backend<Block> for Blockchain<Block> {
 			.and_then(|b| b.extrinsics().map(|x| x.to_vec())))
 	}
 
-	fn justifications(&self, id: BlockId<Block>) -> sp_blockchain::Result<Option<Justifications>> {
-		Ok(self.id(id).and_then(|hash| {
-			self.storage.read().blocks.get(&hash).and_then(|b| b.justifications().cloned())
-		}))
+	fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result<Option<Justifications>> {
+		Ok(self.storage.read().blocks.get(&hash).and_then(|b| b.justifications().cloned()))
 	}
 
 	fn last_finalized(&self) -> sp_blockchain::Result<Block::Hash> {
@@ -816,7 +814,7 @@ pub fn check_genesis_storage(storage: &Storage) -> sp_blockchain::Result<()> {
 #[cfg(test)]
 mod tests {
 	use crate::{in_mem::Blockchain, NewBlockState};
-	use sp_api::{BlockId, HeaderT};
+	use sp_api::HeaderT;
 	use sp_blockchain::Backend;
 	use sp_runtime::{ConsensusEngineId, Justifications};
 	use substrate_test_runtime::{Block, Header, H256};
@@ -870,10 +868,7 @@ mod tests {
 			just.append((ID2, vec![4]));
 			just
 		};
-		assert_eq!(
-			blockchain.justifications(BlockId::Hash(last_finalized)).unwrap(),
-			Some(justifications)
-		);
+		assert_eq!(blockchain.justifications(&last_finalized).unwrap(), Some(justifications));
 	}
 
 	#[test]

substrate/client/beefy/src/communication/request_response/incoming_requests_handler.rs

@@ -26,7 +26,7 @@ use log::{debug, trace};
 use sc_client_api::BlockBackend;
 use sc_network::{config as netconfig, config::RequestResponseConfig, PeerId, ReputationChange};
 use sc_network_common::protocol::ProtocolName;
-use sp_runtime::{generic::BlockId, traits::Block};
+use sp_runtime::traits::Block;
 use std::{marker::PhantomData, sync::Arc};
 
 use crate::communication::request_response::{
@@ -149,13 +149,18 @@ where
 	fn handle_request(&self, request: IncomingRequest<B>) -> Result<(), Error> {
 		// TODO (issue #12293): validate `request` and change peer reputation for invalid requests.
 
-		let maybe_encoded_proof = self
-			.client
-			.justifications(&BlockId::Number(request.payload.begin))
-			.map_err(Error::Client)?
-			.and_then(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned())
-			// No BEEFY justification present.
-			.ok_or(());
+		let maybe_encoded_proof = if let Some(hash) =
+			self.client.block_hash(request.payload.begin).map_err(Error::Client)?
+		{
+			self.client
+				.justifications(&hash)
+				.map_err(Error::Client)?
+				.and_then(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned())
+				// No BEEFY justification present.
+				.ok_or(())
+		} else {
+			Err(())
+		};
 
 		request
 			.pending_response

substrate/client/beefy/src/tests.rs

@@ -741,9 +741,9 @@ fn beefy_importing_blocks() {
 
 	let full_client = client.as_client();
 	let parent_id = BlockId::Number(0);
-	let block_id = BlockId::Number(1);
 	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
 	let block = builder.build().unwrap().block;
+	let hashof1 = block.header.hash();
 
 	// Import without justifications.
 	let mut justif_recv = justif_stream.subscribe();
@@ -760,7 +760,7 @@ fn beefy_importing_blocks() {
 		// none in backend,
 		assert_eq!(
 			full_client
-				.justifications(&block_id)
+				.justifications(&hashof1)
 				.unwrap()
 				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
 			None
@@ -784,6 +784,7 @@ fn beefy_importing_blocks() {
 
 	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
 	let block = builder.build().unwrap().block;
+	let hashof2 = block.header.hash();
 	let mut justif_recv = justif_stream.subscribe();
 	assert_eq!(
 		block_on(block_import.import_block(params(block, justif), HashMap::new())).unwrap(),
@@ -798,7 +799,7 @@ fn beefy_importing_blocks() {
 		// still not in backend (worker is responsible for appending to backend),
 		assert_eq!(
 			full_client
-				.justifications(&block_id)
+				.justifications(&hashof2)
 				.unwrap()
 				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
 			None
@@ -826,6 +827,7 @@ fn beefy_importing_blocks() {
 
 	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
 	let block = builder.build().unwrap().block;
+	let hashof3 = block.header.hash();
 	let mut justif_recv = justif_stream.subscribe();
 	assert_eq!(
 		block_on(block_import.import_block(params(block, justif), HashMap::new())).unwrap(),
@@ -841,7 +843,7 @@ fn beefy_importing_blocks() {
 		// none in backend,
 		assert_eq!(
 			full_client
-				.justifications(&BlockId::Number(block_num))
+				.justifications(&hashof3)
 				.unwrap()
 				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
 			None

substrate/client/beefy/src/worker.rs

@@ -709,7 +709,7 @@ where
 			// a BEEFY justification, or at this session's boundary; voter will resume from there.
 			loop {
 				if let Some(true) = blockchain
-					.justifications(BlockId::hash(header.hash()))
+					.justifications(&header.hash())
 					.ok()
 					.flatten()
 					.map(|justifs| justifs.get(BEEFY_ENGINE_ID).is_some())
@@ -1401,7 +1401,7 @@ pub(crate) mod tests {
 		}));
 
 		// check BEEFY justifications are also appended to backend
-		let justifs = backend.blockchain().justifications(BlockId::number(2)).unwrap().unwrap();
+		let justifs = backend.blockchain().justifications(&hashof2).unwrap().unwrap();
 		assert!(justifs.get(BEEFY_ENGINE_ID).is_some())
 	}
 

substrate/client/db/src/lib.rs

@@ -642,8 +642,13 @@ impl<Block: BlockT> sc_client_api::blockchain::Backend<Block> for BlockchainDb<B
 		Ok(None)
 	}
 
-	fn justifications(&self, id: BlockId<Block>) -> ClientResult<Option<Justifications>> {
-		match read_db(&*self.db, columns::KEY_LOOKUP, columns::JUSTIFICATIONS, id)? {
+	fn justifications(&self, hash: &Block::Hash) -> ClientResult<Option<Justifications>> {
+		match read_db(
+			&*self.db,
+			columns::KEY_LOOKUP,
+			columns::JUSTIFICATIONS,
+			BlockId::<Block>::Hash(*hash),
+		)? {
 			Some(justifications) => match Decode::decode(&mut &justifications[..]) {
 				Ok(justifications) => Ok(Some(justifications)),
 				Err(err) =>
@@ -2039,7 +2044,7 @@ impl<Block: BlockT> sc_client_api::backend::Backend<Block> for Backend<Block> {
 		}
 
 		let justifications = if let Some(mut stored_justifications) =
-			self.blockchain.justifications(BlockId::Hash(*hash))?
+			self.blockchain.justifications(hash)?
 		{
 			if !stored_justifications.append(justification) {
 				return Err(ClientError::BadJustification("Duplicate consensus engine ID".into()))
@@ -3017,7 +3022,7 @@ pub(crate) mod tests {
 		backend.finalize_block(&block1, justification.clone()).unwrap();
 
 		assert_eq!(
-			backend.blockchain().justifications(BlockId::Number(1)).unwrap(),
+			backend.blockchain().justifications(&block1).unwrap(),
 			justification.map(Justifications::from),
 		);
 	}
@@ -3048,10 +3053,7 @@ pub(crate) mod tests {
 			just.append(just1);
 			just
 		};
-		assert_eq!(
-			backend.blockchain().justifications(BlockId::Number(1)).unwrap(),
-			Some(justifications),
-		);
+		assert_eq!(backend.blockchain().justifications(&block1).unwrap(), Some(justifications),);
 	}
 
 	#[test]

substrate/client/finality-grandpa/src/finality_proof.rs

@@ -183,10 +183,12 @@ where
 			}
 		},
 		AuthoritySetChangeId::Set(_, last_block_for_set) => {
-			let last_block_for_set_id = BlockId::Number(last_block_for_set);
+			let last_block_for_set_id = backend
+				.blockchain()
+				.expect_block_hash_from_id(&BlockId::Number(last_block_for_set))?;
 			let justification = if let Some(grandpa_justification) = backend
 				.blockchain()
-				.justifications(last_block_for_set_id)?
+				.justifications(&last_block_for_set_id)?
 				.and_then(|justifications| justifications.into_justification(GRANDPA_ENGINE_ID))
 			{
 				grandpa_justification

substrate/client/finality-grandpa/src/tests.rs

@@ -363,9 +363,11 @@ fn finalize_3_voters_no_observers() {
 	runtime.spawn(initialize_grandpa(&mut net, peers));
 	net.peer(0).push_blocks(20, false);
 	net.block_until_sync();
+	let hashof20 = net.peer(0).client().info().best_hash;
 
 	for i in 0..3 {
 		assert_eq!(net.peer(i).client().info().best_number, 20, "Peer #{} failed to sync", i);
+		assert_eq!(net.peer(i).client().info().best_hash, hashof20, "Peer #{} failed to sync", i);
 	}
 
 	let net = Arc::new(Mutex::new(net));
@@ -373,12 +375,7 @@ fn finalize_3_voters_no_observers() {
 
 	// normally there's no justification for finalized blocks
 	assert!(
-		net.lock()
-			.peer(0)
-			.client()
-			.justifications(&BlockId::Number(20))
-			.unwrap()
-			.is_none(),
+		net.lock().peer(0).client().justifications(&hashof20).unwrap().is_none(),
 		"Extra justification for block#1",
 	);
 }
@@ -616,19 +613,15 @@ fn justification_is_generated_periodically() {
 	net.peer(0).push_blocks(32, false);
 	net.block_until_sync();
 
+	let hashof32 = net.peer(0).client().info().best_hash;
+
 	let net = Arc::new(Mutex::new(net));
 	run_to_completion(&mut runtime, 32, net.clone(), peers);
 
 	// when block#32 (justification_period) is finalized, justification
 	// is required => generated
 	for i in 0..3 {
-		assert!(net
-			.lock()
-			.peer(i)
-			.client()
-			.justifications(&BlockId::Number(32))
-			.unwrap()
-			.is_some());
+		assert!(net.lock().peer(i).client().justifications(&hashof32).unwrap().is_some());
 	}
 }
 
@@ -648,7 +641,7 @@ fn sync_justifications_on_change_blocks() {
 	net.peer(0).push_blocks(20, false);
 
 	// at block 21 we do add a transition which is instant
-	net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| {
+	let hashof21 = net.peer(0).generate_blocks(1, BlockOrigin::File, |builder| {
 		let mut block = builder.build().unwrap().block;
 		add_scheduled_change(
 			&mut block,
@@ -672,25 +665,12 @@ fn sync_justifications_on_change_blocks() {
 	// the first 3 peers are grandpa voters and therefore have already finalized
 	// block 21 and stored a justification
 	for i in 0..3 {
-		assert!(net
-			.lock()
-			.peer(i)
-			.client()
-			.justifications(&BlockId::Number(21))
-			.unwrap()
-			.is_some());
+		assert!(net.lock().peer(i).client().justifications(&hashof21).unwrap().is_some());
 	}
 
 	// the last peer should get the justification by syncing from other peers
 	futures::executor::block_on(futures::future::poll_fn(move |cx| {
-		if net
-			.lock()
-			.peer(3)
-			.client()
-			.justifications(&BlockId::Number(21))
-			.unwrap()
-			.is_none()
-		{
+		if net.lock().peer(3).client().justifications(&hashof21).unwrap().is_none() {
 			net.lock().poll(cx);
 			Poll::Pending
 		} else {
@@ -1686,7 +1666,7 @@ fn imports_justification_for_regular_blocks_on_import() {
 	);
 
 	// the justification should be imported and available from the client
-	assert!(client.justifications(&BlockId::Hash(block_hash)).unwrap().is_some());
+	assert!(client.justifications(&block_hash).unwrap().is_some());
 }
 
 #[test]

substrate/client/finality-grandpa/src/warp_proof.rs

@@ -130,7 +130,7 @@ impl<Block: BlockT> WarpSyncProof<Block> {
 			}
 
 			let justification = blockchain
-				.justifications(BlockId::Number(*last_block))?
+				.justifications(&header.hash())?
 				.and_then(|just| just.into_justification(GRANDPA_ENGINE_ID))
 				.expect(
 					"header is last in set and contains standard change signal; \

substrate/client/network/sync/src/block_request_handler.rs

@@ -331,11 +331,8 @@ where
 			let number = *header.number();
 			let hash = header.hash();
 			let parent_hash = *header.parent_hash();
-			let justifications = if get_justification {
-				self.client.justifications(&BlockId::Hash(hash))?
-			} else {
-				None
-			};
+			let justifications =
+				if get_justification { self.client.justifications(&hash)? } else { None };
 
 			let (justifications, justification, is_empty_justification) =
 				if support_multiple_justifications {

substrate/client/network/test/src/block_import.rs

@@ -40,7 +40,7 @@ fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock<Block>)
 
 	let (hash, number) = (client.block_hash(1).unwrap().unwrap(), 1);
 	let header = client.header(&BlockId::Number(1)).unwrap();
-	let justifications = client.justifications(&BlockId::Number(1)).unwrap();
+	let justifications = client.justifications(&hash).unwrap();
 	let peer_id = PeerId::random();
 	(
 		client,

substrate/client/network/test/src/lib.rs

@@ -176,8 +176,11 @@ impl PeersClient {
 		self.backend.have_state_at(&header.hash(), *header.number())
 	}
 
-	pub fn justifications(&self, block: &BlockId<Block>) -> ClientResult<Option<Justifications>> {
-		self.client.justifications(block)
+	pub fn justifications(
+		&self,
+		hash: &<Block as BlockT>::Hash,
+	) -> ClientResult<Option<Justifications>> {
+		self.client.justifications(hash)
 	}
 
 	pub fn finality_notification_stream(&self) -> FinalityNotifications<Block> {

substrate/client/network/test/src/sync.rs

@@ -246,15 +246,16 @@ fn sync_justifications() {
 	net.peer(0).push_blocks(20, false);
 	net.block_until_sync();
 
-	// there's currently no justification for block #10
-	assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None);
-	assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None);
-
-	// we finalize block #10, #15 and #20 for peer 0 with a justification
 	let backend = net.peer(0).client().as_backend();
 	let hashof10 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(10)).unwrap();
 	let hashof15 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(15)).unwrap();
 	let hashof20 = backend.blockchain().expect_block_hash_from_id(&BlockId::Number(20)).unwrap();
+
+	// there's currently no justification for block #10
+	assert_eq!(net.peer(0).client().justifications(&hashof10).unwrap(), None);
+	assert_eq!(net.peer(1).client().justifications(&hashof10).unwrap(), None);
+
+	// we finalize block #10, #15 and #20 for peer 0 with a justification
 	let just = (*b"FRNK", Vec::new());
 	net.peer(0)
 		.client()
@@ -269,25 +270,25 @@ fn sync_justifications() {
 		.finalize_block(&hashof20, Some(just.clone()), true)
 		.unwrap();
 
-	let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap();
-	let h2 = net.peer(1).client().header(&BlockId::Number(15)).unwrap().unwrap();
-	let h3 = net.peer(1).client().header(&BlockId::Number(20)).unwrap().unwrap();
+	let hashof10 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap().hash();
+	let hashof15 = net.peer(1).client().header(&BlockId::Number(15)).unwrap().unwrap().hash();
+	let hashof20 = net.peer(1).client().header(&BlockId::Number(20)).unwrap().unwrap().hash();
 
 	// peer 1 should get the justifications from the network
-	net.peer(1).request_justification(&h1.hash().into(), 10);
-	net.peer(1).request_justification(&h2.hash().into(), 15);
-	net.peer(1).request_justification(&h3.hash().into(), 20);
+	net.peer(1).request_justification(&hashof10, 10);
+	net.peer(1).request_justification(&hashof15, 15);
+	net.peer(1).request_justification(&hashof20, 20);
 
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		net.poll(cx);
 
-		for height in (10..21).step_by(5) {
-			if net.peer(0).client().justifications(&BlockId::Number(height)).unwrap() !=
+		for hash in [hashof10, hashof15, hashof20] {
+			if net.peer(0).client().justifications(&hash).unwrap() !=
 				Some(Justifications::from((*b"FRNK", Vec::new())))
 			{
 				return Poll::Pending
 			}
-			if net.peer(1).client().justifications(&BlockId::Number(height)).unwrap() !=
+			if net.peer(1).client().justifications(&hash).unwrap() !=
 				Some(Justifications::from((*b"FRNK", Vec::new())))
 			{
 				return Poll::Pending
@@ -321,9 +322,9 @@ fn sync_justifications_across_forks() {
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		net.poll(cx);
 
-		if net.peer(0).client().justifications(&BlockId::Number(10)).unwrap() ==
+		if net.peer(0).client().justifications(&f1_best).unwrap() ==
 			Some(Justifications::from((*b"FRNK", Vec::new()))) &&
-			net.peer(1).client().justifications(&BlockId::Number(10)).unwrap() ==
+			net.peer(1).client().justifications(&f1_best).unwrap() ==
 				Some(Justifications::from((*b"FRNK", Vec::new())))
 		{
 			Poll::Ready(())
@@ -952,14 +953,14 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() {
 	net.peer(0).push_blocks(10, false);
 	net.block_until_sync();
 
-	// there's currently no justification for block #10
-	assert_eq!(net.peer(0).client().justifications(&BlockId::Number(10)).unwrap(), None);
-	assert_eq!(net.peer(1).client().justifications(&BlockId::Number(10)).unwrap(), None);
+	let hashof10 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap().hash();
 
-	let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap();
+	// there's currently no justification for block #10
+	assert_eq!(net.peer(0).client().justifications(&hashof10).unwrap(), None);
+	assert_eq!(net.peer(1).client().justifications(&hashof10).unwrap(), None);
 
 	// Let's assume block 10 was finalized, but we still need the justification from the network.
-	net.peer(1).request_justification(&h1.hash().into(), 10);
+	net.peer(1).request_justification(&hashof10, 10);
 
 	// Let's build some more blocks and wait always for the network to have synced them
 	for _ in 0..5 {
@@ -987,7 +988,7 @@ fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() {
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		net.poll(cx);
 
-		if net.peer(1).client().justifications(&BlockId::Number(10)).unwrap() !=
+		if net.peer(1).client().justifications(&hashof10).unwrap() !=
 			Some(Justifications::from((*b"FRNK", Vec::new())))
 		{
 			return Poll::Pending

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

@@ -1948,7 +1948,7 @@ where
 		Ok(match self.header(id)? {
 			Some(header) => {
 				let hash = header.hash();
-				match (self.body(&hash)?, self.justifications(id)?) {
+				match (self.body(&hash)?, self.justifications(&hash)?) {
 					(Some(extrinsics), justifications) =>
 						Some(SignedBlock { block: Block::new(header, extrinsics), justifications }),
 					_ => None,
@@ -1962,8 +1962,8 @@ where
 		Client::block_status(self, id)
 	}
 
-	fn justifications(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<Justifications>> {
-		self.backend.blockchain().justifications(*id)
+	fn justifications(&self, hash: &Block::Hash) -> sp_blockchain::Result<Option<Justifications>> {
+		self.backend.blockchain().justifications(hash)
 	}
 
 	fn block_hash(&self, number: NumberFor<Block>) -> sp_blockchain::Result<Option<Block::Hash>> {

substrate/client/service/test/src/client/mod.rs

@@ -884,11 +884,11 @@ fn import_with_justification() {
 
 	assert_eq!(client.chain_info().finalized_hash, a3.hash());
 
-	assert_eq!(client.justifications(&BlockId::Hash(a3.hash())).unwrap(), Some(justification));
+	assert_eq!(client.justifications(&a3.hash()).unwrap(), Some(justification));
 
-	assert_eq!(client.justifications(&BlockId::Hash(a1.hash())).unwrap(), None);
+	assert_eq!(client.justifications(&a1.hash()).unwrap(), None);
 
-	assert_eq!(client.justifications(&BlockId::Hash(a2.hash())).unwrap(), None);
+	assert_eq!(client.justifications(&a2.hash()).unwrap(), None);
 
 	finality_notification_check(&mut finality_notifications, &[a1.hash(), a2.hash()], &[]);
 	finality_notification_check(&mut finality_notifications, &[a3.hash()], &[]);

substrate/primitives/blockchain/src/backend.rs

@@ -91,7 +91,7 @@ pub trait Backend<Block: BlockT>:
 	/// Get block body. Returns `None` if block is not found.
 	fn body(&self, hash: &Block::Hash) -> Result<Option<Vec<<Block as BlockT>::Extrinsic>>>;
 	/// Get block justifications. Returns `None` if no justification exists.
-	fn justifications(&self, id: BlockId<Block>) -> Result<Option<Justifications>>;
+	fn justifications(&self, hash: &Block::Hash) -> Result<Option<Justifications>>;
 	/// Get last finalized block hash.
 	fn last_finalized(&self) -> Result<Block::Hash>;