Contracts: Use RuntimeUpgrade hooks instead of Hooks::on_runtime_upgrade

[pgherveou]

We are trying to upgrade contracts-rococo to the latest version, but try-runtime fails because some pallets do not export a struct that implements OnRuntimeUpgrade that we can pass to frame_executive::Executive.

This PR brings the following changes:

• Define such missing OnRuntimeUpgrade structs for the pallets contracts-rococo depends on
• Fixes the STORAGE_VERSION for pallets that have a STORAGE_VERSION that does not match the one set by the migration
• Remove the on_runtime_upgrade hooks for cumulus_pallet_parachain_system, cumulus_pallet_xcmp_queue, cumulus_pallet_dmp_queue (pallets contracts-rococo depends on) and instead move these migrations to the runtime through frame_executive

Depends on paritytech/substrate#14045

[athei]

The background here is that we want to upgrade contracts-rococo and we are upgrading from an older release. Apparently the hooks are no longer called (at least during try-runtime) so we need to migrate.

[pgherveou]

Note:
it looks like leaving the Hooks:on_runtime_upgrade
generate these warnings in try-runtime

2023-05-12 13:23:53 ⚠️ ParachainSystem declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(2)`
2023-05-12 13:23:53 ⚠️ XcmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(3)` vs current storage version `StorageVersion(3)`
2023-05-12 13:23:53 ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
2023-05-12 13:23:53 ⚠️ DmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(2)`
2

Should we get rid of them in this PR?

[athei]

I am not entirely sure we need some reviews here.

[bkchr]

Note: it looks like leaving the Hooks:on_runtime_upgrade generate these warnings in try-runtime

2023-05-12 13:23:53 ⚠️ ParachainSystem declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(2)`
2023-05-12 13:23:53 ⚠️ XcmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(3)` vs current storage version `StorageVersion(3)`
2023-05-12 13:23:53 ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
2023-05-12 13:23:53 ⚠️ DmpQueue declares internal migrations (which *might* execute). On-chain `StorageVersion(2)` vs current storage version `StorageVersion(2)`
2

Should we get rid of them in this PR?

This is being printed because the pallets declare a on runtime upgrade.

[KiChjang]

Since we've touched the DMP pallet migration here, we may also want to include the DMP Migration struct into all of the runtimes that uses the DMP pallet, so that they get the right storage version.

[pgherveou]

@KiChjang @bkchr I should have addressed your comments

[pgherveou]

@bkchr anything else blocking here? if not can you ✅ 🙏

[bkchr]

Didn't we already executed the migrations for almost all chains?

[bkchr]

We probably already have applied these migrations?

[pgherveou]

but isn't is good practice to keep it here for next time we update these chains?

[bkchr]

Sorry for not having said this earlier, but I did not thought of this before. Basically you don't want to carry around all the migrations that were ever written. For 3 migrations it probably doesn't make such a big difference, but at some point you will see an impact on the size of the wasm file, which is bad. Because you will pay for useless code size.

So, the best would actually be to split up all the Migrations into single migrations as we do it for all the other pallets in FRAME. Sorry 🙈

[KiChjang]

The only exception to that is the DMP one; we've touched the storage version there, so I think it's a good idea to leave it there as we did bump the storage version.

[pgherveou]

gotcha, so should I undo all changes that don't touch the chain my team owns (contracts-rococo). All teams willing to upgrade their chain should just run try-runtime and fix all the errors themself, then once the upgrade is done , they can adjust the migration, to drop what's not necessary for the next upgrade.

This is what the new design unlock right? The migration code is configured by the runtime instead of the pallet and leave the responsibility to the runtime to pick and choose the migrations that are needed, using try-runtime to ensure that everything is properly configured

[pgherveou]

So, the best would actually be to split up all the Migrations into single migrations as we do it for all the other pallets in FRAME. Sorry 🙈

Fyi that's what we do now in our multiblock migration framework we are shipping soon (for pallets-contract), plus as a bonus we make sure at compile time that there are no versions missing (ie if you go from 9 -> 11, we ensure that you have v9,10,11)

[bkchr]

This is what the new design unlock right? The migration code is configured by the runtime instead of the pallet and leave the responsibility to the runtime to pick and choose the migrations that are needed, using try-runtime to ensure that everything is properly configured

Yes

[pgherveou]

Sorry for not having said this earlier, but I did not thought of this before. Basically you don't want to carry around all the migrations that were ever written. For 3 migrations it probably doesn't make such a big difference, but at some point you will see an impact on the size of the wasm file, which is bad. Because you will pay for useless code size.

So, the best would actually be to split up all the Migrations into single migrations as we do it for all the other pallets in FRAME. Sorry 🙈

removed all but our stuff, anyone that runs a migration on a chain should probably run try-runtime and fixed the errors.

[pgherveou]

bot merge