diff --git a/substrate/bin/node/executor/tests/basic.rs b/substrate/bin/node/executor/tests/basic.rs index 1ee0a17c81..fccf4a62cc 100644 --- a/substrate/bin/node/executor/tests/basic.rs +++ b/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![], }, diff --git a/substrate/client/basic-authorship/src/basic_authorship.rs b/substrate/client/basic-authorship/src/basic_authorship.rs index b3bf486aae..37bb34a4b6 100644 --- a/substrate/client/basic-authorship/src/basic_authorship.rs +++ b/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 ProposerInner // 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 ProposerInner 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); diff --git a/substrate/frame/authorship/src/lib.rs b/substrate/frame/authorship/src/lib.rs index d71a71e5bf..e6249849bf 100644 --- a/substrate/frame/authorship/src/lib.rs +++ b/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) -> dispatch::DispatchResult { ensure_none(origin)?; ensure!(new_uncles.len() <= MAX_UNCLES, Error::::TooManyUncles); diff --git a/substrate/frame/finality-tracker/src/lib.rs b/substrate/frame/finality-tracker/src/lib.rs index 4a6e2392f2..8200543ffa 100644 --- a/substrate/frame/finality-tracker/src/lib.rs +++ b/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!(!::Update::exists(), Error::::AlreadyUpdated); diff --git a/substrate/frame/support/src/weights.rs b/substrate/frame/support/src/weights.rs index 4d501305f0..ea3368550f 100644 --- a/substrate/frame/support/src/weights.rs +++ b/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 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 WeighData for SimpleDispatchInfo { @@ -220,9 +240,9 @@ impl WeighData 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 PaysFee for SimpleDispatchInfo { SimpleDispatchInfo::FixedNormal(_) => true, SimpleDispatchInfo::MaxNormal => true, SimpleDispatchInfo::InsecureFreeNormal => true, - SimpleDispatchInfo::FixedOperational(_) => true, SimpleDispatchInfo::MaxOperational => true, + SimpleDispatchInfo::FixedMandatory(_) => true, } } } diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 7823916347..050ab2654b 100644 --- a/substrate/frame/system/src/lib.rs +++ b/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 CheckWeight { /// a portion. fn get_dispatch_limit_ratio(class: DispatchClass) -> Perbill { match class { - DispatchClass::Operational => ::one(), + DispatchClass::Operational | DispatchClass::Mandatory + => ::one(), DispatchClass::Normal => T::AvailableBlockRatio::get(), } } @@ -1186,7 +1187,7 @@ impl CheckWeight { 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 CheckWeight { fn get_priority(info: ::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 SignedExtension for CheckWeight { 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 SignedExtension for CheckWeight { info: Self::DispatchInfo, len: usize, ) -> TransactionValidity { + if info.class == DispatchClass::Mandatory { + Err(InvalidTransaction::MandatoryDispatch)? + } Self::do_validate(info, len) } @@ -1299,6 +1308,21 @@ impl SignedExtension for CheckWeight { ) -> 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 Debug for CheckWeight { diff --git a/substrate/frame/timestamp/src/lib.rs b/substrate/frame/timestamp/src/lib.rs index 54b4eeca6b..8ba756d683 100644 --- a/substrate/frame/timestamp/src/lib.rs +++ b/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!(!::DidUpdate::exists(), "Timestamp must be updated only once in the block"); diff --git a/substrate/primitives/runtime/src/generic/checked_extrinsic.rs b/substrate/primitives/runtime/src/generic/checked_extrinsic.rs index 7a71ef5218..673501bb91 100644 --- a/substrate/primitives/runtime/src/generic/checked_extrinsic.rs +++ b/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)); - Extra::post_dispatch(pre, info, len); - Ok(res.map(|_| ()).map_err(|e| e.error)) + let res = self.function.dispatch(Origin::from(maybe_who)) + .map(|_| ()) + .map_err(|e| e.error); + Extra::post_dispatch(pre, info.clone(), len, &res)?; + Ok(res) } } diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs index 8bbaa50266..da5d5c4a81 100644 --- a/substrate/primitives/runtime/src/traits.rs +++ b/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 { @@ -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; } @@ -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. - fn post_dispatch(_pre: Self::Pre, _info: Self::DispatchInfo, _len: usize) { } + /// Do any post-flight stuff for an extrinsic. + /// + /// 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 SignedExtension for Tuple { pre: Self::Pre, info: Self::DispatchInfo, len: usize, - ) { - for_tuples!( #( Tuple::post_dispatch(pre.Tuple, info.clone(), len); )* ) + result: &DispatchResult, + ) -> Result<(), TransactionValidityError> { + for_tuples!( #( Tuple::post_dispatch(pre.Tuple, info.clone(), len, result)?; )* ); + Ok(()) } fn identifier() -> Vec<&'static str> { diff --git a/substrate/primitives/runtime/src/transaction_validity.rs b/substrate/primitives/runtime/src/transaction_validity.rs index 78f724b4d2..94cf44384d 100644 --- a/substrate/primitives/runtime/src/transaction_validity.rs +++ b/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 for &'static str { @@ -76,6 +91,10 @@ impl From 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 for &'static str {