Move `type Migrations` to `Config` by juangirini · Pull Request #14309 · paritytech/substrate

juangirini · 2023-06-06T11:31:31Z

This PR removes the hardcoded migration sequence from migrations.rs and places it within Config so it can be configured in the runtime.

How to update the code

Developers need to update their runtime configuration with their pending migrations (or () if none). For example:

use pallet_contracts::migration::{v10, v11, v9};

// ...

impl pallet_contracts::Config for Runtime {

// ...

   type Migrations = (v9::Migration<Runtime>, v10::Migration<Runtime>, v11::Migration<Runtime>);

This is a breaking change as it adds a new type to the Config, there's another breaking PR #14020 that changes it, so it would be a good idea to have them in the same release.

cumulus companion: paritytech/cumulus#2701

pgherveou

Looks good, left a few comments

frame/contracts/src/lib.rs

frame/contracts/src/migration.rs

pgherveou · 2023-06-06T11:39:50Z

you will have to fix cumulus too

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

frame/contracts/src/migration.rs

pgherveou · 2023-06-06T13:04:50Z

while we are making this change we might also want to be more strict in is_upgrade_supported here, and only allow a migration sequence that goes from current + 1 -> target. This should ensure that users don't include unneeded migration step in their wasm runtime

- in_storage >= first_supported && target == high
+ in_storage == first_supported && target == high

(+ fixing the test that touch this)

frame/contracts/src/migration.rs

pgherveou · 2023-06-06T15:35:51Z

bin/node/runtime/src/lib.rs

 	type MaxStorageKeyLen = ConstU32<128>;
 	type UnsafeUnstableInterface = ConstBool<false>;
 	type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
+	type Migrations = (NoopMigration<1>, NoopMigration<2>);


maybe we can add a quick comments to specify that these noop migrations are used for benchmarking the migration framework.

I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

A () should suffice.

these NoopMigrations are used by benchmarks, so we should keep them here

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

Lines 281 to 290 in 28f56b6

#[pov_mode = Measured]

migrate {

StorageVersion::new(0).put::<Pallet<T>>();

<Migration::<T> as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade();

let origin: RawOrigin<<T as frame_system::Config>::AccountId> = RawOrigin::Signed(whitelisted_caller());

}: {

<Contracts<T>>::migrate(origin.into(), Weight::MAX).unwrap()

} verify {

assert_eq!(StorageVersion::get::<Pallet<T>>(), 1);

}

Then how does this look like in an actual runtime?
Could be as ugly as this...

I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

@agryaznov I think that's because that is part of the config, we can't avoid having it here. Compile flags worked before because the type was internal to the pallet.

Then how does this look like in an actual runtime? Could be as ugly as this...

We could do something as ugly as that or as ugly as this (which was the initial approach)

/// Performs all necessary migrations based on `StorageVersion`. #[cfg(not(feature = "runtime-benchmarks"))] pub struct Migration<T: Config, M: MigrateSequence = T::Migrations>(PhantomData<(T, M)>); /// Custom migration for running runtime-benchmarks and tests. #[cfg(feature = "runtime-benchmarks")] pub struct Migration<T: Config, M: MigrateSequence = (NoopMigration<1>, NoopMigration<2>)>( PhantomData<(T, M)>, );

I like how it is setup in @ggwpez 's link at least it makes it clear that these noopMigration are intended for benchmarks

I have applied PG's suggested fix #14309 (comment)

bkchr · 2023-06-06T15:39:21Z

@juangirini I don't get the need for this pr. Migrations can already be specified in the runtime lib.rs, we do this all the time. Like here:

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

Lines 1981 to 1985 in b1bb12d

    
           type Migrations = ( 
        
           	pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>, 
        
           	pallet_alliance::migration::Migration<Runtime>, 
        
           	pallet_contracts::Migration<Runtime>, 
        
           );

juangirini · 2023-06-06T16:39:24Z

@juangirini I don't get the need for this pr. Migrations can already be specified in the runtime lib.rs, we do this all the time. Like here:

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

Lines 1981 to 1985 in b1bb12d

type Migrations = (

pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,

pallet_alliance::migration::Migration<Runtime>,

pallet_contracts::Migration<Runtime>,

);

@bkchr The idea here is to remove the hardcoded Migrations from here:

substrate/frame/contracts/src/migration.rs

Lines 61 to 63 in b1bb12d

    
           // In benchmark and tests we use noop migrations, to test and bench the migration framework itself. 
        
           #[cfg(not(any(feature = "runtime-benchmarks", test)))] 
        
           type Migrations<T> = (v9::Migration<T>, v10::Migration<T>, v11::Migration<T>);

so they can be fine tuned in the runtime.

Why do we need this?
As we are moving away from ReserveableCurrency in pallet_contracts #14020 (and similarly elsewhere paritytech/polkadot-sdk#226), we won't have T::Currency to perform this migration for instance, and therefore this migration will need to be generic over Currency where Currency would be the 'old' one before introducing the fungible traits.

Then we could have something like

impl pallet_contracts::Config for Runtime {

// ...

   type Migrations = (v9::Migration<Runtime>, v10::Migration<Runtime, Currency>, v11::Migration<Runtime>);

This solution indeed creates the developers the need to maintain the Migrations type.

Alternatively, we could let live T::Currency and T::Fungible for a while and keep the migration as it is. The issue with this solution is that we would then need to let T::Currency live there for the sake of migrations, and at some point later remove it. Creating two breaking changes, one when introducing T::Fungible and another one when removing T::Currency.

bkchr · 2023-06-06T16:42:30Z

Everything you said is already possible with the way we are doing runtime migrations in the lib.rs

pgherveou · 2023-06-06T17:57:40Z

Everything you said is already possible with the way we are doing runtime migrations in the lib.rs

@bkchr, unlike other pallets, we are using a new multi-block migration framework. The PR #14045 has more details on how it works, but essentially on_runtime_upgrade instead of doing the migration we set a custom storage MigrationInProgress. This storage defines a cursor for the current migration step. If it exists, a migration is in progress and it's value hold a cursor for the current migration step. These migration steps are executed on_idle or when a new migration dispatchable is invoked.

We need this new configuration trait to customize the migration steps in the runtime and use it in the pallet in these 2 places. The frame team is working on a similar solution that might not require such informations in the Config but until then, I believe that is the best way for us to configure our multi-block migrations.

Here are the relevant bits of codes discussed:

substrate/frame/contracts/src/lib.rs

Lines 329 to 332 in e88854a

    
           fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight { 
        
           	use migration::MigrateResult::*; 
        
           	let (result, weight) = Migration::<T>::migrate(remaining_weight);

substrate/frame/contracts/src/lib.rs

Lines 745 to 752 in e88854a

    
           		#[pallet::call_index(9)] 
        
           		#[pallet::weight(T::WeightInfo::migrate().saturating_add(*weight_limit))] 
        
           		pub fn migrate(origin: OriginFor<T>, weight_limit: Weight) -> DispatchResultWithPostInfo { 
        
           			use migration::MigrateResult::*; 
        
           			ensure_signed(origin)?; 
        
           			let weight_limit = weight_limit.saturating_add(T::WeightInfo::migrate()); 
        
           			let (result, weight) = Migration::<T>::migrate(weight_limit);

substrate/frame/contracts/src/migration.rs

Lines 317 to 345 in e88854a

    
           	pub(crate) fn migrate(weight_limit: Weight) -> (MigrateResult, Weight) { 
        
           		let name = <Pallet<T>>::name(); 
        
           		let mut weight_left = weight_limit; 
        
           		if weight_left.checked_reduce(T::WeightInfo::migrate()).is_none() { 
        
           			return (MigrateResult::NoMigrationPerformed, Weight::zero()) 
        
           		} 
        
           		MigrationInProgress::<T>::mutate_exists(|progress| { 
        
           			let Some(cursor_before) = progress.as_mut() else { 
        
           				return (MigrateResult::NoMigrationInProgress, T::WeightInfo::migration_noop()) 
        
           			}; 
        
           			// if a migration is running it is always upgrading to the next version 
        
           			let storage_version = <Pallet<T>>::on_chain_storage_version(); 
        
           			let in_progress_version = storage_version + 1; 
        
           			log::info!( 
        
           				target: LOG_TARGET, 
        
           				"{name}: Migrating from {:?} to {:?},", 
        
           				storage_version, 
        
           				in_progress_version, 
        
           			); 
        
           			let result = match T::Migrations::steps( 
        
           				in_progress_version, 
        
           				cursor_before.as_ref(), 
        
           				&mut weight_left, 
        
           			) {

juangirini · 2023-06-07T08:21:29Z

@pg thanks for the comment, I was not explicit about the fact that this pallet is using a custom MBM.

frame/contracts/src/migration.rs

agryaznov · 2023-06-07T08:53:33Z

bin/node/runtime/src/lib.rs

 	type MaxStorageKeyLen = ConstU32<128>;
 	type UnsafeUnstableInterface = ConstBool<false>;
 	type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
+	type Migrations = (NoopMigration<1>, NoopMigration<2>);


I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

frame/contracts/src/migration.rs

bin/node/runtime/src/lib.rs

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

juangirini · 2023-06-07T12:53:38Z

I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

@agryaznov as commented here #14309 (comment) this has been resolved. Even though the feature gated Migrations already serves as an example now, I have added a docs example

juangirini · 2023-06-09T09:58:06Z

bot merge

paritytech-processbot · 2023-06-09T09:58:15Z

Error: Github API says paritytech/cumulus#2701 is not mergeable

juangirini · 2023-06-09T11:04:22Z

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 * undo is_upgrade_supported change * Update bin/node/runtime/src/lib.rs Co-authored-by: PG Herveou <pgherveou@gmail.com> * add rust doc example for `Migrations` * feature gate NoopMigration * fix example code * improve example --------- Co-authored-by: PG Herveou <pgherveou@gmail.com>

juangirini added 2 commits June 6, 2023 13:13

move migrate sequence to config

5c14529

remove commented out code

e8c493c

juangirini requested a review from pgherveou June 6, 2023 11:31

juangirini requested a review from athei as a code owner June 6, 2023 11:31

juangirini self-assigned this Jun 6, 2023

juangirini requested a review from a team June 6, 2023 11:31

juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 6, 2023

juangirini requested a review from agryaznov June 6, 2023 11:35

juangirini added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 6, 2023

pgherveou reviewed Jun 6, 2023

View reviewed changes

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

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

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

Update frame/contracts/src/lib.rs

0e62930

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

juangirini removed the T4-smart_contracts This PR/Issue is related to smart contracts. label Jun 6, 2023

remove Migrations generic

14a7175

juangirini requested a review from pgherveou June 6, 2023 12:04

pgherveou reviewed Jun 6, 2023

View reviewed changes

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

pgherveou approved these changes Jun 6, 2023

View reviewed changes

juangirini added 2 commits June 6, 2023 17:11

make runtime use noop migrations

2601d79

restrict is_upgrade_supported

e88854a

juangirini requested a review from pgherveou June 6, 2023 15:30

pgherveou approved these changes Jun 6, 2023

View reviewed changes

juangirini requested review from bkchr and ggwpez June 7, 2023 08:21

juangirini mentioned this pull request Jun 7, 2023

Add Migrations to contracts config paritytech/cumulus#2701

Merged

agryaznov suggested changes Jun 7, 2023

View reviewed changes

undo is_upgrade_supported change

1400ab0

ggwpez reviewed Jun 7, 2023

View reviewed changes

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

pgherveou reviewed Jun 7, 2023

View reviewed changes

bin/node/runtime/src/lib.rs Show resolved Hide resolved

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

cc7609c

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

juangirini requested a review from agryaznov June 7, 2023 12:54

juangirini added 4 commits June 7, 2023 15:38

add rust doc example for Migrations

c173ad7

feature gate NoopMigration

c9cf9c6

fix example code

de545a3

improve example

62bec96

agryaznov approved these changes Jun 8, 2023

View reviewed changes

paritytech-processbot bot merged commit ff3be27 into master Jun 9, 2023

paritytech-processbot bot deleted the jg/remove-default-migrate-sequence branch June 9, 2023 11:04

nazar-pc mentioned this pull request Jun 16, 2023

Upgrade Substrate (with core-evm-relay removed) autonomys/subspace#1549

Merged

1 task

crystalin mentioned this pull request Oct 20, 2023

Update substrate/polkadot/cumulus from v0.9.43 to v1.1.0 moonbeam-foundation/moonbeam#2535

Closed

	#[pov_mode = Measured]
	migrate {
	StorageVersion::new(0).put::<Pallet<T>>();
	<Migration::<T> as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade();
	let origin: RawOrigin<<T as frame_system::Config>::AccountId> = RawOrigin::Signed(whitelisted_caller());
	}: {
	<Contracts<T>>::migrate(origin.into(), Weight::MAX).unwrap()
	} verify {
	assert_eq!(StorageVersion::get::<Pallet<T>>(), 1);
	}

Conversation

juangirini commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to update the code

Uh oh!

pgherveou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgherveou commented Jun 6, 2023

Uh oh!

Uh oh!

pgherveou commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgherveou Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

agryaznov Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

pgherveou Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

ggwpez Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgherveou Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juangirini Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgherveou Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

juangirini Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jun 6, 2023

Uh oh!

juangirini commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Jun 6, 2023

Uh oh!

pgherveou commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juangirini commented Jun 7, 2023

Uh oh!

Uh oh!

Uh oh!

agryaznov Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juangirini commented Jun 7, 2023

Uh oh!

juangirini commented Jun 9, 2023

Uh oh!

paritytech-processbot bot commented Jun 9, 2023

Uh oh!

juangirini commented Jun 9, 2023

Uh oh!

Reviewers

juangirini commented Jun 6, 2023 •

edited

Loading

pgherveou commented Jun 6, 2023 •

edited

Loading

agryaznov Jun 7, 2023 •

edited

Loading

ggwpez Jun 7, 2023 •

edited

Loading

pgherveou Jun 7, 2023 •

edited

Loading

juangirini Jun 7, 2023 •

edited

Loading

juangirini commented Jun 6, 2023 •

edited

Loading

pgherveou commented Jun 6, 2023 •

edited

Loading

agryaznov Jun 7, 2023 •

edited

Loading