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 13 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
138 changes: 77 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,44 @@ 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 weight = Weight::zero();
for_tuples!( #(
let _guard = frame_support::StorageNoopGuard::default();
// expected unwrap: we want to panic if any checks fail right here right now.
let state = Tuple::pre_upgrade().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to just have some new on_runtime_upgrade_with_pre_and_post that returns a result. (the name is for sure shit!)

Copy link
Contributor Author

@ruseinov ruseinov Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To be fair I'm not sure if this is needed, try-runtime is used for testing. If any of the checks panic - it's they need to be fixed. Seems like extra complexity to come up with another method that does have a Result return type just for the sake of a couple of unwraps.

On the other hand it hurts readability a little bit.

If I'm being honest I just don't want to add yet another method to this interface and make things more confusing for future implementors.

Correct me if I'm wrong, you are proposing to add an extra method on OnRuntimeUpgrade when try-runtime.
The one that's going to be called instead of on_runtime_upgrade for try-runtime scenarios.

Alternatively we could introduce this method and still call it in on_runtime_upgrade, but then we'd have to unwrap anyway, so there is no point.

Copy link
Contributor

Choose a reason for hiding this comment

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

If @bkchr's point is to only prevent unwrap(), I disagree.

If there's more benefit in the new method, I am all ears.

Copy link
Member

Choose a reason for hiding this comment

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

My point here was to prevent the unwrap. However, if we keep it. I demand a expect that provides more context, at least like the index of the tuple to have some idea what failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've discussed it before and I think the real answer here is RUST_BACKTRACE=1, which gives you exactly the failing line. That being said, @ruseinov can you please update the unwraps to expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point here was to prevent the unwrap. However, if we keep it. I demand a expect that provides more context, at least like the index of the tuple to have some idea what failed.

Will do!

drop(_guard);

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

let _guard = frame_support::StorageNoopGuard::default();
// expected unwrap: we want to panic if any checks fail right here right now.
Copy link
Contributor

Choose a reason for hiding this comment

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

bad indentation (my bad)

Tuple::post_upgrade(state).unwrap();
ruseinov marked this conversation as resolved.
Show resolved Hide resolved
drop(_guard);
)* );
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 +360,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 +374,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 +439,26 @@ mod tests {
#[cfg(feature = "try-runtime")]
#[test]
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;
}

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);
assert_eq!(s, "Test1");
Ok(())
}
Expand All @@ -437,6 +469,7 @@ mod tests {
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: u32 = Decode::decode(&mut state.as_slice()).unwrap();
Test2Assertions::mutate(|val| *val += 1);
assert_eq!(s, 100);
Ok(())
}
Expand All @@ -447,60 +480,43 @@ mod tests {
}
fn post_upgrade(state: Vec<u8>) -> Result<(), &'static str> {
let s: bool = Decode::decode(&mut state.as_slice()).unwrap();
Test3Assertions::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 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);

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);

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);
});
}
}