From b74e6cae660c5b93f4a8c0e7cb41bed7127f44e7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 19 Jul 2024 22:00:50 +0200 Subject: [PATCH] Bench runner fix genesis storage (from https://github.com/paritytech/polkadot-sdk/pull/5083) Signed-off-by: Oliver Tale-Yazdi Fix changes_to_storage Signed-off-by: Oliver Tale-Yazdi Better error messages Signed-off-by: Oliver Tale-Yazdi genesis-preset flag Signed-off-by: Oliver Tale-Yazdi Bench runner fix genesis storage Signed-off-by: Oliver Tale-Yazdi clippy --- substrate/primitives/state-machine/src/lib.rs | 2 +- .../src/overlayed_changes/changeset.rs | 7 +- .../src/overlayed_changes/mod.rs | 6 +- .../benchmarking-cli/src/pallet/command.rs | 89 ++++++++++++------- .../frame/benchmarking-cli/src/pallet/mod.rs | 7 ++ .../benchmarking-cli/src/pallet/writer.rs | 7 +- 6 files changed, 81 insertions(+), 37 deletions(-) diff --git a/substrate/primitives/state-machine/src/lib.rs b/substrate/primitives/state-machine/src/lib.rs index 289b08755f68..158b21225ab2 100644 --- a/substrate/primitives/state-machine/src/lib.rs +++ b/substrate/primitives/state-machine/src/lib.rs @@ -31,7 +31,7 @@ mod ext; pub mod fuzzing; #[cfg(feature = "std")] mod in_memory_backend; -pub(crate) mod overlayed_changes; +pub mod overlayed_changes; // FAIL-CI #[cfg(feature = "std")] mod read_only; mod stats; diff --git a/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs b/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs index c478983e979a..53efb9328d12 100644 --- a/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs +++ b/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs @@ -65,11 +65,12 @@ pub enum ExecutionMode { Runtime, } +#[allow(missing_docs)] #[derive(Debug, Default, Clone)] #[cfg_attr(test, derive(PartialEq))] -struct InnerValue { +pub struct InnerValue { /// Current value. None if value has been deleted. - value: V, + pub value: V, /// The set of extrinsic indices where the values has been changed. extrinsics: Extrinsics, } @@ -80,7 +81,7 @@ struct InnerValue { pub struct OverlayedEntry { /// The individual versions of that value. /// One entry per transactions during that the value was actually written. - transactions: Transactions, + pub transactions: Transactions, } impl Default for OverlayedEntry { diff --git a/substrate/primitives/state-machine/src/overlayed_changes/mod.rs b/substrate/primitives/state-machine/src/overlayed_changes/mod.rs index c2dc637bc71a..f76dad3d15c7 100644 --- a/substrate/primitives/state-machine/src/overlayed_changes/mod.rs +++ b/substrate/primitives/state-machine/src/overlayed_changes/mod.rs @@ -17,7 +17,7 @@ //! The overlayed changes to state. -mod changeset; +pub mod changeset; // FAIL-CI mod offchain; use self::changeset::OverlayedChangeSet; @@ -93,7 +93,7 @@ impl Extrinsics { /// It allows changes to be modified using nestable transactions. pub struct OverlayedChanges { /// Top level storage changes. - top: OverlayedChangeSet, + pub top: OverlayedChangeSet, /// Child storage changes. The map key is the child storage key without the common prefix. children: Map, /// Offchain related changes. @@ -813,7 +813,9 @@ where /// or an owned extension. #[cfg(feature = "std")] pub enum OverlayedExtension<'a> { + #[allow(missing_docs)] MutRef(&'a mut Box), + #[allow(missing_docs)] Owned(Box), } 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()))?;