From 3631111f6396b43cf73db5bf7de3a9195906afa9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Sep 2024 19:34:36 +0200 Subject: [PATCH 01/19] Set genesis state after each benchmark run Signed-off-by: Oliver Tale-Yazdi --- .../benchmarking-cli/src/pallet/command.rs | 89 ++++++++++++------- .../frame/benchmarking-cli/src/pallet/mod.rs | 7 ++ .../benchmarking-cli/src/pallet/writer.rs | 7 +- 3 files changed, 72 insertions(+), 31 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 471919815206..3f716f6bc574 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -42,7 +42,7 @@ use sp_core::{ use sp_externalities::Extensions; use sp_genesis_builder::{PresetId, Result as GenesisBuildResult}; use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; -use sp_runtime::traits::Hash; +use sp_runtime::{traits::Hash, RuntimeString}; use sp_state_machine::{OverlayedChanges, StateMachine}; use sp_trie::{proof_size_extension::ProofSizeExt, recorder::Recorder}; use sp_wasm_interface::HostFunctions; @@ -162,9 +162,6 @@ generate the genesis state is deprecated. Please remove the `--chain`/`--dev`/`- point `--runtime` to your runtime blob and set `--genesis-builder=runtime`. This warning may \ become a hard error any time after December 2024."; -/// The preset that we expect to find in the GenesisBuilder runtime API. -const GENESIS_PRESET: &str = "development"; - impl PalletCmd { /// Runs the command and benchmarks a pallet. #[deprecated( @@ -214,9 +211,7 @@ impl PalletCmd { return self.output_from_results(&batches) } - let (genesis_storage, genesis_changes) = - self.genesis_storage::(&chain_spec)?; - let mut changes = genesis_changes.clone(); + let genesis_storage = self.genesis_storage::(&chain_spec)?; let cache_size = Some(self.database_cache_size as usize); let state_with_tracking = BenchmarkingState::::new( @@ -262,7 +257,7 @@ impl PalletCmd { Self::exec_state_machine( StateMachine::new( state, - &mut changes, + &mut Default::default(), &executor, "Benchmark_benchmark_metadata", &(self.extra).encode(), @@ -347,7 +342,6 @@ impl PalletCmd { for (s, selected_components) in all_components.iter().enumerate() { // First we run a verification if !self.no_verify { - let mut changes = genesis_changes.clone(); let state = &state_without_tracking; // Don't use these results since verification code will add overhead. let _batch: Vec = match Self::exec_state_machine::< @@ -357,7 +351,7 @@ impl PalletCmd { >( StateMachine::new( state, - &mut changes, + &mut Default::default(), &executor, "Benchmark_dispatch_benchmark", &( @@ -389,7 +383,6 @@ impl PalletCmd { } // Do one loop of DB tracking. { - let mut changes = genesis_changes.clone(); let state = &state_with_tracking; let batch: Vec = match Self::exec_state_machine::< std::result::Result, String>, @@ -398,7 +391,7 @@ impl PalletCmd { >( StateMachine::new( state, // todo remove tracking - &mut changes, + &mut Default::default(), &executor, "Benchmark_dispatch_benchmark", &( @@ -432,7 +425,6 @@ impl PalletCmd { } // Finally run a bunch of loops to get extrinsic timing information. for r in 0..self.external_repeat { - let mut changes = genesis_changes.clone(); let state = &state_without_tracking; let batch = match Self::exec_state_machine::< std::result::Result, String>, @@ -441,7 +433,7 @@ impl PalletCmd { >( StateMachine::new( state, // todo remove tracking - &mut changes, + &mut Default::default(), &executor, "Benchmark_dispatch_benchmark", &( @@ -567,16 +559,13 @@ impl PalletCmd { Ok(benchmarks_to_run) } - /// Produce a genesis storage and genesis changes. + /// Build the genesis storage by either the Genesis Builder API, chain spec or nothing. /// - /// It would be easier to only return one type, but there is no easy way to convert them. - // TODO: Re-write `BenchmarkingState` to not be such a clusterfuck and only accept - // `OverlayedChanges` instead of a mix between `OverlayedChanges` and `State`. But this can only - // be done once we deprecated and removed the legacy interface :( + /// Behaviour can be controlled by the `--genesis-builder` flag. fn genesis_storage( &self, chain_spec: &Option>, - ) -> Result<(sp_storage::Storage, OverlayedChanges)> { + ) -> Result { Ok(match (self.genesis_builder, self.runtime.is_some()) { (Some(GenesisBuilder::None), _) => Default::default(), (Some(GenesisBuilder::Spec), _) | (None, false) => { @@ -589,13 +578,34 @@ impl PalletCmd { .build_storage() .map_err(|e| format!("{ERROR_CANNOT_BUILD_GENESIS}\nError: {e}"))?; - (storage, Default::default()) + storage }, (Some(GenesisBuilder::Runtime), _) | (None, true) => - (Default::default(), self.genesis_from_runtime::()?), + self.genesis_from_runtime::().and_then(Self::changes_to_storage)?, }) } + /// Convert some overlayed changes to a storage. + fn changes_to_storage( + mut changes: OverlayedChanges, + ) -> Result { + let mut top = BTreeMap::, Vec>::new(); + // The backend state here does not matter: + let state = BenchmarkingState::::new(Default::default(), None, false, false)?; + + let changes = changes.drain_storage_changes(&state, sp_storage::StateVersion::V1)?; + + for (k, v) in changes.main_storage_changes { + if let Some(v) = v { + top.insert(k, v); + } else { + top.remove(&k); + } + } + + Ok(sp_storage::Storage { top, children_default: Default::default() }) + } + /// Generate the genesis changeset by the runtime API. fn genesis_from_runtime(&self) -> Result> { let state = BenchmarkingState::::new( @@ -643,13 +653,14 @@ impl PalletCmd { let base_genesis_json = serde_json::from_slice::(&base_genesis_json) .map_err(|e| format!("GenesisBuilder::get_preset returned invalid JSON: {:?}", e))?; + let preset_name = RuntimeString::Owned(self.genesis_builder_preset.clone()); let dev_genesis_json: Option> = Self::exec_state_machine( StateMachine::new( &state, &mut Default::default(), &executor, "GenesisBuilder_get_preset", - &Some::(GENESIS_PRESET.into()).encode(), // Use the default preset + &Some::(preset_name).encode(), &mut Self::build_extensions(executor.clone(), state.recorder()), &runtime_code, CallContext::Offchain, @@ -666,9 +677,10 @@ impl PalletCmd { })?; json_merge(&mut genesis_json, dev); } else { - log::warn!( - "Could not find genesis preset '{GENESIS_PRESET}'. Falling back to default." - ); + return Err(format!( + "GenesisBuilder::get_preset returned no data for preset `{}`. Manually specify `--genesis-builder-preset` or use a different `--genesis-builder`.", + self.genesis_builder_preset).into() + ) } let json_pretty_str = serde_json::to_string_pretty(&genesis_json) @@ -738,8 +750,17 @@ impl PalletCmd { state: &'a BenchmarkingState, ) -> Result, H>> { if let Some(runtime) = &self.runtime { + let runtime = std::path::absolute(runtime) + .map_err(|e| format!("Could not get absolute path for runtime file: {e}"))?; log::info!("Loading WASM from {}", runtime.display()); - let code = fs::read(runtime)?; + + let code = fs::read(&runtime).map_err(|e| { + format!( + "Could not load runtime file from path: {}, error: {}", + runtime.display(), + e + ) + })?; let hash = sp_core::blake2_256(&code).to_vec(); let wrapped_code = WrappedRuntimeCode(Cow::Owned(code)); @@ -990,19 +1011,27 @@ impl PalletCmd { if let Some(output_path) = &self.output { if !output_path.is_dir() && output_path.file_name().is_none() { - return Err("Output file or path is invalid!".into()) + return Err(format!( + "Output path is neither a directory nor a file: {:?}", + output_path + ) + .into()) } } if let Some(header_file) = &self.header { if !header_file.is_file() { - return Err("Header file is invalid!".into()) + return Err(format!("Header file could not be found: {:?}", &header_file).into()) }; } if let Some(handlebars_template_file) = &self.template { if !handlebars_template_file.is_file() { - return Err("Handlebars template file is invalid!".into()) + return Err(format!( + "Handlebars template file could not be found: {:?}", + &handlebars_template_file + ) + .into()) }; } diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs index ebf737be1dbf..36418d130d40 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs @@ -181,6 +181,13 @@ pub struct PalletCmd { #[arg(long, value_enum)] pub genesis_builder: Option, + /// The preset that we expect to find in the GenesisBuilder runtime API. + /// + /// This can be useful when a runtime has a dedicated benchmarking preset instead of using the + /// default one. + #[arg(long, default_value = "development")] + pub genesis_builder_preset: String, + /// DEPRECATED: This argument has no effect. #[arg(long = "execution")] pub execution: Option, diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs index df7d81b2822e..d4742361e9e2 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs @@ -484,7 +484,12 @@ pub(crate) fn write_results( benchmarks: results.clone(), }; - let mut output_file = fs::File::create(&file_path)?; + let file_path = std::path::absolute(&file_path).map_err(|e| { + format!("Could not get absolute path for: {:?}. Error: {:?}", &file_path, e) + })?; + let mut output_file = fs::File::create(&file_path).map_err(|e| { + format!("Could not write weight file to: {:?}. Error: {:?}", &file_path, e) + })?; handlebars .render_template_to_write(&template, &hbs_data, &mut output_file) .map_err(|e| io_error(&e.to_string()))?; From a2cc75a3b658d942e636a84634bb412cae64b174 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Sep 2024 20:07:47 +0200 Subject: [PATCH 02/19] Add test Signed-off-by: Oliver Tale-Yazdi --- .../benchmarking/pov/src/benchmarking.rs | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/substrate/frame/benchmarking/pov/src/benchmarking.rs b/substrate/frame/benchmarking/pov/src/benchmarking.rs index bf3d406d0b2b..4ceb050e6dfd 100644 --- a/substrate/frame/benchmarking/pov/src/benchmarking.rs +++ b/substrate/frame/benchmarking/pov/src/benchmarking.rs @@ -26,6 +26,10 @@ use frame_support::traits::UnfilteredDispatchable; use frame_system::{Pallet as System, RawOrigin}; use sp_runtime::traits::Hash; +frame_support::parameter_types! { + pub static StorageRootHash: Option> = None; +} + #[benchmarks] mod benchmarks { use super::*; @@ -392,6 +396,30 @@ mod benchmarks { } } + #[benchmark] + fn storage_root_is_the_same_every_time(i: Linear<0, 10>) { + let root = sp_io::storage::root(sp_runtime::StateVersion::V1); + + match (i, StorageRootHash::get()) { + (0, Some(_)) => panic!("StorageRootHash should be None initially"), + (0, None) => StorageRootHash::set(Some(root)), + (_, Some(r)) if r == root => {}, + (_, Some(r)) => + panic!("StorageRootHash should be the same every time: {:?} vs {:?}", r, root), + (_, None) => panic!("StorageRootHash should be Some after the first iteration"), + } + + // Also test that everything is reset correctly: + sp_io::storage::set(b"key1", b"value"); + + #[block] + { + sp_io::storage::set(b"key2", b"value"); + } + + sp_io::storage::set(b"key3", b"value"); + } + impl_benchmark_test_suite!(Pallet, super::mock::new_test_ext(), super::mock::Test,); } From 50ce303e5ed6fdef517ff2fa8c6fe6f7a3ddc750 Mon Sep 17 00:00:00 2001 From: ggwpez Date: Mon, 9 Sep 2024 18:15:11 +0000 Subject: [PATCH 03/19] Add PrDoc (auto generated) --- prdoc/pr_5655.prdoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 prdoc/pr_5655.prdoc diff --git a/prdoc/pr_5655.prdoc b/prdoc/pr_5655.prdoc new file mode 100644 index 000000000000..3efeb2621f14 --- /dev/null +++ b/prdoc/pr_5655.prdoc @@ -0,0 +1,15 @@ +title: '[benchmarking] Reset to genesis storage after each run' +doc: +- audience: Runtime Dev + description: |- + The genesis state is currently partially provided via `OverlayedChanges`, but these changes are reset by the runtime after the first repetition, causing the second repitition to use an invalid genesis state. + + Changes: + - Provide the genesis state as a `Storage` without any `OverlayedChanges` to make it work correctly with repetitions. + - Add `--genesis-builder-preset` option to use different genesis preset names. + - Improve error messages. +crates: +- name: frame-benchmarking-cli + bump: minor +- name: frame-benchmarking-pallet-pov + bump: minor From 6e87e579705496f7e633422520d3ccd17b4c53b7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 10 Sep 2024 16:40:14 +0200 Subject: [PATCH 04/19] Use DEV_RUNTIME_PRESET instead of hard-coding Signed-off-by: Oliver Tale-Yazdi --- substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs index 36418d130d40..057c2da88643 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs @@ -185,7 +185,7 @@ pub struct PalletCmd { /// /// This can be useful when a runtime has a dedicated benchmarking preset instead of using the /// default one. - #[arg(long, default_value = "development")] + #[arg(long, default_value = sp_genesis_builder::DEV_RUNTIME_PRESET)] pub genesis_builder_preset: String, /// DEPRECATED: This argument has no effect. From 9aa865fd0b139acbe24a3711848d00b16b6a4c2b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 10 Sep 2024 19:09:55 +0200 Subject: [PATCH 05/19] INSTALL WITH LOCKED FFS Signed-off-by: Oliver Tale-Yazdi --- .github/workflows/check-semver.yml | 2 +- .github/workflows/publish-check-crates.yml | 2 +- .github/workflows/publish-claim-crates.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/check-semver.yml b/.github/workflows/check-semver.yml index 15eb32f4062c..7fe6a8557ead 100644 --- a/.github/workflows/check-semver.yml +++ b/.github/workflows/check-semver.yml @@ -73,7 +73,7 @@ jobs: - name: install parity-publish # Set the target dir to cache the build. - run: CARGO_TARGET_DIR=./target/ cargo install parity-publish@0.8.0 -q + run: CARGO_TARGET_DIR=./target/ cargo install parity-publish@0.8.0 --locked -q - name: check semver run: | diff --git a/.github/workflows/publish-check-crates.yml b/.github/workflows/publish-check-crates.yml index 9f96b92e0ce7..498240395fb2 100644 --- a/.github/workflows/publish-check-crates.yml +++ b/.github/workflows/publish-check-crates.yml @@ -20,7 +20,7 @@ jobs: cache-on-failure: true - name: install parity-publish - run: cargo install parity-publish@0.8.0 + run: cargo install parity-publish@0.8.0 --locked -q - name: parity-publish check run: parity-publish --color always check --allow-unpublished diff --git a/.github/workflows/publish-claim-crates.yml b/.github/workflows/publish-claim-crates.yml index bee709a12076..5d39a5fdfb40 100644 --- a/.github/workflows/publish-claim-crates.yml +++ b/.github/workflows/publish-claim-crates.yml @@ -18,7 +18,7 @@ jobs: cache-on-failure: true - name: install parity-publish - run: cargo install parity-publish@0.8.0 + run: cargo install parity-publish@0.8.0 --locked -q - name: parity-publish claim env: From 6bb2cb51e2ec589d53bb9fcc445f17dfa9036600 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 10 Sep 2024 19:13:21 +0200 Subject: [PATCH 06/19] Revert "INSTALL WITH LOCKED FFS" This reverts commit 9aa865fd0b139acbe24a3711848d00b16b6a4c2b. --- .github/workflows/check-semver.yml | 2 +- .github/workflows/publish-check-crates.yml | 2 +- .github/workflows/publish-claim-crates.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/check-semver.yml b/.github/workflows/check-semver.yml index 7fe6a8557ead..15eb32f4062c 100644 --- a/.github/workflows/check-semver.yml +++ b/.github/workflows/check-semver.yml @@ -73,7 +73,7 @@ jobs: - name: install parity-publish # Set the target dir to cache the build. - run: CARGO_TARGET_DIR=./target/ cargo install parity-publish@0.8.0 --locked -q + run: CARGO_TARGET_DIR=./target/ cargo install parity-publish@0.8.0 -q - name: check semver run: | diff --git a/.github/workflows/publish-check-crates.yml b/.github/workflows/publish-check-crates.yml index 498240395fb2..9f96b92e0ce7 100644 --- a/.github/workflows/publish-check-crates.yml +++ b/.github/workflows/publish-check-crates.yml @@ -20,7 +20,7 @@ jobs: cache-on-failure: true - name: install parity-publish - run: cargo install parity-publish@0.8.0 --locked -q + run: cargo install parity-publish@0.8.0 - name: parity-publish check run: parity-publish --color always check --allow-unpublished diff --git a/.github/workflows/publish-claim-crates.yml b/.github/workflows/publish-claim-crates.yml index 5d39a5fdfb40..bee709a12076 100644 --- a/.github/workflows/publish-claim-crates.yml +++ b/.github/workflows/publish-claim-crates.yml @@ -18,7 +18,7 @@ jobs: cache-on-failure: true - name: install parity-publish - run: cargo install parity-publish@0.8.0 --locked -q + run: cargo install parity-publish@0.8.0 - name: parity-publish claim env: From c846cf5b9f0c27d8ec257f8f63e4f50374fe22e7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 13 Sep 2024 18:15:07 +0200 Subject: [PATCH 07/19] Only test in std Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/benchmarking/pov/src/benchmarking.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/frame/benchmarking/pov/src/benchmarking.rs b/substrate/frame/benchmarking/pov/src/benchmarking.rs index 4ceb050e6dfd..c18738235b81 100644 --- a/substrate/frame/benchmarking/pov/src/benchmarking.rs +++ b/substrate/frame/benchmarking/pov/src/benchmarking.rs @@ -26,6 +26,7 @@ use frame_support::traits::UnfilteredDispatchable; use frame_system::{Pallet as System, RawOrigin}; use sp_runtime::traits::Hash; +#[cfg(feature = "std")] frame_support::parameter_types! { pub static StorageRootHash: Option> = None; } @@ -400,6 +401,7 @@ mod benchmarks { fn storage_root_is_the_same_every_time(i: Linear<0, 10>) { let root = sp_io::storage::root(sp_runtime::StateVersion::V1); + #[cfg(feature = "std")] match (i, StorageRootHash::get()) { (0, Some(_)) => panic!("StorageRootHash should be None initially"), (0, None) => StorageRootHash::set(Some(root)), From 8809ce58bc144fc828635178e9b1669c4792b5eb Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 13 Sep 2024 18:15:40 +0200 Subject: [PATCH 08/19] omni-bencher: Fix runtime genesis generation Signed-off-by: Oliver Tale-Yazdi --- .../node/cli/tests/benchmark_pallet_works.rs | 35 +++ .../benchmarking-cli/src/pallet/command.rs | 207 ++++++------------ .../benchmarking-cli/src/pallet/types.rs | 4 +- 3 files changed, 105 insertions(+), 141 deletions(-) diff --git a/substrate/bin/node/cli/tests/benchmark_pallet_works.rs b/substrate/bin/node/cli/tests/benchmark_pallet_works.rs index 8441333429be..d913228881a4 100644 --- a/substrate/bin/node/cli/tests/benchmark_pallet_works.rs +++ b/substrate/bin/node/cli/tests/benchmark_pallet_works.rs @@ -33,6 +33,31 @@ fn benchmark_pallet_works() { benchmark_pallet(20, 50, true); } +#[test] +fn benchmark_pallet_args_work() { + benchmark_pallet_args(&["--list", "--pallet=pallet_balances"], true); + benchmark_pallet_args(&["--list", "--pallet=pallet_balances"], true); + benchmark_pallet_args( + &["--list", "--pallet=pallet_balances", "--genesis-builder=spec-genesis"], + true, + ); + benchmark_pallet_args( + &["--list", "--pallet=pallet_balances", "--chain=dev", "--genesis-builder=spec-genesis"], + true, + ); + + // Error because the genesis runtime does not have any presets in it: + benchmark_pallet_args( + &["--list", "--pallet=pallet_balances", "--chain=dev", "--genesis-builder=spec-runtime"], + false, + ); + // Error because no runtime is provided: + benchmark_pallet_args( + &["--list", "--pallet=pallet_balances", "--chain=dev", "--genesis-builder=runtime"], + false, + ); +} + fn benchmark_pallet(steps: u32, repeat: u32, should_work: bool) { let status = Command::new(cargo_bin("substrate-node")) .args(["benchmark", "pallet", "--dev"]) @@ -51,3 +76,13 @@ fn benchmark_pallet(steps: u32, repeat: u32, should_work: bool) { assert_eq!(status.success(), should_work); } + +fn benchmark_pallet_args(args: &[&str], should_work: bool) { + let status = Command::new(cargo_bin("substrate-node")) + .args(["benchmark", "pallet"]) + .args(args) + .status() + .unwrap(); + + assert_eq!(status.success(), should_work); +} diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 3f716f6bc574..1eb4477b357b 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -27,7 +27,7 @@ use frame_benchmarking::{ }; use frame_support::traits::StorageInfo; use linked_hash_map::LinkedHashMap; -use sc_chain_spec::json_patch::merge as json_merge; +use sc_chain_spec::{json_patch::merge as json_merge, GenesisConfigBuilderRuntimeCaller}; use sc_cli::{execution_method_from_cli, ChainSpec, CliConfiguration, Result, SharedParams}; use sc_client_db::BenchmarkingState; use sc_executor::{HeapAllocStrategy, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY}; @@ -44,6 +44,7 @@ use sp_genesis_builder::{PresetId, Result as GenesisBuildResult}; use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; use sp_runtime::{traits::Hash, RuntimeString}; use sp_state_machine::{OverlayedChanges, StateMachine}; +use sp_storage::{well_known_keys::CODE, Storage}; use sp_trie::{proof_size_extension::ProofSizeExt, recorder::Recorder}; use sp_wasm_interface::HostFunctions; use std::{ @@ -211,7 +212,7 @@ impl PalletCmd { return self.output_from_results(&batches) } - let genesis_storage = self.genesis_storage::(&chain_spec)?; + let genesis_storage = self.genesis_storage::(&chain_spec)?; let cache_size = Some(self.database_cache_size as usize); let state_with_tracking = BenchmarkingState::::new( @@ -562,13 +563,16 @@ impl PalletCmd { /// Build the genesis storage by either the Genesis Builder API, chain spec or nothing. /// /// Behaviour can be controlled by the `--genesis-builder` flag. - fn genesis_storage( + fn genesis_storage( &self, chain_spec: &Option>, ) -> Result { - Ok(match (self.genesis_builder, self.runtime.is_some()) { - (Some(GenesisBuilder::None), _) => Default::default(), - (Some(GenesisBuilder::Spec), _) | (None, false) => { + Ok(match (self.genesis_builder, self.runtime.as_ref()) { + (Some(GenesisBuilder::None), Some(_)) => return Err("Cannot use `--genesis-builder=none` with `--runtime` since the runtime would be ignored.".into()), + (Some(GenesisBuilder::None), None) => Storage::default(), + (Some(GenesisBuilder::SpecGenesis), Some(_)) => + return Err("Cannot use `--genesis-builder=spec-genesis` with `--runtime` since the runtime would be ignored.".into()), + (Some(GenesisBuilder::SpecGenesis), None) | (None, None) => { log::warn!("{WARN_SPEC_GENESIS_CTOR}"); let Some(chain_spec) = chain_spec else { return Err("No chain spec specified to generate the genesis state".into()); @@ -580,132 +584,73 @@ impl PalletCmd { storage }, - (Some(GenesisBuilder::Runtime), _) | (None, true) => - self.genesis_from_runtime::().and_then(Self::changes_to_storage)?, + (Some(GenesisBuilder::SpecRuntime), Some(_)) => + return Err("Cannot use `--genesis-builder=spec` with `--runtime` since the runtime would be ignored.".into()), + (Some(GenesisBuilder::SpecRuntime), None) => { + let Some(chain_spec) = chain_spec else { + return Err("No chain spec specified to generate the genesis state".into()); + }; + + self.genesis_from_spec_runtime::(chain_spec.as_ref())? + }, + (Some(GenesisBuilder::Runtime), None) => return Err("Cannot use `--genesis-builder=runtime` without `--runtime`".into()), + (Some(GenesisBuilder::Runtime), Some(runtime)) | (None, Some(runtime)) => { + let runtime = std::path::absolute(runtime) + .map_err(|e| format!("Could not get absolute path for runtime file: {e}"))?; + log::info!("Loading WASM from {}", runtime.display()); + + let code = fs::read(&runtime).map_err(|e| { + format!( + "Could not load runtime file from path: {}, error: {}", + runtime.display(), + e + ) + })?; + + self.genesis_from_code::(&code)? + } }) } - /// Convert some overlayed changes to a storage. - fn changes_to_storage( - mut changes: OverlayedChanges, - ) -> Result { - let mut top = BTreeMap::, Vec>::new(); - // The backend state here does not matter: - let state = BenchmarkingState::::new(Default::default(), None, false, false)?; + /// Setup the genesis state by calling the runtime APIs of the chain-specs genesis runtime. + fn genesis_from_spec_runtime( + &self, + chain_spec: &dyn ChainSpec, + ) -> Result { + log::info!("Building genesis state from chain spec runtime"); + let storage = chain_spec + .build_storage() + .map_err(|e| format!("{ERROR_CANNOT_BUILD_GENESIS}\nError: {e}"))?; - let changes = changes.drain_storage_changes(&state, sp_storage::StateVersion::V1)?; + let code: &Vec = + storage.top.get(CODE).ok_or("No runtime code in the genesis storage")?; - for (k, v) in changes.main_storage_changes { - if let Some(v) = v { - top.insert(k, v); - } else { - top.remove(&k); - } - } - - Ok(sp_storage::Storage { top, children_default: Default::default() }) + self.genesis_from_code::(code) } - /// Generate the genesis changeset by the runtime API. - fn genesis_from_runtime(&self) -> Result> { - let state = BenchmarkingState::::new( - Default::default(), - Some(self.database_cache_size as usize), - false, - false, - )?; - - // Create a dummy WasmExecutor just to build the genesis storage. - let method = - execution_method_from_cli(self.wasm_method, self.wasmtime_instantiation_strategy); - let executor = WasmExecutor::<( + fn genesis_from_code(&self, code: &[u8]) -> Result { + let genesis_config_caller = GenesisConfigBuilderRuntimeCaller::<( sp_io::SubstrateHostFunctions, frame_benchmarking::benchmarking::HostFunctions, - F, - )>::builder() - .with_execution_method(method) - .with_allow_missing_host_functions(self.allow_missing_host_functions) - .build(); - - let runtime = self.runtime_blob(&state)?; - let runtime_code = runtime.code()?; - - // We cannot use the `GenesisConfigBuilderRuntimeCaller` here since it returns the changes - // as `Storage` item, but we need it as `OverlayedChanges`. - let genesis_json: Option> = Self::exec_state_machine( - StateMachine::new( - &state, - &mut Default::default(), - &executor, - "GenesisBuilder_get_preset", - &None::.encode(), // Use the default preset - &mut Self::build_extensions(executor.clone(), state.recorder()), - &runtime_code, - CallContext::Offchain, - ), - "build the genesis spec", - )?; - - let Some(base_genesis_json) = genesis_json else { - return Err("GenesisBuilder::get_preset returned no data".into()) - }; - - let base_genesis_json = serde_json::from_slice::(&base_genesis_json) - .map_err(|e| format!("GenesisBuilder::get_preset returned invalid JSON: {:?}", e))?; - - let preset_name = RuntimeString::Owned(self.genesis_builder_preset.clone()); - let dev_genesis_json: Option> = Self::exec_state_machine( - StateMachine::new( - &state, - &mut Default::default(), - &executor, - "GenesisBuilder_get_preset", - &Some::(preset_name).encode(), - &mut Self::build_extensions(executor.clone(), state.recorder()), - &runtime_code, - CallContext::Offchain, - ), - "build the genesis spec", - )?; - - let mut genesis_json = serde_json::Value::default(); - json_merge(&mut genesis_json, base_genesis_json); + EHF, + )>::new(code); + let preset = Some(&self.genesis_builder_preset); - if let Some(dev) = dev_genesis_json { - let dev: serde_json::Value = serde_json::from_slice(&dev).map_err(|e| { - format!("GenesisBuilder::get_preset returned invalid JSON: {:?}", e) + let mut storage = + genesis_config_caller.get_storage_for_named_preset(preset).map_err(|e| { + let presets = genesis_config_caller.preset_names().unwrap_or_default(); + log::error!( + "Please pick one of the available presets with \ + `--genesis-builder-preset=`. Available presets ({}): {:?}.", + presets.len(), + presets + ); + e })?; - json_merge(&mut genesis_json, dev); - } else { - return Err(format!( - "GenesisBuilder::get_preset returned no data for preset `{}`. Manually specify `--genesis-builder-preset` or use a different `--genesis-builder`.", - self.genesis_builder_preset).into() - ) - } - - let json_pretty_str = serde_json::to_string_pretty(&genesis_json) - .map_err(|e| format!("json to string failed: {e}"))?; - - let mut changes = Default::default(); - let build_res: GenesisBuildResult = Self::exec_state_machine( - StateMachine::new( - &state, - &mut changes, - &executor, - "GenesisBuilder_build_state", - &json_pretty_str.encode(), - &mut Extensions::default(), - &runtime_code, - CallContext::Offchain, - ), - "populate the genesis state", - )?; - if let Err(e) = build_res { - return Err(format!("GenesisBuilder::build_state failed: {}", e).into()) - } + storage.top.insert(CODE.into(), code.into()); - Ok(changes) + Ok(storage) } /// Execute a state machine and decode its return value as `R`. @@ -749,28 +694,10 @@ impl PalletCmd { &self, state: &'a BenchmarkingState, ) -> Result, H>> { - if let Some(runtime) = &self.runtime { - let runtime = std::path::absolute(runtime) - .map_err(|e| format!("Could not get absolute path for runtime file: {e}"))?; - log::info!("Loading WASM from {}", runtime.display()); - - let code = fs::read(&runtime).map_err(|e| { - format!( - "Could not load runtime file from path: {}, error: {}", - runtime.display(), - e - ) - })?; - let hash = sp_core::blake2_256(&code).to_vec(); - let wrapped_code = WrappedRuntimeCode(Cow::Owned(code)); + log::info!("Loading WASM from state"); + let state = sp_state_machine::backend::BackendRuntimeCode::new(state); - Ok(FetchedCode::FromFile { wrapped_code, heap_pages: self.heap_pages, hash }) - } else { - log::info!("Loading WASM from genesis state"); - let state = sp_state_machine::backend::BackendRuntimeCode::new(state); - - Ok(FetchedCode::FromGenesis { state }) - } + Ok(FetchedCode::FromGenesis { state }) } /// Allocation strategy for pallet benchmarking. diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs index 2bb00d66560f..1e383c3921fb 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs @@ -32,8 +32,10 @@ pub enum GenesisBuilder { None, /// Let the runtime build the genesis state through its `BuildGenesisConfig` runtime API. Runtime, + // Use the runtime from the Spec file to build the genesis state. + SpecRuntime, /// Use the spec file to build the genesis state. This fails when there is no spec. - Spec, + SpecGenesis, } /// A runtime blob that was either fetched from genesis storage or loaded from a file. From a9ec9758a47f0647a3d9fc66b810117f4ceca0c1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 16 Sep 2024 22:14:40 +0200 Subject: [PATCH 09/19] Update substrate/utils/frame/benchmarking-cli/src/pallet/command.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 1eb4477b357b..a36c195e79d2 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -595,8 +595,6 @@ impl PalletCmd { }, (Some(GenesisBuilder::Runtime), None) => return Err("Cannot use `--genesis-builder=runtime` without `--runtime`".into()), (Some(GenesisBuilder::Runtime), Some(runtime)) | (None, Some(runtime)) => { - let runtime = std::path::absolute(runtime) - .map_err(|e| format!("Could not get absolute path for runtime file: {e}"))?; log::info!("Loading WASM from {}", runtime.display()); let code = fs::read(&runtime).map_err(|e| { From f205230940eef6f69700b323bb3a0bd211b67a45 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 16 Sep 2024 22:14:47 +0200 Subject: [PATCH 10/19] Update substrate/utils/frame/benchmarking-cli/src/pallet/command.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index a36c195e79d2..3151dc1bd794 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -946,7 +946,7 @@ impl PalletCmd { if let Some(header_file) = &self.header { if !header_file.is_file() { - return Err(format!("Header file could not be found: {:?}", &header_file).into()) + return Err(format!("Header file could not be found: {header_file:?}").into()) }; } From 669c88af4393cfde65264282a64bbb423d5a65ef Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 16 Sep 2024 22:14:52 +0200 Subject: [PATCH 11/19] Update substrate/utils/frame/benchmarking-cli/src/pallet/command.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 3151dc1bd794..ad3d66a4c950 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -937,8 +937,7 @@ impl PalletCmd { if let Some(output_path) = &self.output { if !output_path.is_dir() && output_path.file_name().is_none() { return Err(format!( - "Output path is neither a directory nor a file: {:?}", - output_path + "Output path is neither a directory nor a file: {output_path:?}" ) .into()) } From a539e2a2d796de21f750974a863ebc9f8b46d497 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 16 Sep 2024 22:14:59 +0200 Subject: [PATCH 12/19] Update substrate/utils/frame/benchmarking-cli/src/pallet/command.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index ad3d66a4c950..381e0268042c 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -952,8 +952,7 @@ impl PalletCmd { if let Some(handlebars_template_file) = &self.template { if !handlebars_template_file.is_file() { return Err(format!( - "Handlebars template file could not be found: {:?}", - &handlebars_template_file + "Handlebars template file could not be found: {handlebars_template_file:?}" ) .into()) }; From f64151777ec744e4526b886dae73e0d841a4f2e8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 16 Sep 2024 22:17:17 +0200 Subject: [PATCH 13/19] Stay backwards compatible Signed-off-by: Oliver Tale-Yazdi --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 4 ++-- substrate/utils/frame/benchmarking-cli/src/pallet/types.rs | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 381e0268042c..eeab5cbc33d5 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -570,9 +570,9 @@ impl PalletCmd { Ok(match (self.genesis_builder, self.runtime.as_ref()) { (Some(GenesisBuilder::None), Some(_)) => return Err("Cannot use `--genesis-builder=none` with `--runtime` since the runtime would be ignored.".into()), (Some(GenesisBuilder::None), None) => Storage::default(), - (Some(GenesisBuilder::SpecGenesis), Some(_)) => + (Some(GenesisBuilder::SpecGenesis | GenesisBuilder::Spec), Some(_)) => return Err("Cannot use `--genesis-builder=spec-genesis` with `--runtime` since the runtime would be ignored.".into()), - (Some(GenesisBuilder::SpecGenesis), None) | (None, None) => { + (Some(GenesisBuilder::SpecGenesis | GenesisBuilder::Spec), None) | (None, None) => { log::warn!("{WARN_SPEC_GENESIS_CTOR}"); let Some(chain_spec) = chain_spec else { return Err("No chain spec specified to generate the genesis state".into()); diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs index 1e383c3921fb..8d74131760a6 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs @@ -36,6 +36,8 @@ pub enum GenesisBuilder { SpecRuntime, /// Use the spec file to build the genesis state. This fails when there is no spec. SpecGenesis, + /// Same as `SpecGenesis` - only here for backwards compatibility. + Spec, } /// A runtime blob that was either fetched from genesis storage or loaded from a file. From 1da1213a3b42b16a34412b9c46221eb99c36ec2a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 21 Sep 2024 00:06:47 +0200 Subject: [PATCH 14/19] Rename to GenesisBuilderPolicy Signed-off-by: Oliver Tale-Yazdi --- .../benchmarking-cli/src/pallet/command.rs | 18 +++++++++--------- .../frame/benchmarking-cli/src/pallet/mod.rs | 9 +++++---- .../frame/benchmarking-cli/src/pallet/types.rs | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index eeab5cbc33d5..9c765028d3ed 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -19,7 +19,7 @@ use super::{ types::{ComponentRange, ComponentRangeMap}, writer, ListOutput, PalletCmd, }; -use crate::pallet::{types::FetchedCode, GenesisBuilder}; +use crate::pallet::{types::FetchedCode, GenesisBuilderPolicy}; use codec::{Decode, Encode}; use frame_benchmarking::{ Analysis, BenchmarkBatch, BenchmarkBatchSplitResults, BenchmarkList, BenchmarkParameter, @@ -568,11 +568,11 @@ impl PalletCmd { chain_spec: &Option>, ) -> Result { Ok(match (self.genesis_builder, self.runtime.as_ref()) { - (Some(GenesisBuilder::None), Some(_)) => return Err("Cannot use `--genesis-builder=none` with `--runtime` since the runtime would be ignored.".into()), - (Some(GenesisBuilder::None), None) => Storage::default(), - (Some(GenesisBuilder::SpecGenesis | GenesisBuilder::Spec), Some(_)) => + (Some(GenesisBuilderPolicy::None), Some(_)) => return Err("Cannot use `--genesis-builder=none` with `--runtime` since the runtime would be ignored.".into()), + (Some(GenesisBuilderPolicy::None), None) => Storage::default(), + (Some(GenesisBuilderPolicy::SpecGenesis | GenesisBuilderPolicy::Spec), Some(_)) => return Err("Cannot use `--genesis-builder=spec-genesis` with `--runtime` since the runtime would be ignored.".into()), - (Some(GenesisBuilder::SpecGenesis | GenesisBuilder::Spec), None) | (None, None) => { + (Some(GenesisBuilderPolicy::SpecGenesis | GenesisBuilderPolicy::Spec), None) | (None, None) => { log::warn!("{WARN_SPEC_GENESIS_CTOR}"); let Some(chain_spec) = chain_spec else { return Err("No chain spec specified to generate the genesis state".into()); @@ -584,17 +584,17 @@ impl PalletCmd { storage }, - (Some(GenesisBuilder::SpecRuntime), Some(_)) => + (Some(GenesisBuilderPolicy::SpecRuntime), Some(_)) => return Err("Cannot use `--genesis-builder=spec` with `--runtime` since the runtime would be ignored.".into()), - (Some(GenesisBuilder::SpecRuntime), None) => { + (Some(GenesisBuilderPolicy::SpecRuntime), None) => { let Some(chain_spec) = chain_spec else { return Err("No chain spec specified to generate the genesis state".into()); }; self.genesis_from_spec_runtime::(chain_spec.as_ref())? }, - (Some(GenesisBuilder::Runtime), None) => return Err("Cannot use `--genesis-builder=runtime` without `--runtime`".into()), - (Some(GenesisBuilder::Runtime), Some(runtime)) | (None, Some(runtime)) => { + (Some(GenesisBuilderPolicy::Runtime), None) => return Err("Cannot use `--genesis-builder=runtime` without `--runtime`".into()), + (Some(GenesisBuilderPolicy::Runtime), Some(runtime)) | (None, Some(runtime)) => { log::info!("Loading WASM from {}", runtime.display()); let code = fs::read(&runtime).map_err(|e| { diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs index 057c2da88643..a507c1d1f548 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs @@ -19,7 +19,7 @@ mod command; mod types; mod writer; -use crate::{pallet::types::GenesisBuilder, shared::HostInfoParams}; +use crate::{pallet::types::GenesisBuilderPolicy, shared::HostInfoParams}; use clap::ValueEnum; use sc_cli::{ WasmExecutionMethod, WasmtimeInstantiationStrategy, DEFAULT_WASMTIME_INSTANTIATION_STRATEGY, @@ -177,9 +177,10 @@ pub struct PalletCmd { /// How to construct the genesis state. /// - /// Uses `GenesisBuilder::Spec` by default and `GenesisBuilder::Runtime` if `runtime` is set. - #[arg(long, value_enum)] - pub genesis_builder: Option, + /// Uses `GenesisBuilderPolicy::Spec` by default and `GenesisBuilderPolicy::Runtime` if + /// `runtime` is set. + #[arg(long, value_enum, alias = "genesis-builder-policy")] + pub genesis_builder: Option, /// The preset that we expect to find in the GenesisBuilder runtime API. /// diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs index 8d74131760a6..a4799dc92369 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs @@ -24,7 +24,7 @@ use sp_runtime::traits::Hash; /// How the genesis state for benchmarking should be build. #[derive(clap::ValueEnum, Debug, Eq, PartialEq, Clone, Copy)] #[clap(rename_all = "kebab-case")] -pub enum GenesisBuilder { +pub enum GenesisBuilderPolicy { /// Do not provide any genesis state. /// /// Benchmarks are advised to function with this, since they should setup their own required From 68413fa065e7386c1369bae8f7b0ff96b525e931 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 21 Sep 2024 15:35:28 +0200 Subject: [PATCH 15/19] clippy Signed-off-by: Oliver Tale-Yazdi --- .../frame/benchmarking-cli/src/pallet/command.rs | 12 +++++------- .../frame/benchmarking-cli/src/pallet/types.rs | 16 ++++------------ 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 9c765028d3ed..8ba97c2eb3c2 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -27,7 +27,7 @@ use frame_benchmarking::{ }; use frame_support::traits::StorageInfo; use linked_hash_map::LinkedHashMap; -use sc_chain_spec::{json_patch::merge as json_merge, GenesisConfigBuilderRuntimeCaller}; +use sc_chain_spec::GenesisConfigBuilderRuntimeCaller; use sc_cli::{execution_method_from_cli, ChainSpec, CliConfiguration, Result, SharedParams}; use sc_client_db::BenchmarkingState; use sc_executor::{HeapAllocStrategy, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY}; @@ -36,19 +36,17 @@ use sp_core::{ testing::{TestOffchainExt, TestTransactionPoolExt}, OffchainDbExt, OffchainWorkerExt, TransactionPoolExt, }, - traits::{CallContext, CodeExecutor, ReadRuntimeVersionExt, WrappedRuntimeCode}, + traits::{CallContext, CodeExecutor, ReadRuntimeVersionExt}, Hasher, }; use sp_externalities::Extensions; -use sp_genesis_builder::{PresetId, Result as GenesisBuildResult}; use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; -use sp_runtime::{traits::Hash, RuntimeString}; -use sp_state_machine::{OverlayedChanges, StateMachine}; +use sp_runtime::traits::Hash; +use sp_state_machine::StateMachine; use sp_storage::{well_known_keys::CODE, Storage}; use sp_trie::{proof_size_extension::ProofSizeExt, recorder::Recorder}; use sp_wasm_interface::HostFunctions; use std::{ - borrow::Cow, collections::{BTreeMap, BTreeSet, HashMap}, fmt::Debug, fs, @@ -695,7 +693,7 @@ impl PalletCmd { log::info!("Loading WASM from state"); let state = sp_state_machine::backend::BackendRuntimeCode::new(state); - Ok(FetchedCode::FromGenesis { state }) + Ok(FetchedCode { state }) } /// Allocation strategy for pallet benchmarking. diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs index a4799dc92369..ce37be455e8a 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/types.rs @@ -18,7 +18,7 @@ //! Various types used by this crate. use sc_cli::Result; -use sp_core::traits::{RuntimeCode, WrappedRuntimeCode}; +use sp_core::traits::RuntimeCode; use sp_runtime::traits::Hash; /// How the genesis state for benchmarking should be build. @@ -43,9 +43,8 @@ pub enum GenesisBuilderPolicy { /// A runtime blob that was either fetched from genesis storage or loaded from a file. // NOTE: This enum is only needed for the annoying lifetime bounds on `RuntimeCode`. Otherwise we // could just directly return the blob. -pub enum FetchedCode<'a, B, H> { - FromGenesis { state: sp_state_machine::backend::BackendRuntimeCode<'a, B, H> }, - FromFile { wrapped_code: WrappedRuntimeCode<'a>, heap_pages: Option, hash: Vec }, +pub struct FetchedCode<'a, B, H> { + pub state: sp_state_machine::backend::BackendRuntimeCode<'a, B, H>, } impl<'a, B, H> FetchedCode<'a, B, H> @@ -55,14 +54,7 @@ where { /// The runtime blob. pub fn code(&'a self) -> Result> { - match self { - Self::FromGenesis { state } => state.runtime_code().map_err(Into::into), - Self::FromFile { wrapped_code, heap_pages, hash } => Ok(RuntimeCode { - code_fetcher: wrapped_code, - heap_pages: *heap_pages, - hash: hash.clone(), - }), - } + self.state.runtime_code().map_err(Into::into) } } From 8b97944485d1197b70b37bf4c9d2faf548eecf90 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 21 Sep 2024 15:45:10 +0200 Subject: [PATCH 16/19] clippy Signed-off-by: Oliver Tale-Yazdi --- .../utils/frame/benchmarking-cli/src/pallet/command.rs | 7 ++++--- .../utils/frame/benchmarking-cli/src/pallet/writer.rs | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 8ba97c2eb3c2..b19e4a06da74 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -633,13 +633,14 @@ impl PalletCmd { let preset = Some(&self.genesis_builder_preset); let mut storage = - genesis_config_caller.get_storage_for_named_preset(preset).map_err(|e| { + genesis_config_caller.get_storage_for_named_preset(preset).inspect_err(|e| { let presets = genesis_config_caller.preset_names().unwrap_or_default(); log::error!( "Please pick one of the available presets with \ - `--genesis-builder-preset=`. Available presets ({}): {:?}.", + `--genesis-builder-preset=`. Available presets ({}): {:?}. Error: {:?}", presets.len(), - presets + presets, + e ); e })?; diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs index d4742361e9e2..34f31734da67 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs @@ -484,7 +484,7 @@ pub(crate) fn write_results( benchmarks: results.clone(), }; - let file_path = std::path::absolute(&file_path).map_err(|e| { + let file_path = fs::canonicalize(&file_path).map_err(|e| { format!("Could not get absolute path for: {:?}. Error: {:?}", &file_path, e) })?; let mut output_file = fs::File::create(&file_path).map_err(|e| { From 41cb2bf231616dcda5b2e3dc335d5ec31bb0b9e1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 21 Sep 2024 16:02:51 +0200 Subject: [PATCH 17/19] clippy Signed-off-by: Oliver Tale-Yazdi --- substrate/utils/frame/benchmarking-cli/src/pallet/command.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index b19e4a06da74..f1499f41c2ea 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -642,7 +642,6 @@ impl PalletCmd { presets, e ); - e })?; storage.top.insert(CODE.into(), code.into()); From b3e8e158325179269941c9640109839bb0e5eb1e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sat, 21 Sep 2024 17:22:23 +0200 Subject: [PATCH 18/19] fix bench test Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/benchmarking/pov/src/benchmarking.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/benchmarking/pov/src/benchmarking.rs b/substrate/frame/benchmarking/pov/src/benchmarking.rs index c18738235b81..d52fcc2689c4 100644 --- a/substrate/frame/benchmarking/pov/src/benchmarking.rs +++ b/substrate/frame/benchmarking/pov/src/benchmarking.rs @@ -399,6 +399,7 @@ mod benchmarks { #[benchmark] fn storage_root_is_the_same_every_time(i: Linear<0, 10>) { + #[cfg(feature = "std")] let root = sp_io::storage::root(sp_runtime::StateVersion::V1); #[cfg(feature = "std")] From 74b6a13ad9d15c610e93576f40990f0e0ed3281d Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Sun, 22 Sep 2024 20:28:06 +0200 Subject: [PATCH 19/19] Update prdoc/pr_5655.prdoc --- prdoc/pr_5655.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_5655.prdoc b/prdoc/pr_5655.prdoc index 3efeb2621f14..bfa2e295d157 100644 --- a/prdoc/pr_5655.prdoc +++ b/prdoc/pr_5655.prdoc @@ -10,6 +10,6 @@ doc: - Improve error messages. crates: - name: frame-benchmarking-cli - bump: minor + bump: major - name: frame-benchmarking-pallet-pov - bump: minor + bump: patch