bench pallet: only require Hash instead of Block (#3244)

Preparation for #2664

Changes:
- Only require `Hash` instead of `Block` for the benchmarking
- Refactor DB types to do the same

## Integration

This breaking change can easily be integrated into your node via:  
```patch
- cmd.run::<Block, ()>(config)
+ cmd.run::<HashingFor<Block>, ()>(config)
```

Status: waiting for CI checks

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: cheme <emericchevalier.pro@gmail.com>

cumulus/parachain-template/node/src/command.rs

@@ -183,7 +183,7 @@ pub fn run() -> Result<()> {
 			match cmd {
 				BenchmarkCmd::Pallet(cmd) =>
 					if cfg!(feature = "runtime-benchmarks") {
-						runner.sync_run(|config| cmd.run::<Block, ()>(config))
+						runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ()>(config))
 					} else {
 						Err("Benchmarking wasn't enabled when building the node. \
 					You can enable it with `--features runtime-benchmarks`."

cumulus/polkadot-parachain/src/command.rs

@@ -584,7 +584,7 @@ pub fn run() -> Result<()> {
 			match cmd {
 				BenchmarkCmd::Pallet(cmd) =>
 					if cfg!(feature = "runtime-benchmarks") {
-						runner.sync_run(|config| cmd.run::<Block, ()>(config))
+						runner.sync_run(|config| cmd.run::<sp_runtime::traits::HashingFor<Block>, ()>(config))
 					} else {
 						Err("Benchmarking wasn't enabled when building the node. \
 				You can enable it with `--features runtime-benchmarks`."

polkadot/cli/Cargo.toml

@@ -42,6 +42,7 @@ sc-tracing = { path = "../../substrate/client/tracing", optional = true }
 sc-sysinfo = { path = "../../substrate/client/sysinfo" }
 sc-executor = { path = "../../substrate/client/executor" }
 sc-storage-monitor = { path = "../../substrate/client/storage-monitor" }
+sp-runtime = { path = "../../substrate/primitives/runtime" }
 
 [build-dependencies]
 substrate-build-script-utils = { path = "../../substrate/utils/build-script-utils" }
@@ -63,9 +64,14 @@ runtime-benchmarks = [
 	"polkadot-node-metrics/runtime-benchmarks",
 	"sc-service?/runtime-benchmarks",
 	"service/runtime-benchmarks",
+	"sp-runtime/runtime-benchmarks",
 ]
 full-node = ["service/full-node"]
-try-runtime = ["service/try-runtime", "try-runtime-cli/try-runtime"]
+try-runtime = [
+	"service/try-runtime",
+	"sp-runtime/try-runtime",
+	"try-runtime-cli/try-runtime",
+]
 fast-runtime = ["service/fast-runtime"]
 pyroscope = ["pyro", "pyroscope_pprofrs"]
 

polkadot/cli/src/command.rs

@@ -451,7 +451,7 @@ pub fn run() -> Result<()> {
 
 					if cfg!(feature = "runtime-benchmarks") {
 						runner.sync_run(|config| {
-							cmd.run::<service::Block, ()>(config)
+							cmd.run::<sp_runtime::traits::HashingFor<service::Block>, ()>(config)
 								.map_err(|e| Error::SubstrateCli(e))
 						})
 					} else {

prdoc/pr_3244.prdoc

@@ -0,0 +1,18 @@
+title: "Make the `benchmark pallet` command only require a Hasher"
+
+doc:
+  - audience: Node Dev
+    description: |
+      Currently the `benchmark pallet` command requires a `Block` type, while only using its hasher.
+      Now this is changed to only require the hasher. This means to use `HashingFor<Block>` in the
+      place where `Block` was required.  
+      Example patch for your node with `cmd` being `BenchmarkCmd::Pallet(cmd)`:
+
+      ```patch
+      - cmd.run::<Block, ()>(config)
+      + cmd.run::<HashingFor<Block>, ()>(config)
+      ```
+
+crates:
+  - name: sc-client-db
+  - name: frame-benchmarking-cli

substrate/bin/node-template/node/src/command.rs

@@ -117,7 +117,7 @@ pub fn run() -> sc_cli::Result<()> {
 							)
 						}
 
-						cmd.run::<Block, ()>(config)
+						cmd.run::<sp_runtime::traits::HashingFor<Block>, ()>(config)
 					},
 					BenchmarkCmd::Block(cmd) => {
 						let PartialComponents { client, .. } = service::new_partial(&config)?;

substrate/bin/node/cli/src/command.rs

@@ -28,6 +28,7 @@ use node_primitives::Block;
 use sc_cli::{Result, SubstrateCli};
 use sc_service::PartialComponents;
 use sp_keyring::Sr25519Keyring;
+use sp_runtime::traits::HashingFor;
 
 use std::sync::Arc;
 
@@ -106,7 +107,7 @@ pub fn run() -> Result<()> {
 							)
 						}
 
-						cmd.run::<Block, sp_statement_store::runtime_api::HostFunctions>(config)
+						cmd.run::<HashingFor<Block>, sp_statement_store::runtime_api::HostFunctions>(config)
 					},
 					BenchmarkCmd::Block(cmd) => {
 						// ensure that we keep the task manager alive

substrate/client/db/src/bench.rs

@@ -19,18 +19,15 @@
 //! State backend that's useful for benchmarking
 
 use crate::{DbState, DbStateBuilder};
-use hash_db::{Hasher, Prefix};
+use hash_db::{Hasher as DbHasher, Prefix};
 use kvdb::{DBTransaction, KeyValueDB};
 use linked_hash_map::LinkedHashMap;
 use parking_lot::Mutex;
 use sp_core::{
 	hexdisplay::HexDisplay,
 	storage::{ChildInfo, TrackedStorageKey},
 };
-use sp_runtime::{
-	traits::{Block as BlockT, HashingFor},
-	StateVersion, Storage,
-};
+use sp_runtime::{traits::Hash, StateVersion, Storage};
 use sp_state_machine::{
 	backend::Backend as StateBackend, BackendTransaction, ChildStorageCollection, DBValue,
 	IterArgs, StorageCollection, StorageIterator, StorageKey, StorageValue,
@@ -45,16 +42,16 @@ use std::{
 	sync::Arc,
 };
 
-type State<B> = DbState<B>;
+type State<H> = DbState<H>;
 
-struct StorageDb<Block: BlockT> {
+struct StorageDb<Hasher> {
 	db: Arc<dyn KeyValueDB>,
-	_block: std::marker::PhantomData<Block>,
+	_phantom: std::marker::PhantomData<Hasher>,
 }
 
-impl<Block: BlockT> sp_state_machine::Storage<HashingFor<Block>> for StorageDb<Block> {
-	fn get(&self, key: &Block::Hash, prefix: Prefix) -> Result<Option<DBValue>, String> {
-		let prefixed_key = prefixed_key::<HashingFor<Block>>(key, prefix);
+impl<Hasher: Hash> sp_state_machine::Storage<Hasher> for StorageDb<Hasher> {
+	fn get(&self, key: &Hasher::Output, prefix: Prefix) -> Result<Option<DBValue>, String> {
+		let prefixed_key = prefixed_key::<Hasher>(key, prefix);
 		self.db
 			.get(0, &prefixed_key)
 			.map_err(|e| format!("Database backend error: {:?}", e))
@@ -75,29 +72,29 @@ struct KeyTracker {
 }
 
 /// State that manages the backend database reference. Allows runtime to control the database.
-pub struct BenchmarkingState<B: BlockT> {
-	root: Cell<B::Hash>,
-	genesis_root: B::Hash,
-	state: RefCell<Option<State<B>>>,
+pub struct BenchmarkingState<Hasher: Hash> {
+	root: Cell<Hasher::Output>,
+	genesis_root: Hasher::Output,
+	state: RefCell<Option<State<Hasher>>>,
 	db: Cell<Option<Arc<dyn KeyValueDB>>>,
 	genesis: HashMap<Vec<u8>, (Vec<u8>, i32)>,
 	record: Cell<Vec<Vec<u8>>>,
 	key_tracker: Arc<Mutex<KeyTracker>>,
 	whitelist: RefCell<Vec<TrackedStorageKey>>,
-	proof_recorder: Option<sp_trie::recorder::Recorder<HashingFor<B>>>,
-	proof_recorder_root: Cell<B::Hash>,
-	shared_trie_cache: SharedTrieCache<HashingFor<B>>,
+	proof_recorder: Option<sp_trie::recorder::Recorder<Hasher>>,
+	proof_recorder_root: Cell<Hasher::Output>,
+	shared_trie_cache: SharedTrieCache<Hasher>,
 }
 
 /// A raw iterator over the `BenchmarkingState`.
-pub struct RawIter<B: BlockT> {
-	inner: <DbState<B> as StateBackend<HashingFor<B>>>::RawIter,
+pub struct RawIter<Hasher: Hash> {
+	inner: <DbState<Hasher> as StateBackend<Hasher>>::RawIter,
 	child_trie: Option<Vec<u8>>,
 	key_tracker: Arc<Mutex<KeyTracker>>,
 }
 
-impl<B: BlockT> StorageIterator<HashingFor<B>> for RawIter<B> {
-	type Backend = BenchmarkingState<B>;
+impl<Hasher: Hash> StorageIterator<Hasher> for RawIter<Hasher> {
+	type Backend = BenchmarkingState<Hasher>;
 	type Error = String;
 
 	fn next_key(&mut self, backend: &Self::Backend) -> Option<Result<StorageKey, Self::Error>> {
@@ -128,7 +125,7 @@ impl<B: BlockT> StorageIterator<HashingFor<B>> for RawIter<B> {
 	}
 }
 
-impl<B: BlockT> BenchmarkingState<B> {
+impl<Hasher: Hash> BenchmarkingState<Hasher> {
 	/// Create a new instance that creates a database in a temporary dir.
 	pub fn new(
 		genesis: Storage,
@@ -137,9 +134,9 @@ impl<B: BlockT> BenchmarkingState<B> {
 		enable_tracking: bool,
 	) -> Result<Self, String> {
 		let state_version = sp_runtime::StateVersion::default();
-		let mut root = B::Hash::default();
-		let mut mdb = MemoryDB::<HashingFor<B>>::default();
-		sp_trie::trie_types::TrieDBMutBuilderV1::<HashingFor<B>>::new(&mut mdb, &mut root).build();
+		let mut root = Default::default();
+		let mut mdb = MemoryDB::<Hasher>::default();
+		sp_trie::trie_types::TrieDBMutBuilderV1::<Hasher>::new(&mut mdb, &mut root).build();
 
 		let mut state = BenchmarkingState {
 			state: RefCell::new(None),
@@ -169,7 +166,7 @@ impl<B: BlockT> BenchmarkingState<B> {
 				child_content.data.iter().map(|(k, v)| (k.as_ref(), Some(v.as_ref()))),
 			)
 		});
-		let (root, transaction): (B::Hash, _) =
+		let (root, transaction): (Hasher::Output, _) =
 			state.state.borrow().as_ref().unwrap().full_storage_root(
 				genesis.top.iter().map(|(k, v)| (k.as_ref(), Some(v.as_ref()))),
 				child_delta,
@@ -193,9 +190,9 @@ impl<B: BlockT> BenchmarkingState<B> {
 			recorder.reset();
 			self.proof_recorder_root.set(self.root.get());
 		}
-		let storage_db = Arc::new(StorageDb::<B> { db, _block: Default::default() });
+		let storage_db = Arc::new(StorageDb::<Hasher> { db, _phantom: Default::default() });
 		*self.state.borrow_mut() = Some(
-			DbStateBuilder::<B>::new(storage_db, self.root.get())
+			DbStateBuilder::<Hasher>::new(storage_db, self.root.get())
 				.with_optional_recorder(self.proof_recorder.clone())
 				.with_cache(self.shared_trie_cache.local_cache())
 				.build(),
@@ -341,17 +338,17 @@ fn state_err() -> String {
 	"State is not open".into()
 }
 
-impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
-	type Error = <DbState<B> as StateBackend<HashingFor<B>>>::Error;
-	type TrieBackendStorage = <DbState<B> as StateBackend<HashingFor<B>>>::TrieBackendStorage;
-	type RawIter = RawIter<B>;
+impl<Hasher: Hash> StateBackend<Hasher> for BenchmarkingState<Hasher> {
+	type Error = <DbState<Hasher> as StateBackend<Hasher>>::Error;
+	type TrieBackendStorage = <DbState<Hasher> as StateBackend<Hasher>>::TrieBackendStorage;
+	type RawIter = RawIter<Hasher>;
 
 	fn storage(&self, key: &[u8]) -> Result<Option<Vec<u8>>, Self::Error> {
 		self.add_read_key(None, key);
 		self.state.borrow().as_ref().ok_or_else(state_err)?.storage(key)
 	}
 
-	fn storage_hash(&self, key: &[u8]) -> Result<Option<B::Hash>, Self::Error> {
+	fn storage_hash(&self, key: &[u8]) -> Result<Option<Hasher::Output>, Self::Error> {
 		self.add_read_key(None, key);
 		self.state.borrow().as_ref().ok_or_else(state_err)?.storage_hash(key)
 	}
@@ -373,7 +370,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 		&self,
 		child_info: &ChildInfo,
 		key: &[u8],
-	) -> Result<Option<B::Hash>, Self::Error> {
+	) -> Result<Option<Hasher::Output>, Self::Error> {
 		self.add_read_key(Some(child_info.storage_key()), key);
 		self.state
 			.borrow()
@@ -385,7 +382,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 	fn closest_merkle_value(
 		&self,
 		key: &[u8],
-	) -> Result<Option<MerkleValue<B::Hash>>, Self::Error> {
+	) -> Result<Option<MerkleValue<Hasher::Output>>, Self::Error> {
 		self.add_read_key(None, key);
 		self.state.borrow().as_ref().ok_or_else(state_err)?.closest_merkle_value(key)
 	}
@@ -394,7 +391,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 		&self,
 		child_info: &ChildInfo,
 		key: &[u8],
-	) -> Result<Option<MerkleValue<B::Hash>>, Self::Error> {
+	) -> Result<Option<MerkleValue<Hasher::Output>>, Self::Error> {
 		self.add_read_key(None, key);
 		self.state
 			.borrow()
@@ -443,7 +440,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 		&self,
 		delta: impl Iterator<Item = (&'a [u8], Option<&'a [u8]>)>,
 		state_version: StateVersion,
-	) -> (B::Hash, BackendTransaction<HashingFor<B>>) {
+	) -> (Hasher::Output, BackendTransaction<Hasher>) {
 		self.state
 			.borrow()
 			.as_ref()
@@ -455,7 +452,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 		child_info: &ChildInfo,
 		delta: impl Iterator<Item = (&'a [u8], Option<&'a [u8]>)>,
 		state_version: StateVersion,
-	) -> (B::Hash, bool, BackendTransaction<HashingFor<B>>) {
+	) -> (Hasher::Output, bool, BackendTransaction<Hasher>) {
 		self.state
 			.borrow()
 			.as_ref()
@@ -479,8 +476,8 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 
 	fn commit(
 		&self,
-		storage_root: <HashingFor<B> as Hasher>::Out,
-		mut transaction: BackendTransaction<HashingFor<B>>,
+		storage_root: <Hasher as DbHasher>::Out,
+		mut transaction: BackendTransaction<Hasher>,
 		main_storage_changes: StorageCollection,
 		child_storage_changes: ChildStorageCollection,
 	) -> Result<(), Self::Error> {
@@ -634,8 +631,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 				log::debug!(target: "benchmark", "Some proof size: {}", &proof_size);
 				proof_size
 			} else {
-				if let Some(size) = proof.encoded_compact_size::<HashingFor<B>>(proof_recorder_root)
-				{
+				if let Some(size) = proof.encoded_compact_size::<Hasher>(proof_recorder_root) {
 					size as u32
 				} else if proof_recorder_root == self.root.get() {
 					log::debug!(target: "benchmark", "No changes - no proof");
@@ -654,7 +650,7 @@ impl<B: BlockT> StateBackend<HashingFor<B>> for BenchmarkingState<B> {
 	}
 }
 
-impl<Block: BlockT> std::fmt::Debug for BenchmarkingState<Block> {
+impl<Hasher: Hash> std::fmt::Debug for BenchmarkingState<Hasher> {
 	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
 		write!(f, "Bench DB")
 	}
@@ -663,6 +659,7 @@ impl<Block: BlockT> std::fmt::Debug for BenchmarkingState<Block> {
 #[cfg(test)]
 mod test {
 	use crate::bench::BenchmarkingState;
+	use sp_runtime::traits::HashingFor;
 	use sp_state_machine::backend::Backend as _;
 
 	fn hex(hex: &str) -> Vec<u8> {
@@ -681,7 +678,8 @@ mod test {
 			..sp_runtime::Storage::default()
 		};
 		let bench_state =
-			BenchmarkingState::<crate::tests::Block>::new(storage, None, false, true).unwrap();
+			BenchmarkingState::<HashingFor<crate::tests::Block>>::new(storage, None, false, true)
+				.unwrap();
 
 		assert_eq!(bench_state.read_write_count(), (0, 0, 0, 0));
 		assert_eq!(bench_state.keys(Default::default()).unwrap().count(), 1);
@@ -690,9 +688,13 @@ mod test {
 
 	#[test]
 	fn read_to_main_and_child_tries() {
-		let bench_state =
-			BenchmarkingState::<crate::tests::Block>::new(Default::default(), None, false, true)
-				.unwrap();
+		let bench_state = BenchmarkingState::<HashingFor<crate::tests::Block>>::new(
+			Default::default(),
+			None,
+			false,
+			true,
+		)
+		.unwrap();
 
 		for _ in 0..2 {
 			let child1 = sp_core::storage::ChildInfo::new_default(b"child1");