Add Authorize Upgrade Pattern to Frame System (paritytech#2682)

Adds the `authorize_upgrade` -> `enact_authorized_upgrade` pattern to
`frame-system`. This will be useful for upgrading bridged chains that
are under the governance of Polkadot without passing entire runtime Wasm
blobs over a bridge.

Notes:

- Changed `enact_authorized_upgrade` to `apply_authorized_upgrade`.
Personal opinion, "apply" more accurately expresses what it's doing. Can
change back if outvoted.
- Remove `check_version` in favor of two extrinsics, so as to make
_checked_ the default.
- Left calls in `parachain-system` and marked as deprecated to prevent
breaking the API. They just call into the `frame-system` functions.
- Updated `frame-system` benchmarks to v2 syntax.

---------

Co-authored-by: command-bot <>

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

@@ -94,6 +94,8 @@ fn module_error_outer_enum_expand_explicit() {
 			frame_system::Error::InvalidTask => (),
 			#[cfg(feature = "experimental")]
 			frame_system::Error::FailedTask => (),
+			frame_system::Error::NothingAuthorized => (),
+			frame_system::Error::Unauthorized => (),
 			frame_system::Error::__Ignore(_, _) => (),
 		},
 

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

@@ -94,6 +94,8 @@ fn module_error_outer_enum_expand_implicit() {
 			frame_system::Error::InvalidTask => (),
 			#[cfg(feature = "experimental")]
 			frame_system::Error::FailedTask => (),
+			frame_system::Error::NothingAuthorized => (),
+			frame_system::Error::Unauthorized => (),
 			frame_system::Error::__Ignore(_, _) => (),
 		},
 

substrate/frame/system/benchmarking/src/lib.rs

@@ -21,10 +21,7 @@
 #![cfg(feature = "runtime-benchmarks")]
 
 use codec::Encode;
-use frame_benchmarking::{
-	v1::{benchmarks, whitelisted_caller},
-	BenchmarkError,
-};
+use frame_benchmarking::{impl_benchmark_test_suite, v2::*};
 use frame_support::{dispatch::DispatchClass, storage, traits::Get};
 use frame_system::{Call, Pallet as System, RawOrigin};
 use sp_core::storage::well_known_keys;
@@ -55,69 +52,104 @@ pub trait Config: frame_system::Config {
 	}
 }
 
-benchmarks! {
-	remark {
-		let b in 0 .. *T::BlockLength::get().max.get(DispatchClass::Normal) as u32;
+#[benchmarks]
+mod benchmarks {
+	use super::*;
+
+	#[benchmark]
+	fn remark(
+		b: Linear<0, { *T::BlockLength::get().max.get(DispatchClass::Normal) as u32 }>,
+	) -> Result<(), BenchmarkError> {
 		let remark_message = vec![1; b as usize];
 		let caller = whitelisted_caller();
-	}: _(RawOrigin::Signed(caller), remark_message)
 
-	remark_with_event {
-		let b in 0 .. *T::BlockLength::get().max.get(DispatchClass::Normal) as u32;
+		#[extrinsic_call]
+		remark(RawOrigin::Signed(caller), remark_message);
+
+		Ok(())
+	}
+
+	#[benchmark]
+	fn remark_with_event(
+		b: Linear<0, { *T::BlockLength::get().max.get(DispatchClass::Normal) as u32 }>,
+	) -> Result<(), BenchmarkError> {
 		let remark_message = vec![1; b as usize];
-		let caller = whitelisted_caller();
-	}: _(RawOrigin::Signed(caller), remark_message)
+		let caller: T::AccountId = whitelisted_caller();
+		let hash = T::Hashing::hash(&remark_message[..]);
 
-	set_heap_pages {
-	}: _(RawOrigin::Root, Default::default())
+		#[extrinsic_call]
+		remark_with_event(RawOrigin::Signed(caller.clone()), remark_message);
 
-	set_code {
+		System::<T>::assert_last_event(
+			frame_system::Event::<T>::Remarked { sender: caller, hash }.into(),
+		);
+		Ok(())
+	}
+
+	#[benchmark]
+	fn set_heap_pages() -> Result<(), BenchmarkError> {
+		#[extrinsic_call]
+		set_heap_pages(RawOrigin::Root, Default::default());
+
+		Ok(())
+	}
+
+	#[benchmark]
+	fn set_code() -> Result<(), BenchmarkError> {
 		let runtime_blob = T::prepare_set_code_data();
 		T::setup_set_code_requirements(&runtime_blob)?;
-	}: _(RawOrigin::Root, runtime_blob)
-	verify {
-		T::verify_set_code()
+
+		#[extrinsic_call]
+		set_code(RawOrigin::Root, runtime_blob);
+
+		T::verify_set_code();
+		Ok(())
 	}
 
-	#[extra]
-	set_code_without_checks {
+	#[benchmark(extra)]
+	fn set_code_without_checks() -> Result<(), BenchmarkError> {
 		// Assume Wasm ~4MB
 		let code = vec![1; 4_000_000 as usize];
 		T::setup_set_code_requirements(&code)?;
-	}: _(RawOrigin::Root, code)
-	verify {
-		let current_code = storage::unhashed::get_raw(well_known_keys::CODE).ok_or("Code not stored.")?;
+
+		#[block]
+		{
+			System::<T>::set_code_without_checks(RawOrigin::Root.into(), code)?;
+		}
+
+		let current_code =
+			storage::unhashed::get_raw(well_known_keys::CODE).ok_or("Code not stored.")?;
 		assert_eq!(current_code.len(), 4_000_000 as usize);
+		Ok(())
 	}
 
-	#[skip_meta]
-	set_storage {
-		let i in 0 .. 1000;
-
+	#[benchmark(skip_meta)]
+	fn set_storage(i: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> {
 		// Set up i items to add
 		let mut items = Vec::new();
-		for j in 0 .. i {
+		for j in 0..i {
 			let hash = (i, j).using_encoded(T::Hashing::hash).as_ref().to_vec();
 			items.push((hash.clone(), hash.clone()));
 		}
 
 		let items_to_verify = items.clone();
-	}: _(RawOrigin::Root, items)
-	verify {
+
+		#[extrinsic_call]
+		set_storage(RawOrigin::Root, items);
+
 		// Verify that they're actually in the storage.
 		for (item, _) in items_to_verify {
 			let value = storage::unhashed::get_raw(&item).ok_or("No value stored")?;
 			assert_eq!(value, *item);
 		}
+		Ok(())
 	}
 
-	#[skip_meta]
-	kill_storage {
-		let i in 0 .. 1000;
-
+	#[benchmark(skip_meta)]
+	fn kill_storage(i: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> {
 		// Add i items to storage
 		let mut items = Vec::with_capacity(i as usize);
-		for j in 0 .. i {
+		for j in 0..i {
 			let hash = (i, j).using_encoded(T::Hashing::hash).as_ref().to_vec();
 			storage::unhashed::put_raw(&hash, &hash);
 			items.push(hash);
@@ -130,22 +162,23 @@ benchmarks! {
 		}
 
 		let items_to_verify = items.clone();
-	}: _(RawOrigin::Root, items)
-	verify {
+
+		#[extrinsic_call]
+		kill_storage(RawOrigin::Root, items);
+
 		// Verify that they're not in the storage anymore.
 		for item in items_to_verify {
 			assert!(storage::unhashed::get_raw(&item).is_none());
 		}
+		Ok(())
 	}
 
-	#[skip_meta]
-	kill_prefix {
-		let p in 0 .. 1000;
-
+	#[benchmark(skip_meta)]
+	fn kill_prefix(p: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> {
 		let prefix = p.using_encoded(T::Hashing::hash).as_ref().to_vec();
 		let mut items = Vec::with_capacity(p as usize);
 		// add p items that share a prefix
-		for i in 0 .. p {
+		for i in 0..p {
 			let hash = (p, i).using_encoded(T::Hashing::hash).as_ref().to_vec();
 			let key = [&prefix[..], &hash[..]].concat();
 			storage::unhashed::put_raw(&key, &key);
@@ -157,12 +190,45 @@ benchmarks! {
 			let value = storage::unhashed::get_raw(item).ok_or("No value stored")?;
 			assert_eq!(value, *item);
 		}
-	}: _(RawOrigin::Root, prefix, p)
-	verify {
+
+		#[extrinsic_call]
+		kill_prefix(RawOrigin::Root, prefix, p);
+
 		// Verify that they're not in the storage anymore.
 		for item in items {
 			assert!(storage::unhashed::get_raw(&item).is_none());
 		}
+		Ok(())
+	}
+
+	#[benchmark]
+	fn authorize_upgrade() -> Result<(), BenchmarkError> {
+		let runtime_blob = T::prepare_set_code_data();
+		T::setup_set_code_requirements(&runtime_blob)?;
+		let hash = T::Hashing::hash(&runtime_blob);
+
+		#[extrinsic_call]
+		authorize_upgrade(RawOrigin::Root, hash);
+
+		assert!(System::<T>::authorized_upgrade().is_some());
+		Ok(())
+	}
+
+	#[benchmark]
+	fn apply_authorized_upgrade() -> Result<(), BenchmarkError> {
+		let runtime_blob = T::prepare_set_code_data();
+		T::setup_set_code_requirements(&runtime_blob)?;
+		let hash = T::Hashing::hash(&runtime_blob);
+		// Will be heavier when it needs to do verification (i.e. don't use `...without_checks`).
+		System::<T>::authorize_upgrade(RawOrigin::Root.into(), hash)?;
+
+		#[extrinsic_call]
+		apply_authorized_upgrade(RawOrigin::Root, runtime_blob);
+
+		// Can't check for `CodeUpdated` in parachain upgrades. Just check that the authorization is
+		// gone.
+		assert!(System::<T>::authorized_upgrade().is_none());
+		Ok(())
 	}
 
 	impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test);