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 10 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
133 changes: 72 additions & 61 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,6 +22,7 @@ use impl_trait_for_tuples::impl_for_tuples;
use sp_runtime::traits::AtLeast32BitUnsigned;
use sp_std::prelude::*;

#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

What you want to achieve here? That this is also enabled when we are compiling the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has to be enabled only when it's both test and try-runtime and is exactly what it currently does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To give some context:

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

Will give an unused import error, because this import is only used within tests, so when we are building with try-runtime - there's a warning emitted and build fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To give a bit more context cargo remote -- test --features=try-runtime this is the only time this import has to be enabled.

Copy link
Member

Choose a reason for hiding this comment

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

#[cfg(all(feature = "try-runtime", test))]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was looking for that :)

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

Expand Down Expand Up @@ -165,27 +166,41 @@ 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 seveal
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
/// consecutive migrations for the same pallet without errors. Therefore pre and post upgrade
/// hooks for tuples are a noop.
fn on_runtime_upgrade() -> Weight {
let mut state: Vec<u8> = Vec::default();
let mut weight = Weight::zero();
for_tuples!( #(
let _guard = frame_support::StorageNoopGuard::default();
state = Tuple::pre_upgrade().unwrap();
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
drop(_guard);
weight = weight.saturating_add(Tuple::on_runtime_upgrade());
let _guard = frame_support::StorageNoopGuard::default();
Tuple::post_upgrade(state).unwrap();
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
drop(_guard);
)* );
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
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")]
/// noop
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
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")
)?; )* );
Ok(())
}
}
Expand Down Expand Up @@ -342,7 +357,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 +371,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 @@ -417,16 +436,24 @@ mod tests {
#[cfg(feature = "try-runtime")]
#[test]
fn on_runtime_upgrade_tuple() {
use sp_io::TestExternalities;
use sp_std::sync::atomic::{AtomicUsize, Ordering};

struct Test1;
struct Test2;
struct Test3;

static TEST1_ASSERTIONS: AtomicUsize = AtomicUsize::new(0);
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
static TEST2_ASSERTIONS: AtomicUsize = AtomicUsize::new(0);
static TEST3_ASSERTIONS: AtomicUsize = AtomicUsize::new(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();
TEST1_ASSERTIONS.fetch_add(1, Ordering::SeqCst);
assert_eq!(s, "Test1");
Ok(())
}
Expand All @@ -437,6 +464,7 @@ mod tests {
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: u32 = Decode::decode(&mut state.as_slice()).unwrap();
TEST2_ASSERTIONS.fetch_add(1, Ordering::SeqCst);
assert_eq!(s, 100);
Ok(())
}
Expand All @@ -447,60 +475,43 @@ mod tests {
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: bool = Decode::decode(&mut state.as_slice()).unwrap();
TEST3_ASSERTIONS.fetch_add(1, Ordering::SeqCst);
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!(TEST1_ASSERTIONS.load(Ordering::SeqCst), 0);
<Test1Tuple as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(TEST1_ASSERTIONS.load(Ordering::SeqCst), 1);

type Test123 = (Test1, Test2, Test3);
<Test123 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(TEST1_ASSERTIONS.load(Ordering::SeqCst), 2);
assert_eq!(TEST2_ASSERTIONS.load(Ordering::SeqCst), 1);
assert_eq!(TEST3_ASSERTIONS.load(Ordering::SeqCst), 1);

type Test321 = (Test3, Test2, Test1);
<Test321 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(TEST1_ASSERTIONS.load(Ordering::SeqCst), 3);
assert_eq!(TEST2_ASSERTIONS.load(Ordering::SeqCst), 2);
assert_eq!(TEST3_ASSERTIONS.load(Ordering::SeqCst), 2);

type TestNested123 = (Test1, (Test2, Test3));
<TestNested123 as OnRuntimeUpgrade>::on_runtime_upgrade();
assert_eq!(TEST1_ASSERTIONS.load(Ordering::SeqCst), 4);
assert_eq!(TEST2_ASSERTIONS.load(Ordering::SeqCst), 3);
assert_eq!(TEST3_ASSERTIONS.load(Ordering::SeqCst), 3);
});
}
}