From 6e466e91aee9b33d8acfa63c0b7f3f82d1458a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 26 Sep 2022 17:34:09 +0200 Subject: [PATCH 1/4] 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. --- frame/executive/src/lib.rs | 51 ++++++++++++++++++- frame/support/src/dispatch.rs | 4 +- frame/system/src/extensions/check_weight.rs | 20 ++------ .../runtime/src/transaction_validity.rs | 10 ++-- 4 files changed, 59 insertions(+), 26 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index a41c82da5757c..011f47e37d732 100644 --- a/frame/executive/src/lib.rs +++ b/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::(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 opens some attack vector. + if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { + return Err(InvalidTransaction::BadMandatory.into()) + } + >::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::(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) -> 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() + ); + }) + } } diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index db2bc90658ee2..9368bd974dc56 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -359,13 +359,13 @@ where /// Implementation for test extrinsic. #[cfg(feature = "std")] -impl GetDispatchInfo for sp_runtime::testing::TestXt { +impl GetDispatchInfo for sp_runtime::testing::TestXt { 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, } } } diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 15a88913cd337..b6008fae4a7e7 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/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, 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, 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, post_info: &PostDispatchInfoOf, _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::::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; diff --git a/primitives/runtime/src/transaction_validity.rs b/primitives/runtime/src/transaction_validity.rs index 7cc8b70df9f96..6d68b1a08ab98 100644 --- a/primitives/runtime/src/transaction_validity.rs +++ b/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 was 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 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", } From 3fa46202bdb1e1e45e10a74bd370dce0146ce081 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 27 Sep 2022 15:20:02 +0200 Subject: [PATCH 2/4] FMT --- frame/support/src/dispatch.rs | 4 +++- frame/system/src/extensions/check_weight.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index 9368bd974dc56..365e8ac2668e3 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -359,7 +359,9 @@ where /// Implementation for test extrinsic. #[cfg(feature = "std")] -impl GetDispatchInfo for sp_runtime::testing::TestXt { +impl GetDispatchInfo + for sp_runtime::testing::TestXt +{ fn get_dispatch_info(&self) -> DispatchInfo { // for testing: weight == size. DispatchInfo { diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index b6008fae4a7e7..b63eff20fbf61 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -254,7 +254,7 @@ mod tests { use super::*; use crate::{ mock::{new_test_ext, System, Test, CALL}, - AllExtrinsicsLen, BlockWeight, DispatchClass + AllExtrinsicsLen, BlockWeight, DispatchClass, }; use frame_support::{assert_err, assert_ok, dispatch::Pays, weights::Weight}; use sp_std::marker::PhantomData; From 3edcfaebe3d6ed16d3efb2146f1c1329b2afb6eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 26 Nov 2022 10:22:59 +0100 Subject: [PATCH 3/4] Update frame/executive/src/lib.rs Co-authored-by: Keith Yeung --- frame/executive/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index f788990eb38df..71f138b596b8d 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -501,7 +501,7 @@ where // Mandatory(inherents) are not allowed to fail. // // The entire block should be discarded if an inherent fails to apply. Otherwise - // it may opens some attack vector. + // it may open an attack vector. if r.is_err() && dispatch_info.class == DispatchClass::Mandatory { return Err(InvalidTransaction::BadMandatory.into()) } From c0dce71cb1a35b5238adb58a9bcb7b659349cbb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 26 Nov 2022 10:23:04 +0100 Subject: [PATCH 4/4] Update primitives/runtime/src/transaction_validity.rs Co-authored-by: Keith Yeung --- primitives/runtime/src/transaction_validity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime/src/transaction_validity.rs b/primitives/runtime/src/transaction_validity.rs index 28c2f0565145e..d8e71cc2761ec 100644 --- a/primitives/runtime/src/transaction_validity.rs +++ b/primitives/runtime/src/transaction_validity.rs @@ -77,7 +77,7 @@ 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, - /// An extrinsic with a Mandatory dispatch was tried to be validated. + /// 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.