Mandatory dispatch class (#5515)

* Mandatory dispatch class

* Tweaks

* Docs

* Fix test

* Update frame/support/src/weights.rs

Co-Authored-By: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Introduce logic that was stated in PR.

* Use

* Docs.

* Fix test

* Fix merge

* Update frame/support/src/weights.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Fix.

* Fix

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/bin/node/executor/tests/basic.rs

@@ -338,7 +338,7 @@ fn full_native_block_import_works() {
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(0),
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 10000, class: DispatchClass::Operational, pays_fee: true }
+					DispatchInfo { weight: 10000, class: DispatchClass::Mandatory, pays_fee: true }
 				)),
 				topics: vec![],
 			},
@@ -391,7 +391,7 @@ fn full_native_block_import_works() {
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(0),
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 10000, class: DispatchClass::Operational, pays_fee: true }
+					DispatchInfo { weight: 10000, class: DispatchClass::Mandatory, pays_fee: true }
 				)),
 				topics: vec![],
 			},

substrate/client/basic-authorship/src/basic_authorship.rs

@@ -23,7 +23,7 @@ use sc_client_api::backend;
 use codec::Decode;
 use sp_consensus::{evaluation, Proposal, RecordProof};
 use sp_inherents::InherentData;
-use log::{error, info, debug, trace};
+use log::{error, info, debug, trace, warn};
 use sp_core::ExecutionContext;
 use sp_runtime::{
 	generic::BlockId,
@@ -34,7 +34,7 @@ use sc_telemetry::{telemetry, CONSENSUS_INFO};
 use sc_block_builder::{BlockBuilderApi, BlockBuilderProvider};
 use sp_api::{ProvideRuntimeApi, ApiExt};
 use futures::{executor, future, future::Either};
-use sp_blockchain::{HeaderBackend, ApplyExtrinsicFailed};
+use sp_blockchain::{HeaderBackend, ApplyExtrinsicFailed::Validity, Error::ApplyExtrinsicFailed};
 use std::marker::PhantomData;
 
 /// Proposer factory.
@@ -196,14 +196,25 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A>
 
 		// We don't check the API versions any further here since the dispatch compatibility
 		// check should be enough.
-		for extrinsic in self.client.runtime_api()
+		for inherent in self.client.runtime_api()
 			.inherent_extrinsics_with_context(
 				&self.parent_id,
 				ExecutionContext::BlockConstruction,
 				inherent_data
 			)?
 		{
-			block_builder.push(extrinsic)?;
+			match block_builder.push(inherent) {
+				Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() =>
+					warn!("⚠️  Dropping non-mandatory inherent from overweight block."),
+				Err(ApplyExtrinsicFailed(Validity(e))) if e.was_mandatory() => {
+					error!("❌️ Mandatory inherent extrinsic returned error. Block cannot be produced.");
+					Err(ApplyExtrinsicFailed(Validity(e)))?
+				}
+				Err(e) => {
+					warn!("❗️ Inherent extrinsic returned unexpected error: {}. Dropping.", e);
+				}
+				Ok(_) => {}
+			}
 		}
 
 		// proceed with transactions
@@ -241,7 +252,7 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A>
 				Ok(()) => {
 					debug!("[{:?}] Pushed to the block.", pending_tx_hash);
 				}
-				Err(sp_blockchain::Error::ApplyExtrinsicFailed(ApplyExtrinsicFailed::Validity(e)))
+				Err(ApplyExtrinsicFailed(Validity(e)))
 						if e.exhausted_resources() => {
 					if is_first {
 						debug!("[{:?}] Invalid transaction: FullBlock on empty block", pending_tx_hash);

substrate/frame/authorship/src/lib.rs

@@ -207,7 +207,7 @@ decl_module! {
 		}
 
 		/// Provide a set of uncles.
-		#[weight = SimpleDispatchInfo::FixedOperational(10_000)]
+		#[weight = SimpleDispatchInfo::FixedMandatory(10_000)]
 		fn set_uncles(origin, new_uncles: Vec<T::Header>) -> dispatch::DispatchResult {
 			ensure_none(origin)?;
 			ensure!(new_uncles.len() <= MAX_UNCLES, Error::<T>::TooManyUncles);

substrate/frame/finality-tracker/src/lib.rs

@@ -76,7 +76,7 @@ decl_module! {
 
 		/// Hint that the author of this block thinks the best finalized
 		/// block is the given number.
-		#[weight = frame_support::weights::SimpleDispatchInfo::default()]
+		#[weight = frame_support::weights::SimpleDispatchInfo::FixedMandatory(10_000)]
 		fn final_hint(origin, #[compact] hint: T::BlockNumber) {
 			ensure_none(origin)?;
 			ensure!(!<Self as Store>::Update::exists(), Error::<T>::AlreadyUpdated);

substrate/frame/support/src/weights.rs

@@ -85,6 +85,19 @@ pub enum DispatchClass {
 	Normal,
 	/// An operational dispatch.
 	Operational,
+	/// A mandatory dispatch. These kinds of dispatch are always included regardless of their
+	/// weight, therefore it is critical that they are separately validated to ensure that a
+	/// malicious validator cannot craft a valid but impossibly heavy block. Usually this just means
+	/// ensuring that the extrinsic can only be included once and that it is always very light.
+	///
+	/// Do *NOT* use it for extrinsics that can be heavy.
+	///
+	/// The only real use case for this is inherent extrinsics that are required to execute in a
+	/// block for the block to be valid, and it solves the issue in the case that the block
+	/// initialization is sufficiently heavy to mean that those inherents do not fit into the
+	/// block. Essentially, we assume that in these exceptional circumstances, it is better to
+	/// allow an overweight block to be created than to not allow any block at all to be created.
+	Mandatory,
 }
 
 impl Default for DispatchClass {
@@ -102,6 +115,8 @@ impl From<SimpleDispatchInfo> for DispatchClass {
 			SimpleDispatchInfo::FixedNormal(_) => DispatchClass::Normal,
 			SimpleDispatchInfo::MaxNormal => DispatchClass::Normal,
 			SimpleDispatchInfo::InsecureFreeNormal => DispatchClass::Normal,
+
+			SimpleDispatchInfo::FixedMandatory(_) => DispatchClass::Mandatory,
 		}
 	}
 }
@@ -212,6 +227,11 @@ pub enum SimpleDispatchInfo {
 	FixedOperational(Weight),
 	/// An operational dispatch with the maximum weight.
 	MaxOperational,
+	/// A mandatory dispatch with fixed weight.
+	///
+	/// NOTE: Signed transactions may not (directly) dispatch this kind of a call, so the other
+	/// attributes concerning transactability (e.g. priority, fee paying) are moot.
+	FixedMandatory(Weight),
 }
 
 impl<T> WeighData<T> for SimpleDispatchInfo {
@@ -220,9 +240,9 @@ impl<T> WeighData<T> for SimpleDispatchInfo {
 			SimpleDispatchInfo::FixedNormal(w) => *w,
 			SimpleDispatchInfo::MaxNormal => Bounded::max_value(),
 			SimpleDispatchInfo::InsecureFreeNormal => Bounded::min_value(),
-
 			SimpleDispatchInfo::FixedOperational(w) => *w,
 			SimpleDispatchInfo::MaxOperational => Bounded::max_value(),
+			SimpleDispatchInfo::FixedMandatory(w) => *w,
 		}
 	}
 }
@@ -239,9 +259,9 @@ impl<T> PaysFee<T> for SimpleDispatchInfo {
 			SimpleDispatchInfo::FixedNormal(_) => true,
 			SimpleDispatchInfo::MaxNormal => true,
 			SimpleDispatchInfo::InsecureFreeNormal => true,
-
 			SimpleDispatchInfo::FixedOperational(_) => true,
 			SimpleDispatchInfo::MaxOperational => true,
+			SimpleDispatchInfo::FixedMandatory(_) => true,
 		}
 	}
 }

substrate/frame/system/src/lib.rs

@@ -99,7 +99,7 @@ use sp_std::marker::PhantomData;
 use sp_std::fmt::Debug;
 use sp_version::RuntimeVersion;
 use sp_runtime::{
-	RuntimeDebug, Perbill, DispatchOutcome, DispatchError,
+	RuntimeDebug, Perbill, DispatchOutcome, DispatchError, DispatchResult,
 	generic::{self, Era},
 	transaction_validity::{
 		ValidTransaction, TransactionPriority, TransactionLongevity, TransactionValidityError,
@@ -119,7 +119,7 @@ use frame_support::{
 		Contains, Get, ModuleToIndex, OnNewAccount, OnKilledAccount, IsDeadAccount, Happened,
 		StoredMap, EnsureOrigin,
 	},
-	weights::{Weight, DispatchInfo, DispatchClass, SimpleDispatchInfo, FunctionOf},
+	weights::{Weight, DispatchInfo, DispatchClass, SimpleDispatchInfo, FunctionOf}
 };
 use codec::{Encode, Decode, FullCodec, EncodeLike};
 
@@ -1170,7 +1170,8 @@ impl<T: Trait + Send + Sync> CheckWeight<T> {
 	/// a portion.
 	fn get_dispatch_limit_ratio(class: DispatchClass) -> Perbill {
 		match class {
-			DispatchClass::Operational => <Perbill as sp_runtime::PerThing>::one(),
+			DispatchClass::Operational | DispatchClass::Mandatory
+				=> <Perbill as sp_runtime::PerThing>::one(),
 			DispatchClass::Normal => T::AvailableBlockRatio::get(),
 		}
 	}
@@ -1186,7 +1187,7 @@ impl<T: Trait + Send + Sync> CheckWeight<T> {
 		let limit = Self::get_dispatch_limit_ratio(info.class) * maximum_weight;
 		let added_weight = info.weight.min(limit);
 		let next_weight = current_weight.saturating_add(added_weight);
-		if next_weight > limit {
+		if next_weight > limit && info.class != DispatchClass::Mandatory {
 			Err(InvalidTransaction::ExhaustsResources.into())
 		} else {
 			Ok(next_weight)
@@ -1216,7 +1217,9 @@ impl<T: Trait + Send + Sync> CheckWeight<T> {
 	fn get_priority(info: <Self as SignedExtension>::DispatchInfo) -> TransactionPriority {
 		match info.class {
 			DispatchClass::Normal => info.weight.into(),
-			DispatchClass::Operational => Bounded::max_value()
+			DispatchClass::Operational => Bounded::max_value(),
+			// Mandatory extrinsics are only for inherents; never transactions.
+			DispatchClass::Mandatory => Bounded::min_value(),
 		}
 	}
 
@@ -1271,6 +1274,9 @@ impl<T: Trait + Send + Sync> SignedExtension for CheckWeight<T> {
 		info: Self::DispatchInfo,
 		len: usize,
 	) -> Result<(), TransactionValidityError> {
+		if info.class == DispatchClass::Mandatory {
+			Err(InvalidTransaction::MandatoryDispatch)?
+		}
 		Self::do_pre_dispatch(info, len)
 	}
 
@@ -1281,6 +1287,9 @@ impl<T: Trait + Send + Sync> SignedExtension for CheckWeight<T> {
 		info: Self::DispatchInfo,
 		len: usize,
 	) -> TransactionValidity {
+		if info.class == DispatchClass::Mandatory {
+			Err(InvalidTransaction::MandatoryDispatch)?
+		}
 		Self::do_validate(info, len)
 	}
 
@@ -1299,6 +1308,21 @@ impl<T: Trait + Send + Sync> SignedExtension for CheckWeight<T> {
 	) -> TransactionValidity {
 		Self::do_validate(info, len)
 	}
+
+	fn post_dispatch(
+		_pre: Self::Pre,
+		info: Self::DispatchInfo,
+		_len: usize,
+		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 info.class == DispatchClass::Mandatory && result.is_err() {
+			Err(InvalidTransaction::BadMandatory)?
+		}
+		Ok(())
+	}
 }
 
 impl<T: Trait + Send + Sync> Debug for CheckWeight<T> {

substrate/frame/timestamp/src/lib.rs

@@ -147,7 +147,7 @@ decl_module! {
 		/// `MinimumPeriod`.
 		///
 		/// The dispatch origin for this call must be `Inherent`.
-		#[weight = SimpleDispatchInfo::FixedOperational(10_000)]
+		#[weight = SimpleDispatchInfo::FixedMandatory(10_000)]
 		fn set(origin, #[compact] now: T::Moment) {
 			ensure_none(origin)?;
 			assert!(!<Self as Store>::DidUpdate::exists(), "Timestamp must be updated only once in the block");

substrate/primitives/runtime/src/generic/checked_extrinsic.rs

@@ -78,8 +78,10 @@ where
 			U::pre_dispatch(&self.function)?;
 			(None, pre)
 		};
-		let res = self.function.dispatch(Origin::from(maybe_who));
+		let res = self.function.dispatch(Origin::from(maybe_who))
-		Extra::post_dispatch(pre, info, len);
+			.map(|_| ())
-		Ok(res.map(|_| ()).map_err(|e| e.error))
+			.map_err(|e| e.error);
+		Extra::post_dispatch(pre, info.clone(), len, &res)?;
+		Ok(res)
 	}
 }

substrate/primitives/runtime/src/traits.rs

@@ -39,6 +39,7 @@ pub use sp_arithmetic::traits::{
 };
 use sp_application_crypto::AppKey;
 use impl_trait_for_tuples::impl_for_tuples;
+use crate::DispatchResult;
 
 /// A lazy value.
 pub trait Lazy<T: ?Sized> {
@@ -627,7 +628,7 @@ pub trait Dispatchable {
 	/// Additional information that is returned by `dispatch`. Can be used to supply the caller
 	/// with information about a `Dispatchable` that is ownly known post dispatch.
 	type PostInfo: Eq + PartialEq + Clone + Copy + Encode + Decode + Printable;
-	/// Actually dispatch this call and result the result of it.
+	/// Actually dispatch this call and return the result of it.
 	fn dispatch(self, origin: Self::Origin) -> crate::DispatchResultWithInfo<Self::PostInfo>;
 }
 
@@ -735,8 +736,27 @@ pub trait SignedExtension: Codec + Debug + Sync + Send + Clone + Eq + PartialEq
 			.map_err(Into::into)
 	}
 
-	/// Do any post-flight stuff for a transaction.
+	/// Do any post-flight stuff for an extrinsic.
-	fn post_dispatch(_pre: Self::Pre, _info: Self::DispatchInfo, _len: usize) { }
+	///
+	/// This gets given the `DispatchResult` `_result` from the extrinsic and can, if desired,
+	/// introduce a `TransactionValidityError`, causing the block to become invalid for including
+	/// it.
+	///
+	/// WARNING: It is dangerous to return an error here. To do so will fundamentally invalidate the
+	/// transaction and any block that it is included in, causing the block author to not be
+	/// compensated for their work in validating the transaction or producing the block so far.
+	///
+	/// It can only be used safely when you *know* that the extrinsic is one that can only be
+	/// introduced by the current block author; generally this implies that it is an inherent and
+	/// will come from either an offchain-worker or via `InherentData`.
+	fn post_dispatch(
+		_pre: Self::Pre,
+		_info: Self::DispatchInfo,
+		_len: usize,
+		_result: &DispatchResult,
+	) -> Result<(), TransactionValidityError> {
+		Ok(())
+	}
 
 	/// Returns the list of unique identifier for this signed extension.
 	///
@@ -804,8 +824,10 @@ impl<AccountId, Call, Info: Clone> SignedExtension for Tuple {
 		pre: Self::Pre,
 		info: Self::DispatchInfo,
 		len: usize,
-	) {
+		result: &DispatchResult,
-		for_tuples!( #( Tuple::post_dispatch(pre.Tuple, info.clone(), len); )* )
+	) -> Result<(), TransactionValidityError> {
+		for_tuples!( #( Tuple::post_dispatch(pre.Tuple, info.clone(), len, result)?; )* );
+		Ok(())
 	}
 
 	fn identifier() -> Vec<&'static str> {

substrate/primitives/runtime/src/transaction_validity.rs

@@ -52,6 +52,13 @@ pub enum InvalidTransaction {
 	ExhaustsResources,
 	/// Any other custom invalid validity that is not covered by this enum.
 	Custom(u8),
+	/// An extrinsic with a Mandatory dispatch resulted in Error. This is indicative of either a
+	/// 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,
 }
 
 impl InvalidTransaction {
@@ -62,6 +69,14 @@ impl InvalidTransaction {
 			_ => false,
 		}
 	}
+
+	/// Returns if the reason for the invalidity was a mandatory call failing.
+	pub fn was_mandatory(&self) -> bool {
+		match self {
+			Self::BadMandatory => true,
+			_ => false,
+		}
+	}
 }
 
 impl From<InvalidTransaction> for &'static str {
@@ -76,6 +91,10 @@ impl From<InvalidTransaction> for &'static str {
 				"Transaction would exhausts the block limits",
 			InvalidTransaction::Payment =>
 				"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 =>
+				"Tranaction dispatch is mandatory; transactions may not have mandatory dispatches.",
 			InvalidTransaction::Custom(_) => "InvalidTransaction custom error",
 		}
 	}
@@ -123,6 +142,15 @@ impl TransactionValidityError {
 			Self::Unknown(_) => false,
 		}
 	}
+
+	/// Returns `true` if the reason for the error was it being a mandatory dispatch that could not
+	/// be completed successfully.
+	pub fn was_mandatory(&self) -> bool {
+		match self {
+			Self::Invalid(e) => e.was_mandatory(),
+			Self::Unknown(_) => false,
+		}
+	}
 }
 
 impl From<TransactionValidityError> for &'static str {