From c1fdf16324efc7ac3266a03fac188a28a6227d5f Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 1 Dec 2021 09:59:09 +0900 Subject: [PATCH] Fix order of hook execution (#10043) * fix order * Update frame/support/procedural/src/construct_runtime/mod.rs Co-authored-by: Alexander Popiak * format * more accurate description * format * better explicit types * fix tests * address feedback * add a type to ease non breaking implementation * add comment about constraint * fix test * add test for generated types Co-authored-by: Alexander Popiak --- bin/node-template/runtime/src/lib.rs | 2 +- bin/node/runtime/src/lib.rs | 4 +- frame/executive/src/lib.rs | 104 +++++++-------- .../procedural/src/construct_runtime/mod.rs | 71 +++++++++-- frame/support/test/tests/pallet.rs | 120 +++++++++++++++++- frame/support/test/tests/pallet_instance.rs | 19 ++- 6 files changed, 231 insertions(+), 89 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index 8ecb2199dda71..4eaa0aa00d0b6 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -323,7 +323,7 @@ pub type Executive = frame_executive::Executive< Block, frame_system::ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, >; impl_runtime_apis! { diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 44cefecd067f1..3f553221e67ac 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1268,6 +1268,8 @@ construct_runtime!( Utility: pallet_utility, Babe: pallet_babe, Timestamp: pallet_timestamp, + // Authorship must be before session in order to note author in the correct session and era + // for im-online and staking. Authorship: pallet_authorship, Indices: pallet_indices, Balances: pallet_balances, @@ -1345,7 +1347,7 @@ pub type Executive = frame_executive::Executive< Block, frame_system::ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, pallet_bags_list::migrations::CheckCounterPrefix, >; diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index dd0a9abf8687b..9b81527fadb35 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -59,7 +59,7 @@ //! # type Context = frame_system::ChainContext; //! # pub type Block = generic::Block; //! # pub type Balances = u64; -//! # pub type AllPallets = u64; +//! # pub type AllPalletsWithSystem = u64; //! # pub enum Runtime {}; //! # use sp_runtime::transaction_validity::{ //! # TransactionValidity, UnknownTransaction, TransactionSource, @@ -73,7 +73,7 @@ //! # } //! # } //! /// Executive: handles dispatch to the various modules. -//! pub type Executive = executive::Executive; +//! pub type Executive = executive::Executive; //! ``` //! //! ### Custom `OnRuntimeUpgrade` logic @@ -90,7 +90,7 @@ //! # type Context = frame_system::ChainContext; //! # pub type Block = generic::Block; //! # pub type Balances = u64; -//! # pub type AllPallets = u64; +//! # pub type AllPalletsWithSystem = u64; //! # pub enum Runtime {}; //! # use sp_runtime::transaction_validity::{ //! # TransactionValidity, UnknownTransaction, TransactionSource, @@ -111,7 +111,7 @@ //! } //! } //! -//! pub type Executive = executive::Executive; +//! pub type Executive = executive::Executive; //! ``` #![cfg_attr(not(feature = "std"), no_std)] @@ -147,11 +147,26 @@ pub type OriginOf = as Dispatchable>::Origin; /// - `Block`: The block type of the runtime /// - `Context`: The context that is used when checking an extrinsic. /// - `UnsignedValidator`: The unsigned transaction validator of the runtime. -/// - `AllPallets`: Tuple that contains all modules. Will be used to call e.g. `on_initialize`. +/// - `AllPalletsWithSystem`: Tuple that contains all pallets including frame system pallet. Will be +/// used to call hooks e.g. `on_initialize`. /// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are -/// already called by `AllPallets`. It will be called before all modules will be called. -pub struct Executive( - PhantomData<(System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade)>, +/// already called by `AllPalletsWithSystem`. It will be called before all modules will be called. +pub struct Executive< + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + OnRuntimeUpgrade = (), +>( + PhantomData<( + System, + Block, + Context, + UnsignedValidator, + AllPalletsWithSystem, + OnRuntimeUpgrade, + )>, ); impl< @@ -159,14 +174,14 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPallets: OnRuntimeUpgrade + AllPalletsWithSystem: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, > ExecuteBlock - for Executive + for Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -181,7 +196,7 @@ where Block, Context, UnsignedValidator, - AllPallets, + AllPalletsWithSystem, COnRuntimeUpgrade, >::execute_block(block); } @@ -192,13 +207,13 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPallets: OnRuntimeUpgrade + AllPalletsWithSystem: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, - > Executive + > Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -209,14 +224,7 @@ where { /// Execute all `OnRuntimeUpgrade` of this runtime, and return the aggregate weight. pub fn execute_on_runtime_upgrade() -> frame_support::weights::Weight { - let mut weight = 0; - weight = weight.saturating_add(COnRuntimeUpgrade::on_runtime_upgrade()); - weight = weight.saturating_add( - as OnRuntimeUpgrade>::on_runtime_upgrade(), - ); - weight = weight.saturating_add(::on_runtime_upgrade()); - - weight + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::on_runtime_upgrade() } /// Execute given block, but don't do any of the `final_checks`. @@ -255,19 +263,10 @@ where /// This should only be used for testing. #[cfg(feature = "try-runtime")] pub fn try_runtime_upgrade() -> Result { - < - (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) - as - OnRuntimeUpgrade - >::pre_upgrade().unwrap(); - + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::pre_upgrade().unwrap(); let weight = Self::execute_on_runtime_upgrade(); - < - (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) - as - OnRuntimeUpgrade - >::post_upgrade().unwrap(); + <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::post_upgrade().unwrap(); Ok(weight) } @@ -305,12 +304,9 @@ where digest, frame_system::InitKind::Full, ); - weight = weight.saturating_add( as OnInitialize< + weight = weight.saturating_add(>::on_initialize(*block_number)); - weight = weight.saturating_add( - >::on_initialize(*block_number), - ); weight = weight.saturating_add( >::get().base_block, ); @@ -415,30 +411,20 @@ where fn idle_and_finalize_hook(block_number: NumberFor) { let weight = >::block_weight(); let max_weight = >::get().max_block; - let mut remaining_weight = max_weight.saturating_sub(weight.total()); + let remaining_weight = max_weight.saturating_sub(weight.total()); if remaining_weight > 0 { - let mut used_weight = - as OnIdle>::on_idle( - block_number, - remaining_weight, - ); - remaining_weight = remaining_weight.saturating_sub(used_weight); - used_weight = >::on_idle( + let used_weight = >::on_idle( block_number, remaining_weight, - ) - .saturating_add(used_weight); + ); >::register_extra_weight_unchecked( used_weight, DispatchClass::Mandatory, ); } - as OnFinalize>::on_finalize( - block_number, - ); - >::on_finalize(block_number); + >::on_finalize(block_number); } /// Apply extrinsic outside of the block execution function. @@ -567,7 +553,9 @@ where // as well. frame_system::BlockHash::::insert(header.number(), header.hash()); - >::offchain_worker(*header.number()) + >::offchain_worker( + *header.number(), + ) } } @@ -849,7 +837,7 @@ mod tests { Block, ChainContext, Runtime, - AllPallets, + AllPalletsWithSystem, CustomOnRuntimeUpgrade, >; @@ -1360,23 +1348,19 @@ mod tests { )); // All weights that show up in the `initialize_block_impl` - let frame_system_upgrade_weight = frame_system::Pallet::::on_runtime_upgrade(); let custom_runtime_upgrade_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade(); - let runtime_upgrade_weight = ::on_runtime_upgrade(); - let frame_system_on_initialize_weight = - frame_system::Pallet::::on_initialize(block_number); + let runtime_upgrade_weight = + ::on_runtime_upgrade(); let on_initialize_weight = - >::on_initialize(block_number); + >::on_initialize(block_number); let base_block_weight = ::BlockWeights::get().base_block; // Weights are recorded correctly assert_eq!( frame_system::Pallet::::block_weight().total(), - frame_system_upgrade_weight + - custom_runtime_upgrade_weight + + custom_runtime_upgrade_weight + runtime_upgrade_weight + - frame_system_on_initialize_weight + on_initialize_weight + base_block_weight, ); }); diff --git a/frame/support/procedural/src/construct_runtime/mod.rs b/frame/support/procedural/src/construct_runtime/mod.rs index a5da775b9c9ea..b32ecf1bccd5c 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -304,35 +304,82 @@ fn decl_all_pallets<'a>( types.extend(type_decl); names.push(&pallet_declaration.name); } - // Make nested tuple structure like (((Babe, Consensus), Grandpa), ...) + + // Make nested tuple structure like: + // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` // But ignore the system pallet. - let all_pallets = names + let all_pallets_without_system = names .iter() .filter(|n| **n != SYSTEM_PALLET_NAME) + .rev() .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + // Make nested tuple structure like: + // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` let all_pallets_with_system = names .iter() + .rev() + .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + + // Make nested tuple structure like: + // `((LastPallet, (SecondLastPallet, ( ... , FirstPallet) ... ))))` + // But ignore the system pallet. + let all_pallets_without_system_reversed = names + .iter() + .filter(|n| **n != SYSTEM_PALLET_NAME) .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + // Make nested tuple structure like: + // `((LastPallet, (SecondLastPallet, ( ... , FirstPallet) ... ))))` + let all_pallets_with_system_reversed = names + .iter() + .fold(TokenStream2::default(), |combined, name| quote!((#name, #combined))); + + let system_pallet = match names.iter().find(|n| **n == SYSTEM_PALLET_NAME) { + Some(name) => name, + None => + return syn::Error::new( + proc_macro2::Span::call_site(), + "`System` pallet declaration is missing. \ + Please add this line: `System: frame_system::{Pallet, Call, Storage, Config, Event},`", + ) + .into_compile_error(), + }; + quote!( #types /// All pallets included in the runtime as a nested tuple of types. - /// Excludes the System pallet. - pub type AllPallets = ( #all_pallets ); + #[deprecated(note = "The type definition has changed from representing all pallets \ + excluding system, in reversed order to become the representation of all pallets \ + including system pallet in regular order. For this reason it is encouraged to use \ + explicitly one of `AllPalletsWithSystem`, `AllPalletsWithoutSystem`, \ + `AllPalletsWithSystemReversed`, `AllPalletsWithoutSystemReversed`. \ + Note that the type `frame_executive::Executive` expects one of `AllPalletsWithSystem` \ + , `AllPalletsWithSystemReversed`, `AllPalletsReversedWithSystemFirst`. More details in \ + https://github.com/paritytech/substrate/pull/10043")] + pub type AllPallets = AllPalletsWithSystem; + /// All pallets included in the runtime as a nested tuple of types. pub type AllPalletsWithSystem = ( #all_pallets_with_system ); - /// All modules included in the runtime as a nested tuple of types. + /// All pallets included in the runtime as a nested tuple of types. + /// Excludes the System pallet. + pub type AllPalletsWithoutSystem = ( #all_pallets_without_system ); + + /// All pallets included in the runtime as a nested tuple of types in reversed order. /// Excludes the System pallet. - #[deprecated(note = "use `AllPallets` instead")] - #[allow(dead_code)] - pub type AllModules = ( #all_pallets ); - /// All modules included in the runtime as a nested tuple of types. - #[deprecated(note = "use `AllPalletsWithSystem` instead")] - #[allow(dead_code)] - pub type AllModulesWithSystem = ( #all_pallets_with_system ); + pub type AllPalletsWithoutSystemReversed = ( #all_pallets_without_system_reversed ); + + /// All pallets included in the runtime as a nested tuple of types in reversed order. + pub type AllPalletsWithSystemReversed = ( #all_pallets_with_system_reversed ); + + /// All pallets included in the runtime as a nested tuple of types in reversed order. + /// With the system pallet first. + pub type AllPalletsReversedWithSystemFirst = ( + #system_pallet, + AllPalletsWithoutSystemReversed + ); ) } diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index dcf739af614b8..509e3217ddf96 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -453,9 +453,21 @@ pub mod pallet2 { pub struct Pallet(_); #[pallet::hooks] - impl Hooks> for Pallet where - T::AccountId: From + SomeAssociation1 + impl Hooks> for Pallet + where + T::AccountId: From + SomeAssociation1, { + fn on_initialize(_: BlockNumberFor) -> Weight { + Self::deposit_event(Event::Something(11)); + 0 + } + fn on_finalize(_: BlockNumberFor) { + Self::deposit_event(Event::Something(21)); + } + fn on_runtime_upgrade() -> Weight { + Self::deposit_event(Event::Something(31)); + 0 + } } #[pallet::call] @@ -469,6 +481,7 @@ pub mod pallet2 { CountedStorageMap; #[pallet::event] + #[pallet::generate_deposit(fn deposit_event)] pub enum Event { /// Something Something(u32), @@ -963,10 +976,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPallets::on_initialize(1), 10); - AllPallets::on_finalize(1); + assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 10); + AllPalletsWithoutSystem::on_finalize(1); - assert_eq!(AllPallets::on_runtime_upgrade(), 30); + assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 30); assert_eq!( frame_system::Pallet::::events()[0].event, @@ -974,10 +987,62 @@ fn pallet_hooks_expand() { ); assert_eq!( frame_system::Pallet::::events()[1].event, + Event::Example2(pallet2::Event::Something(11)), + ); + assert_eq!( + frame_system::Pallet::::events()[2].event, Event::Example(pallet::Event::Something(20)), ); + assert_eq!( + frame_system::Pallet::::events()[3].event, + Event::Example2(pallet2::Event::Something(21)), + ); + assert_eq!( + frame_system::Pallet::::events()[4].event, + Event::Example(pallet::Event::Something(30)), + ); + assert_eq!( + frame_system::Pallet::::events()[5].event, + Event::Example2(pallet2::Event::Something(31)), + ); + }) +} + +#[test] +fn all_pallets_type_reversed_order_is_correct() { + TestExternalities::default().execute_with(|| { + frame_system::Pallet::::set_block_number(1); + + #[allow(deprecated)] + { + assert_eq!(AllPalletsWithoutSystemReversed::on_initialize(1), 10); + AllPalletsWithoutSystemReversed::on_finalize(1); + + assert_eq!(AllPalletsWithoutSystemReversed::on_runtime_upgrade(), 30); + } + + assert_eq!( + frame_system::Pallet::::events()[0].event, + Event::Example2(pallet2::Event::Something(11)), + ); + assert_eq!( + frame_system::Pallet::::events()[1].event, + Event::Example(pallet::Event::Something(10)), + ); assert_eq!( frame_system::Pallet::::events()[2].event, + Event::Example2(pallet2::Event::Something(21)), + ); + assert_eq!( + frame_system::Pallet::::events()[3].event, + Event::Example(pallet::Event::Something(20)), + ); + assert_eq!( + frame_system::Pallet::::events()[4].event, + Event::Example2(pallet2::Event::Something(31)), + ); + assert_eq!( + frame_system::Pallet::::events()[5].event, Event::Example(pallet::Event::Something(30)), ); }) @@ -1499,3 +1564,48 @@ fn test_storage_info() { ], ); } + +#[test] +fn assert_type_all_pallets_reversed_with_system_first_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsReversedWithSystemFirst) {} + fn _b(t: (System, (Example4, (Example2, (Example,))))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_with_system_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithSystem) {} + fn _b(t: (System, (Example, (Example2, (Example4,))))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_without_system_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithoutSystem) {} + fn _b(t: (Example, (Example2, (Example4,)))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_with_system_reversed_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithSystemReversed) {} + fn _b(t: (Example4, (Example2, (Example, (System,))))) { + _a(t) + } +} + +#[test] +fn assert_type_all_pallets_without_system_reversed_is_correct() { + // Just ensure the 2 types are same. + fn _a(_t: AllPalletsWithoutSystemReversed) {} + fn _b(t: (Example4, (Example2, (Example,)))) { + _a(t) + } +} diff --git a/frame/support/test/tests/pallet_instance.rs b/frame/support/test/tests/pallet_instance.rs index c031ac9fe1bf5..de70b0e7e404e 100644 --- a/frame/support/test/tests/pallet_instance.rs +++ b/frame/support/test/tests/pallet_instance.rs @@ -551,35 +551,34 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPallets::on_initialize(1), 21); - AllPallets::on_finalize(1); + assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 21); + AllPalletsWithoutSystem::on_finalize(1); - assert_eq!(AllPallets::on_runtime_upgrade(), 61); + assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 61); - // The order is indeed reversed due to https://github.com/paritytech/substrate/issues/6280 assert_eq!( frame_system::Pallet::::events()[0].event, - Event::Instance1Example(pallet::Event::Something(11)), + Event::Example(pallet::Event::Something(10)), ); assert_eq!( frame_system::Pallet::::events()[1].event, - Event::Example(pallet::Event::Something(10)), + Event::Instance1Example(pallet::Event::Something(11)), ); assert_eq!( frame_system::Pallet::::events()[2].event, - Event::Instance1Example(pallet::Event::Something(21)), + Event::Example(pallet::Event::Something(20)), ); assert_eq!( frame_system::Pallet::::events()[3].event, - Event::Example(pallet::Event::Something(20)), + Event::Instance1Example(pallet::Event::Something(21)), ); assert_eq!( frame_system::Pallet::::events()[4].event, - Event::Instance1Example(pallet::Event::Something(31)), + Event::Example(pallet::Event::Something(30)), ); assert_eq!( frame_system::Pallet::::events()[5].event, - Event::Example(pallet::Event::Something(30)), + Event::Instance1Example(pallet::Event::Something(31)), ); }) }