BlockId removal: refactor: StorageProvider (paritytech#12510)

* BlockId removal: refactor: StorageProvider

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

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

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <[email protected]>

* Update client/api/src/backend.rs

Co-authored-by: Dmitrii Markin <[email protected]>

* GrandpaBlockImport::current_set_id reworked

* ExportStateCmd reworked

* trigger CI job

* trigger CI job

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Dmitrii Markin <[email protected]>

client/api/src/backend.rs

@@ -357,77 +357,77 @@ where
 
 /// Provides acess to storage primitives
 pub trait StorageProvider<Block: BlockT, B: Backend<Block>> {
-	/// Given a `BlockId` and a key, return the value under the key in that block.
+	/// Given a block's `Hash` and a key, return the value under the key in that block.
 	fn storage(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		key: &StorageKey,
 	) -> sp_blockchain::Result<Option<StorageData>>;
 
-	/// Given a `BlockId` and a key prefix, return the matching storage keys in that block.
+	/// Given a block's `Hash` and a key prefix, return the matching storage keys in that block.
 	fn storage_keys(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		key_prefix: &StorageKey,
 	) -> sp_blockchain::Result<Vec<StorageKey>>;
 
-	/// Given a `BlockId` and a key, return the value under the hash in that block.
+	/// Given a block's `Hash` and a key, return the value under the hash in that block.
 	fn storage_hash(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		key: &StorageKey,
 	) -> sp_blockchain::Result<Option<Block::Hash>>;
 
-	/// Given a `BlockId` and a key prefix, return the matching child storage keys and values in
-	/// that block.
+	/// Given a block's `Hash` and a key prefix, return the matching child storage keys and values
+	/// in that block.
 	fn storage_pairs(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		key_prefix: &StorageKey,
 	) -> sp_blockchain::Result<Vec<(StorageKey, StorageData)>>;
 
-	/// Given a `BlockId` and a key prefix, return a `KeyIterator` iterates matching storage keys in
-	/// that block.
+	/// Given a block's `Hash` and a key prefix, return a `KeyIterator` iterates matching storage
+	/// keys in that block.
 	fn storage_keys_iter<'a>(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		prefix: Option<&'a StorageKey>,
 		start_key: Option<&StorageKey>,
 	) -> sp_blockchain::Result<KeyIterator<'a, B::State, Block>>;
 
-	/// Given a `BlockId`, a key and a child storage key, return the value under the key in that
-	/// block.
+	/// Given a block's `Hash`, a key and a child storage key, return the value under the key in
+	/// that block.
 	fn child_storage(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		child_info: &ChildInfo,
 		key: &StorageKey,
 	) -> sp_blockchain::Result<Option<StorageData>>;
 
-	/// Given a `BlockId`, a key prefix, and a child storage key, return the matching child storage
-	/// keys.
+	/// Given a block's `Hash`, a key prefix, and a child storage key, return the matching child
+	/// storage keys.
 	fn child_storage_keys(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		child_info: &ChildInfo,
 		key_prefix: &StorageKey,
 	) -> sp_blockchain::Result<Vec<StorageKey>>;
 
-	/// Given a `BlockId` and a key `prefix` and a child storage key,
+	/// Given a block's `Hash` and a key `prefix` and a child storage key,
 	/// return a `KeyIterator` that iterates matching storage keys in that block.
 	fn child_storage_keys_iter<'a>(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		child_info: ChildInfo,
 		prefix: Option<&'a StorageKey>,
 		start_key: Option<&StorageKey>,
 	) -> sp_blockchain::Result<KeyIterator<'a, B::State, Block>>;
 
-	/// Given a `BlockId`, a key and a child storage key, return the hash under the key in that
+	/// Given a block's `Hash`, a key and a child storage key, return the hash under the key in that
 	/// block.
 	fn child_storage_hash(
 		&self,
-		id: &BlockId<Block>,
+		hash: &Block::Hash,
 		child_info: &ChildInfo,
 		key: &StorageKey,
 	) -> sp_blockchain::Result<Option<Block::Hash>>;

client/cli/src/commands/export_state_cmd.rs

@@ -23,7 +23,7 @@ use crate::{
 };
 use clap::Parser;
 use log::info;
-use sc_client_api::{StorageProvider, UsageProvider};
+use sc_client_api::{HeaderBackend, StorageProvider, UsageProvider};
 use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
 use std::{fmt::Debug, io::Write, str::FromStr, sync::Arc};
 
@@ -57,15 +57,19 @@ impl ExportStateCmd {
 	) -> error::Result<()>
 	where
 		B: BlockT,
-		C: UsageProvider<B> + StorageProvider<B, BA>,
+		C: UsageProvider<B> + StorageProvider<B, BA> + HeaderBackend<B>,
 		BA: sc_client_api::backend::Backend<B>,
 		B::Hash: FromStr,
 		<B::Hash as FromStr>::Err: Debug,
 		<<B::Header as HeaderT>::Number as FromStr>::Err: Debug,
 	{
 		info!("Exporting raw state...");
 		let block_id = self.input.as_ref().map(|b| b.parse()).transpose()?;
-		let raw_state = sc_service::chain_ops::export_raw_state(client, block_id)?;
+		let hash = match block_id {
+			Some(id) => client.expect_block_hash_from_id(&id)?,
+			None => client.usage_info().chain.best_hash,
+		};
+		let raw_state = sc_service::chain_ops::export_raw_state(client, &hash)?;
 		input_spec.set_storage(raw_state);
 
 		info!("Generating new chain spec...");

client/finality-grandpa/src/import.rs

@@ -424,13 +424,15 @@ where
 	}
 
 	/// Read current set id form a given state.
-	fn current_set_id(&self, id: &BlockId<Block>) -> Result<SetId, ConsensusError> {
+	fn current_set_id(&self, hash: &Block::Hash) -> Result<SetId, ConsensusError> {
+		let id = &BlockId::hash(*hash);
 		let runtime_version = self.inner.runtime_api().version(id).map_err(|e| {
 			ConsensusError::ClientImport(format!(
 				"Unable to retrieve current runtime version. {}",
 				e
 			))
 		})?;
+
 		if runtime_version
 			.api_version(&<dyn GrandpaApi<Block>>::ID)
 			.map_or(false, |v| v < 3)
@@ -439,7 +441,8 @@ where
 			// This code may be removed once warp sync to an old runtime is no longer needed.
 			for prefix in ["GrandpaFinality", "Grandpa"] {
 				let k = [twox_128(prefix.as_bytes()), twox_128(b"CurrentSetId")].concat();
-				if let Ok(Some(id)) = self.inner.storage(id, &sc_client_api::StorageKey(k.to_vec()))
+				if let Ok(Some(id)) =
+					self.inner.storage(hash, &sc_client_api::StorageKey(k.to_vec()))
 				{
 					if let Ok(id) = SetId::decode(&mut id.0.as_ref()) {
 						return Ok(id)
@@ -472,13 +475,12 @@ where
 				// finality proofs and that the state is correct and final.
 				// So we can read the authority list and set id from the state.
 				self.authority_set_hard_forks.clear();
-				let block_id = BlockId::hash(hash);
 				let authorities = self
 					.inner
 					.runtime_api()
-					.grandpa_authorities(&block_id)
+					.grandpa_authorities(&BlockId::hash(hash))
 					.map_err(|e| ConsensusError::ClientImport(e.to_string()))?;
-				let set_id = self.current_set_id(&block_id)?;
+				let set_id = self.current_set_id(&hash)?;
 				let authority_set = AuthoritySet::new(
 					authorities.clone(),
 					set_id,

client/rpc/src/state/state_full.rs

@@ -145,10 +145,9 @@ where
 	) -> Result<()> {
 		for block_hash in &range.hashes {
 			let mut block_changes = StorageChangeSet { block: *block_hash, changes: Vec::new() };
-			let id = BlockId::hash(*block_hash);
 			for key in keys {
 				let (has_changed, data) = {
-					let curr_data = self.client.storage(&id, key).map_err(client_err)?;
+					let curr_data = self.client.storage(block_hash, key).map_err(client_err)?;
 					match last_values.get(key) {
 						Some(prev_data) => (curr_data != *prev_data, curr_data),
 						None => (true, curr_data),
@@ -214,7 +213,7 @@ where
 		prefix: StorageKey,
 	) -> std::result::Result<Vec<StorageKey>, Error> {
 		self.block_or_best(block)
-			.and_then(|block| self.client.storage_keys(&BlockId::Hash(block), &prefix))
+			.and_then(|block| self.client.storage_keys(&block, &prefix))
 			.map_err(client_err)
 	}
 
@@ -224,7 +223,7 @@ where
 		prefix: StorageKey,
 	) -> std::result::Result<Vec<(StorageKey, StorageData)>, Error> {
 		self.block_or_best(block)
-			.and_then(|block| self.client.storage_pairs(&BlockId::Hash(block), &prefix))
+			.and_then(|block| self.client.storage_pairs(&block, &prefix))
 			.map_err(client_err)
 	}
 
@@ -237,11 +236,7 @@ where
 	) -> std::result::Result<Vec<StorageKey>, Error> {
 		self.block_or_best(block)
 			.and_then(|block| {
-				self.client.storage_keys_iter(
-					&BlockId::Hash(block),
-					prefix.as_ref(),
-					start_key.as_ref(),
-				)
+				self.client.storage_keys_iter(&block, prefix.as_ref(), start_key.as_ref())
 			})
 			.map(|iter| iter.take(count as usize).collect())
 			.map_err(client_err)
@@ -253,7 +248,7 @@ where
 		key: StorageKey,
 	) -> std::result::Result<Option<StorageData>, Error> {
 		self.block_or_best(block)
-			.and_then(|block| self.client.storage(&BlockId::Hash(block), &key))
+			.and_then(|block| self.client.storage(&block, &key))
 			.map_err(client_err)
 	}
 
@@ -267,14 +262,14 @@ where
 			Err(e) => return Err(client_err(e)),
 		};
 
-		match self.client.storage(&BlockId::Hash(block), &key) {
+		match self.client.storage(&block, &key) {
 			Ok(Some(d)) => return Ok(Some(d.0.len() as u64)),
 			Err(e) => return Err(client_err(e)),
 			Ok(None) => {},
 		}
 
 		self.client
-			.storage_pairs(&BlockId::Hash(block), &key)
+			.storage_pairs(&block, &key)
 			.map(|kv| {
 				let item_sum = kv.iter().map(|(_, v)| v.0.len() as u64).sum::<u64>();
 				if item_sum > 0 {
@@ -292,7 +287,7 @@ where
 		key: StorageKey,
 	) -> std::result::Result<Option<Block::Hash>, Error> {
 		self.block_or_best(block)
-			.and_then(|block| self.client.storage_hash(&BlockId::Hash(block), &key))
+			.and_then(|block| self.client.storage_hash(&block, &key))
 			.map_err(client_err)
 	}
 
@@ -418,7 +413,7 @@ where
 			let changes = keys
 				.into_iter()
 				.map(|key| {
-					let v = self.client.storage(&BlockId::Hash(block), &key).ok().flatten();
+					let v = self.client.storage(&block, &key).ok().flatten();
 					(key, v)
 				})
 				.collect();
@@ -522,7 +517,7 @@ where
 						ChildInfo::new_default(storage_key),
 					None => return Err(sp_blockchain::Error::InvalidChildStorageKey),
 				};
-				self.client.child_storage_keys(&BlockId::Hash(block), &child_info, &prefix)
+				self.client.child_storage_keys(&block, &child_info, &prefix)
 			})
 			.map_err(client_err)
 	}
@@ -543,7 +538,7 @@ where
 					None => return Err(sp_blockchain::Error::InvalidChildStorageKey),
 				};
 				self.client.child_storage_keys_iter(
-					&BlockId::Hash(block),
+					&block,
 					child_info,
 					prefix.as_ref(),
 					start_key.as_ref(),
@@ -566,7 +561,7 @@ where
 						ChildInfo::new_default(storage_key),
 					None => return Err(sp_blockchain::Error::InvalidChildStorageKey),
 				};
-				self.client.child_storage(&BlockId::Hash(block), &child_info, &key)
+				self.client.child_storage(&block, &child_info, &key)
 			})
 			.map_err(client_err)
 	}
@@ -589,10 +584,7 @@ where
 
 		keys.into_iter()
 			.map(move |key| {
-				client
-					.clone()
-					.child_storage(&BlockId::Hash(block), &child_info, &key)
-					.map_err(client_err)
+				client.clone().child_storage(&block, &child_info, &key).map_err(client_err)
 			})
 			.collect()
 	}
@@ -610,7 +602,7 @@ where
 						ChildInfo::new_default(storage_key),
 					None => return Err(sp_blockchain::Error::InvalidChildStorageKey),
 				};
-				self.client.child_storage_hash(&BlockId::Hash(block), &child_info, &key)
+				self.client.child_storage_hash(&block, &child_info, &key)
 			})
 			.map_err(client_err)
 	}

client/service/src/chain_ops/export_raw_state.rs

@@ -19,25 +19,20 @@
 use crate::error::Error;
 use sc_client_api::{StorageProvider, UsageProvider};
 use sp_core::storage::{well_known_keys, ChildInfo, Storage, StorageChild, StorageKey, StorageMap};
-use sp_runtime::{generic::BlockId, traits::Block as BlockT};
+use sp_runtime::traits::Block as BlockT;
 
 use std::{collections::HashMap, sync::Arc};
 
 /// Export the raw state at the given `block`. If `block` is `None`, the
 /// best block will be used.
-pub fn export_raw_state<B, BA, C>(
-	client: Arc<C>,
-	block: Option<BlockId<B>>,
-) -> Result<Storage, Error>
+pub fn export_raw_state<B, BA, C>(client: Arc<C>, hash: &B::Hash) -> Result<Storage, Error>
 where
 	C: UsageProvider<B> + StorageProvider<B, BA>,
 	B: BlockT,
 	BA: sc_client_api::backend::Backend<B>,
 {
-	let block = block.unwrap_or_else(|| BlockId::Hash(client.usage_info().chain.best_hash));
-
 	let empty_key = StorageKey(Vec::new());
-	let mut top_storage = client.storage_pairs(&block, &empty_key)?;
+	let mut top_storage = client.storage_pairs(hash, &empty_key)?;
 	let mut children_default = HashMap::new();
 
 	// Remove all default child storage roots from the top storage and collect the child storage
@@ -52,10 +47,10 @@ where
 			StorageKey(key.0[well_known_keys::DEFAULT_CHILD_STORAGE_KEY_PREFIX.len()..].to_vec());
 		let child_info = ChildInfo::new_default(&key.0);
 
-		let keys = client.child_storage_keys(&block, &child_info, &empty_key)?;
+		let keys = client.child_storage_keys(hash, &child_info, &empty_key)?;
 		let mut pairs = StorageMap::new();
 		keys.into_iter().try_for_each(|k| {
-			if let Some(value) = client.child_storage(&block, &child_info, &k)? {
+			if let Some(value) = client.child_storage(hash, &child_info, &k)? {
 				pairs.insert(k.0, value.0);
 			}