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

[Feature] Sequential migration execution for try-runtime #12319

Merged
merged 24 commits into from
Sep 23, 2022
Merged

[Feature] Sequential migration execution for try-runtime #12319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bfce676
[Feature] Sequential migration execution for try-runtime
ruseinov Sep 21, 2022
efc3d29
remove unused
ruseinov Sep 21, 2022
5de382e
guards
ruseinov Sep 21, 2022
3f3fdda
reinstate encode/decode
ruseinov Sep 21, 2022
2c4b856
proper feature gate
ruseinov Sep 21, 2022
4c455a8
proper test feature gate
ruseinov Sep 21, 2022
7522aac
Update frame/support/src/traits/hooks.rs
ruseinov Sep 21, 2022
2e13236
Update frame/support/src/traits/hooks.rs
ruseinov Sep 21, 2022
551f04a
fix tests
ruseinov Sep 22, 2022
7bd6c17
redo Tuple tests
ruseinov Sep 22, 2022
7676a47
Update frame/support/src/traits/hooks.rs
ruseinov Sep 22, 2022
da7dd03
use parameter_types for testing
ruseinov Sep 22, 2022
d2efbba
lint fix
ruseinov Sep 22, 2022
626c5bf
Update frame/support/src/traits/hooks.rs
ruseinov Sep 22, 2022
20c0793
Update frame/support/src/traits/hooks.rs
ruseinov Sep 22, 2022
98fc5a3
Merge remote-tracking branch 'origin/master' into ru/feature/try-runt…
Sep 22, 2022
3d2098a
eloquent feature gate
ruseinov Sep 22, 2022
d7102dc
redo tests
ruseinov Sep 23, 2022
1537c99
more fixes
ruseinov Sep 23, 2022
6eb790e
properly handle pre/post errors
ruseinov Sep 23, 2022
d7d6078
remove some tests and fix the others
ruseinov Sep 23, 2022
57d0f0b
add format import
ruseinov Sep 23, 2022
11c2a6b
import fix
ruseinov Sep 23, 2022
759b851
more import fixes
ruseinov Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions frame/executive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,15 +287,7 @@ where
///
/// This should only be used for testing.
pub fn try_runtime_upgrade() -> Result<frame_support::weights::Weight, &'static str> {
// ensure both `pre_upgrade` and `post_upgrade` won't change the storage root
let state = {
let _guard = frame_support::StorageNoopGuard::default();
<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::pre_upgrade().unwrap()
};
let weight = Self::execute_on_runtime_upgrade();
let _guard = frame_support::StorageNoopGuard::default();
<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::post_upgrade(state)
.unwrap();
Ok(weight)
}
}
Expand Down
169 changes: 106 additions & 63 deletions frame/support/src/traits/hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use impl_trait_for_tuples::impl_for_tuples;
use sp_runtime::traits::AtLeast32BitUnsigned;
use sp_std::prelude::*;

#[cfg(feature = "try-runtime")]
#[cfg(all(feature = "try-runtime", test))]
use codec::{Decode, Encode};

/// The block initialization trait.
Expand Down Expand Up @@ -165,27 +165,50 @@ pub trait OnRuntimeUpgrade {
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
impl OnRuntimeUpgrade for Tuple {
#[cfg(not(feature = "try-runtime"))]
fn on_runtime_upgrade() -> Weight {
let mut weight = Weight::zero();
for_tuples!( #( weight = weight.saturating_add(Tuple::on_runtime_upgrade()); )* );
weight
}

#[cfg(feature = "try-runtime")]
/// We are executing pre- and post-checks sequentially in order to be able to test several
/// consecutive migrations for the same pallet without errors. Therefore pre and post upgrade
/// hooks for tuples are a noop.
fn on_runtime_upgrade() -> Weight {
use scale_info::prelude::format;

let mut weight = Weight::zero();
// migration index in the tuple, start with 1 for better readability
let mut i = 1;
for_tuples!( #(
let _guard = frame_support::StorageNoopGuard::default();
// we want to panic if any checks fail right here right now.
let state = Tuple::pre_upgrade().expect(&format!("PreUpgrade failed for migration #{}", i));
drop(_guard);

weight = weight.saturating_add(Tuple::on_runtime_upgrade());

let _guard = frame_support::StorageNoopGuard::default();
// we want to panic if any checks fail right here right now.
Tuple::post_upgrade(state).expect(&format!("PostUpgrade failed for migration #{}", i));
drop(_guard);

i += 1;
)* );
weight
}

#[cfg(feature = "try-runtime")]
/// noop
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
let mut state: Vec<Vec<u8>> = Vec::default();
for_tuples!( #( state.push(Tuple::pre_upgrade()?); )* );
Ok(state.encode())
Ok(Vec::new())
}

#[cfg(feature = "try-runtime")]
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let state: Vec<Vec<u8>> = Decode::decode(&mut state.as_slice())
.expect("the state parameter should be the same as pre_upgrade generated");
let mut state_iter = state.into_iter();
for_tuples!( #( Tuple::post_upgrade(
state_iter.next().expect("the state parameter should be the same as pre_upgrade generated")
)?; )* );
/// noop
fn post_upgrade(_state: Vec<u8>) -> Result<(), &'static str> {
Ok(())
}
}
Expand Down Expand Up @@ -342,7 +365,9 @@ mod tests {

#[test]
fn on_initialize_and_on_runtime_upgrade_weight_merge_works() {
use sp_io::TestExternalities;
struct Test;

impl OnInitialize<u8> for Test {
fn on_initialize(_n: u8) -> Weight {
Weight::from_ref_time(10)
Expand All @@ -354,8 +379,10 @@ mod tests {
}
}

assert_eq!(<(Test, Test)>::on_initialize(0), Weight::from_ref_time(20));
assert_eq!(<(Test, Test)>::on_runtime_upgrade(), Weight::from_ref_time(40));
TestExternalities::default().execute_with(|| {
assert_eq!(<(Test, Test)>::on_initialize(0), Weight::from_ref_time(20));
assert_eq!(<(Test, Test)>::on_runtime_upgrade(), Weight::from_ref_time(40));
});
}

#[test]
Expand Down Expand Up @@ -416,91 +443,107 @@ mod tests {

#[cfg(feature = "try-runtime")]
#[test]
#[allow(dead_code)]
fn on_runtime_upgrade_tuple() {
use frame_support::parameter_types;
use sp_io::TestExternalities;

struct Test1;
struct Test2;
struct Test3;

parameter_types! {
static Test1Assertions: u8 = 0;
static Test2Assertions: u8 = 0;
static Test3Assertions: u8 = 0;
static EnableSequentialTest: bool = false;
static SequentialAssertions: u8 = 0;
}

impl OnRuntimeUpgrade for Test1 {
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
Ok("Test1".encode())
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: String = Decode::decode(&mut state.as_slice()).unwrap();
Test1Assertions::mutate(|val| *val += 1);
if EnableSequentialTest::get() {
SequentialAssertions::mutate(|val| *val += 1);
}
assert_eq!(s, "Test1");
Ok(())
}
}

impl OnRuntimeUpgrade for Test2 {
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
Ok(100u32.encode())
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: u32 = Decode::decode(&mut state.as_slice()).unwrap();
Test2Assertions::mutate(|val| *val += 1);
if EnableSequentialTest::get() {
assert_eq!(SequentialAssertions::get(), 1);
SequentialAssertions::mutate(|val| *val += 1);
}
assert_eq!(s, 100);
Ok(())
}
}

impl OnRuntimeUpgrade for Test3 {
fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
Ok(true.encode())
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: bool = Decode::decode(&mut state.as_slice()).unwrap();
Test3Assertions::mutate(|val| *val += 1);
if EnableSequentialTest::get() {
assert_eq!(SequentialAssertions::get(), 2);
SequentialAssertions::mutate(|val| *val += 1);
}
assert_eq!(s, true);
Ok(())
}
}

type TestEmpty = ();
let origin_state = <TestEmpty as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let states: Vec<Vec<u8>> = Decode::decode(&mut origin_state.as_slice()).unwrap();
assert!(states.is_empty());
<TestEmpty as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();

type Test1Tuple = (Test1,);
let origin_state = <Test1Tuple as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let states: Vec<Vec<u8>> = Decode::decode(&mut origin_state.as_slice()).unwrap();
assert_eq!(states.len(), 1);
assert_eq!(
<String as Decode>::decode(&mut states[0].as_slice()).unwrap(),
"Test1".to_owned()
);
<Test1Tuple as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();

type Test123 = (Test1, Test2, Test3);
let origin_state = <Test123 as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let states: Vec<Vec<u8>> = Decode::decode(&mut origin_state.as_slice()).unwrap();
assert_eq!(
<String as Decode>::decode(&mut states[0].as_slice()).unwrap(),
"Test1".to_owned()
);
assert_eq!(<u32 as Decode>::decode(&mut states[1].as_slice()).unwrap(), 100u32);
assert_eq!(<bool as Decode>::decode(&mut states[2].as_slice()).unwrap(), true);
<Test123 as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();

type Test321 = (Test3, Test2, Test1);
let origin_state = <Test321 as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let states: Vec<Vec<u8>> = Decode::decode(&mut origin_state.as_slice()).unwrap();
assert_eq!(<bool as Decode>::decode(&mut states[0].as_slice()).unwrap(), true);
assert_eq!(<u32 as Decode>::decode(&mut states[1].as_slice()).unwrap(), 100u32);
assert_eq!(
<String as Decode>::decode(&mut states[2].as_slice()).unwrap(),
"Test1".to_owned()
);
<Test321 as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();

type TestNested123 = (Test1, (Test2, Test3));
let origin_state = <TestNested123 as OnRuntimeUpgrade>::pre_upgrade().unwrap();
let states: Vec<Vec<u8>> = Decode::decode(&mut origin_state.as_slice()).unwrap();
assert_eq!(
<String as Decode>::decode(&mut states[0].as_slice()).unwrap(),
"Test1".to_owned()
);
// nested state for (Test2, Test3)
let nested_states: Vec<Vec<u8>> = Decode::decode(&mut states[1].as_slice()).unwrap();
assert_eq!(<u32 as Decode>::decode(&mut nested_states[0].as_slice()).unwrap(), 100u32);
assert_eq!(<bool as Decode>::decode(&mut nested_states[1].as_slice()).unwrap(), true);
<TestNested123 as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();
TestExternalities::default().execute_with(|| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not quite needed anymore as we don't combine the outputs anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, it is nice that we're testing impl_trait_for_tuples, but I think it should be more meaningful.

For example, with the new logic, I would test the following:

  1. if feature = try-runtime, pre-migrate has been run. You can assert this by writing to some static value in pre_migrate
  2. if if not(feature = try-runtime), they don't happen. You can test this by making sure a OnRuntimeUpgrade that panics pre_upgrade runs fine in not(feature = try-runtime).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see a reason to introduce those tbh. try-runtime methods are feature-gated, so are the tests. We can’t test feature-gated stuff in a non-feature-gated test.
We have assertions in the test implementations of OnRuntimeUpgrade trait and I think it’s totally valid to test all those scenarios to see that things have been executed with nested tuples, for example.

type TestEmpty = ();
let origin_state = <TestEmpty as OnRuntimeUpgrade>::pre_upgrade().unwrap();
assert!(origin_state.is_empty());
<TestEmpty as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();

type Test1Tuple = (Test1,);
let origin_state = <Test1Tuple as OnRuntimeUpgrade>::pre_upgrade().unwrap();
assert!(origin_state.is_empty());
<Test1Tuple as OnRuntimeUpgrade>::post_upgrade(origin_state).unwrap();
assert_eq!(Test1Assertions::get(), 0);
<Test1Tuple as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(Test1Assertions::take(), 1);

type Test321 = (Test3, Test2, Test1);
<Test321 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(Test1Assertions::take(), 1);
assert_eq!(Test2Assertions::take(), 1);
assert_eq!(Test3Assertions::take(), 1);

// enable sequential tests
EnableSequentialTest::mutate(|val| *val = true);

type Test123 = (Test1, Test2, Test3);
<Test123 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(Test1Assertions::take(), 1);
assert_eq!(Test2Assertions::take(), 1);
assert_eq!(Test3Assertions::take(), 1);

// reset assertions
SequentialAssertions::take();

type TestNested123 = (Test1, (Test2, Test3));
<TestNested123 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(Test1Assertions::take(), 1);
assert_eq!(Test2Assertions::take(), 1);
assert_eq!(Test3Assertions::take(), 1);
});
}
}