diff --git a/Cargo.lock b/Cargo.lock index e36f252ecb39..f0a133227b32 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6923,15 +6923,18 @@ dependencies = [ "parity-scale-codec", "paste", "rusty-fork", + "sc-client-db", "scale-info", "serde", "sp-api 26.0.0", "sp-application-crypto 30.0.0", "sp-core 28.0.0", + "sp-externalities 0.25.0", "sp-io 30.0.0", "sp-keystore 0.34.0", "sp-runtime 31.0.1", "sp-runtime-interface 24.0.0", + "sp-state-machine 0.35.0", "sp-storage 19.0.0", "static_assertions", ] diff --git a/prdoc/pr_6435.prdoc b/prdoc/pr_6435.prdoc new file mode 100644 index 000000000000..025c666d9115 --- /dev/null +++ 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 index aaa1398a13bc..cec981c05602 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -1180,7 +1180,7 @@ impl Backend { /// 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>, sp_database::ColumnId) { (self.storage.db.clone(), columns::STATE) } @@ -1188,7 +1188,7 @@ impl Backend { /// 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>> { self.storage.clone() } diff --git a/substrate/frame/benchmarking/Cargo.toml b/substrate/frame/benchmarking/Cargo.toml index 9ea350a1d290..0c74d94b33b8 100644 --- 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 index ecffbd1a018f..428f29e2bc16 100644 --- 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, n: u32) -> DispatchResult { let _sender = ensure_signed(origin)?; + assert!(n >= T::LowerBound::get()); Value::::put(n); Ok(()) } @@ -81,6 +82,7 @@ frame_support::construct_runtime!( { System: frame_system, TestPallet: pallet_test, + TestPallet2: pallet_test::, } ); @@ -117,6 +119,12 @@ impl pallet_test::Config for Test { type UpperBound = ConstU32<100>; } +impl pallet_test::Config 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 ( >::LowerBound::get() ) .. ( >::UpperBound::get() ); let caller = account::("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::::new(); + let config = crate::BenchmarkConfig { + pallet: "pallet_test".bytes().collect::>(), + // We only want that this `instance` is used. + // Otherwise the wrong components are used. + instance: "TestPallet".bytes().collect::>(), + benchmark: "set_value".bytes().collect::>(), + 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::>()) + } else { + None + } + }) + .unwrap(), + verify: false, + internal_repeats: 1, + }; + let params = (&config, &whitelist); + + let state = sc_client_db::BenchmarkingState::::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 index fb55cee99e81..3a10e43d83b8 100644 --- a/substrate/frame/benchmarking/src/utils.rs +++ b/substrate/frame/benchmarking/src/utils.rs @@ -200,6 +200,8 @@ impl From for BenchmarkError { pub struct BenchmarkConfig { /// The encoded name of the pallet to benchmark. pub pallet: Vec, + /// The encoded name of the pallet instance to benchmark. + pub instance: Vec, /// The encoded name of the benchmark/extrinsic to run. pub benchmark: Vec, /// 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 index e73ed1f4382f..64f93b22cf1b 100644 --- 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 index 32dba3436595..e9b4eb04ed51 100644 --- 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 index 6f7e79f16384..0c068fc585ba 100644 --- 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( + &, + sp_runtime::generic::UncheckedExtrinsic<(), (), (), ()>, + >, + > as sp_api::RuntimeApiInfo>::ID, + ) + .ok_or_else(|| ERROR_API_NOT_FOUND)?; + let (list, storage_info): (Vec, Vec) = 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 { + 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(), + ¶ms(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(), + ¶ms(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(), + ¶ms(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` 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