From 65a2ad4b3a9ef8a294a7dd109e6321e1e54ebc2b Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 10 Jan 2022 10:18:52 +1300 Subject: [PATCH 1/6] reset events before apply runtime upgrade --- frame/aura/src/tests.rs | 4 +-- frame/babe/src/mock.rs | 7 ++--- frame/executive/src/lib.rs | 8 +++--- frame/merkle-mountain-range/src/tests.rs | 2 +- frame/randomness-collective-flip/src/lib.rs | 3 ++- frame/system/src/lib.rs | 30 --------------------- frame/system/src/tests.rs | 21 ++++++++++----- 7 files changed, 28 insertions(+), 47 deletions(-) diff --git a/frame/aura/src/tests.rs b/frame/aura/src/tests.rs index 30003632729ea..ce09f85678c00 100644 --- a/frame/aura/src/tests.rs +++ b/frame/aura/src/tests.rs @@ -22,7 +22,6 @@ use crate::mock::{new_test_ext, Aura, MockDisabledValidators, System}; use codec::Encode; use frame_support::traits::OnInitialize; -use frame_system::InitKind; use sp_consensus_aura::{Slot, AURA_ENGINE_ID}; use sp_runtime::{Digest, DigestItem}; @@ -45,7 +44,8 @@ fn disabled_validators_cannot_author_blocks() { let pre_digest = Digest { logs: vec![DigestItem::PreRuntime(AURA_ENGINE_ID, slot.encode())] }; - System::initialize(&42, &System::parent_hash(), &pre_digest, InitKind::Full); + System::reset_events(); + System::initialize(&42, &System::parent_hash(), &pre_digest); // let's disable the validator MockDisabledValidators::disable_validator(1); diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index f0faa55e5b333..22d7befab6686 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -24,7 +24,6 @@ use frame_support::{ parameter_types, traits::{ConstU128, ConstU32, ConstU64, GenesisBuild, KeyOwnerProofSystem, OnInitialize}, }; -use frame_system::InitKind; use pallet_session::historical as pallet_session_historical; use pallet_staking::EraIndex; use sp_consensus_babe::{AuthorityId, AuthorityPair, Slot}; @@ -255,7 +254,8 @@ pub fn go_to_block(n: u64, s: u64) { let pre_digest = make_secondary_plain_pre_digest(0, s.into()); - System::initialize(&n, &parent_hash, &pre_digest, InitKind::Full); + System::reset_events(); + System::initialize(&n, &parent_hash, &pre_digest); Babe::on_initialize(n); Session::on_initialize(n); @@ -421,7 +421,8 @@ pub fn generate_equivocation_proof( let make_header = || { let parent_hash = System::parent_hash(); let pre_digest = make_secondary_plain_pre_digest(offender_authority_index, slot); - System::initialize(¤t_block, &parent_hash, &pre_digest, InitKind::Full); + System::reset_events(); + System::initialize(¤t_block, &parent_hash, &pre_digest); System::set_block_number(current_block); Timestamp::set_timestamp(current_block); System::finalize() diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index be944954eaa59..e2231ae609a58 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -294,6 +294,11 @@ where parent_hash: &System::Hash, digest: &Digest, ) { + // Reset events before apply runtime upgrade hook. + // This is required to preserve events from runtime upgrade hook. + // This means the format of all the event related storages must always be compatible. + >::reset_events(); + let mut weight = 0; if Self::runtime_upgraded() { weight = weight.saturating_add(Self::execute_on_runtime_upgrade()); @@ -302,7 +307,6 @@ where block_number, parent_hash, digest, - frame_system::InitKind::Full, ); weight = weight.saturating_add(::block_number() + One::one()), &block_hash, &Default::default(), - frame_system::InitKind::Inspection, ); enter_span! { sp_tracing::Level::TRACE, "validate_transaction" }; @@ -545,7 +548,6 @@ where header.number(), header.parent_hash(), &digests, - frame_system::InitKind::Inspection, ); // Frame system only inserts the parent hash into the block hashes as normally we don't know diff --git a/frame/merkle-mountain-range/src/tests.rs b/frame/merkle-mountain-range/src/tests.rs index 588a407d6d371..b82378d6efa90 100644 --- a/frame/merkle-mountain-range/src/tests.rs +++ b/frame/merkle-mountain-range/src/tests.rs @@ -40,11 +40,11 @@ fn new_block() -> u64 { let hash = H256::repeat_byte(number as u8); LEAF_DATA.with(|r| r.borrow_mut().a = number); + frame_system::Pallet::::reset_events(); frame_system::Pallet::::initialize( &number, &hash, &Default::default(), - frame_system::InitKind::Full, ); MMR::on_initialize(number) } diff --git a/frame/randomness-collective-flip/src/lib.rs b/frame/randomness-collective-flip/src/lib.rs index 5f4994c471424..fbc8aaaa7ec59 100644 --- a/frame/randomness-collective-flip/src/lib.rs +++ b/frame/randomness-collective-flip/src/lib.rs @@ -239,7 +239,8 @@ mod tests { let mut parent_hash = System::parent_hash(); for i in 1..(blocks + 1) { - System::initialize(&i, &parent_hash, &Default::default(), frame_system::InitKind::Full); + System::reset_events(); + System::initialize(&i, &parent_hash, &Default::default()); CollectiveFlip::on_initialize(i); let header = System::finalize(); diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index cec45a8aa1cb1..0167f7aefdef8 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -944,27 +944,6 @@ where } } -/// A type of block initialization to perform. -pub enum InitKind { - /// Leave inspectable storage entries in state. - /// - /// i.e. `Events` are not being reset. - /// Should only be used for off-chain calls, - /// regular block execution should clear those. - Inspection, - - /// Reset also inspectable storage entries. - /// - /// This should be used for regular block execution. - Full, -} - -impl Default for InitKind { - fn default() -> Self { - InitKind::Full - } -} - /// Reference status; can be either referenced or unreferenced. #[derive(RuntimeDebug)] pub enum RefStatus { @@ -1306,7 +1285,6 @@ impl Pallet { number: &T::BlockNumber, parent_hash: &T::Hash, digest: &generic::Digest, - kind: InitKind, ) { // populate environment ExecutionPhase::::put(Phase::Initialization); @@ -1318,13 +1296,6 @@ impl Pallet { // Remove previous block data from storage BlockWeight::::kill(); - - // Kill inspectable storage entries in state when `InitKind::Full`. - if let InitKind::Full = kind { - >::kill(); - EventCount::::kill(); - >::remove_all(None); - } } /// Remove temporary "environment" entries in storage, compute the storage root and return the @@ -1446,7 +1417,6 @@ impl Pallet { /// Reset events. Can be used as an alternative to /// `initialize` for tests that don't need to bother with the other environment entries. - #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] pub fn reset_events() { >::kill(); EventCount::::kill(); diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs index 411925c70275f..0facd796b2a0c 100644 --- a/frame/system/src/tests.rs +++ b/frame/system/src/tests.rs @@ -154,7 +154,8 @@ fn provider_required_to_support_consumer() { #[test] fn deposit_event_should_work() { new_test_ext().execute_with(|| { - System::initialize(&1, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&1, &[0u8; 32].into(), &Default::default()); System::note_finished_extrinsics(); System::deposit_event(SysEvent::CodeUpdated); System::finalize(); @@ -167,7 +168,8 @@ fn deposit_event_should_work() { }] ); - System::initialize(&2, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&2, &[0u8; 32].into(), &Default::default()); System::deposit_event(SysEvent::NewAccount { account: 32 }); System::note_finished_initialize(); System::deposit_event(SysEvent::KilledAccount { account: 42 }); @@ -216,7 +218,8 @@ fn deposit_event_should_work() { #[test] fn deposit_event_uses_actual_weight() { new_test_ext().execute_with(|| { - System::initialize(&1, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&1, &[0u8; 32].into(), &Default::default()); System::note_finished_initialize(); let pre_info = DispatchInfo { weight: 1000, ..Default::default() }; @@ -275,7 +278,8 @@ fn deposit_event_topics() { new_test_ext().execute_with(|| { const BLOCK_NUMBER: u64 = 1; - System::initialize(&BLOCK_NUMBER, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&BLOCK_NUMBER, &[0u8; 32].into(), &Default::default()); System::note_finished_extrinsics(); let topics = vec![H256::repeat_byte(1), H256::repeat_byte(2), H256::repeat_byte(3)]; @@ -333,7 +337,8 @@ fn prunes_block_hash_mappings() { new_test_ext().execute_with(|| { // simulate import of 15 blocks for n in 1..=15 { - System::initialize(&n, &[n as u8 - 1; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&n, &[n as u8 - 1; 32].into(), &Default::default()); System::finalize(); } @@ -464,7 +469,8 @@ fn events_not_emitted_during_genesis() { #[test] fn extrinsics_root_is_calculated_correctly() { new_test_ext().execute_with(|| { - System::initialize(&1, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&1, &[0u8; 32].into(), &Default::default()); System::note_finished_initialize(); System::note_extrinsic(vec![1]); System::note_applied_extrinsic(&Ok(().into()), Default::default()); @@ -481,7 +487,8 @@ fn extrinsics_root_is_calculated_correctly() { #[test] fn runtime_updated_digest_emitted_when_heap_pages_changed() { new_test_ext().execute_with(|| { - System::initialize(&1, &[0u8; 32].into(), &Default::default(), InitKind::Full); + System::reset_events(); + System::initialize(&1, &[0u8; 32].into(), &Default::default()); System::set_heap_pages(RawOrigin::Root.into(), 5).unwrap(); assert_runtime_updated_digest(1); }); From 7991703c6f69a824794a55b5af952d1039a982cc Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 10 Jan 2022 22:40:38 +1300 Subject: [PATCH 2/6] fix tests --- frame/authorship/src/lib.rs | 6 ++++-- frame/babe/src/tests.rs | 17 ++++++++--------- frame/contracts/src/tests.rs | 3 ++- frame/executive/src/lib.rs | 12 ++---------- frame/grandpa/src/mock.rs | 6 ++++-- frame/merkle-mountain-range/src/tests.rs | 6 +----- frame/system/src/lib.rs | 6 +----- 7 files changed, 22 insertions(+), 34 deletions(-) diff --git a/frame/authorship/src/lib.rs b/frame/authorship/src/lib.rs index db453fc57e9e6..3fa081f5263e3 100644 --- a/frame/authorship/src/lib.rs +++ b/frame/authorship/src/lib.rs @@ -596,7 +596,8 @@ mod tests { }; let initialize_block = |number, hash: H256| { - System::initialize(&number, &hash, &Default::default(), Default::default()) + System::reset_events(); + System::initialize(&number, &hash, &Default::default()) }; for number in 1..8 { @@ -689,7 +690,8 @@ mod tests { seal_header(create_header(1, Default::default(), [1; 32].into()), author); header.digest_mut().pop(); // pop the seal off. - System::initialize(&1, &Default::default(), header.digest(), Default::default()); + System::reset_events(); + System::initialize(&1, &Default::default(), header.digest()); assert_eq!(Authorship::author(), Some(author)); }); diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs index 89911efe4e8af..65c9de85586e4 100644 --- a/frame/babe/src/tests.rs +++ b/frame/babe/src/tests.rs @@ -67,7 +67,8 @@ fn first_block_epoch_zero_start() { let pre_digest = make_primary_pre_digest(0, genesis_slot, first_vrf.clone(), vrf_proof); assert_eq!(Babe::genesis_slot(), Slot::from(0)); - System::initialize(&1, &Default::default(), &pre_digest, Default::default()); + System::reset_events(); + System::initialize(&1, &Default::default(), &pre_digest); // see implementation of the function for details why: we issue an // epoch-change digest but don't do it via the normal session mechanism. @@ -112,7 +113,8 @@ fn author_vrf_output_for_primary() { let (vrf_output, vrf_proof, vrf_randomness) = make_vrf_output(genesis_slot, &pairs[0]); let primary_pre_digest = make_primary_pre_digest(0, genesis_slot, vrf_output, vrf_proof); - System::initialize(&1, &Default::default(), &primary_pre_digest, Default::default()); + System::reset_events(); + System::initialize(&1, &Default::default(), &primary_pre_digest); Babe::do_initialize(1); assert_eq!(Babe::author_vrf_randomness(), Some(vrf_randomness)); @@ -133,7 +135,8 @@ fn author_vrf_output_for_secondary_vrf() { let secondary_vrf_pre_digest = make_secondary_vrf_pre_digest(0, genesis_slot, vrf_output, vrf_proof); - System::initialize(&1, &Default::default(), &secondary_vrf_pre_digest, Default::default()); + System::reset_events(); + System::initialize(&1, &Default::default(), &secondary_vrf_pre_digest); Babe::do_initialize(1); assert_eq!(Babe::author_vrf_randomness(), Some(vrf_randomness)); @@ -150,12 +153,8 @@ fn no_author_vrf_output_for_secondary_plain() { let genesis_slot = Slot::from(10); let secondary_plain_pre_digest = make_secondary_plain_pre_digest(0, genesis_slot); - System::initialize( - &1, - &Default::default(), - &secondary_plain_pre_digest, - Default::default(), - ); + System::reset_events(); + System::initialize(&1, &Default::default(), &secondary_plain_pre_digest); assert_eq!(Babe::author_vrf_randomness(), None); Babe::do_initialize(1); diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 17e8f746be1ff..be9904e71c5c9 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -533,7 +533,8 @@ fn run_out_of_gas() { } fn initialize_block(number: u64) { - System::initialize(&number, &[0u8; 32].into(), &Default::default(), Default::default()); + System::reset_events(); + System::initialize(&number, &[0u8; 32].into(), &Default::default()); } #[test] diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index e2231ae609a58..bf86808f8dd8b 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -303,11 +303,7 @@ where if Self::runtime_upgraded() { weight = weight.saturating_add(Self::execute_on_runtime_upgrade()); } - >::initialize( - block_number, - parent_hash, - digest, - ); + >::initialize(block_number, parent_hash, digest); weight = weight.saturating_add(>::on_initialize(*block_number)); @@ -544,11 +540,7 @@ where // OffchainWorker RuntimeApi should skip initialization. let digests = header.digest().clone(); - >::initialize( - header.number(), - header.parent_hash(), - &digests, - ); + >::initialize(header.number(), header.parent_hash(), &digests); // Frame system only inserts the parent hash into the block hashes as normally we don't know // the hash for the header before. However, here we are aware of the hash and we can add it diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index ddfae3d7ea75b..91006d66b7deb 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -326,7 +326,8 @@ pub fn start_session(session_index: SessionIndex) { System::parent_hash() }; - System::initialize(&(i as u64 + 1), &parent_hash, &Default::default(), Default::default()); + System::reset_events(); + System::initialize(&(i as u64 + 1), &parent_hash, &Default::default()); System::set_block_number((i + 1).into()); Timestamp::set_timestamp(System::block_number() * 6000); @@ -345,7 +346,8 @@ pub fn start_era(era_index: EraIndex) { } pub fn initialize_block(number: u64, parent_hash: H256) { - System::initialize(&number, &parent_hash, &Default::default(), Default::default()); + System::reset_events(); + System::initialize(&number, &parent_hash, &Default::default()); } pub fn generate_equivocation_proof( diff --git a/frame/merkle-mountain-range/src/tests.rs b/frame/merkle-mountain-range/src/tests.rs index b82378d6efa90..576a7ace8f1c0 100644 --- a/frame/merkle-mountain-range/src/tests.rs +++ b/frame/merkle-mountain-range/src/tests.rs @@ -41,11 +41,7 @@ fn new_block() -> u64 { LEAF_DATA.with(|r| r.borrow_mut().a = number); frame_system::Pallet::::reset_events(); - frame_system::Pallet::::initialize( - &number, - &hash, - &Default::default(), - ); + frame_system::Pallet::::initialize(&number, &hash, &Default::default()); MMR::on_initialize(number) } diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 0167f7aefdef8..a0a4c43518294 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1281,11 +1281,7 @@ impl Pallet { } /// Start the execution of a particular block. - pub fn initialize( - number: &T::BlockNumber, - parent_hash: &T::Hash, - digest: &generic::Digest, - ) { + pub fn initialize(number: &T::BlockNumber, parent_hash: &T::Hash, digest: &generic::Digest) { // populate environment ExecutionPhase::::put(Phase::Initialization); storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &0u32); From 517d61ce7bed15930c3ded117236a74429560cef Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 10 Jan 2022 23:04:15 +1300 Subject: [PATCH 3/6] add test --- frame/executive/src/lib.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index bf86808f8dd8b..d19ea8127bad3 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -824,6 +824,7 @@ mod tests { fn on_runtime_upgrade() -> Weight { sp_io::storage::set(TEST_KEY, "custom_upgrade".as_bytes()); sp_io::storage::set(CUSTOM_ON_RUNTIME_KEY, &true.encode()); + System::deposit_event(frame_system::Event::CodeUpdated); 100 } } @@ -1290,6 +1291,30 @@ mod tests { }); } + #[test] + fn event_from_runtime_upgrade_is_included() { + new_test_ext(1).execute_with(|| { + // Make sure `on_runtime_upgrade` is called. + RUNTIME_VERSION.with(|v| { + *v.borrow_mut() = + sp_version::RuntimeVersion { spec_version: 1, ..Default::default() } + }); + + // set block number to non zero so events are not exlcuded + System::set_block_number(1); + + Executive::initialize_block(&Header::new( + 2, + H256::default(), + H256::default(), + [69u8; 32].into(), + Digest::default(), + )); + + System::assert_last_event(frame_system::Event::::CodeUpdated.into()); + }); + } + /// Regression test that ensures that the custom on runtime upgrade is called when executive is /// used through the `ExecuteBlock` trait. #[test] From 12d39ad70f166ecea3cd1c7abd08fd0124813b87 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Mon, 10 Jan 2022 23:11:34 +1300 Subject: [PATCH 4/6] update comment --- frame/system/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index a0a4c43518294..b36bb23a9ced5 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1411,8 +1411,8 @@ impl Pallet { AllExtrinsicsLen::::put(len as u32); } - /// Reset events. Can be used as an alternative to - /// `initialize` for tests that don't need to bother with the other environment entries. + /// Reset events. This needs to be used in addition with `initialize` for each new block + /// to clear events from previous block. pub fn reset_events() { >::kill(); EventCount::::kill(); From 89c22e8d842f3732359ff10dccf1c236d65aa21b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 10 Jan 2022 11:27:37 +0100 Subject: [PATCH 5/6] Update frame/system/src/lib.rs --- frame/system/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index b36bb23a9ced5..7615424ba57ee 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1411,7 +1411,9 @@ impl Pallet { AllExtrinsicsLen::::put(len as u32); } - /// Reset events. This needs to be used in addition with `initialize` for each new block + /// Reset events. + /// + /// This needs to be used in prior calling [`initialize`](Self::initialize) for each new block /// to clear events from previous block. pub fn reset_events() { >::kill(); From 9815c9c5d4d3dfcb757d8430ccdedf01858c29f7 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Wed, 12 Jan 2022 14:52:35 +1300 Subject: [PATCH 6/6] trigger CI