Fix Society v2 migration

[liamaharon]

Polkadot companion: paritytech/polkadot#7454

Partial: paritytech/polkadot-sdk#481

Gets the Society V2 migration passing on-runtime-upgrade checks and updates pre/post hooks so they won't begin panicing after the upgrade is enacted.

---

There's also a failing check in the post_upgrade assert_internal_consistency function:

if let Some(founder) = Founder::<T, I>::get() {
    assert_eq!(Members::<T, I>::get(founder).expect("founder is member").index, 0);
}

on-runtime-upgrade output:

2023-06-20 13:50:38.230 ERROR main runtime: panicked at 'assertion failed: `(left == right)`
  left: `27`,
 right: `0`', substrate/frame/society/src/migrations.rs:209:9

I've commented it out on this branch to verify that the migration is otherwise successful, but I think it needs to be addressed. @gavofyork would you like to look into this or shall I?

TODO

• investigate and fix breakage in assert_internal_consistency

Update (July 3rd)

Spoke with Gav about the above issue. swapping the Founder with the 0th index member as part of the migration is fine. members are no longer sorted in V2, so Founder will stay there.

[kianenigma]

I am not aware of the latest paradigm of how we do pre_upgrade checks and why they are causing so much issues.

It seems like now we are writing the codes in pre_upgrade and post_upgrade more leniently, such that they won't panic if they are executed ahead of time, or at a later point.

The root cause of all of this to me is that we have a massive nested tuple of migrations, we pass it all to executive, and it will blindly execute it all.

My suggested solution to this, which I may have hinted at before, will be along the lines of my comments in #14311 (comment). Something like: each OnRuntimeUpgrade should have a known version, and Executive should skip executing it if the pallet is already at the given version.

I see that now we keep the migrations in a sorted tuple by version, like:

type Migrations = (V1, V2, V3, ..);

I think it is a reasonable ask from @ParityReleases to listen to chain runtime upgrade enactments, and every time a chain is updated, this list must also be updated and the migrations associated with that version removed from master.

I believe this should make our try-runtime CI job in Polkadot always runnable.

[liamaharon]

My suggested solution to this, which I may have hinted at before, will be along the lines of my comments in #14311 (comment). Something like: each OnRuntimeUpgrade should have a known version, and Executive should skip executing it if the pallet is already at the given version.

I'm not sure if it's necessary to add extra logic to Executive for this, VersionedOnRuntimeUpgrade already performs the checks you described and skips the upgrade as well as its pre/post checks if it has already been enacted on-chain.

I think it is a reasonable ask from @ParityReleases to listen to chain runtime upgrade enactments, and every time a chain is updated, this list must also be updated and the migrations associated with that version removed from master.

I'm not sure about this approach. It is zero extra effort when using VersionedOnRuntimeUpgrade to write migrations that won't break on-runtime-upgrade after they are enacted. Adding an additional TODO item for every relaychain and parachain devops team seems like unnecessary burden to me.

Sure, teams should be free to clean up old migrations from the Migrations tuple after they're enacted (and probably should), but I don't think it should be a necessity that you must do it immediately after every upgrade or your CI and runtime upgrade testing will start to break.

[bkchr]

Suggested change

defensive!("Unexpected onchain version: {:?}. Expected 0.");

We don't want migrations to fail when they are already applied. I know this is only defensive and will not trigger on chain, but it could fail in the try runtime checks.

[ggwpez]

But this should not happen anymore since we wrap it in a VersionedRuntimeUpgrade.

[bkchr]

Only when you enable the experimental feature. But still, just because there is maybe some way to prevent this, I don't see any reason we can not fix it at the root of the problem.

[liamaharon]

I'll change it back to a log::warn.

[liamaharon]

Keep in mind, VersionedRuntimeUpgrade is intended to allow developers to not need to worry about any version checking in their OnRuntimeUpgrade hooks.

So when it is out of experimental, we need some way to mark an upgrade in a way so it can only be used when wrapped in VersionedRuntimeUpgrade.

[bkchr]

So when it is out of experimental, we need some way to mark an upgrade in a way so it can only be used when wrapped in VersionedRuntimeUpgrade.

You signal this by not making the "inner migration" public and only export the wrapped VersionedRuntimeUpgrade type.

[liamaharon]

I already tried removing pub from the Unchecked migration actually but it results in a compiler error:

error[E0446]: private type `VersionUncheckedMigrateToV2<T, I, PastPayouts>` in public interface
   --> frame/society/src/migrations.rs:102:1
    |
31  | struct VersionUncheckedMigrateToV2<T: Config<I>, I: 'static, PastPayouts>(
    | ------------------------------------------------------------------------- `VersionUncheckedMigrateToV2<T, I, PastPayouts>` declared as private
...
102 | pub type VersionCheckedMigrateToV2<T, I, PastPayouts> =
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't leak private type

I tried hacking on this a little and the only way I could make it work was via a wrapper struct, but it's really verbose and ugly:

type VersionCheckedMigrateToV2Type<T, I, PastPayouts> =
	frame_support::migrations::VersionedRuntimeUpgrade<
		0,
		2,
		VersionUncheckedMigrateToV2<T, I, PastPayouts>,
		crate::pallet::Pallet<T, I>,
		<T as frame_system::Config>::DbWeight,
	>;

pub struct VersionCheckedMigrateToV2<
	T: Config<I>,
	I: Instance + 'static,
	PastPayouts: Get<Vec<(<T as frame_system::Config>::AccountId, BalanceOf<T, I>)>>,
>(core::marker::PhantomData<(T, I, PastPayouts)>);

impl<
		T: Config<I>,
		I: Instance + 'static,
		PastPayouts: Get<Vec<(<T as frame_system::Config>::AccountId, BalanceOf<T, I>)>>,
	> OnRuntimeUpgrade for VersionCheckedMigrateToV2<T, I, PastPayouts>
{
	fn on_runtime_upgrade() -> Weight {
		VersionCheckedMigrateToV2Type::<T, I, PastPayouts>::on_runtime_upgrade()
	}

	#[cfg(feature = "try-runtime")]
	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
		VersionCheckedMigrateToV2Type::<T, I, PastPayouts>::pre_upgrade()
	}

	#[cfg(feature = "try-runtime")]
	fn post_upgrade(_state: Vec<u8>) -> Result<(), TryRuntimeError> {
		VersionCheckedMigrateToV2Type::<T, I, PastPayouts>::post_upgrade(_state)
	}

	#[cfg(feature = "try-runtime")]
	fn try_on_runtime_upgrade(checks: bool) -> Result<Weight, TryRuntimeError> {
		VersionCheckedMigrateToV2Type::<T, I, PastPayouts>::try_on_runtime_upgrade(checks)
	}
}

Not finished by any means, just a really rough PoC.

Could be a good candidate for a macro, but I would like to avoid that and keep it vanilla Rust for the sake of simplicity if we can.

Let me know if you have any other ideas & if I may be missing something easy, otherwise I'll create an issue to track establishing a pattern for private unchecked types and updating examples.

[bkchr]

Yeah the type needs to be public when it appears in the generic. However, you can make the type public but not export it from the migrations module ;)

[liamaharon]

bot merge

[paritytech-processbot]

Error: "Check reviews" status is not passing for paritytech/polkadot#7454

[ggwpez]

bot merge