Update VersionedMigration to ensure it is not possible to use the unchecked migration

[xlc]

We should have a new UncheckedOnRuntimeUpgrade trait, and have the unchecked migration implements it instead of OnRuntimeUpgrade. In this way, it will not be possible to accidental use the unchecked migration in the actual migration.

[dastansam]

hey @ggwpez

can I start working on this?

[ggwpez]

Need to check with @liamaharon first. The idea is to rename+deprecate the current OnRuntimeUpgrade trait and then only advertise the checked version?

[liamaharon]

@xlc do you mind elaborating here, to make sure we're on the same page?

[xlc]

polkadot-sdk/cumulus/pallets/xcmp-queue/src/migration.rs

Lines 95 to 138 in b1c5131

	/// Migrates `QueueConfigData` from v1 (using only reference time weights) to v2 (with
	/// 2D weights).
	pub struct UncheckedMigrationToV2<T: Config>(PhantomData<T>);
	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrationToV2<T> {
	#[allow(deprecated)]
	fn on_runtime_upgrade() -> Weight {
	let translate = |pre: v1::QueueConfigData| -> v2::QueueConfigData {
	v2::QueueConfigData {
	suspend_threshold: pre.suspend_threshold,
	drop_threshold: pre.drop_threshold,
	resume_threshold: pre.resume_threshold,
	threshold_weight: Weight::from_parts(pre.threshold_weight, 0),
	weight_restrict_decay: Weight::from_parts(pre.weight_restrict_decay, 0),
	xcmp_max_individual_weight: Weight::from_parts(
	pre.xcmp_max_individual_weight,
	DEFAULT_POV_SIZE,
	),
	}
	};
	if v2::QueueConfig::<T>::translate(|pre| pre.map(translate)).is_err() {
	log::error!(
	target: crate::LOG_TARGET,
	"unexpected error when performing translation of the QueueConfig type \
	during storage upgrade to v2"
	);
	}
	T::DbWeight::get().reads_writes(1, 1)
	}
	}
	/// [`UncheckedMigrationToV2`] wrapped in a
	/// [`VersionedMigration`](frame_support::migrations::VersionedMigration), ensuring the
	/// migration is only performed when on-chain version is 1.
	#[allow(dead_code)]
	pub type MigrationToV2<T> = frame_support::migrations::VersionedMigration<
	1,
	2,
	UncheckedMigrationToV2<T>,
	Pallet<T>,
	<T as frame_system::Config>::DbWeight,
	>;

this is an example migration using VersionedMigration

It implements a UncheckedMigrationToV2, which is unsafe. We have to wrap it with VersionedMigration and use it.

However, we can't prevent people from using the UncheckedMigrationToV2.

This is because VersionedMigration takes a type implements OnRuntimeUpgrade, and offers an implementation of OnRuntimeUpgrade.

The solution is add a new trait UncheckedOnRuntimeUpgrade, and make VersionedMigration take it. In that way, the UncheckedMigration does not implement OnRuntimeUpgrade and therefore cannot be accidentally used.

[liamaharon]

Cool thanks @xlc I think this makes sense. So we should leave OnRuntimeUpgrade as-is, but make a breaking change to VersionedMigration so that it takes a struct implementing an identical trait with a different name UncheckedOnRuntimeUpgrade.

[liamaharon]

@dastansam I've assigned you, don't hesitate to drop questions here if you have any.