Fix order of hook execution

bin/node-template/runtime/src/lib.rs

@@ -323,7 +323,7 @@ pub type Executive = frame_executive::Executive<
 	Block,
 	frame_system::ChainContext<Runtime>,
 	Runtime,
-	AllPallets,
+	AllPalletsWithSystem,
 >;
 
 impl_runtime_apis! {

bin/node/runtime/src/lib.rs

@@ -1328,7 +1328,7 @@ pub type Executive = frame_executive::Executive<
 	Block,
 	frame_system::ChainContext<Runtime>,
 	Runtime,
-	AllPallets,
+	AllPalletsWithSystem,
 	(),
 >;
 

frame/executive/src/lib.rs

@@ -148,7 +148,8 @@ 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`.
+/// - `AllPallets`: 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 = ()>(
@@ -210,14 +211,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, AllPallets) as OnRuntimeUpgrade>::on_runtime_upgrade()
 	}
 
 	/// Execute given block, but don't do any of the [`final_checks`].
@@ -256,19 +250,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, AllPallets) as OnRuntimeUpgrade>::pre_upgrade().unwrap();
 		let weight = Self::execute_on_runtime_upgrade();
 
-		<
-			(frame_system::Pallet::<System>, COnRuntimeUpgrade, AllPallets)
-			as
-			OnRuntimeUpgrade
-		>::post_upgrade().unwrap();
+		<(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::post_upgrade().unwrap();
 
 		Ok(weight)
 	}
@@ -306,9 +291,6 @@ where
 			digest,
 			frame_system::InitKind::Full,
 		);
-		weight = weight.saturating_add(<frame_system::Pallet<System> as OnInitialize<
-			System::BlockNumber,
-		>>::on_initialize(*block_number));
 		weight = weight.saturating_add(
 			<AllPallets as OnInitialize<System::BlockNumber>>::on_initialize(*block_number),
 		);
@@ -416,29 +398,19 @@ 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 = <AllPallets 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);
 	}
 
@@ -850,7 +822,7 @@ mod tests {
 		Block<TestXt>,
 		ChainContext<Runtime>,
 		Runtime,
-		AllPallets,
+		AllPalletsWithSystem,
 		CustomOnRuntimeUpgrade,
 	>;
 
@@ -1361,23 +1333,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,
 			);
 		});

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

@@ -204,35 +204,64 @@ 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)));
 
 	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` \
+			and `AllPalletsWithSystemReversed. 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 );
 	)
 }
 

frame/support/test/tests/pallet.rs

@@ -452,9 +452,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]
@@ -468,6 +480,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),
@@ -939,21 +952,73 @@ 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,
 			Event::Example(pallet::Event::Something(10)),
 		);
 		assert_eq!(
 			frame_system::Pallet::<Runtime>::events()[1].event,
+			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)),
 		);
 	})

frame/support/test/tests/pallet_instance.rs

@@ -552,35 +552,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)),
+			Event::Example(pallet::Event::Something(10)),
 		);
 		assert_eq!(
 			frame_system::Pallet::<Runtime>::events()[1].event,
-			Event::Example(pallet::Event::Something(10)),
+			Event::Instance1Example(pallet::Event::Something(11)),
 		);
 		assert_eq!(
 			frame_system::Pallet::<Runtime>::events()[2].event,
-			Event::Instance1Example(pallet::Event::Something(21)),
+			Event::Example(pallet::Event::Something(20)),
 		);
 		assert_eq!(
 			frame_system::Pallet::<Runtime>::events()[3].event,
-			Event::Example(pallet::Event::Something(20)),
+			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)),
 		);
 	})
 }