From 287ecc2974f14aa5d51f4e2ba6efac32d4093a50 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 17 Aug 2020 19:36:29 +0200 Subject: [PATCH 1/9] pow: add access to pre-digest for algorithm verifiers (#6900) * pow: fetch pre-runtime digest to verifier * Add Other error type * Fix log target and change docs to refer to pre_runtime --- client/consensus/pow/src/lib.rs | 41 +++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index f8da1877665fc..ca1a8584e2a0b 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -88,10 +88,13 @@ pub enum Error { CreateInherents(sp_inherents::Error), #[display(fmt = "Checking inherents failed: {}", _0)] CheckInherents(String), + #[display(fmt = "Multiple pre-runtime digests")] + MultiplePreRuntimeDigests, Client(sp_blockchain::Error), Codec(codec::Error), Environment(String), - Runtime(RuntimeString) + Runtime(RuntimeString), + Other(String), } impl std::convert::From> for String { @@ -172,6 +175,7 @@ pub trait PowAlgorithm { &self, parent: &BlockId, pre_hash: &B::Hash, + pre_digest: Option<&[u8]>, seal: &Seal, difficulty: Self::Difficulty, ) -> Result>; @@ -180,6 +184,7 @@ pub trait PowAlgorithm { &self, parent: &BlockId, pre_hash: &B::Hash, + pre_digest: Option<&[u8]>, difficulty: Self::Difficulty, round: u32, ) -> Result, Error>; @@ -368,9 +373,11 @@ impl BlockImport for PowBlockImport(&block.header)?; if !self.algorithm.verify( &BlockId::hash(parent_hash), &pre_hash, + pre_digest.as_ref().map(|v| &v[..]), &inner_seal, difficulty, )? { @@ -519,7 +526,7 @@ pub fn import_queue( /// However, it's not recommended to use background threads in the rest of the /// codebase. /// -/// `preruntime` is a parameter that allows a custom additional pre-runtime +/// `pre_runtime` is a parameter that allows a custom additional pre-runtime /// digest to be inserted for blocks being built. This can encode authorship /// information, or just be a graffiti. `round` is for number of rounds the /// CPU miner runs each time. This parameter should be tweaked so that each @@ -529,7 +536,7 @@ pub fn start_mine( client: Arc, algorithm: Algorithm, mut env: E, - preruntime: Option>, + pre_runtime: Option>, round: u32, mut sync_oracle: SO, build_time: std::time::Duration, @@ -557,7 +564,7 @@ pub fn start_mine( client.as_ref(), &algorithm, &mut env, - preruntime.as_ref(), + pre_runtime.as_ref(), round, &mut sync_oracle, build_time.clone(), @@ -581,7 +588,7 @@ fn mine_loop( client: &C, algorithm: &Algorithm, env: &mut E, - preruntime: Option<&Vec>, + pre_runtime: Option<&Vec>, round: u32, sync_oracle: &mut SO, build_time: std::time::Duration, @@ -640,8 +647,8 @@ fn mine_loop( let inherent_data = inherent_data_providers .create_inherent_data().map_err(Error::CreateInherents)?; let mut inherent_digest = Digest::default(); - if let Some(preruntime) = &preruntime { - inherent_digest.push(DigestItem::PreRuntime(POW_ENGINE_ID, preruntime.to_vec())); + if let Some(pre_runtime) = &pre_runtime { + inherent_digest.push(DigestItem::PreRuntime(POW_ENGINE_ID, pre_runtime.to_vec())); } let proposal = futures::executor::block_on(proposer.propose( inherent_data, @@ -658,6 +665,7 @@ fn mine_loop( let seal = algorithm.mine( &BlockId::Hash(best_hash), &header.hash(), + pre_runtime.map(|v| &v[..]), difficulty, round, )?; @@ -702,3 +710,22 @@ fn mine_loop( .map_err(|e| Error::BlockBuiltError(best_hash, e))?; } } + +/// Find PoW pre-runtime. +fn find_pre_digest(header: &B::Header) -> Result>, Error> { + let mut pre_digest: Option<_> = None; + for log in header.digest().logs() { + trace!(target: "pow", "Checking log {:?}, looking for pre runtime digest", log); + match (log, pre_digest.is_some()) { + (DigestItem::PreRuntime(POW_ENGINE_ID, _), true) => { + return Err(Error::MultiplePreRuntimeDigests) + }, + (DigestItem::PreRuntime(POW_ENGINE_ID, v), false) => { + pre_digest = Some(v.clone()); + }, + (_, _) => trace!(target: "pow", "Ignoring digest not meant for us"), + } + } + + Ok(pre_digest) +} From 399421abeb3de5b6bf7bbd1531764c5f94206eaa Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 17 Aug 2020 21:07:30 +0200 Subject: [PATCH 2/9] Derive Clone for AlwaysCanAuthor, NeverCanAuthor, CanAuthorWithNativeVersion (#6906) --- primitives/consensus/common/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/primitives/consensus/common/src/lib.rs b/primitives/consensus/common/src/lib.rs index 0e4dd91dd497e..fa4f233c680fa 100644 --- a/primitives/consensus/common/src/lib.rs +++ b/primitives/consensus/common/src/lib.rs @@ -212,6 +212,7 @@ pub trait CanAuthorWith { /// Checks if the node can author blocks by using /// [`NativeVersion::can_author_with`](sp_version::NativeVersion::can_author_with). +#[derive(Clone)] pub struct CanAuthorWithNativeVersion(T); impl CanAuthorWithNativeVersion { @@ -239,6 +240,7 @@ impl, Block: BlockT> CanAuthorWith CanAuthorWith for AlwaysCanAuthor { @@ -248,6 +250,7 @@ impl CanAuthorWith for AlwaysCanAuthor { } /// Never can author. +#[derive(Clone)] pub struct NeverCanAuthor; impl CanAuthorWith for NeverCanAuthor { From 8e1ed7d96df71688820788eff6939b8cbec8803c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 17 Aug 2020 22:59:23 +0200 Subject: [PATCH 3/9] WeightInfo for System, Timestamp, and Utility (#6868) * initial updates to system * fix compile * Update writer.rs * update weights * finish system weights * timestamp weights * utility weight * Fix overflow in weight calculations * add back weight notes * Update for whitelisted benchmarks * add trait bounds * Revert "add trait bounds" This reverts commit 12b08b7189aa3969f96fa19b211a370860fdb240. * Update weights for unaccounted for read --- bin/node/runtime/src/lib.rs | 6 +- bin/node/runtime/src/weights/frame_system.rs | 58 +++++++++++++++++++ bin/node/runtime/src/weights/mod.rs | 3 + .../runtime/src/weights/pallet_timestamp.rs | 34 +++++++++++ .../runtime/src/weights/pallet_utility.rs | 35 +++++++++++ frame/system/benchmarking/src/lib.rs | 21 +++---- frame/system/src/default_weights.rs | 57 ++++++++++++++++++ frame/system/src/lib.rs | 38 ++++-------- frame/timestamp/src/default_weights.rs | 35 +++++++++++ frame/timestamp/src/lib.rs | 18 ++---- frame/utility/src/default_weights.rs | 34 +++++++++++ frame/utility/src/lib.rs | 19 ++---- frame/utility/src/tests.rs | 20 ++++++- utils/frame/benchmarking-cli/src/writer.rs | 6 ++ 14 files changed, 316 insertions(+), 68 deletions(-) create mode 100644 bin/node/runtime/src/weights/frame_system.rs create mode 100644 bin/node/runtime/src/weights/pallet_timestamp.rs create mode 100644 bin/node/runtime/src/weights/pallet_utility.rs create mode 100644 frame/system/src/default_weights.rs create mode 100644 frame/timestamp/src/default_weights.rs create mode 100644 frame/utility/src/default_weights.rs diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index acc1b07281834..aa0ddfc61a7bb 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -182,13 +182,13 @@ impl frame_system::Trait for Runtime { type AccountData = pallet_balances::AccountData; type OnNewAccount = (); type OnKilledAccount = (); - type SystemWeightInfo = (); + type SystemWeightInfo = weights::frame_system::WeightInfo; } impl pallet_utility::Trait for Runtime { type Event = Event; type Call = Call; - type WeightInfo = (); + type WeightInfo = weights::pallet_utility::WeightInfo; } parameter_types! { @@ -352,7 +352,7 @@ impl pallet_timestamp::Trait for Runtime { type Moment = Moment; type OnTimestampSet = Babe; type MinimumPeriod = MinimumPeriod; - type WeightInfo = (); + type WeightInfo = weights::pallet_timestamp::WeightInfo; } parameter_types! { diff --git a/bin/node/runtime/src/weights/frame_system.rs b/bin/node/runtime/src/weights/frame_system.rs new file mode 100644 index 0000000000000..9522fa7520390 --- /dev/null +++ b/bin/node/runtime/src/weights/frame_system.rs @@ -0,0 +1,58 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +pub struct WeightInfo; +impl frame_system::WeightInfo for WeightInfo { + // WARNING! Some components were not used: ["b"] + fn remark() -> Weight { + (1305000 as Weight) + } + fn set_heap_pages() -> Weight { + (2023000 as Weight) + .saturating_add(DbWeight::get().writes(1 as Weight)) + } + // WARNING! Some components were not used: ["d"] + fn set_changes_trie_config() -> Weight { + (10026000 as Weight) + .saturating_add(DbWeight::get().reads(1 as Weight)) + .saturating_add(DbWeight::get().writes(2 as Weight)) + } + fn set_storage(i: u32, ) -> Weight { + (0 as Weight) + .saturating_add((656000 as Weight).saturating_mul(i as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(i as Weight))) + } + fn kill_storage(i: u32, ) -> Weight { + (4327000 as Weight) + .saturating_add((478000 as Weight).saturating_mul(i as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(i as Weight))) + } + fn kill_prefix(p: u32, ) -> Weight { + (8349000 as Weight) + .saturating_add((838000 as Weight).saturating_mul(p as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(p as Weight))) + } + fn suicide() -> Weight { + (29247000 as Weight) + } +} diff --git a/bin/node/runtime/src/weights/mod.rs b/bin/node/runtime/src/weights/mod.rs index 70bae879ce05c..0e078e7ac0859 100644 --- a/bin/node/runtime/src/weights/mod.rs +++ b/bin/node/runtime/src/weights/mod.rs @@ -15,5 +15,8 @@ //! A list of the different weight modules for our runtime. +pub mod frame_system; pub mod pallet_balances; pub mod pallet_democracy; +pub mod pallet_timestamp; +pub mod pallet_utility; diff --git a/bin/node/runtime/src/weights/pallet_timestamp.rs b/bin/node/runtime/src/weights/pallet_timestamp.rs new file mode 100644 index 0000000000000..cfd5f192d3529 --- /dev/null +++ b/bin/node/runtime/src/weights/pallet_timestamp.rs @@ -0,0 +1,34 @@ +// Copyright (C) 2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +pub struct WeightInfo; +impl pallet_timestamp::WeightInfo for WeightInfo { + // WARNING! Some components were not used: ["t"] + fn set() -> Weight { + (9133000 as Weight) + .saturating_add(DbWeight::get().reads(2 as Weight)) + .saturating_add(DbWeight::get().writes(1 as Weight)) + } + // WARNING! Some components were not used: ["t"] + fn on_finalize() -> Weight { + (5915000 as Weight) + } +} diff --git a/bin/node/runtime/src/weights/pallet_utility.rs b/bin/node/runtime/src/weights/pallet_utility.rs new file mode 100644 index 0000000000000..c9ae0d7d2333b --- /dev/null +++ b/bin/node/runtime/src/weights/pallet_utility.rs @@ -0,0 +1,35 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +pub struct WeightInfo; +impl pallet_utility::WeightInfo for WeightInfo { + fn batch(c: u32, ) -> Weight { + (16461000 as Weight) + .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + } + // WARNING! Some components were not used: ["u"] + fn as_derivative() -> Weight { + (4086000 as Weight) + } +} diff --git a/frame/system/benchmarking/src/lib.rs b/frame/system/benchmarking/src/lib.rs index 049fa5298c677..a3e7797996ac9 100644 --- a/frame/system/benchmarking/src/lib.rs +++ b/frame/system/benchmarking/src/lib.rs @@ -25,6 +25,7 @@ use sp_std::prelude::*; use sp_core::{ChangesTrieConfiguration, storage::well_known_keys}; use sp_runtime::traits::Hash; use frame_benchmarking::{benchmarks, account}; +use frame_support::traits::Get; use frame_support::storage::{self, StorageMap}; use frame_system::{Module as System, Call, RawOrigin, DigestItemOf, AccountInfo}; @@ -39,29 +40,26 @@ benchmarks! { _ { } remark { - // # of Bytes - let b in 0 .. 16_384; + let b in 0 .. T::MaximumBlockLength::get(); let remark_message = vec![1; b as usize]; let caller = account("caller", 0, SEED); }: _(RawOrigin::Signed(caller), remark_message) set_heap_pages { - // Heap page size - let i in 0 .. u32::max_value(); - }: _(RawOrigin::Root, i.into()) + }: _(RawOrigin::Root, Default::default()) // `set_code` was not benchmarked because it is pretty hard to come up with a real // Wasm runtime to test the upgrade with. But this is okay because we will make // `set_code` take a full block anyway. + #[extra] set_code_without_checks { - // Version number - let b in 0 .. 16_384; - let code = vec![1; b as usize]; + // Assume Wasm ~4MB + let code = vec![1; 4_000_000 as usize]; }: _(RawOrigin::Root, code) verify { let current_code = storage::unhashed::get_raw(well_known_keys::CODE).ok_or("Code not stored.")?; - assert_eq!(current_code.len(), b as usize); + assert_eq!(current_code.len(), 4_000_000 as usize); } set_changes_trie_config { @@ -141,16 +139,15 @@ benchmarks! { } suicide { - let n in 1 .. 1000; let caller: T::AccountId = account("caller", 0, SEED); let account_info = AccountInfo:: { - nonce: n.into(), + nonce: 1337.into(), refcount: 0, data: T::AccountData::default() }; frame_system::Account::::insert(&caller, account_info); let new_account_info = System::::account(caller.clone()); - assert_eq!(new_account_info.nonce, n.into()); + assert_eq!(new_account_info.nonce, 1337.into()); }: _(RawOrigin::Signed(caller.clone())) verify { let account_info = System::::account(&caller); diff --git a/frame/system/src/default_weights.rs b/frame/system/src/default_weights.rs new file mode 100644 index 0000000000000..8a84cb0b7903a --- /dev/null +++ b/frame/system/src/default_weights.rs @@ -0,0 +1,57 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +impl crate::WeightInfo for () { + // WARNING! Some components were not used: ["b"] + fn remark() -> Weight { + (1305000 as Weight) + } + fn set_heap_pages() -> Weight { + (2023000 as Weight) + .saturating_add(DbWeight::get().writes(1 as Weight)) + } + // WARNING! Some components were not used: ["d"] + fn set_changes_trie_config() -> Weight { + (10026000 as Weight) + .saturating_add(DbWeight::get().reads(1 as Weight)) + .saturating_add(DbWeight::get().writes(2 as Weight)) + } + fn set_storage(i: u32, ) -> Weight { + (0 as Weight) + .saturating_add((656000 as Weight).saturating_mul(i as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(i as Weight))) + } + fn kill_storage(i: u32, ) -> Weight { + (4327000 as Weight) + .saturating_add((478000 as Weight).saturating_mul(i as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(i as Weight))) + } + fn kill_prefix(p: u32, ) -> Weight { + (8349000 as Weight) + .saturating_add((838000 as Weight).saturating_mul(p as Weight)) + .saturating_add(DbWeight::get().writes((1 as Weight).saturating_mul(p as Weight))) + } + fn suicide() -> Weight { + (29247000 as Weight) + } +} diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 30d5d019fc5ff..fcd31923a2453 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -139,6 +139,7 @@ mod extensions; mod weights; #[cfg(test)] mod tests; +mod default_weights; pub use extensions::{ check_mortality::CheckMortality, check_genesis::CheckGenesis, check_nonce::CheckNonce, @@ -159,25 +160,13 @@ pub fn extrinsics_data_root(xts: Vec>) -> H::Output { } pub trait WeightInfo { - fn remark(b: u32, ) -> Weight; - fn set_heap_pages(i: u32, ) -> Weight; - fn set_code_without_checks(b: u32, ) -> Weight; - fn set_changes_trie_config(d: u32, ) -> Weight; + fn remark() -> Weight; + fn set_heap_pages() -> Weight; + fn set_changes_trie_config() -> Weight; fn set_storage(i: u32, ) -> Weight; fn kill_storage(i: u32, ) -> Weight; fn kill_prefix(p: u32, ) -> Weight; - fn suicide(n: u32, ) -> Weight; -} - -impl WeightInfo for () { - fn remark(_b: u32, ) -> Weight { 1_000_000_000 } - fn set_heap_pages(_i: u32, ) -> Weight { 1_000_000_000 } - fn set_code_without_checks(_b: u32, ) -> Weight { 1_000_000_000 } - fn set_changes_trie_config(_d: u32, ) -> Weight { 1_000_000_000 } - fn set_storage(_i: u32, ) -> Weight { 1_000_000_000 } - fn kill_storage(_i: u32, ) -> Weight { 1_000_000_000 } - fn kill_prefix(_p: u32, ) -> Weight { 1_000_000_000 } - fn suicide(_n: u32, ) -> Weight { 1_000_000_000 } + fn suicide() -> Weight; } pub trait Trait: 'static + Eq + Clone { @@ -564,7 +553,7 @@ decl_module! { /// - Base Weight: 0.665 µs, independent of remark length. /// - No DB operations. /// # - #[weight = 700_000] + #[weight = T::SystemWeightInfo::remark()] fn remark(origin, _remark: Vec) { ensure_signed(origin)?; } @@ -577,7 +566,7 @@ decl_module! { /// - Base Weight: 1.405 µs /// - 1 write to HEAP_PAGES /// # - #[weight = (T::DbWeight::get().writes(1) + 1_500_000, DispatchClass::Operational)] + #[weight = (T::SystemWeightInfo::set_heap_pages(), DispatchClass::Operational)] fn set_heap_pages(origin, pages: u64) { ensure_root(origin)?; storage::unhashed::put_raw(well_known_keys::HEAP_PAGES, &pages.encode()); @@ -627,7 +616,7 @@ decl_module! { /// - DB Weight: /// - Writes: Changes Trie, System Digest /// # - #[weight = (T::DbWeight::get().writes(2) + 10_000_000, DispatchClass::Operational)] + #[weight = (T::SystemWeightInfo::set_changes_trie_config(), DispatchClass::Operational)] pub fn set_changes_trie_config(origin, changes_trie_config: Option) { ensure_root(origin)?; match changes_trie_config.clone() { @@ -653,8 +642,7 @@ decl_module! { /// - Writes: Number of items /// # #[weight = ( - T::DbWeight::get().writes(items.len() as Weight) - .saturating_add((items.len() as Weight).saturating_mul(600_000)), + T::SystemWeightInfo::set_storage(items.len() as u32), DispatchClass::Operational, )] fn set_storage(origin, items: Vec) { @@ -673,8 +661,7 @@ decl_module! { /// - Writes: Number of items /// # #[weight = ( - T::DbWeight::get().writes(keys.len() as Weight) - .saturating_add((keys.len() as Weight).saturating_mul(400_000)), + T::SystemWeightInfo::kill_storage(keys.len() as u32), DispatchClass::Operational, )] fn kill_storage(origin, keys: Vec) { @@ -696,8 +683,7 @@ decl_module! { /// - Writes: Number of subkeys + 1 /// # #[weight = ( - T::DbWeight::get().writes(Weight::from(*_subkeys) + 1) - .saturating_add((Weight::from(*_subkeys) + 1).saturating_mul(850_000)), + T::SystemWeightInfo::kill_prefix(_subkeys.saturating_add(1)), DispatchClass::Operational, )] fn kill_prefix(origin, prefix: Key, _subkeys: u32) { @@ -715,7 +701,7 @@ decl_module! { /// Base Weight: 8.626 µs /// No DB Read or Write operations because caller is already in overlay /// # - #[weight = (10_000_000, DispatchClass::Operational)] + #[weight = (T::SystemWeightInfo::suicide(), DispatchClass::Operational)] pub fn suicide(origin) { let who = ensure_signed(origin)?; let account = Account::::get(&who); diff --git a/frame/timestamp/src/default_weights.rs b/frame/timestamp/src/default_weights.rs new file mode 100644 index 0000000000000..726b3444e2532 --- /dev/null +++ b/frame/timestamp/src/default_weights.rs @@ -0,0 +1,35 @@ +// This file is part of Substrate. + +// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +impl crate::WeightInfo for () { + // WARNING! Some components were not used: ["t"] + fn set() -> Weight { + (9133000 as Weight) + .saturating_add(DbWeight::get().reads(2 as Weight)) + .saturating_add(DbWeight::get().writes(1 as Weight)) + } + // WARNING! Some components were not used: ["t"] + fn on_finalize() -> Weight { + (5915000 as Weight) + } +} diff --git a/frame/timestamp/src/lib.rs b/frame/timestamp/src/lib.rs index 1177165abed8c..d74a94cb9201b 100644 --- a/frame/timestamp/src/lib.rs +++ b/frame/timestamp/src/lib.rs @@ -93,6 +93,7 @@ #![cfg_attr(not(feature = "std"), no_std)] mod benchmarking; +mod default_weights; use sp_std::{result, cmp}; use sp_inherents::{ProvideInherent, InherentData, InherentIdentifier}; @@ -116,13 +117,8 @@ use sp_timestamp::{ }; pub trait WeightInfo { - fn set(t: u32, ) -> Weight; - fn on_finalize(t: u32, ) -> Weight; -} - -impl WeightInfo for () { - fn set(_t: u32, ) -> Weight { 1_000_000_000 } - fn on_finalize(_t: u32, ) -> Weight { 1_000_000_000 } + fn set() -> Weight; + fn on_finalize() -> Weight; } /// The module configuration trait @@ -166,12 +162,9 @@ decl_module! { /// - `O(T)` where `T` complexity of `on_timestamp_set` /// - 1 storage read and 1 storage mutation (codec `O(1)`). (because of `DidUpdate::take` in `on_finalize`) /// - 1 event handler `on_timestamp_set` `O(T)`. - /// - Benchmark: 7.678 (min squares analysis) - /// - NOTE: This benchmark was done for a runtime with insignificant `on_timestamp_set` handlers. - /// New benchmarking is needed when adding new handlers. /// # #[weight = ( - T::DbWeight::get().reads_writes(2, 1) + 8_000_000, + T::WeightInfo::set(), DispatchClass::Mandatory )] fn set(origin, #[compact] now: T::Moment) { @@ -191,13 +184,12 @@ decl_module! { /// dummy `on_initialize` to return the weight used in `on_finalize`. fn on_initialize() -> Weight { // weight of `on_finalize` - 5_000_000 + T::WeightInfo::on_finalize() } /// # /// - `O(1)` /// - 1 storage deletion (codec `O(1)`). - /// - Benchmark: 4.928 µs (min squares analysis) /// # fn on_finalize() { assert!(::DidUpdate::take(), "Timestamp must be updated once in the block"); diff --git a/frame/utility/src/default_weights.rs b/frame/utility/src/default_weights.rs new file mode 100644 index 0000000000000..d023dbddd4f80 --- /dev/null +++ b/frame/utility/src/default_weights.rs @@ -0,0 +1,34 @@ +// This file is part of Substrate. + +// Copyright (C) 2019-2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 + +#![allow(unused_parens)] +#![allow(unused_imports)] + +use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; + +impl crate::WeightInfo for () { + fn batch(c: u32, ) -> Weight { + (16461000 as Weight) + .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + } + // WARNING! Some components were not used: ["u"] + fn as_derivative() -> Weight { + (4086000 as Weight) + } +} diff --git a/frame/utility/src/lib.rs b/frame/utility/src/lib.rs index cf2ea9119b973..d67fdc85db5a5 100644 --- a/frame/utility/src/lib.rs +++ b/frame/utility/src/lib.rs @@ -69,15 +69,11 @@ use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable}; mod tests; mod benchmarking; +mod default_weights; pub trait WeightInfo { fn batch(c: u32, ) -> Weight; - fn as_derivative(u: u32, ) -> Weight; -} - -impl WeightInfo for () { - fn batch(_c: u32, ) -> Weight { 1_000_000_000 } - fn as_derivative(_u: u32, ) -> Weight { 1_000_000_000 } + fn as_derivative() -> Weight; } /// Configuration trait. @@ -145,7 +141,8 @@ decl_module! { #[weight = ( calls.iter() .map(|call| call.get_dispatch_info().weight) - .fold(15_000_000, |a: Weight, n| a.saturating_add(n).saturating_add(1_000_000)), + .fold(0, |total: Weight, weight: Weight| total.saturating_add(weight)) + .saturating_add(T::WeightInfo::batch(calls.len() as u32)), { let all_operational = calls.iter() .map(|call| call.get_dispatch_info().class) @@ -186,13 +183,9 @@ decl_module! { /// NOTE: Prior to version *12, this was called `as_limited_sub`. /// /// The dispatch origin for this call must be _Signed_. - /// - /// # - /// - Base weight: 2.861 µs - /// - Plus the weight of the `call` - /// # #[weight = ( - call.get_dispatch_info().weight.saturating_add(3_000_000), + T::WeightInfo::as_derivative() + .saturating_add(call.get_dispatch_info().weight), call.get_dispatch_info().class, )] fn as_derivative(origin, index: u16, call: Box<::Call>) -> DispatchResult { diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index 6de70506e452c..611c42907ca04 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -54,7 +54,7 @@ impl_outer_dispatch! { pub struct Test; parameter_types! { pub const BlockHashCount: u64 = 250; - pub const MaximumBlockWeight: Weight = 1024; + pub const MaximumBlockWeight: Weight = Weight::max_value(); pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); } @@ -121,6 +121,7 @@ type System = frame_system::Module; type Balances = pallet_balances::Module; type Utility = Module; +use frame_system::Call as SystemCall; use pallet_balances::Call as BalancesCall; use pallet_balances::Error as BalancesError; @@ -236,3 +237,20 @@ fn batch_early_exit_works() { assert_eq!(Balances::free_balance(2), 15); }); } + +#[test] +fn batch_weight_calculation_doesnt_overflow() { + new_test_ext().execute_with(|| { + let big_call = Call::System(SystemCall::fill_block(Perbill::from_percent(50))); + assert_eq!(big_call.get_dispatch_info().weight, Weight::max_value() / 2); + + // 3 * 50% saturates to 100% + let batch_call = Call::Utility(crate::Call::batch(vec![ + big_call.clone(), + big_call.clone(), + big_call.clone(), + ])); + + assert_eq!(batch_call.get_dispatch_info().weight, Weight::max_value()); + }); +} diff --git a/utils/frame/benchmarking-cli/src/writer.rs b/utils/frame/benchmarking-cli/src/writer.rs index 2bc17aa85bddb..964c1bf5fc112 100644 --- a/utils/frame/benchmarking-cli/src/writer.rs +++ b/utils/frame/benchmarking-cli/src/writer.rs @@ -107,6 +107,12 @@ pub fn write_results(batches: &[BenchmarkBatch]) -> Result<(), std::io::Error> { VERSION, )?; + // allow statements + write!( + file, + "#![allow(unused_parens)]\n#![allow(unused_imports)]\n\n", + )?; + // general imports write!( file, From f8c83bd5aec65dad638daaaf49ede837165e026c Mon Sep 17 00:00:00 2001 From: Roman Borschel Date: Tue, 18 Aug 2020 07:59:32 +0200 Subject: [PATCH 4/9] Add support for sourced metrics. (#6895) * Add support for sourced metrics. A sourced metric is a metric that obtains its values from an existing source, rather than the values being independently recorded. It thus allows collecting metrics from existing counters or gauges without having to duplicate them in a dedicated prometheus counter or gauge (and hence another atomic value). The first use-case is to feed the bandwidth counters from libp2p directly into prometheus. * Tabs, not spaces. * Tweak bandwidth counter registration. * Add debug assertion for variable labels and values. * Document monotonicity requirement for sourced counters. * CI * Update client/network/src/service.rs Co-authored-by: Max Inden Co-authored-by: Max Inden --- client/network/src/service.rs | 59 +++++++++---- utils/prometheus/src/lib.rs | 3 + utils/prometheus/src/sourced.rs | 143 ++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 18 deletions(-) create mode 100644 utils/prometheus/src/sourced.rs diff --git a/client/network/src/service.rs b/client/network/src/service.rs index d42af16f1d22e..713357772d417 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -53,6 +53,7 @@ use parking_lot::Mutex; use prometheus_endpoint::{ register, Counter, CounterVec, Gauge, GaugeVec, Histogram, HistogramOpts, HistogramVec, Opts, PrometheusError, Registry, U64, + SourcedCounter, MetricSource }; use sc_peerset::PeersetHandle; use sp_consensus::import_queue::{BlockImportError, BlockImportResult, ImportQueue, Link}; @@ -240,12 +241,6 @@ impl NetworkWorker { local_peer_id_legacy ); - // Initialize the metrics. - let metrics = match ¶ms.metrics_registry { - Some(registry) => Some(Metrics::register(®istry)?), - None => None - }; - let checker = params.on_demand.as_ref() .map(|od| od.checker().clone()) .unwrap_or_else(|| Arc::new(AlwaysBadChecker)); @@ -353,6 +348,17 @@ impl NetworkWorker { (builder.build(), bandwidth) }; + // Initialize the metrics. + let metrics = match ¶ms.metrics_registry { + Some(registry) => { + // Sourced metrics. + BandwidthCounters::register(registry, bandwidth.clone())?; + // Other (i.e. new) metrics. + Some(Metrics::register(registry)?) + } + None => None + }; + // Listen on multiaddresses. for addr in ¶ms.network_config.listen_addresses { if let Err(err) = Swarm::::listen_on(&mut swarm, addr.clone()) { @@ -1152,9 +1158,6 @@ struct Metrics { kbuckets_num_nodes: GaugeVec, listeners_local_addresses: Gauge, listeners_errors_total: Counter, - // Note: `network_bytes_total` is a monotonic gauge obtained by - // sampling an existing counter. - network_bytes_total: GaugeVec, notifications_sizes: HistogramVec, notifications_streams_closed_total: CounterVec, notifications_streams_opened_total: CounterVec, @@ -1168,6 +1171,35 @@ struct Metrics { requests_out_started_total: CounterVec, } +/// The source for bandwidth metrics. +#[derive(Clone)] +struct BandwidthCounters(Arc); + +impl BandwidthCounters { + fn register(registry: &Registry, sinks: Arc) + -> Result<(), PrometheusError> + { + register(SourcedCounter::new( + &Opts::new( + "sub_libp2p_network_bytes_total", + "Total bandwidth usage" + ).variable_label("direction"), + BandwidthCounters(sinks), + )?, registry)?; + + Ok(()) + } +} + +impl MetricSource for BandwidthCounters { + type N = u64; + + fn collect(&self, mut set: impl FnMut(&[&str], Self::N)) { + set(&[&"in"], self.0.total_inbound()); + set(&[&"out"], self.0.total_outbound()); + } +} + impl Metrics { fn register(registry: &Registry) -> Result { Ok(Self { @@ -1271,13 +1303,6 @@ impl Metrics { "sub_libp2p_listeners_errors_total", "Total number of non-fatal errors reported by a listener" )?, registry)?, - network_bytes_total: register(GaugeVec::new( - Opts::new( - "sub_libp2p_network_bytes_total", - "Total bandwidth usage" - ), - &["direction"] - )?, registry)?, notifications_sizes: register(HistogramVec::new( HistogramOpts { common_opts: Opts::new( @@ -1725,8 +1750,6 @@ impl Future for NetworkWorker { this.is_major_syncing.store(is_major_syncing, Ordering::Relaxed); if let Some(metrics) = this.metrics.as_ref() { - metrics.network_bytes_total.with_label_values(&["in"]).set(this.service.bandwidth.total_inbound()); - metrics.network_bytes_total.with_label_values(&["out"]).set(this.service.bandwidth.total_outbound()); metrics.is_major_syncing.set(is_major_syncing as u64); for (proto, num_entries) in this.network_service.num_kbuckets_entries() { let proto = maybe_utf8_bytes_to_string(proto.as_bytes()); diff --git a/utils/prometheus/src/lib.rs b/utils/prometheus/src/lib.rs index 9030704cb746f..be7050a8a0736 100644 --- a/utils/prometheus/src/lib.rs +++ b/utils/prometheus/src/lib.rs @@ -31,6 +31,9 @@ use std::net::SocketAddr; #[cfg(not(target_os = "unknown"))] mod networking; +mod sourced; + +pub use sourced::{SourcedCounter, SourcedGauge, MetricSource}; #[cfg(target_os = "unknown")] pub use unknown_os::init_prometheus; diff --git a/utils/prometheus/src/sourced.rs b/utils/prometheus/src/sourced.rs new file mode 100644 index 0000000000000..58f60e4969bb8 --- /dev/null +++ b/utils/prometheus/src/sourced.rs @@ -0,0 +1,143 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Metrics that are collected from existing sources. + +use prometheus::core::{Collector, Desc, Describer, Number, Opts}; +use prometheus::proto; +use std::{cmp::Ordering, marker::PhantomData}; + +/// A counter whose values are obtained from an existing source. +/// +/// > **Note*: The counter values provided by the source `S` +/// > must be monotonically increasing. Otherwise use a +/// > [`SourcedGauge`] instead. +pub type SourcedCounter = SourcedMetric; + +/// A gauge whose values are obtained from an existing source. +pub type SourcedGauge = SourcedMetric; + +/// The type of a sourced counter. +#[derive(Copy, Clone)] +pub enum Counter {} + +/// The type of a sourced gauge. +#[derive(Copy, Clone)] +pub enum Gauge {} + +/// A metric whose values are obtained from an existing source, +/// instead of being independently recorded. +#[derive(Debug, Clone)] +pub struct SourcedMetric { + source: S, + desc: Desc, + _type: PhantomData, +} + +/// A source of values for a [`SourcedMetric`]. +pub trait MetricSource: Sync + Send + Clone { + /// The type of the collected values. + type N: Number; + /// Collects the current values of the metrics from the source. + fn collect(&self, set: impl FnMut(&[&str], Self::N)); +} + +impl SourcedMetric { + /// Creates a new metric that obtains its values from the given source. + pub fn new(opts: &Opts, source: S) -> prometheus::Result { + let desc = opts.describe()?; + Ok(Self { source, desc, _type: PhantomData }) + } +} + +impl Collector for SourcedMetric { + fn desc(&self) -> Vec<&Desc> { + vec![&self.desc] + } + + fn collect(&self) -> Vec { + let mut counters = Vec::new(); + + self.source.collect(|label_values, value| { + let mut m = proto::Metric::default(); + + match T::proto() { + proto::MetricType::COUNTER => { + let mut c = proto::Counter::default(); + c.set_value(value.into_f64()); + m.set_counter(c); + } + proto::MetricType::GAUGE => { + let mut g = proto::Gauge::default(); + g.set_value(value.into_f64()); + m.set_gauge(g); + } + t => { + log::error!("Unsupported sourced metric type: {:?}", t); + } + } + + debug_assert_eq!(self.desc.variable_labels.len(), label_values.len()); + match self.desc.variable_labels.len().cmp(&label_values.len()) { + Ordering::Greater => + log::warn!("Missing label values for sourced metric {}", self.desc.fq_name), + Ordering::Less => + log::warn!("Too many label values for sourced metric {}", self.desc.fq_name), + Ordering::Equal => {} + } + + m.set_label(self.desc.variable_labels.iter().zip(label_values) + .map(|(l_name, l_value)| { + let mut l = proto::LabelPair::default(); + l.set_name(l_name.to_string()); + l.set_value(l_value.to_string()); + l + }) + .chain(self.desc.const_label_pairs.iter().cloned()) + .collect::>()); + + counters.push(m); + }); + + let mut m = proto::MetricFamily::default(); + m.set_name(self.desc.fq_name.clone()); + m.set_help(self.desc.help.clone()); + m.set_field_type(T::proto()); + m.set_metric(counters); + + vec![m] + } +} + +/// Types of metrics that can obtain their values from an existing source. +pub trait SourcedType: private::Sealed + Sync + Send { + #[doc(hidden)] + fn proto() -> proto::MetricType; +} + +impl SourcedType for Counter { + fn proto() -> proto::MetricType { proto::MetricType::COUNTER } +} + +impl SourcedType for Gauge { + fn proto() -> proto::MetricType { proto::MetricType::GAUGE } +} + +mod private { + pub trait Sealed {} + impl Sealed for super::Counter {} + impl Sealed for super::Gauge {} +} From 3f49041b474f224ab08b29526364e77333ce62d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 18 Aug 2020 09:36:58 +0200 Subject: [PATCH 5/9] Import `IterableStorage*` traits by `decl_storage!` (#6907) Import `IterableStorageMap` and `IterableStorageDoubleMap` automatically by `decl_storage!` as the other storage traits. --- frame/support/procedural/src/storage/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/support/procedural/src/storage/mod.rs b/frame/support/procedural/src/storage/mod.rs index b42639c30c5b9..0aa0a3cad7cd1 100644 --- a/frame/support/procedural/src/storage/mod.rs +++ b/frame/support/procedural/src/storage/mod.rs @@ -416,6 +416,8 @@ pub fn decl_storage_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStr StorageMap as _, StorageDoubleMap as _, StoragePrefixedMap as _, + IterableStorageMap as _, + IterableStorageDoubleMap as _, }; #scrate_decl From 265dd7418306d80da9669f8108ab8475b34f9ad7 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Wed, 19 Aug 2020 13:45:30 +0200 Subject: [PATCH 6/9] Distribute the network future polling time more evenly (#6903) * Distribute the network future polling time more evenly * Update client/network/src/service.rs --- client/network/src/service.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 713357772d417..3ca7452593552 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1405,7 +1405,22 @@ impl Future for NetworkWorker { } } + // At the time of writing of this comment, due to a high volume of messages, the network + // worker sometimes takes a long time to process the loop below. When that happens, the + // rest of the polling is frozen. In order to avoid negative side-effects caused by this + // freeze, a limit to the number of iterations is enforced below. If the limit is reached, + // the task is interrupted then scheduled again. + // + // This allows for a more even distribution in the time taken by each sub-part of the + // polling. + let mut num_iterations = 0; loop { + num_iterations += 1; + if num_iterations >= 100 { + cx.waker().wake_by_ref(); + break; + } + // Process the next message coming from the `NetworkService`. let msg = match this.from_service.poll_next_unpin(cx) { Poll::Ready(Some(msg)) => msg, @@ -1445,7 +1460,16 @@ impl Future for NetworkWorker { } } + // `num_iterations` serves the same purpose as in the previous loop. + // See the previous loop for explanations. + let mut num_iterations = 0; loop { + num_iterations += 1; + if num_iterations >= 1000 { + cx.waker().wake_by_ref(); + break; + } + // Process the next action coming from the network. let next_event = this.network_service.next_event(); futures::pin_mut!(next_event); From bf3aefd3fed60ff6316602987b584721fbb91c2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 19 Aug 2020 17:30:56 +0200 Subject: [PATCH 7/9] Combine default values used at initialization in a trait (#6857) This moves default values used in the Substrate code base when initializing a service into a common trait. Currently this trait only contains listen ports, but this could be extended in the future. Essentially this will make overriding these values much easier for Cumulus, where we have 2 nodes running in one binary. --- client/cli/src/commands/mod.rs | 42 +++++++++------ client/cli/src/commands/run_cmd.rs | 17 +++--- client/cli/src/config.rs | 69 +++++++++++++++++++------ client/cli/src/lib.rs | 2 +- client/cli/src/params/network_params.rs | 3 +- 5 files changed, 94 insertions(+), 39 deletions(-) diff --git a/client/cli/src/commands/mod.rs b/client/cli/src/commands/mod.rs index 04cce66bef80d..5d4f4fe18db7c 100644 --- a/client/cli/src/commands/mod.rs +++ b/client/cli/src/commands/mod.rs @@ -64,7 +64,6 @@ pub enum Subcommand { ExportState(ExportStateCmd), } -// TODO: move to config.rs? /// Macro that helps implement CliConfiguration on an enum of subcommand automatically /// /// # Example @@ -189,17 +188,24 @@ macro_rules! substrate_cli_subcommands { fn network_config( &self, - chain_spec: &::std::boxed::Box, + chain_spec: &std::boxed::Box, is_dev: bool, - net_config_dir: ::std::path::PathBuf, + net_config_dir: std::path::PathBuf, client_id: &str, node_name: &str, - node_key: ::sc_service::config::NodeKeyConfig, + node_key: sc_service::config::NodeKeyConfig, + default_listen_port: u16, ) -> $crate::Result<::sc_service::config::NetworkConfiguration> { match self { $( $enum::$variant(cmd) => cmd.network_config( - chain_spec, is_dev, net_config_dir, client_id, node_name, node_key + chain_spec, + is_dev, + net_config_dir, + client_id, + node_name, + node_key, + default_listen_port, ) ),* } @@ -291,15 +297,21 @@ macro_rules! substrate_cli_subcommands { } } - fn rpc_http(&self) -> $crate::Result<::std::option::Option<::std::net::SocketAddr>> { + fn rpc_http( + &self, + default_listen_port: u16, + ) -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.rpc_http()),* + $($enum::$variant(cmd) => cmd.rpc_http(default_listen_port)),* } } - fn rpc_ws(&self) -> $crate::Result<::std::option::Option<::std::net::SocketAddr>> { + fn rpc_ws( + &self, + default_listen_port: u16, + ) -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.rpc_ws()),* + $($enum::$variant(cmd) => cmd.rpc_ws(default_listen_port)),* } } @@ -316,23 +328,23 @@ macro_rules! substrate_cli_subcommands { } fn rpc_cors(&self, is_dev: bool) - -> $crate::Result<::std::option::Option<::std::vec::Vec>> { + -> $crate::Result>> { match self { $($enum::$variant(cmd) => cmd.rpc_cors(is_dev)),* } } - fn prometheus_config(&self) - -> $crate::Result<::std::option::Option<::sc_service::config::PrometheusConfig>> { + fn prometheus_config(&self, default_listen_port: u16) + -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.prometheus_config()),* + $($enum::$variant(cmd) => cmd.prometheus_config(default_listen_port)),* } } fn telemetry_endpoints( &self, - chain_spec: &Box, - ) -> $crate::Result<::std::option::Option<::sc_service::config::TelemetryEndpoints>> { + chain_spec: &Box, + ) -> $crate::Result> { match self { $($enum::$variant(cmd) => cmd.telemetry_endpoints(chain_spec)),* } diff --git a/client/cli/src/commands/run_cmd.rs b/client/cli/src/commands/run_cmd.rs index de5589196f27f..019b760e5b4ae 100644 --- a/client/cli/src/commands/run_cmd.rs +++ b/client/cli/src/commands/run_cmd.rs @@ -382,7 +382,7 @@ impl CliConfiguration for RunCmd { Ok(self.shared_params.dev || self.force_authoring) } - fn prometheus_config(&self) -> Result> { + fn prometheus_config(&self, default_listen_port: u16) -> Result> { Ok(if self.no_prometheus { None } else { @@ -393,7 +393,10 @@ impl CliConfiguration for RunCmd { }; Some(PrometheusConfig::new_with_default_registry( - SocketAddr::new(interface.into(), self.prometheus_port.unwrap_or(9615)) + SocketAddr::new( + interface.into(), + self.prometheus_port.unwrap_or(default_listen_port), + ) )) }) } @@ -427,7 +430,7 @@ impl CliConfiguration for RunCmd { .into()) } - fn rpc_http(&self) -> Result> { + fn rpc_http(&self, default_listen_port: u16) -> Result> { let interface = rpc_interface( self.rpc_external, self.unsafe_rpc_external, @@ -435,22 +438,22 @@ impl CliConfiguration for RunCmd { self.validator )?; - Ok(Some(SocketAddr::new(interface, self.rpc_port.unwrap_or(9933)))) + Ok(Some(SocketAddr::new(interface, self.rpc_port.unwrap_or(default_listen_port)))) } fn rpc_ipc(&self) -> Result> { Ok(self.ipc_path.clone()) } - fn rpc_ws(&self) -> Result> { + fn rpc_ws(&self, default_listen_port: u16) -> Result> { let interface = rpc_interface( self.ws_external, self.unsafe_ws_external, self.rpc_methods, - self.validator + self.validator, )?; - Ok(Some(SocketAddr::new(interface, self.ws_port.unwrap_or(9944)))) + Ok(Some(SocketAddr::new(interface, self.ws_port.unwrap_or(default_listen_port)))) } fn rpc_methods(&self) -> Result { diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index efda45a0eca9a..ff0222216ce15 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -41,8 +41,44 @@ pub(crate) const NODE_NAME_MAX_LENGTH: usize = 64; /// default sub directory to store network config pub(crate) const DEFAULT_NETWORK_CONFIG_PATH: &'static str = "network"; +/// Default configuration values used by Substrate +/// +/// These values will be used by [`CliConfiguritation`] to set +/// default values for e.g. the listen port or the RPC port. +pub trait DefaultConfigurationValues { + /// The port Substrate should listen on for p2p connections. + /// + /// By default this is `30333`. + fn p2p_listen_port() -> u16 { + 30333 + } + + /// The port Substrate should listen on for websocket connections. + /// + /// By default this is `9944`. + fn rpc_ws_listen_port() -> u16 { + 9944 + } + + /// The port Substrate should listen on for http connections. + /// + /// By default this is `9933`. + fn rpc_http_listen_port() -> u16 { + 9933 + } + + /// The port Substrate should listen on for prometheus connections. + /// + /// By default this is `9615`. + fn prometheus_listen_port() -> u16 { + 9615 + } +} + +impl DefaultConfigurationValues for () {} + /// A trait that allows converting an object to a Configuration -pub trait CliConfiguration: Sized { +pub trait CliConfiguration: Sized { /// Get the SharedParams for this object fn shared_params(&self) -> &SharedParams; @@ -122,6 +158,7 @@ pub trait CliConfiguration: Sized { client_id: &str, node_name: &str, node_key: NodeKeyConfig, + default_listen_port: u16, ) -> Result { Ok(if let Some(network_params) = self.network_params() { network_params.network_config( @@ -131,6 +168,7 @@ pub trait CliConfiguration: Sized { client_id, node_name, node_key, + default_listen_port, ) } else { NetworkConfiguration::new( @@ -257,22 +295,22 @@ pub trait CliConfiguration: Sized { /// Get the RPC HTTP address (`None` if disabled). /// /// By default this is `None`. - fn rpc_http(&self) -> Result> { - Ok(Default::default()) + fn rpc_http(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Get the RPC IPC path (`None` if disabled). /// /// By default this is `None`. fn rpc_ipc(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the RPC websocket address (`None` if disabled). /// /// By default this is `None`. - fn rpc_ws(&self) -> Result> { - Ok(Default::default()) + fn rpc_ws(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Returns the RPC method set to expose. @@ -287,12 +325,12 @@ pub trait CliConfiguration: Sized { /// /// By default this is `None`. fn rpc_ws_max_connections(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the RPC cors (`None` if disabled) /// - /// By default this is `None`. + /// By default this is `Some(Vec::new())`. fn rpc_cors(&self, _is_dev: bool) -> Result>> { Ok(Some(Vec::new())) } @@ -300,8 +338,8 @@ pub trait CliConfiguration: Sized { /// Get the prometheus configuration (`None` if disabled) /// /// By default this is `None`. - fn prometheus_config(&self) -> Result> { - Ok(Default::default()) + fn prometheus_config(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Get the telemetry endpoints (if any) @@ -318,14 +356,14 @@ pub trait CliConfiguration: Sized { /// /// By default this is `None`. fn telemetry_external_transport(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the default value for heap pages /// /// By default this is `None`. fn default_heap_pages(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Returns an offchain worker config wrapped in `Ok(_)` @@ -445,6 +483,7 @@ pub trait CliConfiguration: Sized { client_id.as_str(), self.node_name()?.as_str(), node_key, + DCV::p2p_listen_port(), )?, keystore: self.keystore_config(&config_dir)?, database: self.database_config(&config_dir, database_cache_size, database)?, @@ -453,13 +492,13 @@ pub trait CliConfiguration: Sized { pruning: self.pruning(unsafe_pruning, &role)?, wasm_method: self.wasm_method()?, execution_strategies: self.execution_strategies(is_dev, is_validator)?, - rpc_http: self.rpc_http()?, - rpc_ws: self.rpc_ws()?, + rpc_http: self.rpc_http(DCV::rpc_http_listen_port())?, + rpc_ws: self.rpc_ws(DCV::rpc_ws_listen_port())?, rpc_ipc: self.rpc_ipc()?, rpc_methods: self.rpc_methods()?, rpc_ws_max_connections: self.rpc_ws_max_connections()?, rpc_cors: self.rpc_cors(is_dev)?, - prometheus_config: self.prometheus_config()?, + prometheus_config: self.prometheus_config(DCV::prometheus_listen_port())?, telemetry_endpoints: self.telemetry_endpoints(&chain_spec)?, telemetry_external_transport: self.telemetry_external_transport()?, default_heap_pages: self.default_heap_pages()?, diff --git a/client/cli/src/lib.rs b/client/cli/src/lib.rs index f940ab0b95d71..021f349aaf255 100644 --- a/client/cli/src/lib.rs +++ b/client/cli/src/lib.rs @@ -209,7 +209,7 @@ pub trait SubstrateCli: Sized { } /// Only create a Configuration for the command provided in argument - fn create_configuration( + fn create_configuration, DVC: DefaultConfigurationValues>( &self, command: &T, task_executor: TaskExecutor, diff --git a/client/cli/src/params/network_params.rs b/client/cli/src/params/network_params.rs index 253585544d260..4a33644e8934e 100644 --- a/client/cli/src/params/network_params.rs +++ b/client/cli/src/params/network_params.rs @@ -114,8 +114,9 @@ impl NetworkParams { client_id: &str, node_name: &str, node_key: NodeKeyConfig, + default_listen_port: u16, ) -> NetworkConfiguration { - let port = self.port.unwrap_or(30333); + let port = self.port.unwrap_or(default_listen_port); let listen_addresses = if self.listen_addr.is_empty() { vec![ From 3c3461d1cbcabf29e783ddcfd99b2eb54d67d887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Wed, 19 Aug 2020 17:11:14 +0100 Subject: [PATCH 8/9] babe: handle error when checking/reporting equivocations (#6915) --- client/consensus/babe/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 9e7c3c9081b54..67aca1dd43e7a 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -994,13 +994,15 @@ where // the header is valid but let's check if there was something else already // proposed at the same slot by the given author. if there was, we will // report the equivocation to the runtime. - self.check_and_report_equivocation( + if let Err(err) = self.check_and_report_equivocation( slot_now, slot_number, &header, &verified_info.author, &origin, - )?; + ) { + warn!(target: "babe", "Error checking/reporting BABE equivocation: {:?}", err); + } // if the body is passed through, we need to use the runtime // to check that the internally-set timestamp in the inherents From 368903f7aa9ef652bf8157d476dc13cb36e3affc Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 19 Aug 2020 18:15:50 +0200 Subject: [PATCH 9/9] Dynamic Benchmarking DB Whitelist (#6815) * Add `get_whitelist` api * add whitelisted caller * Whitelist caller * remove caller 0 * initial piping of origin (not actual value yet) * remove attempt to pass origin around * Add whitelist for `DidUpdate` storage on `pallet_timestamp` * fix traits * only add to whitelist if !contains * PassBy not implemented error * Whitelist read/writes explicitly per key * update docs * reduce trait constraint * copy pasta * Apply suggestions from code review Co-authored-by: Guillaume Thiolliere Co-authored-by: Alexander Popiak * rename functions @apopiak * missed some renaming * enable doc tests * Update docs Co-authored-by: Guillaume Thiolliere Co-authored-by: Alexander Popiak --- Cargo.lock | 4 + bin/node/runtime/src/lib.rs | 18 +- client/db/src/bench.rs | 37 +-- frame/balances/src/benchmarking.rs | 8 +- frame/benchmarking/Cargo.toml | 4 + frame/benchmarking/src/lib.rs | 293 ++++++++++++++-------- frame/benchmarking/src/tests.rs | 12 +- frame/benchmarking/src/utils.rs | 47 +++- frame/collective/src/benchmarking.rs | 14 +- frame/indices/src/benchmarking.rs | 10 +- frame/proxy/src/benchmarking.rs | 18 +- frame/staking/src/benchmarking.rs | 20 +- frame/system/benchmarking/src/lib.rs | 8 +- frame/timestamp/src/benchmarking.rs | 14 +- frame/treasury/src/benchmarking.rs | 22 +- frame/utility/src/benchmarking.rs | 7 +- frame/vesting/src/benchmarking.rs | 12 +- primitives/externalities/src/lib.rs | 11 +- primitives/runtime-interface/Cargo.toml | 1 + primitives/runtime-interface/src/impls.rs | 4 + primitives/state-machine/src/backend.rs | 14 +- primitives/state-machine/src/basic.rs | 8 +- primitives/state-machine/src/ext.rs | 8 +- primitives/state-machine/src/read_only.rs | 8 +- primitives/storage/Cargo.toml | 3 +- primitives/storage/src/lib.rs | 21 ++ 26 files changed, 421 insertions(+), 205 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ec5af8aca4ecf..c80c0557443a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1559,6 +1559,7 @@ version = "2.0.0-rc5" dependencies = [ "frame-support", "frame-system", + "hex-literal", "linregress", "parity-scale-codec", "paste", @@ -1567,6 +1568,7 @@ dependencies = [ "sp-runtime", "sp-runtime-interface", "sp-std", + "sp-storage", ] [[package]] @@ -8047,6 +8049,7 @@ dependencies = [ "sp-runtime-interface-test-wasm", "sp-state-machine", "sp-std", + "sp-storage", "sp-tracing", "sp-wasm-interface", "static_assertions", @@ -8176,6 +8179,7 @@ name = "sp-storage" version = "2.0.0-rc5" dependencies = [ "impl-serde 0.2.3", + "parity-scale-codec", "ref-cast", "serde", "sp-debug-derive", diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index aa0ddfc61a7bb..9d19f20c5e19a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1130,7 +1130,7 @@ impl_runtime_apis! { repeat: u32, extra: bool, ) -> Result, sp_runtime::RuntimeString> { - use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark}; + use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark, TrackedStorageKey}; // Trying to add benchmarks directly to the Session Pallet caused cyclic dependency issues. // To get around that, we separated the Session benchmarks into its own crate, which is why // we need these two lines below. @@ -1142,21 +1142,19 @@ impl_runtime_apis! { impl pallet_offences_benchmarking::Trait for Runtime {} impl frame_system_benchmarking::Trait for Runtime {} - let whitelist: Vec> = vec![ + let whitelist: Vec = vec![ // Block Number - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(), // Total Issuance - hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec(), + hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(), // Execution Phase - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec().into(), // Event Count - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec().into(), // System Events - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec(), - // Caller 0 Account - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da946c154ffd9992e395af90b5b13cc6f295c77033fce8a9045824a6690bbf99c6db269502f0a8d1d2a008542d5690a0749").to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec().into(), // Treasury Account - hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da95ecffd7b6c0f78751baa9d281e0bfa3a6d6f646c70792f74727372790000000000000000000000000000000000000000").to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da95ecffd7b6c0f78751baa9d281e0bfa3a6d6f646c70792f74727372790000000000000000000000000000000000000000").to_vec().into(), ]; let mut batches = Vec::::new(); diff --git a/client/db/src/bench.rs b/client/db/src/bench.rs index c3bed3e24f617..93b8048529fc4 100644 --- a/client/db/src/bench.rs +++ b/client/db/src/bench.rs @@ -24,7 +24,10 @@ use std::collections::HashMap; use hash_db::{Prefix, Hasher}; use sp_trie::{MemoryDB, prefixed_key}; -use sp_core::{storage::ChildInfo, hexdisplay::HexDisplay}; +use sp_core::{ + storage::{ChildInfo, TrackedStorageKey}, + hexdisplay::HexDisplay +}; use sp_runtime::traits::{Block as BlockT, HashFor}; use sp_runtime::Storage; use sp_state_machine::{DBValue, backend::Backend as StateBackend, StorageCollection}; @@ -95,7 +98,7 @@ pub struct BenchmarkingState { shared_cache: SharedCache, // shared cache is always empty key_tracker: RefCell, KeyTracker>>, read_write_tracker: RefCell, - whitelist: RefCell>>, + whitelist: RefCell>, } impl BenchmarkingState { @@ -155,15 +158,14 @@ impl BenchmarkingState { fn add_whitelist_to_tracker(&self) { let mut key_tracker = self.key_tracker.borrow_mut(); - let whitelisted = KeyTracker { - has_been_read: true, - has_been_written: true, - }; - let whitelist = self.whitelist.borrow(); whitelist.iter().for_each(|key| { - key_tracker.insert(key.to_vec(), whitelisted); + let whitelisted = KeyTracker { + has_been_read: key.has_been_read, + has_been_written: key.has_been_written, + }; + key_tracker.insert(key.key.clone(), whitelisted); }); } @@ -181,18 +183,21 @@ impl BenchmarkingState { let maybe_tracker = key_tracker.get(key); - let has_been_read = KeyTracker { - has_been_read: true, - has_been_written: false, - }; - match maybe_tracker { None => { + let has_been_read = KeyTracker { + has_been_read: true, + has_been_written: false, + }; key_tracker.insert(key.to_vec(), has_been_read); read_write_tracker.add_read(); }, Some(tracker) => { if !tracker.has_been_read { + let has_been_read = KeyTracker { + has_been_read: true, + has_been_written: tracker.has_been_written, + }; key_tracker.insert(key.to_vec(), has_been_read); read_write_tracker.add_read(); } else { @@ -426,7 +431,11 @@ impl StateBackend> for BenchmarkingState { self.wipe_tracker() } - fn set_whitelist(&self, new: Vec>) { + fn get_whitelist(&self) -> Vec { + self.whitelist.borrow().to_vec() + } + + fn set_whitelist(&self, new: Vec) { *self.whitelist.borrow_mut() = new; } diff --git a/frame/balances/src/benchmarking.rs b/frame/balances/src/benchmarking.rs index 73547fe814aa2..21f43c7c63640 100644 --- a/frame/balances/src/benchmarking.rs +++ b/frame/balances/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use frame_system::RawOrigin; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; use sp_runtime::traits::Bounded; use crate::Module as Balances; @@ -40,7 +40,7 @@ benchmarks! { // * Transfer will create the recipient account. transfer { let existential_deposit = T::ExistentialDeposit::get(); - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); // Give some multiple of the existential deposit + creation fee + transfer fee let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); @@ -60,7 +60,7 @@ benchmarks! { // * Both accounts exist and will continue to exist. #[extra] transfer_best_case { - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); @@ -80,7 +80,7 @@ benchmarks! { // Benchmark `transfer_keep_alive` with the worst possible condition: // * The recipient account is created. transfer_keep_alive { - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); diff --git a/frame/benchmarking/Cargo.toml b/frame/benchmarking/Cargo.toml index 37dcd85b5985f..917988a825fad 100644 --- a/frame/benchmarking/Cargo.toml +++ b/frame/benchmarking/Cargo.toml @@ -20,9 +20,13 @@ sp-runtime-interface = { version = "2.0.0-rc5", path = "../../primitives/runtime sp-runtime = { version = "2.0.0-rc5", path = "../../primitives/runtime", default-features = false } sp-std = { version = "2.0.0-rc5", path = "../../primitives/std", default-features = false } sp-io = { version = "2.0.0-rc5", path = "../../primitives/io", default-features = false } +sp-storage = { version = "2.0.0-rc5", path = "../../primitives/storage", default-features = false } frame-support = { version = "2.0.0-rc5", default-features = false, path = "../support" } frame-system = { version = "2.0.0-rc5", default-features = false, path = "../system" } +[dev-dependencies] +hex-literal = "0.2.1" + [features] default = [ "std" ] std = [ diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 7ef274f25b157..cebdcbcfecd25 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -32,6 +32,7 @@ pub use sp_io::storage::root as storage_root; pub use sp_runtime::traits::Zero; pub use frame_support; pub use paste; +pub use sp_storage::TrackedStorageKey; /// Construct pallet benchmarks for weighing dispatchables. /// @@ -418,156 +419,220 @@ macro_rules! benchmarks_iter { #[doc(hidden)] macro_rules! benchmark_backend { // parsing arms - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( $common:tt )* - } { - $( PRE { $( $pre_parsed:tt )* } )* - } { $eval:block } { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( $common:tt )* } + { $( PRE { $( $pre_parsed:tt )* } )* } + { $eval:block } + { let $pre_id:tt : $pre_ty:ty = $pre_ex:expr; $( $rest:tt )* - } $postcode:block) => { + } + $postcode:block + ) => { $crate::benchmark_backend! { - { $( $instance)? } $name { $( $where_clause )* } { $( $common )* } { + { $( $instance)? } + $name + { $( $where_clause )* } + { $( $common )* } + { $( PRE { $( $pre_parsed )* } )* PRE { $pre_id , $pre_ty , $pre_ex } - } { $eval } { $( $rest )* } $postcode + } + { $eval } + { $( $rest )* } + $postcode } }; - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( $common:tt )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $param:ident in ( $param_from:expr ) .. $param_to:expr => $param_instancer:expr; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( $common:tt )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $param:ident in ( $param_from:expr ) .. $param_to:expr => $param_instancer:expr; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { - { $( $instance)? } $name { $( $where_clause )* } { $( $common )* } { + { $( $instance)? } + $name + { $( $where_clause )* } + { $( $common )* } + { $( $parsed )* PARAM { $param , $param_from , $param_to , $param_instancer } - } { $eval } { $( $rest )* } $postcode + } + { $eval } + { $( $rest )* } + $postcode } }; // mutation arm to look after defaulting to a common param - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $param:ident in ...; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $param:ident in ...; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { - { $( $instance)? } $name { $( $where_clause )* } { - $( { $common , $common_from , $common_to , $common_instancer } )* - } { - $( $parsed )* - } { $eval } { + { $( $instance)? } + $name + { $( $where_clause )* } + { $( { $common , $common_from , $common_to , $common_instancer } )* } + { $( $parsed )* } + { $eval } + { let $param in ({ $( let $common = $common_from; )* $param }) .. ({ $( let $common = $common_to; )* $param }) => ({ $( let $common = || -> Result<(), &'static str> { $common_instancer ; Ok(()) }; )* $param()? }); $( $rest )* - } $postcode + } + $postcode } }; // mutation arm to look after defaulting only the range to common param - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $param:ident in _ .. _ => $param_instancer:expr ; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $param:ident in _ .. _ => $param_instancer:expr ; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { - { $( $instance)? } $name { $( $where_clause )* } { - $( { $common , $common_from , $common_to , $common_instancer } )* - } { - $( $parsed )* - } { $eval } { + { $( $instance)? } + $name + { $( $where_clause )* } + { $( { $common , $common_from , $common_to , $common_instancer } )* } + { $( $parsed )* } + { $eval } + { let $param in ({ $( let $common = $common_from; )* $param }) .. ({ $( let $common = $common_to; )* $param }) => $param_instancer ; $( $rest )* - } $postcode + } + $postcode } }; // mutation arm to look after a single tt for param_from. - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( $common:tt )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $param:ident in $param_from:tt .. $param_to:expr => $param_instancer:expr ; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( $common:tt )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $param:ident in $param_from:tt .. $param_to:expr => $param_instancer:expr ; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { { $( $instance)? } - $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { + $name + { $( $where_clause )* } + { $( $common )* } + { $( $parsed )* } + { $eval } + { let $param in ( $param_from ) .. $param_to => $param_instancer; $( $rest )* - } $postcode + } + $postcode } }; // mutation arm to look after the default tail of `=> ()` - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( $common:tt )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $param:ident in $param_from:tt .. $param_to:expr; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( $common:tt )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $param:ident in $param_from:tt .. $param_to:expr; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { { $( $instance)? } - $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { + $name + { $( $where_clause )* } + { $( $common )* } + { $( $parsed )* } + { $eval } + { let $param in $param_from .. $param_to => (); $( $rest )* - } $postcode + } + $postcode } }; // mutation arm to look after `let _ =` - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( $common:tt )* - } { - $( $parsed:tt )* - } { $eval:block } { - let $pre_id:tt = $pre_ex:expr; - $( $rest:tt )* - } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( $common:tt )* } + { $( $parsed:tt )* } + { $eval:block } + { + let $pre_id:tt = $pre_ex:expr; + $( $rest:tt )* + } + $postcode:block + ) => { $crate::benchmark_backend! { { $( $instance)? } - $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { + $name + { $( $where_clause )* } + { $( $common )* } + { $( $parsed )* } + { $eval } + { let $pre_id : _ = $pre_ex; $( $rest )* - } $postcode + } + $postcode } }; // actioning arm - ( { $( $instance:ident )? } $name:ident { - $( $where_clause:tt )* - } { - $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* - } { - $( PRE { $pre_id:tt , $pre_ty:ty , $pre_ex:expr } )* - $( PARAM { $param:ident , $param_from:expr , $param_to:expr , $param_instancer:expr } )* - } { $eval:block } { $( $post:tt )* } $postcode:block) => { + ( + { $( $instance:ident )? } + $name:ident + { $( $where_clause:tt )* } + { $( { $common:ident , $common_from:tt , $common_to:expr , $common_instancer:expr } )* } + { + $( PRE { $pre_id:tt , $pre_ty:ty , $pre_ex:expr } )* + $( PARAM { $param:ident , $param_from:expr , $param_to:expr , $param_instancer:expr } )* + } + { $eval:block } + { $( $post:tt )* } + $postcode:block + ) => { #[allow(non_camel_case_types)] struct $name; #[allow(unused_variables)] @@ -726,7 +791,7 @@ macro_rules! impl_benchmark { highest_range_values: &[u32], steps: &[u32], repeat: u32, - whitelist: &[Vec] + whitelist: &[$crate::TrackedStorageKey] ) -> Result, &'static str> { // Map the input to the selected benchmark. let extrinsic = sp_std::str::from_utf8(extrinsic) @@ -736,8 +801,14 @@ macro_rules! impl_benchmark { _ => return Err("Could not find extrinsic."), }; - // Add whitelist to DB - $crate::benchmarking::set_whitelist(whitelist.to_vec()); + // Add whitelist to DB including whitelisted caller + let mut whitelist = whitelist.to_vec(); + let whitelisted_caller_key = + as frame_support::storage::StorageMap<_,_>>::hashed_key_for( + $crate::whitelisted_caller::() + ); + whitelist.push(whitelisted_caller_key.into()); + $crate::benchmarking::set_whitelist(whitelist); // Warm up the DB $crate::benchmarking::commit_db(); @@ -947,19 +1018,25 @@ macro_rules! impl_benchmark_test { /// let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist); /// ``` /// -/// The `whitelist` is a `Vec>` of storage keys that you would like to skip for DB tracking. For example: +/// The `whitelist` is a parameter you pass to control the DB read/write tracking. +/// We use a vector of [TrackedStorageKey](./struct.TrackedStorageKey.html), which is a simple struct used to set +/// if a key has been read or written to. /// -/// ```ignore -/// let whitelist: Vec> = vec![ +/// For values that should be skipped entirely, we can just pass `key.into()`. For example: +/// +/// ``` +/// use frame_benchmarking::TrackedStorageKey; +/// let whitelist: Vec = vec![ /// // Block Number -/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec(), +/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(), /// // Total Issuance -/// hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec(), +/// hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(), /// // Execution Phase -/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec(), +/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec().into(), /// // Event Count -/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec(), +/// hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec().into(), /// ]; +/// ``` /// /// Then define a mutable local variable to hold your `BenchmarkBatch` object: /// diff --git a/frame/benchmarking/src/tests.rs b/frame/benchmarking/src/tests.rs index 6a4dc7eee4ee7..127645d430564 100644 --- a/frame/benchmarking/src/tests.rs +++ b/frame/benchmarking/src/tests.rs @@ -20,7 +20,6 @@ #![cfg(test)] use super::*; -use codec::Decode; use sp_std::prelude::*; use sp_runtime::{traits::{BlakeTwo256, IdentityLookup}, testing::{H256, Header}}; use frame_support::{ @@ -64,12 +63,10 @@ pub trait OtherTrait { type OtherEvent; } -pub trait Trait: OtherTrait where Self::OtherEvent: Into { +pub trait Trait: frame_system::Trait + OtherTrait + where Self::OtherEvent: Into<::Event> +{ type Event; - type BlockNumber; - type AccountId: 'static + Default + Decode; - type Origin: From> + - Into, Self::Origin>>; } #[derive(Clone, Eq, PartialEq)] @@ -105,9 +102,6 @@ impl frame_system::Trait for Test { impl Trait for Test { type Event = (); - type BlockNumber = u32; - type Origin = Origin; - type AccountId = u64; } impl OtherTrait for Test { diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 5a2bd55ff79aa..8c25f035802fc 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -21,6 +21,7 @@ use codec::{Encode, Decode}; use sp_std::{vec::Vec, prelude::Box}; use sp_io::hashing::blake2_256; use sp_runtime::RuntimeString; +use sp_storage::TrackedStorageKey; /// An alphabet of possible parameters to use for benchmarking. #[derive(Encode, Decode, Clone, Copy, PartialEq, Debug)] @@ -101,19 +102,52 @@ pub trait Benchmarking { self.commit() } - /// Get the read/write count + /// Get the read/write count. fn read_write_count(&self) -> (u32, u32, u32, u32) { self.read_write_count() } - /// Reset the read/write count + /// Reset the read/write count. fn reset_read_write_count(&mut self) { self.reset_read_write_count() } - fn set_whitelist(&mut self, new: Vec>) { + /// Get the DB whitelist. + fn get_whitelist(&self) -> Vec { + self.get_whitelist() + } + + /// Set the DB whitelist. + fn set_whitelist(&mut self, new: Vec) { self.set_whitelist(new) } + + // Add a new item to the DB whitelist. + fn add_to_whitelist(&mut self, add: TrackedStorageKey) { + let mut whitelist = self.get_whitelist(); + match whitelist.iter_mut().find(|x| x.key == add.key) { + // If we already have this key in the whitelist, update to be the most constrained value. + Some(item) => { + *item = TrackedStorageKey { + key: add.key, + has_been_read: item.has_been_read || add.has_been_read, + has_been_written: item.has_been_written || add.has_been_written, + } + }, + // If the key does not exist, add it. + None => { + whitelist.push(add); + } + } + self.set_whitelist(whitelist); + } + + // Remove an item from the DB whitelist. + fn remove_from_whitelist(&mut self, remove: Vec) { + let mut whitelist = self.get_whitelist(); + whitelist.retain(|x| x.key != remove); + self.set_whitelist(whitelist); + } } /// The pallet benchmarking trait. @@ -141,7 +175,7 @@ pub trait Benchmarking { highest_range_values: &[u32], steps: &[u32], repeat: u32, - whitelist: &[Vec] + whitelist: &[TrackedStorageKey] ) -> Result, &'static str>; } @@ -165,3 +199,8 @@ pub fn account(name: &'static str, index: u32, seed let entropy = (name, index, seed).using_encoded(blake2_256); AccountId::decode(&mut &entropy[..]).unwrap_or_default() } + +/// This caller account is automatically whitelisted for DB reads/writes by the benchmarking macro. +pub fn whitelisted_caller() -> AccountId { + account::("whitelisted_caller", 0, 0) +} diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index b9558d8c8ce1c..2c777fadc4cc4 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use frame_system::RawOrigin as SystemOrigin; use frame_system::EventRecord; -use frame_benchmarking::{benchmarks_instance, account}; +use frame_benchmarking::{benchmarks_instance, account, whitelisted_caller}; use sp_runtime::traits::Bounded; use sp_std::mem::size_of; @@ -123,7 +123,7 @@ benchmarks_instance! { members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?; @@ -153,7 +153,7 @@ benchmarks_instance! { members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?; @@ -184,7 +184,7 @@ benchmarks_instance! { let member = account("member", i, SEED); members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?; @@ -377,7 +377,7 @@ benchmarks_instance! { let member = account("member", i, SEED); members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members(SystemOrigin::Root.into(), members.clone(), None, MAX_MEMBERS)?; @@ -458,7 +458,7 @@ benchmarks_instance! { let member = account("member", i, SEED); members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members( SystemOrigin::Root.into(), @@ -530,7 +530,7 @@ benchmarks_instance! { let member = account("member", i, SEED); members.push(member); } - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); members.push(caller.clone()); Collective::::set_members( SystemOrigin::Root.into(), diff --git a/frame/indices/src/benchmarking.rs b/frame/indices/src/benchmarking.rs index a6b543bb43f4a..e8465c44cdc16 100644 --- a/frame/indices/src/benchmarking.rs +++ b/frame/indices/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use frame_system::RawOrigin; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; use sp_runtime::traits::Bounded; use crate::Module as Indices; @@ -35,7 +35,7 @@ benchmarks! { // Index being claimed let i in 0 .. 1000; let account_index = T::AccountIndex::from(i); - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); }: _(RawOrigin::Signed(caller.clone()), account_index) verify { @@ -47,7 +47,7 @@ benchmarks! { let i in 0 .. 1000; let account_index = T::AccountIndex::from(i); // Setup accounts - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); let recipient: T::AccountId = account("recipient", i, SEED); T::Currency::make_free_balance_be(&recipient, BalanceOf::::max_value()); @@ -63,7 +63,7 @@ benchmarks! { let i in 0 .. 1000; let account_index = T::AccountIndex::from(i); // Setup accounts - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); // Claim the index Indices::::claim(RawOrigin::Signed(caller.clone()).into(), account_index)?; @@ -93,7 +93,7 @@ benchmarks! { let i in 0 .. 1000; let account_index = T::AccountIndex::from(i); // Setup accounts - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); // Claim the index Indices::::claim(RawOrigin::Signed(caller.clone()).into(), account_index)?; diff --git a/frame/proxy/src/benchmarking.rs b/frame/proxy/src/benchmarking.rs index 3cbe517dfd7da..f68a2c3a4cd86 100644 --- a/frame/proxy/src/benchmarking.rs +++ b/frame/proxy/src/benchmarking.rs @@ -21,14 +21,14 @@ use super::*; use frame_system::RawOrigin; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; use sp_runtime::traits::Bounded; use crate::Module as Proxy; const SEED: u32 = 0; fn add_proxies(n: u32, maybe_who: Option) -> Result<(), &'static str> { - let caller = maybe_who.unwrap_or_else(|| account("caller", 0, SEED)); + let caller = maybe_who.unwrap_or_else(|| whitelisted_caller()); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); for i in 0..n { Proxy::::add_proxy( @@ -50,35 +50,35 @@ benchmarks! { // In this case the caller is the "target" proxy let caller: T::AccountId = account("target", p - 1, SEED); // ... and "real" is the traditional caller. This is not a typo. - let real: T::AccountId = account("caller", 0, SEED); + let real: T::AccountId = whitelisted_caller(); let call: ::Call = frame_system::Call::::remark(vec![]).into(); }: _(RawOrigin::Signed(caller), real, Some(T::ProxyType::default()), Box::new(call)) add_proxy { let p in ...; - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: _(RawOrigin::Signed(caller), account("target", T::MaxProxies::get().into(), SEED), T::ProxyType::default()) remove_proxy { let p in ...; - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: _(RawOrigin::Signed(caller), account("target", 0, SEED), T::ProxyType::default()) remove_proxies { let p in ...; - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: _(RawOrigin::Signed(caller)) anonymous { let p in ...; - }: _(RawOrigin::Signed(account("caller", 0, SEED)), T::ProxyType::default(), 0) + }: _(RawOrigin::Signed(whitelisted_caller()), T::ProxyType::default(), 0) kill_anonymous { let p in 0 .. (T::MaxProxies::get() - 2).into(); - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - Module::::anonymous(RawOrigin::Signed(account("caller", 0, SEED)).into(), T::ProxyType::default(), 0)?; + Module::::anonymous(RawOrigin::Signed(whitelisted_caller()).into(), T::ProxyType::default(), 0)?; let height = system::Module::::block_number(); let ext_index = system::Module::::extrinsic_index().unwrap_or(0); let anon = Module::::anonymous_account(&caller, &T::ProxyType::default(), 0, None); diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index aab92ef4ce5a1..77eecb2ef04d5 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -23,7 +23,7 @@ use testing_utils::*; use sp_runtime::traits::One; use frame_system::RawOrigin; -pub use frame_benchmarking::{benchmarks, account}; +pub use frame_benchmarking::{benchmarks, account, whitelisted_caller}; const SEED: u32 = 0; const MAX_SPANS: u32 = 100; const MAX_VALIDATORS: u32 = 1000; @@ -280,7 +280,7 @@ benchmarks! { let validator = create_validator_with_nominators::(n, T::MaxNominatorRewardedPerValidator::get() as u32, true)?; let current_era = CurrentEra::get().unwrap(); - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); let balance_before = T::Currency::free_balance(&validator); }: _(RawOrigin::Signed(caller), validator.clone(), current_era) verify { @@ -294,7 +294,7 @@ benchmarks! { let validator = create_validator_with_nominators::(n, T::MaxNominatorRewardedPerValidator::get() as u32, false)?; let current_era = CurrentEra::get().unwrap(); - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); let balance_before = T::Currency::free_balance(&validator); }: payout_stakers(RawOrigin::Signed(caller), validator.clone(), current_era) verify { @@ -419,7 +419,7 @@ benchmarks! { let total_payout = T::Currency::minimum_balance() * 1000.into(); >::insert(current_era, total_payout); - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: { for arg in payout_calls_arg { >::payout_stakers(RawOrigin::Signed(caller.clone()).into(), arg.0, arg.1)?; @@ -471,6 +471,10 @@ benchmarks! { let era = >::current_era().unwrap_or(0); let caller: T::AccountId = account("caller", n, SEED); + + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: { let result = >::submit_election_solution( RawOrigin::Signed(caller.clone()).into(), @@ -532,6 +536,10 @@ benchmarks! { let era = >::current_era().unwrap_or(0); let caller: T::AccountId = account("caller", n, SEED); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); + // submit a very bad solution on-chain { // this is needed to fool the chain to accept this solution. @@ -584,6 +592,10 @@ benchmarks! { let caller: T::AccountId = account("caller", n, SEED); let era = >::current_era().unwrap_or(0); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); + // submit a seq-phragmen with all the good stuff on chain. { let (winners, compact, score, size) = get_seq_phragmen_solution::(true); diff --git a/frame/system/benchmarking/src/lib.rs b/frame/system/benchmarking/src/lib.rs index a3e7797996ac9..653d9536f1797 100644 --- a/frame/system/benchmarking/src/lib.rs +++ b/frame/system/benchmarking/src/lib.rs @@ -24,15 +24,13 @@ use sp_std::vec; use sp_std::prelude::*; use sp_core::{ChangesTrieConfiguration, storage::well_known_keys}; use sp_runtime::traits::Hash; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::traits::Get; use frame_support::storage::{self, StorageMap}; use frame_system::{Module as System, Call, RawOrigin, DigestItemOf, AccountInfo}; mod mock; -const SEED: u32 = 0; - pub struct Module(System); pub trait Trait: frame_system::Trait {} @@ -42,7 +40,7 @@ benchmarks! { remark { let b in 0 .. T::MaximumBlockLength::get(); let remark_message = vec![1; b as usize]; - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), remark_message) set_heap_pages { @@ -139,7 +137,7 @@ benchmarks! { } suicide { - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); let account_info = AccountInfo:: { nonce: 1337.into(), refcount: 0, diff --git a/frame/timestamp/src/benchmarking.rs b/frame/timestamp/src/benchmarking.rs index 9b1c976229e66..1cd0f15ca01b9 100644 --- a/frame/timestamp/src/benchmarking.rs +++ b/frame/timestamp/src/benchmarking.rs @@ -23,7 +23,7 @@ use super::*; use sp_std::prelude::*; use frame_system::RawOrigin; use frame_support::{ensure, traits::OnFinalize}; -use frame_benchmarking::benchmarks; +use frame_benchmarking::{benchmarks, TrackedStorageKey}; use crate::Module as Timestamp; @@ -34,8 +34,14 @@ benchmarks! { set { let t in 1 .. MAX_TIME; + // Ignore write to `DidUpdate` since it transient. + let did_update_key = crate::DidUpdate::hashed_key().to_vec(); + frame_benchmarking::benchmarking::add_to_whitelist(TrackedStorageKey { + key: did_update_key, + has_been_read: false, + has_been_written: true, + }); }: _(RawOrigin::None, t.into()) - verify { ensure!(Timestamp::::now() == t.into(), "Time was not set."); } @@ -44,8 +50,10 @@ benchmarks! { let t in 1 .. MAX_TIME; Timestamp::::set(RawOrigin::None.into(), t.into())?; ensure!(DidUpdate::exists(), "Time was not set."); + // Ignore read/write to `DidUpdate` since it is transient. + let did_update_key = crate::DidUpdate::hashed_key().to_vec(); + frame_benchmarking::benchmarking::add_to_whitelist(did_update_key.into()); }: { Timestamp::::on_finalize(t.into()); } - verify { ensure!(!DidUpdate::exists(), "Time was not removed."); } diff --git a/frame/treasury/src/benchmarking.rs b/frame/treasury/src/benchmarking.rs index 8dddf3581aef2..295326e1639ad 100644 --- a/frame/treasury/src/benchmarking.rs +++ b/frame/treasury/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use frame_system::RawOrigin; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; use frame_support::traits::OnInitialize; use crate::Module as Treasury; @@ -45,7 +45,7 @@ fn setup_proposal(u: u32) -> ( // Create the pre-requisite information needed to create a `report_awesome`. fn setup_awesome(length: u32) -> (T::AccountId, Vec, T::AccountId) { - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); let value = T::TipReportDepositBase::get() + T::TipReportDepositPerByte::get() * length.into() + T::Currency::minimum_balance(); @@ -116,6 +116,9 @@ benchmarks! { propose_spend { let u in 0 .. 1000; let (caller, value, beneficiary_lookup) = setup_proposal::(u); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), value, beneficiary_lookup) reject_proposal { @@ -143,6 +146,9 @@ benchmarks! { report_awesome { let r in 0 .. MAX_BYTES; let (caller, reason, awesome_person) = setup_awesome::(r); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), reason, awesome_person) retract_tip { @@ -155,6 +161,9 @@ benchmarks! { )?; let reason_hash = T::Hashing::hash(&reason[..]); let hash = T::Hashing::hash_of(&(&reason_hash, &awesome_person)); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), hash) tip_new { @@ -162,6 +171,9 @@ benchmarks! { let t in 1 .. MAX_TIPPERS; let (caller, reason, beneficiary, value) = setup_tip::(r, t)?; + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), reason, beneficiary, value) tip { @@ -179,6 +191,9 @@ benchmarks! { ensure!(Tips::::contains_key(hash), "tip does not exist"); create_tips::(t - 1, hash.clone(), value)?; let caller = account("member", t - 1, SEED); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), hash, value) close_tip { @@ -206,6 +221,9 @@ benchmarks! { create_tips::(t, hash.clone(), value)?; let caller = account("caller", t, SEED); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), hash) on_initialize { diff --git a/frame/utility/src/benchmarking.rs b/frame/utility/src/benchmarking.rs index 155a279807afc..8ca0e216f2887 100644 --- a/frame/utility/src/benchmarking.rs +++ b/frame/utility/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use frame_system::{RawOrigin, EventRecord}; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; const SEED: u32 = 0; @@ -43,7 +43,7 @@ benchmarks! { let call = frame_system::Call::remark(vec![]).into(); calls.push(call); } - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), calls) verify { assert_last_event::(Event::BatchCompleted.into()) @@ -53,6 +53,9 @@ benchmarks! { let u in 0 .. 1000; let caller = account("caller", u, SEED); let call = Box::new(frame_system::Call::remark(vec![]).into()); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), u as u16, call) } diff --git a/frame/vesting/src/benchmarking.rs b/frame/vesting/src/benchmarking.rs index 24cdc28c97f20..974289aac3218 100644 --- a/frame/vesting/src/benchmarking.rs +++ b/frame/vesting/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use frame_system::{RawOrigin, Module as System}; -use frame_benchmarking::{benchmarks, account}; +use frame_benchmarking::{benchmarks, account, whitelisted_caller}; use sp_runtime::traits::Bounded; use crate::Module as Vesting; @@ -64,7 +64,7 @@ benchmarks! { vest_locked { let l in 0 .. MAX_LOCKS; - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); add_locks::(&caller, l as u8); add_vesting_schedule::(&caller)?; @@ -88,7 +88,7 @@ benchmarks! { vest_unlocked { let l in 0 .. MAX_LOCKS; - let caller = account("caller", 0, SEED); + let caller = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); add_locks::(&caller, l as u8); add_vesting_schedule::(&caller)?; @@ -125,7 +125,7 @@ benchmarks! { "Vesting schedule not added", ); - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: vest_other(RawOrigin::Signed(caller.clone()), other_lookup) verify { // Nothing happened since everything is still vested. @@ -152,7 +152,7 @@ benchmarks! { "Vesting schedule still active", ); - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); }: vest_other(RawOrigin::Signed(caller.clone()), other_lookup) verify { // Vesting schedule is removed! @@ -166,7 +166,7 @@ benchmarks! { vested_transfer { let l in 0 .. MAX_LOCKS; - let caller: T::AccountId = account("caller", 0, SEED); + let caller: T::AccountId = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); let target: T::AccountId = account("target", 0, SEED); let target_lookup: ::Source = T::Lookup::unlookup(target.clone()); diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 8e141867195b7..01570e0bfadd3 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -25,7 +25,7 @@ use std::any::{Any, TypeId}; -use sp_storage::ChildInfo; +use sp_storage::{ChildInfo, TrackedStorageKey}; pub use scope_limited::{set_and_run_with_externalities, with_externalities}; pub use extensions::{Extension, Extensions, ExtensionStore}; @@ -248,12 +248,19 @@ pub trait Externalities: ExtensionStore { /// Resets read/write count for the benchmarking process. fn reset_read_write_count(&mut self); + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// Benchmarking related functionality and shouldn't be used anywhere else! + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// + /// Gets the current DB tracking whitelist. + fn get_whitelist(&self) -> Vec; + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! /// Benchmarking related functionality and shouldn't be used anywhere else! /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! /// /// Adds new storage keys to the DB tracking whitelist. - fn set_whitelist(&mut self, new: Vec>); + fn set_whitelist(&mut self, new: Vec); } /// Extension for the [`Externalities`] trait. diff --git a/primitives/runtime-interface/Cargo.toml b/primitives/runtime-interface/Cargo.toml index 16d5a14e889b1..f16000bff4924 100644 --- a/primitives/runtime-interface/Cargo.toml +++ b/primitives/runtime-interface/Cargo.toml @@ -21,6 +21,7 @@ sp-externalities = { version = "0.8.0-rc5", optional = true, path = "../external codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false } static_assertions = "1.0.0" primitive-types = { version = "0.7.0", default-features = false } +sp-storage = { version = "2.0.0-rc5", default-features = false, path = "../storage" } [dev-dependencies] sp-runtime-interface-test-wasm = { version = "2.0.0-rc5", path = "test-wasm" } diff --git a/primitives/runtime-interface/src/impls.rs b/primitives/runtime-interface/src/impls.rs index 259d3517f001d..da57cf086beef 100644 --- a/primitives/runtime-interface/src/impls.rs +++ b/primitives/runtime-interface/src/impls.rs @@ -537,3 +537,7 @@ impl PassBy for sp_wasm_interface::ValueType { impl PassBy for sp_wasm_interface::Value { type PassBy = Codec; } + +impl PassBy for sp_storage::TrackedStorageKey { + type PassBy = Codec; +} diff --git a/primitives/state-machine/src/backend.rs b/primitives/state-machine/src/backend.rs index 9ec03c4d1e249..cfff2c6fc6967 100644 --- a/primitives/state-machine/src/backend.rs +++ b/primitives/state-machine/src/backend.rs @@ -19,7 +19,10 @@ use hash_db::Hasher; use codec::{Decode, Encode}; -use sp_core::{traits::RuntimeCode, storage::{ChildInfo, well_known_keys}}; +use sp_core::{ + traits::RuntimeCode, + storage::{ChildInfo, well_known_keys, TrackedStorageKey} +}; use crate::{ trie_backend::TrieBackend, trie_backend_essence::TrieBackendStorage, @@ -226,10 +229,13 @@ pub trait Backend: std::fmt::Debug { unimplemented!() } - /// Update the whitelist for tracking db reads/writes - fn set_whitelist(&self, _: Vec>) { - unimplemented!() + /// Get the whitelist for tracking db reads/writes + fn get_whitelist(&self) -> Vec { + Default::default() } + + /// Update the whitelist for tracking db reads/writes + fn set_whitelist(&self, _: Vec) {} } impl<'a, T: Backend, H: Hasher> Backend for &'a T { diff --git a/primitives/state-machine/src/basic.rs b/primitives/state-machine/src/basic.rs index 3ddf79dbd9127..3db7a54750a02 100644 --- a/primitives/state-machine/src/basic.rs +++ b/primitives/state-machine/src/basic.rs @@ -27,7 +27,7 @@ use sp_trie::trie_types::Layout; use sp_core::{ storage::{ well_known_keys::is_child_storage_key, Storage, - ChildInfo, StorageChild, + ChildInfo, StorageChild, TrackedStorageKey, }, traits::Externalities, Blake2Hasher, }; @@ -325,7 +325,11 @@ impl Externalities for BasicExternalities { unimplemented!("reset_read_write_count is not supported in Basic") } - fn set_whitelist(&mut self, _: Vec>) { + fn get_whitelist(&self) -> Vec { + unimplemented!("get_whitelist is not supported in Basic") + } + + fn set_whitelist(&mut self, _: Vec) { unimplemented!("set_whitelist is not supported in Basic") } } diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index d7d4bc145eb06..e57636b300a5f 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -26,7 +26,7 @@ use crate::{ use hash_db::Hasher; use sp_core::{ offchain::storage::OffchainOverlayedChanges, - storage::{well_known_keys::is_child_storage_key, ChildInfo}, + storage::{well_known_keys::is_child_storage_key, ChildInfo, TrackedStorageKey}, traits::Externalities, hexdisplay::HexDisplay, }; use sp_trie::{trie_types::Layout, empty_child_trie_root}; @@ -609,7 +609,11 @@ where self.backend.reset_read_write_count() } - fn set_whitelist(&mut self, new: Vec>) { + fn get_whitelist(&self) -> Vec { + self.backend.get_whitelist() + } + + fn set_whitelist(&mut self, new: Vec) { self.backend.set_whitelist(new) } } diff --git a/primitives/state-machine/src/read_only.rs b/primitives/state-machine/src/read_only.rs index b8a35ced1eb0c..99023ec772ec3 100644 --- a/primitives/state-machine/src/read_only.rs +++ b/primitives/state-machine/src/read_only.rs @@ -24,7 +24,7 @@ use std::{ use crate::{Backend, StorageKey, StorageValue}; use hash_db::Hasher; use sp_core::{ - storage::ChildInfo, + storage::{ChildInfo, TrackedStorageKey}, traits::Externalities, Blake2Hasher, }; use codec::Encode; @@ -194,7 +194,11 @@ impl<'a, H: Hasher, B: 'a + Backend> Externalities for ReadOnlyExternalities< unimplemented!("reset_read_write_count is not supported in ReadOnlyExternalities") } - fn set_whitelist(&mut self, _: Vec>) { + fn get_whitelist(&self) -> Vec { + unimplemented!("get_whitelist is not supported in ReadOnlyExternalities") + } + + fn set_whitelist(&mut self, _: Vec) { unimplemented!("set_whitelist is not supported in ReadOnlyExternalities") } } diff --git a/primitives/storage/Cargo.toml b/primitives/storage/Cargo.toml index cb7f2daa50e83..46d76fd7d2832 100644 --- a/primitives/storage/Cargo.toml +++ b/primitives/storage/Cargo.toml @@ -18,7 +18,8 @@ serde = { version = "1.0.101", optional = true, features = ["derive"] } impl-serde = { version = "0.2.3", optional = true } ref-cast = "1.0.0" sp-debug-derive = { version = "2.0.0-rc5", path = "../debug-derive" } +codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false, features = ["derive"] } [features] default = [ "std" ] -std = [ "sp-std/std", "serde", "impl-serde" ] +std = [ "sp-std/std", "serde", "impl-serde", "codec/std" ] diff --git a/primitives/storage/src/lib.rs b/primitives/storage/src/lib.rs index 073d80291c13e..b253733e7b29e 100644 --- a/primitives/storage/src/lib.rs +++ b/primitives/storage/src/lib.rs @@ -25,6 +25,7 @@ use sp_debug_derive::RuntimeDebug; use sp_std::{vec::Vec, ops::{Deref, DerefMut}}; use ref_cast::RefCast; +use codec::{Encode, Decode}; /// Storage key. #[derive(PartialEq, Eq, RuntimeDebug)] @@ -34,6 +35,26 @@ pub struct StorageKey( pub Vec, ); +/// Storage key with read/write tracking information. +#[derive(PartialEq, Eq, RuntimeDebug, Clone, Encode, Decode)] +#[cfg_attr(feature = "std", derive(Hash, PartialOrd, Ord))] +pub struct TrackedStorageKey { + pub key: Vec, + pub has_been_read: bool, + pub has_been_written: bool, +} + +// Easily convert a key to a `TrackedStorageKey` that has been read and written to. +impl From> for TrackedStorageKey { + fn from(key: Vec) -> Self { + Self { + key: key, + has_been_read: true, + has_been_written: true, + } + } +} + /// Storage key of a child trie, it contains the prefix to the key. #[derive(PartialEq, Eq, RuntimeDebug)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize, Hash, PartialOrd, Ord, Clone))]