[benchmarking] Reset to genesis storage after each run

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

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<alloc::vec::Vec<u8>> = 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,);
 }
 

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 = sp_genesis_builder::DEV_RUNTIME_PRESET)]
+	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()))?;