Create benchmark for the system::set_code instrisic (paritytech#13373)

* Still WIP # Conflicts: # frame/system/src/weights.rs * Still WIP * Add benchmark for system::set_code intrinsic fixes paritytech#13192 * Fix format * Add missing benchmark runtime * Fix lint warning * Consume the rest of the block and add test verification after the benchmark * Rewrite set_code function * Try to fix benchmarks and tests * Remove weight tags * Update frame/system/src/tests.rs Co-authored-by: Bastian Köcher <git@kchr.de> * Register ReadRuntimeVersionExt for benches Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fix tests * Fix deprecations * Fix deprecations * ".git/.scripts/commands/bench/bench.sh" pallet dev frame_system * Add update info and remove obsolete complexity comments. * ".git/.scripts/commands/fmt/fmt.sh" * Update frame/system/src/lib.rs Co-authored-by: Bastian Köcher <git@kchr.de> * Update frame/system/src/lib.rs Co-authored-by: Bastian Köcher <git@kchr.de> * Update frame/system/src/lib.rs Co-authored-by: Bastian Köcher <git@kchr.de> * Update frame/system/src/lib.rs Co-authored-by: Bastian Köcher <git@kchr.de> * Update frame/system/benchmarking/src/lib.rs Co-authored-by: Bastian Köcher <git@kchr.de> * ".git/.scripts/commands/fmt/fmt.sh" * Update README.md Just trigger CI rebuild * Update README.md Trigger CI --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: command-bot <>
nathanwhit · Jul 19, 2023 · 05cc5b8 · 05cc5b8
1 parent e7d9934
commit 05cc5b8
Show file tree

Hide file tree

Showing 11 changed files with 148 additions and 75 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/frame/system/Cargo.toml b/frame/system/Cargo.toml
@@ -13,6 +13,7 @@ readme = "README.md"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+cfg-if = "1.0"
 codec = { package = "parity-scale-codec", version = "3.2.2", default-features = false, features = ["derive"] }
 log = { version = "0.4.17", default-features = false }
 scale-info = { version = "2.5.0", default-features = false, features = ["derive"] }

diff --git a/frame/system/benchmarking/Cargo.toml b/frame/system/benchmarking/Cargo.toml
@@ -24,6 +24,8 @@ sp-std = { version = "5.0.0", default-features = false, path = "../../../primiti
 
 [dev-dependencies]
 sp-io = { version = "7.0.0", path = "../../../primitives/io" }
+sp-externalities = { version = "0.13.0", path = "../../../primitives/externalities" }
+sp-version = { version = "5.0.0", path = "../../../primitives/version" }
 
 [features]
 default = ["std"]

diff --git a/frame/system/benchmarking/res/README.md b/frame/system/benchmarking/res/README.md
@@ -0,0 +1,5 @@
+These runtimes are used for benchmarking the `set_code` intrinsic.
+
+**Don't use them in production environments!**
+
+To update the just copy the new runtime from `target/release/wbuild/kitchensink-runtime/kitchensink_runtime.compact.compressed.wasm` to here.
diff --git a/frame/system/benchmarking/res/kitchensink_runtime.compact.compressed.wasm b/frame/system/benchmarking/res/kitchensink_runtime.compact.compressed.wasm
diff --git a/frame/system/benchmarking/src/lib.rs b/frame/system/benchmarking/src/lib.rs
@@ -48,9 +48,12 @@ benchmarks! {
 	set_heap_pages {
 	}: _(RawOrigin::Root, Default::default())
 
-	// `set_code` was not benchmarked because it is pretty hard to come up with a real
-	// Wasm runtime to test the upgrade with. But this is okay because we will make
-	// `set_code` take a full block anyway.
+	set_code {
+		let runtime_blob = include_bytes!("../res/kitchensink_runtime.compact.compressed.wasm").to_vec();
+	}: _(RawOrigin::Root, runtime_blob)
+	verify {
+		System::<T>::assert_last_event(frame_system::Event::<T>::CodeUpdated.into());
+	}
 
 	#[extra]
 	set_code_without_checks {

diff --git a/frame/system/benchmarking/src/mock.rs b/frame/system/benchmarking/src/mock.rs
@@ -19,6 +19,7 @@
 
 #![cfg(test)]
 
+use codec::Encode;
 use sp_runtime::traits::IdentityLookup;
 
 type AccountId = u64;
@@ -67,7 +68,29 @@ impl frame_system::Config for Test {
 
 impl crate::Config for Test {}
 
+struct MockedReadRuntimeVersion(Vec<u8>);
+
+impl sp_core::traits::ReadRuntimeVersion for MockedReadRuntimeVersion {
+	fn read_runtime_version(
+		&self,
+		_wasm_code: &[u8],
+		_ext: &mut dyn sp_externalities::Externalities,
+	) -> Result<Vec<u8>, String> {
+		Ok(self.0.clone())
+	}
+}
+
 pub fn new_test_ext() -> sp_io::TestExternalities {
 	let t = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
-	sp_io::TestExternalities::new(t)
+
+	let version = sp_version::RuntimeVersion {
+		spec_name: "spec_name".into(),
+		spec_version: 123,
+		impl_version: 456,
+		..Default::default()
+	};
+	let read_runtime_version = MockedReadRuntimeVersion(version.encode());
+	let mut ext = sp_io::TestExternalities::new(t);
+	ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(read_runtime_version));
+	ext
 }
diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
@@ -373,7 +373,6 @@ pub mod pallet {
 	impl<T: Config> Pallet<T> {
 		/// Make some on-chain remark.
 		///
-		/// ## Complexity
 		/// - `O(1)`
 		#[pallet::call_index(0)]
 		#[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))]
@@ -393,31 +392,26 @@ pub mod pallet {
 		}
 
 		/// Set the new runtime code.
-		///
-		/// ## Complexity
-		/// - `O(C + S)` where `C` length of `code` and `S` complexity of `can_set_code`
 		#[pallet::call_index(2)]
-		#[pallet::weight((T::BlockWeights::get().max_block, DispatchClass::Operational))]
+		#[pallet::weight((T::SystemWeightInfo::set_code(), DispatchClass::Operational))]
 		pub fn set_code(origin: OriginFor<T>, code: Vec<u8>) -> DispatchResultWithPostInfo {
 			ensure_root(origin)?;
 			Self::can_set_code(&code)?;
 			T::OnSetCode::set_code(code)?;
-			Ok(().into())
+			// consume the rest of the block to prevent further transactions
+			Ok(Some(T::BlockWeights::get().max_block).into())
 		}
 
 		/// Set the new runtime code without doing any checks of the given `code`.
-		///
-		/// ## Complexity
-		/// - `O(C)` where `C` length of `code`
 		#[pallet::call_index(3)]
-		#[pallet::weight((T::BlockWeights::get().max_block, DispatchClass::Operational))]
+		#[pallet::weight((T::SystemWeightInfo::set_code(), DispatchClass::Operational))]
 		pub fn set_code_without_checks(
 			origin: OriginFor<T>,
 			code: Vec<u8>,
 		) -> DispatchResultWithPostInfo {
 			ensure_root(origin)?;
 			T::OnSetCode::set_code(code)?;
-			Ok(().into())
+			Ok(Some(T::BlockWeights::get().max_block).into())
 		}
 
 		/// Set some items of storage.
@@ -1414,9 +1408,6 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// Deposits a log and ensures it matches the block's log data.
-	///
-	/// ## Complexity
-	/// - `O(1)`
 	pub fn deposit_log(item: generic::DigestItem) {
 		<Digest<T>>::append(item);
 	}
@@ -1622,15 +1613,23 @@ impl<T: Config> Pallet<T> {
 			.and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok())
 			.ok_or(Error::<T>::FailedToExtractRuntimeVersion)?;
 
-		if new_version.spec_name != current_version.spec_name {
-			return Err(Error::<T>::InvalidSpecName.into())
-		}
+		cfg_if::cfg_if! {
+			 if #[cfg(all(feature = "runtime-benchmarks", not(test)))] {
+					// Let's ensure the compiler doesn't optimize our fetching of the runtime version away.
+					core::hint::black_box((new_version, current_version));
+					Ok(())
+			  } else {
+				  if new_version.spec_name != current_version.spec_name {
+					  return Err(Error::<T>::InvalidSpecName.into())
+				   }
 
-		if new_version.spec_version <= current_version.spec_version {
-			return Err(Error::<T>::SpecVersionNeedsToIncrease.into())
-		}
+				   if new_version.spec_version <= current_version.spec_version {
+						return Err(Error::<T>::SpecVersionNeedsToIncrease.into())
+				   }
 
-		Ok(())
+				   Ok(())
+			  }
+		}
 	}
 }
 

diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs
@@ -550,7 +550,12 @@ fn set_code_checks_works() {
 		("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(PostDispatchInfo::default())),
+		(
+			"test",
+			2,
+			1,
+			Ok(Some(<mock::Test as pallet::Config>::BlockWeights::get().max_block).into()),
+		),
 		("test", 0, 1, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
 		("test", 1, 0, Err(Error::<Test>::SpecVersionNeedsToIncrease)),
 	];