From a4ed9bb9b2aaa51c7e59cf3fec9c718702eb0536 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Tue, 13 Apr 2021 11:30:13 +0200 Subject: [PATCH] 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/Cargo.lock | 1 + substrate/frame/authorship/src/lib.rs | 4 + substrate/frame/executive/Cargo.toml | 1 + substrate/frame/executive/src/lib.rs | 109 +++++++- .../procedural/src/construct_runtime/mod.rs | 5 +- substrate/frame/support/src/inherent.rs | 262 +++++++++++++++--- substrate/frame/support/src/lib.rs | 8 + substrate/frame/support/src/traits.rs | 3 +- substrate/frame/support/src/traits/misc.rs | 37 +++ .../frame/support/test/tests/instance.rs | 8 + substrate/frame/support/test/tests/pallet.rs | 4 + .../support/test/tests/pallet_instance.rs | 4 + .../inherent_check_inner_span.stderr | 5 +- .../tests/pallet_with_name_trait_is_valid.rs | 4 + substrate/frame/timestamp/src/lib.rs | 4 + substrate/primitives/inherents/src/lib.rs | 21 +- substrate/primitives/runtime/src/testing.rs | 8 + 17 files changed, 427 insertions(+), 61 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index abb151282c..47fad18c36 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -1779,6 +1779,7 @@ dependencies = [ "parity-scale-codec", "serde", "sp-core", + "sp-inherents", "sp-io", "sp-runtime", "sp-std", diff --git a/substrate/frame/authorship/src/lib.rs b/substrate/frame/authorship/src/lib.rs index 5e9955f59f..da4b66f229 100644 --- a/substrate/frame/authorship/src/lib.rs +++ b/substrate/frame/authorship/src/lib.rs @@ -392,6 +392,10 @@ impl ProvideInherent for Module { }, } } + + fn is_inherent(call: &Self::Call) -> bool { + matches!(call, Call::set_uncles(_)) + } } #[cfg(test)] diff --git a/substrate/frame/executive/Cargo.toml b/substrate/frame/executive/Cargo.toml index 6a00423087..97c5a5ffdc 100644 --- a/substrate/frame/executive/Cargo.toml +++ b/substrate/frame/executive/Cargo.toml @@ -31,6 +31,7 @@ pallet-indices = { version = "3.0.0", path = "../indices" } pallet-balances = { version = "3.0.0", path = "../balances" } pallet-transaction-payment = { version = "3.0.0", path = "../transaction-payment" } sp-version = { version = "3.0.0", path = "../../primitives/version" } +sp-inherents = { version = "3.0.0", path = "../../primitives/inherents" } [features] default = ["std"] diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 97bdfbfdd5..0102fdea7c 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -119,7 +119,10 @@ use sp_std::{prelude::*, marker::PhantomData}; use frame_support::{ weights::{GetDispatchInfo, DispatchInfo, DispatchClass}, - traits::{OnInitialize, OnIdle, OnFinalize, OnRuntimeUpgrade, OffchainWorker, ExecuteBlock}, + traits::{ + OnInitialize, OnIdle, OnFinalize, OnRuntimeUpgrade, OffchainWorker, ExecuteBlock, + EnsureInherentsAreFirst, + }, dispatch::PostDispatchInfo, }; use sp_runtime::{ @@ -153,7 +156,7 @@ pub struct Executive, Block: traits::Block, Context: Default, UnsignedValidator, @@ -181,7 +184,7 @@ where } impl< - System: frame_system::Config, + System: frame_system::Config + EnsureInherentsAreFirst, Block: traits::Block
, Context: Default, UnsignedValidator, @@ -311,6 +314,10 @@ where && >::block_hash(n - System::BlockNumber::one()) == *header.parent_hash(), "Parent hash should be valid.", ); + + if let Err(i) = System::ensure_inherents_are_first(block) { + panic!("Invalid inherent position for extrinsic at index {}", i); + } } /// Actually execute all transitions for `block`. @@ -543,7 +550,7 @@ mod tests { mod custom { use frame_support::weights::{Weight, DispatchClass}; use sp_runtime::transaction_validity::{ - UnknownTransaction, TransactionSource, TransactionValidity + UnknownTransaction, TransactionSource, TransactionValidity, TransactionValidityError, }; pub trait Config: frame_system::Config {} @@ -574,6 +581,11 @@ mod tests { frame_system::ensure_root(origin)?; } + #[weight = 0] + fn inherent_call(origin) { + let _ = frame_system::ensure_none(origin)?; + } + // module hooks. // one with block number arg and one without fn on_initialize(n: T::BlockNumber) -> Weight { @@ -600,16 +612,29 @@ mod tests { } #[weight = 0] - fn calculate_storage_root(origin) { + fn calculate_storage_root(_origin) { let root = sp_io::storage::root(); sp_io::storage::set("storage_root".as_bytes(), &root); } } } + impl sp_inherents::ProvideInherent for Module { + type Call = Call; + type Error = sp_inherents::MakeFatalError<()>; + const INHERENT_IDENTIFIER: [u8; 8] = *b"test1234"; + fn create_inherent(_data: &sp_inherents::InherentData) -> Option { + None + } + fn is_inherent(call: &Self::Call) -> bool { + *call == Call::::inherent_call() + } + } + impl sp_runtime::traits::ValidateUnsigned for Module { type Call = Call; + // Inherent call is not validated as unsigned fn validate_unsigned( _source: TransactionSource, call: &Self::Call, @@ -618,6 +643,18 @@ mod tests { Call::allowed_unsigned(..) => Ok(Default::default()), _ => UnknownTransaction::NoUnsignedValidator.into(), } + + } + + // Inherent call is accepted for being dispatched + fn pre_dispatch( + call: &Self::Call, + ) -> Result<(), TransactionValidityError> { + match call { + Call::allowed_unsigned(..) => Ok(()), + Call::inherent_call(..) => Ok(()), + _ => Err(UnknownTransaction::NoUnsignedValidator.into()), + } } } } @@ -630,7 +667,7 @@ mod tests { { System: frame_system::{Pallet, Call, Config, Storage, Event}, Balances: pallet_balances::{Pallet, Call, Storage, Config, Event}, - Custom: custom::{Pallet, Call, ValidateUnsigned}, + Custom: custom::{Pallet, Call, ValidateUnsigned, Inherent}, } ); @@ -718,12 +755,7 @@ mod tests { ); type TestXt = sp_runtime::testing::TestXt; type TestBlock = Block; - type TestUncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic< - ::AccountId, - ::Call, - (), - SignedExtra, - >; + type TestUncheckedExtrinsic = TestXt; // Will contain `true` when the custom runtime logic was called. const CUSTOM_ON_RUNTIME_KEY: &[u8] = &*b":custom:on_runtime"; @@ -1227,4 +1259,57 @@ mod tests { Executive::execute_block(Block::new(header, vec![xt])); }); } + + #[test] + #[should_panic(expected = "Invalid inherent position for extrinsic at index 1")] + fn invalid_inherent_position_fail() { + let xt1 = TestXt::new(Call::Balances(BalancesCall::transfer(33, 0)), sign_extra(1, 0, 0)); + let xt2 = TestXt::new(Call::Custom(custom::Call::inherent_call()), None); + + let header = new_test_ext(1).execute_with(|| { + // Let's build some fake block. + Executive::initialize_block(&Header::new( + 1, + H256::default(), + H256::default(), + [69u8; 32].into(), + Digest::default(), + )); + + Executive::apply_extrinsic(xt1.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(xt2.clone()).unwrap().unwrap(); + + Executive::finalize_block() + }); + + new_test_ext(1).execute_with(|| { + Executive::execute_block(Block::new(header, vec![xt1, xt2])); + }); + } + + #[test] + fn valid_inherents_position_works() { + let xt1 = TestXt::new(Call::Custom(custom::Call::inherent_call()), None); + let xt2 = TestXt::new(Call::Balances(BalancesCall::transfer(33, 0)), sign_extra(1, 0, 0)); + + let header = new_test_ext(1).execute_with(|| { + // Let's build some fake block. + Executive::initialize_block(&Header::new( + 1, + H256::default(), + H256::default(), + [69u8; 32].into(), + Digest::default(), + )); + + Executive::apply_extrinsic(xt1.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(xt2.clone()).unwrap().unwrap(); + + Executive::finalize_block() + }); + + new_test_ext(1).execute_with(|| { + Executive::execute_block(Block::new(header, vec![xt1, xt2])); + }); + } } diff --git a/substrate/frame/support/procedural/src/construct_runtime/mod.rs b/substrate/frame/support/procedural/src/construct_runtime/mod.rs index 0951dbdea9..e14f90197f 100644 --- a/substrate/frame/support/procedural/src/construct_runtime/mod.rs +++ b/substrate/frame/support/procedural/src/construct_runtime/mod.rs @@ -167,6 +167,7 @@ fn construct_runtime_parsed(definition: RuntimeDefinition) -> Result( } fn decl_outer_inherent<'a>( + runtime: &'a Ident, block: &'a syn::TypePath, unchecked_extrinsic: &'a syn::TypePath, pallet_declarations: impl Iterator, @@ -251,7 +253,8 @@ fn decl_outer_inherent<'a>( #scrate::impl_outer_inherent!( impl Inherents where Block = #block, - UncheckedExtrinsic = #unchecked_extrinsic + UncheckedExtrinsic = #unchecked_extrinsic, + Runtime = #runtime, { #(#pallets_tokens)* } diff --git a/substrate/frame/support/src/inherent.rs b/substrate/frame/support/src/inherent.rs index 3c201dff29..87e489bd8f 100644 --- a/substrate/frame/support/src/inherent.rs +++ b/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 for Call { + impl crate::traits::IsSubType for Call { fn is_sub_type(&self) -> Option<&CallTest> { match self { Self::Test(test) => Some(test), @@ -171,7 +243,7 @@ mod tests { } } - impl IsSubType for Call { + impl crate::traits::IsSubType 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 { - 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 { - Some(CallTest2::Something) + Some(CallTest2::RequiredInherent) } - fn is_inherent_required(_: &InherentData) -> Result, Self::Error> { + fn is_inherent_required(_: &InherentData) -> Result, Self::Error> { Ok(Some(().into())) } + + fn is_inherent(call: &Self::Call) -> bool { + matches!(call, CallTest2::RequiredInherent) + } } type Block = testing::Block; #[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 { - Some(Self { function }) + fn new(function: Call, signed_data: Option<()>) -> Option { + Some(Self { + function, + signed: signed_data.is_some(), + }) + } + + fn is_signed(&self) -> Option { + 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![(*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); + } } diff --git a/substrate/frame/support/src/lib.rs b/substrate/frame/support/src/lib.rs index deedf0b621..ef5f64cfc2 100644 --- a/substrate/frame/support/src/lib.rs +++ b/substrate/frame/support/src/lib.rs @@ -1939,6 +1939,10 @@ pub mod pallet_prelude { /// fn create_inherent(_data: &InherentData) -> Option { /// 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 { /// unimplemented!(); /// } +/// +/// fn is_inherent(_call: &Self::Call) -> bool { +/// unimplemented!(); +/// } /// } /// /// // Regular rust code needed for implementing ProvideInherent trait diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index 7288f6c0d2..c629990dd6 100644 --- a/substrate/frame/support/src/traits.rs +++ b/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; diff --git a/substrate/frame/support/src/traits/misc.rs b/substrate/frame/support/src/traits/misc.rs index d5cc68840d..d3010358dd 100644 --- a/substrate/frame/support/src/traits/misc.rs +++ b/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; } + + + +/// 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 { + /// 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 ExtrinsicCall for sp_runtime::testing::TestXt where + Call: codec::Codec + Sync + Send, +{ + fn call(&self) -> &Self::Call { + &self.call + } +} + +impl ExtrinsicCall +for sp_runtime::generic::UncheckedExtrinsic +where + Extra: sp_runtime::traits::SignedExtension, +{ + fn call(&self) -> &Self::Call { + &self.function + } +} diff --git a/substrate/frame/support/test/tests/instance.rs b/substrate/frame/support/test/tests/instance.rs index e0dd1d1891..dbffead8ad 100644 --- a/substrate/frame/support/test/tests/instance.rs +++ b/substrate/frame/support/test/tests/instance.rs @@ -122,6 +122,10 @@ mod module1 { fn check_inherent(_: &Self::Call, _: &InherentData) -> std::result::Result<(), Self::Error> { unimplemented!(); } + + fn is_inherent(_call: &Self::Call) -> bool { + unimplemented!(); + } } } @@ -182,6 +186,10 @@ mod module2 { fn check_inherent(_call: &Self::Call, _data: &InherentData) -> std::result::Result<(), Self::Error> { unimplemented!(); } + + fn is_inherent(_call: &Self::Call) -> bool { + unimplemented!(); + } } } diff --git a/substrate/frame/support/test/tests/pallet.rs b/substrate/frame/support/test/tests/pallet.rs index 24a4990dde..04e8b2a8f1 100644 --- a/substrate/frame/support/test/tests/pallet.rs +++ b/substrate/frame/support/test/tests/pallet.rs @@ -288,6 +288,10 @@ pub mod pallet { T::AccountId::from(SomeType6); // Test for where clause unimplemented!(); } + + fn is_inherent(_call: &Self::Call) -> bool { + unimplemented!(); + } } #[derive(codec::Encode, sp_runtime::RuntimeDebug)] diff --git a/substrate/frame/support/test/tests/pallet_instance.rs b/substrate/frame/support/test/tests/pallet_instance.rs index d71242e49e..b181fe0bd6 100644 --- a/substrate/frame/support/test/tests/pallet_instance.rs +++ b/substrate/frame/support/test/tests/pallet_instance.rs @@ -170,6 +170,10 @@ pub mod pallet { fn create_inherent(_data: &InherentData) -> Option { unimplemented!(); } + + fn is_inherent(_call: &Self::Call) -> bool { + unimplemented!(); + } } #[derive(codec::Encode, sp_runtime::RuntimeDebug)] diff --git a/substrate/frame/support/test/tests/pallet_ui/inherent_check_inner_span.stderr b/substrate/frame/support/test/tests/pallet_ui/inherent_check_inner_span.stderr index 75a522889e..bc34c55241 100644 --- a/substrate/frame/support/test/tests/pallet_ui/inherent_check_inner_span.stderr +++ b/substrate/frame/support/test/tests/pallet_ui/inherent_check_inner_span.stderr @@ -1,10 +1,11 @@ -error[E0046]: not all trait items implemented, missing: `Call`, `Error`, `INHERENT_IDENTIFIER`, `create_inherent` +error[E0046]: not all trait items implemented, missing: `Call`, `Error`, `INHERENT_IDENTIFIER`, `create_inherent`, `is_inherent` --> $DIR/inherent_check_inner_span.rs:19:2 | 19 | impl ProvideInherent for Pallet {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `Call`, `Error`, `INHERENT_IDENTIFIER`, `create_inherent` in implementation + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `Call`, `Error`, `INHERENT_IDENTIFIER`, `create_inherent`, `is_inherent` in implementation | = help: implement the missing item: `type Call = Type;` = help: implement the missing item: `type Error = Type;` = help: implement the missing item: `const INHERENT_IDENTIFIER: [u8; 8] = value;` = help: implement the missing item: `fn create_inherent(_: &InherentData) -> std::option::Option<::Call> { todo!() }` + = help: implement the missing item: `fn is_inherent(_: &::Call) -> bool { todo!() }` diff --git a/substrate/frame/support/test/tests/pallet_with_name_trait_is_valid.rs b/substrate/frame/support/test/tests/pallet_with_name_trait_is_valid.rs index 9fc7055ce1..3d96fe24d9 100644 --- a/substrate/frame/support/test/tests/pallet_with_name_trait_is_valid.rs +++ b/substrate/frame/support/test/tests/pallet_with_name_trait_is_valid.rs @@ -81,6 +81,10 @@ impl sp_inherents::ProvideInherent for Module { fn check_inherent(_: &Self::Call, _: &sp_inherents::InherentData) -> std::result::Result<(), Self::Error> { unimplemented!(); } + + fn is_inherent(_call: &Self::Call) -> bool { + unimplemented!(); + } } #[cfg(test)] diff --git a/substrate/frame/timestamp/src/lib.rs b/substrate/frame/timestamp/src/lib.rs index dabf5a93c1..301d993c09 100644 --- a/substrate/frame/timestamp/src/lib.rs +++ b/substrate/frame/timestamp/src/lib.rs @@ -241,6 +241,10 @@ pub mod pallet { Ok(()) } } + + fn is_inherent(call: &Self::Call) -> bool { + matches!(call, Call::set(_)) + } } } diff --git a/substrate/primitives/inherents/src/lib.rs b/substrate/primitives/inherents/src/lib.rs index 0110db5680..facc620810 100644 --- a/substrate/primitives/inherents/src/lib.rs +++ b/substrate/primitives/inherents/src/lib.rs @@ -423,8 +423,10 @@ pub trait ProvideInherent { /// /// - `Err(_)` indicates that this function failed and further operations should be aborted. /// - /// CAUTION: This check has a bug when used in pallets that also provide unsigned transactions. - /// See for details. + /// NOTE: If inherent is required then the runtime asserts that the block contains at least + /// one inherent for which: + /// * type is [`Self::Call`], + /// * [`Self::is_inherent`] returns true. fn is_inherent_required(_: &InherentData) -> Result, Self::Error> { Ok(None) } /// Check whether the given inherent is valid. Checking the inherent is optional and can be @@ -433,9 +435,24 @@ pub trait ProvideInherent { /// When checking an inherent, the first parameter represents the inherent that is actually /// included in the block by its author. Whereas the second parameter represents the inherent /// data that the verifying node calculates. + /// + /// NOTE: A block can contains multiple inherent. fn check_inherent(_: &Self::Call, _: &InherentData) -> Result<(), Self::Error> { Ok(()) } + + /// Return whether the call is an inherent call. + /// + /// NOTE: Signed extrinsics are not inherent, but signed extrinsic with the given call variant + /// can be dispatched. + /// + /// # Warning + /// + /// In FRAME, inherent are enforced to be before other extrinsics, for this reason, + /// pallets with unsigned transactions **must ensure** that no unsigned transaction call + /// is an inherent call, when implementing `ValidateUnsigned::validate_unsigned`. + /// Otherwise block producer can produce invalid blocks by including them after non inherent. + fn is_inherent(call: &Self::Call) -> bool; } #[cfg(test)] diff --git a/substrate/primitives/runtime/src/testing.rs b/substrate/primitives/runtime/src/testing.rs index b6d2641f01..f473dc7028 100644 --- a/substrate/primitives/runtime/src/testing.rs +++ b/substrate/primitives/runtime/src/testing.rs @@ -303,6 +303,14 @@ impl traits::Extrinsic for TestXt } } +impl traits::ExtrinsicMetadata for TestXt where + Call: Codec + Sync + Send, + Extra: SignedExtension, +{ + type SignedExtensions = Extra; + const VERSION: u8 = 0u8; +} + impl Applyable for TestXt where Call: 'static + Sized + Send + Sync + Clone + Eq + Codec + Debug + Dispatchable, Extra: SignedExtension,