Ensure inherent are first (#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>

substrate/frame/support/src/inherent.rs

@@ -20,8 +20,10 @@ pub use crate::sp_std::vec::Vec;
 #[doc(hidden)]
 pub use crate::sp_runtime::traits::{Block as BlockT, Extrinsic};
 #[doc(hidden)]
-pub use sp_inherents::{InherentData, ProvideInherent, CheckInherentsResult, IsFatalError};
-
+pub use sp_inherents::{
+	InherentData, ProvideInherent, CheckInherentsResult, IsFatalError, InherentIdentifier,
+	MakeFatalError,
+};
 
 /// Implement the outer inherent.
 /// All given modules need to implement `ProvideInherent`.
@@ -30,7 +32,11 @@ pub use sp_inherents::{InherentData, ProvideInherent, CheckInherentsResult, IsFa
 ///
 /// ```nocompile
 /// impl_outer_inherent! {
-///     impl Inherents where Block = Block, UncheckedExtrinsic = UncheckedExtrinsic {
+///     impl Inherents where
+///         Block = Block,
+///         UncheckedExtrinsic = UncheckedExtrinsic,
+///         Runtime = Runtime,
+///     {
 ///         timestamp,
 ///         consensus,
 ///         aura,
@@ -42,7 +48,8 @@ macro_rules! impl_outer_inherent {
 	(
 		impl Inherents where
 			Block = $block:ident,
-			UncheckedExtrinsic = $uncheckedextrinsic:ident
+			UncheckedExtrinsic = $uncheckedextrinsic:ident,
+			Runtime = $runtime:ident,
 		{
 			$( $module:ident, )*
 		}
@@ -56,16 +63,19 @@ macro_rules! impl_outer_inherent {
 		impl InherentDataExt for $crate::inherent::InherentData {
 			fn create_extrinsics(&self) ->
 				$crate::inherent::Vec<<$block as $crate::inherent::BlockT>::Extrinsic> {
-				use $crate::inherent::{ProvideInherent, Extrinsic};
+				use $crate::inherent::ProvideInherent;
 
 				let mut inherents = Vec::new();
 
 				$(
 					if let Some(inherent) = $module::create_inherent(self) {
-						inherents.push($uncheckedextrinsic::new(
+						let inherent = <$uncheckedextrinsic as $crate::inherent::Extrinsic>::new(
 							inherent.into(),
 							None,
-						).expect("Runtime UncheckedExtrinsic is not Opaque, so it has to return `Some`; qed"));
+						).expect("Runtime UncheckedExtrinsic is not Opaque, so it has to return \
+							`Some`; qed");
+
+						inherents.push(inherent);
 					}
 				)*
 
@@ -74,41 +84,64 @@ macro_rules! impl_outer_inherent {
 
 			fn check_extrinsics(&self, block: &$block) -> $crate::inherent::CheckInherentsResult {
 				use $crate::inherent::{ProvideInherent, IsFatalError};
-				use $crate::traits::IsSubType;
+				use $crate::traits::{IsSubType, ExtrinsicCall};
 				use $crate::sp_runtime::traits::Block as _;
 
 				let mut result = $crate::inherent::CheckInherentsResult::new();
+
 				for xt in block.extrinsics() {
+					// Inherents are before any other extrinsics.
+					// And signed extrinsics are not inherents.
 					if $crate::inherent::Extrinsic::is_signed(xt).unwrap_or(false) {
 						break
 					}
 
+					let mut is_inherent = false;
+
 					$({
-						if let Some(call) = IsSubType::<_>::is_sub_type(&xt.function) {
-							if let Err(e) = $module::check_inherent(call, self) {
-								result.put_error(
-									$module::INHERENT_IDENTIFIER, &e
-								).expect("There is only one fatal error; qed");
-								if e.is_fatal_error() {
-									return result
+						let call = <$uncheckedextrinsic as ExtrinsicCall>::call(xt);
+						if let Some(call) = IsSubType::<_>::is_sub_type(call) {
+							if $module::is_inherent(call) {
+								is_inherent = true;
+								if let Err(e) = $module::check_inherent(call, self) {
+									result.put_error(
+										$module::INHERENT_IDENTIFIER, &e
+									).expect("There is only one fatal error; qed");
+									if e.is_fatal_error() {
+										return result
+									}
 								}
 							}
 						}
 					})*
+
+					// Inherents are before any other extrinsics.
+					// No module marked it as inherent thus it is not.
+					if !is_inherent {
+						break
+					}
 				}
 
 				$(
 					match $module::is_inherent_required(self) {
 						Ok(Some(e)) => {
 							let found = block.extrinsics().iter().any(|xt| {
-								if $crate::inherent::Extrinsic::is_signed(xt).unwrap_or(false) {
-									return false
+								let is_signed = $crate::inherent::Extrinsic::is_signed(xt)
+									.unwrap_or(false);
+
+								if !is_signed {
+									let call = <
+										$uncheckedextrinsic as ExtrinsicCall
+									>::call(xt);
+									if let Some(call) = IsSubType::<_>::is_sub_type(call) {
+										$module::is_inherent(&call)
+									} else {
+										false
+									}
+								} else {
+									// Signed extrinsics are not inherents.
+									false
 								}
-
-								let call: Option<&<$module as ProvideInherent>::Call> =
-									xt.function.is_sub_type();
-
-								call.is_some()
 							});
 
 							if !found {
@@ -135,6 +168,46 @@ macro_rules! impl_outer_inherent {
 				result
 			}
 		}
+
+		impl $crate::traits::EnsureInherentsAreFirst<$block> for $runtime {
+			fn ensure_inherents_are_first(block: &$block) -> Result<(), u32> {
+				use $crate::inherent::ProvideInherent;
+				use $crate::traits::{IsSubType, ExtrinsicCall};
+				use $crate::sp_runtime::traits::Block as _;
+
+				let mut first_signed_observed = false;
+
+				for (i, xt) in block.extrinsics().iter().enumerate() {
+					let is_signed = $crate::inherent::Extrinsic::is_signed(xt).unwrap_or(false);
+
+					let is_inherent = if is_signed {
+						// Signed extrinsics are not inherents.
+						false
+					} else {
+						let mut is_inherent = false;
+						$({
+							let call = <$uncheckedextrinsic as ExtrinsicCall>::call(xt);
+							if let Some(call) = IsSubType::<_>::is_sub_type(call) {
+								if $module::is_inherent(&call) {
+									is_inherent = true;
+								}
+							}
+						})*
+						is_inherent
+					};
+
+					if !is_inherent {
+						first_signed_observed = true;
+					}
+
+					if first_signed_observed && is_inherent {
+						return Err(i as u32)
+					}
+				}
+
+				Ok(())
+			}
+		}
 	};
 }
 
@@ -142,7 +215,6 @@ macro_rules! impl_outer_inherent {
 mod tests {
 	use super::*;
 	use sp_runtime::{traits, testing::{Header, self}};
-	use crate::traits::IsSubType;
 
 	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
 	enum Call {
@@ -162,7 +234,7 @@ mod tests {
 		}
 	}
 
-	impl IsSubType<CallTest> for Call {
+	impl crate::traits::IsSubType<CallTest> for Call {
 		fn is_sub_type(&self) -> Option<&CallTest> {
 			match self {
 				Self::Test(test) => Some(test),
@@ -171,7 +243,7 @@ mod tests {
 		}
 	}
 
-	impl IsSubType<CallTest2> for Call {
+	impl crate::traits::IsSubType<CallTest2> for Call {
 		fn is_sub_type(&self) -> Option<&CallTest2> {
 			match self {
 				Self::Test2(test) => Some(test),
@@ -182,13 +254,13 @@ mod tests {
 
 	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
 	enum CallTest {
-		Something,
-		SomethingElse,
+		OptionalInherent(bool),
+		NotInherent,
 	}
 
 	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
 	enum CallTest2 {
-		Something,
+		RequiredInherent,
 	}
 
 	struct ModuleTest;
@@ -198,15 +270,20 @@ mod tests {
 		const INHERENT_IDENTIFIER: sp_inherents::InherentIdentifier = *b"test1235";
 
 		fn create_inherent(_: &InherentData) -> Option<Self::Call> {
-			Some(CallTest::Something)
+			Some(CallTest::OptionalInherent(true))
 		}
 
 		fn check_inherent(call: &Self::Call, _: &InherentData) -> Result<(), Self::Error> {
 			match call {
-				CallTest::Something => Ok(()),
-				CallTest::SomethingElse => Err(().into()),
+				CallTest::OptionalInherent(true) => Ok(()),
+				CallTest::OptionalInherent(false) => Err(().into()),
+				_ => unreachable!("other calls are not inherents"),
 			}
 		}
+
+		fn is_inherent(call: &Self::Call) -> bool {
+			matches!(call, CallTest::OptionalInherent(_))
+		}
 	}
 
 	struct ModuleTest2;
@@ -216,18 +293,23 @@ mod tests {
 		const INHERENT_IDENTIFIER: sp_inherents::InherentIdentifier = *b"test1234";
 
 		fn create_inherent(_: &InherentData) -> Option<Self::Call> {
-			Some(CallTest2::Something)
+			Some(CallTest2::RequiredInherent)
 		}
 
-		fn is_inherent_required(_: &InherentData) -> Result<Option<Self::Error>, Self::Error> { 
+		fn is_inherent_required(_: &InherentData) -> Result<Option<Self::Error>, Self::Error> {
 			Ok(Some(().into()))
 		}
+
+		fn is_inherent(call: &Self::Call) -> bool {
+			matches!(call, CallTest2::RequiredInherent)
+		}
 	}
 
 	type Block = testing::Block<Extrinsic>;
 
 	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
 	struct Extrinsic {
+		signed: bool,
 		function: Call,
 	}
 
@@ -235,15 +317,34 @@ mod tests {
 		type Call = Call;
 		type SignaturePayload = ();
 
-		fn new(function: Call, _: Option<()>) -> Option<Self> {
-			Some(Self { function })
+		fn new(function: Call, signed_data: Option<()>) -> Option<Self> {
+			Some(Self {
+				function,
+				signed: signed_data.is_some(),
+			})
+		}
+
+		fn is_signed(&self) -> Option<bool> {
+			Some(self.signed)
+		}
+	}
+
+	impl crate::traits::ExtrinsicCall for Extrinsic {
+		fn call(&self) -> &Self::Call {
+			&self.function
 		}
 	}
 
 	parity_util_mem::malloc_size_of_is_0!(Extrinsic);
 
+	struct Runtime;
+
 	impl_outer_inherent! {
-		impl Inherents where Block = Block, UncheckedExtrinsic = Extrinsic {
+		impl Inherents where
+			Block = Block,
+			UncheckedExtrinsic = Extrinsic,
+			Runtime = Runtime,
+		{
 			ModuleTest,
 			ModuleTest2,
 		}
@@ -254,8 +355,8 @@ mod tests {
 		let inherents = InherentData::new().create_extrinsics();
 
 		let expected = vec![
-			Extrinsic { function: Call::Test(CallTest::Something) },
-			Extrinsic { function: Call::Test2(CallTest2::Something) },
+			Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false },
+			Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
 		];
 		assert_eq!(expected, inherents);
 	}
@@ -265,8 +366,8 @@ mod tests {
 		let block = Block::new(
 			Header::new_from_number(1),
 			vec![
-				Extrinsic { function: Call::Test2(CallTest2::Something) },
-				Extrinsic { function: Call::Test(CallTest::Something) },
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false },
 			],
 		);
 
@@ -275,8 +376,8 @@ mod tests {
 		let block = Block::new(
 			Header::new_from_number(1),
 			vec![
-				Extrinsic { function: Call::Test2(CallTest2::Something) },
-				Extrinsic { function: Call::Test(CallTest::SomethingElse) },
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(false)), signed: false },
 			],
 		);
 
@@ -287,9 +388,84 @@ mod tests {
 	fn required_inherents_enforced() {
 		let block = Block::new(
 			Header::new_from_number(1),
-			vec![Extrinsic { function: Call::Test(CallTest::Something) }],
+			vec![
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false }
+			],
 		);
 
 		assert!(InherentData::new().check_extrinsics(&block).fatal_error());
 	}
+
+	#[test]
+	fn signed_are_not_inherent() {
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				// NOTE: checking this call would fail, but it is not checked as it is not an
+				// inherent, because it is signed.
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(false)), signed: true },
+			],
+		);
+
+		assert!(InherentData::new().check_extrinsics(&block).ok());
+
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![
+				// NOTE: this is not considered an inherent, thus block is failing because of
+				// missing required inherent.
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: true },
+			],
+		);
+
+		assert_eq!(
+			InherentData::new().check_extrinsics(&block).into_errors().collect::<Vec<_>>(),
+			vec![(*b"test1234", vec![])],
+		);
+	}
+
+	#[test]
+	fn inherent_first_works() {
+		use crate::traits::EnsureInherentsAreFirst;
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false },
+				Extrinsic { function: Call::Test(CallTest::NotInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::NotInherent), signed: false },
+			],
+		);
+
+		assert!(Runtime::ensure_inherents_are_first(&block).is_ok());
+	}
+
+	#[test]
+	fn inherent_cannot_be_placed_after_non_inherent() {
+		use crate::traits::EnsureInherentsAreFirst;
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::NotInherent), signed: false },
+				// This inherent is placed after non inherent: invalid
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false },
+			],
+		);
+
+		assert_eq!(Runtime::ensure_inherents_are_first(&block).err().unwrap(), 2);
+
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![
+				Extrinsic { function: Call::Test2(CallTest2::RequiredInherent), signed: false },
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: true },
+				// This inherent is placed after non inherent: invalid
+				Extrinsic { function: Call::Test(CallTest::OptionalInherent(true)), signed: false },
+			],
+		);
+
+		assert_eq!(Runtime::ensure_inherents_are_first(&block).err().unwrap(), 2);
+	}
 }

substrate/frame/support/src/lib.rs

@@ -1939,6 +1939,10 @@ pub mod pallet_prelude {
 /// 		fn create_inherent(_data: &InherentData) -> Option<Self::Call> {
 /// 			unimplemented!();
 /// 		}
+///
+/// 		fn is_inherent(_call: &Self::Call) -> bool {
+/// 			unimplemented!();
+/// 		}
 /// 	}
 ///
 /// 	// Regular rust code needed for implementing ProvideInherent trait
@@ -2066,6 +2070,10 @@ pub mod pallet_prelude {
 /// 		fn create_inherent(_data: &InherentData) -> Option<Self::Call> {
 /// 			unimplemented!();
 /// 		}
+///
+/// 		fn is_inherent(_call: &Self::Call) -> bool {
+/// 			unimplemented!();
+/// 		}
 /// 	}
 ///
 /// 	// Regular rust code needed for implementing ProvideInherent trait

substrate/frame/support/src/traits.rs

@@ -49,7 +49,8 @@ pub use filter::{
 mod misc;
 pub use misc::{
 	Len, Get, GetDefault, HandleLifetime, TryDrop, Time, UnixTime, IsType, IsSubType, ExecuteBlock,
-	SameOrOther, OnNewAccount, OnKilledAccount, OffchainWorker, GetBacking, Backing,
+	SameOrOther, OnNewAccount, OnKilledAccount, OffchainWorker, GetBacking, Backing, ExtrinsicCall,
+	EnsureInherentsAreFirst,
 };
 
 mod stored_map;

substrate/frame/support/src/traits/misc.rs

@@ -284,3 +284,40 @@ pub trait GetBacking {
 	/// implicit motion. `None` if it does not.
 	fn get_backing(&self) -> Option<Backing>;
 }
+
+
+
+/// A trait to ensure the inherent are before non-inherent in a block.
+///
+/// This is typically implemented on runtime, through `construct_runtime!`.
+pub trait EnsureInherentsAreFirst<Block> {
+	/// Ensure the position of inherent is correct, i.e. they are before non-inherents.
+	///
+	/// On error return the index of the inherent with invalid position (counting from 0).
+	fn ensure_inherents_are_first(block: &Block) -> Result<(), u32>;
+}
+
+/// An extrinsic on which we can get access to call.
+pub trait ExtrinsicCall: sp_runtime::traits::Extrinsic {
+	/// Get the call of the extrinsic.
+	fn call(&self) -> &Self::Call;
+}
+
+#[cfg(feature = "std")]
+impl<Call, Extra> ExtrinsicCall for sp_runtime::testing::TestXt<Call, Extra> where
+	Call: codec::Codec + Sync + Send,
+{
+	fn call(&self) -> &Self::Call {
+		&self.call
+	}
+}
+
+impl<Address, Call, Signature, Extra> ExtrinsicCall
+for sp_runtime::generic::UncheckedExtrinsic<Address, Call, Signature, Extra>
+where
+	Extra: sp_runtime::traits::SignedExtension,
+{
+	fn call(&self) -> &Self::Call {
+		&self.function
+	}
+}