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

ruseinov · 2022-09-21T12:08:48Z

Fixes #12295

@bkchr let me know if that's what you had in mind.

frame/support/src/traits/hooks.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

frame/support/src/traits/hooks.rs

kianenigma

You should now be able to bring back this commit and it should work? cc @Ank4n

635bada

Logic def. sounds good, but not sure what is going on in the tests, have to look later.

Also, please add to the right project :p

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

ruseinov · 2022-09-22T11:36:47Z

Logic def. sounds good, but not sure what is going on in the tests, have to look later.

Just removed the redundant calls to pre/post. Because all of this is happening in on_runtime_upgrade now and the state is being passed from one to another there too, sequentially.

frame/support/src/traits/hooks.rs

ggwpez

This was also annoying me, thanks for changing it!

frame/support/src/traits/hooks.rs

bkchr · 2022-09-22T12:36:52Z

frame/support/src/traits/hooks.rs

@@ -22,6 +22,7 @@ use impl_trait_for_tuples::impl_for_tuples;
 use sp_runtime::traits::AtLeast32BitUnsigned;
 use sp_std::prelude::*;

+#[cfg(test)]


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

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

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.

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

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

yeah, I was looking for that :)

bkchr · 2022-09-22T12:37:55Z

frame/support/src/traits/hooks.rs

+			// expected unwrap: we want to panic if any checks fail right here right now.
+			let state = Tuple::pre_upgrade().unwrap();


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

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.

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

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

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.

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?

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!

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ruseinov · 2022-09-22T14:38:08Z

bot rebase

…ime-sequential

paritytech-processbot · 2022-09-22T14:38:13Z

Rebased

kianenigma · 2022-09-22T19:11:04Z

frame/support/src/traits/hooks.rs

-		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(|| {


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

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:

if feature = try-runtime, pre-migrate has been run. You can assert this by writing to some static value in pre_migrate

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

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.

kianenigma · 2022-09-22T19:14:52Z

frame/support/src/traits/hooks.rs

+			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.


bad indentation (my bad)

kianenigma

Tests need rework, logic all good.

ruseinov · 2022-09-23T14:07:18Z

bot merge

paritytech-processbot · 2022-09-23T14:07:26Z

Waiting for commit status.

* master: [Fix] parameter_types! dead code errors (#12340) [Feature] Sequential migration execution for try-runtime (#12319) bench: Use `_` instead of `::` in auto-generated file names (#12332) Fast Unstake Pallet (#12129) Rename anonymous to pure proxy (#12283) Migrate remaining old decl_* macros to the new pallet attribute macros (#12271) pallet-utility: Disallow none origin (#12321) Make automatic storage deposits resistant against changing deposit prices (#12083) Format templates and fix `--steps` default value (#12286) Bump `wasmtime` to 1.0.0 (#12317) Introduce 'intermediate_insert' method to hide implementation details (#12215) Bound staking storage items (#12230) Use `array-bytes` for All Array/Bytes/Hex Operations (#12190) BREAKING: Rename Origin (#12258) Use temporary db for benchmarking (#12254) rpc: Implement `chainSpec` RPC API (#12261) Import target block body during warp sync (#12300) Proper naming wrt expectations (#12311) [ci] Revert cancel-pipeline job (#12309)

…12319) * [Feature] Sequential migration execution for try-runtime * remove unused * guards * reinstate encode/decode * proper feature gate * proper test feature gate * Update frame/support/src/traits/hooks.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/support/src/traits/hooks.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * fix tests * redo Tuple tests * Update frame/support/src/traits/hooks.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * use parameter_types for testing * lint fix * Update frame/support/src/traits/hooks.rs Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> * Update frame/support/src/traits/hooks.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * eloquent feature gate * redo tests * more fixes * properly handle pre/post errors * remove some tests and fix the others * add format import * import fix * more import fixes Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: parity-processbot <>

[Feature] Sequential migration execution for try-runtime

bfce676

ruseinov requested review from bkchr and kianenigma September 21, 2022 12:08

github-actions bot added the A0-please_review Pull request needs code review. label Sep 21, 2022

remove unused

efc3d29

ruseinov added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 21, 2022

ruseinov added 2 commits September 21, 2022 14:19

guards

5de382e

reinstate encode/decode

3f3fdda

ruseinov added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label Sep 21, 2022

ruseinov added 2 commits September 21, 2022 16:27

proper feature gate

2c4b856

proper test feature gate

4c455a8

kianenigma reviewed Sep 21, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 21, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

ruseinov and others added 4 commits September 21, 2022 16:48

Update frame/support/src/traits/hooks.rs

7522aac

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Update frame/support/src/traits/hooks.rs

2e13236

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

fix tests

551f04a

redo Tuple tests

7bd6c17

ruseinov requested a review from kianenigma September 22, 2022 08:58

kianenigma reviewed Sep 22, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 22, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 22, 2022

View reviewed changes

Update frame/support/src/traits/hooks.rs

7676a47

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

ruseinov added 2 commits September 22, 2022 13:47

use parameter_types for testing

da7dd03

lint fix

d2efbba

ruseinov requested a review from kianenigma September 22, 2022 11:52

jsidorenko reviewed Sep 22, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

ggwpez approved these changes Sep 22, 2022

View reviewed changes

frame/support/src/traits/hooks.rs Outdated Show resolved Hide resolved

bkchr reviewed Sep 22, 2022

View reviewed changes

ruseinov and others added 2 commits September 22, 2022 16:08

Update frame/support/src/traits/hooks.rs

626c5bf

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

Update frame/support/src/traits/hooks.rs

20c0793

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into ru/feature/try-runt…

98fc5a3

…ime-sequential

ruseinov requested review from bkchr and jsidorenko September 22, 2022 14:38

Ank4n mentioned this pull request Sep 22, 2022

Undo removing migration checks #12330

Merged

eloquent feature gate

3d2098a

kianenigma reviewed Sep 22, 2022

View reviewed changes

kianenigma suggested changes Sep 22, 2022

View reviewed changes

redo tests

d7102dc

kianenigma approved these changes Sep 23, 2022

View reviewed changes

ruseinov added 6 commits September 23, 2022 14:37

more fixes

1537c99

properly handle pre/post errors

6eb790e

remove some tests and fix the others

d7d6078

add format import

57d0f0b

import fix

11c2a6b

more import fixes

759b851

paritytech-processbot bot merged commit 8161ee1 into master Sep 23, 2022

paritytech-processbot bot deleted the ru/feature/try-runtime-sequential branch September 23, 2022 14:30

girazoki mentioned this pull request Nov 23, 2022

Moonbeam polkadot v0.9.32 moonbeam-foundation/moonbeam#1933

Merged

10 tasks

tgmichel mentioned this pull request Mar 28, 2023

try-runtime pre-post hooks not executed if defined within the pallet #13681

Closed

This was referenced Sep 5, 2023

Fix try-runtime pre/post hook (#2553) moonbeam-foundation/moonbeam#2475

Merged

Fix try-runtime pre/post hook (#2553) Moonsong-Labs/moonkit#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ruseinov commented Sep 21, 2022 •

edited

Loading

kianenigma left a comment •

edited

Loading

ruseinov commented Sep 22, 2022

ggwpez left a comment

bkchr Sep 22, 2022

ruseinov Sep 22, 2022

ruseinov Sep 22, 2022

ruseinov Sep 22, 2022

ggwpez Sep 22, 2022

ruseinov Sep 22, 2022

bkchr Sep 22, 2022

ruseinov Sep 22, 2022 •

edited

Loading

kianenigma Sep 22, 2022

bkchr Sep 22, 2022

kianenigma Sep 23, 2022

ruseinov Sep 23, 2022

ruseinov commented Sep 22, 2022

paritytech-processbot bot commented Sep 22, 2022

kianenigma Sep 22, 2022

kianenigma Sep 22, 2022

ruseinov Sep 23, 2022

kianenigma Sep 22, 2022

kianenigma left a comment

ruseinov commented Sep 23, 2022

paritytech-processbot bot commented Sep 23, 2022

		// expected unwrap: we want to panic if any checks fail right here right now.
		let state = Tuple::pre_upgrade().unwrap();

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

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

Conversation

ruseinov commented Sep 21, 2022 • edited Loading

kianenigma left a comment • edited Loading

Choose a reason for hiding this comment

ruseinov commented Sep 22, 2022

ggwpez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruseinov Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruseinov commented Sep 22, 2022

paritytech-processbot bot commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

ruseinov commented Sep 23, 2022

paritytech-processbot bot commented Sep 23, 2022

ruseinov commented Sep 21, 2022 •

edited

Loading

kianenigma left a comment •

edited

Loading

ruseinov Sep 22, 2022 •

edited

Loading