From c36c51cac3444d65f512aafaa79647712d6f3b8a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 8 Feb 2024 18:27:52 +0100 Subject: [PATCH] `bench pallet`: only require `Hash` instead of `Block` (#3244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preparation for https://github.com/paritytech/polkadot-sdk/issues/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::(config) + cmd.run::, ()>(config) ``` Status: waiting for CI checks --------- Signed-off-by: Oliver Tale-Yazdi Co-authored-by: Bastian Köcher Co-authored-by: cheme --- Cargo.lock | 1 + .../parachain-template/node/src/command.rs | 2 +- cumulus/polkadot-parachain/src/command.rs | 2 +- polkadot/cli/Cargo.toml | 8 +- polkadot/cli/src/command.rs | 2 +- prdoc/pr_3244.prdoc | 18 ++++ .../bin/node-template/node/src/command.rs | 2 +- substrate/bin/node/cli/src/command.rs | 3 +- substrate/client/db/src/bench.rs | 98 ++++++++++--------- substrate/client/db/src/lib.rs | 51 +++++----- .../benchmarking-cli/src/pallet/command.rs | 13 ++- .../benchmarking-cli/src/storage/write.rs | 6 +- 12 files changed, 120 insertions(+), 86 deletions(-) create mode 100644 prdoc/pr_3244.prdoc diff --git a/Cargo.lock b/Cargo.lock index 49507ecd4f93..b6fa96cd0a37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12130,6 +12130,7 @@ dependencies = [ "sp-io", "sp-keyring", "sp-maybe-compressed-blob", + "sp-runtime", "substrate-build-script-utils", "thiserror", "try-runtime-cli", diff --git a/cumulus/parachain-template/node/src/command.rs b/cumulus/parachain-template/node/src/command.rs index 6ddb68a359a7..72b3ab7bb4b9 100644 --- a/cumulus/parachain-template/node/src/command.rs +++ b/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::(config)) + runner.sync_run(|config| cmd.run::, ()>(config)) } else { Err("Benchmarking wasn't enabled when building the node. \ You can enable it with `--features runtime-benchmarks`." diff --git a/cumulus/polkadot-parachain/src/command.rs b/cumulus/polkadot-parachain/src/command.rs index 19d067068f48..3be232ce6989 100644 --- a/cumulus/polkadot-parachain/src/command.rs +++ b/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::(config)) + runner.sync_run(|config| cmd.run::, ()>(config)) } else { Err("Benchmarking wasn't enabled when building the node. \ You can enable it with `--features runtime-benchmarks`." diff --git a/polkadot/cli/Cargo.toml b/polkadot/cli/Cargo.toml index 3186e31f39e8..75afa2d199f4 100644 --- a/polkadot/cli/Cargo.toml +++ b/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"] diff --git a/polkadot/cli/src/command.rs b/polkadot/cli/src/command.rs index 7319f28a2e2a..f71891ecde34 100644 --- a/polkadot/cli/src/command.rs +++ b/polkadot/cli/src/command.rs @@ -451,7 +451,7 @@ pub fn run() -> Result<()> { if cfg!(feature = "runtime-benchmarks") { runner.sync_run(|config| { - cmd.run::(config) + cmd.run::, ()>(config) .map_err(|e| Error::SubstrateCli(e)) }) } else { diff --git a/prdoc/pr_3244.prdoc b/prdoc/pr_3244.prdoc new file mode 100644 index 000000000000..3d851c8afe0d --- /dev/null +++ b/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` in the + place where `Block` was required. + Example patch for your node with `cmd` being `BenchmarkCmd::Pallet(cmd)`: + + ```patch + - cmd.run::(config) + + cmd.run::, ()>(config) + ``` + +crates: + - name: sc-client-db + - name: frame-benchmarking-cli diff --git a/substrate/bin/node-template/node/src/command.rs b/substrate/bin/node-template/node/src/command.rs index a25157693cd4..3778df664229 100644 --- a/substrate/bin/node-template/node/src/command.rs +++ b/substrate/bin/node-template/node/src/command.rs @@ -117,7 +117,7 @@ pub fn run() -> sc_cli::Result<()> { ) } - cmd.run::(config) + cmd.run::, ()>(config) }, BenchmarkCmd::Block(cmd) => { let PartialComponents { client, .. } = service::new_partial(&config)?; diff --git a/substrate/bin/node/cli/src/command.rs b/substrate/bin/node/cli/src/command.rs index dc28705c2aea..964517320286 100644 --- a/substrate/bin/node/cli/src/command.rs +++ b/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::(config) + cmd.run::, sp_statement_store::runtime_api::HostFunctions>(config) }, BenchmarkCmd::Block(cmd) => { // ensure that we keep the task manager alive diff --git a/substrate/client/db/src/bench.rs b/substrate/client/db/src/bench.rs index 03ad4817b53b..32503cf63c0a 100644 --- a/substrate/client/db/src/bench.rs +++ b/substrate/client/db/src/bench.rs @@ -19,7 +19,7 @@ //! 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; @@ -27,10 +27,7 @@ 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 = DbState; +type State = DbState; -struct StorageDb { +struct StorageDb { db: Arc, - _block: std::marker::PhantomData, + _phantom: std::marker::PhantomData, } -impl sp_state_machine::Storage> for StorageDb { - fn get(&self, key: &Block::Hash, prefix: Prefix) -> Result, String> { - let prefixed_key = prefixed_key::>(key, prefix); +impl sp_state_machine::Storage for StorageDb { + fn get(&self, key: &Hasher::Output, prefix: Prefix) -> Result, String> { + let prefixed_key = prefixed_key::(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 { - root: Cell, - genesis_root: B::Hash, - state: RefCell>>, +pub struct BenchmarkingState { + root: Cell, + genesis_root: Hasher::Output, + state: RefCell>>, db: Cell>>, genesis: HashMap, (Vec, i32)>, record: Cell>>, key_tracker: Arc>, whitelist: RefCell>, - proof_recorder: Option>>, - proof_recorder_root: Cell, - shared_trie_cache: SharedTrieCache>, + proof_recorder: Option>, + proof_recorder_root: Cell, + shared_trie_cache: SharedTrieCache, } /// A raw iterator over the `BenchmarkingState`. -pub struct RawIter { - inner: as StateBackend>>::RawIter, +pub struct RawIter { + inner: as StateBackend>::RawIter, child_trie: Option>, key_tracker: Arc>, } -impl StorageIterator> for RawIter { - type Backend = BenchmarkingState; +impl StorageIterator for RawIter { + type Backend = BenchmarkingState; type Error = String; fn next_key(&mut self, backend: &Self::Backend) -> Option> { @@ -128,7 +125,7 @@ impl StorageIterator> for RawIter { } } -impl BenchmarkingState { +impl BenchmarkingState { /// Create a new instance that creates a database in a temporary dir. pub fn new( genesis: Storage, @@ -137,9 +134,9 @@ impl BenchmarkingState { enable_tracking: bool, ) -> Result { let state_version = sp_runtime::StateVersion::default(); - let mut root = B::Hash::default(); - let mut mdb = MemoryDB::>::default(); - sp_trie::trie_types::TrieDBMutBuilderV1::>::new(&mut mdb, &mut root).build(); + let mut root = Default::default(); + let mut mdb = MemoryDB::::default(); + sp_trie::trie_types::TrieDBMutBuilderV1::::new(&mut mdb, &mut root).build(); let mut state = BenchmarkingState { state: RefCell::new(None), @@ -169,7 +166,7 @@ impl BenchmarkingState { 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 BenchmarkingState { recorder.reset(); self.proof_recorder_root.set(self.root.get()); } - let storage_db = Arc::new(StorageDb:: { db, _block: Default::default() }); + let storage_db = Arc::new(StorageDb:: { db, _phantom: Default::default() }); *self.state.borrow_mut() = Some( - DbStateBuilder::::new(storage_db, self.root.get()) + DbStateBuilder::::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 StateBackend> for BenchmarkingState { - type Error = as StateBackend>>::Error; - type TrieBackendStorage = as StateBackend>>::TrieBackendStorage; - type RawIter = RawIter; +impl StateBackend for BenchmarkingState { + type Error = as StateBackend>::Error; + type TrieBackendStorage = as StateBackend>::TrieBackendStorage; + type RawIter = RawIter; fn storage(&self, key: &[u8]) -> Result>, 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, Self::Error> { + fn storage_hash(&self, key: &[u8]) -> Result, 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 StateBackend> for BenchmarkingState { &self, child_info: &ChildInfo, key: &[u8], - ) -> Result, Self::Error> { + ) -> Result, Self::Error> { self.add_read_key(Some(child_info.storage_key()), key); self.state .borrow() @@ -385,7 +382,7 @@ impl StateBackend> for BenchmarkingState { fn closest_merkle_value( &self, key: &[u8], - ) -> Result>, Self::Error> { + ) -> Result>, 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 StateBackend> for BenchmarkingState { &self, child_info: &ChildInfo, key: &[u8], - ) -> Result>, Self::Error> { + ) -> Result>, Self::Error> { self.add_read_key(None, key); self.state .borrow() @@ -443,7 +440,7 @@ impl StateBackend> for BenchmarkingState { &self, delta: impl Iterator)>, state_version: StateVersion, - ) -> (B::Hash, BackendTransaction>) { + ) -> (Hasher::Output, BackendTransaction) { self.state .borrow() .as_ref() @@ -455,7 +452,7 @@ impl StateBackend> for BenchmarkingState { child_info: &ChildInfo, delta: impl Iterator)>, state_version: StateVersion, - ) -> (B::Hash, bool, BackendTransaction>) { + ) -> (Hasher::Output, bool, BackendTransaction) { self.state .borrow() .as_ref() @@ -479,8 +476,8 @@ impl StateBackend> for BenchmarkingState { fn commit( &self, - storage_root: as Hasher>::Out, - mut transaction: BackendTransaction>, + storage_root: ::Out, + mut transaction: BackendTransaction, main_storage_changes: StorageCollection, child_storage_changes: ChildStorageCollection, ) -> Result<(), Self::Error> { @@ -634,8 +631,7 @@ impl StateBackend> for BenchmarkingState { log::debug!(target: "benchmark", "Some proof size: {}", &proof_size); proof_size } else { - if let Some(size) = proof.encoded_compact_size::>(proof_recorder_root) - { + if let Some(size) = proof.encoded_compact_size::(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 StateBackend> for BenchmarkingState { } } -impl std::fmt::Debug for BenchmarkingState { +impl std::fmt::Debug for BenchmarkingState { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "Bench DB") } @@ -663,6 +659,7 @@ impl std::fmt::Debug for BenchmarkingState { #[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 { @@ -681,7 +678,8 @@ mod test { ..sp_runtime::Storage::default() }; let bench_state = - BenchmarkingState::::new(storage, None, false, true).unwrap(); + BenchmarkingState::>::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::::new(Default::default(), None, false, true) - .unwrap(); + let bench_state = BenchmarkingState::>::new( + Default::default(), + None, + false, + true, + ) + .unwrap(); for _ in 0..2 { let child1 = sp_core::storage::ChildInfo::new_default(b"child1"); diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs index 2d8622d5f12d..870b4150b9df 100644 --- a/substrate/client/db/src/lib.rs +++ b/substrate/client/db/src/lib.rs @@ -101,14 +101,11 @@ pub use bench::BenchmarkingState; const CACHE_HEADERS: usize = 8; /// DB-backed patricia trie state, transaction type is an overlay of changes to commit. -pub type DbState = - sp_state_machine::TrieBackend>>, HashingFor>; +pub type DbState = sp_state_machine::TrieBackend>, H>; /// Builder for [`DbState`]. -pub type DbStateBuilder = sp_state_machine::TrieBackendBuilder< - Arc>>, - HashingFor, ->; +pub type DbStateBuilder = + sp_state_machine::TrieBackendBuilder>, Hasher>; /// Length of a [`DbHash`]. const DB_HASH_LEN: usize = 32; @@ -135,13 +132,17 @@ enum DbExtrinsic { /// It makes sure that the hash we are using stays pinned in storage /// until this structure is dropped. pub struct RefTrackingState { - state: DbState, + state: DbState>, storage: Arc>, parent_hash: Option, } impl RefTrackingState { - fn new(state: DbState, storage: Arc>, parent_hash: Option) -> Self { + fn new( + state: DbState>, + storage: Arc>, + parent_hash: Option, + ) -> Self { RefTrackingState { state, parent_hash, storage } } } @@ -162,12 +163,12 @@ impl std::fmt::Debug for RefTrackingState { /// A raw iterator over the `RefTrackingState`. pub struct RawIter { - inner: as StateBackend>>::RawIter, + inner: > as StateBackend>>::RawIter, } impl StorageIterator> for RawIter { type Backend = RefTrackingState; - type Error = as StateBackend>>::Error; + type Error = > as StateBackend>>::Error; fn next_key(&mut self, backend: &Self::Backend) -> Option> { self.inner.next_key(&backend.state) @@ -186,8 +187,9 @@ impl StorageIterator> for RawIter { } impl StateBackend> for RefTrackingState { - type Error = as StateBackend>>::Error; - type TrieBackendStorage = as StateBackend>>::TrieBackendStorage; + type Error = > as StateBackend>>::Error; + type TrieBackendStorage = + > as StateBackend>>::TrieBackendStorage; type RawIter = RawIter; fn storage(&self, key: &[u8]) -> Result>, Self::Error> { @@ -284,7 +286,8 @@ impl StateBackend> for RefTrackingState { } impl AsTrieBackend> for RefTrackingState { - type TrieBackendStorage = as StateBackend>>::TrieBackendStorage; + type TrieBackendStorage = + > as StateBackend>>::TrieBackendStorage; fn as_trie_backend( &self, @@ -1936,7 +1939,7 @@ impl Backend { fn empty_state(&self) -> RecordStatsState, Block> { let root = EmptyStorage::::new().0; // Empty trie - let db_state = DbStateBuilder::::new(self.storage.clone(), root) + let db_state = DbStateBuilder::>::new(self.storage.clone(), root) .with_optional_cache(self.shared_trie_cache.as_ref().map(|c| c.local_cache())) .build(); let state = RefTrackingState::new(db_state, self.storage.clone(), None); @@ -2428,9 +2431,12 @@ impl sc_client_api::backend::Backend for Backend { if hash == self.blockchain.meta.read().genesis_hash { if let Some(genesis_state) = &*self.genesis_state.read() { let root = genesis_state.root; - let db_state = DbStateBuilder::::new(genesis_state.clone(), root) - .with_optional_cache(self.shared_trie_cache.as_ref().map(|c| c.local_cache())) - .build(); + let db_state = + DbStateBuilder::>::new(genesis_state.clone(), root) + .with_optional_cache( + self.shared_trie_cache.as_ref().map(|c| c.local_cache()), + ) + .build(); let state = RefTrackingState::new(db_state, self.storage.clone(), None); return Ok(RecordStatsState::new(state, None, self.state_usage.clone())) @@ -2449,11 +2455,12 @@ impl sc_client_api::backend::Backend for Backend { self.storage.state_db.pin(&hash, hdr.number.saturated_into::(), hint) { let root = hdr.state_root; - let db_state = DbStateBuilder::::new(self.storage.clone(), root) - .with_optional_cache( - self.shared_trie_cache.as_ref().map(|c| c.local_cache()), - ) - .build(); + let db_state = + DbStateBuilder::>::new(self.storage.clone(), root) + .with_optional_cache( + self.shared_trie_cache.as_ref().map(|c| c.local_cache()), + ) + .build(); let state = RefTrackingState::new(db_state, self.storage.clone(), Some(hash)); Ok(RecordStatsState::new(state, Some(hash), self.state_usage.clone())) } else { diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs index 5c76ca68e85f..dce49db15f7d 100644 --- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs +++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs @@ -37,7 +37,7 @@ use sp_core::{ }; use sp_externalities::Extensions; use sp_keystore::{testing::MemoryKeystore, KeystoreExt}; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; +use sp_runtime::traits::Hash; use sp_state_machine::StateMachine; use std::{collections::HashMap, fmt::Debug, fs, str::FromStr, time}; @@ -140,11 +140,10 @@ This could mean that you either did not build the node correctly with the \ not created by a node that was compiled with the flag"; impl PalletCmd { - /// Runs the command and benchmarks the chain. - pub fn run(&self, config: Configuration) -> Result<()> + /// Runs the command and benchmarks a pallet. + pub fn run(&self, config: Configuration) -> Result<()> where - BB: BlockT + Debug, - <<::Header as HeaderT>::Number as std::str::FromStr>::Err: std::fmt::Debug, + Hasher: Hash, ExtraHostFunctions: sp_wasm_interface::HostFunctions, { let _d = self.execution.as_ref().map(|exec| { @@ -199,7 +198,7 @@ impl PalletCmd { let genesis_storage = spec.build_storage()?; let mut changes = Default::default(); let cache_size = Some(self.database_cache_size as usize); - let state_with_tracking = BenchmarkingState::::new( + let state_with_tracking = BenchmarkingState::::new( genesis_storage.clone(), cache_size, // Record proof size @@ -207,7 +206,7 @@ impl PalletCmd { // Enable storage tracking true, )?; - let state_without_tracking = BenchmarkingState::::new( + let state_without_tracking = BenchmarkingState::::new( genesis_storage, cache_size, // Do not record proof size diff --git a/substrate/utils/frame/benchmarking-cli/src/storage/write.rs b/substrate/utils/frame/benchmarking-cli/src/storage/write.rs index 65941497bda4..4fa56b6e818f 100644 --- a/substrate/utils/frame/benchmarking-cli/src/storage/write.rs +++ b/substrate/utils/frame/benchmarking-cli/src/storage/write.rs @@ -57,7 +57,7 @@ impl StorageCmd { let best_hash = client.usage_info().chain.best_hash; let header = client.header(best_hash)?.ok_or("Header not found")?; let original_root = *header.state_root(); - let trie = DbStateBuilder::::new(storage.clone(), original_root).build(); + let trie = DbStateBuilder::>::new(storage.clone(), original_root).build(); info!("Preparing keys from block {}", best_hash); // Load all KV pairs and randomly shuffle them. @@ -189,7 +189,7 @@ fn convert_tx( /// if `child_info` exist then it means this is a child tree key fn measure_write( db: Arc>, - trie: &DbState, + trie: &DbState>, key: Vec, new_v: Vec, version: StateVersion, @@ -220,7 +220,7 @@ fn measure_write( /// if `child_info` exist then it means this is a child tree key fn check_new_value( db: Arc>, - trie: &DbState, + trie: &DbState>, key: &Vec, new_v: &Vec, version: StateVersion,