frame-executive: Reject invalid inherents in the executive (#12365)

* frame-executive: Reject invalid inherents in the executive

We already had support for making a block fail if an inherent returned, but it was part of the
signed extension `CheckWeight`. Rejecting blocks with invalid inherents should happen on the
`frame-executive` level without requiring any special signed extension. This is crucial to prevent
any kind of spamming of the network that could may happen with blocks that include failing inherents.

* FMT

* Update frame/executive/src/lib.rs

Co-authored-by: Keith Yeung <[email protected]>

* Update primitives/runtime/src/transaction_validity.rs

Co-authored-by: Keith Yeung <[email protected]>

Co-authored-by: parity-processbot <>
Co-authored-by: Keith Yeung <[email protected]>

frame/executive/src/lib.rs

@@ -119,6 +119,7 @@
 use codec::{Codec, Encode};
 use frame_support::{
 	dispatch::{DispatchClass, DispatchInfo, GetDispatchInfo, PostDispatchInfo},
+	pallet_prelude::InvalidTransaction,
 	traits::{
 		EnsureInherentsAreFirst, ExecuteBlock, OffchainWorker, OnFinalize, OnIdle, OnInitialize,
 		OnRuntimeUpgrade,
@@ -497,6 +498,14 @@ where
 		let dispatch_info = xt.get_dispatch_info();
 		let r = Applyable::apply::<UnsignedValidator>(xt, &dispatch_info, encoded_len)?;
 
+		// Mandatory(inherents) are not allowed to fail.
+		//
+		// The entire block should be discarded if an inherent fails to apply. Otherwise
+		// it may open an attack vector.
+		if r.is_err() && dispatch_info.class == DispatchClass::Mandatory {
+			return Err(InvalidTransaction::BadMandatory.into())
+		}
+
 		<frame_system::Pallet<System>>::note_applied_extrinsic(&r, dispatch_info);
 
 		Ok(r.map(|_| ()).map_err(|e| e.error))
@@ -563,6 +572,10 @@ where
 			xt.get_dispatch_info()
 		};
 
+		if dispatch_info.class == DispatchClass::Mandatory {
+			return Err(InvalidTransaction::MandatoryValidation.into())
+		}
+
 		within_span! {
 			sp_tracing::Level::TRACE, "validate";
 			xt.validate::<UnsignedValidator>(source, &dispatch_info, encoded_len)
@@ -692,9 +705,9 @@ mod tests {
 				Ok(())
 			}
 
-			#[pallet::weight(0)]
+			#[pallet::weight((0, DispatchClass::Mandatory))]
 			pub fn inherent_call(origin: OriginFor<T>) -> DispatchResult {
-				let _ = frame_system::ensure_none(origin)?;
+				frame_system::ensure_none(origin)?;
 				Ok(())
 			}
 
@@ -1533,4 +1546,38 @@ mod tests {
 			Executive::execute_block(Block::new(header, vec![xt1, xt2]));
 		});
 	}
+
+	#[test]
+	#[should_panic(expected = "A call was labelled as mandatory, but resulted in an Error.")]
+	fn invalid_inherents_fail_block_execution() {
+		let xt1 =
+			TestXt::new(RuntimeCall::Custom(custom::Call::inherent_call {}), sign_extra(1, 0, 0));
+
+		new_test_ext(1).execute_with(|| {
+			Executive::execute_block(Block::new(
+				Header::new(
+					1,
+					H256::default(),
+					H256::default(),
+					[69u8; 32].into(),
+					Digest::default(),
+				),
+				vec![xt1],
+			));
+		});
+	}
+
+	// Inherents are created by the runtime and don't need to be validated.
+	#[test]
+	fn inherents_fail_validate_block() {
+		let xt1 = TestXt::new(RuntimeCall::Custom(custom::Call::inherent_call {}), None);
+
+		new_test_ext(1).execute_with(|| {
+			assert_eq!(
+				Executive::validate_transaction(TransactionSource::External, xt1, H256::random())
+					.unwrap_err(),
+				InvalidTransaction::MandatoryValidation.into()
+			);
+		})
+	}
 }

frame/support/src/dispatch.rs

@@ -359,13 +359,15 @@ where
 
 /// Implementation for test extrinsic.
 #[cfg(feature = "std")]
-impl<Call: Encode, Extra: Encode> GetDispatchInfo for sp_runtime::testing::TestXt<Call, Extra> {
+impl<Call: Encode + GetDispatchInfo, Extra: Encode> GetDispatchInfo
+	for sp_runtime::testing::TestXt<Call, Extra>
+{
 	fn get_dispatch_info(&self) -> DispatchInfo {
 		// for testing: weight == size.
 		DispatchInfo {
 			weight: Weight::from_ref_time(self.encode().len() as _),
 			pays_fee: Pays::Yes,
-			..Default::default()
+			class: self.call.get_dispatch_info().class,
 		}
 	}
 }

frame/system/src/extensions/check_weight.rs

@@ -18,7 +18,7 @@
 use crate::{limits::BlockWeights, Config, Pallet};
 use codec::{Decode, Encode};
 use frame_support::{
-	dispatch::{DispatchClass, DispatchInfo, PostDispatchInfo},
+	dispatch::{DispatchInfo, PostDispatchInfo},
 	traits::Get,
 };
 use scale_info::TypeInfo;
@@ -190,9 +190,6 @@ where
 		info: &DispatchInfoOf<Self::Call>,
 		len: usize,
 	) -> Result<(), TransactionValidityError> {
-		if info.class == DispatchClass::Mandatory {
-			return Err(InvalidTransaction::MandatoryDispatch.into())
-		}
 		Self::do_pre_dispatch(info, len)
 	}
 
@@ -203,9 +200,6 @@ where
 		info: &DispatchInfoOf<Self::Call>,
 		len: usize,
 	) -> TransactionValidity {
-		if info.class == DispatchClass::Mandatory {
-			return Err(InvalidTransaction::MandatoryDispatch.into())
-		}
 		Self::do_validate(info, len)
 	}
 
@@ -230,16 +224,8 @@ where
 		info: &DispatchInfoOf<Self::Call>,
 		post_info: &PostDispatchInfoOf<Self::Call>,
 		_len: usize,
-		result: &DispatchResult,
+		_result: &DispatchResult,
 	) -> Result<(), TransactionValidityError> {
-		// Since mandatory dispatched do not get validated for being overweight, we are sensitive
-		// to them actually being useful. Block producers are thus not allowed to include mandatory
-		// extrinsics that result in error.
-		if let (DispatchClass::Mandatory, Err(e)) = (info.class, result) {
-			log::error!(target: "runtime::system", "Bad mandatory: {:?}", e);
-			return Err(InvalidTransaction::BadMandatory.into())
-		}
-
 		let unspent = post_info.calc_unspent(info);
 		if unspent.any_gt(Weight::zero()) {
 			crate::BlockWeight::<T>::mutate(|current_weight| {
@@ -268,7 +254,7 @@ mod tests {
 	use super::*;
 	use crate::{
 		mock::{new_test_ext, System, Test, CALL},
-		AllExtrinsicsLen, BlockWeight,
+		AllExtrinsicsLen, BlockWeight, DispatchClass,
 	};
 	use frame_support::{assert_err, assert_ok, dispatch::Pays, weights::Weight};
 	use sp_std::marker::PhantomData;

primitives/runtime/src/transaction_validity.rs

@@ -77,9 +77,9 @@ pub enum InvalidTransaction {
 	/// malicious validator or a buggy `provide_inherent`. In any case, it can result in
 	/// dangerously overweight blocks and therefore if found, invalidates the block.
 	BadMandatory,
-	/// A transaction with a mandatory dispatch. This is invalid; only inherent extrinsics are
-	/// allowed to have mandatory dispatches.
-	MandatoryDispatch,
+	/// An extrinsic with a mandatory dispatch tried to be validated.
+	/// This is invalid; only inherent extrinsics are allowed to have mandatory dispatches.
+	MandatoryValidation,
 	/// The sending address is disabled or known to be invalid.
 	BadSigner,
 }
@@ -109,8 +109,8 @@ impl From<InvalidTransaction> for &'static str {
 				"Inability to pay some fees (e.g. account balance too low)",
 			InvalidTransaction::BadMandatory =>
 				"A call was labelled as mandatory, but resulted in an Error.",
-			InvalidTransaction::MandatoryDispatch =>
-				"Transaction dispatch is mandatory; transactions may not have mandatory dispatches.",
+			InvalidTransaction::MandatoryValidation =>
+				"Transaction dispatch is mandatory; transactions must not be validated.",
 			InvalidTransaction::Custom(_) => "InvalidTransaction custom error",
 			InvalidTransaction::BadSigner => "Invalid signing address",
 		}