diff --git a/substrate/frame/executive/src/lib.rs b/substrate/frame/executive/src/lib.rs index 3028eaf318e0..225b8ce8a871 100644 --- a/substrate/frame/executive/src/lib.rs +++ b/substrate/frame/executive/src/lib.rs @@ -346,7 +346,7 @@ where return Err(InvalidTransaction::BadMandatory.into()) } - >::note_applied_extrinsic(&r, dispatch_info); + >::note_applied_extrinsic(&r, dispatch_info, is_inherent); Ok(r.map(|_| ()).map_err(|e| e.error)) }; @@ -601,8 +601,8 @@ where DispatchClass::Mandatory, ); - frame_system::Pallet::::note_finished_initialize(); ::PreInherents::pre_inherents(); + frame_system::Pallet::::note_finished_initialize(); } /// Returns if the runtime has been upgraded, based on [`frame_system::LastRuntimeUpgrade`]. @@ -672,6 +672,7 @@ where pub fn inherents_applied() { >::note_inherents_applied(); ::PostInherents::post_inherents(); + >::note_post_inherents_applied(); if ::MultiBlockMigrator::ongoing() { let used_weight = ::MultiBlockMigrator::step(); @@ -806,7 +807,7 @@ where return Err(InvalidTransaction::BadMandatory.into()) } - >::note_applied_extrinsic(&r, dispatch_info); + >::note_applied_extrinsic(&r, dispatch_info, is_inherent); Ok(r.map(|_| ()).map_err(|e| e.error)) } diff --git a/substrate/frame/executive/src/tests.rs b/substrate/frame/executive/src/tests.rs index a3f70a9fc3c2..aa5a38e96a71 100644 --- a/substrate/frame/executive/src/tests.rs +++ b/substrate/frame/executive/src/tests.rs @@ -38,12 +38,19 @@ use frame_support::{ traits::{fungible, ConstU8, Currency, IsInherent}, weights::{ConstantMultiplier, IdentityFee, RuntimeDbWeight, Weight, WeightMeter, WeightToFee}, }; -use frame_system::{pallet_prelude::*, ChainContext, LastRuntimeUpgrade, LastRuntimeUpgradeInfo}; +use frame_system::{ + pallet_prelude::*, ChainContext, LastRuntimeUpgrade, LastRuntimeUpgradeInfo, Phase, +}; use pallet_balances::Call as BalancesCall; use pallet_transaction_payment::CurrencyAdapter; const TEST_KEY: &[u8] = b":test:key:"; +fn assert_execution_phase(want: &Phase) { + let got = frame_system::ExecutionPhase::::get().unwrap(); + assert_eq!(want, &got, "Wrong execution phase"); +} + #[frame_support::pallet(dev_mode)] mod custom { use super::*; @@ -181,6 +188,7 @@ mod custom2 { !MockedSystemCallbacks::pre_inherent_called(), "Pre inherent hook goes after on_initialize" ); + assert_execution_phase::(&Phase::Initialization); Weight::from_parts(0, 0) } @@ -188,20 +196,28 @@ mod custom2 { fn on_idle(_: BlockNumberFor, _: Weight) -> Weight { assert!( MockedSystemCallbacks::post_transactions_called(), - "Post transactions hook goes before after on_idle" + "Post transactions hook goes before on_idle" ); + assert_execution_phase::(&Phase::Finalization); + Weight::from_parts(0, 0) } fn on_finalize(_: BlockNumberFor) { assert!( MockedSystemCallbacks::post_transactions_called(), - "Post transactions hook goes before after on_finalize" + "Post transactions hook goes before on_finalize" ); + assert_execution_phase::(&Phase::Finalization); } fn on_runtime_upgrade() -> Weight { sp_io::storage::set(super::TEST_KEY, "module".as_bytes()); + assert!( + !frame_system::ExecutionPhase::::exists(), + "Runtime upgrades do not have a phase" + ); + Weight::from_parts(0, 0) } } @@ -218,6 +234,17 @@ mod custom2 { assert!(!MockedSystemCallbacks::post_transactions_called()); assert!(System::inherents_applied()); + assert!(matches!( + frame_system::ExecutionPhase::::get(), + Some(frame_system::Phase::ApplyExtrinsic(_) | frame_system::Phase::AfterInherent) + )); + + Ok(()) + } + + pub fn assert_extrinsic_phase(_: OriginFor, expected: u32) -> DispatchResult { + assert_execution_phase::(&Phase::ApplyExtrinsic(expected)); + Ok(()) } @@ -242,6 +269,19 @@ mod custom2 { Ok(()) } + + #[pallet::weight((0, DispatchClass::Mandatory))] + pub fn assert_inherent_phase(_: OriginFor, expected: u32) -> DispatchResult { + assert_execution_phase::(&Phase::ApplyInherent(expected)); + + Ok(()) + } + + pub fn assert_optional_inherent_phase(_: OriginFor, expected: u32) -> DispatchResult { + assert_execution_phase::(&Phase::ApplyInherent(expected)); + + Ok(()) + } } #[pallet::inherent] @@ -257,7 +297,13 @@ mod custom2 { } fn is_inherent(call: &Self::Call) -> bool { - matches!(call, Call::::inherent {} | Call::::optional_inherent {}) + matches!( + call, + Call::::inherent {} | + Call::::optional_inherent {} | + Call::::assert_inherent_phase { .. } | + Call::::assert_optional_inherent_phase { .. } + ) } } @@ -270,6 +316,8 @@ mod custom2 { match call { Call::allowed_unsigned { .. } | Call::optional_inherent { .. } | + Call::assert_inherent_phase { .. } | + Call::assert_optional_inherent_phase { .. } | Call::inherent { .. } => Ok(()), _ => Err(UnknownTransaction::NoUnsignedValidator.into()), } @@ -465,6 +513,7 @@ impl PreInherents for MockedSystemCallbacks { SystemCallbacksCalled::set(1); // Change the storage to modify the root hash: frame_support::storage::unhashed::put(b":pre_inherent", b"0"); + assert_eq!(frame_system::ExecutionPhase::::get(), Some(Phase::Initialization)); } } @@ -474,6 +523,7 @@ impl PostInherents for MockedSystemCallbacks { SystemCallbacksCalled::set(2); // Change the storage to modify the root hash: frame_support::storage::unhashed::put(b":post_inherent", b"0"); + assert_execution_phase::(&Phase::AfterInherent); } } @@ -483,6 +533,7 @@ impl PostTransactions for MockedSystemCallbacks { SystemCallbacksCalled::set(3); // Change the storage to modify the root hash: frame_support::storage::unhashed::put(b":post_transaction", b"0"); + assert_execution_phase::(&Phase::Finalization); } } @@ -990,6 +1041,7 @@ fn all_weights_are_recorded_correctly() { let block_number = 1; + frame_system::ExecutionPhase::::kill(); Executive::initialize_block(&Header::new_from_number(block_number)); // Reset the last runtime upgrade info, to make the second call to `on_runtime_upgrade` @@ -998,20 +1050,21 @@ fn all_weights_are_recorded_correctly() { MockedSystemCallbacks::reset(); // All weights that show up in the `initialize_block_impl` - let custom_runtime_upgrade_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade(); - let runtime_upgrade_weight = - ::on_runtime_upgrade(); - let on_initialize_weight = - >::on_initialize(block_number); + frame_system::ExecutionPhase::::kill(); + let custom_ra_weight = CustomOnRuntimeUpgrade::on_runtime_upgrade(); + + frame_system::ExecutionPhase::::kill(); + let ra_weight = ::on_runtime_upgrade(); + + frame_system::ExecutionPhase::::put(Phase::Initialization); + let init_weight = >::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 + - runtime_upgrade_weight + - on_initialize_weight + - base_block_weight, + custom_ra_weight + ra_weight + init_weight + base_block_weight, ); }); } @@ -1318,10 +1371,11 @@ fn callbacks_in_block_execution_works() { callbacks_in_block_execution_works_inner(true); } +/// Produces a block with `0..15` inherents and `0..15` transactions and runs tests on that. fn callbacks_in_block_execution_works_inner(mbms_active: bool) { MbmActive::set(mbms_active); - for (n_in, n_tx) in (0..10usize).zip(0..10usize) { + for (n_in, n_tx) in (0..15usize).zip(0..15usize) { let mut extrinsics = Vec::new(); let header = new_test_ext(10).execute_with(|| { @@ -1331,9 +1385,13 @@ fn callbacks_in_block_execution_works_inner(mbms_active: bool) { for i in 0..n_in { let xt = if i % 2 == 0 { - UncheckedXt::new_bare(RuntimeCall::Custom(custom::Call::inherent {})) + UncheckedXt::new_bare(RuntimeCall::Custom2( + custom2::Call::assert_inherent_phase { expected: i as u32 }, + )) } else { - UncheckedXt::new_bare(RuntimeCall::Custom2(custom2::Call::optional_inherent {})) + UncheckedXt::new_bare(RuntimeCall::Custom2( + custom2::Call::assert_optional_inherent_phase { expected: i as u32 }, + )) }; Executive::apply_extrinsic(xt.clone()).unwrap().unwrap(); extrinsics.push(xt); @@ -1341,10 +1399,12 @@ fn callbacks_in_block_execution_works_inner(mbms_active: bool) { for t in 0..n_tx { let xt = UncheckedXt::new_signed( - RuntimeCall::Custom2(custom2::Call::some_call {}), + RuntimeCall::Custom2(custom2::Call::assert_extrinsic_phase { + expected: extrinsics.len() as u32, + }), 1, 1.into(), - tx_ext(t as u64, 0), + tx_ext(t as u64, 1), ); // Extrinsics can be applied even when MBMs are active. Only the `execute_block` // will reject it. @@ -1354,11 +1414,12 @@ fn callbacks_in_block_execution_works_inner(mbms_active: bool) { Executive::finalize_block() }); - assert_eq!(SystemCallbacksCalled::get(), 3); + assert!(MockedSystemCallbacks::post_inherent_called()); new_test_ext(10).execute_with(|| { + MockedSystemCallbacks::reset(); let header = std::panic::catch_unwind(|| { - Executive::execute_block(Block::new(header, extrinsics)); + Executive::execute_block(Block::new(header.clone(), extrinsics.clone())); }); match header { @@ -1474,3 +1535,61 @@ fn is_inherent_works() { let ext = UncheckedXt::new_bare(RuntimeCall::Custom2(custom2::Call::allowed_unsigned {})); assert!(!Runtime::is_inherent(&ext), "Unsigned ext are not automatically inherents"); } + +#[test] +fn extrinsic_index_is_correct() { + let in1 = UncheckedXt::new_bare(RuntimeCall::Custom2(custom2::Call::assert_inherent_phase { + expected: 0, + })); + let in2 = UncheckedXt::new_bare(RuntimeCall::Custom2(custom2::Call::assert_inherent_phase { + expected: 1, + })); + let xt1 = UncheckedXt::new_signed( + RuntimeCall::Custom2(custom2::Call::assert_extrinsic_phase { expected: 2 }), + 1, + 1.into(), + tx_ext(0, 1), + ); + let xt2 = UncheckedXt::new_signed( + RuntimeCall::Custom2(custom2::Call::assert_extrinsic_phase { expected: 3 }), + 1, + 1.into(), + tx_ext(1, 1), + ); + let xt3 = UncheckedXt::new_signed( + RuntimeCall::Custom2(custom2::Call::assert_extrinsic_phase { expected: 4 }), + 1, + 1.into(), + tx_ext(2, 1), + ); + + let header = new_test_ext(1).execute_with(|| { + Executive::initialize_block(&Header::new_from_number(1)); + + Executive::apply_extrinsic(in1.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(in2.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(xt1.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(xt2.clone()).unwrap().unwrap(); + Executive::apply_extrinsic(xt3.clone()).unwrap().unwrap(); + + Executive::finalize_block() + }); + + new_test_ext(1).execute_with(|| { + Executive::execute_block(Block::new( + header.clone(), + vec![in1.clone(), in2.clone(), xt1.clone(), xt2.clone(), xt3.clone()], + )); + }); + + #[cfg(feature = "try-runtime")] + new_test_ext(1).execute_with(|| { + Executive::try_execute_block( + Block::new(header, vec![in1, in2, xt1, xt2, xt3]), + true, + true, + frame_try_runtime::TryStateSelect::All, + ) + .unwrap(); + }); +} diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 0318f77342e0..43a712431abf 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -997,7 +997,7 @@ pub mod pallet { /// The execution phase of the block. #[pallet::storage] #[pallet::whitelist_storage] - pub(super) type ExecutionPhase = StorageValue<_, Phase>; + pub type ExecutionPhase = StorageValue<_, Phase>; /// `Some` if a code upgrade has been authorized. #[pallet::storage] @@ -1052,12 +1052,18 @@ pub type KeyValue = (Vec, Vec); #[derive(Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)] #[cfg_attr(feature = "std", derive(Serialize, PartialEq, Eq, Clone))] pub enum Phase { - /// Applying an extrinsic. + /// Applying a non-inherent extrinsic. ApplyExtrinsic(u32), /// Finalizing the block. Finalization, /// Initializing the block. Initialization, + /// Applying the inherent with this index. + ApplyInherent(u32), + /// All inherents have been applied. + /// + /// This is set before any transaction is applied. + AfterInherent, } impl Default for Phase { @@ -1443,6 +1449,12 @@ impl Pallet { /// once per block. pub fn note_inherents_applied() { InherentsApplied::::put(true); + ExecutionPhase::::put(Phase::AfterInherent); + } + + pub fn note_post_inherents_applied() { + debug_assert!(matches!(ExecutionPhase::::get(), Some(Phase::AfterInherent))); + ExecutionPhase::::put(Phase::ApplyExtrinsic(Self::extrinsic_index().unwrap_or(0))); } /// Increment the reference counter on an account. @@ -2002,7 +2014,11 @@ impl Pallet { /// Emits an `ExtrinsicSuccess` or `ExtrinsicFailed` event depending on the outcome. /// The emitted event contains the post-dispatch corrected weight including /// the base-weight for its dispatch class. - pub fn note_applied_extrinsic(r: &DispatchResultWithPostInfo, mut info: DispatchInfo) { + pub fn note_applied_extrinsic( + r: &DispatchResultWithPostInfo, + mut info: DispatchInfo, + inherent: bool, + ) { info.weight = extract_actual_weight(r, &info) .saturating_add(T::BlockWeights::get().get(info.class).base_extrinsic); info.pays_fee = extract_actual_pays_fee(r, &info); @@ -2023,7 +2039,12 @@ impl Pallet { let next_extrinsic_index = Self::extrinsic_index().unwrap_or_default() + 1u32; storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &next_extrinsic_index); - ExecutionPhase::::put(Phase::ApplyExtrinsic(next_extrinsic_index)); + + if inherent { + ExecutionPhase::::put(Phase::ApplyInherent(next_extrinsic_index)); + } else { + ExecutionPhase::::put(Phase::ApplyExtrinsic(next_extrinsic_index)); + } } /// To be called immediately after `note_applied_extrinsic` of the last extrinsic of the block @@ -2035,10 +2056,11 @@ impl Pallet { ExecutionPhase::::put(Phase::Finalization); } - /// To be called immediately after finishing the initialization of the block - /// (e.g., called `on_initialize` for all pallets). + /// To be called immediately after finishing the initialization of the block. + /// + /// This includes runtime upgrades, `on_initialize` of all pallets and pre-inherent hooks. pub fn note_finished_initialize() { - ExecutionPhase::::put(Phase::ApplyExtrinsic(0)) + ExecutionPhase::::put(Phase::ApplyInherent(0)) } /// An account is being created.