Ensure inherent are first (paritytech#8173)

* impl

* fix tests

* impl in execute_block

* fix tests

* add a test in frame-executive

* fix some panic warning

* use trait to get call from extrinsic

* remove unused

* fix test

* fix testing

* fix tests

* return index of extrinsic on error

* fix test

* Update primitives/inherents/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* address comments

rename trait, and refactor

* refactor + doc improvment

* fix tests

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

frame/authorship/src/lib.rs

@@ -392,6 +392,10 @@ impl<T: Config> ProvideInherent for Module<T> {
 			},
 		}
 	}
+
+	fn is_inherent(call: &Self::Call) -> bool {
+		matches!(call, Call::set_uncles(_))
+	}
 }
 
 #[cfg(test)]

frame/executive/Cargo.toml

@@ -31,6 +31,7 @@ pallet-indices = { version = "3.0.0", path = "../indices" }
 pallet-balances = { version = "3.0.0", path = "../balances" }
 pallet-transaction-payment = { version = "3.0.0", path = "../transaction-payment" }
 sp-version = { version = "3.0.0", path = "../../primitives/version" }
+sp-inherents = { version = "3.0.0", path = "../../primitives/inherents" }
 
 [features]
 default = ["std"]

frame/executive/src/lib.rs

@@ -119,7 +119,10 @@
 use sp_std::{prelude::*, marker::PhantomData};
 use frame_support::{
 	weights::{GetDispatchInfo, DispatchInfo, DispatchClass},
-	traits::{OnInitialize, OnIdle, OnFinalize, OnRuntimeUpgrade, OffchainWorker, ExecuteBlock},
+	traits::{
+		OnInitialize, OnIdle, OnFinalize, OnRuntimeUpgrade, OffchainWorker, ExecuteBlock,
+		EnsureInherentsAreFirst,
+	},
 	dispatch::PostDispatchInfo,
 };
 use sp_runtime::{
@@ -153,7 +156,7 @@ pub struct Executive<System, Block, Context, UnsignedValidator, AllPallets, OnRu
 );
 
 impl<
-	System: frame_system::Config,
+	System: frame_system::Config + EnsureInherentsAreFirst<Block>,
 	Block: traits::Block<Header=System::Header, Hash=System::Hash>,
 	Context: Default,
 	UnsignedValidator,
@@ -181,7 +184,7 @@ where
 }
 
 impl<
-		System: frame_system::Config,
+		System: frame_system::Config + EnsureInherentsAreFirst<Block>,
 		Block: traits::Block<Header = System::Header, Hash = System::Hash>,
 		Context: Default,
 		UnsignedValidator,
@@ -311,6 +314,10 @@ where
 			&& <frame_system::Pallet<System>>::block_hash(n - System::BlockNumber::one()) == *header.parent_hash(),
 			"Parent hash should be valid.",
 		);
+
+		if let Err(i) = System::ensure_inherents_are_first(block) {
+			panic!("Invalid inherent position for extrinsic at index {}", i);
+		}
 	}
 
 	/// Actually execute all transitions for `block`.
@@ -543,7 +550,7 @@ mod tests {
 	mod custom {
 		use frame_support::weights::{Weight, DispatchClass};
 		use sp_runtime::transaction_validity::{
-			UnknownTransaction, TransactionSource, TransactionValidity
+			UnknownTransaction, TransactionSource, TransactionValidity, TransactionValidityError,
 		};
 
 		pub trait Config: frame_system::Config {}
@@ -574,6 +581,11 @@ mod tests {
 					frame_system::ensure_root(origin)?;
 				}
 
+				#[weight = 0]
+				fn inherent_call(origin) {
+					let _ = frame_system::ensure_none(origin)?;
+				}
+
 				// module hooks.
 				// one with block number arg and one without
 				fn on_initialize(n: T::BlockNumber) -> Weight {
@@ -600,16 +612,29 @@ mod tests {
 				}
 
 				#[weight = 0]
-				fn calculate_storage_root(origin) {
+				fn calculate_storage_root(_origin) {
 					let root = sp_io::storage::root();
 					sp_io::storage::set("storage_root".as_bytes(), &root);
 				}
 			}
 		}
 
+		impl<T: Config> sp_inherents::ProvideInherent for Module<T> {
+			type Call = Call<T>;
+			type Error = sp_inherents::MakeFatalError<()>;
+			const INHERENT_IDENTIFIER: [u8; 8] = *b"test1234";
+			fn create_inherent(_data: &sp_inherents::InherentData) -> Option<Self::Call> {
+				None
+			}
+			fn is_inherent(call: &Self::Call) -> bool {
+				*call == Call::<T>::inherent_call()
+			}
+		}
+
 		impl<T: Config> sp_runtime::traits::ValidateUnsigned for Module<T> {
 			type Call = Call<T>;
 
+			// Inherent call is not validated as unsigned
 			fn validate_unsigned(
 				_source: TransactionSource,
 				call: &Self::Call,
@@ -618,6 +643,18 @@ mod tests {
 					Call::allowed_unsigned(..) => Ok(Default::default()),
 					_ => UnknownTransaction::NoUnsignedValidator.into(),
 				}
+
+			}
+
+			// Inherent call is accepted for being dispatched
+			fn pre_dispatch(
+				call: &Self::Call,
+			) -> Result<(), TransactionValidityError> {
+				match call {
+					Call::allowed_unsigned(..) => Ok(()),
+					Call::inherent_call(..) => Ok(()),
+					_ => Err(UnknownTransaction::NoUnsignedValidator.into()),
+				}
 			}
 		}
 	}
@@ -630,7 +667,7 @@ mod tests {
 		{
 			System: frame_system::{Pallet, Call, Config, Storage, Event<T>},
 			Balances: pallet_balances::{Pallet, Call, Storage, Config<T>, Event<T>},
-			Custom: custom::{Pallet, Call, ValidateUnsigned},
+			Custom: custom::{Pallet, Call, ValidateUnsigned, Inherent},
 		}
 	);
 
@@ -718,12 +755,7 @@ mod tests {
 	);
 	type TestXt = sp_runtime::testing::TestXt<Call, SignedExtra>;
 	type TestBlock = Block<TestXt>;
-	type TestUncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic<
-		<Runtime as frame_system::Config>::AccountId,
-		<Runtime as frame_system::Config>::Call,
-		(),
-		SignedExtra,
-	>;
+	type TestUncheckedExtrinsic = TestXt;
 
 	// Will contain `true` when the custom runtime logic was called.
 	const CUSTOM_ON_RUNTIME_KEY: &[u8] = &*b":custom:on_runtime";
@@ -1227,4 +1259,57 @@ mod tests {
 			Executive::execute_block(Block::new(header, vec![xt]));
 		});
 	}
+
+	#[test]
+	#[should_panic(expected = "Invalid inherent position for extrinsic at index 1")]
+	fn invalid_inherent_position_fail() {
+		let xt1 = TestXt::new(Call::Balances(BalancesCall::transfer(33, 0)), sign_extra(1, 0, 0));
+		let xt2 = TestXt::new(Call::Custom(custom::Call::inherent_call()), None);
+
+		let header = new_test_ext(1).execute_with(|| {
+			// Let's build some fake block.
+			Executive::initialize_block(&Header::new(
+				1,
+				H256::default(),
+				H256::default(),
+				[69u8; 32].into(),
+				Digest::default(),
+			));
+
+			Executive::apply_extrinsic(xt1.clone()).unwrap().unwrap();
+			Executive::apply_extrinsic(xt2.clone()).unwrap().unwrap();
+
+			Executive::finalize_block()
+		});
+
+		new_test_ext(1).execute_with(|| {
+			Executive::execute_block(Block::new(header, vec![xt1, xt2]));
+		});
+	}
+
+	#[test]
+	fn valid_inherents_position_works() {
+		let xt1 = TestXt::new(Call::Custom(custom::Call::inherent_call()), None);
+		let xt2 = TestXt::new(Call::Balances(BalancesCall::transfer(33, 0)), sign_extra(1, 0, 0));
+
+		let header = new_test_ext(1).execute_with(|| {
+			// Let's build some fake block.
+			Executive::initialize_block(&Header::new(
+				1,
+				H256::default(),
+				H256::default(),
+				[69u8; 32].into(),
+				Digest::default(),
+			));
+
+			Executive::apply_extrinsic(xt1.clone()).unwrap().unwrap();
+			Executive::apply_extrinsic(xt2.clone()).unwrap().unwrap();
+
+			Executive::finalize_block()
+		});
+
+		new_test_ext(1).execute_with(|| {
+			Executive::execute_block(Block::new(header, vec![xt1, xt2]));
+		});
+	}
 }

frame/support/procedural/src/construct_runtime/mod.rs

@@ -167,6 +167,7 @@ fn construct_runtime_parsed(definition: RuntimeDefinition) -> Result<TokenStream
 	let metadata = decl_runtime_metadata(&name, pallets.iter(), &scrate, &unchecked_extrinsic);
 	let outer_config = decl_outer_config(&name, pallets.iter(), &scrate);
 	let inherent = decl_outer_inherent(
+		&name,
 		&block,
 		&unchecked_extrinsic,
 		pallets.iter(),
@@ -235,6 +236,7 @@ fn decl_validate_unsigned<'a>(
 }
 
 fn decl_outer_inherent<'a>(
+	runtime: &'a Ident,
 	block: &'a syn::TypePath,
 	unchecked_extrinsic: &'a syn::TypePath,
 	pallet_declarations: impl Iterator<Item = &'a Pallet>,
@@ -251,7 +253,8 @@ fn decl_outer_inherent<'a>(
 		#scrate::impl_outer_inherent!(
 			impl Inherents where
 				Block = #block,
-				UncheckedExtrinsic = #unchecked_extrinsic
+				UncheckedExtrinsic = #unchecked_extrinsic,
+				Runtime = #runtime,
 			{
 				#(#pallets_tokens)*
 			}