Get rid of migration-guard storage values and only use frame metadata in cargo.toml

[kianenigma]

with #9165, we have something much simpler to work with regarding storage migrations. We should remove all storage values such as

substrate/frame/staking/src/lib.rs

Lines 737 to 749 in df4a588

	// A value placed in storage that represents the current version of the Staking storage. This value
	// is used by the `on_runtime_upgrade` logic to determine whether we run storage migration logic.
	// This should match directly with the semantic versions of the Rust crate.
	#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug)]
	enum Releases {
	V1_0_0Ancient,
	V2_0_0,
	V3_0_0,
	V4_0_0,
	V5_0_0, // blockable validators.
	V6_0_0, // removal of all storage associated with offchain phragmen.
	V7_0_0, // keep track of number of nominators / validators in map
	}

and only use this attribute. Using this unified attribute has many benefits. One important one is that we can systematically detect migrations and no longer depend on a label etc.

[hirschenberger]

I'm looking into this and try and to figure out how to set the default value for StorageVersion. E.g. to make it start at 1 and not the default 0 to keep them in sync with the semantic Releases::V1_0_0?

[bkchr]

E.g. to make it start at 1 and not the default 0 to keep them in sync with the semantic Releases::V1_0_0?

Why you want to start at 1? Not sure I understand what you want to do here.

[hirschenberger]

E.g. in the scheduler pallet, I added #[pallet::storage_version(STORAGE_VERSION)] to the Pallet struct, setting STORAGE_VERSION to 2.
Then I removed

#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug)]
enum Releases {
	V1,
	V2,
}

impl Default for Releases {
	fn default() -> Self {
		Releases::V1
	}
}

then I added:

#[pallet::genesis_build]
	impl<T: Config> GenesisBuild<T> for GenesisConfig {
		fn build(&self) {
			StorageVersion::new(2).put::<Pallet<T>>();
		}
	}

But then the migration test fails here:

    assert_eq!(Scheduler::current_storage_version(), 2);
    assert_eq!(Scheduler::on_chain_storage_version(), 1)

because the on_chain_storage_version is 0 not 1. My assumption was, that it is like that bc. the default value for StorageVersion is zero. Which was overridden in the previous approach by implementing Defaultfor StorageVersion?

[bkchr]

Genesis already sets the latest storage version as on-chain storage version.

[hirschenberger]

But how is that test supposed to work? https://github.com/paritytech/substrate/blob/master/frame/scheduler/src/lib.rs#L1621

[bkchr]

For such a test you would need to overwrite the version before running the migration. This is clearly some old stuff that was done in a different way.

[kianenigma]

This is outdated. Now, we have #[pallet::storage_version]. A fix for this issue should move all pallets to use this.

All in all though, I don't recommend doing this as the whole migration story is not clear. I am not sure if the effort here will be worthwhile.