From 971e52fb70cc3f615da471436469c04b1b99bb3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 24 Jun 2020 12:52:49 +0200 Subject: [PATCH 1/7] seal: Refactor ext_gas_price (#6478) * seal: Refactor ext_gas_price * Remove seals dependency on pallet_transaction_payment * Add weight as an argument to ext_gas_price * Fixed documentation nits from review * Do not use unchecked math even in test code --- Cargo.lock | 1 - bin/node/runtime/src/lib.rs | 2 ++ frame/contracts/Cargo.toml | 2 -- frame/contracts/src/exec.rs | 14 +++++------- frame/contracts/src/lib.rs | 17 ++++++++++---- frame/contracts/src/tests.rs | 12 +++------- frame/contracts/src/wasm/mod.rs | 13 ++++++----- frame/contracts/src/wasm/runtime.rs | 8 ++++--- frame/transaction-payment/src/lib.rs | 33 +++++++++++++--------------- 9 files changed, 51 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08e5102d3402a..c1ea4a479c0ef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4006,7 +4006,6 @@ dependencies = [ "pallet-contracts-primitives", "pallet-randomness-collective-flip", "pallet-timestamp", - "pallet-transaction-payment", "parity-scale-codec", "parity-wasm 0.41.0", "pretty_assertions", diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index cf1b0de8f7922..5acaafcab424f 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -581,6 +581,7 @@ parameter_types! { impl pallet_contracts::Trait for Runtime { type Time = Timestamp; type Randomness = RandomnessCollectiveFlip; + type Currency = Balances; type Call = Call; type Event = Event; type DetermineContractAddress = pallet_contracts::SimpleAddressDeterminer; @@ -594,6 +595,7 @@ impl pallet_contracts::Trait for Runtime { type SurchargeReward = SurchargeReward; type MaxDepth = pallet_contracts::DefaultMaxDepth; type MaxValueSize = pallet_contracts::DefaultMaxValueSize; + type WeightPrice = pallet_transaction_payment::Module; } impl pallet_sudo::Trait for Runtime { diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 57c278a3fb2a9..2dee486fcf681 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -25,7 +25,6 @@ sp-sandbox = { version = "0.8.0-rc3", default-features = false, path = "../../pr frame-support = { version = "2.0.0-rc3", default-features = false, path = "../support" } frame-system = { version = "2.0.0-rc3", default-features = false, path = "../system" } pallet-contracts-primitives = { version = "2.0.0-rc3", default-features = false, path = "common" } -pallet-transaction-payment = { version = "2.0.0-rc3", default-features = false, path = "../transaction-payment" } [dev-dependencies] wabt = "0.9.2" @@ -52,5 +51,4 @@ std = [ "pwasm-utils/std", "wasmi-validation/std", "pallet-contracts-primitives/std", - "pallet-transaction-payment/std", ] diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index ff0d4d9dc0de1..ba3619195d0c5 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -21,10 +21,11 @@ use crate::rent; use crate::storage; use sp_std::prelude::*; -use sp_runtime::traits::{Bounded, Zero}; +use sp_runtime::traits::{Bounded, Zero, Convert}; use frame_support::{ storage::unhashed, dispatch::DispatchError, traits::{ExistenceRequirement, Currency, Time, Randomness}, + weights::Weight, }; pub type AccountIdOf = ::AccountId; @@ -216,8 +217,8 @@ pub trait Ext { /// Returns `None` if the value doesn't exist. fn get_runtime_storage(&self, key: &[u8]) -> Option>; - /// Returns the price of one weight unit. - fn get_weight_price(&self) -> BalanceOf; + /// Returns the price for the specified amount of weight. + fn get_weight_price(&self, weight: Weight) -> BalanceOf; } /// Loader is a companion of the `Vm` trait. It loads an appropriate abstract @@ -874,11 +875,8 @@ where unhashed::get_raw(&key) } - fn get_weight_price(&self) -> BalanceOf { - use pallet_transaction_payment::Module as Payment; - use sp_runtime::SaturatedConversion; - let price = Payment::::weight_to_fee_with_adjustment::(1); - price.saturated_into() + fn get_weight_price(&self, weight: Weight) -> BalanceOf { + T::WeightPrice::convert(weight) } } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index c12029a856c9d..63de1ee164bb5 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -102,7 +102,7 @@ use sp_std::{prelude::*, marker::PhantomData, fmt::Debug}; use codec::{Codec, Encode, Decode}; use sp_runtime::{ traits::{ - Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, + Hash, StaticLookup, Zero, MaybeSerializeDeserialize, Member, Convert, }, RuntimeDebug, }; @@ -117,6 +117,7 @@ use frame_support::traits::{OnUnbalanced, Currency, Get, Time, Randomness}; use frame_support::weights::GetDispatchInfo; use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; use pallet_contracts_primitives::{RentProjection, ContractAccessError}; +use frame_support::weights::Weight; pub type CodeHash = ::Hash; pub type TrieId = Vec; @@ -289,9 +290,10 @@ where } } -pub type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; +pub type BalanceOf = + <::Currency as Currency<::AccountId>>::Balance; pub type NegativeImbalanceOf = - <::Currency as Currency<::AccountId>>::NegativeImbalance; + <::Currency as Currency<::AccountId>>::NegativeImbalance; parameter_types! { /// A reasonable default value for [`Trait::SignedClaimedHandicap`]. @@ -312,10 +314,13 @@ parameter_types! { pub const DefaultMaxValueSize: u32 = 16_384; } -pub trait Trait: frame_system::Trait + pallet_transaction_payment::Trait { +pub trait Trait: frame_system::Trait { type Time: Time; type Randomness: Randomness; + /// The currency in which fees are paid and contract balances are held. + type Currency: Currency; + /// The outer call dispatch type. type Call: Parameter + @@ -371,6 +376,10 @@ pub trait Trait: frame_system::Trait + pallet_transaction_payment::Trait { /// The maximum size of a storage value in bytes. type MaxValueSize: Get; + + /// Used to answer contracts's queries regarding the current weight price. This is **not** + /// used to calculate the actual fee and is only for informational purposes. + type WeightPrice: Convert>; } /// Simple contract address determiner. diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index df6afa8ac51e0..ae81a83be727c 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -30,7 +30,7 @@ use frame_support::{ assert_ok, assert_err_ignore_postinfo, impl_outer_dispatch, impl_outer_event, impl_outer_origin, parameter_types, StorageMap, StorageValue, traits::{Currency, Get}, - weights::{Weight, PostDispatchInfo, IdentityFee}, + weights::{Weight, PostDispatchInfo}, }; use std::cell::RefCell; use frame_system::{self as system, EventRecord, Phase}; @@ -169,17 +169,10 @@ impl Convert> for Test { } } -impl pallet_transaction_payment::Trait for Test { - type Currency = Balances; - type OnTransactionPayment = (); - type TransactionByteFee = TransactionByteFee; - type WeightToFee = IdentityFee>; - type FeeMultiplierUpdate = (); -} - impl Trait for Test { type Time = Timestamp; type Randomness = Randomness; + type Currency = Balances; type Call = Call; type DetermineContractAddress = DummyContractAddressFor; type Event = MetaEvent; @@ -193,6 +186,7 @@ impl Trait for Test { type SurchargeReward = SurchargeReward; type MaxDepth = MaxDepth; type MaxValueSize = MaxValueSize; + type WeightPrice = Self; } type Balances = pallet_balances::Module; diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 890915a793d46..a4814a1b22f43 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -162,6 +162,7 @@ mod tests { use hex_literal::hex; use assert_matches::assert_matches; use sp_runtime::DispatchError; + use frame_support::weights::Weight; const GAS_LIMIT: Gas = 10_000_000_000; @@ -373,8 +374,8 @@ mod tests { ) ) } - fn get_weight_price(&self) -> BalanceOf { - 1312_u32.into() + fn get_weight_price(&self, weight: Weight) -> BalanceOf { + BalanceOf::::from(1312_u32).saturating_mul(weight.into()) } } @@ -479,8 +480,8 @@ mod tests { fn get_runtime_storage(&self, key: &[u8]) -> Option> { (**self).get_runtime_storage(key) } - fn get_weight_price(&self) -> BalanceOf { - (**self).get_weight_price() + fn get_weight_price(&self, weight: Weight) -> BalanceOf { + (**self).get_weight_price(weight) } } @@ -1056,7 +1057,7 @@ mod tests { const CODE_GAS_PRICE: &str = r#" (module - (import "env" "ext_gas_price" (func $ext_gas_price)) + (import "env" "ext_gas_price" (func $ext_gas_price (param i64))) (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) (import "env" "memory" (memory 1 1)) @@ -1072,7 +1073,7 @@ mod tests { (func (export "call") ;; This stores the gas price in the scratch buffer - (call $ext_gas_price) + (call $ext_gas_price (i64.const 1)) ;; assert $ext_scratch_size == 8 (call $assert diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index b393898835ba3..8c4d1bfb99a56 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -696,12 +696,14 @@ define_env!(Env, , Ok(()) }, - // Stores the gas price for the current transaction into the scratch buffer. + // Stores the price for the specified amount of gas in scratch buffer. // // The data is encoded as T::Balance. The current contents of the scratch buffer are overwritten. - ext_gas_price(ctx) => { + // It is recommended to avoid specifying very small values for `gas` as the prices for a single + // gas can be smaller than one. + ext_gas_price(ctx, gas: u64) => { ctx.scratch_buf.clear(); - ctx.ext.get_weight_price().encode_to(&mut ctx.scratch_buf); + ctx.ext.get_weight_price(gas).encode_to(&mut ctx.scratch_buf); Ok(()) }, diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 4d920f8ec53b8..b993a85da3df3 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -51,7 +51,7 @@ use sp_runtime::{ }, traits::{ Zero, Saturating, SignedExtension, SaturatedConversion, Convert, Dispatchable, - DispatchInfoOf, PostDispatchInfoOf, UniqueSaturatedFrom, UniqueSaturatedInto, + DispatchInfoOf, PostDispatchInfoOf, }, }; use pallet_transaction_payment_rpc_runtime_api::RuntimeDispatchInfo; @@ -340,23 +340,6 @@ impl Module where tip } } -} - -impl Module { - /// Compute the fee for the specified weight. - /// - /// This fee is already adjusted by the per block fee adjustment factor and is therefore the - /// share that the weight contributes to the overall fee of a transaction. - /// - /// This function is generic in order to supply the contracts module with a way to calculate the - /// gas price. The contracts module is not able to put the necessary `BalanceOf` constraints - /// on its trait. This function is not to be used by this module. - pub fn weight_to_fee_with_adjustment(weight: Weight) -> Balance where - Balance: UniqueSaturatedFrom - { - let fee: u128 = Self::weight_to_fee(weight).unique_saturated_into(); - Balance::unique_saturated_from(NextFeeMultiplier::get().saturating_mul_acc_int(fee)) - } fn weight_to_fee(weight: Weight) -> BalanceOf { // cap the weight to the maximum defined in runtime, otherwise it will be the @@ -366,6 +349,20 @@ impl Module { } } +impl Convert> for Module where + T: Trait, + BalanceOf: FixedPointOperand, +{ + /// Compute the fee for the specified weight. + /// + /// This fee is already adjusted by the per block fee adjustment factor and is therefore the + /// share that the weight contributes to the overall fee of a transaction. It is mainly + /// for informational purposes and not used in the actual fee calculation. + fn convert(weight: Weight) -> BalanceOf { + NextFeeMultiplier::get().saturating_mul_int(Self::weight_to_fee(weight)) + } +} + /// Require the transactor pay for themselves and maybe include a tip to gain additional priority /// in the queue. #[derive(Encode, Decode, Clone, Eq, PartialEq)] From 19b4b70e7c7cf966cb5f5669a5e153485943095a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 24 Jun 2020 13:53:40 +0200 Subject: [PATCH 2/7] seal: Remove ext_dispatch_call and ext_get_runtime_storage (#6464) Those are way too hard to audit and make only sense with specific chains. They shouldn't be in the core API. --- bin/node/runtime/src/lib.rs | 1 - frame/contracts/fixtures/dispatch_call.wat | 14 - .../fixtures/dispatch_call_then_trap.wat | 15 - .../fixtures/get_runtime_storage.wat | 74 ----- frame/contracts/fixtures/restoration.wat | 4 +- frame/contracts/fixtures/set_rent.wat | 19 +- frame/contracts/src/exec.rs | 48 +--- frame/contracts/src/lib.rs | 44 +-- frame/contracts/src/tests.rs | 260 ------------------ frame/contracts/src/wasm/mod.rs | 144 ---------- frame/contracts/src/wasm/runtime.rs | 54 ---- 11 files changed, 28 insertions(+), 649 deletions(-) delete mode 100644 frame/contracts/fixtures/dispatch_call.wat delete mode 100644 frame/contracts/fixtures/dispatch_call_then_trap.wat delete mode 100644 frame/contracts/fixtures/get_runtime_storage.wat diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 5acaafcab424f..90bb63874e116 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -582,7 +582,6 @@ impl pallet_contracts::Trait for Runtime { type Time = Timestamp; type Randomness = RandomnessCollectiveFlip; type Currency = Balances; - type Call = Call; type Event = Event; type DetermineContractAddress = pallet_contracts::SimpleAddressDeterminer; type TrieIdGenerator = pallet_contracts::TrieIdFromParentCounter; diff --git a/frame/contracts/fixtures/dispatch_call.wat b/frame/contracts/fixtures/dispatch_call.wat deleted file mode 100644 index db0995bd6c79a..0000000000000 --- a/frame/contracts/fixtures/dispatch_call.wat +++ /dev/null @@ -1,14 +0,0 @@ -(module - (import "env" "ext_dispatch_call" (func $ext_dispatch_call (param i32 i32))) - (import "env" "memory" (memory 1 1)) - - (func (export "call") - (call $ext_dispatch_call - (i32.const 8) ;; Pointer to the start of encoded call buffer - (i32.const 11) ;; Length of the buffer - ) - ) - (func (export "deploy")) - - (data (i32.const 8) "\00\00\03\00\00\00\00\00\00\00\C8") -) diff --git a/frame/contracts/fixtures/dispatch_call_then_trap.wat b/frame/contracts/fixtures/dispatch_call_then_trap.wat deleted file mode 100644 index ce949d68236f3..0000000000000 --- a/frame/contracts/fixtures/dispatch_call_then_trap.wat +++ /dev/null @@ -1,15 +0,0 @@ -(module - (import "env" "ext_dispatch_call" (func $ext_dispatch_call (param i32 i32))) - (import "env" "memory" (memory 1 1)) - - (func (export "call") - (call $ext_dispatch_call - (i32.const 8) ;; Pointer to the start of encoded call buffer - (i32.const 11) ;; Length of the buffer - ) - (unreachable) ;; trap so that the top level transaction fails - ) - (func (export "deploy")) - - (data (i32.const 8) "\00\00\03\00\00\00\00\00\00\00\C8") -) diff --git a/frame/contracts/fixtures/get_runtime_storage.wat b/frame/contracts/fixtures/get_runtime_storage.wat deleted file mode 100644 index 6148f1c408c01..0000000000000 --- a/frame/contracts/fixtures/get_runtime_storage.wat +++ /dev/null @@ -1,74 +0,0 @@ -(module - (import "env" "ext_get_runtime_storage" - (func $ext_get_runtime_storage (param i32 i32) (result i32)) - ) - (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) - (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) - (import "env" "ext_scratch_write" (func $ext_scratch_write (param i32 i32))) - (import "env" "memory" (memory 1 1)) - - (func (export "deploy")) - - (func $assert (param i32) - (block $ok - (br_if $ok - (get_local 0) - ) - (unreachable) - ) - ) - - (func $call (export "call") - ;; Load runtime storage for the first key and assert that it exists. - (call $assert - (i32.eq - (call $ext_get_runtime_storage - (i32.const 16) - (i32.const 4) - ) - (i32.const 0) - ) - ) - - ;; assert $ext_scratch_size == 4 - (call $assert - (i32.eq - (call $ext_scratch_size) - (i32.const 4) - ) - ) - - ;; copy contents of the scratch buffer into the contract's memory. - (call $ext_scratch_read - (i32.const 4) ;; Pointer in memory to the place where to copy. - (i32.const 0) ;; Offset from the start of the scratch buffer. - (i32.const 4) ;; Count of bytes to copy. - ) - - ;; assert that contents of the buffer is equal to the i32 value of 0x14144020. - (call $assert - (i32.eq - (i32.load - (i32.const 4) - ) - (i32.const 0x14144020) - ) - ) - - ;; Load the second key and assert that it doesn't exist. - (call $assert - (i32.eq - (call $ext_get_runtime_storage - (i32.const 20) - (i32.const 4) - ) - (i32.const 1) - ) - ) - ) - - ;; The first key, 4 bytes long. - (data (i32.const 16) "\01\02\03\04") - ;; The second key, 4 bytes long. - (data (i32.const 20) "\02\03\04\05") -) diff --git a/frame/contracts/fixtures/restoration.wat b/frame/contracts/fixtures/restoration.wat index 225fdde817800..07e11e9d38146 100644 --- a/frame/contracts/fixtures/restoration.wat +++ b/frame/contracts/fixtures/restoration.wat @@ -51,8 +51,8 @@ ;; Code hash of SET_RENT (data (i32.const 264) - "\c2\1c\41\10\a5\22\d8\59\1c\4c\77\35\dd\2d\bf\a1" - "\13\0b\50\93\76\9b\92\31\97\b7\c5\74\26\aa\38\2a" + "\ab\d6\58\65\1e\83\6e\4a\18\0d\f2\6d\bc\42\ba\e9" + "\3d\64\76\e5\30\5b\33\46\bb\4d\43\99\38\21\ee\32" ) ;; Rent allowance diff --git a/frame/contracts/fixtures/set_rent.wat b/frame/contracts/fixtures/set_rent.wat index d1affa0d7415f..3e6bd491bc475 100644 --- a/frame/contracts/fixtures/set_rent.wat +++ b/frame/contracts/fixtures/set_rent.wat @@ -1,5 +1,5 @@ (module - (import "env" "ext_dispatch_call" (func $ext_dispatch_call (param i32 i32))) + (import "env" "ext_transfer" (func $ext_transfer (param i32 i32 i32 i32) (result i32))) (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32))) (import "env" "ext_clear_storage" (func $ext_clear_storage (param i32))) (import "env" "ext_set_rent_allowance" (func $ext_set_rent_allowance (param i32 i32))) @@ -23,11 +23,13 @@ ) ) - ;; transfer 50 to ALICE + ;; transfer 50 to CHARLIE (func $call_2 - (call $ext_dispatch_call - (i32.const 68) - (i32.const 11) + (call $assert + (i32.eq + (call $ext_transfer (i32.const 68) (i32.const 8) (i32.const 76) (i32.const 8)) + (i32.const 0) + ) ) ) @@ -96,6 +98,9 @@ ;; Encoding of 10 in balance (data (i32.const 0) "\28") - ;; Encoding of call transfer 50 to CHARLIE - (data (i32.const 68) "\00\00\03\00\00\00\00\00\00\00\C8") + ;; encoding of Charlies's account id + (data (i32.const 68) "\03") + + ;; encoding of 50 balance + (data (i32.const 76) "\32") ) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index ba3619195d0c5..4e68aac61511c 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -29,7 +29,6 @@ use frame_support::{ }; pub type AccountIdOf = ::AccountId; -pub type CallOf = ::Call; pub type MomentOf = <::Time as Time>::Moment; pub type SeedOf = ::Hash; pub type BlockNumberOf = ::BlockNumber; @@ -151,9 +150,6 @@ pub trait Ext { input_data: Vec, ) -> ExecResult; - /// Notes a call dispatch. - fn note_dispatch_call(&mut self, call: CallOf); - /// Restores the given destination contract sacrificing the current one. /// /// Since this function removes the self contract eagerly, if succeeded, no further actions should @@ -274,23 +270,11 @@ impl Token for ExecFeeToken { } } -#[cfg_attr(any(feature = "std", test), derive(PartialEq, Eq, Clone))] -#[derive(sp_runtime::RuntimeDebug)] -pub enum DeferredAction { - DispatchRuntimeCall { - /// The account id of the contract who dispatched this call. - origin: T::AccountId, - /// The call to dispatch. - call: ::Call, - }, -} - pub struct ExecutionContext<'a, T: Trait + 'a, V, L> { pub caller: Option<&'a ExecutionContext<'a, T, V, L>>, pub self_account: T::AccountId, pub self_trie_id: Option, pub depth: usize, - pub deferred: Vec>, pub config: &'a Config, pub vm: &'a V, pub loader: &'a L, @@ -314,7 +298,6 @@ where self_trie_id: None, self_account: origin, depth: 0, - deferred: Vec::new(), config: &cfg, vm: &vm, loader: &loader, @@ -331,7 +314,6 @@ where self_trie_id: trie_id, self_account: dest, depth: self.depth + 1, - deferred: Vec::new(), config: self.config, vm: self.vm, loader: self.loader, @@ -532,21 +514,14 @@ where where F: FnOnce(&mut ExecutionContext) -> ExecResult { use frame_support::storage::TransactionOutcome::*; - let (output, deferred) = { - let mut nested = self.nested(dest, trie_id); - let output = frame_support::storage::with_transaction(|| { - let output = func(&mut nested); - match output { - Ok(ref rv) if rv.is_success() => Commit(output), - _ => Rollback(output), - } - })?; - (output, nested.deferred) - }; - if output.is_success() { - self.deferred.extend(deferred); - } - Ok(output) + let mut nested = self.nested(dest, trie_id); + frame_support::storage::with_transaction(|| { + let output = func(&mut nested); + match output { + Ok(ref rv) if rv.is_success() => Commit(output), + _ => Rollback(output), + } + }) } /// Returns whether a contract, identified by address, is currently live in the execution @@ -767,13 +742,6 @@ where self.ctx.call(to.clone(), value, gas_meter, input_data) } - fn note_dispatch_call(&mut self, call: CallOf) { - self.ctx.deferred.push(DeferredAction::DispatchRuntimeCall { - origin: self.ctx.self_account.clone(), - call, - }); - } - fn restore_to( &mut self, dest: AccountIdOf, diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 63de1ee164bb5..4db77a078e92d 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -106,16 +106,13 @@ use sp_runtime::{ }, RuntimeDebug, }; -use frame_support::dispatch::{ - PostDispatchInfo, DispatchResult, Dispatchable, DispatchResultWithPostInfo -}; use frame_support::{ - Parameter, decl_module, decl_event, decl_storage, decl_error, - parameter_types, IsSubType, storage::child::ChildInfo, + decl_module, decl_event, decl_storage, decl_error, + parameter_types, storage::child::ChildInfo, + dispatch::{DispatchResult, DispatchResultWithPostInfo}, + traits::{OnUnbalanced, Currency, Get, Time, Randomness}, }; -use frame_support::traits::{OnUnbalanced, Currency, Get, Time, Randomness}; -use frame_support::weights::GetDispatchInfo; -use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; +use frame_system::{self as system, ensure_signed, ensure_root}; use pallet_contracts_primitives::{RentProjection, ContractAccessError}; use frame_support::weights::Weight; @@ -321,12 +318,6 @@ pub trait Trait: frame_system::Trait { /// The currency in which fees are paid and contract balances are held. type Currency: Currency; - /// The outer call dispatch type. - type Call: - Parameter + - Dispatchable::Origin> + - IsSubType, Self> + GetDispatchInfo; - /// The overarching event type. type Event: From> + Into<::Event>; @@ -644,30 +635,7 @@ impl Module { let vm = WasmVm::new(&cfg.schedule); let loader = WasmLoader::new(&cfg.schedule); let mut ctx = ExecutionContext::top_level(origin.clone(), &cfg, &vm, &loader); - - let result = func(&mut ctx, gas_meter); - - // Execute deferred actions. - ctx.deferred.into_iter().for_each(|deferred| { - use self::exec::DeferredAction::*; - match deferred { - DispatchRuntimeCall { - origin: who, - call, - } => { - let info = call.get_dispatch_info(); - let result = call.dispatch(RawOrigin::Signed(who.clone()).into()); - let post_info = match result { - Ok(post_info) => post_info, - Err(err) => err.post_info, - }; - gas_meter.refund(post_info.calc_unspent(&info)); - Self::deposit_event(RawEvent::Dispatched(who, result.is_ok())); - } - } - }); - - result + func(&mut ctx, gas_meter) } } diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index ae81a83be727c..5303375e016ae 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -173,7 +173,6 @@ impl Trait for Test { type Time = Timestamp; type Randomness = Randomness; type Currency = Balances; - type Call = Call; type DetermineContractAddress = DummyContractAddressFor; type Event = MetaEvent; type TrieIdGenerator = DummyTrieIdGenerator; @@ -446,233 +445,6 @@ fn instantiate_and_call_and_deposit_event() { }); } -#[test] -fn dispatch_call() { - // This test can fail due to the encoding changes. In case it becomes too annoying - // let's rewrite so as we use this module controlled call or we serialize it in runtime. - let encoded = Encode::encode(&Call::Balances(pallet_balances::Call::transfer(CHARLIE, 50))); - assert_eq!(&encoded[..], &hex!("00000300000000000000C8")[..]); - - let (wasm, code_hash) = compile_module::("dispatch_call").unwrap(); - - ExtBuilder::default() - .existential_deposit(50) - .build() - .execute_with(|| { - let _ = Balances::deposit_creating(&ALICE, 1_000_000); - - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - - // Let's keep this assert even though it's redundant. If you ever need to update the - // wasm source this test will fail and will show you the actual hash. - assert_eq!(System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(1)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances(pallet_balances::RawEvent::Endowed(1, 1_000_000)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::CodeStored(code_hash.into())), - topics: vec![], - }, - ]); - - assert_ok!(Contracts::instantiate( - Origin::signed(ALICE), - 100, - GAS_LIMIT, - code_hash.into(), - vec![], - )); - - assert_ok!(Contracts::call( - Origin::signed(ALICE), - BOB, // newly created account - 0, - GAS_LIMIT, - vec![], - )); - - pretty_assertions::assert_eq!(System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(1)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances(pallet_balances::RawEvent::Endowed(1, 1_000_000)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::CodeStored(code_hash.into())), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(BOB)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Endowed(BOB, 100) - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Transfer(ALICE, BOB, 100) - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::Instantiated(ALICE, BOB)), - topics: vec![], - }, - - // Dispatching the call. - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(CHARLIE)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Endowed(CHARLIE, 50) - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Transfer(BOB, CHARLIE, 50) - ), - topics: vec![], - }, - - // Event emitted as a result of dispatch. - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::Dispatched(BOB, true)), - topics: vec![], - } - ]); - }); -} - -#[test] -fn dispatch_call_not_dispatched_after_top_level_transaction_failure() { - // This test can fail due to the encoding changes. In case it becomes too annoying - // let's rewrite so as we use this module controlled call or we serialize it in runtime. - let encoded = Encode::encode(&Call::Balances(pallet_balances::Call::transfer(CHARLIE, 50))); - assert_eq!(&encoded[..], &hex!("00000300000000000000C8")[..]); - - let (wasm, code_hash) = compile_module::("dispatch_call_then_trap").unwrap(); - - ExtBuilder::default() - .existential_deposit(50) - .build() - .execute_with(|| { - let _ = Balances::deposit_creating(&ALICE, 1_000_000); - - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - - // Let's keep this assert even though it's redundant. If you ever need to update the - // wasm source this test will fail and will show you the actual hash. - assert_eq!(System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(1)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances(pallet_balances::RawEvent::Endowed(1, 1_000_000)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::CodeStored(code_hash.into())), - topics: vec![], - }, - ]); - - assert_ok!(Contracts::instantiate( - Origin::signed(ALICE), - 100, - GAS_LIMIT, - code_hash.into(), - vec![], - )); - - // Call the newly instantiated contract. The contract is expected to dispatch a call - // and then trap. - assert_err_ignore_postinfo!( - Contracts::call( - Origin::signed(ALICE), - BOB, // newly created account - 0, - GAS_LIMIT, - vec![], - ), - "contract trapped during execution" - ); - pretty_assertions::assert_eq!(System::events(), vec![ - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(1)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances(pallet_balances::RawEvent::Endowed(1, 1_000_000)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::CodeStored(code_hash.into())), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::system(frame_system::RawEvent::NewAccount(BOB)), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Endowed(BOB, 100) - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::balances( - pallet_balances::RawEvent::Transfer(ALICE, BOB, 100) - ), - topics: vec![], - }, - EventRecord { - phase: Phase::Initialization, - event: MetaEvent::contracts(RawEvent::Instantiated(ALICE, BOB)), - topics: vec![], - }, - // ABSENCE of events which would be caused by dispatched Balances::transfer call - ]); - }); -} - #[test] fn run_out_of_gas() { let (wasm, code_hash) = compile_module::("run_out_of_gas").unwrap(); @@ -1773,38 +1545,6 @@ fn cannot_self_destruct_in_constructor() { }); } -#[test] -fn get_runtime_storage() { - let (wasm, code_hash) = compile_module::("get_runtime_storage").unwrap(); - ExtBuilder::default() - .existential_deposit(50) - .build() - .execute_with(|| { - let _ = Balances::deposit_creating(&ALICE, 1_000_000); - - frame_support::storage::unhashed::put_raw( - &[1, 2, 3, 4], - 0x14144020u32.to_le_bytes().to_vec().as_ref() - ); - - assert_ok!(Contracts::put_code(Origin::signed(ALICE), wasm)); - assert_ok!(Contracts::instantiate( - Origin::signed(ALICE), - 100, - GAS_LIMIT, - code_hash.into(), - vec![], - )); - assert_ok!(Contracts::call( - Origin::signed(ALICE), - BOB, - 0, - GAS_LIMIT, - vec![], - )); - }); -} - #[test] fn crypto_hashes() { let (wasm, code_hash) = compile_module::("crypto_hashes").unwrap(); diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index a4814a1b22f43..3d2f5b154ffd4 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/frame/contracts/src/wasm/mod.rs @@ -206,7 +206,6 @@ mod tests { instantiates: Vec, terminations: Vec, transfers: Vec, - dispatches: Vec, restores: Vec, // (topics, data) events: Vec<(Vec, Vec)>, @@ -299,9 +298,6 @@ mod tests { }); Ok(()) } - fn note_dispatch_call(&mut self, call: Call) { - self.dispatches.push(DispatchEntry(call)); - } fn restore_to( &mut self, dest: u64, @@ -421,9 +417,6 @@ mod tests { ) -> ExecResult { (**self).call(to, value, gas_meter, input_data) } - fn note_dispatch_call(&mut self, call: Call) { - (**self).note_dispatch_call(call) - } fn restore_to( &mut self, dest: u64, @@ -1238,44 +1231,6 @@ mod tests { ).unwrap(); } - const CODE_DISPATCH_CALL: &str = r#" -(module - (import "env" "ext_dispatch_call" (func $ext_dispatch_call (param i32 i32))) - (import "env" "memory" (memory 1 1)) - - (func (export "call") - (call $ext_dispatch_call - (i32.const 8) ;; Pointer to the start of encoded call buffer - (i32.const 13) ;; Length of the buffer - ) - ) - (func (export "deploy")) - - (data (i32.const 8) "\00\01\2A\00\00\00\00\00\00\00\E5\14\00") -) -"#; - - #[test] - fn dispatch_call() { - // This test can fail due to the encoding changes. In case it becomes too annoying - // let's rewrite so as we use this module controlled call or we serialize it in runtime. - - let mut mock_ext = MockExt::default(); - let _ = execute( - CODE_DISPATCH_CALL, - vec![], - &mut mock_ext, - &mut GasMeter::new(GAS_LIMIT), - ).unwrap(); - - assert_eq!( - &mock_ext.dispatches, - &[DispatchEntry( - Call::Balances(pallet_balances::Call::set_balance(42, 1337, 0)), - )] - ); - } - const CODE_RETURN_FROM_START_FN: &str = r#" (module (import "env" "ext_return" (func $ext_return (param i32 i32))) @@ -1883,103 +1838,4 @@ mod tests { assert_eq!(output, ExecReturnValue { status: 17, data: hex!("5566778899").to_vec() }); assert!(!output.is_success()); } - - const CODE_GET_RUNTIME_STORAGE: &str = r#" -(module - (import "env" "ext_get_runtime_storage" - (func $ext_get_runtime_storage (param i32 i32) (result i32)) - ) - (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) - (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) - (import "env" "ext_scratch_write" (func $ext_scratch_write (param i32 i32))) - (import "env" "memory" (memory 1 1)) - - (func (export "deploy")) - - (func $assert (param i32) - (block $ok - (br_if $ok - (get_local 0) - ) - (unreachable) - ) - ) - - (func $call (export "call") - ;; Load runtime storage for the first key and assert that it exists. - (call $assert - (i32.eq - (call $ext_get_runtime_storage - (i32.const 16) - (i32.const 4) - ) - (i32.const 0) - ) - ) - - ;; assert $ext_scratch_size == 4 - (call $assert - (i32.eq - (call $ext_scratch_size) - (i32.const 4) - ) - ) - - ;; copy contents of the scratch buffer into the contract's memory. - (call $ext_scratch_read - (i32.const 4) ;; Pointer in memory to the place where to copy. - (i32.const 0) ;; Offset from the start of the scratch buffer. - (i32.const 4) ;; Count of bytes to copy. - ) - - ;; assert that contents of the buffer is equal to the i32 value of 0x14144020. - (call $assert - (i32.eq - (i32.load - (i32.const 4) - ) - (i32.const 0x14144020) - ) - ) - - ;; Load the second key and assert that it doesn't exist. - (call $assert - (i32.eq - (call $ext_get_runtime_storage - (i32.const 20) - (i32.const 4) - ) - (i32.const 1) - ) - ) - ) - - ;; The first key, 4 bytes long. - (data (i32.const 16) "\01\02\03\04") - ;; The second key, 4 bytes long. - (data (i32.const 20) "\02\03\04\05") -) -"#; - - #[test] - fn get_runtime_storage() { - let mut gas_meter = GasMeter::new(GAS_LIMIT); - let mock_ext = MockExt::default(); - - // "\01\02\03\04" - Some(0x14144020) - // "\02\03\04\05" - None - *mock_ext.runtime_storage_keys.borrow_mut() = [ - ([1, 2, 3, 4].to_vec(), Some(0x14144020u32.to_le_bytes().to_vec())), - ([2, 3, 4, 5].to_vec().to_vec(), None), - ] - .iter() - .cloned() - .collect(); - let _ = execute( - CODE_GET_RUNTIME_STORAGE, - vec![], - mock_ext, - &mut gas_meter, - ).unwrap(); - } } diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 8c4d1bfb99a56..7b64117cd2314 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -32,7 +32,6 @@ use sp_io::hashing::{ blake2_128, sha2_256, }; -use frame_support::weights::GetDispatchInfo; /// The value returned from ext_call and ext_instantiate contract external functions if the call or /// instantiation traps. This value is chosen as if the execution does not trap, the return value @@ -162,8 +161,6 @@ pub enum RuntimeToken { /// The given number of bytes is read from the sandbox memory and /// is returned as the return data buffer of the call. ReturnData(u32), - /// Dispatched a call with the given weight. - DispatchWithWeight(Gas), /// (topic_count, data_bytes): A buffer of the given size is posted as an event indexed with the /// given number of topics. DepositEvent(u32, u32), @@ -204,7 +201,6 @@ impl Token for RuntimeToken { data_and_topics_cost.checked_add(metadata.event_base_cost) ) }, - DispatchWithWeight(gas) => gas.checked_add(metadata.dispatch_base_cost), }; value.unwrap_or_else(|| Bounded::max_value()) @@ -785,30 +781,6 @@ define_env!(Env, , Ok(()) }, - // Decodes the given buffer as a `T::Call` and adds it to the list - // of to-be-dispatched calls. - // - // All calls made it to the top-level context will be dispatched before - // finishing the execution of the calling extrinsic. - ext_dispatch_call(ctx, call_ptr: u32, call_len: u32) => { - let call: <::T as Trait>::Call = - read_sandbox_memory_as(ctx, call_ptr, call_len)?; - - // We already deducted the len costs when reading from the sandbox. - // Bill on the actual weight of the dispatched call. - let info = call.get_dispatch_info(); - charge_gas( - &mut ctx.gas_meter, - ctx.schedule, - &mut ctx.special_trap, - RuntimeToken::DispatchWithWeight(info.weight) - )?; - - ctx.ext.note_dispatch_call(call); - - Ok(()) - }, - // Try to restore the given destination contract sacrificing the caller. // // This function will compute a tombstone hash from the caller's storage and the given code hash @@ -1005,32 +977,6 @@ define_env!(Env, , Ok(()) }, - // Retrieve the value under the given key from the **runtime** storage and return 0. - // If there is no entry under the given key then this function will return 1 and - // clear the scratch buffer. - // - // - key_ptr: the pointer into the linear memory where the requested value is placed. - // - key_len: the length of the key in bytes. - ext_get_runtime_storage(ctx, key_ptr: u32, key_len: u32) -> u32 => { - // Steal the scratch buffer so that we hopefully save an allocation for the `key_buf`. - read_sandbox_memory_into_scratch(ctx, key_ptr, key_len)?; - let key_buf = mem::replace(&mut ctx.scratch_buf, Vec::new()); - - match ctx.ext.get_runtime_storage(&key_buf) { - Some(value_buf) => { - // The given value exists. - ctx.scratch_buf = value_buf; - Ok(0) - } - None => { - // Put back the `key_buf` and allow its allocation to be reused. - ctx.scratch_buf = key_buf; - ctx.scratch_buf.clear(); - Ok(1) - } - } - }, - // Computes the SHA2 256-bit hash on the given input buffer. // // Returns the result directly into the given output buffer. From 357279d9554b8bd996055683aa84ea9d5fadd477 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Wed, 24 Jun 2020 15:32:50 +0200 Subject: [PATCH 3/7] Generic Normalize impl for arithmetic and npos-elections (#6374) * add normalize * better api for normalize * Some grumbles * Update primitives/arithmetic/src/lib.rs Co-authored-by: Guillaume Thiolliere * More great review grumbles * Way better doc for everything. * Some improvement * Update primitives/arithmetic/src/lib.rs Co-authored-by: Bernhard Schuster Co-authored-by: Guillaume Thiolliere Co-authored-by: Bernhard Schuster --- frame/staking/fuzzer/src/submit_solution.rs | 17 +- frame/staking/src/offchain_election.rs | 3 +- frame/staking/src/testing_utils.rs | 7 +- primitives/arithmetic/fuzzer/Cargo.toml | 4 + primitives/arithmetic/fuzzer/src/normalize.rs | 62 +++ .../fuzzer/src/per_thing_rational.rs | 2 +- primitives/arithmetic/src/lib.rs | 364 ++++++++++++++++- primitives/arithmetic/src/per_things.rs | 11 +- primitives/arithmetic/src/traits.rs | 2 +- primitives/npos-elections/benches/phragmen.rs | 4 +- primitives/npos-elections/src/helpers.rs | 57 ++- primitives/npos-elections/src/lib.rs | 114 +++--- primitives/npos-elections/src/tests.rs | 366 +++++++++++------- primitives/runtime/src/lib.rs | 2 +- test-utils/src/lib.rs | 2 +- 15 files changed, 790 insertions(+), 227 deletions(-) create mode 100644 primitives/arithmetic/fuzzer/src/normalize.rs diff --git a/frame/staking/fuzzer/src/submit_solution.rs b/frame/staking/fuzzer/src/submit_solution.rs index 7094c7ed888b2..7293cf23890a5 100644 --- a/frame/staking/fuzzer/src/submit_solution.rs +++ b/frame/staking/fuzzer/src/submit_solution.rs @@ -44,7 +44,9 @@ enum Mode { } pub fn new_test_ext(iterations: u32) -> sp_io::TestExternalities { - let mut ext: sp_io::TestExternalities = frame_system::GenesisConfig::default().build_storage::().map(Into::into) + let mut ext: sp_io::TestExternalities = frame_system::GenesisConfig::default() + .build_storage::() + .map(Into::into) .expect("Failed to create test externalities."); let (offchain, offchain_state) = TestOffchainExt::new(); @@ -70,26 +72,29 @@ fn main() { loop { fuzz!(|data: (u32, u32, u32, u32, u32)| { let (mut num_validators, mut num_nominators, mut edge_per_voter, mut to_elect, mode_u32) = data; + // always run with 5 iterations. let mut ext = new_test_ext(5); let mode: Mode = unsafe { std::mem::transmute(mode_u32) }; num_validators = to_range(num_validators, 50, 1000); num_nominators = to_range(num_nominators, 50, 2000); edge_per_voter = to_range(edge_per_voter, 1, 16); to_elect = to_range(to_elect, 20, num_validators); + let do_reduce = true; - println!("+++ instance with params {} / {} / {} / {:?}({}) / {}", + println!("+++ instance with params {} / {} / {} / {} / {:?}({})", num_nominators, num_validators, edge_per_voter, + to_elect, mode, mode_u32, - to_elect, ); ext.execute_with(|| { // initial setup init_active_era(); + assert_ok!(create_validators_with_nominators_for_era::( num_validators, num_nominators, @@ -97,11 +102,11 @@ fn main() { true, None, )); + >::put(ElectionStatus::Open(1)); assert!(>::create_stakers_snapshot().0); - let origin = RawOrigin::Signed(create_funded_user::("fuzzer", 0, 100)); - println!("++ Chain setup done."); + let origin = RawOrigin::Signed(create_funded_user::("fuzzer", 0, 100)); // stuff to submit let (winners, compact, score, size) = match mode { @@ -141,8 +146,6 @@ fn main() { } }; - println!("++ Submission ready. Score = {:?}", score); - // must have chosen correct number of winners. assert_eq!(winners.len() as u32, >::validator_count()); diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 23453e0524aa4..79f3a5c2d94fe 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -203,7 +203,8 @@ pub fn prepare_submission( } // Convert back to ratio assignment. This takes less space. - let low_accuracy_assignment = sp_npos_elections::assignment_staked_to_ratio(staked); + let low_accuracy_assignment = sp_npos_elections::assignment_staked_to_ratio_normalized(staked) + .map_err(|e| OffchainElectionError::from(e))?; // convert back to staked to compute the score in the receiver's accuracy. This can be done // nicer, for now we do it as such since this code is not time-critical. This ensure that the diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 86d137ac30aba..a73073bb1fcb9 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -201,11 +201,8 @@ pub fn get_weak_solution( }; // convert back to ratio assignment. This takes less space. - let low_accuracy_assignment: Vec> = - staked_assignments - .into_iter() - .map(|sa| sa.into_assignment(true)) - .collect(); + let low_accuracy_assignment = assignment_staked_to_ratio_normalized(staked_assignments) + .expect("Failed to normalize"); // re-calculate score based on what the chain will decode. let score = { diff --git a/primitives/arithmetic/fuzzer/Cargo.toml b/primitives/arithmetic/fuzzer/Cargo.toml index a37ab876ef7f1..b6bbe3d8a6769 100644 --- a/primitives/arithmetic/fuzzer/Cargo.toml +++ b/primitives/arithmetic/fuzzer/Cargo.toml @@ -24,6 +24,10 @@ num-traits = "0.2" name = "biguint" path = "src/biguint.rs" +[[bin]] +name = "normalize" +path = "src/normalize.rs" + [[bin]] name = "per_thing_rational" path = "src/per_thing_rational.rs" diff --git a/primitives/arithmetic/fuzzer/src/normalize.rs b/primitives/arithmetic/fuzzer/src/normalize.rs new file mode 100644 index 0000000000000..34c4ef9cb0ab5 --- /dev/null +++ b/primitives/arithmetic/fuzzer/src/normalize.rs @@ -0,0 +1,62 @@ +// This file is part of Substrate. + +// 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. + + +//! # Running +//! Running this fuzzer can be done with `cargo hfuzz run normalize`. `honggfuzz` CLI options can +//! be used by setting `HFUZZ_RUN_ARGS`, such as `-n 4` to use 4 threads. +//! +//! # Debugging a panic +//! Once a panic is found, it can be debugged with +//! `cargo hfuzz run-debug normalize hfuzz_workspace/normalize/*.fuzz`. + +use honggfuzz::fuzz; +use sp_arithmetic::Normalizable; +use std::convert::TryInto; + +fn main() { + let sum_limit = u32::max_value() as u128; + let len_limit: usize = u32::max_value().try_into().unwrap(); + + loop { + fuzz!(|data: (Vec, u32)| { + let (data, norm) = data; + if data.len() == 0 { return; } + let pre_sum: u128 = data.iter().map(|x| *x as u128).sum(); + + let normalized = data.normalize(norm); + // error cases. + if pre_sum > sum_limit || data.len() > len_limit { + assert!(normalized.is_err()) + } else { + if let Ok(normalized) = normalized { + // if sum goes beyond u128, panic. + let sum: u128 = normalized.iter().map(|x| *x as u128).sum(); + + // if this function returns Ok(), then it will ALWAYS be accurate. + assert_eq!( + sum, + norm as u128, + "sums don't match {:?}, {}", + normalized, + norm, + ); + } + } + }) + } +} diff --git a/primitives/arithmetic/fuzzer/src/per_thing_rational.rs b/primitives/arithmetic/fuzzer/src/per_thing_rational.rs index 0820a35100aee..fc22eacc9e499 100644 --- a/primitives/arithmetic/fuzzer/src/per_thing_rational.rs +++ b/primitives/arithmetic/fuzzer/src/per_thing_rational.rs @@ -114,7 +114,7 @@ fn main() { } } -fn assert_per_thing_equal_error(a: T, b: T, err: u128) { +fn assert_per_thing_equal_error(a: P, b: P, err: u128) { let a_abs = a.deconstruct().saturated_into::(); let b_abs = b.deconstruct().saturated_into::(); let diff = a_abs.max(b_abs) - a_abs.min(b_abs); diff --git a/primitives/arithmetic/src/lib.rs b/primitives/arithmetic/src/lib.rs index 9fdfe4b5e1557..5c0d2baa51db6 100644 --- a/primitives/arithmetic/src/lib.rs +++ b/primitives/arithmetic/src/lib.rs @@ -41,10 +41,11 @@ mod fixed_point; mod rational128; pub use fixed_point::{FixedPointNumber, FixedPointOperand, FixedI64, FixedI128, FixedU128}; -pub use per_things::{PerThing, InnerOf, Percent, PerU16, Permill, Perbill, Perquintill}; +pub use per_things::{PerThing, InnerOf, UpperOf, Percent, PerU16, Permill, Perbill, Perquintill}; pub use rational128::Rational128; -use sp_std::cmp::Ordering; +use sp_std::{prelude::*, cmp::Ordering, fmt::Debug, convert::TryInto}; +use traits::{BaseArithmetic, One, Zero, SaturatedConversion, Unsigned}; /// Trait for comparing two numbers with an threshold. /// @@ -85,8 +86,365 @@ where } } +/// A collection-like object that is made of values of type `T` and can normalize its individual +/// values around a centric point. +/// +/// Note that the order of items in the collection may affect the result. +pub trait Normalizable { + /// Normalize self around `targeted_sum`. + /// + /// Only returns `Ok` if the new sum of results is guaranteed to be equal to `targeted_sum`. + /// Else, returns an error explaining why it failed to do so. + fn normalize(&self, targeted_sum: T) -> Result, &'static str>; +} + +macro_rules! impl_normalize_for_numeric { + ($($numeric:ty),*) => { + $( + impl Normalizable<$numeric> for Vec<$numeric> { + fn normalize(&self, targeted_sum: $numeric) -> Result, &'static str> { + normalize(self.as_ref(), targeted_sum) + } + } + )* + }; +} + +impl_normalize_for_numeric!(u8, u16, u32, u64, u128); + +impl Normalizable

for Vec

{ + fn normalize(&self, targeted_sum: P) -> Result, &'static str> { + let inners = self.iter().map(|p| p.clone().deconstruct().into()).collect::>(); + let normalized = normalize(inners.as_ref(), targeted_sum.deconstruct().into())?; + Ok(normalized.into_iter().map(|i: UpperOf

| P::from_parts(i.saturated_into())).collect()) + } +} + + +/// Normalize `input` so that the sum of all elements reaches `targeted_sum`. +/// +/// This implementation is currently in a balanced position between being performant and accurate. +/// +/// 1. We prefer storing original indices, and sorting the `input` only once. This will save the +/// cost of sorting per round at the cost of a little bit of memory. +/// 2. The granularity of increment/decrements is determined by the number of elements in `input` +/// and their sum difference with `targeted_sum`, namely `diff = diff(sum(input), target_sum)`. +/// This value is then distributed into `per_round = diff / input.len()` and `leftover = diff % +/// round`. First, per_round is applied to all elements of input, and then we move to leftover, +/// in which case we add/subtract 1 by 1 until `leftover` is depleted. +/// +/// When the sum is less than the target, the above approach always holds. In this case, then each +/// individual element is also less than target. Thus, by adding `per_round` to each item, neither +/// of them can overflow the numeric bound of `T`. In fact, neither of the can go beyond +/// `target_sum`*. +/// +/// If sum is more than target, there is small twist. The subtraction of `per_round` +/// form each element might go below zero. In this case, we saturate and add the error to the +/// `leftover` value. This ensures that the result will always stay accurate, yet it might cause the +/// execution to become increasingly slow, since leftovers are applied one by one. +/// +/// All in all, the complicated case above is rare to happen in all substrate use cases, hence we +/// opt for it due to its simplicity. +/// +/// This function will return an error is if length of `input` cannot fit in `T`, or if `sum(input)` +/// cannot fit inside `T`. +/// +/// * This proof is used in the implementation as well. +pub fn normalize(input: &[T], targeted_sum: T) -> Result, &'static str> + where T: Clone + Copy + Ord + BaseArithmetic + Unsigned + Debug, +{ + // compute sum and return error if failed. + let mut sum = T::zero(); + for t in input.iter() { + sum = sum.checked_add(t).ok_or("sum of input cannot fit in `T`")?; + } + + // convert count and return error if failed. + let count = input.len(); + let count_t: T = count.try_into().map_err(|_| "length of `inputs` cannot fit in `T`")?; + + // Nothing to do here. + if count.is_zero() { + return Ok(Vec::::new()); + } + + let diff = targeted_sum.max(sum) - targeted_sum.min(sum); + if diff.is_zero() { + return Ok(input.to_vec()); + } + + let needs_bump = targeted_sum > sum; + let per_round = diff / count_t; + let mut leftover = diff % count_t; + + // sort output once based on diff. This will require more data transfer and saving original + // index, but we sort only twice instead: once now and once at the very end. + let mut output_with_idx = input.iter().cloned().enumerate().collect::>(); + output_with_idx.sort_unstable_by_key(|x| x.1); + + if needs_bump { + // must increase the values a bit. Bump from the min element. Index of minimum is now zero + // because we did a sort. If at any point the min goes greater or equal the `max_threshold`, + // we move to the next minimum. + let mut min_index = 0; + // at this threshold we move to next index. + let threshold = targeted_sum / count_t; + + if !per_round.is_zero() { + for _ in 0..count { + output_with_idx[min_index].1 = output_with_idx[min_index].1 + .checked_add(&per_round) + .expect("Proof provided in the module doc; qed."); + if output_with_idx[min_index].1 >= threshold { + min_index += 1; + min_index = min_index % count; + } + } + } + + // continue with the previous min_index + while !leftover.is_zero() { + output_with_idx[min_index].1 = output_with_idx[min_index].1 + .checked_add(&T::one()) + .expect("Proof provided in the module doc; qed."); + if output_with_idx[min_index].1 >= threshold { + min_index += 1; + min_index = min_index % count; + } + leftover -= One::one() + } + } else { + // must decrease the stakes a bit. decrement from the max element. index of maximum is now + // last. if at any point the max goes less or equal the `min_threshold`, we move to the next + // maximum. + let mut max_index = count - 1; + // at this threshold we move to next index. + let threshold = output_with_idx + .first() + .expect("length of input is greater than zero; it must have a first; qed") + .1; + + if !per_round.is_zero() { + for _ in 0..count { + output_with_idx[max_index].1 = output_with_idx[max_index].1 + .checked_sub(&per_round) + .unwrap_or_else(|| { + let remainder = per_round - output_with_idx[max_index].1; + leftover += remainder; + output_with_idx[max_index].1.saturating_sub(per_round) + }); + if output_with_idx[max_index].1 <= threshold { + max_index = max_index.checked_sub(1).unwrap_or(count - 1); + } + } + } + + // continue with the previous max_index + while !leftover.is_zero() { + if let Some(next) = output_with_idx[max_index].1.checked_sub(&One::one()) { + output_with_idx[max_index].1 = next; + if output_with_idx[max_index].1 <= threshold { + max_index = max_index.checked_sub(1).unwrap_or(count - 1); + } + leftover -= One::one() + } else { + max_index = max_index.checked_sub(1).unwrap_or(count - 1); + } + } + } + + debug_assert_eq!( + output_with_idx.iter().fold(T::zero(), |acc, (_, x)| acc + *x), + targeted_sum, + "sum({:?}) != {:?}", + output_with_idx, + targeted_sum, + ); + + // sort again based on the original index. + output_with_idx.sort_unstable_by_key(|x| x.0); + Ok(output_with_idx.into_iter().map(|(_, t)| t).collect()) +} + +#[cfg(test)] +mod normalize_tests { + use super::*; + + #[test] + fn work_for_all_types() { + macro_rules! test_for { + ($type:ty) => { + assert_eq!( + normalize(vec![8 as $type, 9, 7, 10].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + } + } + // it should work for all types as long as the length of vector can be converted to T. + test_for!(u128); + test_for!(u64); + test_for!(u32); + test_for!(u16); + test_for!(u8); + } + + #[test] + fn fails_on_if_input_sum_large() { + assert!(normalize(vec![1u8; 255].as_ref(), 10).is_ok()); + assert_eq!( + normalize(vec![1u8; 256].as_ref(), 10), + Err("sum of input cannot fit in `T`"), + ); + } + + #[test] + fn does_not_fail_on_subtraction_overflow() { + assert_eq!( + normalize(vec![1u8, 100, 100].as_ref(), 10).unwrap(), + vec![1, 9, 0], + ); + assert_eq!( + normalize(vec![1u8, 8, 9].as_ref(), 1).unwrap(), + vec![0, 1, 0], + ); + } + + #[test] + fn works_for_vec() { + assert_eq!(vec![8u32, 9, 7, 10].normalize(40).unwrap(), vec![10u32, 10, 10, 10]); + } + + #[test] + fn works_for_per_thing() { + assert_eq!( + vec![ + Perbill::from_percent(33), + Perbill::from_percent(33), + Perbill::from_percent(33) + ].normalize(Perbill::one()).unwrap(), + vec![ + Perbill::from_parts(333333334), + Perbill::from_parts(333333333), + Perbill::from_parts(333333333), + ] + ); + + assert_eq!( + vec![ + Perbill::from_percent(20), + Perbill::from_percent(15), + Perbill::from_percent(30) + ].normalize(Perbill::one()).unwrap(), + vec![ + Perbill::from_parts(316666668), + Perbill::from_parts(383333332), + Perbill::from_parts(300000000), + ] + ); + } + + #[test] + fn can_work_for_peru16() { + // Peru16 is a rather special case; since inner type is exactly the same as capacity, we + // could have a situation where the sum cannot be calculated in the inner type. Calculating + // using the upper type of the per_thing should assure this to be okay. + assert_eq!( + vec![ + PerU16::from_percent(40), + PerU16::from_percent(40), + PerU16::from_percent(40), + ].normalize(PerU16::one()).unwrap(), + vec![ + PerU16::from_parts(21845), // 33% + PerU16::from_parts(21845), // 33% + PerU16::from_parts(21845), // 33% + ] + ); + } + + #[test] + fn normalize_works_all_le() { + assert_eq!( + normalize(vec![8u32, 9, 7, 10].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + + assert_eq!( + normalize(vec![7u32, 7, 7, 7].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + + assert_eq!( + normalize(vec![7u32, 7, 7, 10].as_ref(), 40).unwrap(), + vec![11, 11, 8, 10], + ); + + assert_eq!( + normalize(vec![7u32, 8, 7, 10].as_ref(), 40).unwrap(), + vec![11, 8, 11, 10], + ); + + assert_eq!( + normalize(vec![7u32, 7, 8, 10].as_ref(), 40).unwrap(), + vec![11, 11, 8, 10], + ); + } + + #[test] + fn normalize_works_some_ge() { + assert_eq!( + normalize(vec![8u32, 11, 9, 10].as_ref(), 40).unwrap(), + vec![10, 11, 9, 10], + ); + } + + #[test] + fn always_inc_min() { + assert_eq!( + normalize(vec![10u32, 7, 10, 10].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + assert_eq!( + normalize(vec![10u32, 10, 7, 10].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + assert_eq!( + normalize(vec![10u32, 10, 10, 7].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + } + + #[test] + fn normalize_works_all_ge() { + assert_eq!( + normalize(vec![12u32, 11, 13, 10].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + + assert_eq!( + normalize(vec![13u32, 13, 13, 13].as_ref(), 40).unwrap(), + vec![10, 10, 10, 10], + ); + + assert_eq!( + normalize(vec![13u32, 13, 13, 10].as_ref(), 40).unwrap(), + vec![12, 9, 9, 10], + ); + + assert_eq!( + normalize(vec![13u32, 12, 13, 10].as_ref(), 40).unwrap(), + vec![9, 12, 9, 10], + ); + + assert_eq!( + normalize(vec![13u32, 13, 12, 10].as_ref(), 40).unwrap(), + vec![9, 9, 12, 10], + ); + } +} + #[cfg(test)] -mod tests { +mod threshold_compare_tests { use super::*; use crate::traits::Saturating; use sp_std::cmp::Ordering; diff --git a/primitives/arithmetic/src/per_things.rs b/primitives/arithmetic/src/per_things.rs index 50b87d5076e9e..521f4d107412c 100644 --- a/primitives/arithmetic/src/per_things.rs +++ b/primitives/arithmetic/src/per_things.rs @@ -21,24 +21,29 @@ use serde::{Serialize, Deserialize}; use sp_std::{ops, fmt, prelude::*, convert::TryInto}; use codec::{Encode, CompactAs}; use crate::traits::{ - SaturatedConversion, UniqueSaturatedInto, Saturating, BaseArithmetic, Bounded, Zero, + SaturatedConversion, UniqueSaturatedInto, Saturating, BaseArithmetic, Bounded, Zero, Unsigned, }; use sp_debug_derive::RuntimeDebug; /// Get the inner type of a `PerThing`. pub type InnerOf

=

::Inner; +/// Get the upper type of a `PerThing`. +pub type UpperOf

=

::Upper; + /// Something that implements a fixed point ration with an arbitrary granularity `X`, as _parts per /// `X`_. pub trait PerThing: Sized + Saturating + Copy + Default + Eq + PartialEq + Ord + PartialOrd + Bounded + fmt::Debug { /// The data type used to build this per-thingy. - type Inner: BaseArithmetic + Copy + fmt::Debug; + type Inner: BaseArithmetic + Unsigned + Copy + fmt::Debug; /// A data type larger than `Self::Inner`, used to avoid overflow in some computations. /// It must be able to compute `ACCURACY^2`. - type Upper: BaseArithmetic + Copy + From + TryInto + fmt::Debug; + type Upper: + BaseArithmetic + Copy + From + TryInto + + UniqueSaturatedInto + Unsigned + fmt::Debug; /// The accuracy of this type. const ACCURACY: Self::Inner; diff --git a/primitives/arithmetic/src/traits.rs b/primitives/arithmetic/src/traits.rs index 3921d253daf06..29b8e419ef85c 100644 --- a/primitives/arithmetic/src/traits.rs +++ b/primitives/arithmetic/src/traits.rs @@ -22,7 +22,7 @@ use codec::HasCompact; pub use integer_sqrt::IntegerSquareRoot; pub use num_traits::{ Zero, One, Bounded, CheckedAdd, CheckedSub, CheckedMul, CheckedDiv, CheckedNeg, - CheckedShl, CheckedShr, checked_pow, Signed + CheckedShl, CheckedShr, checked_pow, Signed, Unsigned, }; use sp_std::ops::{ Add, Sub, Mul, Div, Rem, AddAssign, SubAssign, MulAssign, DivAssign, diff --git a/primitives/npos-elections/benches/phragmen.rs b/primitives/npos-elections/benches/phragmen.rs index 7e46b9dce1d0b..e2385665bf065 100644 --- a/primitives/npos-elections/benches/phragmen.rs +++ b/primitives/npos-elections/benches/phragmen.rs @@ -59,8 +59,8 @@ mod bench_closure_and_slice { } /// Converts a vector of ratio assignments into ones with absolute budget value. - pub fn assignment_ratio_to_staked_slice( - ratio: Vec>, + pub fn assignment_ratio_to_staked_slice( + ratio: Vec>, stakes: &[VoteWeight], ) -> Vec> where diff --git a/primitives/npos-elections/src/helpers.rs b/primitives/npos-elections/src/helpers.rs index 1c96300c66224..063eac70c57fd 100644 --- a/primitives/npos-elections/src/helpers.rs +++ b/primitives/npos-elections/src/helpers.rs @@ -17,37 +17,72 @@ //! Helper methods for npos-elections. -use crate::{Assignment, ExtendedBalance, VoteWeight, IdentifierT, StakedAssignment, WithApprovalOf}; -use sp_arithmetic::PerThing; +use crate::{Assignment, ExtendedBalance, VoteWeight, IdentifierT, StakedAssignment, WithApprovalOf, Error}; +use sp_arithmetic::{PerThing, InnerOf}; use sp_std::prelude::*; /// Converts a vector of ratio assignments into ones with absolute budget value. -pub fn assignment_ratio_to_staked( - ratio: Vec>, +/// +/// Note that this will NOT attempt at normalizing the result. +pub fn assignment_ratio_to_staked( + ratio: Vec>, stake_of: FS, ) -> Vec> where for<'r> FS: Fn(&'r A) -> VoteWeight, - T: sp_std::ops::Mul, - ExtendedBalance: From<::Inner>, + P: sp_std::ops::Mul, + ExtendedBalance: From>, { ratio .into_iter() .map(|a| { let stake = stake_of(&a.who); - a.into_staked(stake.into(), true) + a.into_staked(stake.into()) }) .collect() } +/// Same as [`assignment_ratio_to_staked`] and try and do normalization. +pub fn assignment_ratio_to_staked_normalized( + ratio: Vec>, + stake_of: FS, +) -> Result>, Error> +where + for<'r> FS: Fn(&'r A) -> VoteWeight, + P: sp_std::ops::Mul, + ExtendedBalance: From>, +{ + let mut staked = assignment_ratio_to_staked(ratio, &stake_of); + staked.iter_mut().map(|a| + a.try_normalize(stake_of(&a.who).into()).map_err(|err| Error::ArithmeticError(err)) + ).collect::>()?; + Ok(staked) +} + /// Converts a vector of staked assignments into ones with ratio values. -pub fn assignment_staked_to_ratio( +/// +/// Note that this will NOT attempt at normalizing the result. +pub fn assignment_staked_to_ratio( + staked: Vec>, +) -> Vec> +where + ExtendedBalance: From>, +{ + staked.into_iter().map(|a| a.into_assignment()).collect() +} + +/// Same as [`assignment_staked_to_ratio`] and try and do normalization. +pub fn assignment_staked_to_ratio_normalized( staked: Vec>, -) -> Vec> +) -> Result>, Error> where - ExtendedBalance: From<::Inner>, + ExtendedBalance: From>, { - staked.into_iter().map(|a| a.into_assignment(true)).collect() + let mut ratio = staked.into_iter().map(|a| a.into_assignment()).collect::>(); + ratio.iter_mut().map(|a| + a.try_normalize().map_err(|err| Error::ArithmeticError(err)) + ).collect::>()?; + Ok(ratio) } /// consumes a vector of winners with backing stake to just winners. diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 72eddf9a1d20c..592ed3b717350 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -30,7 +30,7 @@ use sp_std::{prelude::*, collections::btree_map::BTreeMap, fmt::Debug, cmp::Ordering, convert::TryFrom}; use sp_arithmetic::{ - PerThing, Rational128, ThresholdOrd, + PerThing, Rational128, ThresholdOrd, InnerOf, Normalizable, helpers_128bit::multiply_by_rational, traits::{Zero, Saturating, Bounded, SaturatedConversion}, }; @@ -84,6 +84,8 @@ pub enum Error { CompactTargetOverflow, /// One of the index functions returned none. CompactInvalidIndex, + /// An error occurred in some arithmetic operation. + ArithmeticError(&'static str), } /// A type which is used in the API of this crate as a numeric weight of a vote, most often the @@ -155,16 +157,16 @@ pub struct ElectionResult { /// A voter's stake assignment among a set of targets, represented as ratios. #[derive(Debug, Clone, Default)] #[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] -pub struct Assignment { +pub struct Assignment { /// Voter's identifier. pub who: AccountId, /// The distribution of the voter's stake. - pub distribution: Vec<(AccountId, T)>, + pub distribution: Vec<(AccountId, P)>, } -impl Assignment +impl Assignment where - ExtendedBalance: From<::Inner>, + ExtendedBalance: From>, { /// Convert from a ratio assignment into one with absolute values aka. [`StakedAssignment`]. /// @@ -173,50 +175,49 @@ where /// distribution's sum is exactly equal to the total budget, by adding or subtracting the /// remainder from the last distribution. /// - /// If an edge ratio is [`Bounded::max_value()`], it is dropped. This edge can never mean + /// If an edge ratio is [`Bounded::min_value()`], it is dropped. This edge can never mean /// anything useful. - pub fn into_staked(self, stake: ExtendedBalance, fill: bool) -> StakedAssignment + pub fn into_staked(self, stake: ExtendedBalance) -> StakedAssignment where - T: sp_std::ops::Mul, + P: sp_std::ops::Mul, { - let mut sum: ExtendedBalance = Bounded::min_value(); - let mut distribution = self - .distribution + let distribution = self.distribution .into_iter() .filter_map(|(target, p)| { // if this ratio is zero, then skip it. - if p == Bounded::min_value() { + if p.is_zero() { None } else { // NOTE: this mul impl will always round to the nearest number, so we might both // overflow and underflow. let distribution_stake = p * stake; - // defensive only. We assume that balance cannot exceed extended balance. - sum = sum.saturating_add(distribution_stake); Some((target, distribution_stake)) } }) .collect::>(); - if fill { - // NOTE: we can do this better. - // https://revs.runtime-revolution.com/getting-100-with-rounded-percentages-273ffa70252b - if let Some(leftover) = stake.checked_sub(sum) { - if let Some(last) = distribution.last_mut() { - last.1 = last.1.saturating_add(leftover); - } - } else if let Some(excess) = sum.checked_sub(stake) { - if let Some(last) = distribution.last_mut() { - last.1 = last.1.saturating_sub(excess); - } - } - } - StakedAssignment { who: self.who, distribution, } } + + /// Try and normalize this assignment. + /// + /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to 100%. + pub fn try_normalize(&mut self) -> Result<(), &'static str> { + self.distribution + .iter() + .map(|(_, p)| *p) + .collect::>() + .normalize(P::one()) + .map(|normalized_ratios| + self.distribution + .iter_mut() + .zip(normalized_ratios) + .for_each(|((_, old), corrected)| { *old = corrected; }) + ) + } } /// A voter's stake assignment among a set of targets, represented as absolute values in the scale @@ -243,42 +244,23 @@ impl StakedAssignment { /// /// If an edge stake is so small that it cannot be represented in `T`, it is ignored. This edge /// can never be re-created and does not mean anything useful anymore. - pub fn into_assignment(self, fill: bool) -> Assignment + pub fn into_assignment(self) -> Assignment where - ExtendedBalance: From<::Inner>, + ExtendedBalance: From>, + AccountId: IdentifierT, { - let accuracy: u128 = T::ACCURACY.saturated_into(); - let mut sum: u128 = Zero::zero(); - let stake = self.distribution.iter().map(|x| x.1).sum(); - let mut distribution = self - .distribution + let stake = self.total(); + let distribution = self.distribution .into_iter() .filter_map(|(target, w)| { - let per_thing = T::from_rational_approximation(w, stake); + let per_thing = P::from_rational_approximation(w, stake); if per_thing == Bounded::min_value() { None } else { - sum += per_thing.clone().deconstruct().saturated_into(); Some((target, per_thing)) } }) - .collect::>(); - - if fill { - if let Some(leftover) = accuracy.checked_sub(sum) { - if let Some(last) = distribution.last_mut() { - last.1 = last.1.saturating_add( - T::from_parts(leftover.saturated_into()) - ); - } - } else if let Some(excess) = sum.checked_sub(accuracy) { - if let Some(last) = distribution.last_mut() { - last.1 = last.1.saturating_sub( - T::from_parts(excess.saturated_into()) - ); - } - } - } + .collect::>(); Assignment { who: self.who, @@ -286,6 +268,30 @@ impl StakedAssignment { } } + /// Try and normalize this assignment. + /// + /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to + /// `stake`. + /// + /// NOTE: current implementation of `.normalize` is almost safe to `expect()` upon. The only + /// error case is when the input cannot fit in `T`, or the sum of input cannot fit in `T`. + /// Sadly, both of these are dependent upon the implementation of `VoteLimit`, i.e. the limit + /// of edges per voter which is enforced from upstream. Hence, at this crate, we prefer + /// returning a result and a use the name prefix `try_`. + pub fn try_normalize(&mut self, stake: ExtendedBalance) -> Result<(), &'static str> { + self.distribution + .iter() + .map(|(_, ref weight)| *weight) + .collect::>() + .normalize(stake) + .map(|normalized_weights| + self.distribution + .iter_mut() + .zip(normalized_weights.into_iter()) + .for_each(|((_, weight), corrected)| { *weight = corrected; }) + ) + } + /// Get the total stake of this assignment (aka voter budget). pub fn total(&self) -> ExtendedBalance { self.distribution.iter().fold(Zero::zero(), |a, b| a.saturating_add(b.1)) diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index 08923c69499c3..80c742117d979 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -588,184 +588,276 @@ fn self_votes_should_be_kept() { ); } -#[test] -fn assignment_convert_works() { - let staked = StakedAssignment { - who: 1 as AccountId, - distribution: vec![ - (20, 100 as ExtendedBalance), - (30, 25), - ], - }; - - let assignment = staked.clone().into_assignment(true); - assert_eq!( - assignment, - Assignment { - who: 1, +mod assignment_convert_normalize { + use super::*; + #[test] + fn assignment_convert_works() { + let staked = StakedAssignment { + who: 1 as AccountId, distribution: vec![ - (20, Perbill::from_percent(80)), - (30, Perbill::from_percent(20)), - ] - } - ); - - assert_eq!( - assignment.into_staked(125, true), - staked, - ); -} - -#[test] -fn score_comparison_is_lexicographical_no_epsilon() { - let epsilon = Perbill::zero(); - // only better in the fist parameter, worse in the other two ✅ - assert_eq!( - is_score_better([12, 10, 35], [10, 20, 30], epsilon), - true, - ); - - // worse in the first, better in the other two ❌ - assert_eq!( - is_score_better([9, 30, 10], [10, 20, 30], epsilon), - false, - ); - - // equal in the first, the second one dictates. - assert_eq!( - is_score_better([10, 25, 40], [10, 20, 30], epsilon), - true, - ); - - // equal in the first two, the last one dictates. - assert_eq!( - is_score_better([10, 20, 40], [10, 20, 30], epsilon), - false, - ); -} + (20, 100 as ExtendedBalance), + (30, 25), + ], + }; -#[test] -fn score_comparison_with_epsilon() { - let epsilon = Perbill::from_percent(1); + let assignment = staked.clone().into_assignment(); + assert_eq!( + assignment, + Assignment { + who: 1, + distribution: vec![ + (20, Perbill::from_percent(80)), + (30, Perbill::from_percent(20)), + ] + } + ); - { - // no more than 1 percent (10) better in the first param. assert_eq!( - is_score_better([1009, 5000, 100000], [1000, 5000, 100000], epsilon), - false, + assignment.into_staked(125), + staked, ); + } - // now equal, still not better. + #[test] + fn assignment_convert_will_not_normalize() { assert_eq!( - is_score_better([1010, 5000, 100000], [1000, 5000, 100000], epsilon), - false, + Assignment { + who: 1, + distribution: vec![ + (2, Perbill::from_percent(33)), + (3, Perbill::from_percent(66)), + ] + }.into_staked(100), + StakedAssignment { + who: 1, + distribution: vec![ + (2, 33), + (3, 66), + // sum is not 100! + ], + }, ); - // now it is. assert_eq!( - is_score_better([1011, 5000, 100000], [1000, 5000, 100000], epsilon), - true, + StakedAssignment { + who: 1, + distribution: vec![ + (2, 333_333_333_333_333), + (3, 333_333_333_333_333), + (4, 666_666_666_666_333), + ], + }.into_assignment(), + Assignment { + who: 1, + distribution: vec![ + (2, Perbill::from_parts(250000000)), + (3, Perbill::from_parts(250000000)), + (4, Perbill::from_parts(499999999)), + // sum is not 100%! + ] + }, + ) + } + + #[test] + fn assignment_can_normalize() { + let mut a = Assignment { + who: 1, + distribution: vec![ + (2, Perbill::from_parts(330000000)), + (3, Perbill::from_parts(660000000)), + // sum is not 100%! + ] + }; + a.try_normalize().unwrap(); + assert_eq!( + a, + Assignment { + who: 1, + distribution: vec![ + (2, Perbill::from_parts(340000000)), + (3, Perbill::from_parts(660000000)), + ] + }, ); } - { - // First score score is epsilon better, but first score is no longer `ge`. Then this is - // still not a good solution. + #[test] + fn staked_assignment_can_normalize() { + let mut a = StakedAssignment { + who: 1, + distribution: vec![ + (2, 33), + (3, 66), + ] + }; + a.try_normalize(100).unwrap(); assert_eq!( - is_score_better([999, 6000, 100000], [1000, 5000, 100000], epsilon), - false, + a, + StakedAssignment { + who: 1, + distribution: vec![ + (2, 34), + (3, 66), + ] + }, ); } +} - { - // first score is equal or better, but not epsilon. Then second one is the determinant. +mod score { + use super::*; + #[test] + fn score_comparison_is_lexicographical_no_epsilon() { + let epsilon = Perbill::zero(); + // only better in the fist parameter, worse in the other two ✅ assert_eq!( - is_score_better([1005, 5000, 100000], [1000, 5000, 100000], epsilon), - false, + is_score_better([12, 10, 35], [10, 20, 30], epsilon), + true, ); + // worse in the first, better in the other two ❌ assert_eq!( - is_score_better([1005, 5050, 100000], [1000, 5000, 100000], epsilon), + is_score_better([9, 30, 10], [10, 20, 30], epsilon), false, ); + // equal in the first, the second one dictates. assert_eq!( - is_score_better([1005, 5051, 100000], [1000, 5000, 100000], epsilon), + is_score_better([10, 25, 40], [10, 20, 30], epsilon), true, ); - } - { - // first score and second are equal or less than epsilon more, third is determinant. + // equal in the first two, the last one dictates. assert_eq!( - is_score_better([1005, 5025, 100000], [1000, 5000, 100000], epsilon), + is_score_better([10, 20, 40], [10, 20, 30], epsilon), false, ); + } + + #[test] + fn score_comparison_with_epsilon() { + let epsilon = Perbill::from_percent(1); + + { + // no more than 1 percent (10) better in the first param. + assert_eq!( + is_score_better([1009, 5000, 100000], [1000, 5000, 100000], epsilon), + false, + ); + + // now equal, still not better. + assert_eq!( + is_score_better([1010, 5000, 100000], [1000, 5000, 100000], epsilon), + false, + ); + + // now it is. + assert_eq!( + is_score_better([1011, 5000, 100000], [1000, 5000, 100000], epsilon), + true, + ); + } + + { + // First score score is epsilon better, but first score is no longer `ge`. Then this is + // still not a good solution. + assert_eq!( + is_score_better([999, 6000, 100000], [1000, 5000, 100000], epsilon), + false, + ); + } + + { + // first score is equal or better, but not epsilon. Then second one is the determinant. + assert_eq!( + is_score_better([1005, 5000, 100000], [1000, 5000, 100000], epsilon), + false, + ); + + assert_eq!( + is_score_better([1005, 5050, 100000], [1000, 5000, 100000], epsilon), + false, + ); + + assert_eq!( + is_score_better([1005, 5051, 100000], [1000, 5000, 100000], epsilon), + true, + ); + } + + { + // first score and second are equal or less than epsilon more, third is determinant. + assert_eq!( + is_score_better([1005, 5025, 100000], [1000, 5000, 100000], epsilon), + false, + ); + + assert_eq!( + is_score_better([1005, 5025, 99_000], [1000, 5000, 100000], epsilon), + false, + ); + + assert_eq!( + is_score_better([1005, 5025, 98_999], [1000, 5000, 100000], epsilon), + true, + ); + } + } + + #[test] + fn score_comparison_large_value() { + // some random value taken from eras in kusama. + let initial = [12488167277027543u128, 5559266368032409496, 118749283262079244270992278287436446]; + // this claim is 0.04090% better in the third component. It should be accepted as better if + // epsilon is smaller than 5/10_0000 + let claim = [12488167277027543u128, 5559266368032409496, 118700736389524721358337889258988054]; assert_eq!( - is_score_better([1005, 5025, 99_000], [1000, 5000, 100000], epsilon), - false, + is_score_better( + claim.clone(), + initial.clone(), + Perbill::from_rational_approximation(1u32, 10_000), + ), + true, ); assert_eq!( - is_score_better([1005, 5025, 98_999], [1000, 5000, 100000], epsilon), + is_score_better( + claim.clone(), + initial.clone(), + Perbill::from_rational_approximation(2u32, 10_000), + ), true, ); - } -} - -#[test] -fn score_comparison_large_value() { - // some random value taken from eras in kusama. - let initial = [12488167277027543u128, 5559266368032409496, 118749283262079244270992278287436446]; - // this claim is 0.04090% better in the third component. It should be accepted as better if - // epsilon is smaller than 5/10_0000 - let claim = [12488167277027543u128, 5559266368032409496, 118700736389524721358337889258988054]; - - assert_eq!( - is_score_better( - claim.clone(), - initial.clone(), - Perbill::from_rational_approximation(1u32, 10_000), - ), - true, - ); - - assert_eq!( - is_score_better( - claim.clone(), - initial.clone(), - Perbill::from_rational_approximation(2u32, 10_000), - ), - true, - ); - assert_eq!( - is_score_better( - claim.clone(), - initial.clone(), - Perbill::from_rational_approximation(3u32, 10_000), - ), - true, - ); + assert_eq!( + is_score_better( + claim.clone(), + initial.clone(), + Perbill::from_rational_approximation(3u32, 10_000), + ), + true, + ); - assert_eq!( - is_score_better( - claim.clone(), - initial.clone(), - Perbill::from_rational_approximation(4u32, 10_000), - ), - true, - ); + assert_eq!( + is_score_better( + claim.clone(), + initial.clone(), + Perbill::from_rational_approximation(4u32, 10_000), + ), + true, + ); - assert_eq!( - is_score_better( - claim.clone(), - initial.clone(), - Perbill::from_rational_approximation(5u32, 10_000), - ), - false, - ); + assert_eq!( + is_score_better( + claim.clone(), + initial.clone(), + Perbill::from_rational_approximation(5u32, 10_000), + ), + false, + ); + } } mod compact { diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index a8a518fd7b692..881ba3d724d82 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -71,7 +71,7 @@ pub use sp_core::RuntimeDebug; /// Re-export top-level arithmetic stuff. pub use sp_arithmetic::{ - PerThing, traits::SaturatedConversion, Perquintill, Perbill, Permill, Percent, PerU16, + PerThing, traits::SaturatedConversion, Perquintill, Perbill, Permill, Percent, PerU16, InnerOf, Rational128, FixedI64, FixedI128, FixedU128, FixedPointNumber, FixedPointOperand, }; /// Re-export 128 bit helpers. diff --git a/test-utils/src/lib.rs b/test-utils/src/lib.rs index e600ab9fce926..8163460df7427 100644 --- a/test-utils/src/lib.rs +++ b/test-utils/src/lib.rs @@ -38,7 +38,7 @@ /// ``` #[macro_export] macro_rules! assert_eq_uvec { - ( $x:expr, $y:expr ) => { + ( $x:expr, $y:expr $(,)? ) => { $crate::__assert_eq_uvec!($x, $y); $crate::__assert_eq_uvec!($y, $x); } From d17396cebe11dce352aeeaac0b2645354cb2b328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 24 Jun 2020 17:01:42 +0200 Subject: [PATCH 4/7] Extract frame_system SignedExtensions into separate files. (#6474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Split the code. * Restructure. * Split tests. * Self-review. * Break lines. * Move tests out. * Rename CheckEra -> CheckMortality but keep backwards compatibility * Update frame/system/src/extensions/check_mortality.rs * Don't rename the IDENTIFIER for now. Co-authored-by: Bastian Köcher --- frame/system/src/extensions/check_genesis.rs | 58 + .../system/src/extensions/check_mortality.rs | 124 ++ frame/system/src/extensions/check_nonce.rs | 145 ++ .../src/extensions/check_spec_version.rs | 58 + .../system/src/extensions/check_tx_version.rs | 58 + frame/system/src/extensions/check_weight.rs | 644 +++++++ frame/system/src/extensions/mod.rs | 24 + frame/system/src/lib.rs | 1484 +---------------- frame/system/src/mock.rs | 124 ++ frame/system/src/offchain.rs | 2 +- frame/system/src/tests.rs | 424 +++++ frame/system/src/weights.rs | 76 + 12 files changed, 1757 insertions(+), 1464 deletions(-) create mode 100644 frame/system/src/extensions/check_genesis.rs create mode 100644 frame/system/src/extensions/check_mortality.rs create mode 100644 frame/system/src/extensions/check_nonce.rs create mode 100644 frame/system/src/extensions/check_spec_version.rs create mode 100644 frame/system/src/extensions/check_tx_version.rs create mode 100644 frame/system/src/extensions/check_weight.rs create mode 100644 frame/system/src/extensions/mod.rs create mode 100644 frame/system/src/mock.rs create mode 100644 frame/system/src/tests.rs create mode 100644 frame/system/src/weights.rs diff --git a/frame/system/src/extensions/check_genesis.rs b/frame/system/src/extensions/check_genesis.rs new file mode 100644 index 0000000000000..d0a346519ca23 --- /dev/null +++ b/frame/system/src/extensions/check_genesis.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. + +use codec::{Encode, Decode}; +use crate::{Trait, Module}; +use sp_runtime::{ + traits::{SignedExtension, Zero}, + transaction_validity::TransactionValidityError, +}; + +/// Genesis hash check to provide replay protection between different networks. +#[derive(Encode, Decode, Clone, Eq, PartialEq)] +pub struct CheckGenesis(sp_std::marker::PhantomData); + +impl sp_std::fmt::Debug for CheckGenesis { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckGenesis") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl CheckGenesis { + /// Creates new `SignedExtension` to check genesis hash. + pub fn new() -> Self { + Self(sp_std::marker::PhantomData) + } +} + +impl SignedExtension for CheckGenesis { + type AccountId = T::AccountId; + type Call = ::Call; + type AdditionalSigned = T::Hash; + type Pre = (); + const IDENTIFIER: &'static str = "CheckGenesis"; + + fn additional_signed(&self) -> Result { + Ok(>::block_hash(T::BlockNumber::zero())) + } +} diff --git a/frame/system/src/extensions/check_mortality.rs b/frame/system/src/extensions/check_mortality.rs new file mode 100644 index 0000000000000..cc7496df9a20a --- /dev/null +++ b/frame/system/src/extensions/check_mortality.rs @@ -0,0 +1,124 @@ +// 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. + +use codec::{Encode, Decode}; +use crate::{Trait, Module, BlockHash}; +use frame_support::StorageMap; +use sp_runtime::{ + generic::Era, + traits::{SignedExtension, DispatchInfoOf, SaturatedConversion}, + transaction_validity::{ + ValidTransaction, TransactionValidityError, InvalidTransaction, TransactionValidity, + }, +}; + +/// Check for transaction mortality. +#[derive(Encode, Decode, Clone, Eq, PartialEq)] +pub struct CheckMortality(Era, sp_std::marker::PhantomData); + +impl CheckMortality { + /// utility constructor. Used only in client/factory code. + pub fn from(era: Era) -> Self { + Self(era, sp_std::marker::PhantomData) + } +} + +impl sp_std::fmt::Debug for CheckMortality { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckMortality({:?})", self.0) + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl SignedExtension for CheckMortality { + type AccountId = T::AccountId; + type Call = T::Call; + type AdditionalSigned = T::Hash; + type Pre = (); + // TODO [#6483] rename to CheckMortality + const IDENTIFIER: &'static str = "CheckEra"; + + fn validate( + &self, + _who: &Self::AccountId, + _call: &Self::Call, + _info: &DispatchInfoOf, + _len: usize, + ) -> TransactionValidity { + let current_u64 = >::block_number().saturated_into::(); + let valid_till = self.0.death(current_u64); + Ok(ValidTransaction { + longevity: valid_till.saturating_sub(current_u64), + ..Default::default() + }) + } + + fn additional_signed(&self) -> Result { + let current_u64 = >::block_number().saturated_into::(); + let n = self.0.birth(current_u64).saturated_into::(); + if !>::contains_key(n) { + Err(InvalidTransaction::AncientBirthBlock.into()) + } else { + Ok(>::block_hash(n)) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::mock::{Test, new_test_ext, System, CALL}; + use frame_support::weights::{DispatchClass, DispatchInfo, Pays}; + use sp_core::H256; + + #[test] + fn signed_ext_check_era_should_work() { + new_test_ext().execute_with(|| { + // future + assert_eq!( + CheckMortality::::from(Era::mortal(4, 2)).additional_signed().err().unwrap(), + InvalidTransaction::AncientBirthBlock.into(), + ); + + // correct + System::set_block_number(13); + >::insert(12, H256::repeat_byte(1)); + assert!(CheckMortality::::from(Era::mortal(4, 12)).additional_signed().is_ok()); + }) + } + + #[test] + fn signed_ext_check_era_should_change_longevity() { + new_test_ext().execute_with(|| { + let normal = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; + let len = 0_usize; + let ext = ( + crate::CheckWeight::::default(), + CheckMortality::::from(Era::mortal(16, 256)), + ); + System::set_block_number(17); + >::insert(16, H256::repeat_byte(1)); + + assert_eq!(ext.validate(&1, CALL, &normal, len).unwrap().longevity, 15); + }) + } +} diff --git a/frame/system/src/extensions/check_nonce.rs b/frame/system/src/extensions/check_nonce.rs new file mode 100644 index 0000000000000..1af3a1210aaf4 --- /dev/null +++ b/frame/system/src/extensions/check_nonce.rs @@ -0,0 +1,145 @@ +// 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. + +use codec::{Encode, Decode}; +use crate::Trait; +use frame_support::{ + weights::DispatchInfo, + StorageMap, +}; +use sp_runtime::{ + traits::{SignedExtension, DispatchInfoOf, Dispatchable, One}, + transaction_validity::{ + ValidTransaction, TransactionValidityError, InvalidTransaction, TransactionValidity, + TransactionLongevity, TransactionPriority, + }, +}; +use sp_std::vec; + +/// Nonce check and increment to give replay protection for transactions. +#[derive(Encode, Decode, Clone, Eq, PartialEq)] +pub struct CheckNonce(#[codec(compact)] T::Index); + +impl CheckNonce { + /// utility constructor. Used only in client/factory code. + pub fn from(nonce: T::Index) -> Self { + Self(nonce) + } +} + +impl sp_std::fmt::Debug for CheckNonce { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckNonce({})", self.0) + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl SignedExtension for CheckNonce where + T::Call: Dispatchable +{ + type AccountId = T::AccountId; + type Call = T::Call; + type AdditionalSigned = (); + type Pre = (); + const IDENTIFIER: &'static str = "CheckNonce"; + + fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> { Ok(()) } + + fn pre_dispatch( + self, + who: &Self::AccountId, + _call: &Self::Call, + _info: &DispatchInfoOf, + _len: usize, + ) -> Result<(), TransactionValidityError> { + let mut account = crate::Account::::get(who); + if self.0 != account.nonce { + return Err( + if self.0 < account.nonce { + InvalidTransaction::Stale + } else { + InvalidTransaction::Future + }.into() + ) + } + account.nonce += T::Index::one(); + crate::Account::::insert(who, account); + Ok(()) + } + + fn validate( + &self, + who: &Self::AccountId, + _call: &Self::Call, + info: &DispatchInfoOf, + _len: usize, + ) -> TransactionValidity { + // check index + let account = crate::Account::::get(who); + if self.0 < account.nonce { + return InvalidTransaction::Stale.into() + } + + let provides = vec![Encode::encode(&(who, self.0))]; + let requires = if account.nonce < self.0 { + vec![Encode::encode(&(who, self.0 - One::one()))] + } else { + vec![] + }; + + Ok(ValidTransaction { + priority: info.weight as TransactionPriority, + requires, + provides, + longevity: TransactionLongevity::max_value(), + propagate: true, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::mock::{Test, new_test_ext, CALL}; + + #[test] + fn signed_ext_check_nonce_works() { + new_test_ext().execute_with(|| { + crate::Account::::insert(1, crate::AccountInfo { + nonce: 1, + refcount: 0, + data: 0, + }); + let info = DispatchInfo::default(); + let len = 0_usize; + // stale + assert!(CheckNonce::(0).validate(&1, CALL, &info, len).is_err()); + assert!(CheckNonce::(0).pre_dispatch(&1, CALL, &info, len).is_err()); + // correct + assert!(CheckNonce::(1).validate(&1, CALL, &info, len).is_ok()); + assert!(CheckNonce::(1).pre_dispatch(&1, CALL, &info, len).is_ok()); + // future + assert!(CheckNonce::(5).validate(&1, CALL, &info, len).is_ok()); + assert!(CheckNonce::(5).pre_dispatch(&1, CALL, &info, len).is_err()); + }) + } +} diff --git a/frame/system/src/extensions/check_spec_version.rs b/frame/system/src/extensions/check_spec_version.rs new file mode 100644 index 0000000000000..8dc4d8d9ceddc --- /dev/null +++ b/frame/system/src/extensions/check_spec_version.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. + +use crate::{Trait, Module}; +use codec::{Encode, Decode}; +use sp_runtime::{ + traits::SignedExtension, + transaction_validity::TransactionValidityError, +}; + +/// Ensure the runtime version registered in the transaction is the same as at present. +#[derive(Encode, Decode, Clone, Eq, PartialEq)] +pub struct CheckSpecVersion(sp_std::marker::PhantomData); + +impl sp_std::fmt::Debug for CheckSpecVersion { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckSpecVersion") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl CheckSpecVersion { + /// Create new `SignedExtension` to check runtime version. + pub fn new() -> Self { + Self(sp_std::marker::PhantomData) + } +} + +impl SignedExtension for CheckSpecVersion { + type AccountId = T::AccountId; + type Call = ::Call; + type AdditionalSigned = u32; + type Pre = (); + const IDENTIFIER: &'static str = "CheckSpecVersion"; + + fn additional_signed(&self) -> Result { + Ok(>::runtime_version().spec_version) + } +} diff --git a/frame/system/src/extensions/check_tx_version.rs b/frame/system/src/extensions/check_tx_version.rs new file mode 100644 index 0000000000000..ee6f3349365b9 --- /dev/null +++ b/frame/system/src/extensions/check_tx_version.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. + +use crate::{Trait, Module}; +use codec::{Encode, Decode}; +use sp_runtime::{ + traits::SignedExtension, + transaction_validity::TransactionValidityError, +}; + +/// Ensure the transaction version registered in the transaction is the same as at present. +#[derive(Encode, Decode, Clone, Eq, PartialEq)] +pub struct CheckTxVersion(sp_std::marker::PhantomData); + +impl sp_std::fmt::Debug for CheckTxVersion { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckTxVersion") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +impl CheckTxVersion { + /// Create new `SignedExtension` to check transaction version. + pub fn new() -> Self { + Self(sp_std::marker::PhantomData) + } +} + +impl SignedExtension for CheckTxVersion { + type AccountId = T::AccountId; + type Call = ::Call; + type AdditionalSigned = u32; + type Pre = (); + const IDENTIFIER: &'static str = "CheckTxVersion"; + + fn additional_signed(&self) -> Result { + Ok(>::runtime_version().transaction_version) + } +} diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs new file mode 100644 index 0000000000000..d52138b1e3bbe --- /dev/null +++ b/frame/system/src/extensions/check_weight.rs @@ -0,0 +1,644 @@ +// 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. + +use crate::{Trait, Module}; +use codec::{Encode, Decode}; +use sp_runtime::{ + traits::{SignedExtension, DispatchInfoOf, Dispatchable, PostDispatchInfoOf, Printable}, + transaction_validity::{ + ValidTransaction, TransactionValidityError, InvalidTransaction, TransactionValidity, + TransactionPriority, + }, + Perbill, DispatchResult, +}; +use frame_support::{ + traits::{Get}, + weights::{PostDispatchInfo, DispatchInfo, DispatchClass}, + StorageValue, +}; + +/// Block resource (weight) limit check. +#[derive(Encode, Decode, Clone, Eq, PartialEq, Default)] +pub struct CheckWeight(sp_std::marker::PhantomData); + +impl CheckWeight where + T::Call: Dispatchable +{ + /// Get the quota ratio of each dispatch class type. This indicates that all operational and mandatory + /// dispatches can use the full capacity of any resource, while user-triggered ones can consume + /// a portion. + fn get_dispatch_limit_ratio(class: DispatchClass) -> Perbill { + match class { + DispatchClass::Operational | DispatchClass::Mandatory + => ::one(), + DispatchClass::Normal => T::AvailableBlockRatio::get(), + } + } + + /// Checks if the current extrinsic does not exceed `MaximumExtrinsicWeight` limit. + fn check_extrinsic_weight( + info: &DispatchInfoOf, + ) -> Result<(), TransactionValidityError> { + match info.class { + // Mandatory transactions are included in a block unconditionally, so + // we don't verify weight. + DispatchClass::Mandatory => Ok(()), + // Normal transactions must not exceed `MaximumExtrinsicWeight`. + DispatchClass::Normal => { + let maximum_weight = T::MaximumExtrinsicWeight::get(); + let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); + if extrinsic_weight > maximum_weight { + Err(InvalidTransaction::ExhaustsResources.into()) + } else { + Ok(()) + } + }, + // For operational transactions we make sure it doesn't exceed + // the space alloted for `Operational` class. + DispatchClass::Operational => { + let maximum_weight = T::MaximumBlockWeight::get(); + let operational_limit = + Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let operational_limit = + operational_limit.saturating_sub(T::BlockExecutionWeight::get()); + let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); + if extrinsic_weight > operational_limit { + Err(InvalidTransaction::ExhaustsResources.into()) + } else { + Ok(()) + } + }, + } + } + + /// Checks if the current extrinsic can fit into the block with respect to block weight limits. + /// + /// Upon successes, it returns the new block weight as a `Result`. + fn check_block_weight( + info: &DispatchInfoOf, + ) -> Result { + let maximum_weight = T::MaximumBlockWeight::get(); + let mut all_weight = Module::::block_weight(); + match info.class { + // If we have a dispatch that must be included in the block, it ignores all the limits. + DispatchClass::Mandatory => { + let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); + all_weight.add(extrinsic_weight, DispatchClass::Mandatory); + Ok(all_weight) + }, + // If we have a normal dispatch, we follow all the normal rules and limits. + DispatchClass::Normal => { + let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; + let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) + .ok_or(InvalidTransaction::ExhaustsResources)?; + all_weight.checked_add(extrinsic_weight, DispatchClass::Normal) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; + if all_weight.get(DispatchClass::Normal) > normal_limit { + Err(InvalidTransaction::ExhaustsResources.into()) + } else { + Ok(all_weight) + } + }, + // If we have an operational dispatch, allow it if we have not used our full + // "operational space" (independent of existing fullness). + DispatchClass::Operational => { + let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; + let operational_space = operational_limit.saturating_sub(normal_limit); + + let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) + .ok_or(InvalidTransaction::ExhaustsResources)?; + all_weight.checked_add(extrinsic_weight, DispatchClass::Operational) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; + + // If it would fit in normally, its okay + if all_weight.total() <= maximum_weight || + // If we have not used our operational space + all_weight.get(DispatchClass::Operational) <= operational_space { + Ok(all_weight) + } else { + Err(InvalidTransaction::ExhaustsResources.into()) + } + } + } + } + + /// Checks if the current extrinsic can fit into the block with respect to block length limits. + /// + /// Upon successes, it returns the new block length as a `Result`. + fn check_block_length( + info: &DispatchInfoOf, + len: usize, + ) -> Result { + let current_len = Module::::all_extrinsics_len(); + let maximum_len = T::MaximumBlockLength::get(); + let limit = Self::get_dispatch_limit_ratio(info.class) * maximum_len; + let added_len = len as u32; + let next_len = current_len.saturating_add(added_len); + if next_len > limit { + Err(InvalidTransaction::ExhaustsResources.into()) + } else { + Ok(next_len) + } + } + + /// get the priority of an extrinsic denoted by `info`. + fn get_priority(info: &DispatchInfoOf) -> TransactionPriority { + match info.class { + DispatchClass::Normal => info.weight.into(), + // Don't use up the whole priority space, to allow things like `tip` + // to be taken into account as well. + DispatchClass::Operational => TransactionPriority::max_value() / 2, + // Mandatory extrinsics are only for inherents; never transactions. + DispatchClass::Mandatory => TransactionPriority::min_value(), + } + } + + /// Creates new `SignedExtension` to check weight of the extrinsic. + pub fn new() -> Self { + Self(Default::default()) + } + + /// Do the pre-dispatch checks. This can be applied to both signed and unsigned. + /// + /// It checks and notes the new weight and length. + fn do_pre_dispatch( + info: &DispatchInfoOf, + len: usize, + ) -> Result<(), TransactionValidityError> { + let next_len = Self::check_block_length(info, len)?; + let next_weight = Self::check_block_weight(info)?; + Self::check_extrinsic_weight(info)?; + + crate::AllExtrinsicsLen::put(next_len); + crate::BlockWeight::put(next_weight); + Ok(()) + } + + /// Do the validate checks. This can be applied to both signed and unsigned. + /// + /// It only checks that the block weight and length limit will not exceed. + fn do_validate( + info: &DispatchInfoOf, + len: usize, + ) -> TransactionValidity { + // ignore the next length. If they return `Ok`, then it is below the limit. + let _ = Self::check_block_length(info, len)?; + // during validation we skip block limit check. Since the `validate_transaction` + // call runs on an empty block anyway, by this we prevent `on_initialize` weight + // consumption from causing false negatives. + Self::check_extrinsic_weight(info)?; + + Ok(ValidTransaction { priority: Self::get_priority(info), ..Default::default() }) + } +} + +impl SignedExtension for CheckWeight where + T::Call: Dispatchable +{ + type AccountId = T::AccountId; + type Call = T::Call; + type AdditionalSigned = (); + type Pre = (); + const IDENTIFIER: &'static str = "CheckWeight"; + + fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> { Ok(()) } + + fn pre_dispatch( + self, + _who: &Self::AccountId, + _call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result<(), TransactionValidityError> { + if info.class == DispatchClass::Mandatory { + Err(InvalidTransaction::MandatoryDispatch)? + } + Self::do_pre_dispatch(info, len) + } + + fn validate( + &self, + _who: &Self::AccountId, + _call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> TransactionValidity { + if info.class == DispatchClass::Mandatory { + Err(InvalidTransaction::MandatoryDispatch)? + } + Self::do_validate(info, len) + } + + fn pre_dispatch_unsigned( + _call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result<(), TransactionValidityError> { + Self::do_pre_dispatch(info, len) + } + + fn validate_unsigned( + _call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> TransactionValidity { + Self::do_validate(info, len) + } + + fn post_dispatch( + _pre: Self::Pre, + info: &DispatchInfoOf, + post_info: &PostDispatchInfoOf, + _len: usize, + result: &DispatchResult, + ) -> Result<(), TransactionValidityError> { + // Since mandatory dispatched do not get validated for being overweight, we are sensitive + // to them actually being useful. Block producers are thus not allowed to include mandatory + // extrinsics that result in error. + if let (DispatchClass::Mandatory, Err(e)) = (info.class, result) { + "Bad mandantory".print(); + e.print(); + + Err(InvalidTransaction::BadMandatory)? + } + + let unspent = post_info.calc_unspent(info); + if unspent > 0 { + crate::BlockWeight::mutate(|current_weight| { + current_weight.sub(unspent, info.class); + }) + } + + Ok(()) + } +} + +impl sp_std::fmt::Debug for CheckWeight { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + write!(f, "CheckWeight") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{BlockWeight, AllExtrinsicsLen}; + use crate::mock::{Test, CALL, new_test_ext, System}; + use sp_std::marker::PhantomData; + use frame_support::{assert_ok, assert_noop}; + use frame_support::weights::{Weight, Pays}; + + fn normal_weight_limit() -> Weight { + ::AvailableBlockRatio::get() * ::MaximumBlockWeight::get() + } + + fn normal_length_limit() -> u32 { + ::AvailableBlockRatio::get() * ::MaximumBlockLength::get() + } + + #[test] + fn mandatory_extrinsic_doesnt_care_about_limits() { + fn check(call: impl FnOnce(&DispatchInfo, usize)) { + new_test_ext().execute_with(|| { + let max = DispatchInfo { + weight: Weight::max_value(), + class: DispatchClass::Mandatory, + ..Default::default() + }; + let len = 0_usize; + + call(&max, len); + }); + } + + check(|max, len| { + assert_ok!(CheckWeight::::do_pre_dispatch(max, len)); + assert_eq!(System::block_weight().total(), Weight::max_value()); + assert!(System::block_weight().total() > ::MaximumBlockWeight::get()); + }); + check(|max, len| { + assert_ok!(CheckWeight::::do_validate(max, len)); + }); + } + + #[test] + fn normal_extrinsic_limited_by_maximum_extrinsic_weight() { + new_test_ext().execute_with(|| { + let max = DispatchInfo { + weight: ::MaximumExtrinsicWeight::get() + 1, + class: DispatchClass::Normal, + ..Default::default() + }; + let len = 0_usize; + + assert_noop!( + CheckWeight::::do_validate(&max, len), + InvalidTransaction::ExhaustsResources + ); + }); + } + + #[test] + fn operational_extrinsic_limited_by_operational_space_limit() { + new_test_ext().execute_with(|| { + let operational_limit = CheckWeight::::get_dispatch_limit_ratio( + DispatchClass::Operational + ) * ::MaximumBlockWeight::get(); + let base_weight = ::ExtrinsicBaseWeight::get(); + let block_base = ::BlockExecutionWeight::get(); + + let weight = operational_limit - base_weight - block_base; + let okay = DispatchInfo { + weight, + class: DispatchClass::Operational, + ..Default::default() + }; + let max = DispatchInfo { + weight: weight + 1, + class: DispatchClass::Operational, + ..Default::default() + }; + let len = 0_usize; + + assert_eq!( + CheckWeight::::do_validate(&okay, len), + Ok(ValidTransaction { + priority: CheckWeight::::get_priority(&okay), + ..Default::default() + }) + ); + assert_noop!( + CheckWeight::::do_validate(&max, len), + InvalidTransaction::ExhaustsResources + ); + }); + } + + #[test] + fn register_extra_weight_unchecked_doesnt_care_about_limits() { + new_test_ext().execute_with(|| { + System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Normal); + assert_eq!(System::block_weight().total(), Weight::max_value()); + assert!(System::block_weight().total() > ::MaximumBlockWeight::get()); + }); + } + + #[test] + fn full_block_with_normal_and_operational() { + new_test_ext().execute_with(|| { + // Max block is 1024 + // Max normal is 768 (75%) + // 10 is taken for block execution weight + // So normal extrinsic can be 758 weight (-5 for base extrinsic weight) + // And Operational can be 256 to produce a full block (-5 for base) + let max_normal = DispatchInfo { weight: 753, ..Default::default() }; + let rest_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; + + let len = 0_usize; + + assert_ok!(CheckWeight::::do_pre_dispatch(&max_normal, len)); + assert_eq!(System::block_weight().total(), 768); + assert_ok!(CheckWeight::::do_pre_dispatch(&rest_operational, len)); + assert_eq!(::MaximumBlockWeight::get(), 1024); + assert_eq!(System::block_weight().total(), ::MaximumBlockWeight::get()); + // Checking single extrinsic should not take current block weight into account. + assert_eq!(CheckWeight::::check_extrinsic_weight(&rest_operational), Ok(())); + }); + } + + #[test] + fn dispatch_order_does_not_effect_weight_logic() { + new_test_ext().execute_with(|| { + // We switch the order of `full_block_with_normal_and_operational` + let max_normal = DispatchInfo { weight: 753, ..Default::default() }; + let rest_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; + + let len = 0_usize; + + assert_ok!(CheckWeight::::do_pre_dispatch(&rest_operational, len)); + // Extra 15 here from block execution + base extrinsic weight + assert_eq!(System::block_weight().total(), 266); + assert_ok!(CheckWeight::::do_pre_dispatch(&max_normal, len)); + assert_eq!(::MaximumBlockWeight::get(), 1024); + assert_eq!(System::block_weight().total(), ::MaximumBlockWeight::get()); + }); + } + + #[test] + fn operational_works_on_full_block() { + new_test_ext().execute_with(|| { + // An on_initialize takes up the whole block! (Every time!) + System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Mandatory); + let dispatch_normal = DispatchInfo { weight: 251, class: DispatchClass::Normal, ..Default::default() }; + let dispatch_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; + let len = 0_usize; + + assert_noop!( + CheckWeight::::do_pre_dispatch(&dispatch_normal, len), + InvalidTransaction::ExhaustsResources + ); + // Thank goodness we can still do an operational transaction to possibly save the blockchain. + assert_ok!(CheckWeight::::do_pre_dispatch(&dispatch_operational, len)); + // Not too much though + assert_noop!( + CheckWeight::::do_pre_dispatch(&dispatch_operational, len), + InvalidTransaction::ExhaustsResources + ); + // Even with full block, validity of single transaction should be correct. + assert_eq!(CheckWeight::::check_extrinsic_weight(&dispatch_operational), Ok(())); + }); + } + + #[test] + fn signed_ext_check_weight_works_operational_tx() { + new_test_ext().execute_with(|| { + let normal = DispatchInfo { weight: 100, ..Default::default() }; + let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; + let len = 0_usize; + let normal_limit = normal_weight_limit(); + + // given almost full block + BlockWeight::mutate(|current_weight| { + current_weight.put(normal_limit, DispatchClass::Normal) + }); + // will not fit. + assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err()); + // will fit. + assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok()); + + // likewise for length limit. + let len = 100_usize; + AllExtrinsicsLen::put(normal_length_limit()); + assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err()); + assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok()); + }) + } + + #[test] + fn signed_ext() { + new_test_ext().execute_with(|| { + let normal = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; + let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; + let len = 0_usize; + + let priority = CheckWeight::(PhantomData) + .validate(&1, CALL, &normal, len) + .unwrap() + .priority; + assert_eq!(priority, 100); + + let priority = CheckWeight::(PhantomData) + .validate(&1, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, u64::max_value() / 2); + }) + } + + #[test] + fn signed_ext_check_weight_block_size_works() { + new_test_ext().execute_with(|| { + let normal = DispatchInfo::default(); + let normal_limit = normal_weight_limit() as usize; + let reset_check_weight = |tx, s, f| { + AllExtrinsicsLen::put(0); + let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, tx, s); + if f { assert!(r.is_err()) } else { assert!(r.is_ok()) } + }; + + reset_check_weight(&normal, normal_limit - 1, false); + reset_check_weight(&normal, normal_limit, false); + reset_check_weight(&normal, normal_limit + 1, true); + + // Operational ones don't have this limit. + let op = DispatchInfo { weight: 0, class: DispatchClass::Operational, pays_fee: Pays::Yes }; + reset_check_weight(&op, normal_limit, false); + reset_check_weight(&op, normal_limit + 100, false); + reset_check_weight(&op, 1024, false); + reset_check_weight(&op, 1025, true); + }) + } + + + #[test] + fn signed_ext_check_weight_works_normal_tx() { + new_test_ext().execute_with(|| { + let normal_limit = normal_weight_limit(); + let small = DispatchInfo { weight: 100, ..Default::default() }; + let medium = DispatchInfo { + weight: normal_limit - ::ExtrinsicBaseWeight::get(), + ..Default::default() + }; + let big = DispatchInfo { + weight: normal_limit - ::ExtrinsicBaseWeight::get() + 1, + ..Default::default() + }; + let len = 0_usize; + + let reset_check_weight = |i, f, s| { + BlockWeight::mutate(|current_weight| { + current_weight.put(s, DispatchClass::Normal) + }); + let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, i, len); + if f { assert!(r.is_err()) } else { assert!(r.is_ok()) } + }; + + reset_check_weight(&small, false, 0); + reset_check_weight(&medium, false, 0); + reset_check_weight(&big, true, 1); + }) + } + + #[test] + fn signed_ext_check_weight_refund_works() { + new_test_ext().execute_with(|| { + // This is half of the max block weight + let info = DispatchInfo { weight: 512, ..Default::default() }; + let post_info = PostDispatchInfo { actual_weight: Some(128), }; + let len = 0_usize; + + // We allow 75% for normal transaction, so we put 25% - extrinsic base weight + BlockWeight::mutate(|current_weight| { + current_weight.put(256 - ::ExtrinsicBaseWeight::get(), DispatchClass::Normal) + }); + + let pre = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &info, len).unwrap(); + assert_eq!(BlockWeight::get().total(), info.weight + 256); + + assert!( + CheckWeight::::post_dispatch(pre, &info, &post_info, len, &Ok(())) + .is_ok() + ); + assert_eq!( + BlockWeight::get().total(), + post_info.actual_weight.unwrap() + 256, + ); + }) + } + + #[test] + fn signed_ext_check_weight_actual_weight_higher_than_max_is_capped() { + new_test_ext().execute_with(|| { + let info = DispatchInfo { weight: 512, ..Default::default() }; + let post_info = PostDispatchInfo { actual_weight: Some(700), }; + let len = 0_usize; + + BlockWeight::mutate(|current_weight| { + current_weight.put(128, DispatchClass::Normal) + }); + + let pre = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &info, len).unwrap(); + assert_eq!( + BlockWeight::get().total(), + info.weight + 128 + ::ExtrinsicBaseWeight::get(), + ); + + assert!( + CheckWeight::::post_dispatch(pre, &info, &post_info, len, &Ok(())) + .is_ok() + ); + assert_eq!( + BlockWeight::get().total(), + info.weight + 128 + ::ExtrinsicBaseWeight::get(), + ); + }) + } + + #[test] + fn zero_weight_extrinsic_still_has_base_weight() { + new_test_ext().execute_with(|| { + let free = DispatchInfo { weight: 0, ..Default::default() }; + let len = 0_usize; + + // Initial weight from `BlockExecutionWeight` + assert_eq!(System::block_weight().total(), ::BlockExecutionWeight::get()); + let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &free, len); + assert!(r.is_ok()); + assert_eq!( + System::block_weight().total(), + ::ExtrinsicBaseWeight::get() + ::BlockExecutionWeight::get() + ); + }) + } +} diff --git a/frame/system/src/extensions/mod.rs b/frame/system/src/extensions/mod.rs new file mode 100644 index 0000000000000..ff61353e2d176 --- /dev/null +++ b/frame/system/src/extensions/mod.rs @@ -0,0 +1,24 @@ +// This file is part of Substrate. + +// 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. + +pub mod check_genesis; +pub mod check_mortality; +pub mod check_nonce; +pub mod check_spec_version; +pub mod check_tx_version; +pub mod check_weight; + diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 8eec6a2c37585..18723fff299be 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -102,17 +102,12 @@ use sp_std::marker::PhantomData; use sp_std::fmt::Debug; use sp_version::RuntimeVersion; use sp_runtime::{ - RuntimeDebug, Perbill, DispatchError, DispatchResult, Either, - generic::{self, Era}, - transaction_validity::{ - ValidTransaction, TransactionPriority, TransactionLongevity, TransactionValidityError, - InvalidTransaction, TransactionValidity, - }, + RuntimeDebug, Perbill, DispatchError, Either, generic, traits::{ - self, CheckEqual, AtLeast32Bit, Zero, SignedExtension, Lookup, LookupError, - SimpleBitOps, Hash, Member, MaybeDisplay, BadOrigin, SaturatedConversion, + self, CheckEqual, AtLeast32Bit, Zero, Lookup, LookupError, + SimpleBitOps, Hash, Member, MaybeDisplay, BadOrigin, MaybeSerialize, MaybeSerializeDeserialize, MaybeMallocSizeOf, StaticLookup, One, Bounded, - Dispatchable, DispatchInfoOf, PostDispatchInfoOf, Printable, + Dispatchable, }, offchain::storage_lock::BlockNumberProvider, }; @@ -126,7 +121,7 @@ use frame_support::{ StoredMap, EnsureOrigin, OriginTrait, Filter, }, weights::{ - Weight, RuntimeDbWeight, DispatchInfo, PostDispatchInfo, DispatchClass, + Weight, RuntimeDbWeight, DispatchInfo, DispatchClass, extract_actual_weight, }, dispatch::DispatchResultWithPostInfo, @@ -137,6 +132,21 @@ use codec::{Encode, Decode, FullCodec, EncodeLike}; use sp_io::TestExternalities; pub mod offchain; +#[cfg(test)] +pub(crate) mod mock; + +mod extensions; +mod weights; +#[cfg(test)] +mod tests; + +pub use extensions::{ + check_mortality::CheckMortality, check_genesis::CheckGenesis, check_nonce::CheckNonce, + check_spec_version::CheckSpecVersion, check_tx_version::CheckTxVersion, + check_weight::CheckWeight, +}; +// Backward compatible re-export. +pub use extensions::check_mortality::CheckMortality as CheckEra; /// Compute the trie root of a list of extrinsics. pub fn extrinsics_root(extrinsics: &[E]) -> H::Output { @@ -372,60 +382,6 @@ impl From for LastRuntimeUpgradeInfo { } } -/// An object to track the currently used extrinsic weight in a block. -#[derive(Clone, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode)] -pub struct ExtrinsicsWeight { - normal: Weight, - operational: Weight, -} - -impl ExtrinsicsWeight { - /// Returns the total weight consumed by all extrinsics in the block. - pub fn total(&self) -> Weight { - self.normal.saturating_add(self.operational) - } - - /// Add some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`. - pub fn add(&mut self, weight: Weight, class: DispatchClass) { - let value = self.get_mut(class); - *value = value.saturating_add(weight); - } - - /// Try to add some weight of a specific dispatch class, returning Err(()) if overflow would occur. - pub fn checked_add(&mut self, weight: Weight, class: DispatchClass) -> Result<(), ()> { - let value = self.get_mut(class); - *value = value.checked_add(weight).ok_or(())?; - Ok(()) - } - - /// Subtract some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`. - pub fn sub(&mut self, weight: Weight, class: DispatchClass) { - let value = self.get_mut(class); - *value = value.saturating_sub(weight); - } - - /// Get the current weight of a specific dispatch class. - pub fn get(&self, class: DispatchClass) -> Weight { - match class { - DispatchClass::Operational => self.operational, - DispatchClass::Normal | DispatchClass::Mandatory => self.normal, - } - } - - /// Get a mutable reference to the current weight of a specific dispatch class. - fn get_mut(&mut self, class: DispatchClass) -> &mut Weight { - match class { - DispatchClass::Operational => &mut self.operational, - DispatchClass::Normal | DispatchClass::Mandatory => &mut self.normal, - } - } - - /// Set the weight of a specific dispatch class. - pub fn put(&mut self, new: Weight, class: DispatchClass) { - *self.get_mut(class) = new; - } -} - decl_storage! { trait Store for Module as System { /// The full account information for a particular account ID. @@ -436,7 +392,7 @@ decl_storage! { ExtrinsicCount: Option; /// The current weight for the block. - BlockWeight get(fn block_weight): ExtrinsicsWeight; + BlockWeight get(fn block_weight): weights::ExtrinsicsWeight; /// Total length (in bytes) for all extrinsics put together, for the current block. AllExtrinsicsLen: Option; @@ -1372,360 +1328,6 @@ pub fn split_inner(option: Option, splitter: impl FnOnce(T) -> (R, S } } -/// resource limit check. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckWeight(PhantomData); - -impl CheckWeight where - T::Call: Dispatchable -{ - /// Get the quota ratio of each dispatch class type. This indicates that all operational and mandatory - /// dispatches can use the full capacity of any resource, while user-triggered ones can consume - /// a portion. - fn get_dispatch_limit_ratio(class: DispatchClass) -> Perbill { - match class { - DispatchClass::Operational | DispatchClass::Mandatory - => ::one(), - DispatchClass::Normal => T::AvailableBlockRatio::get(), - } - } - - /// Checks if the current extrinsic does not exceed `MaximumExtrinsicWeight` limit. - fn check_extrinsic_weight( - info: &DispatchInfoOf, - ) -> Result<(), TransactionValidityError> { - match info.class { - // Mandatory transactions are included in a block unconditionally, so - // we don't verify weight. - DispatchClass::Mandatory => Ok(()), - // Normal transactions must not exceed `MaximumExtrinsicWeight`. - DispatchClass::Normal => { - let maximum_weight = T::MaximumExtrinsicWeight::get(); - let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); - if extrinsic_weight > maximum_weight { - Err(InvalidTransaction::ExhaustsResources.into()) - } else { - Ok(()) - } - }, - // For operational transactions we make sure it doesn't exceed - // the space alloted for `Operational` class. - DispatchClass::Operational => { - let maximum_weight = T::MaximumBlockWeight::get(); - let operational_limit = - Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; - let operational_limit = - operational_limit.saturating_sub(T::BlockExecutionWeight::get()); - let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); - if extrinsic_weight > operational_limit { - Err(InvalidTransaction::ExhaustsResources.into()) - } else { - Ok(()) - } - }, - } - } - - /// Checks if the current extrinsic can fit into the block with respect to block weight limits. - /// - /// Upon successes, it returns the new block weight as a `Result`. - fn check_block_weight( - info: &DispatchInfoOf, - ) -> Result { - let maximum_weight = T::MaximumBlockWeight::get(); - let mut all_weight = Module::::block_weight(); - match info.class { - // If we have a dispatch that must be included in the block, it ignores all the limits. - DispatchClass::Mandatory => { - let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); - all_weight.add(extrinsic_weight, DispatchClass::Mandatory); - Ok(all_weight) - }, - // If we have a normal dispatch, we follow all the normal rules and limits. - DispatchClass::Normal => { - let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; - let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) - .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight.checked_add(extrinsic_weight, DispatchClass::Normal) - .map_err(|_| InvalidTransaction::ExhaustsResources)?; - if all_weight.get(DispatchClass::Normal) > normal_limit { - Err(InvalidTransaction::ExhaustsResources.into()) - } else { - Ok(all_weight) - } - }, - // If we have an operational dispatch, allow it if we have not used our full - // "operational space" (independent of existing fullness). - DispatchClass::Operational => { - let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; - let normal_limit = Self::get_dispatch_limit_ratio(DispatchClass::Normal) * maximum_weight; - let operational_space = operational_limit.saturating_sub(normal_limit); - - let extrinsic_weight = info.weight.checked_add(T::ExtrinsicBaseWeight::get()) - .ok_or(InvalidTransaction::ExhaustsResources)?; - all_weight.checked_add(extrinsic_weight, DispatchClass::Operational) - .map_err(|_| InvalidTransaction::ExhaustsResources)?; - - // If it would fit in normally, its okay - if all_weight.total() <= maximum_weight || - // If we have not used our operational space - all_weight.get(DispatchClass::Operational) <= operational_space { - Ok(all_weight) - } else { - Err(InvalidTransaction::ExhaustsResources.into()) - } - } - } - } - - /// Checks if the current extrinsic can fit into the block with respect to block length limits. - /// - /// Upon successes, it returns the new block length as a `Result`. - fn check_block_length( - info: &DispatchInfoOf, - len: usize, - ) -> Result { - let current_len = Module::::all_extrinsics_len(); - let maximum_len = T::MaximumBlockLength::get(); - let limit = Self::get_dispatch_limit_ratio(info.class) * maximum_len; - let added_len = len as u32; - let next_len = current_len.saturating_add(added_len); - if next_len > limit { - Err(InvalidTransaction::ExhaustsResources.into()) - } else { - Ok(next_len) - } - } - - /// get the priority of an extrinsic denoted by `info`. - fn get_priority(info: &DispatchInfoOf) -> TransactionPriority { - match info.class { - DispatchClass::Normal => info.weight.into(), - // Don't use up the whole priority space, to allow things like `tip` - // to be taken into account as well. - DispatchClass::Operational => TransactionPriority::max_value() / 2, - // Mandatory extrinsics are only for inherents; never transactions. - DispatchClass::Mandatory => TransactionPriority::min_value(), - } - } - - /// Creates new `SignedExtension` to check weight of the extrinsic. - pub fn new() -> Self { - Self(PhantomData) - } - - /// Do the pre-dispatch checks. This can be applied to both signed and unsigned. - /// - /// It checks and notes the new weight and length. - fn do_pre_dispatch( - info: &DispatchInfoOf, - len: usize, - ) -> Result<(), TransactionValidityError> { - let next_len = Self::check_block_length(info, len)?; - let next_weight = Self::check_block_weight(info)?; - Self::check_extrinsic_weight(info)?; - - AllExtrinsicsLen::put(next_len); - BlockWeight::put(next_weight); - Ok(()) - } - - /// Do the validate checks. This can be applied to both signed and unsigned. - /// - /// It only checks that the block weight and length limit will not exceed. - fn do_validate( - info: &DispatchInfoOf, - len: usize, - ) -> TransactionValidity { - // ignore the next length. If they return `Ok`, then it is below the limit. - let _ = Self::check_block_length(info, len)?; - // during validation we skip block limit check. Since the `validate_transaction` - // call runs on an empty block anyway, by this we prevent `on_initialize` weight - // consumption from causing false negatives. - Self::check_extrinsic_weight(info)?; - - Ok(ValidTransaction { priority: Self::get_priority(info), ..Default::default() }) - } -} - -impl SignedExtension for CheckWeight where - T::Call: Dispatchable -{ - type AccountId = T::AccountId; - type Call = T::Call; - type AdditionalSigned = (); - type Pre = (); - const IDENTIFIER: &'static str = "CheckWeight"; - - fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> { Ok(()) } - - fn pre_dispatch( - self, - _who: &Self::AccountId, - _call: &Self::Call, - info: &DispatchInfoOf, - len: usize, - ) -> Result<(), TransactionValidityError> { - if info.class == DispatchClass::Mandatory { - Err(InvalidTransaction::MandatoryDispatch)? - } - Self::do_pre_dispatch(info, len) - } - - fn validate( - &self, - _who: &Self::AccountId, - _call: &Self::Call, - info: &DispatchInfoOf, - len: usize, - ) -> TransactionValidity { - if info.class == DispatchClass::Mandatory { - Err(InvalidTransaction::MandatoryDispatch)? - } - Self::do_validate(info, len) - } - - fn pre_dispatch_unsigned( - _call: &Self::Call, - info: &DispatchInfoOf, - len: usize, - ) -> Result<(), TransactionValidityError> { - Self::do_pre_dispatch(info, len) - } - - fn validate_unsigned( - _call: &Self::Call, - info: &DispatchInfoOf, - len: usize, - ) -> TransactionValidity { - Self::do_validate(info, len) - } - - fn post_dispatch( - _pre: Self::Pre, - info: &DispatchInfoOf, - post_info: &PostDispatchInfoOf, - _len: usize, - result: &DispatchResult, - ) -> Result<(), TransactionValidityError> { - // Since mandatory dispatched do not get validated for being overweight, we are sensitive - // to them actually being useful. Block producers are thus not allowed to include mandatory - // extrinsics that result in error. - if let (DispatchClass::Mandatory, Err(e)) = (info.class, result) { - "Bad mandantory".print(); - e.print(); - - Err(InvalidTransaction::BadMandatory)? - } - - let unspent = post_info.calc_unspent(info); - if unspent > 0 { - BlockWeight::mutate(|current_weight| { - current_weight.sub(unspent, info.class); - }) - } - - Ok(()) - } -} - -impl Debug for CheckWeight { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckWeight") - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -/// Nonce check and increment to give replay protection for transactions. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckNonce(#[codec(compact)] T::Index); - -impl CheckNonce { - /// utility constructor. Used only in client/factory code. - pub fn from(nonce: T::Index) -> Self { - Self(nonce) - } -} - -impl Debug for CheckNonce { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckNonce({})", self.0) - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl SignedExtension for CheckNonce where - T::Call: Dispatchable -{ - type AccountId = T::AccountId; - type Call = T::Call; - type AdditionalSigned = (); - type Pre = (); - const IDENTIFIER: &'static str = "CheckNonce"; - - fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> { Ok(()) } - - fn pre_dispatch( - self, - who: &Self::AccountId, - _call: &Self::Call, - _info: &DispatchInfoOf, - _len: usize, - ) -> Result<(), TransactionValidityError> { - let mut account = Account::::get(who); - if self.0 != account.nonce { - return Err( - if self.0 < account.nonce { - InvalidTransaction::Stale - } else { - InvalidTransaction::Future - }.into() - ) - } - account.nonce += T::Index::one(); - Account::::insert(who, account); - Ok(()) - } - - fn validate( - &self, - who: &Self::AccountId, - _call: &Self::Call, - info: &DispatchInfoOf, - _len: usize, - ) -> TransactionValidity { - // check index - let account = Account::::get(who); - if self.0 < account.nonce { - return InvalidTransaction::Stale.into() - } - - let provides = vec![Encode::encode(&(who, self.0))]; - let requires = if account.nonce < self.0 { - vec![Encode::encode(&(who, self.0 - One::one()))] - } else { - vec![] - }; - - Ok(ValidTransaction { - priority: info.weight as TransactionPriority, - requires, - provides, - longevity: TransactionLongevity::max_value(), - propagate: true, - }) - } -} impl IsDeadAccount for Module { fn is_dead_account(who: &T::AccountId) -> bool { @@ -1733,167 +1335,6 @@ impl IsDeadAccount for Module { } } -/// Check for transaction mortality. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckEra(Era, sp_std::marker::PhantomData); - -impl CheckEra { - /// utility constructor. Used only in client/factory code. - pub fn from(era: Era) -> Self { - Self(era, sp_std::marker::PhantomData) - } -} - -impl Debug for CheckEra { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckEra({:?})", self.0) - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl SignedExtension for CheckEra { - type AccountId = T::AccountId; - type Call = T::Call; - type AdditionalSigned = T::Hash; - type Pre = (); - const IDENTIFIER: &'static str = "CheckEra"; - - fn validate( - &self, - _who: &Self::AccountId, - _call: &Self::Call, - _info: &DispatchInfoOf, - _len: usize, - ) -> TransactionValidity { - let current_u64 = >::block_number().saturated_into::(); - let valid_till = self.0.death(current_u64); - Ok(ValidTransaction { - longevity: valid_till.saturating_sub(current_u64), - ..Default::default() - }) - } - - fn additional_signed(&self) -> Result { - let current_u64 = >::block_number().saturated_into::(); - let n = self.0.birth(current_u64).saturated_into::(); - if !>::contains_key(n) { - Err(InvalidTransaction::AncientBirthBlock.into()) - } else { - Ok(>::block_hash(n)) - } - } -} - -/// Nonce check and increment to give replay protection for transactions. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckGenesis(sp_std::marker::PhantomData); - -impl Debug for CheckGenesis { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckGenesis") - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl CheckGenesis { - /// Creates new `SignedExtension` to check genesis hash. - pub fn new() -> Self { - Self(sp_std::marker::PhantomData) - } -} - -impl SignedExtension for CheckGenesis { - type AccountId = T::AccountId; - type Call = ::Call; - type AdditionalSigned = T::Hash; - type Pre = (); - const IDENTIFIER: &'static str = "CheckGenesis"; - - fn additional_signed(&self) -> Result { - Ok(>::block_hash(T::BlockNumber::zero())) - } -} - -/// Ensure the transaction version registered in the transaction is the same as at present. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckTxVersion(sp_std::marker::PhantomData); - -impl Debug for CheckTxVersion { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckTxVersion") - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl CheckTxVersion { - /// Create new `SignedExtension` to check transaction version. - pub fn new() -> Self { - Self(sp_std::marker::PhantomData) - } -} - -impl SignedExtension for CheckTxVersion { - type AccountId = T::AccountId; - type Call = ::Call; - type AdditionalSigned = u32; - type Pre = (); - const IDENTIFIER: &'static str = "CheckTxVersion"; - - fn additional_signed(&self) -> Result { - Ok(>::runtime_version().transaction_version) - } -} - -/// Ensure the runtime version registered in the transaction is the same as at present. -#[derive(Encode, Decode, Clone, Eq, PartialEq)] -pub struct CheckSpecVersion(sp_std::marker::PhantomData); - -impl Debug for CheckSpecVersion { - #[cfg(feature = "std")] - fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - write!(f, "CheckSpecVersion") - } - - #[cfg(not(feature = "std"))] - fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result { - Ok(()) - } -} - -impl CheckSpecVersion { - /// Create new `SignedExtension` to check runtime version. - pub fn new() -> Self { - Self(sp_std::marker::PhantomData) - } -} - -impl SignedExtension for CheckSpecVersion { - type AccountId = T::AccountId; - type Call = ::Call; - type AdditionalSigned = u32; - type Pre = (); - const IDENTIFIER: &'static str = "CheckSpecVersion"; - - fn additional_signed(&self) -> Result { - Ok(>::runtime_version().spec_version) - } -} - pub struct ChainContext(sp_std::marker::PhantomData); impl Default for ChainContext { fn default() -> Self { @@ -1909,886 +1350,3 @@ impl Lookup for ChainContext { ::lookup(s) } } - -#[cfg(test)] -pub(crate) mod tests { - use super::*; - use sp_std::cell::RefCell; - use sp_core::H256; - use sp_runtime::{traits::{BlakeTwo256, IdentityLookup, SignedExtension}, testing::Header, DispatchError}; - use frame_support::{ - impl_outer_origin, parameter_types, assert_ok, assert_noop, - weights::{WithPostDispatchInfo, Pays}, - }; - - impl_outer_origin! { - pub enum Origin for Test where system = super {} - } - - #[derive(Clone, Eq, PartialEq, Debug)] - pub struct Test; - - parameter_types! { - pub const BlockHashCount: u64 = 10; - pub const MaximumBlockWeight: Weight = 1024; - pub const MaximumExtrinsicWeight: Weight = 768; - pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75); - pub const MaximumBlockLength: u32 = 1024; - pub Version: RuntimeVersion = RuntimeVersion { - spec_name: sp_version::create_runtime_str!("test"), - impl_name: sp_version::create_runtime_str!("system-test"), - authoring_version: 1, - spec_version: 1, - impl_version: 1, - apis: sp_version::create_apis_vec!([]), - transaction_version: 1, - }; - pub const BlockExecutionWeight: Weight = 10; - pub const ExtrinsicBaseWeight: Weight = 5; - pub const DbWeight: RuntimeDbWeight = RuntimeDbWeight { - read: 10, - write: 100, - }; - } - - thread_local!{ - pub static KILLED: RefCell> = RefCell::new(vec![]); - } - - pub struct RecordKilled; - impl OnKilledAccount for RecordKilled { - fn on_killed_account(who: &u64) { KILLED.with(|r| r.borrow_mut().push(*who)) } - } - - #[derive(Debug, codec::Encode, codec::Decode)] - pub struct Call; - - impl Dispatchable for Call { - type Origin = Origin; - type Trait = (); - type Info = DispatchInfo; - type PostInfo = PostDispatchInfo; - fn dispatch(self, _origin: Self::Origin) - -> sp_runtime::DispatchResultWithInfo { - panic!("Do not use dummy implementation for dispatch."); - } - } - - impl Trait for Test { - type BaseCallFilter = (); - type Origin = Origin; - type Call = Call; - type Index = u64; - type BlockNumber = u64; - type Hash = H256; - type Hashing = BlakeTwo256; - type AccountId = u64; - type Lookup = IdentityLookup; - type Header = Header; - type Event = Event; - type BlockHashCount = BlockHashCount; - type MaximumBlockWeight = MaximumBlockWeight; - type DbWeight = DbWeight; - type BlockExecutionWeight = BlockExecutionWeight; - type ExtrinsicBaseWeight = ExtrinsicBaseWeight; - type MaximumExtrinsicWeight = MaximumExtrinsicWeight; - type AvailableBlockRatio = AvailableBlockRatio; - type MaximumBlockLength = MaximumBlockLength; - type Version = Version; - type ModuleToIndex = (); - type AccountData = u32; - type OnNewAccount = (); - type OnKilledAccount = RecordKilled; - } - - type System = Module; - type SysEvent = ::Event; - - const CALL: &::Call = &Call; - - fn new_test_ext() -> sp_io::TestExternalities { - let mut ext: sp_io::TestExternalities = GenesisConfig::default().build_storage::().unwrap().into(); - // Add to each test the initial weight of a block - ext.execute_with(|| System::register_extra_weight_unchecked(::BlockExecutionWeight::get(), DispatchClass::Mandatory)); - ext - } - - fn normal_weight_limit() -> Weight { - ::AvailableBlockRatio::get() * ::MaximumBlockWeight::get() - } - - fn normal_length_limit() -> u32 { - ::AvailableBlockRatio::get() * ::MaximumBlockLength::get() - } - - #[test] - fn origin_works() { - let o = Origin::from(RawOrigin::::Signed(1u64)); - let x: Result, Origin> = o.into(); - assert_eq!(x.unwrap(), RawOrigin::::Signed(1u64)); - } - - #[test] - fn stored_map_works() { - new_test_ext().execute_with(|| { - System::insert(&0, 42); - assert!(System::allow_death(&0)); - - System::inc_ref(&0); - assert!(!System::allow_death(&0)); - - System::insert(&0, 69); - assert!(!System::allow_death(&0)); - - System::dec_ref(&0); - assert!(System::allow_death(&0)); - - assert!(KILLED.with(|r| r.borrow().is_empty())); - System::kill_account(&0); - assert_eq!(KILLED.with(|r| r.borrow().clone()), vec![0u64]); - }); - } - - #[test] - fn deposit_event_should_work() { - new_test_ext().execute_with(|| { - System::initialize( - &1, - &[0u8; 32].into(), - &[0u8; 32].into(), - &Default::default(), - InitKind::Full, - ); - System::note_finished_extrinsics(); - System::deposit_event(SysEvent::CodeUpdated); - System::finalize(); - assert_eq!( - System::events(), - vec![ - EventRecord { - phase: Phase::Finalization, - event: SysEvent::CodeUpdated, - topics: vec![], - } - ] - ); - - System::initialize( - &2, - &[0u8; 32].into(), - &[0u8; 32].into(), - &Default::default(), - InitKind::Full, - ); - System::deposit_event(SysEvent::NewAccount(32)); - System::note_finished_initialize(); - System::deposit_event(SysEvent::KilledAccount(42)); - System::note_applied_extrinsic(&Ok(().into()), Default::default()); - System::note_applied_extrinsic( - &Err(DispatchError::BadOrigin.into()), - Default::default() - ); - System::note_finished_extrinsics(); - System::deposit_event(SysEvent::NewAccount(3)); - System::finalize(); - assert_eq!( - System::events(), - vec![ - EventRecord { - phase: Phase::Initialization, - event: SysEvent::NewAccount(32), - topics: vec![], - }, - EventRecord { - phase: Phase::ApplyExtrinsic(0), - event: SysEvent::KilledAccount(42), - topics: vec![] - }, - EventRecord { - phase: Phase::ApplyExtrinsic(0), - event: SysEvent::ExtrinsicSuccess(Default::default()), - topics: vec![] - }, - EventRecord { - phase: Phase::ApplyExtrinsic(1), - event: SysEvent::ExtrinsicFailed( - DispatchError::BadOrigin.into(), - Default::default() - ), - topics: vec![] - }, - EventRecord { - phase: Phase::Finalization, - event: SysEvent::NewAccount(3), - topics: vec![] - }, - ] - ); - }); - } - - #[test] - fn deposit_event_uses_actual_weight() { - new_test_ext().execute_with(|| { - System::initialize( - &1, - &[0u8; 32].into(), - &[0u8; 32].into(), - &Default::default(), - InitKind::Full, - ); - System::note_finished_initialize(); - - let pre_info = DispatchInfo { - weight: 1000, - .. Default::default() - }; - System::note_applied_extrinsic( - &Ok(Some(300).into()), - pre_info, - ); - System::note_applied_extrinsic( - &Ok(Some(1000).into()), - pre_info, - ); - System::note_applied_extrinsic( - // values over the pre info should be capped at pre dispatch value - &Ok(Some(1200).into()), - pre_info, - ); - System::note_applied_extrinsic( - &Err(DispatchError::BadOrigin.with_weight(999)), - pre_info, - ); - - assert_eq!( - System::events(), - vec![ - EventRecord { - phase: Phase::ApplyExtrinsic(0), - event: SysEvent::ExtrinsicSuccess( - DispatchInfo { - weight: 300, - .. Default::default() - }, - ), - topics: vec![] - }, - EventRecord { - phase: Phase::ApplyExtrinsic(1), - event: SysEvent::ExtrinsicSuccess( - DispatchInfo { - weight: 1000, - .. Default::default() - }, - ), - topics: vec![] - }, - EventRecord { - phase: Phase::ApplyExtrinsic(2), - event: SysEvent::ExtrinsicSuccess( - DispatchInfo { - weight: 1000, - .. Default::default() - }, - ), - topics: vec![] - }, - EventRecord { - phase: Phase::ApplyExtrinsic(3), - event: SysEvent::ExtrinsicFailed( - DispatchError::BadOrigin.into(), - DispatchInfo { - weight: 999, - .. Default::default() - }, - ), - topics: vec![] - }, - ] - ); - }); - } - - #[test] - fn deposit_event_topics() { - new_test_ext().execute_with(|| { - const BLOCK_NUMBER: u64 = 1; - - System::initialize( - &BLOCK_NUMBER, - &[0u8; 32].into(), - &[0u8; 32].into(), - &Default::default(), - InitKind::Full, - ); - System::note_finished_extrinsics(); - - let topics = vec![ - H256::repeat_byte(1), - H256::repeat_byte(2), - H256::repeat_byte(3), - ]; - - // We deposit a few events with different sets of topics. - System::deposit_event_indexed(&topics[0..3], SysEvent::NewAccount(1)); - System::deposit_event_indexed(&topics[0..1], SysEvent::NewAccount(2)); - System::deposit_event_indexed(&topics[1..2], SysEvent::NewAccount(3)); - - System::finalize(); - - // Check that topics are reflected in the event record. - assert_eq!( - System::events(), - vec![ - EventRecord { - phase: Phase::Finalization, - event: SysEvent::NewAccount(1), - topics: topics[0..3].to_vec(), - }, - EventRecord { - phase: Phase::Finalization, - event: SysEvent::NewAccount(2), - topics: topics[0..1].to_vec(), - }, - EventRecord { - phase: Phase::Finalization, - event: SysEvent::NewAccount(3), - topics: topics[1..2].to_vec(), - } - ] - ); - - // Check that the topic-events mapping reflects the deposited topics. - // Note that these are indexes of the events. - assert_eq!( - System::event_topics(&topics[0]), - vec![(BLOCK_NUMBER, 0), (BLOCK_NUMBER, 1)], - ); - assert_eq!( - System::event_topics(&topics[1]), - vec![(BLOCK_NUMBER, 0), (BLOCK_NUMBER, 2)], - ); - assert_eq!( - System::event_topics(&topics[2]), - vec![(BLOCK_NUMBER, 0)], - ); - }); - } - - #[test] - fn prunes_block_hash_mappings() { - new_test_ext().execute_with(|| { - // simulate import of 15 blocks - for n in 1..=15 { - System::initialize( - &n, - &[n as u8 - 1; 32].into(), - &[0u8; 32].into(), - &Default::default(), - InitKind::Full, - ); - - System::finalize(); - } - - // first 5 block hashes are pruned - for n in 0..5 { - assert_eq!( - System::block_hash(n), - H256::zero(), - ); - } - - // the remaining 10 are kept - for n in 5..15 { - assert_eq!( - System::block_hash(n), - [n as u8; 32].into(), - ); - } - }) - } - - #[test] - fn signed_ext_check_nonce_works() { - new_test_ext().execute_with(|| { - Account::::insert(1, AccountInfo { nonce: 1, refcount: 0, data: 0 }); - let info = DispatchInfo::default(); - let len = 0_usize; - // stale - assert!(CheckNonce::(0).validate(&1, CALL, &info, len).is_err()); - assert!(CheckNonce::(0).pre_dispatch(&1, CALL, &info, len).is_err()); - // correct - assert!(CheckNonce::(1).validate(&1, CALL, &info, len).is_ok()); - assert!(CheckNonce::(1).pre_dispatch(&1, CALL, &info, len).is_ok()); - // future - assert!(CheckNonce::(5).validate(&1, CALL, &info, len).is_ok()); - assert!(CheckNonce::(5).pre_dispatch(&1, CALL, &info, len).is_err()); - }) - } - - #[test] - fn signed_ext_check_weight_works_normal_tx() { - new_test_ext().execute_with(|| { - let normal_limit = normal_weight_limit(); - let small = DispatchInfo { weight: 100, ..Default::default() }; - let medium = DispatchInfo { - weight: normal_limit - ::ExtrinsicBaseWeight::get(), - ..Default::default() - }; - let big = DispatchInfo { - weight: normal_limit - ::ExtrinsicBaseWeight::get() + 1, - ..Default::default() - }; - let len = 0_usize; - - let reset_check_weight = |i, f, s| { - BlockWeight::mutate(|current_weight| { - current_weight.put(s, DispatchClass::Normal) - }); - let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, i, len); - if f { assert!(r.is_err()) } else { assert!(r.is_ok()) } - }; - - reset_check_weight(&small, false, 0); - reset_check_weight(&medium, false, 0); - reset_check_weight(&big, true, 1); - }) - } - - #[test] - fn signed_ext_check_weight_refund_works() { - new_test_ext().execute_with(|| { - // This is half of the max block weight - let info = DispatchInfo { weight: 512, ..Default::default() }; - let post_info = PostDispatchInfo { actual_weight: Some(128), }; - let len = 0_usize; - - // We allow 75% for normal transaction, so we put 25% - extrinsic base weight - BlockWeight::mutate(|current_weight| { - current_weight.put(256 - ::ExtrinsicBaseWeight::get(), DispatchClass::Normal) - }); - - let pre = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &info, len).unwrap(); - assert_eq!(BlockWeight::get().total(), info.weight + 256); - - assert!( - CheckWeight::::post_dispatch(pre, &info, &post_info, len, &Ok(())) - .is_ok() - ); - assert_eq!( - BlockWeight::get().total(), - post_info.actual_weight.unwrap() + 256, - ); - }) - } - - #[test] - fn signed_ext_check_weight_actual_weight_higher_than_max_is_capped() { - new_test_ext().execute_with(|| { - let info = DispatchInfo { weight: 512, ..Default::default() }; - let post_info = PostDispatchInfo { actual_weight: Some(700), }; - let len = 0_usize; - - BlockWeight::mutate(|current_weight| { - current_weight.put(128, DispatchClass::Normal) - }); - - let pre = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &info, len).unwrap(); - assert_eq!( - BlockWeight::get().total(), - info.weight + 128 + ::ExtrinsicBaseWeight::get(), - ); - - assert!( - CheckWeight::::post_dispatch(pre, &info, &post_info, len, &Ok(())) - .is_ok() - ); - assert_eq!( - BlockWeight::get().total(), - info.weight + 128 + ::ExtrinsicBaseWeight::get(), - ); - }) - } - - #[test] - fn zero_weight_extrinsic_still_has_base_weight() { - new_test_ext().execute_with(|| { - let free = DispatchInfo { weight: 0, ..Default::default() }; - let len = 0_usize; - - // Initial weight from `BlockExecutionWeight` - assert_eq!(System::block_weight().total(), ::BlockExecutionWeight::get()); - let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &free, len); - assert!(r.is_ok()); - assert_eq!( - System::block_weight().total(), - ::ExtrinsicBaseWeight::get() + ::BlockExecutionWeight::get() - ); - }) - } - - #[test] - fn mandatory_extrinsic_doesnt_care_about_limits() { - fn check(call: impl FnOnce(&DispatchInfo, usize)) { - new_test_ext().execute_with(|| { - let max = DispatchInfo { - weight: Weight::max_value(), - class: DispatchClass::Mandatory, - ..Default::default() - }; - let len = 0_usize; - - call(&max, len); - }); - } - - check(|max, len| { - assert_ok!(CheckWeight::::do_pre_dispatch(max, len)); - assert_eq!(System::block_weight().total(), Weight::max_value()); - assert!(System::block_weight().total() > ::MaximumBlockWeight::get()); - }); - check(|max, len| { - assert_ok!(CheckWeight::::do_validate(max, len)); - }); - } - - #[test] - fn normal_extrinsic_limited_by_maximum_extrinsic_weight() { - new_test_ext().execute_with(|| { - let max = DispatchInfo { - weight: MaximumExtrinsicWeight::get() + 1, - class: DispatchClass::Normal, - ..Default::default() - }; - let len = 0_usize; - - assert_noop!( - CheckWeight::::do_validate(&max, len), - InvalidTransaction::ExhaustsResources - ); - }); - } - - #[test] - fn operational_extrinsic_limited_by_operational_space_limit() { - new_test_ext().execute_with(|| { - let operational_limit = CheckWeight::::get_dispatch_limit_ratio( - DispatchClass::Operational - ) * ::MaximumBlockWeight::get(); - let base_weight = ::ExtrinsicBaseWeight::get(); - let block_base = ::BlockExecutionWeight::get(); - - let weight = operational_limit - base_weight - block_base; - let okay = DispatchInfo { - weight, - class: DispatchClass::Operational, - ..Default::default() - }; - let max = DispatchInfo { - weight: weight + 1, - class: DispatchClass::Operational, - ..Default::default() - }; - let len = 0_usize; - - assert_eq!( - CheckWeight::::do_validate(&okay, len), - Ok(ValidTransaction { - priority: CheckWeight::::get_priority(&okay), - ..Default::default() - }) - ); - assert_noop!( - CheckWeight::::do_validate(&max, len), - InvalidTransaction::ExhaustsResources - ); - }); - } - - #[test] - fn register_extra_weight_unchecked_doesnt_care_about_limits() { - new_test_ext().execute_with(|| { - System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Normal); - assert_eq!(System::block_weight().total(), Weight::max_value()); - assert!(System::block_weight().total() > ::MaximumBlockWeight::get()); - }); - } - - #[test] - fn full_block_with_normal_and_operational() { - new_test_ext().execute_with(|| { - // Max block is 1024 - // Max normal is 768 (75%) - // 10 is taken for block execution weight - // So normal extrinsic can be 758 weight (-5 for base extrinsic weight) - // And Operational can be 256 to produce a full block (-5 for base) - let max_normal = DispatchInfo { weight: 753, ..Default::default() }; - let rest_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; - - let len = 0_usize; - - assert_ok!(CheckWeight::::do_pre_dispatch(&max_normal, len)); - assert_eq!(System::block_weight().total(), 768); - assert_ok!(CheckWeight::::do_pre_dispatch(&rest_operational, len)); - assert_eq!(::MaximumBlockWeight::get(), 1024); - assert_eq!(System::block_weight().total(), ::MaximumBlockWeight::get()); - // Checking single extrinsic should not take current block weight into account. - assert_eq!(CheckWeight::::check_extrinsic_weight(&rest_operational), Ok(())); - }); - } - - #[test] - fn dispatch_order_does_not_effect_weight_logic() { - new_test_ext().execute_with(|| { - // We switch the order of `full_block_with_normal_and_operational` - let max_normal = DispatchInfo { weight: 753, ..Default::default() }; - let rest_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; - - let len = 0_usize; - - assert_ok!(CheckWeight::::do_pre_dispatch(&rest_operational, len)); - // Extra 15 here from block execution + base extrinsic weight - assert_eq!(System::block_weight().total(), 266); - assert_ok!(CheckWeight::::do_pre_dispatch(&max_normal, len)); - assert_eq!(::MaximumBlockWeight::get(), 1024); - assert_eq!(System::block_weight().total(), ::MaximumBlockWeight::get()); - }); - } - - #[test] - fn operational_works_on_full_block() { - new_test_ext().execute_with(|| { - // An on_initialize takes up the whole block! (Every time!) - System::register_extra_weight_unchecked(Weight::max_value(), DispatchClass::Mandatory); - let dispatch_normal = DispatchInfo { weight: 251, class: DispatchClass::Normal, ..Default::default() }; - let dispatch_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() }; - let len = 0_usize; - - assert_noop!(CheckWeight::::do_pre_dispatch(&dispatch_normal, len), InvalidTransaction::ExhaustsResources); - // Thank goodness we can still do an operational transaction to possibly save the blockchain. - assert_ok!(CheckWeight::::do_pre_dispatch(&dispatch_operational, len)); - // Not too much though - assert_noop!(CheckWeight::::do_pre_dispatch(&dispatch_operational, len), InvalidTransaction::ExhaustsResources); - // Even with full block, validity of single transaction should be correct. - assert_eq!(CheckWeight::::check_extrinsic_weight(&dispatch_operational), Ok(())); - }); - } - - #[test] - fn signed_ext_check_weight_works_operational_tx() { - new_test_ext().execute_with(|| { - let normal = DispatchInfo { weight: 100, ..Default::default() }; - let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; - let len = 0_usize; - let normal_limit = normal_weight_limit(); - - // given almost full block - BlockWeight::mutate(|current_weight| { - current_weight.put(normal_limit, DispatchClass::Normal) - }); - // will not fit. - assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err()); - // will fit. - assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok()); - - // likewise for length limit. - let len = 100_usize; - AllExtrinsicsLen::put(normal_length_limit()); - assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err()); - assert!(CheckWeight::(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok()); - }) - } - - #[test] - fn signed_ext() { - new_test_ext().execute_with(|| { - let normal = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; - let op = DispatchInfo { weight: 100, class: DispatchClass::Operational, pays_fee: Pays::Yes }; - let len = 0_usize; - - let priority = CheckWeight::(PhantomData) - .validate(&1, CALL, &normal, len) - .unwrap() - .priority; - assert_eq!(priority, 100); - - let priority = CheckWeight::(PhantomData) - .validate(&1, CALL, &op, len) - .unwrap() - .priority; - assert_eq!(priority, u64::max_value() / 2); - }) - } - - #[test] - fn signed_ext_check_weight_block_size_works() { - new_test_ext().execute_with(|| { - let normal = DispatchInfo::default(); - let normal_limit = normal_weight_limit() as usize; - let reset_check_weight = |tx, s, f| { - AllExtrinsicsLen::put(0); - let r = CheckWeight::(PhantomData).pre_dispatch(&1, CALL, tx, s); - if f { assert!(r.is_err()) } else { assert!(r.is_ok()) } - }; - - reset_check_weight(&normal, normal_limit - 1, false); - reset_check_weight(&normal, normal_limit, false); - reset_check_weight(&normal, normal_limit + 1, true); - - // Operational ones don't have this limit. - let op = DispatchInfo { weight: 0, class: DispatchClass::Operational, pays_fee: Pays::Yes }; - reset_check_weight(&op, normal_limit, false); - reset_check_weight(&op, normal_limit + 100, false); - reset_check_weight(&op, 1024, false); - reset_check_weight(&op, 1025, true); - }) - } - - #[test] - fn signed_ext_check_era_should_work() { - new_test_ext().execute_with(|| { - // future - assert_eq!( - CheckEra::::from(Era::mortal(4, 2)).additional_signed().err().unwrap(), - InvalidTransaction::AncientBirthBlock.into(), - ); - - // correct - System::set_block_number(13); - >::insert(12, H256::repeat_byte(1)); - assert!(CheckEra::::from(Era::mortal(4, 12)).additional_signed().is_ok()); - }) - } - - #[test] - fn signed_ext_check_era_should_change_longevity() { - new_test_ext().execute_with(|| { - let normal = DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; - let len = 0_usize; - let ext = ( - CheckWeight::(PhantomData), - CheckEra::::from(Era::mortal(16, 256)), - ); - System::set_block_number(17); - >::insert(16, H256::repeat_byte(1)); - - assert_eq!(ext.validate(&1, CALL, &normal, len).unwrap().longevity, 15); - }) - } - - - #[test] - fn set_code_checks_works() { - struct CallInWasm(Vec); - - impl sp_core::traits::CallInWasm for CallInWasm { - fn call_in_wasm( - &self, - _: &[u8], - _: Option>, - _: &str, - _: &[u8], - _: &mut dyn sp_externalities::Externalities, - _: sp_core::traits::MissingHostFunctions, - ) -> Result, String> { - Ok(self.0.clone()) - } - } - - let test_data = vec![ - ("test", 1, 2, Err(Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 1, Err(Error::::SpecVersionNeedsToIncrease)), - ("test2", 1, 1, Err(Error::::InvalidSpecName)), - ("test", 2, 1, Ok(())), - ("test", 0, 1, Err(Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 0, Err(Error::::SpecVersionNeedsToIncrease)), - ]; - - for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() { - let version = RuntimeVersion { - spec_name: spec_name.into(), - spec_version, - impl_version, - ..Default::default() - }; - let call_in_wasm = CallInWasm(version.encode()); - - let mut ext = new_test_ext(); - ext.register_extension(sp_core::traits::CallInWasmExt::new(call_in_wasm)); - ext.execute_with(|| { - let res = System::set_code( - RawOrigin::Root.into(), - vec![1, 2, 3, 4], - ); - - assert_eq!(expected.map_err(DispatchError::from), res); - }); - } - } - - #[test] - fn set_code_with_real_wasm_blob() { - let executor = substrate_test_runtime_client::new_native_executor(); - let mut ext = new_test_ext(); - ext.register_extension(sp_core::traits::CallInWasmExt::new(executor)); - ext.execute_with(|| { - System::set_block_number(1); - System::set_code( - RawOrigin::Root.into(), - substrate_test_runtime_client::runtime::WASM_BINARY.to_vec(), - ).unwrap(); - - assert_eq!( - System::events(), - vec![EventRecord { - phase: Phase::Initialization, - event: SysEvent::CodeUpdated, - topics: vec![], - }], - ); - }); - } - - #[test] - fn runtime_upgraded_with_set_storage() { - let executor = substrate_test_runtime_client::new_native_executor(); - let mut ext = new_test_ext(); - ext.register_extension(sp_core::traits::CallInWasmExt::new(executor)); - ext.execute_with(|| { - System::set_storage( - RawOrigin::Root.into(), - vec![( - well_known_keys::CODE.to_vec(), - substrate_test_runtime_client::runtime::WASM_BINARY.to_vec() - )], - ).unwrap(); - }); - } - - #[test] - fn events_not_emitted_during_genesis() { - new_test_ext().execute_with(|| { - // Block Number is zero at genesis - assert!(System::block_number().is_zero()); - System::on_created_account(Default::default()); - assert!(System::events().is_empty()); - // Events will be emitted starting on block 1 - System::set_block_number(1); - System::on_created_account(Default::default()); - assert!(System::events().len() == 1); - }); - } - - #[test] - fn ensure_one_of_works() { - fn ensure_root_or_signed(o: RawOrigin) -> Result, Origin> { - EnsureOneOf::, EnsureSigned>::try_origin(o.into()) - } - - assert_eq!(ensure_root_or_signed(RawOrigin::Root).unwrap(), Either::Left(())); - assert_eq!(ensure_root_or_signed(RawOrigin::Signed(0)).unwrap(), Either::Right(0)); - assert!(ensure_root_or_signed(RawOrigin::None).is_err()) - } -} diff --git a/frame/system/src/mock.rs b/frame/system/src/mock.rs new file mode 100644 index 0000000000000..0484b34ba3e3c --- /dev/null +++ b/frame/system/src/mock.rs @@ -0,0 +1,124 @@ +// 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. + +use crate::*; +use sp_std::cell::RefCell; +use sp_core::H256; +use sp_runtime::{ + traits::{BlakeTwo256, IdentityLookup}, + testing::Header, +}; +use frame_support::{ + impl_outer_origin, parameter_types, + weights::PostDispatchInfo, +}; + +impl_outer_origin! { + pub enum Origin for Test where system = super {} +} + +#[derive(Clone, Eq, PartialEq, Debug, Default)] +pub struct Test; + +parameter_types! { + pub const BlockHashCount: u64 = 10; + pub const MaximumBlockWeight: Weight = 1024; + pub const MaximumExtrinsicWeight: Weight = 768; + pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75); + pub const MaximumBlockLength: u32 = 1024; + pub Version: RuntimeVersion = RuntimeVersion { + spec_name: sp_version::create_runtime_str!("test"), + impl_name: sp_version::create_runtime_str!("system-test"), + authoring_version: 1, + spec_version: 1, + impl_version: 1, + apis: sp_version::create_apis_vec!([]), + transaction_version: 1, + }; + pub const BlockExecutionWeight: Weight = 10; + pub const ExtrinsicBaseWeight: Weight = 5; + pub const DbWeight: RuntimeDbWeight = RuntimeDbWeight { + read: 10, + write: 100, + }; +} + +thread_local!{ + pub static KILLED: RefCell> = RefCell::new(vec![]); +} + +pub struct RecordKilled; +impl OnKilledAccount for RecordKilled { + fn on_killed_account(who: &u64) { KILLED.with(|r| r.borrow_mut().push(*who)) } +} + +#[derive(Debug, codec::Encode, codec::Decode)] +pub struct Call; + +impl Dispatchable for Call { + type Origin = Origin; + type Trait = (); + type Info = DispatchInfo; + type PostInfo = PostDispatchInfo; + fn dispatch(self, _origin: Self::Origin) + -> sp_runtime::DispatchResultWithInfo { + panic!("Do not use dummy implementation for dispatch."); + } +} + +impl Trait for Test { + type BaseCallFilter = (); + type Origin = Origin; + type Call = Call; + type Index = u64; + type BlockNumber = u64; + type Hash = H256; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Header = Header; + type Event = Event; + type BlockHashCount = BlockHashCount; + type MaximumBlockWeight = MaximumBlockWeight; + type DbWeight = DbWeight; + type BlockExecutionWeight = BlockExecutionWeight; + type ExtrinsicBaseWeight = ExtrinsicBaseWeight; + type MaximumExtrinsicWeight = MaximumExtrinsicWeight; + type AvailableBlockRatio = AvailableBlockRatio; + type MaximumBlockLength = MaximumBlockLength; + type Version = Version; + type ModuleToIndex = (); + type AccountData = u32; + type OnNewAccount = (); + type OnKilledAccount = RecordKilled; +} + +pub type System = Module; +pub type SysEvent = ::Event; + +pub const CALL: &::Call = &Call; + +/// Create new externalities for `System` module tests. +pub fn new_test_ext() -> sp_io::TestExternalities { + let mut ext: sp_io::TestExternalities = GenesisConfig::default().build_storage::().unwrap().into(); + // Add to each test the initial weight of a block + ext.execute_with(|| System::register_extra_weight_unchecked( + ::BlockExecutionWeight::get(), + DispatchClass::Mandatory + )); + ext +} diff --git a/frame/system/src/offchain.rs b/frame/system/src/offchain.rs index 42699362a36a3..1290ca6378eb8 100644 --- a/frame/system/src/offchain.rs +++ b/frame/system/src/offchain.rs @@ -638,7 +638,7 @@ pub trait SignedPayload: Encode { mod tests { use super::*; use codec::Decode; - use crate::tests::{Test as TestRuntime, Call}; + use crate::mock::{Test as TestRuntime, Call}; use sp_core::offchain::{testing, TransactionPoolExt}; use sp_runtime::testing::{UintAuthorityId, TestSignature, TestXt}; diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs new file mode 100644 index 0000000000000..2f93dc858f10b --- /dev/null +++ b/frame/system/src/tests.rs @@ -0,0 +1,424 @@ +// 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. + +use crate::*; +use mock::{*, Origin}; +use sp_core::H256; +use sp_runtime::DispatchError; +use frame_support::weights::WithPostDispatchInfo; + +#[test] +fn origin_works() { + let o = Origin::from(RawOrigin::::Signed(1u64)); + let x: Result, Origin> = o.into(); + assert_eq!(x.unwrap(), RawOrigin::::Signed(1u64)); +} + +#[test] +fn stored_map_works() { + new_test_ext().execute_with(|| { + System::insert(&0, 42); + assert!(System::allow_death(&0)); + + System::inc_ref(&0); + assert!(!System::allow_death(&0)); + + System::insert(&0, 69); + assert!(!System::allow_death(&0)); + + System::dec_ref(&0); + assert!(System::allow_death(&0)); + + assert!(KILLED.with(|r| r.borrow().is_empty())); + System::kill_account(&0); + assert_eq!(KILLED.with(|r| r.borrow().clone()), vec![0u64]); + }); +} + +#[test] +fn deposit_event_should_work() { + new_test_ext().execute_with(|| { + System::initialize( + &1, + &[0u8; 32].into(), + &[0u8; 32].into(), + &Default::default(), + InitKind::Full, + ); + System::note_finished_extrinsics(); + System::deposit_event(SysEvent::CodeUpdated); + System::finalize(); + assert_eq!( + System::events(), + vec![ + EventRecord { + phase: Phase::Finalization, + event: SysEvent::CodeUpdated, + topics: vec![], + } + ] + ); + + System::initialize( + &2, + &[0u8; 32].into(), + &[0u8; 32].into(), + &Default::default(), + InitKind::Full, + ); + System::deposit_event(SysEvent::NewAccount(32)); + System::note_finished_initialize(); + System::deposit_event(SysEvent::KilledAccount(42)); + System::note_applied_extrinsic(&Ok(().into()), Default::default()); + System::note_applied_extrinsic( + &Err(DispatchError::BadOrigin.into()), + Default::default() + ); + System::note_finished_extrinsics(); + System::deposit_event(SysEvent::NewAccount(3)); + System::finalize(); + assert_eq!( + System::events(), + vec![ + EventRecord { + phase: Phase::Initialization, + event: SysEvent::NewAccount(32), + topics: vec![], + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: SysEvent::KilledAccount(42), + topics: vec![] + }, + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: SysEvent::ExtrinsicSuccess(Default::default()), + topics: vec![] + }, + EventRecord { + phase: Phase::ApplyExtrinsic(1), + event: SysEvent::ExtrinsicFailed( + DispatchError::BadOrigin.into(), + Default::default() + ), + topics: vec![] + }, + EventRecord { + phase: Phase::Finalization, + event: SysEvent::NewAccount(3), + topics: vec![] + }, + ] + ); + }); +} + +#[test] +fn deposit_event_uses_actual_weight() { + new_test_ext().execute_with(|| { + System::initialize( + &1, + &[0u8; 32].into(), + &[0u8; 32].into(), + &Default::default(), + InitKind::Full, + ); + System::note_finished_initialize(); + + let pre_info = DispatchInfo { + weight: 1000, + .. Default::default() + }; + System::note_applied_extrinsic( + &Ok(Some(300).into()), + pre_info, + ); + System::note_applied_extrinsic( + &Ok(Some(1000).into()), + pre_info, + ); + System::note_applied_extrinsic( + // values over the pre info should be capped at pre dispatch value + &Ok(Some(1200).into()), + pre_info, + ); + System::note_applied_extrinsic( + &Err(DispatchError::BadOrigin.with_weight(999)), + pre_info, + ); + + assert_eq!( + System::events(), + vec![ + EventRecord { + phase: Phase::ApplyExtrinsic(0), + event: SysEvent::ExtrinsicSuccess( + DispatchInfo { + weight: 300, + .. Default::default() + }, + ), + topics: vec![] + }, + EventRecord { + phase: Phase::ApplyExtrinsic(1), + event: SysEvent::ExtrinsicSuccess( + DispatchInfo { + weight: 1000, + .. Default::default() + }, + ), + topics: vec![] + }, + EventRecord { + phase: Phase::ApplyExtrinsic(2), + event: SysEvent::ExtrinsicSuccess( + DispatchInfo { + weight: 1000, + .. Default::default() + }, + ), + topics: vec![] + }, + EventRecord { + phase: Phase::ApplyExtrinsic(3), + event: SysEvent::ExtrinsicFailed( + DispatchError::BadOrigin.into(), + DispatchInfo { + weight: 999, + .. Default::default() + }, + ), + topics: vec![] + }, + ] + ); + }); +} + +#[test] +fn deposit_event_topics() { + new_test_ext().execute_with(|| { + const BLOCK_NUMBER: u64 = 1; + + System::initialize( + &BLOCK_NUMBER, + &[0u8; 32].into(), + &[0u8; 32].into(), + &Default::default(), + InitKind::Full, + ); + System::note_finished_extrinsics(); + + let topics = vec![ + H256::repeat_byte(1), + H256::repeat_byte(2), + H256::repeat_byte(3), + ]; + + // We deposit a few events with different sets of topics. + System::deposit_event_indexed(&topics[0..3], SysEvent::NewAccount(1)); + System::deposit_event_indexed(&topics[0..1], SysEvent::NewAccount(2)); + System::deposit_event_indexed(&topics[1..2], SysEvent::NewAccount(3)); + + System::finalize(); + + // Check that topics are reflected in the event record. + assert_eq!( + System::events(), + vec![ + EventRecord { + phase: Phase::Finalization, + event: SysEvent::NewAccount(1), + topics: topics[0..3].to_vec(), + }, + EventRecord { + phase: Phase::Finalization, + event: SysEvent::NewAccount(2), + topics: topics[0..1].to_vec(), + }, + EventRecord { + phase: Phase::Finalization, + event: SysEvent::NewAccount(3), + topics: topics[1..2].to_vec(), + } + ] + ); + + // Check that the topic-events mapping reflects the deposited topics. + // Note that these are indexes of the events. + assert_eq!( + System::event_topics(&topics[0]), + vec![(BLOCK_NUMBER, 0), (BLOCK_NUMBER, 1)], + ); + assert_eq!( + System::event_topics(&topics[1]), + vec![(BLOCK_NUMBER, 0), (BLOCK_NUMBER, 2)], + ); + assert_eq!( + System::event_topics(&topics[2]), + vec![(BLOCK_NUMBER, 0)], + ); + }); +} + +#[test] +fn prunes_block_hash_mappings() { + new_test_ext().execute_with(|| { + // simulate import of 15 blocks + for n in 1..=15 { + System::initialize( + &n, + &[n as u8 - 1; 32].into(), + &[0u8; 32].into(), + &Default::default(), + InitKind::Full, + ); + + System::finalize(); + } + + // first 5 block hashes are pruned + for n in 0..5 { + assert_eq!( + System::block_hash(n), + H256::zero(), + ); + } + + // the remaining 10 are kept + for n in 5..15 { + assert_eq!( + System::block_hash(n), + [n as u8; 32].into(), + ); + } + }) +} + +#[test] +fn set_code_checks_works() { + struct CallInWasm(Vec); + + impl sp_core::traits::CallInWasm for CallInWasm { + fn call_in_wasm( + &self, + _: &[u8], + _: Option>, + _: &str, + _: &[u8], + _: &mut dyn sp_externalities::Externalities, + _: sp_core::traits::MissingHostFunctions, + ) -> Result, String> { + Ok(self.0.clone()) + } + } + + let test_data = vec![ + ("test", 1, 2, Err(Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 1, Err(Error::::SpecVersionNeedsToIncrease)), + ("test2", 1, 1, Err(Error::::InvalidSpecName)), + ("test", 2, 1, Ok(())), + ("test", 0, 1, Err(Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 0, Err(Error::::SpecVersionNeedsToIncrease)), + ]; + + for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() { + let version = RuntimeVersion { + spec_name: spec_name.into(), + spec_version, + impl_version, + ..Default::default() + }; + let call_in_wasm = CallInWasm(version.encode()); + + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::CallInWasmExt::new(call_in_wasm)); + ext.execute_with(|| { + let res = System::set_code( + RawOrigin::Root.into(), + vec![1, 2, 3, 4], + ); + + assert_eq!(expected.map_err(DispatchError::from), res); + }); + } +} + +#[test] +fn set_code_with_real_wasm_blob() { + let executor = substrate_test_runtime_client::new_native_executor(); + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::CallInWasmExt::new(executor)); + ext.execute_with(|| { + System::set_block_number(1); + System::set_code( + RawOrigin::Root.into(), + substrate_test_runtime_client::runtime::WASM_BINARY.to_vec(), + ).unwrap(); + + assert_eq!( + System::events(), + vec![EventRecord { + phase: Phase::Initialization, + event: SysEvent::CodeUpdated, + topics: vec![], + }], + ); + }); +} + +#[test] +fn runtime_upgraded_with_set_storage() { + let executor = substrate_test_runtime_client::new_native_executor(); + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::CallInWasmExt::new(executor)); + ext.execute_with(|| { + System::set_storage( + RawOrigin::Root.into(), + vec![( + well_known_keys::CODE.to_vec(), + substrate_test_runtime_client::runtime::WASM_BINARY.to_vec() + )], + ).unwrap(); + }); +} + +#[test] +fn events_not_emitted_during_genesis() { + new_test_ext().execute_with(|| { + // Block Number is zero at genesis + assert!(System::block_number().is_zero()); + System::on_created_account(Default::default()); + assert!(System::events().is_empty()); + // Events will be emitted starting on block 1 + System::set_block_number(1); + System::on_created_account(Default::default()); + assert!(System::events().len() == 1); + }); +} + +#[test] +fn ensure_one_of_works() { + fn ensure_root_or_signed(o: RawOrigin) -> Result, Origin> { + EnsureOneOf::, EnsureSigned>::try_origin(o.into()) + } + + assert_eq!(ensure_root_or_signed(RawOrigin::Root).unwrap(), Either::Left(())); + assert_eq!(ensure_root_or_signed(RawOrigin::Signed(0)).unwrap(), Either::Right(0)); + assert!(ensure_root_or_signed(RawOrigin::None).is_err()) +} diff --git a/frame/system/src/weights.rs b/frame/system/src/weights.rs new file mode 100644 index 0000000000000..93295093c4fb8 --- /dev/null +++ b/frame/system/src/weights.rs @@ -0,0 +1,76 @@ +// 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. + +use codec::{Encode, Decode}; +use frame_support::weights::{Weight, DispatchClass}; +use sp_runtime::RuntimeDebug; + +/// An object to track the currently used extrinsic weight in a block. +#[derive(Clone, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode)] +pub struct ExtrinsicsWeight { + normal: Weight, + operational: Weight, +} + +impl ExtrinsicsWeight { + /// Returns the total weight consumed by all extrinsics in the block. + pub fn total(&self) -> Weight { + self.normal.saturating_add(self.operational) + } + + /// Add some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`. + pub fn add(&mut self, weight: Weight, class: DispatchClass) { + let value = self.get_mut(class); + *value = value.saturating_add(weight); + } + + /// Try to add some weight of a specific dispatch class, returning Err(()) if overflow would + /// occur. + pub fn checked_add(&mut self, weight: Weight, class: DispatchClass) -> Result<(), ()> { + let value = self.get_mut(class); + *value = value.checked_add(weight).ok_or(())?; + Ok(()) + } + + /// Subtract some weight of a specific dispatch class, saturating at the numeric bounds of + /// `Weight`. + pub fn sub(&mut self, weight: Weight, class: DispatchClass) { + let value = self.get_mut(class); + *value = value.saturating_sub(weight); + } + + /// Get the current weight of a specific dispatch class. + pub fn get(&self, class: DispatchClass) -> Weight { + match class { + DispatchClass::Operational => self.operational, + DispatchClass::Normal | DispatchClass::Mandatory => self.normal, + } + } + + /// Get a mutable reference to the current weight of a specific dispatch class. + fn get_mut(&mut self, class: DispatchClass) -> &mut Weight { + match class { + DispatchClass::Operational => &mut self.operational, + DispatchClass::Normal | DispatchClass::Mandatory => &mut self.normal, + } + } + + /// Set the weight of a specific dispatch class. + pub fn put(&mut self, new: Weight, class: DispatchClass) { + *self.get_mut(class) = new; + } +} From e8378e84ab6355ffe3792b41b9de4481db9353f6 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Wed, 24 Jun 2020 17:24:05 +0200 Subject: [PATCH 5/7] Allow where clause in benchmarking (#6461) * WIP * handle where clause in benchmarking * doc * maybe better syntax * line width --- frame/benchmarking/src/lib.rs | 351 ++++++++++++++++++++------------ frame/benchmarking/src/tests.rs | 45 ++-- 2 files changed, 253 insertions(+), 143 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 47e83cffbcd3e..7cbac3397a978 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -85,6 +85,8 @@ pub use paste; /// Example: /// ```ignore /// benchmarks! { +/// where_clause { where T::A: From } // Optional line to give additional bound on `T`. +/// /// // common parameter; just one for this example. /// // will be `1`, `MAX_LENGTH` or any value inbetween /// _ { @@ -173,6 +175,7 @@ pub use paste; #[macro_export] macro_rules! benchmarks { ( + $( where_clause { where $( $where_ty:ty: $where_bound:path ),* $(,)? } )? _ { $( let $common:ident in $common_from:tt .. $common_to:expr => $common_instancer:expr; @@ -182,6 +185,7 @@ macro_rules! benchmarks { ) => { $crate::benchmarks_iter!( NO_INSTANCE + { $( $( $where_ty: $where_bound ),* )? } { $( { $common , $common_from , $common_to , $common_instancer } )* } ( ) $( $rest )* @@ -189,9 +193,11 @@ macro_rules! benchmarks { } } +/// Same as [`benchmarks`] but for instantiable module. #[macro_export] macro_rules! benchmarks_instance { ( + $( where_clause { where $( $where_ty:ty: $where_bound:path ),* $(,)? } )? _ { $( let $common:ident in $common_from:tt .. $common_to:expr => $common_instancer:expr; @@ -201,6 +207,7 @@ macro_rules! benchmarks_instance { ) => { $crate::benchmarks_iter!( INSTANCE + { $( $( $where_ty: $where_bound ),* )? } { $( { $common , $common_from , $common_to , $common_instancer } )* } ( ) $( $rest )* @@ -209,11 +216,12 @@ macro_rules! benchmarks_instance { } #[macro_export] -#[allow(missing_docs)] +#[doc(hidden)] macro_rules! benchmarks_iter { // mutation arm: ( $instance:ident + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* ) @@ -222,6 +230,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter! { $instance + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: $name ( $origin $( , $arg )* ) @@ -232,6 +241,7 @@ macro_rules! benchmarks_iter { // no instance mutation arm: ( NO_INSTANCE + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) @@ -240,6 +250,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter! { NO_INSTANCE + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: { @@ -254,6 +265,7 @@ macro_rules! benchmarks_iter { // instance mutation arm: ( INSTANCE + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) @@ -262,6 +274,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter! { INSTANCE + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: { @@ -276,6 +289,7 @@ macro_rules! benchmarks_iter { // iteration arm: ( $instance:ident + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $eval:block @@ -285,29 +299,34 @@ macro_rules! benchmarks_iter { $crate::benchmark_backend! { $instance $name + { $( $where_clause )* } { $( $common )* } { } { $eval } { $( $code )* } $postcode } + + #[cfg(test)] + $crate::impl_benchmark_test!( { $( $where_clause )* } $instance $name ); + $crate::benchmarks_iter!( $instance + { $( $where_clause )* } { $( $common )* } ( $( $names )* $name ) $( $rest )* ); }; // iteration-exit arm - ( $instance:ident { $( $common:tt )* } ( $( $names:ident )* ) ) => { - $crate::selected_benchmark!( $instance $( $names ),* ); - $crate::impl_benchmark!( $instance $( $names ),* ); - #[cfg(test)] - $crate::impl_benchmark_tests!( $instance $( $names ),* ); + ( $instance:ident { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) ) => { + $crate::selected_benchmark!( { $( $where_clause)* } $instance $( $names ),* ); + $crate::impl_benchmark!( { $( $where_clause )* } $instance $( $names ),* ); }; // add verify block to _() format ( $instance:ident + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* ) @@ -315,6 +334,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter! { $instance + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: _ ( $origin $( , $arg )* ) @@ -325,6 +345,7 @@ macro_rules! benchmarks_iter { // add verify block to name() format ( $instance:ident + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) @@ -332,6 +353,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter! { $instance + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: $dispatch ( $origin $( , $arg )* ) @@ -342,6 +364,7 @@ macro_rules! benchmarks_iter { // add verify block to {} format ( $instance:ident + { $( $where_clause:tt )* } { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $eval:block @@ -349,6 +372,7 @@ macro_rules! benchmarks_iter { ) => { $crate::benchmarks_iter!( $instance + { $( $where_clause )* } { $( $common )* } ( $( $names )* ) $name { $( $code )* }: $eval @@ -359,10 +383,12 @@ macro_rules! benchmarks_iter { } #[macro_export] -#[allow(missing_docs)] +#[doc(hidden)] macro_rules! benchmark_backend { // parsing arms ($instance:ident $name:ident { + $( $where_clause:tt )* + } { $( $common:tt )* } { $( PRE { $( $pre_parsed:tt )* } )* @@ -371,13 +397,15 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { $( $common )* } { + $instance $name { $( $where_clause )* } { $( $common )* } { $( PRE { $( $pre_parsed )* } )* PRE { $pre_id , $pre_ty , $pre_ex } } { $eval } { $( $rest )* } $postcode } }; ($instance:ident $name:ident { + $( $where_clause:tt )* + } { $( $common:tt )* } { $( $parsed:tt )* @@ -386,7 +414,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { $( $common )* } { + $instance $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* PARAM { $param , $param_from , $param_to , $param_instancer } } { $eval } { $( $rest )* } $postcode @@ -394,6 +422,8 @@ macro_rules! benchmark_backend { }; // 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 )* @@ -402,7 +432,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { + $instance $name { $( $where_clause )* } { $( { $common , $common_from , $common_to , $common_instancer } )* } { $( $parsed )* @@ -417,6 +447,8 @@ macro_rules! benchmark_backend { }; // 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 )* @@ -425,7 +457,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { + $instance $name { $( $where_clause )* } { $( { $common , $common_from , $common_to , $common_instancer } )* } { $( $parsed )* @@ -440,6 +472,8 @@ macro_rules! benchmark_backend { }; // mutation arm to look after a single tt for param_from. ($instance:ident $name:ident { + $( $where_clause:tt )* + } { $( $common:tt )* } { $( $parsed:tt )* @@ -448,7 +482,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { $( $common )* } { $( $parsed )* } { $eval } { + $instance $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { let $param in ( $param_from ) .. $param_to => $param_instancer; $( $rest )* } $postcode @@ -456,6 +490,8 @@ macro_rules! benchmark_backend { }; // mutation arm to look after the default tail of `=> ()` ($instance:ident $name:ident { + $( $where_clause:tt )* + } { $( $common:tt )* } { $( $parsed:tt )* @@ -464,7 +500,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { $( $common )* } { $( $parsed )* } { $eval } { + $instance $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { let $param in $param_from .. $param_to => (); $( $rest )* } $postcode @@ -472,6 +508,8 @@ macro_rules! benchmark_backend { }; // mutation arm to look after `let _ =` ($instance:ident $name:ident { + $( $where_clause:tt )* + } { $( $common:tt )* } { $( $parsed:tt )* @@ -480,7 +518,7 @@ macro_rules! benchmark_backend { $( $rest:tt )* } $postcode:block) => { $crate::benchmark_backend! { - $instance $name { $( $common )* } { $( $parsed )* } { $eval } { + $instance $name { $( $where_clause )* } { $( $common )* } { $( $parsed )* } { $eval } { let $pre_id : _ = $pre_ex; $( $rest )* } $postcode @@ -488,6 +526,8 @@ macro_rules! benchmark_backend { }; // no instance actioning arm (NO_INSTANCE $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 } )* @@ -496,7 +536,9 @@ macro_rules! benchmark_backend { #[allow(non_camel_case_types)] struct $name; #[allow(unused_variables)] - impl $crate::BenchmarkingSetup for $name { + impl $crate::BenchmarkingSetup for $name + where $( $where_clause )* + { fn components(&self) -> Vec<($crate::BenchmarkParameter, u32, u32)> { vec! [ $( @@ -513,7 +555,9 @@ macro_rules! benchmark_backend { )* $( // Prepare instance - let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + let $param = components.iter() + .find(|&c| c.0 == $crate::BenchmarkParameter::$param) + .unwrap().1; )* $( let $pre_id : $pre_ty = $pre_ex; @@ -532,7 +576,9 @@ macro_rules! benchmark_backend { )* $( // Prepare instance - let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + let $param = components.iter() + .find(|&c| c.0 == $crate::BenchmarkParameter::$param) + .unwrap().1; )* $( let $pre_id : $pre_ty = $pre_ex; @@ -546,6 +592,8 @@ macro_rules! benchmark_backend { }; // instance actioning arm (INSTANCE $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 } )* @@ -554,7 +602,9 @@ macro_rules! benchmark_backend { #[allow(non_camel_case_types)] struct $name; #[allow(unused_variables)] - impl, I: Instance> $crate::BenchmarkingSetupInstance for $name { + impl, I: Instance> $crate::BenchmarkingSetupInstance for $name + where $( $where_clause )* + { fn components(&self) -> Vec<($crate::BenchmarkParameter, u32, u32)> { vec! [ $( @@ -571,7 +621,9 @@ macro_rules! benchmark_backend { )* $( // Prepare instance - let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + let $param = components.iter() + .find(|&c| c.0 == $crate::BenchmarkParameter::$param) + .unwrap().1; )* $( let $pre_id : $pre_ty = $pre_ex; @@ -590,7 +642,9 @@ macro_rules! benchmark_backend { )* $( // Prepare instance - let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + let $param = components.iter() + .find(|&c| c.0 == $crate::BenchmarkParameter::$param) + .unwrap().1; )* $( let $pre_id : $pre_ty = $pre_ex; @@ -604,23 +658,25 @@ macro_rules! benchmark_backend { } } -/// Creates a `SelectedBenchmark` enum implementing `BenchmarkingSetup`. -/// -/// Every variant must implement [`BenchmarkingSetup`]. -/// -/// ```nocompile -/// -/// struct Transfer; -/// impl BenchmarkingSetup for Transfer { ... } -/// -/// struct SetBalance; -/// impl BenchmarkingSetup for SetBalance { ... } -/// -/// selected_benchmark!(Transfer, SetBalance); -/// ``` +// Creates a `SelectedBenchmark` enum implementing `BenchmarkingSetup`. +// +// Every variant must implement [`BenchmarkingSetup`]. +// +// ```nocompile +// +// struct Transfer; +// impl BenchmarkingSetup for Transfer { ... } +// +// struct SetBalance; +// impl BenchmarkingSetup for SetBalance { ... } +// +// selected_benchmark!(Transfer, SetBalance); +// ``` #[macro_export] +#[doc(hidden)] macro_rules! selected_benchmark { ( + { $( $where_clause:tt )* } NO_INSTANCE $( $bench:ident ),* ) => { // The list of available benchmarks for this pallet. @@ -630,7 +686,9 @@ macro_rules! selected_benchmark { } // Allow us to select a benchmark from the list of available benchmarks. - impl $crate::BenchmarkingSetup for SelectedBenchmark { + impl $crate::BenchmarkingSetup for SelectedBenchmark + where $( $where_clause )* + { fn components(&self) -> Vec<($crate::BenchmarkParameter, u32, u32)> { match self { $( Self::$bench => <$bench as $crate::BenchmarkingSetup>::components(&$bench), )* @@ -655,6 +713,7 @@ macro_rules! selected_benchmark { } }; ( + { $( $where_clause:tt )* } INSTANCE $( $bench:ident ),* ) => { // The list of available benchmarks for this pallet. @@ -664,7 +723,9 @@ macro_rules! selected_benchmark { } // Allow us to select a benchmark from the list of available benchmarks. - impl, I: Instance> $crate::BenchmarkingSetupInstance for SelectedBenchmark { + impl, I: Instance> $crate::BenchmarkingSetupInstance for SelectedBenchmark + where $( $where_clause )* + { fn components(&self) -> Vec<($crate::BenchmarkParameter, u32, u32)> { match self { $( Self::$bench => <$bench as $crate::BenchmarkingSetupInstance>::components(&$bench), )* @@ -691,12 +752,14 @@ macro_rules! selected_benchmark { } #[macro_export] +#[doc(hidden)] macro_rules! impl_benchmark { ( + { $( $where_clause:tt )* } NO_INSTANCE $( $name:ident ),* ) => { impl $crate::Benchmarking<$crate::BenchmarkResults> for Module - where T: frame_system::Trait + where T: frame_system::Trait, $( $where_clause )* { fn benchmarks() -> Vec<&'static [u8]> { vec![ $( stringify!($name).as_ref() ),* ] @@ -763,8 +826,11 @@ macro_rules! impl_benchmark { // Run the benchmark `repeat` times. for _ in 0..repeat { - // Set up the externalities environment for the setup we want to benchmark. - let closure_to_benchmark = >::instance(&selected_benchmark, &c)?; + // Set up the externalities environment for the setup we want to + // benchmark. + let closure_to_benchmark = < + SelectedBenchmark as $crate::BenchmarkingSetup + >::instance(&selected_benchmark, &c)?; // Set the block number to at least 1 so events are deposited. if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { @@ -776,12 +842,20 @@ macro_rules! impl_benchmark { $crate::benchmarking::commit_db(); // Time the extrinsic logic. - frame_support::debug::trace!(target: "benchmark", "Start Benchmark: {:?} {:?}", name, component_value); + frame_support::debug::trace!( + target: "benchmark", + "Start Benchmark: {:?} {:?}", name, component_value + ); + let start_extrinsic = $crate::benchmarking::current_time(); closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; - frame_support::debug::trace!(target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic); + + frame_support::debug::trace!( + target: "benchmark", + "End Benchmark: {} ns", elapsed_extrinsic + ); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -801,10 +875,12 @@ macro_rules! impl_benchmark { } }; ( + { $( $where_clause:tt )* } INSTANCE $( $name:ident ),* ) => { - impl, I: Instance> $crate::Benchmarking<$crate::BenchmarkResults> for Module - where T: frame_system::Trait + impl, I: Instance> $crate::Benchmarking<$crate::BenchmarkResults> + for Module + where T: frame_system::Trait, $( $where_clause )* { fn benchmarks() -> Vec<&'static [u8]> { vec![ $( stringify!($name).as_ref() ),* ] @@ -829,7 +905,9 @@ macro_rules! impl_benchmark { $crate::benchmarking::commit_db(); $crate::benchmarking::wipe_db(); - let components = >::components(&selected_benchmark); + let components = < + SelectedBenchmark as $crate::BenchmarkingSetupInstance + >::components(&selected_benchmark); let mut results: Vec<$crate::BenchmarkResults> = Vec::new(); // Default number of steps for a component. @@ -872,7 +950,9 @@ macro_rules! impl_benchmark { // Run the benchmark `repeat` times. for _ in 0..repeat { // Set up the externalities environment for the setup we want to benchmark. - let closure_to_benchmark = >::instance(&selected_benchmark, &c)?; + let closure_to_benchmark = < + SelectedBenchmark as $crate::BenchmarkingSetupInstance + >::instance(&selected_benchmark, &c)?; // Set the block number to at least 1 so events are deposited. if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { @@ -884,12 +964,20 @@ macro_rules! impl_benchmark { $crate::benchmarking::commit_db(); // Time the extrinsic logic. - frame_support::debug::trace!(target: "benchmark", "Start Benchmark: {:?} {:?}", name, component_value); + frame_support::debug::trace!( + target: "benchmark", + "Start Benchmark: {:?} {:?}", name, component_value + ); + let start_extrinsic = $crate::benchmarking::current_time(); closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; - frame_support::debug::trace!(target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic); + + frame_support::debug::trace!( + target: "benchmark", + "End Benchmark: {} ns", elapsed_extrinsic + ); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -910,108 +998,115 @@ macro_rules! impl_benchmark { } } -// This creates unit tests from the main benchmark macro. -// They run the benchmark using the `high` and `low` value for each component +// This creates a unit test for one benchmark of the main benchmark macro. +// It runs the benchmark using the `high` and `low` value for each component // and ensure that everything completes successfully. #[macro_export] -macro_rules! impl_benchmark_tests { +#[doc(hidden)] +macro_rules! impl_benchmark_test { ( + { $( $where_clause:tt )* } NO_INSTANCE - $( $name:ident ),* + $name:ident ) => { - $( - $crate::paste::item! { - fn [] () -> Result<(), &'static str> - where T: frame_system::Trait - { - let selected_benchmark = SelectedBenchmark::$name; - let components = >::components(&selected_benchmark); - - assert!( - components.len() != 0, - "You need to add components to your benchmark!", - ); - for (_, (name, low, high)) in components.iter().enumerate() { - // Test only the low and high value, assuming values in the middle won't break - for component_value in vec![low, high] { - // Select the max value for all the other components. - let c: Vec<($crate::BenchmarkParameter, u32)> = components.iter() - .enumerate() - .map(|(_, (n, _, h))| - if n == name { - (*n, *component_value) - } else { - (*n, *h) - } - ) - .collect(); - - // Set up the verification state - let closure_to_verify = >::verify(&selected_benchmark, &c)?; - - // Set the block number to at least 1 so events are deposited. - if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { - frame_system::Module::::set_block_number(1.into()); - } + $crate::paste::item! { + fn [] () -> Result<(), &'static str> + where T: frame_system::Trait, $( $where_clause )* + { + let selected_benchmark = SelectedBenchmark::$name; + let components = < + SelectedBenchmark as $crate::BenchmarkingSetup + >::components(&selected_benchmark); + + assert!( + components.len() != 0, + "You need to add components to your benchmark!", + ); + for (_, (name, low, high)) in components.iter().enumerate() { + // Test only the low and high value, assuming values in the middle won't break + for component_value in vec![low, high] { + // Select the max value for all the other components. + let c: Vec<($crate::BenchmarkParameter, u32)> = components.iter() + .enumerate() + .map(|(_, (n, _, h))| + if n == name { + (*n, *component_value) + } else { + (*n, *h) + } + ) + .collect(); - // Run verification - closure_to_verify()?; + // Set up the verification state + let closure_to_verify = < + SelectedBenchmark as $crate::BenchmarkingSetup + >::verify(&selected_benchmark, &c)?; - // Reset the state - $crate::benchmarking::wipe_db(); + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); } + + // Run verification + closure_to_verify()?; + + // Reset the state + $crate::benchmarking::wipe_db(); } - Ok(()) } + Ok(()) } - )* + } }; ( + { $( $where_clause:tt )* } INSTANCE - $( $name:ident ),* + $name:ident ) => { - $( - $crate::paste::item! { - fn [] () -> Result<(), &'static str> - where T: frame_system::Trait - { - let selected_benchmark = SelectedBenchmark::$name; - let components = >::components(&selected_benchmark); - - for (_, (name, low, high)) in components.iter().enumerate() { - // Test only the low and high value, assuming values in the middle won't break - for component_value in vec![low, high] { - // Select the max value for all the other components. - let c: Vec<($crate::BenchmarkParameter, u32)> = components.iter() - .enumerate() - .map(|(_, (n, _, h))| - if n == name { - (*n, *component_value) - } else { - (*n, *h) - } - ) - .collect(); - - // Set up the verification state - let closure_to_verify = >::verify(&selected_benchmark, &c)?; - - // Set the block number to at least 1 so events are deposited. - if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { - frame_system::Module::::set_block_number(1.into()); - } + $crate::paste::item! { + fn [] () -> Result<(), &'static str> + where T: frame_system::Trait, $( $where_clause )* + { + let selected_benchmark = SelectedBenchmark::$name; + let components = < + SelectedBenchmark as $crate::BenchmarkingSetupInstance + >::components(&selected_benchmark); + + for (_, (name, low, high)) in components.iter().enumerate() { + // Test only the low and high value, assuming values in the middle won't break + for component_value in vec![low, high] { + // Select the max value for all the other components. + let c: Vec<($crate::BenchmarkParameter, u32)> = components.iter() + .enumerate() + .map(|(_, (n, _, h))| + if n == name { + (*n, *component_value) + } else { + (*n, *h) + } + ) + .collect(); - // Run verification - closure_to_verify()?; + // Set up the verification state + let closure_to_verify = < + SelectedBenchmark as $crate::BenchmarkingSetupInstance + >::verify(&selected_benchmark, &c)?; - // Reset the state - $crate::benchmarking::wipe_db(); + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); } + + // Run verification + closure_to_verify()?; + + // Reset the state + $crate::benchmarking::wipe_db(); } - Ok(()) } + Ok(()) } - )* + } }; } diff --git a/frame/benchmarking/src/tests.rs b/frame/benchmarking/src/tests.rs index 85e8bf5a5c1db..674d92eb856aa 100644 --- a/frame/benchmarking/src/tests.rs +++ b/frame/benchmarking/src/tests.rs @@ -30,13 +30,17 @@ use frame_support::{ use frame_system::{RawOrigin, ensure_signed, ensure_none}; decl_storage! { - trait Store for Module as Test { + trait Store for Module as Test where + ::OtherEvent: Into<::Event> + { Value get(fn value): Option; } } decl_module! { - pub struct Module for enum Call where origin: T::Origin { + pub struct Module for enum Call where + origin: T::Origin, ::OtherEvent: Into<::Event> + { #[weight = 0] fn set_value(origin, n: u32) -> DispatchResult { let _sender = ensure_signed(origin)?; @@ -56,11 +60,16 @@ impl_outer_origin! { pub enum Origin for Test where system = frame_system {} } -pub trait Trait { +pub trait OtherTrait { + type OtherEvent; +} + +pub trait Trait: OtherTrait where Self::OtherEvent: Into { type Event; type BlockNumber; type AccountId: 'static + Default + Decode; - type Origin: From> + Into, Self::Origin>>; + type Origin: From> + + Into, Self::Origin>>; } #[derive(Clone, Eq, PartialEq)] @@ -100,6 +109,10 @@ impl Trait for Test { type AccountId = u64; } +impl OtherTrait for Test { + type OtherEvent = (); +} + // This function basically just builds a genesis storage key/value store according to // our desired mockup. fn new_test_ext() -> sp_io::TestExternalities { @@ -107,6 +120,8 @@ fn new_test_ext() -> sp_io::TestExternalities { } benchmarks!{ + where_clause { where ::OtherEvent: Into<::Event> } + _ { // Define a common range for `b`. let b in 1 .. 1000 => (); @@ -156,13 +171,13 @@ benchmarks!{ #[test] fn benchmarks_macro_works() { // Check benchmark creation for `set_value`. - let selected_benchmark = SelectedBenchmark::set_value; + let selected = SelectedBenchmark::set_value; - let components = >::components(&selected_benchmark); + let components = >::components(&selected); assert_eq!(components, vec![(BenchmarkParameter::b, 1, 1000)]); let closure = >::instance( - &selected_benchmark, + &selected, &[(BenchmarkParameter::b, 1)], ).expect("failed to create closure"); @@ -174,12 +189,12 @@ fn benchmarks_macro_works() { #[test] fn benchmarks_macro_rename_works() { // Check benchmark creation for `other_dummy`. - let selected_benchmark = SelectedBenchmark::other_name; - let components = >::components(&selected_benchmark); + let selected = SelectedBenchmark::other_name; + let components = >::components(&selected); assert_eq!(components, vec![(BenchmarkParameter::b, 1, 1000)]); let closure = >::instance( - &selected_benchmark, + &selected, &[(BenchmarkParameter::b, 1)], ).expect("failed to create closure"); @@ -190,13 +205,13 @@ fn benchmarks_macro_rename_works() { #[test] fn benchmarks_macro_works_for_non_dispatchable() { - let selected_benchmark = SelectedBenchmark::sort_vector; + let selected = SelectedBenchmark::sort_vector; - let components = >::components(&selected_benchmark); + let components = >::components(&selected); assert_eq!(components, vec![(BenchmarkParameter::x, 1, 10000)]); let closure = >::instance( - &selected_benchmark, + &selected, &[(BenchmarkParameter::x, 1)], ).expect("failed to create closure"); @@ -206,10 +221,10 @@ fn benchmarks_macro_works_for_non_dispatchable() { #[test] fn benchmarks_macro_verify_works() { // Check postcondition for benchmark `set_value` is valid. - let selected_benchmark = SelectedBenchmark::set_value; + let selected = SelectedBenchmark::set_value; let closure = >::verify( - &selected_benchmark, + &selected, &[(BenchmarkParameter::b, 1)], ).expect("failed to create closure"); From 5a85a43104f1c1b934ec43835492b2d36e84b18b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Wed, 24 Jun 2020 16:42:27 +0100 Subject: [PATCH 6/7] client: fix print of slot duration on startup (#6495) --- client/consensus/slots/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index 950f83fbced18..7687d3114b31d 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -471,7 +471,7 @@ impl SlotDuration { info!( "⏱ Loaded block-time = {:?} milliseconds from genesis on first-launch", - genesis_slot_duration + genesis_slot_duration.slot_duration() ); genesis_slot_duration From 7f5dd736f42a408b62885669f7d76ef5baa13572 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 24 Jun 2020 21:03:55 +0200 Subject: [PATCH 7/7] Add DB Read/Write Tracking to Benchmarking Pipeline (#6386) * initial mockup * add and wipe * track writes * start to add to pipeline * return all reads/writes * Log reads and writes from bench db * causes panic * Allow multiple commits * commit before ending benchmark * doesn't work??? * fix * Update lib.rs * switch to struct for `BenchmarkResults` * add to output * fix test * line width * @kianenigma review * Add Whitelist to DB Tracking in Benchmarks Pipeline (#6405) * hardcoded whitelist * Add whitelist to pipeline * Remove whitelist pipeline from CLI, add to runtime * clean-up unused db initialized whitelist * Add regression analysis to DB Tracking (#6475) * Add selector * add tests * debug formatter for easy formula * Update client/db/src/bench.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: arkpar Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- Cargo.lock | 1 + bin/node/runtime/Cargo.toml | 1 + bin/node/runtime/src/lib.rs | 20 ++- client/db/src/bench.rs | 169 +++++++++++++++++++- frame/benchmarking/src/analysis.rs | 140 ++++++++++++---- frame/benchmarking/src/lib.rs | 56 ++++++- frame/benchmarking/src/utils.rs | 26 ++- primitives/externalities/src/lib.rs | 21 +++ primitives/runtime-interface/src/impls.rs | 4 + primitives/state-machine/src/backend.rs | 17 +- primitives/state-machine/src/basic.rs | 12 ++ primitives/state-machine/src/ext.rs | 13 ++ primitives/state-machine/src/read_only.rs | 12 ++ utils/frame/benchmarking-cli/src/command.rs | 39 +++-- 14 files changed, 471 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c1ea4a479c0ef..1520373790495 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3518,6 +3518,7 @@ dependencies = [ "frame-system", "frame-system-benchmarking", "frame-system-rpc-runtime-api", + "hex-literal", "integer-sqrt", "node-primitives", "pallet-authority-discovery", diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index b26b53cd6c589..3614e4ca0dc8a 100644 --- a/bin/node/runtime/Cargo.toml +++ b/bin/node/runtime/Cargo.toml @@ -18,6 +18,7 @@ codec = { package = "parity-scale-codec", version = "1.3.1", default-features = integer-sqrt = { version = "0.1.2" } serde = { version = "1.0.102", optional = true } static_assertions = "1.1.0" +hex-literal = "0.2.1" # primitives sp-authority-discovery = { version = "2.0.0-rc3", default-features = false, path = "../../../primitives/authority-discovery" } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 90bb63874e116..8b6831b41ebf9 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1075,8 +1075,26 @@ impl_runtime_apis! { impl pallet_offences_benchmarking::Trait for Runtime {} impl frame_system_benchmarking::Trait for Runtime {} + let whitelist: Vec> = vec![ + // Block Number + // frame_system::Number::::hashed_key().to_vec(), + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec(), + // Total Issuance + hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec(), + // Execution Phase + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec(), + // Event Count + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec(), + // System Events + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec(), + // Caller 0 Account + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da946c154ffd9992e395af90b5b13cc6f295c77033fce8a9045824a6690bbf99c6db269502f0a8d1d2a008542d5690a0749").to_vec(), + // Treasury Account + hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7b99d880ec681799c0cf30e8886371da95ecffd7b6c0f78751baa9d281e0bfa3a6d6f646c70792f74727372790000000000000000000000000000000000000000").to_vec(), + ]; + let mut batches = Vec::::new(); - let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat); + let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist); add_benchmark!(params, batches, b"balances", Balances); add_benchmark!(params, batches, b"collective", Council); diff --git a/client/db/src/bench.rs b/client/db/src/bench.rs index 99ce1edae00c5..c3bed3e24f617 100644 --- a/client/db/src/bench.rs +++ b/client/db/src/bench.rs @@ -24,10 +24,10 @@ use std::collections::HashMap; use hash_db::{Prefix, Hasher}; use sp_trie::{MemoryDB, prefixed_key}; -use sp_core::storage::ChildInfo; +use sp_core::{storage::ChildInfo, hexdisplay::HexDisplay}; use sp_runtime::traits::{Block as BlockT, HashFor}; use sp_runtime::Storage; -use sp_state_machine::{DBValue, backend::Backend as StateBackend}; +use sp_state_machine::{DBValue, backend::Backend as StateBackend, StorageCollection}; use kvdb::{KeyValueDB, DBTransaction}; use crate::storage_cache::{CachingState, SharedCache, new_shared_cache}; @@ -50,6 +50,40 @@ impl sp_state_machine::Storage> for StorageDb { root: Cell, @@ -59,6 +93,9 @@ pub struct BenchmarkingState { genesis: HashMap, (Vec, i32)>, record: Cell>>, shared_cache: SharedCache, // shared cache is always empty + key_tracker: RefCell, KeyTracker>>, + read_write_tracker: RefCell, + whitelist: RefCell>>, } impl BenchmarkingState { @@ -76,8 +113,13 @@ impl BenchmarkingState { genesis_root: Default::default(), record: Default::default(), shared_cache: new_shared_cache(0, (1, 10)), + key_tracker: Default::default(), + read_write_tracker: Default::default(), + whitelist: Default::default(), }; + state.add_whitelist_to_tracker(); + state.reopen()?; let child_delta = genesis.children_default.iter().map(|(_storage_key, child_content)| ( &child_content.child_info, @@ -89,7 +131,7 @@ impl BenchmarkingState { ); state.genesis = transaction.clone().drain(); state.genesis_root = root.clone(); - state.commit(root, transaction)?; + state.commit(root, transaction, Vec::new())?; state.record.take(); Ok(state) } @@ -109,6 +151,86 @@ impl BenchmarkingState { )); Ok(()) } + + 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); + }); + } + + fn wipe_tracker(&self) { + *self.key_tracker.borrow_mut() = HashMap::new(); + self.add_whitelist_to_tracker(); + *self.read_write_tracker.borrow_mut() = Default::default(); + } + + fn add_read_key(&self, key: &[u8]) { + log::trace!(target: "benchmark", "Read: {}", HexDisplay::from(&key)); + + let mut key_tracker = self.key_tracker.borrow_mut(); + let mut read_write_tracker = self.read_write_tracker.borrow_mut(); + + let maybe_tracker = key_tracker.get(key); + + let has_been_read = KeyTracker { + has_been_read: true, + has_been_written: false, + }; + + match maybe_tracker { + None => { + key_tracker.insert(key.to_vec(), has_been_read); + read_write_tracker.add_read(); + }, + Some(tracker) => { + if !tracker.has_been_read { + key_tracker.insert(key.to_vec(), has_been_read); + read_write_tracker.add_read(); + } else { + read_write_tracker.add_repeat_read(); + } + } + } + } + + fn add_write_key(&self, key: &[u8]) { + log::trace!(target: "benchmark", "Write: {}", HexDisplay::from(&key)); + + let mut key_tracker = self.key_tracker.borrow_mut(); + let mut read_write_tracker = self.read_write_tracker.borrow_mut(); + + let maybe_tracker = key_tracker.get(key); + + // If we have written to the key, we also consider that we have read from it. + let has_been_written = KeyTracker { + has_been_read: true, + has_been_written: true, + }; + + match maybe_tracker { + None => { + key_tracker.insert(key.to_vec(), has_been_written); + read_write_tracker.add_write(); + }, + Some(tracker) => { + if !tracker.has_been_written { + key_tracker.insert(key.to_vec(), has_been_written); + read_write_tracker.add_write(); + } else { + read_write_tracker.add_repeat_write(); + } + } + } + } } fn state_err() -> String { @@ -121,10 +243,12 @@ impl StateBackend> for BenchmarkingState { type TrieBackendStorage = as StateBackend>>::TrieBackendStorage; fn storage(&self, key: &[u8]) -> Result>, Self::Error> { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.storage(key) } fn storage_hash(&self, key: &[u8]) -> Result, Self::Error> { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.storage_hash(key) } @@ -133,10 +257,12 @@ impl StateBackend> for BenchmarkingState { child_info: &ChildInfo, key: &[u8], ) -> Result>, Self::Error> { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.child_storage(child_info, key) } fn exists_storage(&self, key: &[u8]) -> Result { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.exists_storage(key) } @@ -145,10 +271,12 @@ impl StateBackend> for BenchmarkingState { child_info: &ChildInfo, key: &[u8], ) -> Result { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.exists_child_storage(child_info, key) } fn next_storage_key(&self, key: &[u8]) -> Result>, Self::Error> { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.next_storage_key(key) } @@ -157,6 +285,7 @@ impl StateBackend> for BenchmarkingState { child_info: &ChildInfo, key: &[u8], ) -> Result>, Self::Error> { + self.add_read_key(key); self.state.borrow().as_ref().ok_or_else(state_err)?.next_child_storage_key(child_info, key) } @@ -230,8 +359,11 @@ impl StateBackend> for BenchmarkingState { None } - fn commit(&self, storage_root: as Hasher>::Out, mut transaction: Self::Transaction) - -> Result<(), Self::Error> + fn commit(&self, + storage_root: as Hasher>::Out, + mut transaction: Self::Transaction, + storage_changes: StorageCollection, + ) -> Result<(), Self::Error> { if let Some(db) = self.db.take() { let mut db_transaction = DBTransaction::new(); @@ -245,10 +377,17 @@ impl StateBackend> for BenchmarkingState { } keys.push(key); } - self.record.set(keys); + let mut record = self.record.take(); + record.extend(keys); + self.record.set(record); db.write(db_transaction).map_err(|_| String::from("Error committing transaction"))?; self.root.set(storage_root); - self.db.set(Some(db)) + self.db.set(Some(db)); + + // Track DB Writes + storage_changes.iter().for_each(|(key, _)| { + self.add_write_key(key); + }); } else { return Err("Trying to commit to a closed db".into()) } @@ -272,9 +411,25 @@ impl StateBackend> for BenchmarkingState { self.root.set(self.genesis_root.clone()); self.reopen()?; + self.wipe_tracker(); Ok(()) } + /// Get the key tracking information for the state db. + fn read_write_count(&self) -> (u32, u32, u32, u32) { + let count = *self.read_write_tracker.borrow_mut(); + (count.reads, count.repeat_reads, count.writes, count.repeat_writes) + } + + /// Reset the key tracking information for the state db. + fn reset_read_write_count(&self) { + self.wipe_tracker() + } + + fn set_whitelist(&self, new: Vec>) { + *self.whitelist.borrow_mut() = new; + } + fn register_overlay_stats(&mut self, stats: &sp_state_machine::StateMachineStats) { self.state.borrow_mut().as_mut().map(|s| s.register_overlay_stats(stats)); } diff --git a/frame/benchmarking/src/analysis.rs b/frame/benchmarking/src/analysis.rs index 0446430975550..621f3a2941ff5 100644 --- a/frame/benchmarking/src/analysis.rs +++ b/frame/benchmarking/src/analysis.rs @@ -29,24 +29,40 @@ pub struct Analysis { model: Option, } +pub enum BenchmarkSelector { + ExtrinsicTime, + StorageRootTime, + Reads, + Writes, +} + impl Analysis { - pub fn median_slopes(r: &Vec) -> Option { - let results = r[0].0.iter().enumerate().map(|(i, &(param, _))| { + pub fn median_slopes(r: &Vec, selector: BenchmarkSelector) -> Option { + let results = r[0].components.iter().enumerate().map(|(i, &(param, _))| { let mut counted = BTreeMap::, usize>::new(); - for (params, _, _) in r.iter() { - let mut p = params.iter().map(|x| x.1).collect::>(); + for result in r.iter() { + let mut p = result.components.iter().map(|x| x.1).collect::>(); p[i] = 0; *counted.entry(p).or_default() += 1; } let others: Vec = counted.iter().max_by_key(|i| i.1).expect("r is not empty; qed").0.clone(); let values = r.iter() .filter(|v| - v.0.iter() + v.components.iter() .map(|x| x.1) .zip(others.iter()) .enumerate() .all(|(j, (v1, v2))| j == i || v1 == *v2) - ).map(|(ps, v, _)| (ps[i].1, *v)) + ).map(|result| { + // Extract the data we are interested in analyzing + let data = match selector { + BenchmarkSelector::ExtrinsicTime => result.extrinsic_time, + BenchmarkSelector::StorageRootTime => result.storage_root_time, + BenchmarkSelector::Reads => result.reads.into(), + BenchmarkSelector::Writes => result.writes.into(), + }; + (result.components[i].1, data) + }) .collect::>(); (format!("{:?}", param), i, others, values) }).collect::>(); @@ -97,12 +113,18 @@ impl Analysis { }) } - pub fn min_squares_iqr(r: &Vec) -> Option { + pub fn min_squares_iqr(r: &Vec, selector: BenchmarkSelector) -> Option { let mut results = BTreeMap::, Vec>::new(); - for &(ref params, t, _) in r.iter() { - let p = params.iter().map(|x| x.1).collect::>(); - results.entry(p).or_default().push(t); + for result in r.iter() { + let p = result.components.iter().map(|x| x.1).collect::>(); + results.entry(p).or_default().push(match selector { + BenchmarkSelector::ExtrinsicTime => result.extrinsic_time, + BenchmarkSelector::StorageRootTime => result.storage_root_time, + BenchmarkSelector::Reads => result.reads.into(), + BenchmarkSelector::Writes => result.writes.into(), + }) } + for (_, rs) in results.iter_mut() { rs.sort(); let ql = rs.len() / 4; @@ -111,7 +133,7 @@ impl Analysis { let mut data = vec![("Y", results.iter().flat_map(|x| x.1.iter().map(|v| *v as f64)).collect())]; - let names = r[0].0.iter().map(|x| format!("{:?}", x.0)).collect::>(); + let names = r[0].components.iter().map(|x| format!("{:?}", x.0)).collect::>(); data.extend(names.iter() .enumerate() .map(|(i, p)| ( @@ -217,40 +239,88 @@ impl std::fmt::Display for Analysis { } } +impl std::fmt::Debug for Analysis { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{}", self.base)?; + for (&m, n) in self.slopes.iter().zip(self.names.iter()) { + write!(f, " + ({} * {})", m, n)?; + } + write!(f,"") + } +} + #[cfg(test)] mod tests { use super::*; use crate::BenchmarkParameter; + fn benchmark_result( + components: Vec<(BenchmarkParameter, u32)>, + extrinsic_time: u128, + storage_root_time: u128, + reads: u32, + writes: u32, + ) -> BenchmarkResults { + BenchmarkResults { + components, + extrinsic_time, + storage_root_time, + reads, + repeat_reads: 0, + writes, + repeat_writes: 0, + } + } + #[test] fn analysis_median_slopes_should_work() { - let a = Analysis::median_slopes(&vec![ - (vec![(BenchmarkParameter::n, 1), (BenchmarkParameter::m, 5)], 11_500_000, 0), - (vec![(BenchmarkParameter::n, 2), (BenchmarkParameter::m, 5)], 12_500_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 5)], 13_500_000, 0), - (vec![(BenchmarkParameter::n, 4), (BenchmarkParameter::m, 5)], 14_500_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 1)], 13_100_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 3)], 13_300_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 7)], 13_700_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 10)], 14_000_000, 0), - ]).unwrap(); - assert_eq!(a.base, 10_000_000); - assert_eq!(a.slopes, vec![1_000_000, 100_000]); + let data = vec![ + benchmark_result(vec![(BenchmarkParameter::n, 1), (BenchmarkParameter::m, 5)], 11_500_000, 0, 3, 10), + benchmark_result(vec![(BenchmarkParameter::n, 2), (BenchmarkParameter::m, 5)], 12_500_000, 0, 4, 10), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 5)], 13_500_000, 0, 5, 10), + benchmark_result(vec![(BenchmarkParameter::n, 4), (BenchmarkParameter::m, 5)], 14_500_000, 0, 6, 10), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 1)], 13_100_000, 0, 5, 2), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 3)], 13_300_000, 0, 5, 6), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 7)], 13_700_000, 0, 5, 14), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 10)], 14_000_000, 0, 5, 20), + ]; + + let extrinsic_time = Analysis::median_slopes(&data, BenchmarkSelector::ExtrinsicTime).unwrap(); + assert_eq!(extrinsic_time.base, 10_000_000); + assert_eq!(extrinsic_time.slopes, vec![1_000_000, 100_000]); + + let reads = Analysis::median_slopes(&data, BenchmarkSelector::Reads).unwrap(); + assert_eq!(reads.base, 2); + assert_eq!(reads.slopes, vec![1, 0]); + + let writes = Analysis::median_slopes(&data, BenchmarkSelector::Writes).unwrap(); + assert_eq!(writes.base, 0); + assert_eq!(writes.slopes, vec![0, 2]); } #[test] fn analysis_median_min_squares_should_work() { - let a = Analysis::min_squares_iqr(&vec![ - (vec![(BenchmarkParameter::n, 1), (BenchmarkParameter::m, 5)], 11_500_000, 0), - (vec![(BenchmarkParameter::n, 2), (BenchmarkParameter::m, 5)], 12_500_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 5)], 13_500_000, 0), - (vec![(BenchmarkParameter::n, 4), (BenchmarkParameter::m, 5)], 14_500_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 1)], 13_100_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 3)], 13_300_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 7)], 13_700_000, 0), - (vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 10)], 14_000_000, 0), - ]).unwrap(); - assert_eq!(a.base, 10_000_000); - assert_eq!(a.slopes, vec![1_000_000, 100_000]); + let data = vec![ + benchmark_result(vec![(BenchmarkParameter::n, 1), (BenchmarkParameter::m, 5)], 11_500_000, 0, 3, 10), + benchmark_result(vec![(BenchmarkParameter::n, 2), (BenchmarkParameter::m, 5)], 12_500_000, 0, 4, 10), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 5)], 13_500_000, 0, 5, 10), + benchmark_result(vec![(BenchmarkParameter::n, 4), (BenchmarkParameter::m, 5)], 14_500_000, 0, 6, 10), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 1)], 13_100_000, 0, 5, 2), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 3)], 13_300_000, 0, 5, 6), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 7)], 13_700_000, 0, 5, 14), + benchmark_result(vec![(BenchmarkParameter::n, 3), (BenchmarkParameter::m, 10)], 14_000_000, 0, 5, 20), + ]; + + let extrinsic_time = Analysis::min_squares_iqr(&data, BenchmarkSelector::ExtrinsicTime).unwrap(); + assert_eq!(extrinsic_time.base, 10_000_000); + assert_eq!(extrinsic_time.slopes, vec![1_000_000, 100_000]); + + let reads = Analysis::min_squares_iqr(&data, BenchmarkSelector::Reads).unwrap(); + assert_eq!(reads.base, 2); + assert_eq!(reads.slopes, vec![1, 0]); + + let writes = Analysis::min_squares_iqr(&data, BenchmarkSelector::Writes).unwrap(); + assert_eq!(writes.base, 0); + assert_eq!(writes.slopes, vec![0, 2]); } } diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 7cbac3397a978..7a7848305a01c 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -26,7 +26,7 @@ mod analysis; pub use utils::*; #[cfg(feature = "std")] -pub use analysis::Analysis; +pub use analysis::{Analysis, BenchmarkSelector}; #[doc(hidden)] pub use sp_io::storage::root as storage_root; pub use sp_runtime::traits::Zero; @@ -771,6 +771,7 @@ macro_rules! impl_benchmark { highest_range_values: &[u32], steps: &[u32], repeat: u32, + whitelist: &[Vec] ) -> Result, &'static str> { // Map the input to the selected benchmark. let extrinsic = sp_std::str::from_utf8(extrinsic) @@ -780,6 +781,9 @@ macro_rules! impl_benchmark { _ => return Err("Could not find extrinsic."), }; + // Add whitelist to DB + $crate::benchmarking::set_whitelist(whitelist.to_vec()); + // Warm up the DB $crate::benchmarking::commit_db(); $crate::benchmarking::wipe_db(); @@ -841,6 +845,9 @@ macro_rules! impl_benchmark { // This will enable worst case scenario for reading from the database. $crate::benchmarking::commit_db(); + // Reset the read/write counter so we don't count operations in the setup process. + $crate::benchmarking::reset_read_write_count(); + // Time the extrinsic logic. frame_support::debug::trace!( target: "benchmark", @@ -851,11 +858,17 @@ macro_rules! impl_benchmark { closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; - + // Commit the changes to get proper write count + $crate::benchmarking::commit_db(); frame_support::debug::trace!( target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic ); + let read_write_count = $crate::benchmarking::read_write_count(); + frame_support::debug::trace!( + target: "benchmark", + "Read/Write Count {:?}", read_write_count + ); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -863,7 +876,15 @@ macro_rules! impl_benchmark { let finish_storage_root = $crate::benchmarking::current_time(); let elapsed_storage_root = finish_storage_root - start_storage_root; - results.push((c.clone(), elapsed_extrinsic, elapsed_storage_root)); + results.push($crate::BenchmarkResults { + components: c.clone(), + extrinsic_time: elapsed_extrinsic, + storage_root_time: elapsed_storage_root, + reads: read_write_count.0, + repeat_reads: read_write_count.1, + writes: read_write_count.2, + repeat_writes: read_write_count.3, + }); // Wipe the DB back to the genesis state. $crate::benchmarking::wipe_db(); @@ -892,6 +913,7 @@ macro_rules! impl_benchmark { highest_range_values: &[u32], steps: &[u32], repeat: u32, + whitelist: &[Vec] ) -> Result, &'static str> { // Map the input to the selected benchmark. let extrinsic = sp_std::str::from_utf8(extrinsic) @@ -901,6 +923,9 @@ macro_rules! impl_benchmark { _ => return Err("Could not find extrinsic."), }; + // Add whitelist to DB + $crate::benchmarking::set_whitelist(whitelist.to_vec()); + // Warm up the DB $crate::benchmarking::commit_db(); $crate::benchmarking::wipe_db(); @@ -963,6 +988,9 @@ macro_rules! impl_benchmark { // This will enable worst case scenario for reading from the database. $crate::benchmarking::commit_db(); + // Reset the read/write counter so we don't count operations in the setup process. + $crate::benchmarking::reset_read_write_count(); + // Time the extrinsic logic. frame_support::debug::trace!( target: "benchmark", @@ -973,11 +1001,17 @@ macro_rules! impl_benchmark { closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; - + // Commit the changes to get proper write count + $crate::benchmarking::commit_db(); frame_support::debug::trace!( target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic ); + let read_write_count = $crate::benchmarking::read_write_count(); + frame_support::debug::trace!( + target: "benchmark", + "Read/Write Count {:?}", read_write_count + ); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -985,7 +1019,15 @@ macro_rules! impl_benchmark { let finish_storage_root = $crate::benchmarking::current_time(); let elapsed_storage_root = finish_storage_root - start_storage_root; - results.push((c.clone(), elapsed_extrinsic, elapsed_storage_root)); + results.push($crate::BenchmarkResults { + components: c.clone(), + extrinsic_time: elapsed_extrinsic, + storage_root_time: elapsed_storage_root, + reads: read_write_count.0, + repeat_reads: read_write_count.1, + writes: read_write_count.2, + repeat_writes: read_write_count.3, + }); // Wipe the DB back to the genesis state. $crate::benchmarking::wipe_db(); @@ -1139,7 +1181,7 @@ macro_rules! impl_benchmark_test { #[macro_export] macro_rules! add_benchmark { ( $params:ident, $batches:ident, $name:literal, $( $location:tt )* ) => ( - let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat) = $params; + let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist) = $params; if &pallet[..] == &$name[..] || &pallet[..] == &b"*"[..] { if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] { for benchmark in $( $location )*::benchmarks().into_iter() { @@ -1150,6 +1192,7 @@ macro_rules! add_benchmark { &highest_range_values[..], &steps[..], repeat, + whitelist, )?, pallet: $name.to_vec(), benchmark: benchmark.to_vec(), @@ -1163,6 +1206,7 @@ macro_rules! add_benchmark { &highest_range_values[..], &steps[..], repeat, + whitelist, )?, pallet: $name.to_vec(), benchmark: benchmark.clone(), diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 31ec3783cc061..7f9d9121100e3 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -44,7 +44,16 @@ pub struct BenchmarkBatch { /// Results from running benchmarks on a FRAME pallet. /// Contains duration of the function call in nanoseconds along with the benchmark parameters /// used for that benchmark result. -pub type BenchmarkResults = (Vec<(BenchmarkParameter, u32)>, u128, u128); +#[derive(Encode, Decode, Default, Clone, PartialEq, Debug)] +pub struct BenchmarkResults { + pub components: Vec<(BenchmarkParameter, u32)>, + pub extrinsic_time: u128, + pub storage_root_time: u128, + pub reads: u32, + pub repeat_reads: u32, + pub writes: u32, + pub repeat_writes: u32, +} sp_api::decl_runtime_apis! { /// Runtime api for benchmarking a FRAME runtime. @@ -83,6 +92,20 @@ pub trait Benchmarking { fn commit_db(&mut self) { self.commit() } + + /// Get the read/write count + fn read_write_count(&self) -> (u32, u32, u32, u32) { + self.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>) { + self.set_whitelist(new) + } } /// The pallet benchmarking trait. @@ -106,6 +129,7 @@ pub trait Benchmarking { highest_range_values: &[u32], steps: &[u32], repeat: u32, + whitelist: &[Vec] ) -> Result, &'static str>; } diff --git a/primitives/externalities/src/lib.rs b/primitives/externalities/src/lib.rs index 210fe5b4ef009..8e141867195b7 100644 --- a/primitives/externalities/src/lib.rs +++ b/primitives/externalities/src/lib.rs @@ -233,6 +233,27 @@ pub trait Externalities: ExtensionStore { /// /// Commits all changes to the database and clears all caches. fn commit(&mut self); + + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// Benchmarking related functionality and shouldn't be used anywhere else! + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// + /// Gets the current read/write count for the benchmarking process. + fn read_write_count(&self) -> (u32, u32, u32, u32); + + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// Benchmarking related functionality and shouldn't be used anywhere else! + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// + /// 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! + /// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + /// + /// Adds new storage keys to the DB tracking whitelist. + fn set_whitelist(&mut self, new: Vec>); } /// Extension for the [`Externalities`] trait. diff --git a/primitives/runtime-interface/src/impls.rs b/primitives/runtime-interface/src/impls.rs index 217316c3dd79c..259d3517f001d 100644 --- a/primitives/runtime-interface/src/impls.rs +++ b/primitives/runtime-interface/src/impls.rs @@ -365,6 +365,10 @@ impl PassBy for Option { type PassBy = Codec; } +impl PassBy for (u32, u32, u32, u32) { + type PassBy = Codec; +} + /// Implement `PassBy` with `Inner` for the given fixed sized hash types. macro_rules! for_primitive_types { { $( $hash:ident $n:expr ),* $(,)? } => { diff --git a/primitives/state-machine/src/backend.rs b/primitives/state-machine/src/backend.rs index 20a3ab7500a98..9ec03c4d1e249 100644 --- a/primitives/state-machine/src/backend.rs +++ b/primitives/state-machine/src/backend.rs @@ -212,7 +212,22 @@ pub trait Backend: std::fmt::Debug { } /// Commit given transaction to storage. - fn commit(&self, _: H::Out, _: Self::Transaction) -> Result<(), Self::Error> { + fn commit(&self, _: H::Out, _: Self::Transaction, _: StorageCollection) -> Result<(), Self::Error> { + unimplemented!() + } + + /// Get the read/write count of the db + fn read_write_count(&self) -> (u32, u32, u32, u32) { + unimplemented!() + } + + /// Get the read/write count of the db + fn reset_read_write_count(&self) { + unimplemented!() + } + + /// Update the whitelist for tracking db reads/writes + fn set_whitelist(&self, _: Vec>) { unimplemented!() } } diff --git a/primitives/state-machine/src/basic.rs b/primitives/state-machine/src/basic.rs index dbb4c6c2b82f2..6f1d2a4b5ad91 100644 --- a/primitives/state-machine/src/basic.rs +++ b/primitives/state-machine/src/basic.rs @@ -322,6 +322,18 @@ impl Externalities for BasicExternalities { fn wipe(&mut self) {} fn commit(&mut self) {} + + fn read_write_count(&self) -> (u32, u32, u32, u32) { + unimplemented!("read_write_count is not supported in Basic") + } + + fn reset_read_write_count(&mut self) { + unimplemented!("reset_read_write_count is not supported in Basic") + } + + fn set_whitelist(&mut self, _: Vec>) { + unimplemented!("set_whitelist is not supported in Basic") + } } impl sp_externalities::ExtensionStore for BasicExternalities { diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index 2cd63cde975de..e25a08adb0423 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -590,9 +590,22 @@ where self.backend.commit( changes.transaction_storage_root, changes.transaction, + changes.main_storage_changes, ).expect(EXT_NOT_ALLOWED_TO_FAIL); self.mark_dirty(); } + + fn read_write_count(&self) -> (u32, u32, u32, u32) { + self.backend.read_write_count() + } + + fn reset_read_write_count(&mut self) { + self.backend.reset_read_write_count() + } + + 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 2a5d7fda364de..b8a35ced1eb0c 100644 --- a/primitives/state-machine/src/read_only.rs +++ b/primitives/state-machine/src/read_only.rs @@ -185,6 +185,18 @@ impl<'a, H: Hasher, B: 'a + Backend> Externalities for ReadOnlyExternalities< fn wipe(&mut self) {} fn commit(&mut self) {} + + fn read_write_count(&self) -> (u32, u32, u32, u32) { + unimplemented!("read_write_count is not supported in ReadOnlyExternalities") + } + + fn reset_read_write_count(&mut self) { + unimplemented!("reset_read_write_count is not supported in ReadOnlyExternalities") + } + + fn set_whitelist(&mut self, _: Vec>) { + unimplemented!("set_whitelist is not supported in ReadOnlyExternalities") + } } impl<'a, H: Hasher, B: 'a + Backend> sp_externalities::ExtensionStore for ReadOnlyExternalities<'a, H, B> { diff --git a/utils/frame/benchmarking-cli/src/command.rs b/utils/frame/benchmarking-cli/src/command.rs index f867d75d2ab17..7f55672885d46 100644 --- a/utils/frame/benchmarking-cli/src/command.rs +++ b/utils/frame/benchmarking-cli/src/command.rs @@ -17,7 +17,7 @@ use crate::BenchmarkCmd; use codec::{Decode, Encode}; -use frame_benchmarking::{Analysis, BenchmarkBatch}; +use frame_benchmarking::{Analysis, BenchmarkBatch, BenchmarkSelector}; use sc_cli::{SharedParams, CliConfiguration, ExecutionStrategy, Result}; use sc_client_db::BenchmarkingState; use sc_executor::NativeExecutor; @@ -107,15 +107,22 @@ impl BenchmarkCmd { if self.raw_data { // Print the table header - batch.results[0].0.iter().for_each(|param| print!("{:?},", param.0)); + batch.results[0].components.iter().for_each(|param| print!("{:?},", param.0)); - print!("extrinsic_time,storage_root_time\n"); + print!("extrinsic_time,storage_root_time,reads,repeat_reads,writes,repeat_writes\n"); // Print the values batch.results.iter().for_each(|result| { - let parameters = &result.0; + let parameters = &result.components; parameters.iter().for_each(|param| print!("{:?},", param.1)); // Print extrinsic time and storage root time - print!("{:?},{:?}\n", result.1, result.2); + print!("{:?},{:?},{:?},{:?},{:?},{:?}\n", + result.extrinsic_time, + result.storage_root_time, + result.reads, + result.repeat_reads, + result.writes, + result.repeat_writes, + ); }); println!(); @@ -123,13 +130,27 @@ impl BenchmarkCmd { // Conduct analysis. if !self.no_median_slopes { - if let Some(analysis) = Analysis::median_slopes(&batch.results) { - println!("Median Slopes Analysis\n========\n{}", analysis); + println!("Median Slopes Analysis\n========"); + if let Some(analysis) = Analysis::median_slopes(&batch.results, BenchmarkSelector::ExtrinsicTime) { + println!("-- Extrinsic Time --\n{}", analysis); + } + if let Some(analysis) = Analysis::median_slopes(&batch.results, BenchmarkSelector::Reads) { + println!("Reads = {:?}", analysis); + } + if let Some(analysis) = Analysis::median_slopes(&batch.results, BenchmarkSelector::Writes) { + println!("Writes = {:?}", analysis); } } if !self.no_min_squares { - if let Some(analysis) = Analysis::min_squares_iqr(&batch.results) { - println!("Min Squares Analysis\n========\n{}", analysis); + println!("Min Squares Analysis\n========"); + if let Some(analysis) = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::ExtrinsicTime) { + println!("-- Extrinsic Time --\n{}", analysis); + } + if let Some(analysis) = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Reads) { + println!("Reads = {:?}", analysis); + } + if let Some(analysis) = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Writes) { + println!("Writes = {:?}", analysis); } } },