diff --git a/cumulus/pallets/aura-ext/src/lib.rs b/cumulus/pallets/aura-ext/src/lib.rs index 496cbb694a87f..2da7e8af01203 100644 --- a/cumulus/pallets/aura-ext/src/lib.rs +++ b/cumulus/pallets/aura-ext/src/lib.rs @@ -25,7 +25,6 @@ //! ``` //! # struct Runtime; //! # struct Executive; -//! # struct CheckInherents; //! cumulus_pallet_parachain_system::register_validate_block! { //! Runtime = Runtime, //! BlockExecutor = cumulus_pallet_aura_ext::BlockExecutor::, diff --git a/cumulus/pallets/parachain-system/proc-macro/src/lib.rs b/cumulus/pallets/parachain-system/proc-macro/src/lib.rs index d3693c970b904..3a240b2134338 100644 --- a/cumulus/pallets/parachain-system/proc-macro/src/lib.rs +++ b/cumulus/pallets/parachain-system/proc-macro/src/lib.rs @@ -31,14 +31,12 @@ mod keywords { struct Input { runtime: Path, block_executor: Path, - check_inherents: Option, } impl Parse for Input { fn parse(input: ParseStream) -> Result { let mut runtime = None; let mut block_executor = None; - let mut check_inherents = None; fn parse_inner( input: ParseStream, @@ -67,7 +65,7 @@ impl Parse for Input { } else if lookahead.peek(keywords::BlockExecutor) { parse_inner::(input, &mut block_executor)?; } else if lookahead.peek(keywords::CheckInherents) { - parse_inner::(input, &mut check_inherents)?; + return Err(Error::new(input.span(), "`CheckInherents` is not supported anymore!")); } else { return Err(lookahead.error()) } @@ -76,7 +74,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, }) } } @@ -92,7 +89,7 @@ fn crate_() -> Result { #[proc_macro] pub fn register_validate_block(input: proc_macro::TokenStream) -> proc_macro::TokenStream { - let Input { runtime, block_executor, check_inherents } = match syn::parse(input) { + let Input { runtime, block_executor } = match syn::parse(input) { Ok(t) => t, Err(e) => return e.into_compile_error().into(), }; @@ -102,17 +99,6 @@ pub fn register_validate_block(input: proc_macro::TokenStream) -> proc_macro::To Err(e) => return e.into_compile_error().into(), }; - let check_inherents = match check_inherents { - Some(_check_inherents) => { - quote::quote! { #_check_inherents } - }, - None => { - quote::quote! { - #crate_::DummyCheckInherents<<#runtime as #crate_::validate_block::GetRuntimeBlockType>::RuntimeBlock> - } - }, - }; - if cfg!(not(feature = "std")) { quote::quote! { #[doc(hidden)] @@ -139,7 +125,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_primitives::write_result(&res) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 589c97499acc9..4425fdae54f48 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -55,7 +55,7 @@ use polkadot_parachain_primitives::primitives::RelayChainBlockNumber; use polkadot_runtime_parachains::{FeeTracker, GetMinFeeFactor}; use scale_info::TypeInfo; use sp_runtime::{ - traits::{Block as BlockT, BlockNumberProvider, Hash}, + traits::{BlockNumberProvider, Hash}, FixedU128, RuntimeDebug, SaturatedConversion, }; use xcm::{latest::XcmHash, VersionedLocation, VersionedXcm, MAX_XCM_DECODE_DEPTH}; @@ -96,12 +96,10 @@ pub use consensus_hook::{ConsensusHook, ExpectParentIncluded}; /// ``` /// struct BlockExecutor; /// struct Runtime; -/// struct CheckInherents; /// /// cumulus_pallet_parachain_system::register_validate_block! { /// Runtime = Runtime, /// BlockExecutor = Executive, -/// CheckInherents = CheckInherents, /// } /// /// # fn main() {} @@ -798,8 +796,8 @@ pub mod pallet { pub type NewValidationCode = StorageValue<_, Vec, OptionQuery>; /// The [`PersistedValidationData`] set for this block. - /// This value is expected to be set only once per block and it's never stored - /// in the trie. + /// + /// This value is expected to be set only once by the [`Pallet::set_validation_data`] inherent. #[pallet::storage] pub type ValidationData = StorageValue<_, PersistedValidationData>; @@ -1752,34 +1750,6 @@ impl polkadot_runtime_parachains::EnsureForParachain for Pallet { } } -/// Something that can check the inherents of a block. -#[deprecated(note = "This trait is deprecated and will be removed by September 2024. \ - Consider switching to `cumulus-pallet-parachain-system::ConsensusHook`")] -pub trait CheckInherents { - /// 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; -} - -/// Struct that always returns `Ok` on inherents check, needed for backwards-compatibility. -#[doc(hidden)] -pub struct DummyCheckInherents(core::marker::PhantomData); - -#[allow(deprecated)] -impl CheckInherents for DummyCheckInherents { - fn check_inherents( - _: &Block, - _: &RelayChainStateProof, - ) -> frame_support::inherent::CheckInherentsResult { - sp_inherents::CheckInherentsResult::new() - } -} - /// Something that should be informed about system related events. /// /// This includes events like [`on_validation_data`](Self::on_validation_data) that is being diff --git a/cumulus/pallets/parachain-system/src/validate_block/implementation.rs b/cumulus/pallets/parachain-system/src/validate_block/implementation.rs index e911f7d949b1e..855c7e1c06961 100644 --- a/cumulus/pallets/parachain-system/src/validate_block/implementation.rs +++ b/cumulus/pallets/parachain-system/src/validate_block/implementation.rs @@ -17,25 +17,22 @@ //! The actual implementation of the validate block functionality. use super::{trie_cache, trie_recorder, MemoryOptimizedValidationParams}; -use crate::parachain_inherent::BasicParachainInherentData; use alloc::vec::Vec; use codec::{Decode, Encode}; use cumulus_primitives_core::{ - relay_chain::{Hash as RHash, UMPSignal, UMP_SEPARATOR}, + relay_chain::{BlockNumber as RNumber, Hash as RHash, UMPSignal, UMP_SEPARATOR}, ClaimQueueOffset, CoreSelector, ParachainBlockData, PersistedValidationData, }; use frame_support::{ traits::{ExecuteBlock, Get, IsSubType}, BoundedVec, }; -use polkadot_parachain_primitives::primitives::{ - HeadData, RelayChainBlockNumber, ValidationResult, -}; +use polkadot_parachain_primitives::primitives::{HeadData, ValidationResult}; use sp_core::storage::{ChildInfo, StateVersion}; use sp_externalities::{set_and_run_with_externalities, Externalities}; use sp_io::{hashing::blake2_128, KillStorageResult}; use sp_runtime::traits::{ - Block as BlockT, ExtrinsicCall, ExtrinsicLike, Hash as HashT, HashingFor, Header as HeaderT, + Block as BlockT, ExtrinsicCall, Hash as HashT, HashingFor, Header as HeaderT, }; use sp_state_machine::OverlayedChanges; use sp_trie::{HashDBT, ProofSizeProvider, EMPTY_PREFIX}; @@ -69,21 +66,12 @@ environmental::environmental!(recorder: trait ProofSizeProvider); /// 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)] -#[allow(deprecated)] -pub fn validate_block< - B: BlockT, - E: ExecuteBlock, - PSC: crate::Config, - CI: crate::CheckInherents, ->( +pub fn validate_block, PSC: crate::Config>( MemoryOptimizedValidationParams { block_data, parent_head: parachain_head, @@ -205,49 +193,13 @@ where let mut overlay = OverlayedChanges::default(); parent_header = block.header().clone(); - let inherent_data = extract_parachain_inherent_data(&block); - - validate_validation_data( - &inherent_data.validation_data, - relay_parent_number, - relay_parent_storage_root, - ¶chain_head, - ); - - // We don't need the recorder or the overlay in here. - run_with_externalities_and_recorder::( - &backend, - &mut Default::default(), - &mut Default::default(), - || { - 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"); - - #[allow(deprecated)] - 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_and_recorder::( &execute_backend, // Here is the only place where we want to use the recorder. - // We want to ensure that we not accidentally read something from the proof, that was - // not yet read and thus, alter the proof size. Otherwise we end up with mismatches in - // later blocks. + // We want to ensure that we not accidentally read something from the proof, that + // was not yet read and thus, alter the proof size. Otherwise, we end up with + // mismatches in later blocks. &mut execute_recorder, &mut overlay, || { @@ -262,6 +214,15 @@ where // are passing here the overlay. &mut overlay, || { + // Ensure the validation data is correct. + validate_validation_data( + crate::ValidationData::::get() + .expect("`ValidationData` must be set after executing a block; qed"), + ¶chain_head, + relay_parent_number, + relay_parent_storage_root, + ); + new_validation_code = new_validation_code.take().or(crate::NewValidationCode::::get()); @@ -382,35 +343,14 @@ where } } -/// Extract the [`BasicParachainInherentData`]. -fn extract_parachain_inherent_data( - block: &B, -) -> &BasicParachainInherentData -where - B::Extrinsic: ExtrinsicCall, - ::Call: IsSubType>, -{ - block - .extrinsics() - .iter() - // Inherents are at the front of the block and are unsigned. - .take_while(|e| e.is_bare()) - .filter_map(|e| e.call().is_sub_type()) - .find_map(|c| match c { - crate::Call::set_validation_data { data: validation_data, .. } => Some(validation_data), - _ => None, - }) - .expect("Could not find `set_validation_data` inherent") -} - -/// Validate the given [`PersistedValidationData`] against the [`MemoryOptimizedValidationParams`]. +/// Validates the given [`PersistedValidationData`] against the data from the relay chain. fn validate_validation_data( - validation_data: &PersistedValidationData, - relay_parent_number: RelayChainBlockNumber, + validation_data: PersistedValidationData, + parent_header: &[u8], + relay_parent_number: RNumber, relay_parent_storage_root: RHash, - parent_head: &[u8], ) { - assert_eq!(parent_head, validation_data.parent_head.0, "Parent head doesn't match"); + assert_eq!(parent_header, &validation_data.parent_head.0, "Parent head doesn't match"); assert_eq!( relay_parent_number, validation_data.relay_parent_number, "Relay parent number doesn't match", diff --git a/cumulus/pallets/parachain-system/src/validate_block/tests.rs b/cumulus/pallets/parachain-system/src/validate_block/tests.rs index a68e694b7fe6e..2c055b61beed1 100644 --- a/cumulus/pallets/parachain-system/src/validate_block/tests.rs +++ b/cumulus/pallets/parachain-system/src/validate_block/tests.rs @@ -417,6 +417,8 @@ fn validate_block_fails_on_invalid_validation_data() { Default::default(), ); + let block = seal_parachain_block_data(block, &client); + call_validate_block(parent_head, block, Hash::random()).unwrap_err(); } else { let output = Command::new(env::current_exe().unwrap()) @@ -431,43 +433,6 @@ fn validate_block_fails_on_invalid_validation_data() { } } -#[test] -fn check_inherents_are_unsigned_and_before_all_other_extrinsics() { - sp_tracing::try_init_simple(); - - if env::var("RUN_TEST").is_ok() { - let (client, parent_head) = create_test_client(); - - let TestBlockData { mut block, validation_data, .. } = build_block_with_witness( - &client, - Vec::new(), - parent_head.clone(), - Default::default(), - Default::default(), - ); - - block.blocks_mut()[0].extrinsics.insert(0, transfer(&client, Alice, Bob, 69)); - - call_validate_block(parent_head, block, validation_data.relay_parent_storage_root) - .unwrap_err(); - } else { - let output = Command::new(env::current_exe().unwrap()) - .args([ - "check_inherents_are_unsigned_and_before_all_other_extrinsics", - "--", - "--nocapture", - ]) - .env("RUN_TEST", "1") - .output() - .expect("Runs the test"); - assert!(output.status.success()); - - assert!(String::from_utf8(output.stderr) - .unwrap() - .contains("Could not find `set_validation_data` inherent")); - } -} - /// Test that ensures that `ValidationParams` and `MemoryOptimizedValidationParams` /// are encoding/decoding. #[test] diff --git a/prdoc/pr_9732.prdoc b/prdoc/pr_9732.prdoc new file mode 100644 index 0000000000000..165fa181eb57c --- /dev/null +++ b/prdoc/pr_9732.prdoc @@ -0,0 +1,14 @@ +title: Remove the deprecated `CheckInherents` logic +doc: +- audience: Runtime Dev + description: |- + This removes the `CheckInherents` logic from `register_validate_block`, instead the `ConsensusHook` should be used to verify certain data. + + Besides that it also changes the how `validate_block` verifies the `ValidationData` passed by the collator. Instead of extracting the `ValidationData` before executing the block, the validation data is now checked as part of the pallet logic. +crates: +- name: cumulus-pallet-aura-ext + bump: major +- name: cumulus-pallet-parachain-system-proc-macro + bump: major +- name: cumulus-pallet-parachain-system + bump: major