Better migration API

[gavofyork]

Right now for migration code that exists within pallets we have the on_runtime_upgrade function. While convenient, this is a little problematic since code inside it need not contain any guard that would prevent it from being run repeatedly on later upgrades if not removed by then. Instead, the per-pallet migration API should ensure that any migration logic is contained only under a storage guard. This means ensconcing the current pattern exemplified with the latest democracy pallet into an API in the decl_module macro):

decl_storage! {
	// Snip
		/// True if we have upgraded so that `type RefCount` is `u32`. False (default) if not.
		UpgradedToU32RefCount build(|_| true): bool;
	// Snip
}
decl_module! {
	// Snip
		fn on_runtime_upgrade() -> frame_support::weights::Weight {
			if !UpgradedToU32RefCount::get() {
				Account::<T>::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)|
					Some(AccountInfo { nonce, refcount: rc as RefCount, data })
				);
				UpgradedToU32RefCount::put(true);
				T::MaximumBlockWeight::get()
			} else {
				0
			}
		}
	// Snip
}

would become:

decl_module! {
	// Snip
		#[migration(pallet=System)]
		fn migrate_to_u32_refcount() -> frame_support::weights::Weight {
			// A storage item `done_migrate_to_u32_refcount build(|_| true): bool;` would be introduced into
			// the module's storage. Its value would be checked and this code run if `false` (or non-existent) during
			// this pallet's `on_runtime_upgrade`.
			Account::<T>::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)|
				Some(AccountInfo { nonce, refcount: rc as RefCount, data })
			);
			T::MaximumBlockWeight::get()
		}
	// Snip
}

This might be a bit tricky at present since it would need to combine elements from both decl_module and decl_storage. It could also be that the migrations are declared in the decl_storage macro, if it's easier to wire them into the on_runtime_upgrade than it is the other way around.

[bkchr]

I think we should start binding migrations to crate versions, which should be relative easy after: paritytech/substrate#7208

This means, instead of having one extra storage item per migration, we just use the crate version to check which migrations should be executed.

[bkchr]

However, this would require that we always bump some part of the crate version for any major change, but I think this should be doable.

[gui1117]

So it this would be guideline:
when breaking storage:

• write migration as:

// Here before there should already be a check that if storag_version is before latest supported then nothing can be done.
if get_storage_version() <= current_pallet_version {
    do_migration
}

• and then bump pallet version: if current version is release then bump new major, or if current_version is not release, bump patch (this is needed only because some crate like polkadot use master and not released version)

That means pallet will be released with some version: 3.0.4 for instance if 4 migration were written during the developement of the version 3 of the pallet.

Is that an OK guideline ?
cc @apopiak

[bkchr]

In general I would say that you should bump the minor version for stuff that requires migrations. Maybe some really small stuff can only bump the patch version, but that should normally only be used for bug fixes.
We should probably also stick to normal semver for when and how to bump the version.

[apopiak]

My intuition is also to bump (at least) the minor version for new migrations/storage changes and only use patches for fixes.

[gui1117]

ok, if polkadot still follows master that means we have to bump minor version on every PR which does migration/storage changes, even when the minor version is not published. I'm ok with this.

[apopiak]

related paritytech/substrate#7208

[gui1117]

I think this PR can be closed due to paritytech/substrate#7208

The strategy now should be:

• bump crate version
• add the migration code under if condition:

fn on_runtime_upgrade() {
	const LATEST_SUPPORTED = StorageVersion::new(x, y, z);
	let storage_version = match Self::storage_version() {
		Some(v) if v >= LATEST_SUPPORTED => v,
		_ => // Log some error and return
	};

	if storage_version < PalletVersion::new(a, b, c) {
		// Do some migration.
	}

	...

	if storage_version < PalletVersion::new($new_crate_version) {
		// New migration to write
	}
}

(the code should be adapted if we support migrating from no storage version, but this should be temporary as now pallet automatically write their version in storage).

[apopiak]

I wonder whether we want to offer even more convenient APIs based on the versions introduced by paritytech/substrate#7208?
I could see something like the following:

// migration is only executed if the pallet storage version is 2.0.0
#[migration(pallet=System, from=(2,0,0))]
fn migrate_to_u32_refcount() -> frame_support::weights::Weight {
    Account::<T>::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)|
        Some(AccountInfo { nonce, refcount: rc as RefCount, data })
    );
    T::MaximumBlockWeight::get()
}

[gui1117]

I'm ok to introduce new syntax but what you propose doesn't seems to work:
if you have one migration introduced at version 2.0.0, another migration introduced at version 4.0.0 which depend on the former, and the pallet on chain with storage version 1.0.0 you probably want to execute migration for 2.0.0 and then 4.0.0.

but how could the second migration be written: from 3.0.0 or from 2.0.0, and should the first migration write to storage the version 2.0.0 so that the second migration can be triggered.

Maybe we can write something like:
#[migration(from="1.0.0", to="2.0.0"]which would be executed if version in storage is between [1.0.0, 2.0.0[ and would set 2.0.0 to storage.
#[migration(from="2,0,0", to="4.0.0"] which would be executed if version in storage is between [2.0.0, 4.0.0[ and would set 4.0.0 to storage.
But at the same time the macro would just save a if condition and a set storage.

[apopiak]

related to #353