Remove the deprecated CheckInherents logic (#9732)

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.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: 2 people

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::<Runtime, Executive>,

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<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,
@@ -67,7 +65,7 @@ impl Parse for Input {
 			} 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)?;
+				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<Ident, Error> {
 
 #[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)

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<T: Config> = StorageValue<_, Vec<u8>, 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<T: Config> = StorageValue<_, PersistedValidationData>;
 
@@ -1752,34 +1750,6 @@ impl<T: Config> polkadot_runtime_parachains::EnsureForParachain for Pallet<T> {
 	}
 }
 
-/// 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<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;
-}
-
-/// Struct that always returns `Ok` on inherents check, needed for backwards-compatibility.
-#[doc(hidden)]
-pub struct DummyCheckInherents<Block>(core::marker::PhantomData<Block>);
-
-#[allow(deprecated)]
-impl<Block: BlockT> CheckInherents<Block> for DummyCheckInherents<Block> {
-	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

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<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: 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,
-			&parachain_head,
-		);
-
-		// We don't need the recorder or the overlay in here.
-		run_with_externalities_and_recorder::<B, _, _>(
-			&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::<B, _, _>(
 			&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::<PSC>::get()
+						.expect("`ValidationData` must be set after executing a block; qed"),
+					&parachain_head,
+					relay_parent_number,
+					relay_parent_storage_root,
+				);
+
 				new_validation_code =
 					new_validation_code.take().or(crate::NewValidationCode::<PSC>::get());
 
@@ -382,35 +343,14 @@ where
 	}
 }
 
-/// Extract the [`BasicParachainInherentData`].
-fn extract_parachain_inherent_data<B: BlockT, PSC: crate::Config>(
-	block: &B,
-) -> &BasicParachainInherentData
-where
-	B::Extrinsic: ExtrinsicCall,
-	<B::Extrinsic as ExtrinsicCall>::Call: IsSubType<crate::Call<PSC>>,
-{
-	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",

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]

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