Improve handling of unset StorageVersion (#13417)

* Improve handling of unset `StorageVersion`

When a user is forgetting to set the storage version in a pallet and calls
`current_storage_version` to compare it against the `on_chain_storage_version` it will now fail to
compile the code. Before the pallet macro just returned `StorageVersion::default()` for
`current_storage_version` leading to potential issues with migrations. Besides that it also checks
in `post_upgrade` that the pallet storage version was upgraded and thus, no migration was missed.

* Use correct `Cargo.lock`

* Fixes

* Fix test

* Update frame/support/test/tests/pallet.rs

* Ensure we don't set a storage version when the pallet is missing the attribute

* Fix merge conflict

* Update frame/support/procedural/src/pallet/expand/hooks.rs

Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

* Update frame/support/procedural/src/pallet/expand/hooks.rs

Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

* Fix compilation

* Do not run everything with `try-runtime`

* Fix test

* Apply suggestions from code review

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix `no-metadata-docs`

---------

Co-authored-by: Roman Useinov <roman.useinov@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>

substrate/frame/support/test/tests/derive_no_bound.rs

@@ -50,6 +50,23 @@ struct StructNamed<T: Config, U, V> {
 	phantom: core::marker::PhantomData<(U, V)>,
 }
 
+#[rustversion::attr(not(stable), ignore)]
+#[cfg(not(feature = "disable-ui-tests"))]
+#[test]
+fn test_struct_named_debug_print() {
+	let a_1 = StructNamed::<Runtime, ImplNone, ImplNone> {
+		a: 1,
+		b: 2,
+		c: 3,
+		phantom: Default::default(),
+	};
+
+	assert_eq!(
+		format!("{:?}", a_1),
+		String::from("StructNamed { a: 1, b: 2, c: 3, phantom: PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)> }")
+	);
+}
+
 #[test]
 fn test_struct_named() {
 	let a_1 = StructNamed::<Runtime, ImplNone, ImplNone> {
@@ -70,10 +87,6 @@ fn test_struct_named() {
 	assert_eq!(a_2.b, 2);
 	assert_eq!(a_2.c, 3);
 	assert_eq!(a_2, a_1);
-	assert_eq!(
-		format!("{:?}", a_1),
-		String::from("StructNamed { a: 1, b: 2, c: 3, phantom: PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)> }")
-	);
 
 	let b = StructNamed::<Runtime, ImplNone, ImplNone> {
 		a: 1,
@@ -88,6 +101,14 @@ fn test_struct_named() {
 #[derive(DebugNoBound, CloneNoBound, EqNoBound, PartialEqNoBound, DefaultNoBound)]
 struct StructUnnamed<T: Config, U, V>(u32, u64, T::C, core::marker::PhantomData<(U, V)>);
 
+#[rustversion::attr(not(stable), ignore)]
+#[cfg(not(feature = "disable-ui-tests"))]
+#[test]
+fn test_struct_unnamed_debug_print() {
+	let a_1 = StructUnnamed::<Runtime, ImplNone, ImplNone>(1, 2, 3, Default::default());
+	assert_eq!(format!("{:?}", a_1), String::from("StructUnnamed(1, 2, 3, PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)>)"));
+}
+
 #[test]
 fn test_struct_unnamed() {
 	let a_1 = StructUnnamed::<Runtime, ImplNone, ImplNone>(1, 2, 3, Default::default());
@@ -103,7 +124,6 @@ fn test_struct_unnamed() {
 	assert_eq!(a_2.1, 2);
 	assert_eq!(a_2.2, 3);
 	assert_eq!(a_2, a_1);
-	assert_eq!(format!("{:?}", a_1), String::from("StructUnnamed(1, 2, 3, PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)>)"));
 
 	let b = StructUnnamed::<Runtime, ImplNone, ImplNone>(1, 2, 4, Default::default());
 
@@ -169,6 +189,28 @@ enum Enum3<T: Config> {
 	VariantUnit2,
 }
 
+#[rustversion::attr(not(stable), ignore)]
+#[cfg(not(feature = "disable-ui-tests"))]
+#[test]
+fn test_enum_debug_print() {
+	type TestEnum = Enum<Runtime, ImplNone, ImplNone>;
+	let variant_0 = TestEnum::VariantUnnamed(1, 2, 3, Default::default());
+	let variant_1 = TestEnum::VariantNamed { a: 1, b: 2, c: 3, phantom: Default::default() };
+	let variant_2 = TestEnum::VariantUnit;
+	let variant_3 = TestEnum::VariantUnit2;
+
+	assert_eq!(
+		format!("{:?}", variant_0),
+		String::from("Enum::VariantUnnamed(1, 2, 3, PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)>)"),
+	);
+	assert_eq!(
+		format!("{:?}", variant_1),
+		String::from("Enum::VariantNamed { a: 1, b: 2, c: 3, phantom: PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)> }"),
+	);
+	assert_eq!(format!("{:?}", variant_2), String::from("Enum::VariantUnit"));
+	assert_eq!(format!("{:?}", variant_3), String::from("Enum::VariantUnit2"));
+}
+
 #[test]
 fn test_enum() {
 	type TestEnum = Enum<Runtime, ImplNone, ImplNone>;
@@ -208,15 +250,4 @@ fn test_enum() {
 	assert!(variant_1.clone() == variant_1);
 	assert!(variant_2.clone() == variant_2);
 	assert!(variant_3.clone() == variant_3);
-
-	assert_eq!(
-		format!("{:?}", variant_0),
-		String::from("Enum::VariantUnnamed(1, 2, 3, PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)>)"),
-	);
-	assert_eq!(
-		format!("{:?}", variant_1),
-		String::from("Enum::VariantNamed { a: 1, b: 2, c: 3, phantom: PhantomData<(derive_no_bound::ImplNone, derive_no_bound::ImplNone)> }"),
-	);
-	assert_eq!(format!("{:?}", variant_2), String::from("Enum::VariantUnit"));
-	assert_eq!(format!("{:?}", variant_3), String::from("Enum::VariantUnit2"));
 }

substrate/frame/support/test/tests/pallet.rs

@@ -23,6 +23,7 @@ use frame_support::{
 	},
 	dispatch_context::with_context,
 	pallet_prelude::{StorageInfoTrait, ValueQuery},
+	parameter_types,
 	storage::unhashed,
 	traits::{
 		ConstU32, GetCallIndex, GetCallName, GetStorageVersion, OnFinalize, OnGenesis,
@@ -37,6 +38,11 @@ use sp_io::{
 };
 use sp_runtime::{DispatchError, ModuleError};
 
+parameter_types! {
+	/// Used to control if the storage version should be updated.
+	storage UpdateStorageVersion: bool = false;
+}
+
 /// Latest stable metadata version used for testing.
 const LATEST_METADATA_VERSION: u32 = 14;
 
@@ -500,10 +506,12 @@ pub mod pallet {
 // and that a pallet with the attribute without_storage_info is correctly handled.
 #[frame_support::pallet]
 pub mod pallet2 {
-	use super::{SomeAssociation1, SomeType1};
+	use super::{SomeAssociation1, SomeType1, UpdateStorageVersion};
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;
 
+	pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
+
 	#[pallet::config]
 	pub trait Config: frame_system::Config
 	where
@@ -513,6 +521,7 @@ pub mod pallet2 {
 	}
 
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
@@ -530,6 +539,11 @@ pub mod pallet2 {
 		}
 		fn on_runtime_upgrade() -> Weight {
 			Self::deposit_event(Event::Something(31));
+
+			if UpdateStorageVersion::get() {
+				Self::current_storage_version().put::<Self>();
+			}
+
 			Weight::zero()
 		}
 	}
@@ -683,7 +697,8 @@ impl pallet5::Config for Runtime {
 
 pub type Header = sp_runtime::generic::Header<u32, sp_runtime::traits::BlakeTwo256>;
 pub type Block = sp_runtime::generic::Block<Header, UncheckedExtrinsic>;
-pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic<u32, RuntimeCall, (), ()>;
+pub type UncheckedExtrinsic =
+	sp_runtime::testing::TestXt<RuntimeCall, frame_system::CheckNonZeroSender<Runtime>>;
 
 frame_support::construct_runtime!(
 	pub struct Runtime where
@@ -812,7 +827,7 @@ fn inherent_expand() {
 	let inherents = InherentData::new().create_extrinsics();
 
 	let expected = vec![UncheckedExtrinsic {
-		function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+		call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
 		signature: None,
 	}];
 	assert_eq!(expected, inherents);
@@ -827,11 +842,11 @@ fn inherent_expand() {
 		),
 		vec![
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+				call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 0 }),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 0 }),
 				signature: None,
 			},
 		],
@@ -849,11 +864,11 @@ fn inherent_expand() {
 		),
 		vec![
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+				call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 0, bar: 0 }),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 0, bar: 0 }),
 				signature: None,
 			},
 		],
@@ -870,7 +885,7 @@ fn inherent_expand() {
 			Digest::default(),
 		),
 		vec![UncheckedExtrinsic {
-			function: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
+			call: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
 			signature: None,
 		}],
 	);
@@ -888,8 +903,8 @@ fn inherent_expand() {
 			Digest::default(),
 		),
 		vec![UncheckedExtrinsic {
-			function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
-			signature: Some((1, (), ())),
+			call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+			signature: Some((1, Default::default())),
 		}],
 	);
 
@@ -907,11 +922,11 @@ fn inherent_expand() {
 		),
 		vec![
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
+				call: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
 				signature: None,
 			},
 		],
@@ -929,15 +944,15 @@ fn inherent_expand() {
 		),
 		vec![
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
+				call: RuntimeCall::Example(pallet::Call::foo_storage_layer { foo: 0 }),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+				call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
 				signature: None,
 			},
 		],
@@ -955,15 +970,15 @@ fn inherent_expand() {
 		),
 		vec![
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 1 }),
 				signature: None,
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 0 }),
-				signature: Some((1, (), ())),
+				call: RuntimeCall::Example(pallet::Call::foo { foo: 1, bar: 0 }),
+				signature: Some((1, Default::default())),
 			},
 			UncheckedExtrinsic {
-				function: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
+				call: RuntimeCall::Example(pallet::Call::foo_no_post_info {}),
 				signature: None,
 			},
 		],
@@ -1278,7 +1293,7 @@ fn migrate_from_pallet_version_to_storage_version() {
 		assert!(sp_io::storage::get(&pallet_version_key(System::name())).is_none());
 
 		assert_eq!(Example::on_chain_storage_version(), pallet::STORAGE_VERSION);
-		assert_eq!(Example2::on_chain_storage_version(), StorageVersion::new(0));
+		assert_eq!(Example2::on_chain_storage_version(), pallet2::STORAGE_VERSION);
 		assert_eq!(System::on_chain_storage_version(), StorageVersion::new(0));
 	});
 }
@@ -2040,6 +2055,95 @@ fn test_storage_alias() {
 	})
 }
 
+#[cfg(feature = "try-runtime")]
+#[test]
+fn post_runtime_upgrade_detects_storage_version_issues() {
+	use frame_support::traits::UpgradeCheckSelect;
+
+	struct CustomUpgrade;
+
+	impl OnRuntimeUpgrade for CustomUpgrade {
+		fn on_runtime_upgrade() -> Weight {
+			Example2::current_storage_version().put::<Example2>();
+
+			Default::default()
+		}
+	}
+
+	struct CustomUpgradePallet4;
+
+	impl OnRuntimeUpgrade for CustomUpgradePallet4 {
+		fn on_runtime_upgrade() -> Weight {
+			StorageVersion::new(100).put::<Example4>();
+
+			Default::default()
+		}
+	}
+
+	type Executive = frame_executive::Executive<
+		Runtime,
+		Block,
+		frame_system::ChainContext<Runtime>,
+		Runtime,
+		AllPalletsWithSystem,
+	>;
+
+	type ExecutiveWithUpgrade = frame_executive::Executive<
+		Runtime,
+		Block,
+		frame_system::ChainContext<Runtime>,
+		Runtime,
+		AllPalletsWithSystem,
+		CustomUpgrade,
+	>;
+
+	type ExecutiveWithUpgradePallet4 = frame_executive::Executive<
+		Runtime,
+		Block,
+		frame_system::ChainContext<Runtime>,
+		Runtime,
+		AllPalletsWithSystem,
+		CustomUpgradePallet4,
+	>;
+
+	TestExternalities::default().execute_with(|| {
+		// Call `on_genesis` to put the storage version of `Example` into the storage.
+		Example::on_genesis();
+		// The version isn't changed, we should detect it.
+		assert!(Executive::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost)
+			.unwrap_err()
+			.contains("On chain and current storage version do not match"));
+	});
+
+	TestExternalities::default().execute_with(|| {
+		// Call `on_genesis` to put the storage version of `Example` into the storage.
+		Example::on_genesis();
+		// We set the new storage version in the pallet and that should be detected.
+		UpdateStorageVersion::set(&true);
+		Executive::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost).unwrap();
+	});
+
+	TestExternalities::default().execute_with(|| {
+		// Call `on_genesis` to put the storage version of `Example` into the storage.
+		Example::on_genesis();
+		// We set the new storage version in the custom upgrade and that should be detected.
+		ExecutiveWithUpgrade::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost).unwrap();
+	});
+
+	TestExternalities::default().execute_with(|| {
+		// Call `on_genesis` to put the storage version of `Example` into the storage.
+		Example::on_genesis();
+		// We need to set the correct storage version for `Example2`
+		UpdateStorageVersion::set(&true);
+
+		// `CustomUpgradePallet4` will set a storage version for `Example4` while this doesn't has
+		// any storage version "enabled".
+		assert!(ExecutiveWithUpgradePallet4::try_runtime_upgrade(UpgradeCheckSelect::PreAndPost)
+			.unwrap_err()
+			.contains("On chain storage version set, while the pallet doesn't"));
+	});
+}
+
 #[test]
 fn test_dispatch_context() {
 	TestExternalities::default().execute_with(|| {

substrate/frame/support/test/tests/pallet_ui/compare_unset_storage_version.rs

@@ -0,0 +1,27 @@
+#[frame_support::pallet]
+mod pallet {
+	use frame_support::pallet_prelude::*;
+	use frame_system::pallet_prelude::*;
+
+	#[pallet::config]
+	pub trait Config: frame_system::Config {}
+
+	#[pallet::pallet]
+	pub struct Pallet<T>(core::marker::PhantomData<T>);
+
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_runtime_upgrade() -> Weight {
+			if Self::current_storage_version() != Self::on_chain_storage_version() {
+
+			}
+
+			Default::default()
+		}
+	}
+
+	#[pallet::call]
+	impl<T: Config> Pallet<T> {}
+}
+
+fn main() {}

substrate/frame/support/test/tests/pallet_ui/compare_unset_storage_version.stderr

@@ -0,0 +1,7 @@
+error[E0369]: binary operation `!=` cannot be applied to type `NoStorageVersionSet`
+  --> tests/pallet_ui/compare_unset_storage_version.rs:15:39
+   |
+15 |             if Self::current_storage_version() != Self::on_chain_storage_version() {
+   |                ------------------------------- ^^ -------------------------------- StorageVersion
+   |                |
+   |                NoStorageVersionSet

substrate/frame/support/test/tests/runtime_metadata.rs

@@ -218,5 +218,5 @@ fn runtime_metadata() {
 
 	let rt = Runtime;
 	let runtime_metadata = (&rt).runtime_metadata();
-	assert_eq!(runtime_metadata, expected_runtime_metadata);
+	pretty_assertions::assert_eq!(runtime_metadata, expected_runtime_metadata);
 }