[FRAME Core] Simple Multi-block Migrations

[gavofyork]

Needed for non-trivial migrations on parachains, and helpful for such migration on Relay/solo chains.

This is just a simple strategy and will work in probably around 80% of circumstances. It's good for pallets which are not depended upon by code which fires in on_initialize.

Have a migrations pallet (not too dissimilar from that of Moonbeam). This is configured with a tuple of all migrations, which provides introspection and access to each migration which needs to be completed. Migrations look like calls/tasks: they have a possibly-pessimistic weight estimation and can return the actual weight consumed. They are always benchmarked. Migrations should be written to proceed "piecewise" (e.g. one unit at a time, rather than migrating all entries of a storage map all at once).

Each migration defines a type Cursor which is Default + FullCodec + MaxEncodeLen. The migration function accepts a Cursor value and returns an Option<Cursor> value which is None if the migration is complete. If Some then it is up to the migration pallet to ensure it is used in the next call to the migrate function, storing it in state if weight constraints deem it necessary to wait until the next block before resuming.

The migrations pallet should suspend all non-critical subsystems in on_runtime_upgrade. This includes XCM and transaction processing. It should only re-enable them when all migrations' cursors are None.

One new hook function should be added: fn poll() -> Weight. This is guaranteed to execute regularly but not necessarily every block, providing a softer version of on_initialize. All Substrate pallets should have their on_initialize/on_finalize functions checked; if they can be moved or adapted to poll, they should be. While on_initialize/on_finalize should be called within an incomplete migration, poll need not (and indeed, should not) be.

Long-term

Deprecate on_* hooks

on_initialize/on_finalize should be deprecated. Pallets which absolutely require per-block-execution hooks on hard-deadlines (HDH) or mandatory extrinsics (ME) should go through a System config trait item which can give them low-level access at the cost of requiring the System config trait to be explicitly configured to reference them. There should be a regular facility for soft-deadline execution (which will work fine in almost all use-cases of parachain teams) which is implemented using tasks.

Guarantees

We should mark all storage items so they fall into one of three groups:

1. Can be accessed during a hard-deadline-hook or mandatory extrinsic
2. Can be accessed during a multi-block-migration
3. Unavailable to both MBMs and HDHs/MIs (the default)

Ideally we could have some means of statically verifying that HDH code only accessed items marked as (1) and MBM code only accessed items marked as (2).

Unfortunately, this probably cannot be done statically (can it?) but at least could form a basis for code reviews and audit.

[athei]

I am currently implementing a multi block migration system for pallet-contracts (#13638) that uses a very similar design. Getting this done quickly is time critical. So I would still merge the contracts specific migration system. But I can generalize and move it into FRAME afterwards.

[kianenigma]

on_initialize/on_finalize should be deprecated. Pallets which absolutely require per-block-execution hooks on hard-deadlines should go through a System config trait item which can give them low-level access at the cost of requiring the System config trait to be explicitly configured to reference them. There should be a regular facility for soft-deadline execution (which will work fine in almost all use-cases of parachain teams) which is implemented using tasks.

My 2 cents is that we should not make using things like on_initialize too hard, as it is a major selling point of "why should I build a pallet instead of a contract". Moreover, most of the headache related to usage of these hooks is related to parachains, not solo-chains.

So while

System config trait item which can give them low-level access at the cost of requiring the System config trait to be explicitly configured to reference them

Doesn't sound too bad, I would consider keeping it a bit simpler, something some marker along the lines of unsafe fn in Rust.

[gavofyork]

Yes that's actually what I thought of first and I'd not be against a requirement to mark it #[allow(deprecated)] or something. But I figured we do want to strongly discourage usage since it not only makes the pallet itself un-multi-block-migratable, but it also makes any pallets it uses un-multi-block-migratable. In effect it's super toxic and I can imagine random ecosystem devs not familiar with this tradeoff just automatically using it out of short-sighted laziness, leading to chains using it unable to migrate their pallets (since the migration code would automatically assume multi-block).

While on_initialize is useful, I think there are enough other things (tasks, for a start) which Frame can provide to make it basically fine to reduce the visibility of.

[gavofyork]

Also, by making it more explicit, we solve one other notable issue with the use of on_initialize: ordering. Currently some pallets must be placed with lower indices than others in the construct_runtime macro which is less than great. If we insisted on explicit knitting of these pallets (probably via traits and config items) then we'd reduce the possibility of accidental mis-configuration.

[athei]

Okay bear with me as this might be shaving the yak. I can spin that out to another issue later but I just want to write it down here for now: In my prototype that I am building right now the migration introspection also includes the storage version it upgrades to. Right, now that allows me to check statically that the sequence of migrations makes sense (is in order without holes). It allows my to check dynamically (during try-runtime) that the sequence of migrations contains all the needed migrations for the current storage.

What if we emit the information about the migrations that a runtime contains into a custom section. We also need to emit the in-code version of each pallet. We can then check on set_code that it has all the required migrations. I think this will remove another a big pain point of migrations: Making sure that a needed migration wasn't removed.

Maybe we could even emit each migration into its own custom section and then make offline tooling only include the required migrations when uploading the runtime. Then we might not need to periodically remove them for size reasons.

[ggwpez]

Yes I think that is its own issue @athei

We have some things planned to avoid the issue of forgetting migrations:

• Versioned OnRuntimeUpgrade substrate#13107
• Improve handling of unset StorageVersion substrate#13417
• TryDecodeEntireState check for storage types and pallets substrate#13013

I dont get what you mean with "custom section". Do you think the mentioned issue above is not enough?

[athei]

I dont get what you mean with "custom section". Do you think the mentioned issue above is not enough?

With custom section I mean a section in a wasm file. We use that to store the runtime version for example. This would allow us to introspect a wasm file whether it contains all required migrations before deploying it. It would probably happen off-chain by tooling which then removes the sections before upload.

It is something that comes on top of paritytech/substrate#13107.

[juangirini]

I want to bring @pgherveou in the loop as he's currently working on paritytech/substrate#13638

[ggwpez]

Just thinking about whether per-pallet migrations could be come feasible again with MB migration (i dont think so):

Originally, the migrations were part of the pallets in on_runtime_upgrade. This was easy to use for the runtime developers,
since they dont have to configure anything and can thereby not miss migrations.
We stopped doing this since it could stall parachains that execute a slow pallet migration. Now with the MBM, we could go back to per-pallet migration because they won't stall parachains anymore. It could be done with static configuration via tuples or dynamic dispatch.

But for both approaches i see the biggest downside being the ever-increasing WASM size…

[kianenigma]

Yes, I also think that we should not yet fully deprecate Pallet::on_runtimne_upgrade because with a more automatic execution, they can be orchestrated easier. But this is more a matter of on_runtime_upgrade's future, and is being discussed here.

From what I see here, MBMs would have a new interface, and a pallet wishing to define an MBM has to declare a new type/trait and has the Cursor described above. For example:

pub trait MultiBlockMigration<B> {
	type Cursor: codec::Codec + Default + codec::MaxEncodedLen;

	fn execute(now: B) -> (Weight, Option<Self::Cursor>);
}

In other words, a type is declared to a mandatory/classic migration by implementing OnRuntimeUpgrade, and one is declared to be an MBM by implementing MultiBlockMigration. The two are related, by distinct. Most notably, former is WeightClass::Mandatory, while the latter is not.

For now, it is safe to build this system, and assume that the pallet will expose these MBM types as public items, and the runtime developer's responsibility is to include them in the new pallet-mbm, very similar to what we do for classic migrations and executive.

// runtime amalgamator;
type Migrations = (pallet_foo::Foo, pallet_bar::Bar);
type MBMs = (pallet_buzz::Buzz, pallet_qux::Qux);

type Executive = Executive<_, ..., Migrations>;

impl pallet_mbm::Config for Runtime {
    type Migrations = MBMs;
}

That being said, I think we are all very familiar with the pains of this approach, and could think about how we can streamline it such that you don't need to manually tweak these lists so much. Importantly, it would be great if we alleviate any risk associated with forgetting to remove the old migrations.

[ggwpez]

The moonbeam pallet uses dynamic dispatch. Do we also want that? I think it is fine in this case since we only do one dynamic call per migration and block. It also gives us the possibility of a &self pointer for storing configuration, otherwise all the config needs to be compile-time.

PS: Okay not sure if it is possible, since the dyn Migration requires the Cursor type to be know (likedyn Migration<Cursor = ?>), and we dont know that…

[kianenigma]

PS: Okay not sure if it is possible, since the dyn Migration requires the Cursor type to be know (likedyn Migration<Cursor = ?>), and we dont know that…

Yeah I have exactly faced the same issue. Because different Migrations have different cursors, they are affectively different types.

I would initially try with tuples, but in general I find code written without them easier to argue about.

[ggwpez]

I would initially try with tuples, but in general I find code written without them easier to argue about.

Tuples dont make this easier. I also tried that, and the aggregated tuple Cursor type is even worse, since it then becomes something like (u8, (Migration1::Cursor, Migration2::Cursor, …)) and there are no easy functions to manipulate tuples in an impl_trait_for_tuples.
I resorted to opaque cursors by using BoundedVec now, that seems easily work.

[kianenigma]

I resorted to opaque cursors by using BoundedVec now, that seems easily work.

Yes, tuple or dynamic dispatch, the cursor for each migration is different, so it has to be encoded as opaque and decoded individually.

This also means that, you need to keep the _type_s for the migrations (and consequently how to decode their cursor) around for as long as they need to execute. Afterwards they can be removed.

[athei]

You could also request the Migration to be Codec itself and make the migrate function take &mut self. Saves you defining one associated type. We did this and also opted for the tuple approach. The infrastructure encodes/decoded it into a BoundedVec<u8> as you suggested. Makes the migrations itself really concise: https://github.com/paritytech/substrate/blob/a626589bf9363b0b8659b75a1cf18a246cf1ed58/frame/contracts/src/migration/v9.rs#L86-L117

The definition looks clean, too:
https://github.com/paritytech/substrate/blob/a626589bf9363b0b8659b75a1cf18a246cf1ed58/frame/contracts/src/lib.rs#L323-L331

The tuple approach allows for statically checking that the sequence of versions make sense:
https://github.com/paritytech/substrate/blob/a626589bf9363b0b8659b75a1cf18a246cf1ed58/frame/contracts/src/migration.rs#L396-L415

[ggwpez]

You could also request the Migration to be Codec itself and make the migrate function take &mut self. Saves you defining one associated type. We did this and also opted for the tuple approach. The infrastructure encodes/decoded it into a BoundedVec<u8> as you suggested. Makes the migrations itself really concise:

Huh, that's a great idea. Did not think about it. I will try it locally, thanks!

[pgherveou]

One interesting thing that the framework facilitate as well, and that we are pushing in this last PR, is also to enforce that no more than the exact set of migration steps are embedded into your wasm runtime

https://github.com/paritytech/substrate/blob/c927735d8f43f05ba67cc1e1c4cbb3cb5eb386b5/frame/contracts/src/migration.rs#L309-L319