BlockId removal: refactor: CallExecutor trait (paritytech#13173)

* BlockId removal: refactor: CallExecutor trait

It changes the arguments of CallExecutor methods:
-  `call`, 'contextual_call', 'runtime_version', 'prove_execution'

from: `BlockId<Block>` to: `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 <git@kchr.de>

* ".git/.scripts/commands/fmt/fmt.sh"

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

Author: 2 people

client/api/src/call_executor.rs

@@ -19,7 +19,7 @@
 //! A method call executor interface.
 
 use sc_executor::{RuntimeVersion, RuntimeVersionOf};
-use sp_runtime::{generic::BlockId, traits::Block as BlockT};
+use sp_runtime::traits::Block as BlockT;
 use sp_state_machine::{ExecutionStrategy, OverlayedChanges, StorageProof};
 use std::cell::RefCell;
 
@@ -54,7 +54,7 @@ pub trait CallExecutor<B: BlockT>: RuntimeVersionOf {
 	/// No changes are made.
 	fn call(
 		&self,
-		id: &BlockId<B>,
+		at_hash: B::Hash,
 		method: &str,
 		call_data: &[u8],
 		strategy: ExecutionStrategy,
@@ -67,7 +67,7 @@ pub trait CallExecutor<B: BlockT>: RuntimeVersionOf {
 	/// of the execution context.
 	fn contextual_call(
 		&self,
-		at: &BlockId<B>,
+		at_hash: B::Hash,
 		method: &str,
 		call_data: &[u8],
 		changes: &RefCell<OverlayedChanges>,
@@ -83,14 +83,14 @@ pub trait CallExecutor<B: BlockT>: RuntimeVersionOf {
 	/// Extract RuntimeVersion of given block
 	///
 	/// No changes are made.
-	fn runtime_version(&self, id: &BlockId<B>) -> Result<RuntimeVersion, sp_blockchain::Error>;
+	fn runtime_version(&self, at_hash: B::Hash) -> Result<RuntimeVersion, sp_blockchain::Error>;
 
 	/// Prove the execution of the given `method`.
 	///
 	/// No changes are made.
 	fn prove_execution(
 		&self,
-		at: &BlockId<B>,
+		at_hash: B::Hash,
 		method: &str,
 		call_data: &[u8],
 	) -> Result<(Vec<u8>, StorageProof), sp_blockchain::Error>;

client/finality-grandpa/src/lib.rs

@@ -464,18 +464,18 @@ pub trait GenesisAuthoritySetProvider<Block: BlockT> {
 	fn get(&self) -> Result<AuthorityList, ClientError>;
 }
 
-impl<Block: BlockT, E> GenesisAuthoritySetProvider<Block>
-	for Arc<dyn ExecutorProvider<Block, Executor = E>>
+impl<Block: BlockT, E, Client> GenesisAuthoritySetProvider<Block> for Arc<Client>
 where
 	E: CallExecutor<Block>,
+	Client: ExecutorProvider<Block, Executor = E> + HeaderBackend<Block>,
 {
 	fn get(&self) -> Result<AuthorityList, ClientError> {
 		// This implementation uses the Grandpa runtime API instead of reading directly from the
 		// `GRANDPA_AUTHORITIES_KEY` as the data may have been migrated since the genesis block of
 		// the chain, whereas the runtime API is backwards compatible.
 		self.executor()
 			.call(
-				&BlockId::Number(Zero::zero()),
+				self.expect_block_hash_from_id(&BlockId::Number(Zero::zero()))?,
 				"GrandpaApi_grandpa_authorities",
 				&[],
 				ExecutionStrategy::NativeElseWasm,

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

@@ -744,7 +744,7 @@ where
 			let res = client
 				.executor()
 				.call(
-					&BlockId::Hash(hash),
+					hash,
 					&function,
 					&call_parameters,
 					client.execution_extensions().strategies().other,

client/rpc/src/state/state_full.rs

@@ -203,7 +203,7 @@ where
 				self.client
 					.executor()
 					.call(
-						&BlockId::Hash(block),
+						block,
 						&method,
 						&call_data,
 						self.client.execution_extensions().strategies().other,

client/service/src/client/call_executor.rs

@@ -81,32 +81,32 @@ where
 	fn check_override<'a>(
 		&'a self,
 		onchain_code: RuntimeCode<'a>,
-		id: &BlockId<Block>,
+		hash: <Block as BlockT>::Hash,
 	) -> sp_blockchain::Result<(RuntimeCode<'a>, RuntimeVersion)>
 	where
 		Block: BlockT,
 		B: backend::Backend<Block>,
 	{
-		let on_chain_version = self.on_chain_runtime_version(id)?;
+		let on_chain_version = self.on_chain_runtime_version(hash)?;
 		let code_and_version = if let Some(d) = self.wasm_override.as_ref().as_ref().and_then(|o| {
 			o.get(
 				&on_chain_version.spec_version,
 				onchain_code.heap_pages,
 				&on_chain_version.spec_name,
 			)
 		}) {
-			log::debug!(target: "wasm_overrides", "using WASM override for block {}", id);
+			log::debug!(target: "wasm_overrides", "using WASM override for block {}", hash);
 			d
 		} else if let Some(s) =
 			self.wasm_substitutes
-				.get(on_chain_version.spec_version, onchain_code.heap_pages, id)
+				.get(on_chain_version.spec_version, onchain_code.heap_pages, hash)
 		{
-			log::debug!(target: "wasm_substitutes", "Using WASM substitute for block {:?}", id);
+			log::debug!(target: "wasm_substitutes", "Using WASM substitute for block {:?}", hash);
 			s
 		} else {
 			log::debug!(
 				target: "wasm_overrides",
-				"Neither WASM override nor substitute available for block {id}, using onchain code",
+				"Neither WASM override nor substitute available for block {hash}, using onchain code",
 			);
 			(onchain_code, on_chain_version)
 		};
@@ -115,14 +115,10 @@ where
 	}
 
 	/// Returns the on chain runtime version.
-	fn on_chain_runtime_version(
-		&self,
-		id: &BlockId<Block>,
-	) -> sp_blockchain::Result<RuntimeVersion> {
+	fn on_chain_runtime_version(&self, hash: Block::Hash) -> sp_blockchain::Result<RuntimeVersion> {
 		let mut overlay = OverlayedChanges::default();
 
-		let at_hash = self.backend.blockchain().expect_block_hash_from_id(id)?;
-		let state = self.backend.state_at(at_hash)?;
+		let state = self.backend.state_at(hash)?;
 		let mut cache = StorageTransactionCache::<Block, B::State>::default();
 		let mut ext = Ext::new(&mut overlay, &mut cache, &state, None);
 		let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state);
@@ -166,21 +162,21 @@ where
 
 	fn call(
 		&self,
-		at: &BlockId<Block>,
+		at_hash: Block::Hash,
 		method: &str,
 		call_data: &[u8],
 		strategy: ExecutionStrategy,
 	) -> sp_blockchain::Result<Vec<u8>> {
 		let mut changes = OverlayedChanges::default();
-		let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?;
-		let at_number = self.backend.blockchain().expect_block_number_from_id(at)?;
+		let at_number =
+			self.backend.blockchain().expect_block_number_from_id(&BlockId::Hash(at_hash))?;
 		let state = self.backend.state_at(at_hash)?;
 
 		let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state);
 		let runtime_code =
 			state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?;
 
-		let runtime_code = self.check_override(runtime_code, at)?.0;
+		let runtime_code = self.check_override(runtime_code, at_hash)?.0;
 
 		let extensions = self.execution_extensions.extensions(
 			at_hash,
@@ -206,7 +202,7 @@ where
 
 	fn contextual_call(
 		&self,
-		at: &BlockId<Block>,
+		at_hash: Block::Hash,
 		method: &str,
 		call_data: &[u8],
 		changes: &RefCell<OverlayedChanges>,
@@ -216,8 +212,8 @@ where
 	) -> Result<Vec<u8>, sp_blockchain::Error> {
 		let mut storage_transaction_cache = storage_transaction_cache.map(|c| c.borrow_mut());
 
-		let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?;
-		let at_number = self.backend.blockchain().expect_block_number_from_id(at)?;
+		let at_number =
+			self.backend.blockchain().expect_block_number_from_id(&BlockId::Hash(at_hash))?;
 		let state = self.backend.state_at(at_hash)?;
 
 		let (execution_manager, extensions) =
@@ -232,7 +228,7 @@ where
 
 		let runtime_code =
 			state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?;
-		let runtime_code = self.check_override(runtime_code, at)?.0;
+		let runtime_code = self.check_override(runtime_code, at_hash)?.0;
 
 		match recorder {
 			Some(recorder) => {
@@ -275,32 +271,31 @@ where
 		.map_err(Into::into)
 	}
 
-	fn runtime_version(&self, id: &BlockId<Block>) -> sp_blockchain::Result<RuntimeVersion> {
-		let at_hash = self.backend.blockchain().expect_block_hash_from_id(id)?;
+	fn runtime_version(&self, at_hash: Block::Hash) -> sp_blockchain::Result<RuntimeVersion> {
 		let state = self.backend.state_at(at_hash)?;
 		let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state);
 
 		let runtime_code =
 			state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?;
-		self.check_override(runtime_code, id).map(|(_, v)| v)
+		self.check_override(runtime_code, at_hash).map(|(_, v)| v)
 	}
 
 	fn prove_execution(
 		&self,
-		at: &BlockId<Block>,
+		at_hash: Block::Hash,
 		method: &str,
 		call_data: &[u8],
 	) -> sp_blockchain::Result<(Vec<u8>, StorageProof)> {
-		let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?;
-		let at_number = self.backend.blockchain().expect_block_number_from_id(at)?;
+		let at_number =
+			self.backend.blockchain().expect_block_number_from_id(&BlockId::Hash(at_hash))?;
 		let state = self.backend.state_at(at_hash)?;
 
 		let trie_backend = state.as_trie_backend();
 
 		let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(trie_backend);
 		let runtime_code =
 			state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?;
-		let runtime_code = self.check_override(runtime_code, at)?.0;
+		let runtime_code = self.check_override(runtime_code, at_hash)?.0;
 
 		sp_state_machine::prove_execution_on_trie_backend(
 			trie_backend,
@@ -340,7 +335,7 @@ where
 	E: CodeExecutor + RuntimeVersionOf + Clone + 'static,
 	Block: BlockT,
 {
-	fn runtime_version(&self, at: &BlockId<Block>) -> Result<sp_version::RuntimeVersion, String> {
+	fn runtime_version(&self, at: Block::Hash) -> Result<sp_version::RuntimeVersion, String> {
 		CallExecutor::runtime_version(self, at).map_err(|e| e.to_string())
 	}
 }
@@ -359,6 +354,7 @@ where
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use backend::Backend;
 	use sc_client_api::in_mem;
 	use sc_executor::{NativeElseWasmExecutor, WasmExecutionMethod};
 	use sp_core::{
@@ -432,7 +428,7 @@ mod tests {
 		};
 
 		let check = call_executor
-			.check_override(onchain_code, &BlockId::Number(Default::default()))
+			.check_override(onchain_code, backend.blockchain().info().genesis_hash)
 			.expect("RuntimeCode override")
 			.0;
 

client/service/src/client/client.rs

@@ -493,7 +493,8 @@ where
 
 	/// Get the RuntimeVersion at a given block.
 	pub fn runtime_version_at(&self, id: &BlockId<Block>) -> sp_blockchain::Result<RuntimeVersion> {
-		CallExecutor::runtime_version(&self.executor, id)
+		let hash = self.backend.blockchain().expect_block_hash_from_id(id)?;
+		CallExecutor::runtime_version(&self.executor, hash)
 	}
 
 	/// Apply a checked and validated block to an operation. If a justification is provided
@@ -1241,7 +1242,7 @@ where
 		method: &str,
 		call_data: &[u8],
 	) -> sp_blockchain::Result<(Vec<u8>, StorageProof)> {
-		self.executor.prove_execution(&BlockId::Hash(hash), method, call_data)
+		self.executor.prove_execution(hash, method, call_data)
 	}
 
 	fn read_proof_collection(
@@ -1726,9 +1727,10 @@ where
 		&self,
 		params: CallApiAtParams<Block, B::State>,
 	) -> Result<Vec<u8>, sp_api::ApiError> {
+		let at_hash = self.expect_block_hash_from_id(params.at)?;
 		self.executor
 			.contextual_call(
-				params.at,
+				at_hash,
 				params.function,
 				&params.arguments,
 				params.overlayed_changes,
@@ -1740,7 +1742,8 @@ where
 	}
 
 	fn runtime_version_at(&self, at: &BlockId<Block>) -> Result<RuntimeVersion, sp_api::ApiError> {
-		CallExecutor::runtime_version(&self.executor, at).map_err(Into::into)
+		let hash = self.backend.blockchain().expect_block_hash_from_id(at)?;
+		CallExecutor::runtime_version(&self.executor, hash).map_err(Into::into)
 	}
 
 	fn state_at(&self, at: &BlockId<Block>) -> Result<Self::StateBackend, sp_api::ApiError> {

client/service/src/client/wasm_substitutes.rs

@@ -22,10 +22,7 @@ use sc_client_api::backend;
 use sc_executor::RuntimeVersionOf;
 use sp_blockchain::{HeaderBackend, Result};
 use sp_core::traits::{FetchRuntimeCode, RuntimeCode, WrappedRuntimeCode};
-use sp_runtime::{
-	generic::BlockId,
-	traits::{Block as BlockT, NumberFor},
-};
+use sp_runtime::traits::{Block as BlockT, NumberFor};
 use sp_state_machine::BasicExternalities;
 use sp_version::RuntimeVersion;
 use std::{
@@ -54,10 +51,13 @@ impl<Block: BlockT> WasmSubstitute<Block> {
 		RuntimeCode { code_fetcher: self, hash: self.hash.clone(), heap_pages }
 	}
 
-	/// Returns `true` when the substitute matches for the given `block_id`.
-	fn matches(&self, block_id: &BlockId<Block>, backend: &impl backend::Backend<Block>) -> bool {
-		let requested_block_number =
-			backend.blockchain().block_number_from_id(block_id).ok().flatten();
+	/// Returns `true` when the substitute matches for the given `hash`.
+	fn matches(
+		&self,
+		hash: <Block as BlockT>::Hash,
+		backend: &impl backend::Backend<Block>,
+	) -> bool {
+		let requested_block_number = backend.blockchain().number(hash).ok().flatten();
 
 		Some(self.block_number) <= requested_block_number
 	}
@@ -147,10 +147,10 @@ where
 		&self,
 		spec: u32,
 		pages: Option<u64>,
-		block_id: &BlockId<Block>,
+		hash: Block::Hash,
 	) -> Option<(RuntimeCode<'_>, RuntimeVersion)> {
 		let s = self.substitutes.get(&spec)?;
-		s.matches(block_id, &*self.backend)
+		s.matches(hash, &*self.backend)
 			.then(|| (s.runtime_code(pages), s.version.clone()))
 	}
 

primitives/version/src/lib.rs

@@ -48,7 +48,7 @@ pub use sp_runtime::{create_runtime_str, StateVersion};
 pub use sp_std;
 
 #[cfg(feature = "std")]
-use sp_runtime::{generic::BlockId, traits::Block as BlockT};
+use sp_runtime::traits::Block as BlockT;
 
 #[cfg(feature = "std")]
 pub mod embed;
@@ -370,14 +370,14 @@ pub trait GetNativeVersion {
 #[cfg(feature = "std")]
 pub trait GetRuntimeVersionAt<Block: BlockT> {
 	/// Returns the version of runtime at the given block.
-	fn runtime_version(&self, at: &BlockId<Block>) -> Result<RuntimeVersion, String>;
+	fn runtime_version(&self, at: <Block as BlockT>::Hash) -> Result<RuntimeVersion, String>;
 }
 
 #[cfg(feature = "std")]
 impl<T: GetRuntimeVersionAt<Block>, Block: BlockT> GetRuntimeVersionAt<Block>
 	for std::sync::Arc<T>
 {
-	fn runtime_version(&self, at: &BlockId<Block>) -> Result<RuntimeVersion, String> {
+	fn runtime_version(&self, at: <Block as BlockT>::Hash) -> Result<RuntimeVersion, String> {
 		(&**self).runtime_version(at)
 	}
 }