Versioned OnRuntimeUpgrade

[ggwpez]

We currently have the pattern of first manually checking whether a migration should run or can be deleted.
Then setting the version and in the post_upgrade we check that it was set. This is error prone and redundant.

We could improve here by using either associated types/consts or const generics to track the required version directly in the trait.
Both being options for the case that there is (and should) be no version.

pub trait OnRuntimeUpgrade {
    const From: Option<u32>,
    const To: Option<u32>,

To keep it backwards compatible we should probably create a new or wrapper trait.

[ECJ222]

This looks interesting @ggwpez.

Can you provide an example of how the associated types or const generics would be used in practice to determine whether a migration should run or be deleted?

[ggwpez]

Can you provide an example of how the associated types or const generics would be used in practice to determine whether a migration should run or be deleted?

I would like to wait for @kianenigma to tell us his thought about this.
In general you could familiarize yourself with the patterns that we use through migrations eg here. This is done manually in nearly every migrations.

[kianenigma]

Yeah, this is very much like what I had in mind. a VersionedOnRuntimeUpgrade has a blanket impl of OnRuntimeUpgrade and does all of this stuff automatically.

[ECJ222]

Interesting, this was the solution I had in mind based on what @kianenigma proposed.

pub trait VersionedOnRuntimeUpgrade {
	/// Version before upgrade.
	/// It will be used to check whether the migration is required or not.
	const FROM: Option<u32>;
	/// Version after upgrade.
	/// It will be used to check whether the migration is required or not
	/// and to set the new version after the upgrade.
	const TO: Option<u32>;
	/// Checks if the two constants are different and
	/// returns a boolean indicating whether the migration should be run or deleted.
	fn is_required() -> bool;
}

migration implementation.

impl<T: OnRuntimeUpgrade> VersionedOnRuntimeUpgrade for T {
	const FROM: Option<u32> = Some(StorageVersion::<T>::get()); // 12
	const TO: Option<u32> = Some(Pallet::<T>::current_storage_version()); // 13

	fn is_required() -> bool {
		Self::FROM != Self::TO
	}
}

if is_required() returns true we upgrade the migration to the latest version of the runtime otherwise just delete or skip the migration. @ggwpez

[ggwpez]

@ECJ222 I am not sure if i correctly understood your example code.
The FROM and TO would be hard-coded, not dynamic. The we can compare FROM with the current on-chain version and if that maches; do the upgrade and overwrite the on-chain version with TO.
Otherwise if it not matches; do nothing / log error and possibly fail in pre_upgrade.

[kianenigma]

pub struct AutoMigrate<From, To, Inner, Pallet>(
	sp_std::marker::PhantomData<(From, To, Inner, Pallet)>,
);
impl<
		From: Get<StorageVersion>,
		To: Get<StorageVersion>,
		Inner: OnRuntimeUpgrade,
		Pallet: GetStorageVersion + PalletInfoAccess,
	> OnRuntimeUpgrade for AutoMigrate<From, To, Inner, Pallet>
{
	fn on_runtime_upgrade() -> Weight {
		let current = Pallet::current_storage_version();
		let on_chain = Pallet::on_chain_storage_version();

		if current == To::get() && on_chain == From::get() {
			let weight = Inner::on_runtime_upgrade();
			current.put::<Pallet>();

			log!(info, "{:?} applied successfully.", To::get());
			weight
		} else {
			log!(warn, "{:?} not applied.", To::get());
			todo!("need to get DbWeight somehow as well..");
		}
	}
}

Here's how I would do this.

[bkchr]

		if current == To::get() && on_chain == From::get() {
			let weight = Inner::on_runtime_upgrade();
			current.put::<Pallet>();

This logic is wrong. Especially if you may need to put multiple migrations in one runtime upgrade.

		if on_chain == From::get() {
			let weight = Inner::on_runtime_upgrade();
			To::get().put::<Pallet>();

That is correct.

[kianenigma]

Okay, here's a new thought about this, as discussed in DM with @ggwpez when discussing 298dacd. A typical runtime migration looks like this:

// assuming you have set `#[pallet::storage_verson(2)]`

fn on_runtime_upgrade() -> Weight {
	let current = Pallet::<T>::current_storage_version();
	let onchain = Pallet::<T>::on_chain_storage_version();

	if current == 2 && onchain == 1 {
		// do stuff.
		current.put::<Pallet<T>>();
	} else {
		log!(info, "Migration did not executed. This probably should be removed");
	}
}

Using this current is more problematic than useful. It prevents migration chaining, and it is also complicated to explain.

First, I would remove #[pallet::storage_version] aka current.

Each OnRuntimeUpgrade would have type Version: Get<Option<u32>>.

If this is None, then the executive would execute the migration script in any case. migrations defined in the pallet should initially be like this, and we should strive to deprecate them.

If this is Some(x), the migration would only be enacted of x == onchain + 1, and x is automatically written at the end of the migration. In essence, instead of playing with two variables, we will only have one.

Moreover, once we have this, Executive can actually have a more educated approach towards executing them. All migrations are coming from the runtime. executive can first scan them, re-order them if multiple of them are related to the same pallet, potentially detect missing ones if there are multiple of them being chained, and safely ignore the old ones.

This will bring:

1. simpler understanding. #[pallet::storage_version] is too much magic.
2. simpler chaining of migrations.
3. safer execution: migration scripts do not need to be removed from the runtime.

[liamaharon]

I'm planning to start on this soon, after I wrap up my current main work effort (lazy-download).

[juangirini]

Let's look at this issue when working on this

[liamaharon]

simpler understanding. #[pallet::storage_version] is too much magic.

The only issue I can think of with removing this is that we would not know what initial on-chain version to set for a pallet when it's introduced to the runtime, blocking us from fixing paritytech/polkadot-sdk#109. You're correct though that we shouldn't need it in any migrations.

If this is None, then the executive would execute the migration script in any case. migrations defined in the pallet should initially be like this, and we should strive to deprecate them.

Cool. I like the idea of the executive pallet managing this, so we don't need to think about versions at all inside the on_runtime_upgrade function itself.

[bkchr]

First, I would remove #[pallet::storage_version] aka current.

To what version are you then setting the storage version in the genesis state? I'm with you that we should probably remove the current_storage_version function, but I don't see how you want to remove the attribute.

[liamaharon]

First, I would remove #[pallet::storage_version] aka current.

To what version are you then setting the storage version in the genesis state? I'm with you that we should probably remove the current_storage_version function, but I don't see how you want to remove the attribute.

That was also my initial thought.

However, if we tightly couple at least the most recent migration to the pallet, we could initialise the on-chain version based on the latest migrations To. See this comment #14311 (comment).

I'm still unsure if we should do it, but it's something to consider.