Bench runner fix genesis storage (from #5083)

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix changes_to_storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Better error messages

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

genesis-preset flag

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Bench runner fix genesis storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

clippy

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;

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<V> {
+pub struct InnerValue<V> {
 	/// 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<V> {
 pub struct OverlayedEntry<V> {
 	/// The individual versions of that value.
 	/// One entry per transactions during that the value was actually written.
-	transactions: Transactions<V>,
+	pub transactions: Transactions<V>,
 }
 
 impl<V> Default for OverlayedEntry<V> {

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<H: Hasher> {
 	/// 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<StorageKey, (OverlayedChangeSet, ChildInfo)>,
 	/// 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<dyn Extension>),
+	#[allow(missing_docs)]
 	Owned(Box<dyn Extension>),
 }
 

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::<Hasher, ExtraHostFunctions>(&chain_spec)?;
-		let mut changes = genesis_changes.clone();
+		let genesis_storage = self.genesis_storage::<Hasher, ExtraHostFunctions>(&chain_spec)?;
 
 		let cache_size = Some(self.database_cache_size as usize);
 		let state_with_tracking = BenchmarkingState::<Hasher>::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<BenchmarkBatch> = 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<BenchmarkBatch> = match Self::exec_state_machine::<
 						std::result::Result<Vec<BenchmarkBatch>, 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<Vec<BenchmarkBatch>, 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<H: Hash, F: HostFunctions>(
 		&self,
 		chain_spec: &Option<Box<dyn ChainSpec>>,
-	) -> Result<(sp_storage::Storage, OverlayedChanges<H>)> {
+	) -> Result<sp_storage::Storage> {
 		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::<H, F>()?),
+				self.genesis_from_runtime::<H, F>().and_then(Self::changes_to_storage)?,
 		})
 	}
 
+	/// Convert some overlayed changes to a storage.
+	fn changes_to_storage<H: Hash>(
+		mut changes: OverlayedChanges<H>,
+	) -> Result<sp_storage::Storage> {
+		let mut top = BTreeMap::<Vec<u8>, Vec<u8>>::new();
+		// The backend state here does not matter:
+		let state = BenchmarkingState::<H>::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<H: Hash, F: HostFunctions>(&self) -> Result<OverlayedChanges<H>> {
 		let state = BenchmarkingState::<H>::new(
@@ -643,13 +653,14 @@ impl PalletCmd {
 		let base_genesis_json = serde_json::from_slice::<serde_json::Value>(&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<Vec<u8>> = Self::exec_state_machine(
 			StateMachine::new(
 				&state,
 				&mut Default::default(),
 				&executor,
 				"GenesisBuilder_get_preset",
-				&Some::<PresetId>(GENESIS_PRESET.into()).encode(), // Use the default preset
+				&Some::<PresetId>(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<H>,
 	) -> Result<FetchedCode<'a, BenchmarkingState<H>, 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())
 			};
 		}
 

substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs

@@ -181,6 +181,13 @@ pub struct PalletCmd {
 	#[arg(long, value_enum)]
 	pub genesis_builder: Option<GenesisBuilder>,
 
+	/// 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<String>,

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()))?;