frame-benchmarking: Use correct components for pallet instances (#6435)

When using multiple instances of the same pallet, each instance was executed with the components of all instances. While actually each instance should only be executed with the components generated for the particular instance. The problem here was that in the runtime only the pallet-name was used to determine if a certain pallet should be benchmarked. When using instances, the pallet name is the same for both of these instances. The solution is to also take the instance name into account. The fix requires to change the `Benchmark` runtime api to also take the `instance`. The node side is written in a backwards compatible way to also support runtimes which do not yet support the `instance` parameter. --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: clangenb <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
paritytech · Nov 13, 2024 · ac2546b · ac2546b
1 parent e87bffc
commit ac2546b
Show file tree

Hide file tree

Showing 9 changed files with 155 additions and 34 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/prdoc/pr_6435.prdoc b/prdoc/pr_6435.prdoc
@@ -0,0 +1,16 @@
+title: 'frame-benchmarking: Use correct components for pallet instances'
+doc:
+- audience: Runtime Dev
+  description: |-
+    When benchmarking multiple instances of the same pallet, each instance was executed with the components of all instances. While actually each instance should only be executed with the components generated for the particular instance. The problem here was that in the runtime only the pallet-name was used to determine if a certain pallet should be benchmarked. When using instances, the pallet name is the same for both of these instances. The solution is to also take the instance name into account.
+
+    The fix requires to change the `Benchmark` runtime api to also take the `instance`. The node side is written in a backwards compatible way to also support runtimes which do not yet support the `instance` parameter.
+crates:
+- name: frame-benchmarking
+  bump: major
+- name: frame-benchmarking-cli
+  bump: major
+- name: sc-client-db
+  bump: none
+- name: pallet-referenda
+  bump: none
diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs
@@ -1180,15 +1180,15 @@ impl<Block: BlockT> Backend<Block> {
 	/// The second argument is the Column that stores the State.
 	///
 	/// Should only be needed for benchmarking.
-	#[cfg(any(feature = "runtime-benchmarks"))]
+	#[cfg(feature = "runtime-benchmarks")]
 	pub fn expose_db(&self) -> (Arc<dyn sp_database::Database<DbHash>>, sp_database::ColumnId) {
 		(self.storage.db.clone(), columns::STATE)
 	}
 
 	/// Expose the Storage that is used by this backend.
 	///
 	/// Should only be needed for benchmarking.
-	#[cfg(any(feature = "runtime-benchmarks"))]
+	#[cfg(feature = "runtime-benchmarks")]
 	pub fn expose_storage(&self) -> Arc<dyn sp_state_machine::Storage<HashingFor<Block>>> {
 		self.storage.clone()
 	}

diff --git a/substrate/frame/benchmarking/Cargo.toml b/substrate/frame/benchmarking/Cargo.toml
@@ -38,6 +38,9 @@ static_assertions = { workspace = true, default-features = true }
 array-bytes = { workspace = true, default-features = true }
 rusty-fork = { workspace = true }
 sp-keystore = { workspace = true, default-features = true }
+sc-client-db = { workspace = true }
+sp-state-machine = { workspace = true }
+sp-externalities = { workspace = true }
 
 [features]
 default = ["std"]
@@ -53,14 +56,17 @@ std = [
 	"sp-api/std",
 	"sp-application-crypto/std",
 	"sp-core/std",
+	"sp-externalities/std",
 	"sp-io/std",
 	"sp-keystore/std",
 	"sp-runtime-interface/std",
 	"sp-runtime/std",
+	"sp-state-machine/std",
 	"sp-storage/std",
 ]
 runtime-benchmarks = [
 	"frame-support/runtime-benchmarks",
 	"frame-system/runtime-benchmarks",
+	"sc-client-db/runtime-benchmarks",
 	"sp-runtime/runtime-benchmarks",
 ]
diff --git a/substrate/frame/benchmarking/src/tests_instance.rs b/substrate/frame/benchmarking/src/tests_instance.rs
@@ -61,6 +61,7 @@ mod pallet_test {
 		#[pallet::weight({0})]
 		pub fn set_value(origin: OriginFor<T>, n: u32) -> DispatchResult {
 			let _sender = ensure_signed(origin)?;
+			assert!(n >= T::LowerBound::get());
 			Value::<T, I>::put(n);
 			Ok(())
 		}
@@ -81,6 +82,7 @@ frame_support::construct_runtime!(
 	{
 		System: frame_system,
 		TestPallet: pallet_test,
+		TestPallet2: pallet_test::<Instance2>,
 	}
 );
 
@@ -117,6 +119,12 @@ impl pallet_test::Config for Test {
 	type UpperBound = ConstU32<100>;
 }
 
+impl pallet_test::Config<pallet_test::Instance2> for Test {
+	type RuntimeEvent = RuntimeEvent;
+	type LowerBound = ConstU32<50>;
+	type UpperBound = ConstU32<100>;
+}
+
 impl pallet_test::OtherConfig for Test {
 	type OtherEvent = RuntimeEvent;
 }
@@ -130,6 +138,7 @@ mod benchmarks {
 	use crate::account;
 	use frame_support::ensure;
 	use frame_system::RawOrigin;
+	use sp_core::Get;
 
 	// Additional used internally by the benchmark macro.
 	use super::pallet_test::{Call, Config, Pallet};
@@ -143,7 +152,7 @@ mod benchmarks {
 		}
 
 		set_value {
-			let b in 1 .. 1000;
+			let b in ( <T as Config<I>>::LowerBound::get() ) .. ( <T as Config<I>>::UpperBound::get() );
 			let caller = account::<T::AccountId>("caller", 0, 0);
 		}: _ (RawOrigin::Signed(caller), b.into())
 		verify {
@@ -173,3 +182,53 @@ mod benchmarks {
 		)
 	}
 }
+
+#[test]
+fn ensure_correct_instance_is_selected() {
+	use crate::utils::Benchmarking;
+
+	crate::define_benchmarks!(
+		[pallet_test, TestPallet]
+		[pallet_test, TestPallet2]
+	);
+
+	let whitelist = vec![];
+
+	let mut batches = Vec::<crate::BenchmarkBatch>::new();
+	let config = crate::BenchmarkConfig {
+		pallet: "pallet_test".bytes().collect::<Vec<_>>(),
+		// We only want that this `instance` is used.
+		// Otherwise the wrong components are used.
+		instance: "TestPallet".bytes().collect::<Vec<_>>(),
+		benchmark: "set_value".bytes().collect::<Vec<_>>(),
+		selected_components: TestPallet::benchmarks(false)
+			.into_iter()
+			.find_map(|b| {
+				if b.name == "set_value".as_bytes() {
+					Some(b.components.into_iter().map(|c| (c.0, c.1)).collect::<Vec<_>>())
+				} else {
+					None
+				}
+			})
+			.unwrap(),
+		verify: false,
+		internal_repeats: 1,
+	};
+	let params = (&config, &whitelist);
+
+	let state = sc_client_db::BenchmarkingState::<sp_runtime::traits::BlakeTwo256>::new(
+		Default::default(),
+		None,
+		false,
+		false,
+	)
+	.unwrap();
+
+	let mut overlay = Default::default();
+	let mut ext = sp_state_machine::Ext::new(&mut overlay, &state, None);
+	sp_externalities::set_and_run_with_externalities(&mut ext, || {
+		add_benchmarks!(params, batches);
+		Ok::<_, crate::BenchmarkError>(())
+	})
+	.unwrap();
+}
diff --git a/substrate/frame/benchmarking/src/utils.rs b/substrate/frame/benchmarking/src/utils.rs
@@ -200,6 +200,8 @@ impl From<DispatchError> for BenchmarkError {
 pub struct BenchmarkConfig {
 	/// The encoded name of the pallet to benchmark.
 	pub pallet: Vec<u8>,
+	/// The encoded name of the pallet instance to benchmark.
+	pub instance: Vec<u8>,
 	/// The encoded name of the benchmark/extrinsic to run.
 	pub benchmark: Vec<u8>,
 	/// The selected component values to use when running the benchmark.
@@ -229,6 +231,7 @@ pub struct BenchmarkMetadata {
 
 sp_api::decl_runtime_apis! {
 	/// Runtime api for benchmarking a FRAME runtime.
+	#[api_version(2)]
 	pub trait Benchmark {
 		/// Get the benchmark metadata available for this runtime.
 		///

diff --git a/substrate/frame/benchmarking/src/v1.rs b/substrate/frame/benchmarking/src/v1.rs
@@ -1821,12 +1821,13 @@ macro_rules! add_benchmark {
 		let (config, whitelist) = $params;
 		let $crate::BenchmarkConfig {
 			pallet,
+			instance,
 			benchmark,
 			selected_components,
 			verify,
 			internal_repeats,
 		} = config;
-		if &pallet[..] == &name_string[..] {
+		if &pallet[..] == &name_string[..] && &instance[..] == &instance_string[..] {
 			let benchmark_result = <$location>::run_benchmark(
 				&benchmark[..],
 				&selected_components[..],

diff --git a/substrate/frame/referenda/Cargo.toml b/substrate/frame/referenda/Cargo.toml
@@ -57,7 +57,6 @@ std = [
 ]
 runtime-benchmarks = [
 	"assert_matches",
-	"frame-benchmarking",
 	"frame-benchmarking/runtime-benchmarks",
 	"frame-support/runtime-benchmarks",
 	"frame-system/runtime-benchmarks",

diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
@@ -96,6 +96,7 @@ pub(crate) type PovModesMap =
 #[derive(Debug, Clone)]
 struct SelectedBenchmark {
 	pallet: String,
+	instance: String,
 	extrinsic: String,
 	components: Vec<(BenchmarkParameter, u32, u32)>,
 	pov_modes: Vec<(String, String)>,
@@ -152,7 +153,7 @@ fn combine_batches(
 }
 
 /// Explains possible reasons why the metadata for the benchmarking could not be found.
-const ERROR_METADATA_NOT_FOUND: &'static str = "Did not find the benchmarking metadata. \
+const ERROR_API_NOT_FOUND: &'static str = "Did not find the benchmarking runtime api. \
 This could mean that you either did not build the node correctly with the \
 `--features runtime-benchmarks` flag, or the chain spec that you are using was \
 not created by a node that was compiled with the flag";
@@ -306,6 +307,33 @@ impl PalletCmd {
 			.with_runtime_cache_size(2)
 			.build();
 
+		let runtime_version: sp_version::RuntimeVersion = Self::exec_state_machine(
+			StateMachine::new(
+				state,
+				&mut Default::default(),
+				&executor,
+				"Core_version",
+				&[],
+				&mut Self::build_extensions(executor.clone(), state.recorder()),
+				&runtime_code,
+				CallContext::Offchain,
+			),
+			"Could not find `Core::version` runtime api.",
+		)?;
+
+		let benchmark_api_version = runtime_version
+			.api_version(
+				&<dyn frame_benchmarking::Benchmark<
+					// We need to use any kind of `Block` type to make the compiler happy, not
+					// relevant for the `ID`.
+					sp_runtime::generic::Block<
+						sp_runtime::generic::Header<u32, Hasher>,
+						sp_runtime::generic::UncheckedExtrinsic<(), (), (), ()>,
+					>,
+				> as sp_api::RuntimeApiInfo>::ID,
+			)
+			.ok_or_else(|| ERROR_API_NOT_FOUND)?;
+
 		let (list, storage_info): (Vec<BenchmarkList>, Vec<StorageInfo>) =
 			Self::exec_state_machine(
 				StateMachine::new(
@@ -318,7 +346,7 @@ impl PalletCmd {
 					&runtime_code,
 					CallContext::Offchain,
 				),
-				ERROR_METADATA_NOT_FOUND,
+				ERROR_API_NOT_FOUND,
 			)?;
 
 		// Use the benchmark list and the user input to determine the set of benchmarks to run.
@@ -338,7 +366,7 @@ impl PalletCmd {
 		let pov_modes = Self::parse_pov_modes(&benchmarks_to_run)?;
 		let mut failed = Vec::<(String, String)>::new();
 
-		'outer: for (i, SelectedBenchmark { pallet, extrinsic, components, .. }) in
+		'outer: for (i, SelectedBenchmark { pallet, instance, extrinsic, components, .. }) in
 			benchmarks_to_run.clone().into_iter().enumerate()
 		{
 			log::info!(
@@ -392,7 +420,31 @@ impl PalletCmd {
 				}
 				all_components
 			};
+
 			for (s, selected_components) in all_components.iter().enumerate() {
+				let params = |verify: bool, repeats: u32| -> Vec<u8> {
+					if benchmark_api_version >= 2 {
+						(
+							pallet.as_bytes(),
+							instance.as_bytes(),
+							extrinsic.as_bytes(),
+							&selected_components.clone(),
+							verify,
+							repeats,
+						)
+							.encode()
+					} else {
+						(
+							pallet.as_bytes(),
+							extrinsic.as_bytes(),
+							&selected_components.clone(),
+							verify,
+							repeats,
+						)
+							.encode()
+					}
+				};
+
 				// First we run a verification
 				if !self.no_verify {
 					let state = &state_without_tracking;
@@ -407,14 +459,7 @@ impl PalletCmd {
 							&mut Default::default(),
 							&executor,
 							"Benchmark_dispatch_benchmark",
-							&(
-								pallet.as_bytes(),
-								extrinsic.as_bytes(),
-								&selected_components.clone(),
-								true, // run verification code
-								1,    // no need to do internal repeats
-							)
-								.encode(),
+							&params(true, 1),
 							&mut Self::build_extensions(executor.clone(), state.recorder()),
 							&runtime_code,
 							CallContext::Offchain,
@@ -447,14 +492,7 @@ impl PalletCmd {
 							&mut Default::default(),
 							&executor,
 							"Benchmark_dispatch_benchmark",
-							&(
-								pallet.as_bytes(),
-								extrinsic.as_bytes(),
-								&selected_components.clone(),
-								false, // don't run verification code for final values
-								self.repeat,
-							)
-								.encode(),
+							&params(false, self.repeat),
 							&mut Self::build_extensions(executor.clone(), state.recorder()),
 							&runtime_code,
 							CallContext::Offchain,
@@ -489,14 +527,7 @@ impl PalletCmd {
 							&mut Default::default(),
 							&executor,
 							"Benchmark_dispatch_benchmark",
-							&(
-								pallet.as_bytes(),
-								extrinsic.as_bytes(),
-								&selected_components.clone(),
-								false, // don't run verification code for final values
-								self.repeat,
-							)
-								.encode(),
+							&params(false, self.repeat),
 							&mut Self::build_extensions(executor.clone(), state.recorder()),
 							&runtime_code,
 							CallContext::Offchain,
@@ -571,6 +602,7 @@ impl PalletCmd {
 				{
 					benchmarks_to_run.push((
 						item.pallet.clone(),
+						item.instance.clone(),
 						benchmark.name.clone(),
 						benchmark.components.clone(),
 						benchmark.pov_modes.clone(),
@@ -581,13 +613,15 @@ impl PalletCmd {
 		// Convert `Vec<u8>` to `String` for better readability.
 		let benchmarks_to_run: Vec<_> = benchmarks_to_run
 			.into_iter()
-			.map(|(pallet, extrinsic, components, pov_modes)| {
-				let pallet = String::from_utf8(pallet.clone()).expect("Encoded from String; qed");
+			.map(|(pallet, instance, extrinsic, components, pov_modes)| {
+				let pallet = String::from_utf8(pallet).expect("Encoded from String; qed");
+				let instance = String::from_utf8(instance).expect("Encoded from String; qed");
 				let extrinsic =
 					String::from_utf8(extrinsic.clone()).expect("Encoded from String; qed");
 
 				SelectedBenchmark {
 					pallet,
+					instance,
 					extrinsic,
 					components,
 					pov_modes: pov_modes