Update contract multi-block migration #14313

pgherveou · 2023-06-06T21:58:30Z

Follow-up to #14045:

Update migration module docstring
Rename Migrate to MigrationStep
Update is_upgrade_supported
Ensure that we do as many migration steps as possible given the weight limit passed to on_idle

Co-authored-by: PG Herveou <pgherveou@gmail.com>

Ensure that we do as many steps as possible given the weight limit passed to on_idle

Co-authored-by: PG Herveou <pgherveou@gmail.com>

…inons_per_block

frame/contracts/src/migration.rs

Co-authored-by: Juan <juangirini@gmail.com>

juangirini

Gotta love the docs

frame/contracts/src/lib.rs

frame/contracts/src/migration.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

pgherveou · 2023-06-12T10:59:23Z

@agryaznov I think I got them all, thanks for going deep on this review and the previous one 🙏

Could you please address unresolved comments from the original PR here in this follow-up?

agryaznov

Currently this

substrate/frame/contracts/src/migration.rs

Lines 300 to 306 in 103d50c

    
           log::debug!( 
        
           	target: LOG_TARGET, 
        
           	"{}: Range supported {:?}, range requested {:?}", 
        
           	<Pallet<T>>::name(), 
        
           	T::Migrations::VERSION_RANGE, 
        
           	(storage_version, target_version) 
        
           );

prints out a message like this

Contracts: Range supported (12, 12), range requested (StorageVersion(11), StorageVersion(12))

Which imo could cause a cognitive dissonance for a person not quite deep into the MBM implementation details (which I believe could be the most users of the pallet): "requested` (11,12), supported (12,12), but it's okay...".

Maybe it worth making it print supported range like (T::Migrations::VERSION_RANGE.0 - 1, T::Migrations::VERSION_RANGE)?

Or, even better, to print it in the same fashion as it was specified in the runtime config: "range requested/supported (12,)"

frame/contracts/src/benchmarking/mod.rs

frame/contracts/src/migration.rs

frame/contracts/src/migration/v11.rs

frame/contracts/src/lib.rs

frame/contracts/src/migration.rs

frame/contracts/src/migration/v10.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

frame/contracts/src/lib.rs

athei · 2023-06-20T08:56:10Z

frame/contracts/src/lib.rs

+				match result {
+					// There is not enough weight to perform a migration, or make any progress, we
+					// just return the remaining weight.
+					NoMigrationPerformed | InProgress { steps_done: 0 } => return remaining_weight,


Isn't that the same as just break?

well this case means that there might be a migration in progress, but we did not have enough gas to make any kind of progress, so you want to exit early here, are doing any work in this function is unsafe is there an actual migration going on.

athei · 2023-06-20T09:13:03Z

frame/contracts/src/migration.rs

-		if in_storage == target {
-			return true
-		}
-		if in_storage > target {
-			return false
-		}
-
 		let (low, high) = Self::VERSION_RANGE;
-		let Some(first_supported) = low.checked_sub(1) else {
-			return false
-		};
-
-		in_storage >= first_supported && target == high
+		target == high && in_storage + 1 == low


What if no migration is needed and also none is provided? In this case it would return false and pre_upgrade will fail even though migrations will be fine.

then you should probably not include it at all in the runtime executive

As long as we can catch the case when somebody forgets to put it back in again this is fine. I suppose try-runtime will catch this missing migration.

if we want to be lean and embed as little code as possible, we probably want to enforce it, I would be for leaving it like that and adding an explicit message in the pre_upgrade if storage_version == target_version

It does indeed, last time I tried, will test it again here

it is also only invoked in try_runtime, you will get away with just a warning if you were to run such a migration

substrate/frame/contracts/src/migration.rs

Lines 264 to 271 in 921dc43

if storage_version == latest_version {

log::warn!(

target: LOG_TARGET,

"{name}: No Migration performed storage_version = latest_version = {:?}",

&storage_version

);

return T::WeightInfo::on_runtime_upgrade_noop()

}

pgherveou · 2023-06-20T13:16:20Z

bot merge

* move migrate sequence to config * remove commented out code * Update frame/contracts/src/lib.rs Co-authored-by: PG Herveou <pgherveou@gmail.com> * remove Migrations generic * make runtime use noop migrations * restrict is_upgrade_supported * Update contract multi-block migration Ensure that we do as many steps as possible given the weight limit passed to on_idle * undo is_upgrade_supported change * Update bin/node/runtime/src/lib.rs Co-authored-by: PG Herveou <pgherveou@gmail.com> * wip * fix comment (paritytech#14316) * fix test * fix * Update frame/contracts/src/migration.rs Co-authored-by: Juan <juangirini@gmail.com> * fix test doc * Apply suggestions from code review Co-authored-by: Sasha Gryaznov <hi@agryaznov.com> * Fix compilation with feature runtime-benchmarks * fix example * fix cargo doc --document-private-items * private links * Remove dup comment * add doc for MigrationInProgress * PR review remove duplicate asserts * simplify upper bound * fix link * typo * typo * no unwrap() * correct log message * missing * fix typo * PR comment * Add example with single element tuple * Improve migration message * Update frame/contracts/src/benchmarking/mod.rs Co-authored-by: Sasha Gryaznov <hi@agryaznov.com> * Update frame/contracts/src/migration.rs Co-authored-by: Sasha Gryaznov <hi@agryaznov.com> * Update frame/contracts/src/migration.rs Co-authored-by: Sasha Gryaznov <hi@agryaznov.com> * use saturating_accrue instead of += * add more doc * Contracts: Better migration types (paritytech#14418) * Add explicit error, if try-runtime runs a noop migration * use mut remaining_weight --------- Co-authored-by: Juan Girini <juangirini@gmail.com> Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

juangirini and others added 7 commits June 6, 2023 13:13

move migrate sequence to config

5c14529

remove commented out code

e8c493c

Update frame/contracts/src/lib.rs

0e62930

Co-authored-by: PG Herveou <pgherveou@gmail.com>

remove Migrations generic

14a7175

make runtime use noop migrations

2601d79

restrict is_upgrade_supported

e88854a

Update contract multi-block migration

a8caaf2

Ensure that we do as many steps as possible given the weight limit passed to on_idle

pgherveou requested a review from athei as a code owner June 6, 2023 21:58

pgherveou requested review from a team, agryaznov and juangirini June 6, 2023 21:58

undo is_upgrade_supported change

1400ab0

agryaznov mentioned this pull request Jun 7, 2023

Contracts: fix benchmark comment #14316

Merged

juangirini and others added 6 commits June 7, 2023 14:14

Update bin/node/runtime/src/lib.rs

cc7609c

Co-authored-by: PG Herveou <pgherveou@gmail.com>

wip

92360cd

fix comment (#14316)

137ecc8

Merge branch 'jg/remove-default-migrate-sequence' into pg/more_migrat…

cfaf7e8

…inons_per_block

fix test

27e5cf8

fix

2935e99

juangirini reviewed Jun 7, 2023

View reviewed changes

frame/contracts/src/migration.rs Outdated Show resolved Hide resolved

Update frame/contracts/src/migration.rs

a4bb302

Co-authored-by: Juan <juangirini@gmail.com>

juangirini approved these changes Jun 7, 2023

View reviewed changes

fix test doc

f3e81ce

agryaznov reviewed Jun 8, 2023

View reviewed changes

pgherveou and others added 3 commits June 8, 2023 15:09

Apply suggestions from code review

f10f5bd

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

Fix compilation with feature runtime-benchmarks

dc556df

fix example

a7ef997

pgherveou added A0-please_review Pull request needs code review. 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 Jun 8, 2023

pgherveou added 6 commits June 9, 2023 14:24

no unwrap()

d4349fb

correct log message

7511bac

Merge branch 'master' into pg/more_migratinons_per_block

e4e5ba1

missing

3545f04

fix typo

08f84fb

PR comment

85f334f

pgherveou requested a review from agryaznov June 13, 2023 08:01

Add example with single element tuple

103d50c

agryaznov reviewed Jun 13, 2023

View reviewed changes

Improve migration message

dbb20d3

pgherveou requested a review from agryaznov June 13, 2023 17:53

pgherveou mentioned this pull request Jun 14, 2023

contracts: switch to wasmi gas metering #14084

Merged

agryaznov reviewed Jun 15, 2023

View reviewed changes

pgherveou and others added 6 commits June 15, 2023 17:40

Update frame/contracts/src/benchmarking/mod.rs

3b3c179

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

Update frame/contracts/src/migration.rs

fc977cb

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

Update frame/contracts/src/migration.rs

d45d570

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

use saturating_accrue instead of +=

9d5b6b8

add more doc

fe412a7

Merge branch 'master' into pg/more_migratinons_per_block

921dc43

pgherveou mentioned this pull request Jun 20, 2023

[FRAME Core] Simple Multi-block Migrations paritytech/polkadot-sdk#198

Closed

agryaznov approved these changes Jun 20, 2023

View reviewed changes

athei reviewed Jun 20, 2023

View reviewed changes

pgherveou added 3 commits June 20, 2023 11:45

Contracts: Better migration types (#14418)

8643fcd

Add explicit error, if try-runtime runs a noop migration

7a2c272

use mut remaining_weight

030554d

paritytech-processbot bot merged commit 8bf628d into master Jun 20, 2023

paritytech-processbot bot deleted the pg/more_migratinons_per_block branch June 20, 2023 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update contract multi-block migration #14313

Update contract multi-block migration #14313

pgherveou commented Jun 6, 2023 •

edited by agryaznov

Loading

juangirini left a comment

pgherveou commented Jun 12, 2023

agryaznov left a comment •

edited

Loading

athei Jun 20, 2023

pgherveou Jun 20, 2023

athei Jun 20, 2023

pgherveou Jun 20, 2023

athei Jun 20, 2023

pgherveou Jun 20, 2023

pgherveou Jun 20, 2023

pgherveou Jun 20, 2023

pgherveou commented Jun 20, 2023

	log::debug!(
	target: LOG_TARGET,
	"{}: Range supported {:?}, range requested {:?}",
	<Pallet<T>>::name(),
	T::Migrations::VERSION_RANGE,
	(storage_version, target_version)
	);

	if storage_version == latest_version {
	log::warn!(
	target: LOG_TARGET,
	"{name}: No Migration performed storage_version = latest_version = {:?}",
	&storage_version
	);
	return T::WeightInfo::on_runtime_upgrade_noop()
	}

Update contract multi-block migration #14313

Update contract multi-block migration #14313

Conversation

pgherveou commented Jun 6, 2023 • edited by agryaznov Loading

juangirini left a comment

Choose a reason for hiding this comment

pgherveou commented Jun 12, 2023

agryaznov left a comment • 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

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

pgherveou commented Jun 20, 2023

pgherveou commented Jun 6, 2023 •

edited by agryaznov

Loading

agryaznov left a comment •

edited

Loading