From d67066ae00fc665cea265e4515adf21f9421557b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 11 Jul 2019 18:26:24 +0200 Subject: [PATCH 1/4] Operational tx weight. --- .../src/generic/checked_extrinsic.rs | 7 +++ core/sr-primitives/src/testing.rs | 11 ++++- core/sr-primitives/src/weights.rs | 44 +++++++++++++++---- srml/executive/src/lib.rs | 14 +++--- srml/support/src/dispatch.rs | 44 ++++++++++++++++--- 5 files changed, 97 insertions(+), 23 deletions(-) diff --git a/core/sr-primitives/src/generic/checked_extrinsic.rs b/core/sr-primitives/src/generic/checked_extrinsic.rs index ee43b3af2e951..e7cd204015766 100644 --- a/core/sr-primitives/src/generic/checked_extrinsic.rs +++ b/core/sr-primitives/src/generic/checked_extrinsic.rs @@ -19,6 +19,7 @@ use crate::traits::{self, Member, SimpleArithmetic, MaybeDisplay}; use crate::weights::{Weighable, Weight}; +use crate::transaction_validity::TransactionPriority; /// Definition of something that the external world might want to say; its /// existence implies that it has been checked and is good, particularly with @@ -63,4 +64,10 @@ where fn weight(&self, len: usize) -> Weight { self.function.weight(len) } + fn priority(&self, len: usize) -> TransactionPriority { + self.function.priority(len) + } + fn is_block_full(&self, block_weight: Weight, len: usize) -> bool { + self.function.is_block_full(block_weight, len) + } } diff --git a/core/sr-primitives/src/testing.rs b/core/sr-primitives/src/testing.rs index f8df25ec596b0..04459a9966286 100644 --- a/core/sr-primitives/src/testing.rs +++ b/core/sr-primitives/src/testing.rs @@ -21,7 +21,8 @@ use std::{fmt::Debug, ops::Deref, fmt}; use crate::codec::{Codec, Encode, Decode}; use crate::traits::{self, Checkable, Applyable, BlakeTwo256, OpaqueKeys, TypedKey}; use crate::{generic, KeyTypeId}; -use crate::weights::{Weighable, Weight}; +use crate::weights::{Weighable, Weight, MAX_TRANSACTIONS_WEIGHT}; +use crate::transaction_validity::TransactionPriority; pub use substrate_primitives::H256; use substrate_primitives::U256; use substrate_primitives::ed25519::{Public as AuthorityId}; @@ -237,7 +238,13 @@ impl Applyable for TestXt where } impl Weighable for TestXt { fn weight(&self, len: usize) -> Weight { - // for testing: weight == size. len as Weight } + fn priority(&self, len: usize) -> TransactionPriority { + len as TransactionPriority + } + fn is_block_full(&self, current_weight: Weight, len: usize) + -> bool { + current_weight + self.weight(len) > MAX_TRANSACTIONS_WEIGHT + } } diff --git a/core/sr-primitives/src/weights.rs b/core/sr-primitives/src/weights.rs index 3443992c7396b..9eaac14d3c0a2 100644 --- a/core/sr-primitives/src/weights.rs +++ b/core/sr-primitives/src/weights.rs @@ -24,10 +24,15 @@ //! Note that the decl_module macro _cannot_ enforce this and will simply fail //! if an invalid struct is passed in. +use crate::transaction_validity::TransactionPriority; + /// The final type that each `#[weight = $x:expr]`'s /// expression must evaluate to. pub type Weight = u32; +/// The maximum possible weight of a block. +pub const MAX_TRANSACTIONS_WEIGHT: u32 = 4 * 1024 * 1024; + /// A `Call` enum (aka transaction) that can be weighted using the custom weight attribute of /// its dispatchable functions. Is implemented by default in the `decl_module!`. /// @@ -37,6 +42,13 @@ pub trait Weighable { /// Return the weight of this call. /// The `len` argument is the encoded length of the transaction/call. fn weight(&self, len: usize) -> Weight; + + /// Return the priority of this transaction. + fn priority(&self, len: usize) -> TransactionPriority; + + /// Determine, given the current transaction weight and sum of weights in the current block, if + /// the block is now full or not. + fn is_block_full(&self, block_weight: Weight, len: usize) -> bool; } /// Default type used as the weight representative in a `#[weight = x]` attribute. @@ -46,22 +58,36 @@ pub trait Weighable { pub enum TransactionWeight { /// Basic weight (base, byte). /// The values contained are the base weight and byte weight respectively. + /// + /// The priority of this transaction will be proportional to its computed weight, w.r.t. to the + /// total available weight. Basic(Weight, Weight), - /// Maximum fee. This implies that this transaction _might_ get included but - /// no more transaction can be added. This can be done by setting the - /// implementation to _maximum block weight_. - Max, - /// Free. The transaction does not increase the total weight - /// (i.e. is not included in weight calculation). - Free, + /// Operational transaction. These are typically root transactions for operational updates, + /// runtime code changes or consensus reports through inherents. These transactions are still + /// subject to the same (base, byte) fee but will have maximum priority and will not affect + /// block fullness at all. + Operational(Weight, Weight), } impl Weighable for TransactionWeight { fn weight(&self, len: usize) -> Weight { match self { TransactionWeight::Basic(base, byte) => base + byte * len as Weight, - TransactionWeight::Max => 3 * 1024 * 1024, - TransactionWeight::Free => 0, + TransactionWeight::Operational(_, _) => 0, + } + } + + fn priority(&self, len: usize) -> TransactionPriority { + match self { + TransactionWeight::Basic(_, _) => self.weight(len) as TransactionPriority, + TransactionWeight::Operational(_, _) => TransactionPriority::max_value(), + } + } + + fn is_block_full(&self, block_weight: Weight, len: usize) -> bool { + match self { + TransactionWeight::Basic(_, _) => self.weight(len) + block_weight > MAX_TRANSACTIONS_WEIGHT, + TransactionWeight::Operational(_, _) => false } } } diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index 49d4addb3bc2b..bc5c82501d2d6 100644 --- a/srml/executive/src/lib.rs +++ b/srml/executive/src/lib.rs @@ -86,12 +86,10 @@ use srml_support::{Dispatchable, traits::MakePayment}; use parity_codec::{Codec, Encode}; use system::{extrinsics_root, DigestOf}; use primitives::{ApplyOutcome, ApplyError}; -use primitives::transaction_validity::{TransactionValidity, TransactionPriority, TransactionLongevity}; +use primitives::transaction_validity::{TransactionValidity, TransactionLongevity}; use primitives::weights::Weighable; mod internal { - pub const MAX_TRANSACTIONS_WEIGHT: u32 = 4 * 1024 * 1024; - pub enum ApplyError { BadSignature(&'static str), Stale, @@ -265,8 +263,8 @@ where let xt = uxt.check(&Default::default()).map_err(internal::ApplyError::BadSignature)?; // Check the weight of the block if that extrinsic is applied. - let weight = xt.weight(encoded_len); - if >::all_extrinsics_weight() + weight > internal::MAX_TRANSACTIONS_WEIGHT { + let block_weight = >::all_extrinsics_weight(); + if xt.is_block_full(block_weight, encoded_len) { return Err(internal::ApplyError::FullBlock); } @@ -277,6 +275,7 @@ where if index < &expected_index { internal::ApplyError::Stale } else { internal::ApplyError::Future } ) } // pay any fees + // TODO TODO: use weight to deduct the fee once #2854 is merged. Payment::make_payment(sender, encoded_len).map_err(|_| internal::ApplyError::CantPay)?; // AUDIT: Under no circumstances may this function panic from here onwards. @@ -370,7 +369,7 @@ where }; TransactionValidity::Valid { - priority: encoded_len as TransactionPriority, + priority: xt.priority(encoded_len), requires, provides, longevity: TransactionLongevity::max_value(), @@ -397,6 +396,7 @@ mod tests { use substrate_primitives::{H256, Blake2Hasher}; use primitives::traits::{Header as HeaderT, BlakeTwo256, IdentityLookup}; use primitives::testing::{Digest, Header, Block}; + use primitives::weights::MAX_TRANSACTIONS_WEIGHT; use srml_support::{impl_outer_event, impl_outer_origin, parameter_types}; use srml_support::traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReasons, WithdrawReason}; use system; @@ -586,7 +586,7 @@ mod tests { let xt = primitives::testing::TestXt(Some(1), 0, Call::transfer(33, 69)); let xt2 = primitives::testing::TestXt(Some(1), 1, Call::transfer(33, 69)); let encoded = xt2.encode(); - let len = if should_fail { (internal::MAX_TRANSACTIONS_WEIGHT - 1) as usize } else { encoded.len() }; + let len = if should_fail { (MAX_TRANSACTIONS_WEIGHT - 1) as usize } else { encoded.len() }; with_externalities(&mut t, || { Executive::initialize_block(&Header::new( 1, diff --git a/srml/support/src/dispatch.rs b/srml/support/src/dispatch.rs index f990cbd8d5a1e..52110de3eedc9 100644 --- a/srml/support/src/dispatch.rs +++ b/srml/support/src/dispatch.rs @@ -26,6 +26,7 @@ pub use srml_metadata::{ ModuleConstantMetadata, DefaultByte, DefaultByteGetter, }; pub use sr_primitives::weights::{TransactionWeight, Weighable, Weight}; +pub use sr_primitives::transaction_validity::TransactionPriority; /// A type that cannot be instantiated. pub enum Never {} @@ -1126,6 +1127,22 @@ macro_rules! decl_module { $call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") }, } } + fn priority(&self, _len: usize) -> $crate::dispatch::TransactionPriority { + match self { + $( $call_type::$fn_name(..) => $crate::dispatch::Weighable::priority(&$weight, _len), )* + $call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") }, + } + } + fn is_block_full(&self, _block_weight: $crate::dispatch::Weight, _len: usize) -> bool { + match self { + $( $call_type::$fn_name(..) => $crate::dispatch::Weighable::is_block_full( + &$weight, + _block_weight, + _len, + ), )* + $call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") }, + } + } } // manual implementation of clone/eq/partialeq because using derive erroneously requires @@ -1275,6 +1292,17 @@ macro_rules! impl_outer_dispatch { $( $call_type::$camelcase(call) => call.weight(len), )* } } + fn priority(&self, len: usize) -> $crate::dispatch::TransactionPriority { + match self { + $( $call_type::$camelcase(call) => call.priority(len), )* + } + } + fn is_block_full(&self, block_weight: $crate::dispatch::Weight, len: usize) -> bool { + match self { + + $( $call_type::$camelcase(call) => call.is_block_full(block_weight, len), )* + } + } } impl $crate::dispatch::Dispatchable for $call_type { type Origin = $origin; @@ -1613,8 +1641,8 @@ mod tests { fn on_finalize(n: T::BlockNumber) { if n.into() == 42 { panic!("on_finalize") } } fn offchain_worker() {} - #[weight = TransactionWeight::Max] - fn weighted(_origin) { unreachable!() } + #[weight = TransactionWeight::Operational(0, 1)] + fn security_tx(_origin) { unreachable!() } } } @@ -1680,7 +1708,8 @@ mod tests { documentation: DecodeDifferent::Encode(&[]), }, FunctionMetadata { - name: DecodeDifferent::Encode("weighted"), + + name: DecodeDifferent::Encode("security_tx"), arguments: DecodeDifferent::Encode(&[]), documentation: DecodeDifferent::Encode(&[]), }, @@ -1754,10 +1783,15 @@ mod tests { #[test] fn weight_should_attach_to_call_enum() { - // max weight. not dependent on input. - assert_eq!(Call::::weighted().weight(100), 3 * 1024 * 1024); + // Operational tx. + assert_eq!(Call::::security_tx().weight(100), 0); + assert_eq!(Call::::security_tx().priority(100), u64::max_value()); + // one simply does not return false of the transaction type is operational!. + assert_eq!(Call::::security_tx().is_block_full(u32::max_value(), usize::max_value()), false); + // default weight. assert_eq!(Call::::aux_0().weight(5), 5 /*tx-len*/); + // custom basic assert_eq!(Call::::aux_3().weight(5), 10 + 100 * 5 ); } From b494863598e8ecd6f789c6fb9fbf3293e231a55f Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 11 Jul 2019 18:37:45 +0200 Subject: [PATCH 2/4] Bump. --- node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 04bc98cecb021..fc6d05cbbac72 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -69,7 +69,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to equal spec_version. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 109, + spec_version: 110, impl_version: 110, apis: RUNTIME_API_VERSIONS, }; From d936df82ffa7d94ee8af53bf6393cbc077863786 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 11 Jul 2019 18:46:15 +0200 Subject: [PATCH 3/4] nit. --- core/sr-primitives/src/weights.rs | 3 +-- node/runtime/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/sr-primitives/src/weights.rs b/core/sr-primitives/src/weights.rs index 9eaac14d3c0a2..0d08010e9f1a6 100644 --- a/core/sr-primitives/src/weights.rs +++ b/core/sr-primitives/src/weights.rs @@ -59,8 +59,7 @@ pub enum TransactionWeight { /// Basic weight (base, byte). /// The values contained are the base weight and byte weight respectively. /// - /// The priority of this transaction will be proportional to its computed weight, w.r.t. to the - /// total available weight. + /// The priority of this transaction will be proportional to its computed weight. Basic(Weight, Weight), /// Operational transaction. These are typically root transactions for operational updates, /// runtime code changes or consensus reports through inherents. These transactions are still diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index fc6d05cbbac72..dbb0162b0b126 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -69,8 +69,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to equal spec_version. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 110, - impl_version: 110, + spec_version: 109, + impl_version: 111, apis: RUNTIME_API_VERSIONS, }; From 1e76d97d1c7b5cad3efd65005747ab7fee788254 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 11 Jul 2019 18:51:29 +0200 Subject: [PATCH 4/4] Better function call type. --- srml/executive/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/srml/executive/src/lib.rs b/srml/executive/src/lib.rs index bc5c82501d2d6..62b6b614c0680 100644 --- a/srml/executive/src/lib.rs +++ b/srml/executive/src/lib.rs @@ -264,7 +264,7 @@ where // Check the weight of the block if that extrinsic is applied. let block_weight = >::all_extrinsics_weight(); - if xt.is_block_full(block_weight, encoded_len) { + if as Weighable>::is_block_full(&xt, block_weight, encoded_len) { return Err(internal::ApplyError::FullBlock); }