Skip to content

Commit

Permalink
Block import and verification refactoring (paritytech#4844)
Browse files Browse the repository at this point in the history
A few refactorings to block import and block verification that should
not be controversial.

Block verification before block import is stateless by design as
described in https://substrate.stackexchange.com/a/1322/25 and the fact
that it wasn't yet I consider to be a bug. Some code that requires it
had to use `Mutex`, but I do not expect it to have a measurable
performance impact.

Similarly with block import checking whether block preconditions should
not be an exclusive operation, there is nothing fundamentally wrong with
checking a few competing blocks whose parent blocks exist at the same
time (and even import them concurrently later, though IIRC this is not
yet implemented either).

They were originally a part of
paritytech#4842 and upstreaming
will help us to reduce the size of the patch we need to apply on top of
upstream code at Subspace every time we upgrade. There are no new
features introduced here, just refactoring to get rid of unnecessary
requirements.
  • Loading branch information
nazar-pc authored and TarekkMA committed Aug 2, 2024

Verified

This commit was signed with the committer’s verified signature.
abelsromero Abel Salgado Romero
1 parent 0bf8e0b commit 428c637
Showing 22 changed files with 244 additions and 168 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cumulus/client/consensus/aura/Cargo.toml
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ workspace = true
async-trait = { workspace = true }
codec = { features = ["derive"], workspace = true, default-features = true }
futures = { workspace = true }
parking_lot = { workspace = true }
tracing = { workspace = true, default-features = true }
schnellru = { workspace = true }

Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@
/// should be thrown out and which ones should be kept.
use codec::Codec;
use cumulus_client_consensus_common::ParachainBlockImportMarker;
use parking_lot::Mutex;
use schnellru::{ByLength, LruMap};

use sc_consensus::{
@@ -70,7 +71,7 @@ impl NaiveEquivocationDefender {
struct Verifier<P, Client, Block, CIDP> {
client: Arc<Client>,
create_inherent_data_providers: CIDP,
defender: NaiveEquivocationDefender,
defender: Mutex<NaiveEquivocationDefender>,
telemetry: Option<TelemetryHandle>,
_phantom: std::marker::PhantomData<fn() -> (Block, P)>,
}
@@ -88,7 +89,7 @@ where
CIDP: CreateInherentDataProviders<Block, ()>,
{
async fn verify(
&mut self,
&self,
mut block_params: BlockImportParams<Block>,
) -> Result<BlockImportParams<Block>, String> {
// Skip checks that include execution, if being told so, or when importing only state.
@@ -137,7 +138,7 @@ where
block_params.post_hash = Some(post_hash);

// Check for and reject egregious amounts of equivocations.
if self.defender.insert_and_check(slot) {
if self.defender.lock().insert_and_check(slot) {
return Err(format!(
"Rejecting block {:?} due to excessive equivocations at slot",
post_hash,
@@ -243,7 +244,7 @@ where
let verifier = Verifier::<P, _, _, _> {
client,
create_inherent_data_providers,
defender: NaiveEquivocationDefender::default(),
defender: Mutex::new(NaiveEquivocationDefender::default()),
telemetry,
_phantom: std::marker::PhantomData,
};
2 changes: 1 addition & 1 deletion cumulus/client/consensus/common/src/import_queue.rs
Original file line number Diff line number Diff line change
@@ -50,7 +50,7 @@ pub struct VerifyNothing;
#[async_trait::async_trait]
impl<Block: BlockT> Verifier<Block> for VerifyNothing {
async fn verify(
&mut self,
&self,
params: BlockImportParams<Block>,
) -> Result<BlockImportParams<Block>, String> {
Ok(params)
4 changes: 2 additions & 2 deletions cumulus/client/consensus/common/src/lib.rs
Original file line number Diff line number Diff line change
@@ -172,13 +172,13 @@ impl<Block: BlockT, I: Clone, BE> Clone for ParachainBlockImport<Block, I, BE> {
impl<Block, BI, BE> BlockImport<Block> for ParachainBlockImport<Block, BI, BE>
where
Block: BlockT,
BI: BlockImport<Block> + Send,
BI: BlockImport<Block> + Send + Sync,
BE: Backend<Block>,
{
type Error = BI::Error;

async fn check_block(
&mut self,
&self,
block: sc_consensus::BlockCheckParams<Block>,
) -> Result<sc_consensus::ImportResult, Self::Error> {
self.inner.check_block(block).await
2 changes: 1 addition & 1 deletion cumulus/client/consensus/relay-chain/src/import_queue.rs
Original file line number Diff line number Diff line change
@@ -52,7 +52,7 @@ where
CIDP: CreateInherentDataProviders<Block, ()>,
{
async fn verify(
&mut self,
&self,
mut block_params: BlockImportParams<Block>,
) -> Result<BlockImportParams<Block>, String> {
block_params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::Custom(
91 changes: 36 additions & 55 deletions cumulus/polkadot-parachain/src/service.rs
Original file line number Diff line number Diff line change
@@ -498,43 +498,26 @@ pub async fn start_shell_node<Net: NetworkBackend<Block, Hash>>(
.await
}

enum BuildOnAccess<R> {
Uninitialized(Option<Box<dyn FnOnce() -> R + Send + Sync>>),
Initialized(R),
}

impl<R> BuildOnAccess<R> {
fn get_mut(&mut self) -> &mut R {
loop {
match self {
Self::Uninitialized(f) => {
*self = Self::Initialized((f.take().unwrap())());
},
Self::Initialized(ref mut r) => return r,
}
}
}
}

struct Verifier<Client, AuraId> {
client: Arc<Client>,
aura_verifier: BuildOnAccess<Box<dyn VerifierT<Block>>>,
aura_verifier: Box<dyn VerifierT<Block>>,
relay_chain_verifier: Box<dyn VerifierT<Block>>,
_phantom: PhantomData<AuraId>,
}

#[async_trait::async_trait]
impl<Client, AuraId: AuraIdT> VerifierT<Block> for Verifier<Client, AuraId>
impl<Client, AuraId> VerifierT<Block> for Verifier<Client, AuraId>
where
Client: sp_api::ProvideRuntimeApi<Block> + Send + Sync,
Client: ProvideRuntimeApi<Block> + Send + Sync,
Client::Api: AuraRuntimeApi<Block, AuraId>,
AuraId: AuraIdT + Sync,
{
async fn verify(
&mut self,
&self,
block_import: BlockImportParams<Block>,
) -> Result<BlockImportParams<Block>, String> {
if self.client.runtime_api().has_aura_api(*block_import.header.parent_hash()) {
self.aura_verifier.get_mut().verify(block_import).await
self.aura_verifier.verify(block_import).await
} else {
self.relay_chain_verifier.verify(block_import).await
}
@@ -543,7 +526,7 @@ where

/// Build the import queue for parachain runtimes that started with relay chain consensus and
/// switched to aura.
pub fn build_relay_to_aura_import_queue<RuntimeApi, AuraId: AuraIdT>(
pub fn build_relay_to_aura_import_queue<RuntimeApi, AuraId>(
client: Arc<ParachainClient<RuntimeApi>>,
block_import: ParachainBlockImport<RuntimeApi>,
config: &Configuration,
@@ -553,46 +536,43 @@ pub fn build_relay_to_aura_import_queue<RuntimeApi, AuraId: AuraIdT>(
where
RuntimeApi: ConstructNodeRuntimeApi<Block, ParachainClient<RuntimeApi>>,
RuntimeApi::RuntimeApi: AuraRuntimeApi<Block, AuraId>,
AuraId: AuraIdT + Sync,
{
let verifier_client = client.clone();

let aura_verifier = move || {
Box::new(cumulus_client_consensus_aura::build_verifier::<
<AuraId as AppCrypto>::Pair,
_,
_,
_,
>(cumulus_client_consensus_aura::BuildVerifierParams {
client: verifier_client.clone(),
create_inherent_data_providers: move |parent_hash, _| {
let cidp_client = verifier_client.clone();
async move {
let slot_duration = cumulus_client_consensus_aura::slot_duration_at(
&*cidp_client,
parent_hash,
)?;
let timestamp = sp_timestamp::InherentDataProvider::from_system_time();

let slot =
sp_consensus_aura::inherents::InherentDataProvider::from_timestamp_and_slot_duration(
*timestamp,
slot_duration,
);

Ok((slot, timestamp))
}
},
telemetry: telemetry_handle,
})) as Box<_>
};
let aura_verifier = cumulus_client_consensus_aura::build_verifier::<
<AuraId as AppCrypto>::Pair,
_,
_,
_,
>(cumulus_client_consensus_aura::BuildVerifierParams {
client: verifier_client.clone(),
create_inherent_data_providers: move |parent_hash, _| {
let cidp_client = verifier_client.clone();
async move {
let slot_duration =
cumulus_client_consensus_aura::slot_duration_at(&*cidp_client, parent_hash)?;
let timestamp = sp_timestamp::InherentDataProvider::from_system_time();

let slot =
sp_consensus_aura::inherents::InherentDataProvider::from_timestamp_and_slot_duration(
*timestamp,
slot_duration,
);

Ok((slot, timestamp))
}
},
telemetry: telemetry_handle,
});

let relay_chain_verifier =
Box::new(RelayChainVerifier::new(client.clone(), |_, _| async { Ok(()) })) as Box<_>;

let verifier = Verifier {
client,
relay_chain_verifier,
aura_verifier: BuildOnAccess::Uninitialized(Some(Box::new(aura_verifier))),
aura_verifier: Box::new(aura_verifier),
_phantom: PhantomData,
};

@@ -632,7 +612,7 @@ pub async fn start_generic_aura_lookahead_node<Net: NetworkBackend<Block, Hash>>
///
/// Uses the lookahead collator to support async backing.
#[sc_tracing::logging::prefix_logs_with("Parachain")]
pub async fn start_asset_hub_lookahead_node<RuntimeApi, AuraId: AuraIdT, Net>(
pub async fn start_asset_hub_lookahead_node<RuntimeApi, AuraId, Net>(
parachain_config: Configuration,
polkadot_config: Configuration,
collator_options: CollatorOptions,
@@ -644,6 +624,7 @@ where
RuntimeApi::RuntimeApi: AuraRuntimeApi<Block, AuraId>
+ pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>
+ substrate_frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
AuraId: AuraIdT + Sync,
Net: NetworkBackend<Block, Hash>,
{
start_node_impl::<RuntimeApi, _, _, _, Net>(
34 changes: 34 additions & 0 deletions prdoc/pr_4844.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
title: Make `Verifier::verify` and `BlockImport::check_block` use `&self` instead of `&mut self`

doc:
- audience: Node Dev
description: |
`Verifier::verify` and `BlockImport::check_block` were refactored to use `&self` instead of `&mut self`
because there is no fundamental requirement for those operations to be exclusive in nature.

crates:
- name: sc-consensus
bump: major
validate: false
- name: sc-consensus-aura
bump: major
- name: sc-consensus-babe
bump: major
- name: sc-consensus-beefy
bump: major
- name: sc-consensus-grandpa
bump: major
- name: sc-consensus-manual-seal
bump: major
- name: sc-consensus-pow
bump: major
- name: sc-service
bump: major
- name: cumulus-client-consensus-common
bump: major
- name: cumulus-client-consensus-aura
bump: major
- name: cumulus-client-consensus-relay-chain
bump: major
- name: polkadot-parachain-bin
validate: false
2 changes: 1 addition & 1 deletion substrate/client/consensus/aura/src/import_queue.rs
Original file line number Diff line number Diff line change
@@ -174,7 +174,7 @@ where
CIDP::InherentDataProviders: InherentDataProviderExt + Send + Sync,
{
async fn verify(
&mut self,
&self,
mut block: BlockImportParams<B>,
) -> Result<BlockImportParams<B>, String> {
// Skip checks that include execution, if being told so or when importing only state.
4 changes: 2 additions & 2 deletions substrate/client/consensus/babe/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1128,7 +1128,7 @@ where
CIDP::InherentDataProviders: InherentDataProviderExt + Send + Sync,
{
async fn verify(
&mut self,
&self,
mut block: BlockImportParams<Block>,
) -> Result<BlockImportParams<Block>, String> {
trace!(
@@ -1681,7 +1681,7 @@ where
}

async fn check_block(
&mut self,
&self,
block: BlockCheckParams<Block>,
) -> Result<ImportResult, Self::Error> {
self.inner.check_block(block).await.map_err(Into::into)
10 changes: 5 additions & 5 deletions substrate/client/consensus/babe/src/tests.rs
Original file line number Diff line number Diff line change
@@ -143,11 +143,11 @@ thread_local! {
pub struct PanickingBlockImport<B>(B);

#[async_trait::async_trait]
impl<B: BlockImport<TestBlock>> BlockImport<TestBlock> for PanickingBlockImport<B>
impl<BI> BlockImport<TestBlock> for PanickingBlockImport<BI>
where
B: Send,
BI: BlockImport<TestBlock> + Send + Sync,
{
type Error = B::Error;
type Error = BI::Error;

async fn import_block(
&mut self,
@@ -157,7 +157,7 @@ where
}

async fn check_block(
&mut self,
&self,
block: BlockCheckParams<TestBlock>,
) -> Result<ImportResult, Self::Error> {
Ok(self.0.check_block(block).await.expect("checking block failed"))
@@ -198,7 +198,7 @@ impl Verifier<TestBlock> for TestVerifier {
/// new set of validators to import. If not, err with an Error-Message
/// presented to the User in the logs.
async fn verify(
&mut self,
&self,
mut block: BlockImportParams<TestBlock>,
) -> Result<BlockImportParams<TestBlock>, String> {
// apply post-sealing mutations (i.e. stripping seal, if desired).
2 changes: 1 addition & 1 deletion substrate/client/consensus/beefy/src/import.rs
Original file line number Diff line number Diff line change
@@ -192,7 +192,7 @@ where
}

async fn check_block(
&mut self,
&self,
block: BlockCheckParams<Block>,
) -> Result<ImportResult, Self::Error> {
self.inner.check_block(block).await
15 changes: 3 additions & 12 deletions substrate/client/consensus/common/src/block_import.rs
Original file line number Diff line number Diff line change
@@ -307,10 +307,7 @@ pub trait BlockImport<B: BlockT> {
type Error: std::error::Error + Send + 'static;

/// Check block preconditions.
async fn check_block(
&mut self,
block: BlockCheckParams<B>,
) -> Result<ImportResult, Self::Error>;
async fn check_block(&self, block: BlockCheckParams<B>) -> Result<ImportResult, Self::Error>;

/// Import a block.
async fn import_block(
@@ -324,10 +321,7 @@ impl<B: BlockT> BlockImport<B> for crate::import_queue::BoxBlockImport<B> {
type Error = sp_consensus::error::Error;

/// Check block preconditions.
async fn check_block(
&mut self,
block: BlockCheckParams<B>,
) -> Result<ImportResult, Self::Error> {
async fn check_block(&self, block: BlockCheckParams<B>) -> Result<ImportResult, Self::Error> {
(**self).check_block(block).await
}

@@ -348,10 +342,7 @@ where
{
type Error = E;

async fn check_block(
&mut self,
block: BlockCheckParams<B>,
) -> Result<ImportResult, Self::Error> {
async fn check_block(&self, block: BlockCheckParams<B>) -> Result<ImportResult, Self::Error> {
(&**self).check_block(block).await
}

Loading

0 comments on commit 428c637

Please sign in to comment.