Fix order of hook execution (#10043)

* fix order

* Update frame/support/procedural/src/construct_runtime/mod.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* 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 <alexander.popiak@parity.io>
This commit is contained in:
Guillaume Thiolliere
2021-12-01 09:59:09 +09:00
committed by GitHub
parent d58aeb040a
commit db28ba9dfd
6 changed files with 236 additions and 94 deletions
@@ -323,7 +323,7 @@ pub type Executive = frame_executive::Executive<
Block,
frame_system::ChainContext<Runtime>,
Runtime,
AllPallets,
AllPalletsWithSystem,
>;
impl_runtime_apis! {
+3 -1
View File
@@ -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>,
Runtime,
AllPallets,
AllPalletsWithSystem,
pallet_bags_list::migrations::CheckCounterPrefix<Runtime>,
>;
+44 -60
View File
@@ -59,7 +59,7 @@
//! # type Context = frame_system::ChainContext<Runtime>;
//! # pub type Block = generic::Block<Header, UncheckedExtrinsic>;
//! # 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<Runtime, Block, Context, Runtime, AllPallets>;
//! pub type Executive = executive::Executive<Runtime, Block, Context, Runtime, AllPalletsWithSystem>;
//! ```
//!
//! ### Custom `OnRuntimeUpgrade` logic
@@ -90,7 +90,7 @@
//! # type Context = frame_system::ChainContext<Runtime>;
//! # pub type Block = generic::Block<Header, UncheckedExtrinsic>;
//! # 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<Runtime, Block, Context, Runtime, AllPallets, CustomOnRuntimeUpgrade>;
//! pub type Executive = executive::Executive<Runtime, Block, Context, Runtime, AllPalletsWithSystem, CustomOnRuntimeUpgrade>;
//! ```
#![cfg_attr(not(feature = "std"), no_std)]
@@ -147,11 +147,26 @@ pub type OriginOf<E, C> = <CallOf<E, C> 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<System, Block, Context, UnsignedValidator, AllPallets, OnRuntimeUpgrade = ()>(
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<Header = System::Header, Hash = System::Hash>,
Context: Default,
UnsignedValidator,
AllPallets: OnRuntimeUpgrade
AllPalletsWithSystem: OnRuntimeUpgrade
+ OnInitialize<System::BlockNumber>
+ OnIdle<System::BlockNumber>
+ OnFinalize<System::BlockNumber>
+ OffchainWorker<System::BlockNumber>,
COnRuntimeUpgrade: OnRuntimeUpgrade,
> ExecuteBlock<Block>
for Executive<System, Block, Context, UnsignedValidator, AllPallets, COnRuntimeUpgrade>
for Executive<System, Block, Context, UnsignedValidator, AllPalletsWithSystem, COnRuntimeUpgrade>
where
Block::Extrinsic: Checkable<Context> + Codec,
CheckedOf<Block::Extrinsic, Context>: Applyable + GetDispatchInfo,
@@ -181,7 +196,7 @@ where
Block,
Context,
UnsignedValidator,
AllPallets,
AllPalletsWithSystem,
COnRuntimeUpgrade,
>::execute_block(block);
}
@@ -192,13 +207,13 @@ impl<
Block: traits::Block<Header = System::Header, Hash = System::Hash>,
Context: Default,
UnsignedValidator,
AllPallets: OnRuntimeUpgrade
AllPalletsWithSystem: OnRuntimeUpgrade
+ OnInitialize<System::BlockNumber>
+ OnIdle<System::BlockNumber>
+ OnFinalize<System::BlockNumber>
+ OffchainWorker<System::BlockNumber>,
COnRuntimeUpgrade: OnRuntimeUpgrade,
> Executive<System, Block, Context, UnsignedValidator, AllPallets, COnRuntimeUpgrade>
> Executive<System, Block, Context, UnsignedValidator, AllPalletsWithSystem, COnRuntimeUpgrade>
where
Block::Extrinsic: Checkable<Context> + Codec,
CheckedOf<Block::Extrinsic, Context>: 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(
<frame_system::Pallet<System> as OnRuntimeUpgrade>::on_runtime_upgrade(),
);
weight = weight.saturating_add(<AllPallets as OnRuntimeUpgrade>::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_support::weights::Weight, &'static str> {
<
(frame_system::Pallet::<System>, COnRuntimeUpgrade, AllPallets)
as
OnRuntimeUpgrade
>::pre_upgrade().unwrap();
<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let weight = Self::execute_on_runtime_upgrade();
<
(frame_system::Pallet::<System>, 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(<frame_system::Pallet<System> as OnInitialize<
weight = weight.saturating_add(<AllPalletsWithSystem as OnInitialize<
System::BlockNumber,
>>::on_initialize(*block_number));
weight = weight.saturating_add(
<AllPallets as OnInitialize<System::BlockNumber>>::on_initialize(*block_number),
);
weight = weight.saturating_add(
<System::BlockWeights as frame_support::traits::Get<_>>::get().base_block,
);
@@ -415,30 +411,20 @@ where
fn idle_and_finalize_hook(block_number: NumberFor<Block>) {
let weight = <frame_system::Pallet<System>>::block_weight();
let max_weight = <System::BlockWeights as frame_support::traits::Get<_>>::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 =
<frame_system::Pallet<System> as OnIdle<System::BlockNumber>>::on_idle(
block_number,
remaining_weight,
);
remaining_weight = remaining_weight.saturating_sub(used_weight);
used_weight = <AllPallets as OnIdle<System::BlockNumber>>::on_idle(
let used_weight = <AllPalletsWithSystem as OnIdle<System::BlockNumber>>::on_idle(
block_number,
remaining_weight,
)
.saturating_add(used_weight);
);
<frame_system::Pallet<System>>::register_extra_weight_unchecked(
used_weight,
DispatchClass::Mandatory,
);
}
<frame_system::Pallet<System> as OnFinalize<System::BlockNumber>>::on_finalize(
block_number,
);
<AllPallets as OnFinalize<System::BlockNumber>>::on_finalize(block_number);
<AllPalletsWithSystem as OnFinalize<System::BlockNumber>>::on_finalize(block_number);
}
/// Apply extrinsic outside of the block execution function.
@@ -567,7 +553,9 @@ where
// as well.
frame_system::BlockHash::<System>::insert(header.number(), header.hash());
<AllPallets as OffchainWorker<System::BlockNumber>>::offchain_worker(*header.number())
<AllPalletsWithSystem as OffchainWorker<System::BlockNumber>>::offchain_worker(
*header.number(),
)
}
}
@@ -849,7 +837,7 @@ mod tests {
Block<TestXt>,
ChainContext<Runtime>,
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::<Runtime>::on_runtime_upgrade();
let custom_runtime_upgrade_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade();
let runtime_upgrade_weight = <AllPallets as OnRuntimeUpgrade>::on_runtime_upgrade();
let frame_system_on_initialize_weight =
frame_system::Pallet::<Runtime>::on_initialize(block_number);
let runtime_upgrade_weight =
<AllPalletsWithSystem as OnRuntimeUpgrade>::on_runtime_upgrade();
let on_initialize_weight =
<AllPallets as OnInitialize<u64>>::on_initialize(block_number);
<AllPalletsWithSystem as OnInitialize<u64>>::on_initialize(block_number);
let base_block_weight =
<Runtime as frame_system::Config>::BlockWeights::get().base_block;
// Weights are recorded correctly
assert_eq!(
frame_system::Pallet::<Runtime>::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,
);
});
@@ -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)));
let all_pallets_with_system = names
// 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<T>},`",
)
.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.
#[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 AllPalletsWithoutSystem = ( #all_pallets_without_system );
/// All pallets included in the runtime as a nested tuple of types in reversed order.
/// 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
);
)
}
+116 -6
View File
@@ -453,9 +453,21 @@ pub mod pallet2 {
pub struct Pallet<T>(_);
#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> where
T::AccountId: From<SomeType1> + SomeAssociation1
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T>
where
T::AccountId: From<SomeType1> + SomeAssociation1,
{
fn on_initialize(_: BlockNumberFor<T>) -> Weight {
Self::deposit_event(Event::Something(11));
0
}
fn on_finalize(_: BlockNumberFor<T>) {
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<Hasher = Twox64Concat, Key = u8, Value = u32>;
#[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::<Runtime>::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::<Runtime>::events()[0].event,
@@ -974,10 +987,62 @@ fn pallet_hooks_expand() {
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[1].event,
Event::Example(pallet::Event::Something(20)),
Event::Example2(pallet2::Event::Something(11)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[2].event,
Event::Example(pallet::Event::Something(20)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[3].event,
Event::Example2(pallet2::Event::Something(21)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[4].event,
Event::Example(pallet::Event::Something(30)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::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::<Runtime>::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::<Runtime>::events()[0].event,
Event::Example2(pallet2::Event::Something(11)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[1].event,
Event::Example(pallet::Event::Something(10)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[2].event,
Event::Example2(pallet2::Event::Something(21)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[3].event,
Event::Example(pallet::Event::Something(20)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[4].event,
Event::Example2(pallet2::Event::Something(31)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::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)
}
}
@@ -551,35 +551,34 @@ fn pallet_hooks_expand() {
TestExternalities::default().execute_with(|| {
frame_system::Pallet::<Runtime>::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::<Runtime>::events()[0].event,
Event::Instance1Example(pallet::Event::Something(11)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[1].event,
Event::Example(pallet::Event::Something(10)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[2].event,
Event::Instance1Example(pallet::Event::Something(21)),
frame_system::Pallet::<Runtime>::events()[1].event,
Event::Instance1Example(pallet::Event::Something(11)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[3].event,
frame_system::Pallet::<Runtime>::events()[2].event,
Event::Example(pallet::Event::Something(20)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[3].event,
Event::Instance1Example(pallet::Event::Something(21)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[4].event,
Event::Instance1Example(pallet::Event::Something(31)),
Event::Example(pallet::Event::Something(30)),
);
assert_eq!(
frame_system::Pallet::<Runtime>::events()[5].event,
Event::Example(pallet::Event::Something(30)),
Event::Instance1Example(pallet::Event::Something(31)),
);
})
}