BlockId removal: refactor: BlockBackend::block|block_status (#13014)

* BlockId removal: refactor: BlockBackend::block|block_status

It changes the arguments of:
-  `BlockBackend::block`
-  `BlockBackend::block_status`

method from: `BlockId<Block>` to: `Block::Hash`

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

* non-obvious reworks

* doc fix

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <git@kchr.de>

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>

substrate/client/api/src/client.rs

@@ -21,7 +21,7 @@
 use sp_consensus::BlockOrigin;
 use sp_core::storage::StorageKey;
 use sp_runtime::{
-	generic::{BlockId, SignedBlock},
+	generic::SignedBlock,
 	traits::{Block as BlockT, NumberFor},
 	Justifications,
 };
@@ -120,14 +120,13 @@ pub trait BlockBackend<Block: BlockT> {
 	/// that are indexed by the runtime with `storage_index_transaction`.
 	fn block_indexed_body(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<Vec<Vec<u8>>>>;
 
-	/// Get full block by id.
-	fn block(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<SignedBlock<Block>>>;
+	/// Get full block by hash.
+	fn block(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<SignedBlock<Block>>>;
 
-	/// Get block status.
-	fn block_status(&self, id: &BlockId<Block>)
-		-> sp_blockchain::Result<sp_consensus::BlockStatus>;
+	/// Get block status by block hash.
+	fn block_status(&self, hash: Block::Hash) -> sp_blockchain::Result<sp_consensus::BlockStatus>;
 
-	/// Get block justifications for the block with the given id.
+	/// Get block justifications for the block with the given hash.
 	fn justifications(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<Justifications>>;
 
 	/// Get block hash by number.

substrate/client/cli/src/commands/export_blocks_cmd.rs

@@ -23,7 +23,7 @@ use crate::{
 };
 use clap::Parser;
 use log::info;
-use sc_client_api::{BlockBackend, UsageProvider};
+use sc_client_api::{BlockBackend, HeaderBackend, UsageProvider};
 use sc_service::{chain_ops::export_blocks, config::DatabaseSource};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use std::{fmt::Debug, fs, io, path::PathBuf, str::FromStr, sync::Arc};
@@ -73,7 +73,7 @@ impl ExportBlocksCmd {
 	) -> error::Result<()>
 	where
 		B: BlockT,
-		C: BlockBackend<B> + UsageProvider<B> + 'static,
+		C: HeaderBackend<B> + BlockBackend<B> + UsageProvider<B> + 'static,
 		<<B::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
 		if let Some(path) = database_config.path() {

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

@@ -86,7 +86,6 @@ use sp_consensus::{
 	BlockOrigin, BlockStatus,
 };
 use sp_runtime::{
-	generic::BlockId,
 	traits::{
 		Block as BlockT, CheckedSub, Hash, HashFor, Header as HeaderT, NumberFor, One,
 		SaturatedConversion, Zero,
@@ -1865,8 +1864,7 @@ where
 		self.best_queued_number = info.best_number;
 
 		if self.mode == SyncMode::Full &&
-			self.client.block_status(&BlockId::hash(info.best_hash))? !=
-				BlockStatus::InChainWithState
+			self.client.block_status(info.best_hash)? != BlockStatus::InChainWithState
 		{
 			self.import_existing = true;
 			// Latest state is missing, start with the last finalized state or genesis instead.
@@ -1898,7 +1896,7 @@ where
 		if self.queue_blocks.contains(hash) {
 			return Ok(BlockStatus::Queued)
 		}
-		self.client.block_status(&BlockId::Hash(*hash))
+		self.client.block_status(*hash)
 	}
 
 	/// Is the block corresponding to the given hash known?
@@ -2455,9 +2453,7 @@ where
 						if queue.contains(hash) {
 							BlockStatus::Queued
 						} else {
-							client
-								.block_status(&BlockId::Hash(*hash))
-								.unwrap_or(BlockStatus::Unknown)
+							client.block_status(*hash).unwrap_or(BlockStatus::Unknown)
 						}
 					},
 				) {
@@ -3202,6 +3198,7 @@ mod test {
 	};
 	use sp_blockchain::HeaderBackend;
 	use sp_consensus::block_validation::DefaultBlockAnnounceValidator;
+	use sp_runtime::generic::BlockId;
 	use substrate_test_runtime_client::{
 		runtime::{Block, Hash, Header},
 		BlockBuilderExt, ClientBlockImportExt, ClientExt, DefaultTestClientBuilderExt, TestClient,

substrate/client/rpc-spec-v2/src/chain_head/chain_head.rs

@@ -573,7 +573,7 @@ where
 				return
 			}
 
-			let event = match client.block(&BlockId::Hash(hash)) {
+			let event = match client.block(hash) {
 				Ok(Some(signed_block)) => {
 					let extrinsics = signed_block.block.extrinsics();
 					let result = format!("0x{:?}", HexDisplay::from(&extrinsics.encode()));

substrate/client/rpc/src/chain/chain_full.rs

@@ -29,10 +29,7 @@ use futures::{
 use jsonrpsee::SubscriptionSink;
 use sc_client_api::{BlockBackend, BlockchainEvents};
 use sp_blockchain::HeaderBackend;
-use sp_runtime::{
-	generic::{BlockId, SignedBlock},
-	traits::Block as BlockT,
-};
+use sp_runtime::{generic::SignedBlock, traits::Block as BlockT};
 
 /// Blockchain API backend for full nodes. Reads all the data from local database.
 pub struct FullChain<Block: BlockT, Client> {
@@ -66,7 +63,7 @@ where
 	}
 
 	fn block(&self, hash: Option<Block::Hash>) -> Result<Option<SignedBlock<Block>>, Error> {
-		self.client.block(&BlockId::Hash(self.unwrap_or_best(hash))).map_err(client_err)
+		self.client.block(self.unwrap_or_best(hash)).map_err(client_err)
 	}
 
 	fn subscribe_all_heads(&self, sink: SubscriptionSink) {

substrate/client/rpc/src/dev/mod.rs

@@ -69,10 +69,7 @@ where
 		self.deny_unsafe.check_if_safe()?;
 
 		let block = {
-			let block = self
-				.client
-				.block(&BlockId::Hash(hash))
-				.map_err(|e| Error::BlockQueryError(Box::new(e)))?;
+			let block = self.client.block(hash).map_err(|e| Error::BlockQueryError(Box::new(e)))?;
 			if let Some(block) = block {
 				let (mut header, body) = block.block.deconstruct();
 				// Remove the `Seal` to ensure we have the number of digests as expected by the

substrate/client/service/src/chain_ops/check_block.rs

@@ -18,34 +18,37 @@
 
 use crate::error::Error;
 use codec::Encode;
-use futures::{future, prelude::*};
 use sc_client_api::{BlockBackend, HeaderBackend};
 use sc_consensus::import_queue::ImportQueue;
 use sp_runtime::{generic::BlockId, traits::Block as BlockT};
 
 use crate::chain_ops::import_blocks;
-use std::{pin::Pin, sync::Arc};
+use std::sync::Arc;
 
 /// Re-validate known block.
-pub fn check_block<B, IQ, C>(
+pub async fn check_block<B, IQ, C>(
 	client: Arc<C>,
 	import_queue: IQ,
 	block_id: BlockId<B>,
-) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send>>
+) -> Result<(), Error>
 where
 	C: BlockBackend<B> + HeaderBackend<B> + Send + Sync + 'static,
 	B: BlockT + for<'de> serde::Deserialize<'de>,
 	IQ: ImportQueue<B> + 'static,
 {
-	match client.block(&block_id) {
-		Ok(Some(block)) => {
+	let maybe_block = client
+		.block_hash_from_id(&block_id)?
+		.map(|hash| client.block(hash))
+		.transpose()?
+		.flatten();
+	match maybe_block {
+		Some(block) => {
 			let mut buf = Vec::new();
 			1u64.encode_to(&mut buf);
 			block.encode_to(&mut buf);
 			let reader = std::io::Cursor::new(buf);
-			import_blocks(client, import_queue, reader, true, true)
+			import_blocks(client, import_queue, reader, true, true).await
 		},
-		Ok(None) => Box::pin(future::err("Unknown block".into())),
-		Err(e) => Box::pin(future::err(format!("Error reading block: {}", e).into())),
+		None => Err("Unknown block")?,
 	}
 }

substrate/client/service/src/chain_ops/export_blocks.rs

@@ -25,7 +25,7 @@ use sp_runtime::{
 	traits::{Block as BlockT, NumberFor, One, SaturatedConversion, Zero},
 };
 
-use sc_client_api::{BlockBackend, UsageProvider};
+use sc_client_api::{BlockBackend, HeaderBackend, UsageProvider};
 use std::{io::Write, pin::Pin, sync::Arc, task::Poll};
 
 /// Performs the blocks export.
@@ -37,7 +37,7 @@ pub fn export_blocks<B, C>(
 	binary: bool,
 ) -> Pin<Box<dyn Future<Output = Result<(), Error>>>>
 where
-	C: BlockBackend<B> + UsageProvider<B> + 'static,
+	C: HeaderBackend<B> + BlockBackend<B> + UsageProvider<B> + 'static,
 	B: BlockT,
 {
 	let mut block = from;
@@ -75,7 +75,12 @@ where
 			wrote_header = true;
 		}
 
-		match client.block(&BlockId::number(block))? {
+		match client
+			.block_hash_from_id(&BlockId::number(block))?
+			.map(|hash| client.block(hash))
+			.transpose()?
+			.flatten()
+		{
 			Some(block) =>
 				if binary {
 					output.write_all(&block.encode())?;
@@ -83,7 +88,6 @@ where
 					serde_json::to_writer(&mut output, &block)
 						.map_err(|e| format!("Error writing JSON: {}", e))?;
 				},
-			// Reached end of the chain.
 			None => return Poll::Ready(Ok(())),
 		}
 		if (block % 10000u32.into()).is_zero() {

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

@@ -784,7 +784,8 @@ where
 		let parent_hash = import_block.header.parent_hash();
 		let at = BlockId::Hash(*parent_hash);
 		let state_action = std::mem::replace(&mut import_block.state_action, StateAction::Skip);
-		let (enact_state, storage_changes) = match (self.block_status(&at)?, state_action) {
+		let (enact_state, storage_changes) = match (self.block_status(*parent_hash)?, state_action)
+		{
 			(BlockStatus::KnownBad, _) =>
 				return Ok(PrepareStorageChangesResult::Discard(ImportResult::KnownBad)),
 			(
@@ -1025,17 +1026,18 @@ where
 	}
 
 	/// Get block status.
-	pub fn block_status(&self, id: &BlockId<Block>) -> sp_blockchain::Result<BlockStatus> {
+	pub fn block_status(&self, hash: Block::Hash) -> sp_blockchain::Result<BlockStatus> {
 		// this can probably be implemented more efficiently
-		if let BlockId::Hash(ref h) = id {
-			if self.importing_block.read().as_ref().map_or(false, |importing| h == importing) {
-				return Ok(BlockStatus::Queued)
-			}
+		if self
+			.importing_block
+			.read()
+			.as_ref()
+			.map_or(false, |importing| &hash == importing)
+		{
+			return Ok(BlockStatus::Queued)
 		}
-		let hash_and_number = match *id {
-			BlockId::Hash(hash) => self.backend.blockchain().number(hash)?.map(|n| (hash, n)),
-			BlockId::Number(n) => self.backend.blockchain().hash(n)?.map(|hash| (hash, n)),
-		};
+
+		let hash_and_number = self.backend.blockchain().number(hash)?.map(|n| (hash, n));
 		match hash_and_number {
 			Some((hash, number)) =>
 				if self.backend.have_state_at(hash, number) {
@@ -1779,7 +1781,7 @@ where
 		// Own status must be checked first. If the block and ancestry is pruned
 		// this function must return `AlreadyInChain` rather than `MissingState`
 		match self
-			.block_status(&BlockId::Hash(hash))
+			.block_status(hash)
 			.map_err(|e| ConsensusError::ClientImport(e.to_string()))?
 		{
 			BlockStatus::InChainWithState | BlockStatus::Queued =>
@@ -1792,7 +1794,7 @@ where
 		}
 
 		match self
-			.block_status(&BlockId::Hash(parent_hash))
+			.block_status(parent_hash)
 			.map_err(|e| ConsensusError::ClientImport(e.to_string()))?
 		{
 			BlockStatus::InChainWithState | BlockStatus::Queued => {},
@@ -1947,20 +1949,16 @@ where
 		self.body(hash)
 	}
 
-	fn block(&self, id: &BlockId<Block>) -> sp_blockchain::Result<Option<SignedBlock<Block>>> {
-		Ok(match self.backend.blockchain().block_hash_from_id(id)? {
-			Some(hash) =>
-				match (self.header(hash)?, self.body(hash)?, self.justifications(hash)?) {
-					(Some(header), Some(extrinsics), justifications) =>
-						Some(SignedBlock { block: Block::new(header, extrinsics), justifications }),
-					_ => None,
-				},
-			None => None,
+	fn block(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<SignedBlock<Block>>> {
+		Ok(match (self.header(hash)?, self.body(hash)?, self.justifications(hash)?) {
+			(Some(header), Some(extrinsics), justifications) =>
+				Some(SignedBlock { block: Block::new(header, extrinsics), justifications }),
+			_ => None,
 		})
 	}
 
-	fn block_status(&self, id: &BlockId<Block>) -> sp_blockchain::Result<BlockStatus> {
-		Client::block_status(self, id)
+	fn block_status(&self, hash: Block::Hash) -> sp_blockchain::Result<BlockStatus> {
+		Client::block_status(self, hash)
 	}
 
 	fn justifications(&self, hash: Block::Hash) -> sp_blockchain::Result<Option<Justifications>> {
@@ -2055,9 +2053,9 @@ where
 {
 	fn block_status(
 		&self,
-		id: &BlockId<B>,
+		hash: B::Hash,
 	) -> Result<BlockStatus, Box<dyn std::error::Error + Send>> {
-		Client::block_status(self, id).map_err(|e| Box::new(e) as Box<_>)
+		Client::block_status(self, hash).map_err(|e| Box::new(e) as Box<_>)
 	}
 }
 

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

@@ -1137,7 +1137,7 @@ fn get_block_by_bad_block_hash_returns_none() {
 	let client = substrate_test_runtime_client::new();
 
 	let hash = H256::from_low_u64_be(5);
-	assert!(client.block(&BlockId::Hash(hash)).unwrap().is_none());
+	assert!(client.block(hash).unwrap().is_none());
 }
 
 #[test]
@@ -1497,10 +1497,7 @@ fn returns_status_for_pruned_blocks() {
 		block_on(client.check_block(check_block_a1.clone())).unwrap(),
 		ImportResult::imported(false),
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(),
-		BlockStatus::Unknown,
-	);
+	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::Unknown);
 
 	block_on(client.import_as_final(BlockOrigin::Own, a1.clone())).unwrap();
 
@@ -1508,10 +1505,7 @@ fn returns_status_for_pruned_blocks() {
 		block_on(client.check_block(check_block_a1.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(),
-		BlockStatus::InChainWithState,
-	);
+	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainWithState);
 
 	let a2 = client
 		.new_block_at(&BlockId::Hash(a1.hash()), Default::default(), false)
@@ -1534,18 +1528,12 @@ fn returns_status_for_pruned_blocks() {
 		block_on(client.check_block(check_block_a1.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(),
-		BlockStatus::InChainPruned,
-	);
+	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned);
 	assert_eq!(
 		block_on(client.check_block(check_block_a2.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(),
-		BlockStatus::InChainWithState,
-	);
+	assert_eq!(client.block_status(check_block_a2.hash).unwrap(), BlockStatus::InChainWithState);
 
 	let a3 = client
 		.new_block_at(&BlockId::Hash(a2.hash()), Default::default(), false)
@@ -1569,26 +1557,17 @@ fn returns_status_for_pruned_blocks() {
 		block_on(client.check_block(check_block_a1.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a1.hash)).unwrap(),
-		BlockStatus::InChainPruned,
-	);
+	assert_eq!(client.block_status(check_block_a1.hash).unwrap(), BlockStatus::InChainPruned);
 	assert_eq!(
 		block_on(client.check_block(check_block_a2.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a2.hash)).unwrap(),
-		BlockStatus::InChainPruned,
-	);
+	assert_eq!(client.block_status(check_block_a2.hash).unwrap(), BlockStatus::InChainPruned);
 	assert_eq!(
 		block_on(client.check_block(check_block_a3.clone())).unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	assert_eq!(
-		client.block_status(&BlockId::hash(check_block_a3.hash)).unwrap(),
-		BlockStatus::InChainWithState,
-	);
+	assert_eq!(client.block_status(check_block_a3.hash).unwrap(), BlockStatus::InChainWithState);
 
 	let mut check_block_b1 = BlockCheckParams {
 		hash: b1.hash(),

substrate/primitives/consensus/common/src/block_validation.rs

@@ -19,18 +19,18 @@
 
 use crate::BlockStatus;
 use futures::FutureExt as _;
-use sp_runtime::{generic::BlockId, traits::Block};
+use sp_runtime::traits::Block;
 use std::{error::Error, future::Future, pin::Pin, sync::Arc};
 
 /// A type which provides access to chain information.
 pub trait Chain<B: Block> {
-	/// Retrieve the status of the block denoted by the given [`BlockId`].
-	fn block_status(&self, id: &BlockId<B>) -> Result<BlockStatus, Box<dyn Error + Send>>;
+	/// Retrieve the status of the block denoted by the given [`Block::Hash`].
+	fn block_status(&self, hash: B::Hash) -> Result<BlockStatus, Box<dyn Error + Send>>;
 }
 
 impl<T: Chain<B>, B: Block> Chain<B> for Arc<T> {
-	fn block_status(&self, id: &BlockId<B>) -> Result<BlockStatus, Box<dyn Error + Send>> {
-		(&**self).block_status(id)
+	fn block_status(&self, hash: B::Hash) -> Result<BlockStatus, Box<dyn Error + Send>> {
+		(&**self).block_status(hash)
 	}
 }
 
@@ -60,7 +60,7 @@ pub trait BlockAnnounceValidator<B: Block> {
 	/// Returning [`Validation::Failure`] will lead to a decrease of the
 	/// peers reputation as it sent us invalid data.
 	///
-	/// The returned future should only resolve to an error iff there was an internal error
+	/// The returned future should only resolve to an error if there was an internal error
 	/// validating the block announcement. If the block announcement itself is invalid, this should
 	/// *always* return [`Validation::Failure`].
 	fn validate(

substrate/utils/frame/benchmarking-cli/src/block/bench.rs

@@ -94,9 +94,9 @@ where
 			let block_num = BlockId::Number(i.into());
 			let parent_num = BlockId::Number(((i - 1) as u32).into());
 			let consumed = self.consumed_weight(&block_num)?;
+			let hash = self.client.expect_block_hash_from_id(&block_num)?;
 
-			let block =
-				self.client.block(&block_num)?.ok_or(format!("Block {} not found", block_num))?;
+			let block = self.client.block(hash)?.ok_or(format!("Block {} not found", block_num))?;
 			let block = self.unsealed(block.block);
 			let took = self.measure_block(&block, &parent_num)?;