Skip to content

Commit

Permalink
Block import and verification refactoring (#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
#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 Jun 26, 2024
1 parent 2f3a1bf commit 0ed3f04
Show file tree
Hide file tree
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
Expand Up @@ -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 }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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)>,
}
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
};
Expand Down
2 changes: 1 addition & 1 deletion cumulus/client/consensus/common/src/import_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions cumulus/client/consensus/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
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
Expand Up @@ -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(
Expand Down
91 changes: 36 additions & 55 deletions cumulus/polkadot-parachain/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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,
Expand All @@ -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,
};

Expand Down Expand Up @@ -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,
Expand All @@ -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>(
Expand Down
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
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions substrate/client/consensus/babe/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions substrate/client/consensus/babe/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"))
Expand Down Expand Up @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion substrate/client/consensus/beefy/src/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 3 additions & 12 deletions substrate/client/consensus/common/src/block_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
}

Expand All @@ -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
}

Expand Down
Loading

0 comments on commit 0ed3f04

Please sign in to comment.