Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

aura-ext: check slot in consensus hook and remove all CheckInherents logic #2658

Merged
merged 9 commits into from
Jul 3, 2023
Merged

aura-ext: check slot in consensus hook and remove all CheckInherents logic #2658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9751951
aura-ext: check slot in consensus hook
slumber May 30, 2023
3bf8776
convert relay chain slot
slumber May 31, 2023
10b9a81
Make relay chain slot duration generic
slumber Jun 1, 2023
68eb7e1
use fixed velocity hook for pallets with aura
slumber Jun 1, 2023
1dcf3f0
purge timestamp inherent
slumber Jun 1, 2023
bc8e7bb
fix warning
slumber Jun 2, 2023
2a10bdf
adjust runtime tests
slumber Jun 5, 2023
76065bd
fix slots in tests
slumber Jun 6, 2023
f1cbacd
Make `xcm-emulator` test pass for new consensus hook (#2722)
NachoPal Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 2 additions & 29 deletions Cargo.lock
View file
Open in desktop

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

1 change: 0 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ members = [
"parachain-template/runtime",
"primitives/core",
"primitives/parachain-inherent",
"primitives/timestamp",
"primitives/utility",
"polkadot-parachain",
"parachains/common",
Expand Down
1 change: 1 addition & 0 deletions pallets/aura-ext/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ scale-info = { version = "2.7.0", default-features = false, features = ["derive"
frame-support = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
frame-system = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
pallet-aura = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
pallet-timestamp= { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-application-crypto = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-consensus-aura = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
Expand Down
37 changes: 30 additions & 7 deletions pallets/aura-ext/src/consensus_hook.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,51 @@
//!
//! The velocity `V` refers to the rate of block processing by the relay chain.

use super::pallet;
use super::{pallet, Aura};
use cumulus_pallet_parachain_system::{
consensus_hook::{ConsensusHook, UnincludedSegmentCapacity},
relay_state_snapshot::RelayChainStateProof,
};
use frame_support::pallet_prelude::*;
use sp_consensus_aura::{Slot, SlotDuration};
use sp_std::{marker::PhantomData, num::NonZeroU32};

const MILLIS_PER_SECOND: u64 = 1000;

/// A consensus hook for a fixed block processing velocity and unincluded segment capacity.
pub struct FixedVelocityConsensusHook<T, const V: u32, const C: u32>(PhantomData<T>);
///
/// Relay chain slot duration must be provided in seconds.
pub struct FixedVelocityConsensusHook<
T,
const RELAY_CHAIN_SLOT_DURATION: u32,
const V: u32,
const C: u32,
>(PhantomData<T>);

impl<T: pallet::Config, const V: u32, const C: u32> ConsensusHook
for FixedVelocityConsensusHook<T, V, C>
impl<T: pallet::Config, const RELAY_CHAIN_SLOT_DURATION: u32, const V: u32, const C: u32>
ConsensusHook for FixedVelocityConsensusHook<T, RELAY_CHAIN_SLOT_DURATION, V, C>
where
<T as pallet_timestamp::Config>::Moment: Into<u64>,
Comment on lines +45 to +46
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aura::slot_duration returns T::Moment with a definition

type Moment: Parameter
			+ Default
			+ AtLeast32Bit // From<u16> + From<u32>
			+ Scale<Self::BlockNumber, Output = Self::Moment>
			+ Copy
			+ MaxEncodedLen
			+ scale_info::StaticTypeInfo;

And SlotDuration cannot be built out of it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are workarounds - what have you tried?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't come up with any other way. Ideally, I would expect Aura::slot_duration to return SlotDuration.
As you can see from the type definition, it's not convertible to any numeric primitive, so we have to enforce it in boundary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's reasonable to enforce it in the boundary as long as that aligns with the instantiations that are typically used in runtime declarations. Seems like a good solution to me.

{
// Validates the number of authored blocks within the slot with respect to the `V + 1` limit.
fn on_state_proof(_state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) {
fn on_state_proof(state_proof: &RelayChainStateProof) -> (Weight, UnincludedSegmentCapacity) {
// Ensure velocity is non-zero.
let velocity = V.max(1);
let relay_chain_slot = state_proof.read_slot().expect("failed to read relay chain slot");
Copy link
Contributor

@BradleyOlson64 BradleyOlson64 Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here if the collator isn't up to date with the latest relay block? I think a "slot number mismatch" would occur.

Right now it seems that collation is still triggered by new incoming relay blocks, so the scenario I described won't occur. But my understanding is that we will want to unteather these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the state proof comes from the relay parent the para is building on top of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right - the parachain has no information about what the latest relay-parent is, other than what the collator gives it. But validators will only accept blocks with recent relay-parents.


let authored = pallet::Pallet::<T>::slot_info()
.map(|(_slot, authored)| authored)
let (slot, authored) = pallet::Pallet::<T>::slot_info()
.expect("slot info is inserted on block initialization");
Comment on lines +54 to 55
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rphmeier
Now that integration tests are failing due to this panic, I wonder if it's OK to optionally verify the slot and skip it if the value is not present?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that it's most likely the case that blocks built in tests don't contain the pre-runtime digest which sets the slot in pallet_aura and that they need to.


// Convert relay chain timestamp.
let relay_chain_timestamp = u64::from(RELAY_CHAIN_SLOT_DURATION)
.saturating_mul(*relay_chain_slot)
.saturating_mul(MILLIS_PER_SECOND);
let para_slot_duration = SlotDuration::from_millis(Aura::<T>::slot_duration().into());
let para_slot_from_relay =
Slot::from_timestamp(relay_chain_timestamp.into(), para_slot_duration);

// Perform checks.
assert_eq!(slot, para_slot_from_relay, "slot number mismatch");
if authored > velocity + 1 {
panic!("authored blocks limit is reached for the slot")
}
Expand Down
2 changes: 0 additions & 2 deletions pallets/aura-ext/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@
//! ```
//!# struct Runtime;
//!# struct Executive;
//!# struct CheckInherents;
//! cumulus_pallet_parachain_system::register_validate_block! {
//! Runtime = Runtime,
//! BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::<Runtime, Executive>,
//! CheckInherents = CheckInherents,
//! }
//! ```

Expand Down
11 changes: 2 additions & 9 deletions pallets/parachain-system/proc-macro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,17 @@ use syn::{
mod keywords {
syn::custom_keyword!(Runtime);
syn::custom_keyword!(BlockExecutor);
syn::custom_keyword!(CheckInherents);
}

struct Input {
runtime: Path,
block_executor: Path,
check_inherents: Path,
}

impl Parse for Input {
fn parse(input: ParseStream) -> Result<Self, Error> {
let mut runtime = None;
let mut block_executor = None;
let mut check_inherents = None;

fn parse_inner<KW: Parse + Spanned>(
input: ParseStream,
Expand All @@ -59,15 +56,13 @@ impl Parse for Input {
}
}

while runtime.is_none() || block_executor.is_none() || check_inherents.is_none() {
while runtime.is_none() || block_executor.is_none() {
let lookahead = input.lookahead1();

if lookahead.peek(keywords::Runtime) {
parse_inner::<keywords::Runtime>(input, &mut runtime)?;
} else if lookahead.peek(keywords::BlockExecutor) {
parse_inner::<keywords::BlockExecutor>(input, &mut block_executor)?;
} else if lookahead.peek(keywords::CheckInherents) {
parse_inner::<keywords::CheckInherents>(input, &mut check_inherents)?;
} else {
return Err(lookahead.error())
}
Expand All @@ -81,7 +76,6 @@ impl Parse for Input {
Ok(Self {
runtime: runtime.expect("Everything is parsed before; qed"),
block_executor: block_executor.expect("Everything is parsed before; qed"),
check_inherents: check_inherents.expect("Everything is parsed before; qed"),
})
}
}
Expand All @@ -97,7 +91,7 @@ fn crate_() -> Result<Ident, Error> {

#[proc_macro]
pub fn register_validate_block(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
let Input { runtime, check_inherents, block_executor } = match syn::parse(input) {
let Input { runtime, block_executor } = match syn::parse(input) {
Ok(t) => t,
Err(e) => return e.into_compile_error().into(),
};
Expand Down Expand Up @@ -133,7 +127,6 @@ pub fn register_validate_block(input: proc_macro::TokenStream) -> proc_macro::To
<#runtime as #crate_::validate_block::GetRuntimeBlockType>::RuntimeBlock,
#block_executor,
#runtime,
#check_inherents,
>(params);

#crate_::validate_block::polkadot_parachain::write_result(&res)
Expand Down
16 changes: 1 addition & 15 deletions pallets/parachain-system/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use frame_system::{ensure_none, ensure_root};
use polkadot_parachain::primitives::RelayChainBlockNumber;
use scale_info::TypeInfo;
use sp_runtime::{
traits::{Block as BlockT, BlockNumberProvider, Hash},
traits::{BlockNumberProvider, Hash},
transaction_validity::{
InvalidTransaction, TransactionLongevity, TransactionSource, TransactionValidity,
ValidTransaction,
Expand Down Expand Up @@ -85,12 +85,10 @@ pub use consensus_hook::ConsensusHook;
/// ```
/// struct BlockExecutor;
/// struct Runtime;
/// struct CheckInherents;
///
/// cumulus_pallet_parachain_system::register_validate_block! {
/// Runtime = Runtime,
/// BlockExecutor = Executive,
/// CheckInherents = CheckInherents,
/// }
///
/// # fn main() {}
Expand Down Expand Up @@ -1387,18 +1385,6 @@ impl<T: Config> UpwardMessageSender for Pallet<T> {
}
}

/// Something that can check the inherents of a block.
pub trait CheckInherents<Block: BlockT> {
/// Check all inherents of the block.
///
/// This function gets passed all the extrinsics of the block, so it is up to the callee to
/// identify the inherents. The `validation_data` can be used to access the
fn check_inherents(
block: &Block,
validation_data: &RelayChainStateProof,
) -> frame_support::inherent::CheckInherentsResult;
}

/// Something that should be informed about system related events.
///
/// This includes events like [`on_validation_data`](Self::on_validation_data) that is being
Expand Down
33 changes: 2 additions & 31 deletions pallets/parachain-system/src/validate_block/implementation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,12 @@ fn with_externalities<F: FnOnce(&mut dyn Externalities) -> R, R>(f: F) -> R {
/// we have the in-memory database that contains all the values from the state of the parachain
/// that we require to verify the block.
///
/// 5. We are going to run `check_inherents`. This is important to check stuff like the timestamp
/// matching the real world time.
///
/// 6. The last step is to execute the entire block in the machinery we just have setup. Executing
/// 5. The last step is to execute the entire block in the machinery we just have setup. Executing
/// the blocks include running all transactions in the block against our in-memory database and
/// ensuring that the final storage root matches the storage root in the header of the block. In the
/// end we return back the [`ValidationResult`] with all the required information for the validator.
#[doc(hidden)]
pub fn validate_block<
B: BlockT,
E: ExecuteBlock<B>,
PSC: crate::Config,
CI: crate::CheckInherents<B>,
>(
pub fn validate_block<B: BlockT, E: ExecuteBlock<B>, PSC: crate::Config>(
MemoryOptimizedValidationParams {
block_data,
parent_head,
Expand Down Expand Up @@ -158,27 +150,6 @@ where
sp_io::offchain_index::host_clear.replace_implementation(host_offchain_index_clear),
);

run_with_externalities::<B, _, _>(&backend, || {
let relay_chain_proof = crate::RelayChainStateProof::new(
PSC::SelfParaId::get(),
inherent_data.validation_data.relay_parent_storage_root,
inherent_data.relay_chain_state.clone(),
)
.expect("Invalid relay chain state proof");

let res = CI::check_inherents(&block, &relay_chain_proof);

if !res.ok() {
if log::log_enabled!(log::Level::Error) {
res.into_errors().for_each(|e| {
log::error!("Checking inherent with identifier `{:?}` failed", e.0)
});
}

panic!("Checking inherents failed");
}
});

run_with_externalities::<B, _, _>(&backend, || {
let head_data = HeadData(block.header().encode());

Expand Down
Loading