BlockId removal: runtime-api refactor (paritytech#13255)

* BlockId removal: refactor of runtime API

It changes the arguments of:
- `ApiExt` methods:  `has_api`, `has_api_with`, `api_version`
- `CallApiAt` method: `runtime_version_at`
from: `BlockId<Block>` to: `Block::Hash`

It also changes the first argument of all generated runtime API calls from: `BlockId<Block>` to: `Block::Hash`

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

* BlockId removal: refactor of runtime API - tests

- tests adjusted to new runtime API,
- some tests migrated from block number to block hash

* benchmarking-cli: BlockId(0) migrated to info().genesis_hash

`runtime_api.call()` now requires the block hash instead of BlockId::Number.
To access the genesis hash widely used in benchmarking engine the Client
was constrained to satisfy `sp_blockchain::HeaderBackend<Block>` trait
which provides `info().genesis_hash`.

* trivial: api.call(BlockId) -> api.call(Hash)

- Migrated all `runtime_api.calls` to use Hash
- Noteworthy (?):
-- `validate_transaction_blocking` in transaction pool,

* CallApiAtParams::at changed to Block::Hash

* missed doc updated

* Apply suggestions from code review

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

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

* BlockId removal: Benchmark::consumed_weight

Little refactor around `Benchmark::consumed_weight`: `BlockId` removed.

* at_hash renamed

* wrong merge fixed

* beefy worker: merged with master

* beefy: tests: missing block problem fixed

* Apply review suggestion

* fix

---------

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

bin/node/cli/src/service.rs

@@ -60,7 +60,7 @@ pub fn fetch_nonce(client: &FullClient, account: sp_core::sr25519::Pair) -> u32
 	let best_hash = client.chain_info().best_hash;
 	client
 		.runtime_api()
-		.account_nonce(&generic::BlockId::Hash(best_hash), account.public().into())
+		.account_nonce(best_hash, account.public().into())
 		.expect("Fetching account nonce works; qed")
 }
 

bin/node/testing/src/bench.rs

@@ -53,7 +53,6 @@ use sp_consensus::BlockOrigin;
 use sp_core::{blake2_256, ed25519, sr25519, traits::SpawnNamed, ExecutionContext, Pair, Public};
 use sp_inherents::InherentData;
 use sp_runtime::{
-	generic::BlockId,
 	traits::{Block as BlockT, IdentifyAccount, Verify},
 	OpaqueExtrinsic,
 };
@@ -274,7 +273,6 @@ pub struct BlockContentIterator<'a> {
 impl<'a> BlockContentIterator<'a> {
 	fn new(content: BlockContent, keyring: &'a BenchKeyring, client: &Client) -> Self {
 		let genesis_hash = client.chain_info().genesis_hash;
-
 		let runtime_version = client
 			.runtime_version_at(genesis_hash)
 			.expect("There should be runtime version at 0");
@@ -442,7 +440,7 @@ impl BenchDb {
 		client
 			.runtime_api()
 			.inherent_extrinsics_with_context(
-				&BlockId::number(0),
+				client.chain_info().genesis_hash,
 				ExecutionContext::BlockConstruction,
 				inherent_data,
 			)

client/authority-discovery/src/worker.rs

@@ -55,7 +55,7 @@ use sp_blockchain::HeaderBackend;
 
 use sp_core::crypto::{key_types, CryptoTypePublicPair, Pair};
 use sp_keystore::CryptoStore;
-use sp_runtime::{generic::BlockId, traits::Block as BlockT};
+use sp_runtime::traits::Block as BlockT;
 
 mod addr_cache;
 /// Dht payload schemas generated from Protobuf definitions via Prost crate in build.rs.
@@ -171,7 +171,7 @@ where
 		&self,
 		at: Block::Hash,
 	) -> std::result::Result<Vec<AuthorityId>, ApiError> {
-		self.runtime_api().authorities(&BlockId::Hash(at))
+		self.runtime_api().authorities(at)
 	}
 }
 

client/basic-authorship/src/basic_authorship.rs

@@ -731,7 +731,7 @@ mod tests {
 		assert_eq!(proposal.block.extrinsics().len(), 1);
 
 		let api = client.runtime_api();
-		api.execute_block(&BlockId::Hash(genesis_hash), proposal.block).unwrap();
+		api.execute_block(genesis_hash, proposal.block).unwrap();
 
 		let state = backend.state_at(genesis_hash).unwrap();
 

client/beefy/src/import.rs

@@ -24,7 +24,6 @@ use sp_api::{ProvideRuntimeApi, TransactionFor};
 use sp_blockchain::well_known_cache_keys;
 use sp_consensus::Error as ConsensusError;
 use sp_runtime::{
-	generic::BlockId,
 	traits::{Block as BlockT, Header as HeaderT, NumberFor},
 	EncodedJustification,
 };
@@ -93,11 +92,10 @@ where
 		hash: <Block as BlockT>::Hash,
 	) -> Result<BeefyVersionedFinalityProof<Block>, ConsensusError> {
 		use ConsensusError::ClientImport as ImportError;
-		let block_id = BlockId::hash(hash);
 		let beefy_genesis = self
 			.runtime
 			.runtime_api()
-			.beefy_genesis(&block_id)
+			.beefy_genesis(hash)
 			.map_err(|e| ImportError(e.to_string()))?
 			.ok_or_else(|| ImportError("Unknown BEEFY genesis".to_string()))?;
 		if number < beefy_genesis {
@@ -106,7 +104,7 @@ where
 		let validator_set = self
 			.runtime
 			.runtime_api()
-			.validator_set(&block_id)
+			.validator_set(hash)
 			.map_err(|e| ImportError(e.to_string()))?
 			.ok_or_else(|| ImportError("Unknown validator set".to_string()))?;
 

client/beefy/src/lib.rs

@@ -52,10 +52,7 @@ use sp_blockchain::{
 use sp_consensus::{Error as ConsensusError, SyncOracle};
 use sp_keystore::SyncCryptoStorePtr;
 use sp_mmr_primitives::MmrApi;
-use sp_runtime::{
-	generic::BlockId,
-	traits::{Block, Zero},
-};
+use sp_runtime::traits::{Block, Zero};
 use std::{collections::VecDeque, marker::PhantomData, sync::Arc};
 
 mod aux_schema;
@@ -345,7 +342,7 @@ where
 {
 	let beefy_genesis = runtime
 		.runtime_api()
-		.beefy_genesis(&BlockId::hash(best_grandpa.hash()))
+		.beefy_genesis(best_grandpa.hash())
 		.ok()
 		.flatten()
 		.ok_or_else(|| ClientError::Backend("BEEFY pallet expected to be active.".into()))?;
@@ -369,7 +366,7 @@ where
 			let best_beefy = *header.number();
 			// If no session boundaries detected so far, just initialize new rounds here.
 			if sessions.is_empty() {
-				let active_set = expect_validator_set(runtime, BlockId::hash(header.hash()))?;
+				let active_set = expect_validator_set(runtime, header.hash())?;
 				let mut rounds = Rounds::new(best_beefy, active_set);
 				// Mark the round as already finalized.
 				rounds.conclude(best_beefy);
@@ -383,8 +380,8 @@ where
 
 		if *header.number() == beefy_genesis {
 			// We've reached BEEFY genesis, initialize voter here.
-			let genesis_set = expect_validator_set(runtime, BlockId::hash(header.hash()))
-				.and_then(genesis_set_sanity_check)?;
+			let genesis_set =
+				expect_validator_set(runtime, header.hash()).and_then(genesis_set_sanity_check)?;
 			info!(
 				target: LOG_TARGET,
 				"🥩 Loading BEEFY voter state from genesis on what appears to be first startup. \
@@ -409,16 +406,11 @@ where
 
 		// Check if state is still available if we move up the chain.
 		let parent_hash = *header.parent_hash();
-		runtime
-			.runtime_api()
-			.validator_set(&BlockId::hash(parent_hash))
-			.ok()
-			.flatten()
-			.ok_or_else(|| {
-				let msg = format!("{}. Could not initialize BEEFY voter.", parent_hash);
-				error!(target: LOG_TARGET, "🥩 {}", msg);
-				ClientError::Consensus(sp_consensus::Error::StateUnavailable(msg))
-			})?;
+		runtime.runtime_api().validator_set(parent_hash).ok().flatten().ok_or_else(|| {
+			let msg = format!("{}. Could not initialize BEEFY voter.", parent_hash);
+			error!(target: LOG_TARGET, "🥩 {}", msg);
+			ClientError::Consensus(sp_consensus::Error::StateUnavailable(msg))
+		})?;
 
 		// Move up the chain.
 		header = blockchain.expect_header(parent_hash)?;
@@ -449,8 +441,8 @@ where
 					Some(notif) => notif,
 					None => break
 				};
-				let at = BlockId::hash(notif.header.hash());
-				if let Some(start) = runtime.runtime_api().beefy_genesis(&at).ok().flatten() {
+				let at = notif.header.hash();
+				if let Some(start) = runtime.runtime_api().beefy_genesis(at).ok().flatten() {
 					if *notif.header.number() >= start {
 						// Beefy pallet available, return header for best grandpa at the time.
 						info!(
@@ -485,7 +477,7 @@ fn genesis_set_sanity_check(
 
 fn expect_validator_set<B, R>(
 	runtime: &R,
-	at: BlockId<B>,
+	at_hash: B::Hash,
 ) -> ClientResult<ValidatorSet<AuthorityId>>
 where
 	B: Block,
@@ -494,7 +486,7 @@ where
 {
 	runtime
 		.runtime_api()
-		.validator_set(&at)
+		.validator_set(at_hash)
 		.ok()
 		.flatten()
 		.ok_or_else(|| ClientError::Backend("BEEFY pallet expected to be active.".into()))

client/beefy/src/worker.rs

@@ -963,10 +963,21 @@ where
 		}
 
 		let number = *proof.round_number();
+		let hash = self
+			.backend
+			.blockchain()
+			.expect_block_hash_from_id(&BlockId::Number(number))
+			.map_err(|err| {
+				let err_msg = format!(
+					"Couldn't get hash for block #{:?} (error: {:?}), skipping report for equivocation",
+					number, err
+				);
+				Error::Backend(err_msg)
+			})?;
 		let runtime_api = self.runtime.runtime_api();
 		// generate key ownership proof at that block
 		let key_owner_proof = match runtime_api
-			.generate_key_ownership_proof(&BlockId::Number(number), validator_set_id, offender_id)
+			.generate_key_ownership_proof(hash, validator_set_id, offender_id)
 			.map_err(Error::RuntimeApi)?
 		{
 			Some(proof) => proof,
@@ -982,11 +993,7 @@ where
 		// submit equivocation report at **best** block
 		let best_block_hash = self.backend.blockchain().info().best_hash;
 		runtime_api
-			.submit_report_equivocation_unsigned_extrinsic(
-				&BlockId::Hash(best_block_hash),
-				proof,
-				key_owner_proof,
-			)
+			.submit_report_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proof)
 			.map_err(Error::RuntimeApi)?;
 
 		Ok(())
@@ -1628,6 +1635,9 @@ pub(crate) mod tests {
 		let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1, validator_set.clone());
 		worker.runtime = api_alice.clone();
 
+		// let there be a block with num = 1:
+		let _ = net.peer(0).push_blocks(1, false);
+
 		let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]);
 		let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]);
 

client/block-builder/src/lib.rs

@@ -137,7 +137,6 @@ pub struct BlockBuilder<'a, Block: BlockT, A: ProvideRuntimeApi<Block>, B> {
 	extrinsics: Vec<Block::Extrinsic>,
 	api: ApiRef<'a, A::Api>,
 	version: u32,
-	block_id: BlockId<Block>,
 	parent_hash: Block::Hash,
 	backend: &'a B,
 	/// The estimated size of the block header.
@@ -181,20 +180,21 @@ where
 			api.record_proof();
 		}
 
-		let block_id = BlockId::Hash(parent_hash);
-
-		api.initialize_block_with_context(&block_id, ExecutionContext::BlockConstruction, &header)?;
+		api.initialize_block_with_context(
+			parent_hash,
+			ExecutionContext::BlockConstruction,
+			&header,
+		)?;
 
 		let version = api
-			.api_version::<dyn BlockBuilderApi<Block>>(&block_id)?
+			.api_version::<dyn BlockBuilderApi<Block>>(parent_hash)?
 			.ok_or_else(|| Error::VersionInvalid("BlockBuilderApi".to_string()))?;
 
 		Ok(Self {
 			parent_hash,
 			extrinsics: Vec::new(),
 			api,
 			version,
-			block_id,
 			backend,
 			estimated_header_size,
 		})
@@ -204,22 +204,22 @@ where
 	///
 	/// This will ensure the extrinsic can be validly executed (by executing it).
 	pub fn push(&mut self, xt: <Block as BlockT>::Extrinsic) -> Result<(), Error> {
-		let block_id = &self.block_id;
+		let parent_hash = self.parent_hash;
 		let extrinsics = &mut self.extrinsics;
 		let version = self.version;
 
 		self.api.execute_in_transaction(|api| {
 			let res = if version < 6 {
 				#[allow(deprecated)]
 				api.apply_extrinsic_before_version_6_with_context(
-					block_id,
+					parent_hash,
 					ExecutionContext::BlockConstruction,
 					xt.clone(),
 				)
 				.map(legacy::byte_sized_error::convert_to_latest)
 			} else {
 				api.apply_extrinsic_with_context(
-					block_id,
+					parent_hash,
 					ExecutionContext::BlockConstruction,
 					xt.clone(),
 				)
@@ -246,7 +246,7 @@ where
 	pub fn build(mut self) -> Result<BuiltBlock<Block, backend::StateBackendFor<B, Block>>, Error> {
 		let header = self
 			.api
-			.finalize_block_with_context(&self.block_id, ExecutionContext::BlockConstruction)?;
+			.finalize_block_with_context(self.parent_hash, ExecutionContext::BlockConstruction)?;
 
 		debug_assert_eq!(
 			header.extrinsics_root().clone(),
@@ -279,13 +279,13 @@ where
 		&mut self,
 		inherent_data: sp_inherents::InherentData,
 	) -> Result<Vec<Block::Extrinsic>, Error> {
-		let block_id = self.block_id;
+		let parent_hash = self.parent_hash;
 		self.api
 			.execute_in_transaction(move |api| {
 				// `create_inherents` should not change any state, to ensure this we always rollback
 				// the transaction.
 				TransactionOutcome::Rollback(api.inherent_extrinsics_with_context(
-					&block_id,
+					parent_hash,
 					ExecutionContext::BlockConstruction,
 					inherent_data,
 				))

client/consensus/aura/src/import_queue.rs

@@ -41,7 +41,6 @@ use sp_consensus_slots::Slot;
 use sp_core::{crypto::Pair, ExecutionContext};
 use sp_inherents::{CreateInherentDataProviders, InherentDataProvider as _};
 use sp_runtime::{
-	generic::BlockId,
 	traits::{Block as BlockT, Header, NumberFor},
 	DigestItem,
 };
@@ -142,7 +141,7 @@ where
 	async fn check_inherents<B: BlockT>(
 		&self,
 		block: B,
-		block_id: BlockId<B>,
+		at_hash: B::Hash,
 		inherent_data: sp_inherents::InherentData,
 		create_inherent_data_providers: CIDP::InherentDataProviders,
 		execution_context: ExecutionContext,
@@ -155,7 +154,7 @@ where
 		let inherent_res = self
 			.client
 			.runtime_api()
-			.check_inherents_with_context(&block_id, execution_context, block, inherent_data)
+			.check_inherents_with_context(at_hash, execution_context, block, inherent_data)
 			.map_err(|e| Error::Client(e.into()))?;
 
 		if !inherent_res.ok() {
@@ -248,15 +247,12 @@ where
 					if self
 						.client
 						.runtime_api()
-						.has_api_with::<dyn BlockBuilderApi<B>, _>(
-							&BlockId::Hash(parent_hash),
-							|v| v >= 2,
-						)
+						.has_api_with::<dyn BlockBuilderApi<B>, _>(parent_hash, |v| v >= 2)
 						.map_err(|e| e.to_string())?
 					{
 						self.check_inherents(
 							new_block.clone(),
-							BlockId::Hash(parent_hash),
+							parent_hash,
 							inherent_data,
 							create_inherent_data_providers,
 							block.origin.into(),

client/consensus/aura/src/lib.rs

@@ -53,7 +53,6 @@ use sp_core::crypto::{ByteArray, Pair, Public};
 use sp_inherents::CreateInherentDataProviders;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
 use sp_runtime::{
-	generic::BlockId,
 	traits::{Block as BlockT, Header, Member, NumberFor, Zero},
 	DigestItem,
 };
@@ -121,8 +120,10 @@ where
 	C: AuxStore + ProvideRuntimeApi<B> + UsageProvider<B>,
 	C::Api: AuraApi<B, A>,
 {
-	let best_block_id = BlockId::Hash(client.usage_info().chain.best_hash);
-	client.runtime_api().slot_duration(&best_block_id).map_err(|err| err.into())
+	client
+		.runtime_api()
+		.slot_duration(client.usage_info().chain.best_hash)
+		.map_err(|err| err.into())
 }
 
 /// Get slot author for given block along with authorities.
@@ -613,7 +614,7 @@ where
 			if *until > context_block_number {
 				runtime_api
 					.initialize_block(
-						&BlockId::Hash(parent_hash),
+						parent_hash,
 						&B::Header::new(
 							context_block_number,
 							Default::default(),
@@ -627,7 +628,7 @@ where
 	}
 
 	runtime_api
-		.authorities(&BlockId::Hash(parent_hash))
+		.authorities(parent_hash)
 		.ok()
 		.ok_or(sp_consensus::Error::InvalidAuthoritiesSet)
 }