Skip to content

Commit

Permalink
BlockId removal: runtime-api refactor (paritytech#13255)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>

* ".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 <[email protected]>
Co-authored-by: command-bot <>
  • Loading branch information
2 people authored and ukint-vs committed Apr 10, 2023
1 parent 9b608ec commit 6fe61c9
Show file tree
Hide file tree
Showing 52 changed files with 321 additions and 391 deletions.
2 changes: 1 addition & 1 deletion bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
4 changes: 1 addition & 3 deletions bin/node/testing/src/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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,
)
Expand Down
4 changes: 2 additions & 2 deletions client/authority-discovery/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion client/basic-authorship/src/basic_authorship.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
6 changes: 2 additions & 4 deletions client/beefy/src/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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 {
Expand All @@ -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()))?;

Expand Down
36 changes: 14 additions & 22 deletions client/beefy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()))?;
Expand All @@ -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);
Expand All @@ -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. \
Expand All @@ -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)?;
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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,
Expand All @@ -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()))
Expand Down
22 changes: 16 additions & 6 deletions client/beefy/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(())
Expand Down Expand Up @@ -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]);

Expand Down
24 changes: 12 additions & 12 deletions client/block-builder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
})
Expand All @@ -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(),
)
Expand All @@ -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(),
Expand Down Expand Up @@ -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,
))
Expand Down
12 changes: 4 additions & 8 deletions client/consensus/aura/src/import_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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,
Expand All @@ -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() {
Expand Down Expand Up @@ -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(),
Expand Down
11 changes: 6 additions & 5 deletions client/consensus/aura/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
Expand All @@ -627,7 +628,7 @@ where
}

runtime_api
.authorities(&BlockId::Hash(parent_hash))
.authorities(parent_hash)
.ok()
.ok_or(sp_consensus::Error::InvalidAuthoritiesSet)
}
Expand Down
Loading

0 comments on commit 6fe61c9

Please sign in to comment.