Move runtime upgrade to frame-executive (#5197)

* Move runtime upgrade to `frame-executive`

Instead of storing the runtime upgraded in the space of `frame-system`,
this moves it to `frame-executive`. We also start storing the
`spec_version` and `impl_version` the last runtime upgrade was applied.

This scheme has multiple advantages:
- We don't need to make sure that runtime upgrade was set.
- This will work in the future when we want to test a runtime upgrade,
because the `on_runtime_upgrade` will be executed automatically when the
runtime changes.

* Move `LAST_RUNTIME_UPGRADE` key to `frame-executive`

* Add some more documentation

* Make sure `spec_version` always increases between runtime upgrades

* Fix test

* Upgrade `parity-multiaddr` to fix compilation

* Also check if the `spec_name` changed

* Remove `RuntimeUpgraded` storage entry

* Don't run on genesis

substrate/Cargo.lock

@@ -1500,6 +1500,7 @@ dependencies = [
  "sp-io",
  "sp-runtime",
  "sp-std",
+ "sp-version",
 ]
 
 [[package]]

substrate/client/rpc/src/state/tests.rs

@@ -402,7 +402,7 @@ fn should_return_runtime_version() {
 	let api = new_full(client.clone(), Subscriptions::new(Arc::new(core.executor())));
 
 	let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
-		\"specVersion\":1,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",2],\
+		\"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",2],\
 		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",1],[\"0x40fe3ad401f8959a\",4],\
 		[\"0xc6e9a76309f39b09\",1],[\"0xdd718d5cc53262d4\",1],[\"0xcbca25e39f142387\",1],\
 		[\"0xf78b278be53f454c\",2],[\"0xab3c0572291feb8b\",1],[\"0xbc9d89904f5b923f\",1]]}";

substrate/frame/executive/Cargo.toml

@@ -13,16 +13,17 @@ codec = { package = "parity-scale-codec", version = "1.2.0", default-features =
 frame-support = { version = "2.0.0-alpha.2", default-features = false, path = "../support" }
 frame-system = { version = "2.0.0-alpha.2", default-features = false, path = "../system" }
 serde = { version = "1.0.101", optional = true }
-sp-io ={ path = "../../primitives/io", default-features = false , version = "2.0.0-alpha.2"}
 sp-runtime = { version = "2.0.0-alpha.2", default-features = false, path = "../../primitives/runtime" }
 sp-std = { version = "2.0.0-alpha.2", default-features = false, path = "../../primitives/std" }
 
 [dev-dependencies]
 hex-literal = "0.2.1"
 sp-core = { version = "2.0.0-alpha.2", path = "../../primitives/core" }
+sp-io ={ path = "../../primitives/io", version = "2.0.0-alpha.2"}
 pallet-indices = { version = "2.0.0-alpha.2", path = "../indices" }
 pallet-balances = { version = "2.0.0-alpha.2", path = "../balances" }
 pallet-transaction-payment = { version = "2.0.0-alpha.2", path = "../transaction-payment" }
+sp-version = { version = "2.0.0-alpha.2", path = "../../primitives/version" }
 
 [features]
 default = ["std"]
@@ -31,7 +32,6 @@ std = [
 	"frame-support/std",
 	"frame-system/std",
 	"serde",
-	"sp-io/std",
 	"sp-runtime/std",
 	"sp-std/std",
 ]

substrate/frame/executive/src/lib.rs

@@ -75,10 +75,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use sp_std::{prelude::*, marker::PhantomData};
-use frame_support::{
-	storage::StorageValue,
-	weights::{GetDispatchInfo, WeighBlock, DispatchInfo}
-};
+use frame_support::{storage::StorageValue, weights::{GetDispatchInfo, WeighBlock, DispatchInfo}};
 use sp_runtime::{
 	generic::Digest, ApplyExtrinsicResult,
 	traits::{
@@ -179,7 +176,9 @@ where
 		extrinsics_root: &System::Hash,
 		digest: &Digest<System::Hash>,
 	) {
-		if frame_system::RuntimeUpgraded::take() {
+		if Self::runtime_upgraded() {
+			// System is not part of `AllModules`, so we need to call this manually.
+			<frame_system::Module::<System> as OnRuntimeUpgrade>::on_runtime_upgrade();
 			<AllModules as OnRuntimeUpgrade>::on_runtime_upgrade();
 			<frame_system::Module<System>>::register_extra_weight_unchecked(
 				<AllModules as WeighBlock<System::BlockNumber>>::on_runtime_upgrade()
@@ -201,6 +200,21 @@ where
 		);
 	}
 
+	/// Returns if the runtime was upgraded since the last time this function was called.
+	fn runtime_upgraded() -> bool {
+		let last = frame_system::LastRuntimeUpgrade::get();
+		let current = <System::Version as frame_support::traits::Get<_>>::get();
+
+		if last.map(|v| v.was_upgraded(&current)).unwrap_or(true) {
+			frame_system::LastRuntimeUpgrade::put(
+				frame_system::LastRuntimeUpgradeInfo::from(current),
+			);
+			true
+		} else {
+			false
+		}
+	}
+
 	fn initial_checks(block: &Block) {
 		let header = block.header();
 
@@ -376,7 +390,7 @@ mod tests {
 		weights::Weight,
 		traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReasons, WithdrawReason},
 	};
-	use frame_system::{self as system, Call as SystemCall, ChainContext};
+	use frame_system::{self as system, Call as SystemCall, ChainContext, LastRuntimeUpgradeInfo};
 	use pallet_balances::Call as BalancesCall;
 	use hex_literal::hex;
 
@@ -461,7 +475,7 @@ mod tests {
 		type MaximumBlockWeight = MaximumBlockWeight;
 		type AvailableBlockRatio = AvailableBlockRatio;
 		type MaximumBlockLength = MaximumBlockLength;
-		type Version = ();
+		type Version = RuntimeVersion;
 		type ModuleToIndex = ();
 		type AccountData = pallet_balances::AccountData<u64>;
 		type OnNewAccount = ();
@@ -507,6 +521,18 @@ mod tests {
 		}
 	}
 
+	pub struct RuntimeVersion;
+	impl frame_support::traits::Get<sp_version::RuntimeVersion> for RuntimeVersion {
+		fn get() -> sp_version::RuntimeVersion {
+			RUNTIME_VERSION.with(|v| v.borrow().clone())
+		}
+	}
+
+	thread_local! {
+		pub static RUNTIME_VERSION: std::cell::RefCell<sp_version::RuntimeVersion> =
+			Default::default();
+	}
+
 	type SignedExtra = (
 		frame_system::CheckEra<Runtime>,
 		frame_system::CheckNonce<Runtime>,
@@ -569,7 +595,7 @@ mod tests {
 				header: Header {
 					parent_hash: [69u8; 32].into(),
 					number: 1,
-					state_root: hex!("8a22606e925c39bb0c8e8f6f5871c0aceab88a2fcff6b2d92660af8f6daff0b1").into(),
+					state_root: hex!("e97d724f480f6e3215bd5c24b9ba51250e2514ac1c99e563fd77bfb9d6100b1c").into(),
 					extrinsics_root: hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").into(),
 					digest: Digest { logs: vec![], },
 				},
@@ -759,4 +785,76 @@ mod tests {
 			assert_eq!(<frame_system::Module<Runtime>>::all_extrinsics_weight(), 150 + 25);
 		})
 	}
+
+	#[test]
+	fn runtime_upgraded_should_work() {
+		new_test_ext(1).execute_with(|| {
+			RUNTIME_VERSION.with(|v| *v.borrow_mut() = Default::default());
+			// It should be added at genesis
+			assert!(frame_system::LastRuntimeUpgrade::exists());
+			assert!(!Executive::runtime_upgraded());
+
+			RUNTIME_VERSION.with(|v| *v.borrow_mut() = sp_version::RuntimeVersion {
+				spec_version: 1,
+				..Default::default()
+			});
+			assert!(Executive::runtime_upgraded());
+			assert_eq!(
+				Some(LastRuntimeUpgradeInfo { spec_version: 1.into(), spec_name: "".into() }),
+				frame_system::LastRuntimeUpgrade::get(),
+			);
+
+			RUNTIME_VERSION.with(|v| *v.borrow_mut() = sp_version::RuntimeVersion {
+				spec_version: 1,
+				spec_name: "test".into(),
+				..Default::default()
+			});
+			assert!(Executive::runtime_upgraded());
+			assert_eq!(
+				Some(LastRuntimeUpgradeInfo { spec_version: 1.into(), spec_name: "test".into() }),
+				frame_system::LastRuntimeUpgrade::get(),
+			);
+
+			RUNTIME_VERSION.with(|v| *v.borrow_mut() = sp_version::RuntimeVersion {
+				spec_version: 1,
+				spec_name: "test".into(),
+				impl_version: 2,
+				..Default::default()
+			});
+			assert!(!Executive::runtime_upgraded());
+
+			frame_system::LastRuntimeUpgrade::take();
+			assert!(Executive::runtime_upgraded());
+			assert_eq!(
+				Some(LastRuntimeUpgradeInfo { spec_version: 1.into(), spec_name: "test".into() }),
+				frame_system::LastRuntimeUpgrade::get(),
+			);
+		})
+	}
+
+	#[test]
+	fn last_runtime_upgrade_was_upgraded_works() {
+		let test_data = vec![
+			(0, "", 1, "", true),
+			(1, "", 1, "", false),
+			(1, "", 1, "test", true),
+			(1, "", 0, "", false),
+			(1, "", 0, "test", true),
+		];
+
+		for (spec_version, spec_name, c_spec_version, c_spec_name, result) in test_data {
+			let current = sp_version::RuntimeVersion {
+				spec_version: c_spec_version,
+				spec_name: c_spec_name.into(),
+				..Default::default()
+			};
+
+			let last = LastRuntimeUpgradeInfo {
+				spec_version: spec_version.into(),
+				spec_name: spec_name.into(),
+			};
+
+			assert_eq!(result, last.was_upgraded(&current));
+		}
+	}
 }

substrate/frame/system/src/lib.rs

@@ -307,6 +307,33 @@ pub struct AccountInfo<Index, AccountData> {
 	pub data: AccountData,
 }
 
+/// Stores the `spec_version` and `spec_name` of when the last runtime upgrade
+/// happened.
+#[derive(sp_runtime::RuntimeDebug, Encode, Decode)]
+#[cfg_attr(feature = "std", derive(PartialEq))]
+pub struct LastRuntimeUpgradeInfo {
+	pub spec_version: codec::Compact<u32>,
+	pub spec_name: sp_runtime::RuntimeString,
+}
+
+impl LastRuntimeUpgradeInfo {
+	/// Returns if the runtime was upgraded in comparison of `self` and `current`.
+	///
+	/// Checks if either the `spec_version` increased or the `spec_name` changed.
+	pub fn was_upgraded(&self, current: &sp_version::RuntimeVersion) -> bool {
+		current.spec_version > self.spec_version.0 || current.spec_name != self.spec_name
+	}
+}
+
+impl From<sp_version::RuntimeVersion> for LastRuntimeUpgradeInfo {
+	fn from(version: sp_version::RuntimeVersion) -> Self {
+		Self {
+			spec_version: version.spec_version.into(),
+			spec_name: version.spec_name,
+		}
+	}
+}
+
 decl_storage! {
 	trait Store for Module<T: Trait> as System {
 		/// The full account information for a particular account ID.
@@ -368,8 +395,8 @@ decl_storage! {
 		/// no notification will be triggered thus the event might be lost.
 		EventTopics get(fn event_topics): map hasher(blake2_256) T::Hash => Vec<(T::BlockNumber, EventIndex)>;
 
-		/// A bool to track if the runtime was upgraded last block.
-		pub RuntimeUpgraded: bool;
+		/// Stores the `spec_version` and `spec_name` of when the last runtime upgrade happened.
+		pub LastRuntimeUpgrade build(|_| Some(LastRuntimeUpgradeInfo::from(T::Version::get()))): Option<LastRuntimeUpgradeInfo>;
 	}
 	add_extra_genesis {
 		config(changes_trie_config): Option<ChangesTrieConfiguration>;
@@ -416,13 +443,7 @@ decl_error! {
 		InvalidSpecName,
 		/// The specification version is not allowed to decrease between the current runtime
 		/// and the new runtime.
-		SpecVersionNotAllowedToDecrease,
-		/// The implementation version is not allowed to decrease between the current runtime
-		/// and the new runtime.
-		ImplVersionNotAllowedToDecrease,
-		/// The specification or the implementation version need to increase between the
-		/// current runtime and the new runtime.
-		SpecOrImplVersionNeedToIncrease,
+		SpecVersionNeedsToIncrease,
 		/// Failed to extract the runtime version from the new runtime.
 		///
 		/// Either calling `Core_version` or decoding `RuntimeVersion` failed.
@@ -478,18 +499,11 @@ decl_module! {
 				Err(Error::<T>::InvalidSpecName)?
 			}
 
-			if new_version.spec_version < current_version.spec_version {
-				Err(Error::<T>::SpecVersionNotAllowedToDecrease)?
-			} else if new_version.spec_version == current_version.spec_version {
-				if new_version.impl_version < current_version.impl_version {
-					Err(Error::<T>::ImplVersionNotAllowedToDecrease)?
-				} else if new_version.impl_version == current_version.impl_version {
-					Err(Error::<T>::SpecOrImplVersionNeedToIncrease)?
-				}
+			if new_version.spec_version <= current_version.spec_version {
+				Err(Error::<T>::SpecVersionNeedsToIncrease)?
 			}
 
 			storage::unhashed::put_raw(well_known_keys::CODE, &code);
-			RuntimeUpgraded::put(true);
 			Self::deposit_event(RawEvent::CodeUpdated);
 		}
 
@@ -498,7 +512,6 @@ decl_module! {
 		pub fn set_code_without_checks(origin, code: Vec<u8>) {
 			ensure_root(origin)?;
 			storage::unhashed::put_raw(well_known_keys::CODE, &code);
-			RuntimeUpgraded::put(true);
 			Self::deposit_event(RawEvent::CodeUpdated);
 		}
 
@@ -526,9 +539,6 @@ decl_module! {
 			ensure_root(origin)?;
 			for i in &items {
 				storage::unhashed::put_raw(&i.0, &i.1);
-				if i.0 == well_known_keys::CODE {
-					RuntimeUpgraded::put(true);
-				}
 			}
 		}
 
@@ -558,6 +568,13 @@ decl_module! {
 			ensure!(account.data == T::AccountData::default(), Error::<T>::NonDefaultComposite);
 			Account::<T>::remove(who);
 		}
+
+		fn on_runtime_upgrade() {
+			// Remove the old `RuntimeUpgraded` storage entry.
+			let mut runtime_upgraded_key = sp_io::hashing::twox_128(b"System").to_vec();
+			runtime_upgraded_key.extend(&sp_io::hashing::twox_128(b"RuntimeUpgraded"));
+			sp_io::storage::clear(&runtime_upgraded_key);
+		}
 	}
 }
 
@@ -1935,12 +1952,12 @@ mod tests {
 		}
 
 		let test_data = vec![
-			("test", 1, 2, Ok(())),
-			("test", 1, 1, Err(Error::<Test>::SpecOrImplVersionNeedToIncrease)),
+			("test", 1, 2, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
+			("test", 1, 1, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
 			("test2", 1, 1, Err(Error::<Test>::InvalidSpecName)),
 			("test", 2, 1, Ok(())),
-			("test", 0, 1, Err(Error::<Test>::SpecVersionNotAllowedToDecrease)),
-			("test", 1, 0, Err(Error::<Test>::ImplVersionNotAllowedToDecrease)),
+			("test", 0, 1, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
+			("test", 1, 0, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
 		];
 
 		for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() {
@@ -1980,8 +1997,6 @@ mod tests {
 				System::events(),
 				vec![EventRecord { phase: Phase::ApplyExtrinsic(0), event: 102u16, topics: vec![] }],
 			);
-
-			assert_eq!(RuntimeUpgraded::get(), true);
 		});
 	}
 
@@ -1998,8 +2013,6 @@ mod tests {
 					substrate_test_runtime_client::runtime::WASM_BINARY.to_vec()
 				)],
 			).unwrap();
-
-			assert_eq!(RuntimeUpgraded::get(), true);
 		});
 	}
 }

substrate/primitives/storage/src/lib.rs

@@ -207,7 +207,7 @@ impl<'a> ChildInfo<'a> {
 		}
 	}
 
-	/// Create child info from a linear byte packed value and a given type. 
+	/// Create child info from a linear byte packed value and a given type.
 	pub fn resolve_child_info(child_type: u32, data: &'a[u8]) -> Option<Self> {
 		match child_type {
 			x if x == ChildType::CryptoUniqueId as u32 => Some(ChildInfo::new_default(data)),

substrate/test-utils/runtime/src/lib.rs

@@ -43,7 +43,7 @@ use sp_runtime::{
 	},
 };
 use sp_version::RuntimeVersion;
-pub use sp_core::{hash::H256};
+pub use sp_core::hash::H256;
 #[cfg(any(feature = "std", test))]
 use sp_version::NativeVersion;
 use frame_support::{impl_outer_origin, parameter_types, weights::Weight};
@@ -64,10 +64,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("test"),
 	impl_name: create_runtime_str!("parity-test"),
 	authoring_version: 1,
-	spec_version: 1,
-	#[cfg(feature = "std")]
-	impl_version: 1,
-	#[cfg(not(feature = "std"))]
+	spec_version: 2,
 	impl_version: 2,
 	apis: RUNTIME_API_VERSIONS,
 };