From a50dbbf537e43e7ce4f9b475096c9912bad69373 Mon Sep 17 00:00:00 2001 From: Wilfried Kopp Date: Fri, 14 Jan 2022 18:01:13 +0100 Subject: [PATCH] Revert "Fix order of hook execution (#10043)" This reverts commit d36550a8da62fc968fac415c3379ad95d85105d1. --- 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, 89 insertions(+), 231 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index 4eaa0aa00d0b6..8ecb2199dda71 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, - AllPalletsWithSystem, + AllPallets, >; impl_runtime_apis! { diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 3f553221e67ac..44cefecd067f1 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1268,8 +1268,6 @@ 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, @@ -1347,7 +1345,7 @@ pub type Executive = frame_executive::Executive< Block, frame_system::ChainContext, Runtime, - AllPalletsWithSystem, + AllPallets, pallet_bags_list::migrations::CheckCounterPrefix, >; diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 9b81527fadb35..dd0a9abf8687b 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 AllPalletsWithSystem = u64; +//! # pub type AllPallets = 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 AllPalletsWithSystem = u64; +//! # pub type AllPallets = 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,26 +147,11 @@ 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. -/// - `AllPalletsWithSystem`: Tuple that contains all pallets including frame system pallet. Will be -/// used to call hooks e.g. `on_initialize`. +/// - `AllPallets`: Tuple that contains all modules. Will be used to call e.g. `on_initialize`. /// - `OnRuntimeUpgrade`: Custom logic that should be called after a runtime upgrade. Modules are -/// 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, - )>, +/// already called by `AllPallets`. It will be called before all modules will be called. +pub struct Executive( + PhantomData<(System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade)>, ); impl< @@ -174,14 +159,14 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPalletsWithSystem: OnRuntimeUpgrade + AllPallets: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, > ExecuteBlock - for Executive + for Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -196,7 +181,7 @@ where Block, Context, UnsignedValidator, - AllPalletsWithSystem, + AllPallets, COnRuntimeUpgrade, >::execute_block(block); } @@ -207,13 +192,13 @@ impl< Block: traits::Block
, Context: Default, UnsignedValidator, - AllPalletsWithSystem: OnRuntimeUpgrade + AllPallets: OnRuntimeUpgrade + OnInitialize + OnIdle + OnFinalize + OffchainWorker, COnRuntimeUpgrade: OnRuntimeUpgrade, - > Executive + > Executive where Block::Extrinsic: Checkable + Codec, CheckedOf: Applyable + GetDispatchInfo, @@ -224,7 +209,14 @@ where { /// Execute all `OnRuntimeUpgrade` of this runtime, and return the aggregate weight. pub fn execute_on_runtime_upgrade() -> frame_support::weights::Weight { - <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::on_runtime_upgrade() + 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 } /// Execute given block, but don't do any of the `final_checks`. @@ -263,10 +255,19 @@ where /// This should only be used for testing. #[cfg(feature = "try-runtime")] pub fn try_runtime_upgrade() -> Result { - <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::pre_upgrade().unwrap(); + < + (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) + as + OnRuntimeUpgrade + >::pre_upgrade().unwrap(); + let weight = Self::execute_on_runtime_upgrade(); - <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::post_upgrade().unwrap(); + < + (frame_system::Pallet::, COnRuntimeUpgrade, AllPallets) + as + OnRuntimeUpgrade + >::post_upgrade().unwrap(); Ok(weight) } @@ -304,9 +305,12 @@ where digest, frame_system::InitKind::Full, ); - weight = weight.saturating_add( as OnInitialize< System::BlockNumber, >>::on_initialize(*block_number)); + weight = weight.saturating_add( + >::on_initialize(*block_number), + ); weight = weight.saturating_add( >::get().base_block, ); @@ -411,20 +415,30 @@ where fn idle_and_finalize_hook(block_number: NumberFor) { let weight = >::block_weight(); let max_weight = >::get().max_block; - let remaining_weight = max_weight.saturating_sub(weight.total()); + let mut remaining_weight = max_weight.saturating_sub(weight.total()); if remaining_weight > 0 { - let used_weight = >::on_idle( + let mut used_weight = + as OnIdle>::on_idle( + block_number, + remaining_weight, + ); + remaining_weight = remaining_weight.saturating_sub(used_weight); + used_weight = >::on_idle( block_number, remaining_weight, - ); + ) + .saturating_add(used_weight); >::register_extra_weight_unchecked( used_weight, DispatchClass::Mandatory, ); } - >::on_finalize(block_number); + as OnFinalize>::on_finalize( + block_number, + ); + >::on_finalize(block_number); } /// Apply extrinsic outside of the block execution function. @@ -553,9 +567,7 @@ where // as well. frame_system::BlockHash::::insert(header.number(), header.hash()); - >::offchain_worker( - *header.number(), - ) + >::offchain_worker(*header.number()) } } @@ -837,7 +849,7 @@ mod tests { Block, ChainContext, Runtime, - AllPalletsWithSystem, + AllPallets, CustomOnRuntimeUpgrade, >; @@ -1348,19 +1360,23 @@ 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 runtime_upgrade_weight = ::on_runtime_upgrade(); + let frame_system_on_initialize_weight = + frame_system::Pallet::::on_initialize(block_number); 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(), - custom_runtime_upgrade_weight + + frame_system_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 b32ecf1bccd5c..a5da775b9c9ea 100644 --- a/frame/support/procedural/src/construct_runtime/mod.rs +++ b/frame/support/procedural/src/construct_runtime/mod.rs @@ -304,82 +304,35 @@ fn decl_all_pallets<'a>( types.extend(type_decl); names.push(&pallet_declaration.name); } - - // Make nested tuple structure like: - // `((FirstPallet, (SecondPallet, ( ... , LastPallet) ... ))))` + // Make nested tuple structure like (((Babe, Consensus), Grandpa), ...) // But ignore the system pallet. - let all_pallets_without_system = names + let all_pallets = 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. - #[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; - + /// Excludes the System pallet. + pub type AllPallets = ( #all_pallets ); /// All pallets included in the runtime as a nested tuple of types. pub type AllPalletsWithSystem = ( #all_pallets_with_system ); - /// 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. + /// All modules included in the runtime as a nested tuple of types. /// Excludes the System pallet. - 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 - ); + #[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 ); ) } diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 509e3217ddf96..dcf739af614b8 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -453,21 +453,9 @@ 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] @@ -481,7 +469,6 @@ pub mod pallet2 { CountedStorageMap; #[pallet::event] - #[pallet::generate_deposit(fn deposit_event)] pub enum Event { /// Something Something(u32), @@ -976,10 +963,10 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 10); - AllPalletsWithoutSystem::on_finalize(1); + assert_eq!(AllPallets::on_initialize(1), 10); + AllPallets::on_finalize(1); - assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 30); + assert_eq!(AllPallets::on_runtime_upgrade(), 30); assert_eq!( frame_system::Pallet::::events()[0].event, @@ -987,62 +974,10 @@ 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)), ); }) @@ -1564,48 +1499,3 @@ 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 de70b0e7e404e..c031ac9fe1bf5 100644 --- a/frame/support/test/tests/pallet_instance.rs +++ b/frame/support/test/tests/pallet_instance.rs @@ -551,34 +551,35 @@ fn pallet_hooks_expand() { TestExternalities::default().execute_with(|| { frame_system::Pallet::::set_block_number(1); - assert_eq!(AllPalletsWithoutSystem::on_initialize(1), 21); - AllPalletsWithoutSystem::on_finalize(1); + assert_eq!(AllPallets::on_initialize(1), 21); + AllPallets::on_finalize(1); - assert_eq!(AllPalletsWithoutSystem::on_runtime_upgrade(), 61); + assert_eq!(AllPallets::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::Example(pallet::Event::Something(10)), + Event::Instance1Example(pallet::Event::Something(11)), ); assert_eq!( frame_system::Pallet::::events()[1].event, - Event::Instance1Example(pallet::Event::Something(11)), + Event::Example(pallet::Event::Something(10)), ); assert_eq!( frame_system::Pallet::::events()[2].event, - Event::Example(pallet::Event::Something(20)), + Event::Instance1Example(pallet::Event::Something(21)), ); assert_eq!( frame_system::Pallet::::events()[3].event, - Event::Instance1Example(pallet::Event::Something(21)), + Event::Example(pallet::Event::Something(20)), ); assert_eq!( frame_system::Pallet::::events()[4].event, - Event::Example(pallet::Event::Something(30)), + Event::Instance1Example(pallet::Event::Something(31)), ); assert_eq!( frame_system::Pallet::::events()[5].event, - Event::Instance1Example(pallet::Event::Something(31)), + Event::Example(pallet::Event::Something(30)), ); }) }