Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
reset events before apply runtime upgrade (#10620)
Browse files Browse the repository at this point in the history 
* reset events before apply runtime upgrade

* fix tests

* add test

* update comment

* Update frame/system/src/lib.rs

* trigger CI

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
  • Loading branch information
@xlc @bkchr
xlc and bkchr authored Jan 12, 2022
1 parent 3541490 commit 306be17
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 83 deletions.
4 changes: 2 additions & 2 deletions frame/aura/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions frame/authorship/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
});
Expand Down
7 changes: 4 additions & 3 deletions frame/babe/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(&current_block, &parent_hash, &pre_digest, InitKind::Full);
System::reset_events();
System::initialize(&current_block, &parent_hash, &pre_digest);
System::set_block_number(current_block);
Timestamp::set_timestamp(current_block);
System::finalize()
Expand Down
17 changes: 8 additions & 9 deletions frame/babe/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion frame/contracts/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
45 changes: 32 additions & 13 deletions frame/executive/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,16 +294,16 @@ 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.
<frame_system::Pallet<System>>::reset_events();

let mut weight = 0;
if Self::runtime_upgraded() {
weight = weight.saturating_add(Self::execute_on_runtime_upgrade());
}
<frame_system::Pallet<System>>::initialize(
block_number,
parent_hash,
digest,
frame_system::InitKind::Full,
);
<frame_system::Pallet<System>>::initialize(block_number, parent_hash, digest);
weight = weight.saturating_add(<AllPalletsWithSystem as OnInitialize<
System::BlockNumber,
>>::on_initialize(*block_number));
Expand Down Expand Up @@ -510,7 +510,6 @@ where
&(frame_system::Pallet::<System>::block_number() + One::one()),
&block_hash,
&Default::default(),
frame_system::InitKind::Inspection,
);

enter_span! { sp_tracing::Level::TRACE, "validate_transaction" };
Expand Down Expand Up @@ -541,12 +540,7 @@ where
// OffchainWorker RuntimeApi should skip initialization.
let digests = header.digest().clone();

<frame_system::Pallet<System>>::initialize(
header.number(),
header.parent_hash(),
&digests,
frame_system::InitKind::Inspection,
);
<frame_system::Pallet<System>>::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
Expand Down Expand Up @@ -830,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
}
}
Expand Down Expand Up @@ -1296,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::<Runtime>::CodeUpdated.into());
});
}

/// Regression test that ensures that the custom on runtime upgrade is called when executive is
/// used through the `ExecuteBlock` trait.
#[test]
Expand Down
6 changes: 4 additions & 2 deletions frame/grandpa/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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(
Expand Down
8 changes: 2 additions & 6 deletions frame/merkle-mountain-range/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,8 @@ fn new_block() -> u64 {
let hash = H256::repeat_byte(number as u8);
LEAF_DATA.with(|r| r.borrow_mut().a = number);

frame_system::Pallet::<Test>::initialize(
&number,
&hash,
&Default::default(),
frame_system::InitKind::Full,
);
frame_system::Pallet::<Test>::reset_events();
frame_system::Pallet::<Test>::initialize(&number, &hash, &Default::default());
MMR::on_initialize(number)
}

Expand Down
3 changes: 2 additions & 1 deletion frame/randomness-collective-flip/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
42 changes: 5 additions & 37 deletions frame/system/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -1302,12 +1281,7 @@ impl<T: Config> Pallet<T> {
}

/// Start the execution of a particular block.
pub fn initialize(
number: &T::BlockNumber,
parent_hash: &T::Hash,
digest: &generic::Digest,
kind: InitKind,
) {
pub fn initialize(number: &T::BlockNumber, parent_hash: &T::Hash, digest: &generic::Digest) {
// populate environment
ExecutionPhase::<T>::put(Phase::Initialization);
storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &0u32);
Expand All @@ -1318,13 +1292,6 @@ impl<T: Config> Pallet<T> {

// Remove previous block data from storage
BlockWeight::<T>::kill();

// Kill inspectable storage entries in state when `InitKind::Full`.
if let InitKind::Full = kind {
<Events<T>>::kill();
EventCount::<T>::kill();
<EventTopics<T>>::remove_all(None);
}
}

/// Remove temporary "environment" entries in storage, compute the storage root and return the
Expand Down Expand Up @@ -1444,9 +1411,10 @@ impl<T: Config> Pallet<T> {
AllExtrinsicsLen::<T>::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.
#[cfg(any(feature = "std", feature = "runtime-benchmarks", test))]
/// 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() {
<Events<T>>::kill();
EventCount::<T>::kill();
Expand Down
21 changes: 14 additions & 7 deletions frame/system/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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 });
Expand Down Expand Up @@ -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() };
Expand Down Expand Up @@ -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)];
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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());
Expand All @@ -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);
});
Expand Down

0 comments on commit 306be17

Please sign in to comment.