From 749ec078f8fc166274164d95b8521178897b9a2b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 17 Jan 2023 20:19:54 +0100 Subject: [PATCH 01/31] Add WeightLimit Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/limit.rs | 252 ++++++++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 primitives/weights/src/limit.rs diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs new file mode 100644 index 0000000000000..b24b6055a188c --- /dev/null +++ b/primitives/weights/src/limit.rs @@ -0,0 +1,252 @@ +// This file is part of Substrate. + +// Copyright (C) 2023 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. + +#![allow(unused_imports)] // FAIL-CI remove +use crate::Weight; + +use codec::{CompactAs, Decode, Encode, MaxEncodedLen}; +use core::ops::Mul; +use scale_info::TypeInfo; +#[cfg(feature = "std")] +use serde::{Deserialize, Serialize}; +use smallvec::SmallVec; +use sp_arithmetic::{ + traits::{BaseArithmetic, SaturatedConversion, Saturating, Unsigned, Zero}, + Perbill, +}; +use sp_core::Get; +use sp_debug_derive::RuntimeDebug; + +/// Defines a chromatic, inclusive, upper and optional limit for the [`Weight`] type. +/// +/// - The limit is *chromatic* since both components can be limited independently. +/// - It is *inclusive*, since its [`Contains`] the limit itself. +/// - It is *upper* in the sense that it is a maximum value. It is not a minimum value. +/// - Is is *optional*, since it can set to `None` in which case it is *unlimited*. +#[derive(codec::Encode, codec::Decode, Copy, Clone, RuntimeDebug, TypeInfo)] +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +pub struct WeightLimit { + /// An optional upper limit on the ref time component. + pub ref_time: Option, + + /// An optional upper limit on the proof size component. + pub proof_size: Option, +} + +impl WeightLimit { + /// The limit that only contains `Weight::zero()`. + pub const NOTHING: WeightLimit = Self { ref_time: Some(0), proof_size: Some(0) }; + + /// The limit that contains all possible `Weight`s. + pub const UNLIMITED: WeightLimit = Self { ref_time: None, proof_size: None }; + + /// The limit that only contains `Weight::zero()`. + pub const fn nothing() -> Self { + Self::NOTHING + } + + pub const fn unlimited() -> Self { + Self::UNLIMITED + } + + pub const fn from_weight(weight: Weight) -> Self { + Self { ref_time: Some(weight.ref_time), proof_size: Some(weight.proof_size) } + } + + pub const fn from_limits(ref_time: Option, proof_size: Option) -> Self { + Self { ref_time, proof_size } + } + + pub const fn from_some_limits(ref_time: u64, proof_size: u64) -> Self { + Self { ref_time: Some(ref_time), proof_size: Some(proof_size) } + } + + pub const fn from_time_limit(ref_time: u64) -> Self { + Self { ref_time: Some(ref_time), proof_size: None } + } + // `from_some_time_limit` omitted + + pub const fn from_proof_limit(proof_size: u64) -> Self { + Self { ref_time: None, proof_size: Some(proof_size) } + } + // `from_some_proof_limit` omitted + + pub fn with_time_limit(mut self, ref_time: u64) -> Self { + self.ref_time = Some(ref_time); + self + } + + pub fn with_proof_limit(mut self, proof_size: u64) -> Self { + self.proof_size = Some(proof_size); + self + } + + // FAIL-CI: TODO try if const + pub fn is_unlimited(&self) -> bool { + self.ref_time.is_none() && self.proof_size.is_none() + } + + pub fn is_all_unlimited(&self) -> bool { + self.ref_time.is_none() && self.proof_size.is_none() + } + + pub fn is_any_limited(&self) -> bool { + self.ref_time.is_some() || self.proof_size.is_some() + } + + pub fn is_all_limited(&self) -> bool { + self.ref_time.is_some() && self.proof_size.is_some() + } + + /// All components are zero. + pub fn is_nothing(&self) -> bool { + self.ref_time.map_or(false, |t| t.is_zero()) && + self.proof_size.map_or(false, |s| s.is_zero()) + } + + pub fn is_time_limited(&self) -> bool { + self.ref_time.is_some() + } + + pub fn is_proof_limited(&self) -> bool { + self.proof_size.is_some() + } + + pub fn is_time_unlimited(&self) -> bool { + self.ref_time.is_none() + } + + pub fn is_proof_unlimited(&self) -> bool { + self.proof_size.is_none() + } + + pub fn time_limit(&self) -> Option { + self.ref_time + } + + pub fn proof_limit(&self) -> Option { + self.proof_size + } + + pub fn all_gte(&self, weight: &Weight) -> bool { + self.ref_time.map_or(true, |limit| limit >= weight.ref_time) && + self.proof_size.map_or(true, |limit| limit >= weight.proof_size) + } + + pub fn all_gt(&self, weight: &Weight) -> bool { + self.ref_time.map_or(true, |limit| limit > weight.ref_time) && + self.proof_size.map_or(true, |limit| limit > weight.proof_size) + } + + pub fn all_lte(&self, weight: &Weight) -> bool { + self.ref_time.map_or(true, |limit| limit <= weight.ref_time) && + self.proof_size.map_or(true, |limit| limit <= weight.proof_size) + } + + pub fn all_lt(&self, weight: &Weight) -> bool { + self.ref_time.map_or(true, |limit| limit < weight.ref_time) && + self.proof_size.map_or(true, |limit| limit < weight.proof_size) + } + + pub fn any_lt(&self, weight: &Weight) -> bool { + self.ref_time.map_or(false, |limit| limit < weight.ref_time) || + self.proof_size.map_or(false, |limit| limit < weight.proof_size) + } + + pub fn chromatic_min(&self, other: &WeightLimit) -> Self { + Self { + ref_time: self + .ref_time + .map_or(other.ref_time, |t| Some(t.min(other.ref_time.unwrap_or(u64::MAX)))), + proof_size: self + .proof_size + .map_or(other.proof_size, |s| Some(s.min(other.proof_size.unwrap_or(u64::MAX)))), + } + } + + /// Uses the exact value for each *limited* component and `w` for each unlimited one. + pub fn chromatic_limited_or(self, w: Weight) -> Weight { + Weight { + ref_time: self.ref_time.unwrap_or(w.ref_time), + proof_size: self.proof_size.unwrap_or(w.proof_size), + } + } + + pub fn limited_or_max(self) -> Weight { + self.chromatic_limited_or(Weight::MAX) + } + + pub fn limited_or_min(self) -> Weight { + self.chromatic_limited_or(Weight::zero()) + } + + pub fn saturating_sub(self, other: Weight) -> Self { + Self { + ref_time: self.ref_time.map(|t| t.saturating_sub(other.ref_time)), + proof_size: self.proof_size.map(|s| s.saturating_sub(other.proof_size)), + } + } + + pub fn saturating_decrease(&mut self, other: Self) { + self.ref_time = self.ref_time.map(|t| t.saturating_sub(other.ref_time.unwrap_or(0))); + self.proof_size = self.proof_size.map(|s| s.saturating_sub(other.proof_size.unwrap_or(0))); + } + + pub fn exact_limits(self) -> Option { + match (self.ref_time, self.proof_size) { + (Some(t), Some(s)) => Some(Weight { ref_time: t, proof_size: s }), + _ => None, + } + } + + pub fn check_within(&self, weight: Weight) -> Result<(), ()> { + if self.all_gte(&weight) { + Ok(()) + } else { + Err(()) + } + } +} + +impl From for WeightLimit { + fn from(w: Weight) -> Self { + Self::from_weight(w) + } +} + +macro_rules! weight_mul_per_impl { + ($($t:ty),* $(,)?) => { + $( + impl Mul for $t { + type Output = WeightLimit; + fn mul(self, b: WeightLimit) -> WeightLimit { + WeightLimit { + ref_time: b.ref_time.map(|t| self * t), + proof_size: b.proof_size.map(|s| self * s), + } + } + } + )* + } +} +weight_mul_per_impl!( + sp_arithmetic::Percent, + sp_arithmetic::PerU16, + sp_arithmetic::Permill, + sp_arithmetic::Perbill, + sp_arithmetic::Perquintill, +); From a7101beb33a2a715fb9730cf7cc5db5acee875f5 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 17 Jan 2023 20:20:28 +0100 Subject: [PATCH 02/31] Use weight limit to make PoV check optional Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/dispatch.rs | 4 ++ frame/system/src/extensions/check_weight.rs | 72 +++++++++++---------- frame/system/src/limits.rs | 62 ++++++++++-------- frame/system/src/mock.rs | 8 +-- frame/transaction-payment/src/lib.rs | 16 +++-- primitives/weights/src/lib.rs | 2 + primitives/weights/src/weight_v2.rs | 12 +++- 7 files changed, 105 insertions(+), 71 deletions(-) diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index 93cf08c131641..b01f0ce23abcf 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -433,6 +433,10 @@ impl PerDispatchClass { /// Add some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`. pub fn add(&mut self, weight: Weight, class: DispatchClass) { + self.saturating_accrue(weight, class); + } + + pub fn saturating_accrue(&mut self, weight: Weight, class: DispatchClass) { let value = self.get_mut(class); *value = value.saturating_add(weight); } diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 757b2197bc238..0b3a154c0ec73 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -48,12 +48,10 @@ where fn check_extrinsic_weight( info: &DispatchInfoOf, ) -> Result<(), TransactionValidityError> { - let max = T::BlockWeights::get().get(info.class).max_extrinsic; - match max { - Some(max) if info.weight.any_gt(max) => - Err(InvalidTransaction::ExhaustsResources.into()), - _ => Ok(()), - } + let limit = T::BlockWeights::get().get(info.class).max_extrinsic; + limit + .check_within(info.weight) + .map_err(|_| InvalidTransaction::ExhaustsResources.into()) } /// Checks if the current extrinsic can fit into the block with respect to block weight limits. @@ -134,36 +132,40 @@ where let limit_per_class = maximum_weight.get(info.class); // add the weight. If class is unlimited, use saturating add instead of checked one. - if limit_per_class.max_total.is_none() && limit_per_class.reserved.is_none() { - all_weight.add(extrinsic_weight, info.class) + if limit_per_class.max_total.is_time_unlimited() && limit_per_class.reserved.is_time_unlimited() + { + all_weight.saturating_accrue(extrinsic_weight.without_proof_size(), info.class) } else { all_weight - .checked_add(extrinsic_weight, info.class) + .checked_add(extrinsic_weight.without_proof_size(), info.class) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; + } + + if limit_per_class.max_total.is_proof_unlimited() && + limit_per_class.reserved.is_proof_unlimited() + { + all_weight.saturating_accrue(extrinsic_weight.without_ref_time(), info.class) + } else { + all_weight + .checked_add(extrinsic_weight.without_ref_time(), info.class) .map_err(|_| InvalidTransaction::ExhaustsResources)?; } let per_class = *all_weight.get(info.class); // Check if we don't exceed per-class allowance - match limit_per_class.max_total { - Some(max) if per_class.any_gt(max) => - return Err(InvalidTransaction::ExhaustsResources.into()), - // There is no `max_total` limit (`None`), - // or we are below the limit. - _ => {}, - } + limit_per_class + .max_total + .check_within(per_class) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; // In cases total block weight is exceeded, we need to fall back // to `reserved` pool if there is any. if all_weight.total().any_gt(maximum_weight.max_block) { - match limit_per_class.reserved { - // We are over the limit in reserved pool. - Some(reserved) if per_class.any_gt(reserved) => - return Err(InvalidTransaction::ExhaustsResources.into()), - // There is either no limit in reserved pool (`None`), - // or we are below the limit. - _ => {}, - } + limit_per_class + .reserved + .check_within(per_class) + .map_err(|_| InvalidTransaction::ExhaustsResources)?; } Ok(all_weight) @@ -258,6 +260,7 @@ mod tests { }; use frame_support::{assert_err, assert_ok, dispatch::Pays, weights::Weight}; use sp_std::marker::PhantomData; + use sp_weights::WeightLimit; fn block_weights() -> crate::limits::BlockWeights { ::BlockWeights::get() @@ -266,8 +269,8 @@ mod tests { fn normal_weight_limit() -> Weight { block_weights() .get(DispatchClass::Normal) - .max_total - .unwrap_or_else(|| block_weights().max_block) + .max_total // if the class total limit is unlimited, then the block limit is used. + .chromatic_limited_or(block_weights().max_block) } fn block_weight_limit() -> Weight { @@ -307,8 +310,11 @@ mod tests { fn normal_extrinsic_limited_by_maximum_extrinsic_weight() { new_test_ext().execute_with(|| { let max = DispatchInfo { - weight: block_weights().get(DispatchClass::Normal).max_extrinsic.unwrap() + - Weight::from_ref_time(1), + weight: block_weights() + .get(DispatchClass::Normal) + .max_extrinsic + .exact_limits() + .unwrap() + Weight::from_ref_time(1), // FAIL-CI class: DispatchClass::Normal, ..Default::default() }; @@ -327,7 +333,7 @@ mod tests { let operational_limit = weights .get(DispatchClass::Operational) .max_total - .unwrap_or_else(|| weights.max_block); + .chromatic_limited_or(weights.max_block); let base_weight = weights.get(DispatchClass::Operational).base_extrinsic; let weight = operational_limit - base_weight; @@ -668,12 +674,12 @@ mod tests { .base_block(Weight::zero()) .for_class(DispatchClass::non_mandatory(), |w| { w.base_extrinsic = Weight::zero(); - w.max_total = Some(Weight::from_ref_time(20).set_proof_size(u64::MAX)); + w.max_total = Weight::from_ref_time(20).set_proof_size(20000).into(); }) .for_class(DispatchClass::Mandatory, |w| { w.base_extrinsic = Weight::zero(); - w.reserved = Some(Weight::from_ref_time(5).set_proof_size(u64::MAX)); - w.max_total = None; + w.reserved = Weight::from_ref_time(5).set_proof_size(20000).into(); + w.max_total = WeightLimit::UNLIMITED; }) .build_or_panic(); let all_weight = crate::ConsumedWeight::new(|class| match class { @@ -681,7 +687,7 @@ mod tests { DispatchClass::Operational => Weight::from_ref_time(10), DispatchClass::Mandatory => Weight::zero(), }); - assert_eq!(maximum_weight.max_block, all_weight.total().set_proof_size(u64::MAX)); + assert_eq!(maximum_weight.max_block, all_weight.total().set_proof_size(20000)); // fits into reserved let mandatory1 = DispatchInfo { diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 54d27c5b9e86d..d7ead50f6da32 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -30,7 +30,8 @@ use frame_support::{ weights::{constants, Weight}, }; use scale_info::TypeInfo; -use sp_runtime::{traits::Bounded, Perbill, RuntimeDebug}; +use sp_runtime::{Perbill, RuntimeDebug}; +use sp_weights::WeightLimit; /// Block length limit configuration. #[derive(RuntimeDebug, Clone, codec::Encode, codec::Decode, TypeInfo)] @@ -101,24 +102,25 @@ pub struct WeightsPerClass { pub base_extrinsic: Weight, /// Maximal weight of single extrinsic. Should NOT include `base_extrinsic` cost. /// - /// `None` indicates that this class of extrinsics doesn't have a limit. - pub max_extrinsic: Option, + /// FAIL-CI fix docs + /// `UNLIMITED` indicates that this class of extrinsics doesn't have a limit. + pub max_extrinsic: WeightLimit, /// Block maximal total weight for all extrinsics of given class. /// - /// `None` indicates that weight sum of this class of extrinsics is not + /// `UNLIMITED` indicates that weight sum of this class of extrinsics is not /// restricted. Use this value carefully, since it might produce heavily oversized /// blocks. /// /// In the worst case, the total weight consumed by the class is going to be: /// `MAX(max_total) + MAX(reserved)`. - pub max_total: Option, + pub max_total: WeightLimit, /// Block reserved allowance for all extrinsics of a particular class. /// /// Setting to `None` indicates that extrinsics of that class are allowed /// to go over total block weight (but at most `max_total` for that class). /// Setting to `Some(x)` guarantees that at least `x` weight of particular class /// is processed in every block. - pub reserved: Option, + pub reserved: WeightLimit, } /// Block weight limits & base values configuration. @@ -222,16 +224,13 @@ impl BlockWeights { /// Verifies correctness of this `BlockWeights` object. pub fn validate(self) -> ValidationResult { - fn or_max(w: Option) -> Weight { - w.unwrap_or_else(Weight::max_value) - } let mut error = ValidationErrors::default(); for class in DispatchClass::all() { let weights = self.per_class.get(*class); - let max_for_class = or_max(weights.max_total); + let max_for_class = weights.max_total.limited_or_max(); let base_for_class = weights.base_extrinsic; - let reserved = or_max(weights.reserved); + let reserved = weights.reserved.limited_or_max(); // Make sure that if total is set it's greater than base_block && // base_for_class error_assert!( @@ -245,7 +244,7 @@ impl BlockWeights { error_assert!( weights .max_extrinsic - .unwrap_or(Weight::zero()) + .limited_or_min() .all_lte(max_for_class.saturating_sub(base_for_class)), &mut error, "[{:?}] {:?} (max_extrinsic) can't be greater than {:?} (max for class)", @@ -253,9 +252,9 @@ impl BlockWeights { weights.max_extrinsic, max_for_class.saturating_sub(base_for_class), ); - // Max extrinsic should not be 0 + // Max extrinsic should have a value for each component. error_assert!( - weights.max_extrinsic.unwrap_or_else(Weight::max_value).all_gt(Weight::zero()), + !weights.max_extrinsic.is_nothing(), &mut error, "[{:?}] {:?} (max_extrinsic) must not be 0. Check base cost and average initialization cost.", class, weights.max_extrinsic, @@ -271,7 +270,7 @@ impl BlockWeights { ); // Make sure max block is greater than max_total if it's set. error_assert!( - self.max_block.all_gte(weights.max_total.unwrap_or(Weight::zero())), + self.max_block.all_gte(weights.max_total.limited_or_min()), &mut error, "[{:?}] {:?} (max block) has to be greater than {:?} (max for class)", class, @@ -308,7 +307,7 @@ impl BlockWeights { weights.base_extrinsic = Weight::zero(); }) .for_class(DispatchClass::non_mandatory(), |weights| { - weights.max_total = block_weight.into(); + weights.max_total = WeightLimit::from_weight(block_weight); }) .build() .expect("We only specify max_total and leave base values as defaults; qed") @@ -342,13 +341,16 @@ impl BlockWeights { BlockWeightsBuilder { weights: BlockWeights { base_block: constants::BlockExecutionWeight::get(), - max_block: Weight::zero(), + max_block: Weight::zero(), // This will be set by `build`. per_class: PerDispatchClass::new(|class| { - let initial = - if class == DispatchClass::Mandatory { None } else { Some(Weight::zero()) }; + let initial = if class == DispatchClass::Mandatory { + WeightLimit::UNLIMITED + } else { + WeightLimit::NOTHING + }; WeightsPerClass { base_extrinsic: constants::ExtrinsicBaseWeight::get(), - max_extrinsic: None, + max_extrinsic: WeightLimit::UNLIMITED, max_total: initial, reserved: initial, } @@ -407,20 +409,26 @@ impl BlockWeightsBuilder { // compute max block size. for class in DispatchClass::all() { - weights.max_block = match weights.per_class.get(*class).max_total { - Some(max) => max.max(weights.max_block), - _ => weights.max_block, - }; + // FAIL-CI why is this not just maxing to unlimited aka `limited_or_max`? + weights.max_block = + weights.per_class.get(*class).max_total.limited_or_min().max(weights.max_block); } // compute max size of single extrinsic if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); - if per_class.max_extrinsic.is_none() && init_cost.is_some() { + // FAIL-CI do this per component? + if per_class.max_extrinsic.is_time_unlimited() && init_cost.is_some() { + per_class.max_extrinsic = per_class + .max_total + .saturating_sub(init_weight.without_proof_size()) + .saturating_sub(per_class.base_extrinsic.without_proof_size()); + } + if per_class.max_extrinsic.is_proof_unlimited() && init_cost.is_some() { per_class.max_extrinsic = per_class .max_total - .map(|x| x.saturating_sub(init_weight)) - .map(|x| x.saturating_sub(per_class.base_extrinsic)); + .saturating_sub(init_weight.without_ref_time()) + .saturating_sub(per_class.base_extrinsic.without_ref_time()); } } } diff --git a/frame/system/src/mock.rs b/frame/system/src/mock.rs index fb230f66a94f7..b1f3abff3a20f 100644 --- a/frame/system/src/mock.rs +++ b/frame/system/src/mock.rs @@ -64,14 +64,14 @@ parameter_types! { weights.base_extrinsic = Weight::from_ref_time(5); }) .for_class(DispatchClass::Normal, |weights| { - weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAX_BLOCK_WEIGHT); + weights.max_total = (NORMAL_DISPATCH_RATIO * MAX_BLOCK_WEIGHT).into(); }) .for_class(DispatchClass::Operational, |weights| { weights.base_extrinsic = Weight::from_ref_time(10); - weights.max_total = Some(MAX_BLOCK_WEIGHT); - weights.reserved = Some( + weights.max_total = MAX_BLOCK_WEIGHT.into(); + weights.reserved = ( MAX_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAX_BLOCK_WEIGHT - ); + ).into(); }) .avg_block_initialization(Perbill::from_percent(0)) .build_or_panic(); diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index ce747fa6bd85c..17c2988fefef8 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -196,8 +196,10 @@ where let weights = T::BlockWeights::get(); // the computed ratio is only among the normal class. - let normal_max_weight = - weights.get(DispatchClass::Normal).max_total.unwrap_or(weights.max_block); + let normal_max_weight = weights + .get(DispatchClass::Normal) + .max_total + .chromatic_limited_or(weights.max_block); let current_block_weight = >::block_weight(); let normal_block_weight = current_block_weight.get(DispatchClass::Normal).min(normal_max_weight); @@ -402,10 +404,14 @@ pub mod pallet { ); let target = T::FeeMultiplierUpdate::target() * - T::BlockWeights::get().get(DispatchClass::Normal).max_total.expect( - "Setting `max_total` for `Normal` dispatch class is not compatible with \ + T::BlockWeights::get() + .get(DispatchClass::Normal) + .max_total + .exact_limits() + .expect( + "Setting `max_total` for `Normal` dispatch class is not compatible with \ `transaction-payment` pallet.", - ); + ); // add 1 percent; let addition = target / 100; if addition == Weight::zero() { diff --git a/primitives/weights/src/lib.rs b/primitives/weights/src/lib.rs index 928080d139864..c46c749a81f40 100644 --- a/primitives/weights/src/lib.rs +++ b/primitives/weights/src/lib.rs @@ -28,6 +28,7 @@ extern crate self as sp_weights; +mod limit; mod weight_meter; mod weight_v2; @@ -43,6 +44,7 @@ use sp_arithmetic::{ use sp_core::Get; use sp_debug_derive::RuntimeDebug; +pub use limit::*; pub use weight_meter::*; pub use weight_v2::*; diff --git a/primitives/weights/src/weight_v2.rs b/primitives/weights/src/weight_v2.rs index 2933d80099dd7..9c9e64406b349 100644 --- a/primitives/weights/src/weight_v2.rs +++ b/primitives/weights/src/weight_v2.rs @@ -29,10 +29,10 @@ use super::*; pub struct Weight { #[codec(compact)] /// The weight of computational time used based on some reference hardware. - ref_time: u64, + pub(crate) ref_time: u64, #[codec(compact)] /// The weight of storage space used by proof of validity. - proof_size: u64, + pub(crate) proof_size: u64, } impl From for Weight { @@ -74,6 +74,14 @@ impl Weight { &mut self.proof_size } + pub fn without_ref_time(&self) -> Self { + Self { ref_time: 0, proof_size: self.proof_size } + } + + pub fn without_proof_size(&self) -> Self { + Self { ref_time: self.ref_time, proof_size: 0 } + } + pub const MAX: Self = Self { ref_time: u64::MAX, proof_size: u64::MAX }; /// Get the conservative min of `self` and `other` weight. From a0769f718a7c81f705bfb0222c8abfa677a69e9d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 17 Jan 2023 20:20:45 +0100 Subject: [PATCH 03/31] Emit event for >5MiB PoV blocks Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/lib.rs | 45 ++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 3909b1e9c5257..3cdc71f61dfb2 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -503,17 +503,33 @@ pub mod pallet { #[pallet::event] pub enum Event { /// An extrinsic completed successfully. - ExtrinsicSuccess { dispatch_info: DispatchInfo }, + ExtrinsicSuccess { + dispatch_info: DispatchInfo, + }, /// An extrinsic failed. - ExtrinsicFailed { dispatch_error: DispatchError, dispatch_info: DispatchInfo }, + ExtrinsicFailed { + dispatch_error: DispatchError, + dispatch_info: DispatchInfo, + }, /// `:code` was updated. CodeUpdated, /// A new account was created. - NewAccount { account: T::AccountId }, + NewAccount { + account: T::AccountId, + }, /// An account was reaped. - KilledAccount { account: T::AccountId }, + KilledAccount { + account: T::AccountId, + }, /// On on-chain remark happened. - Remarked { sender: T::AccountId, hash: T::Hash }, + Remarked { + sender: T::AccountId, + hash: T::Hash, + }, + PovSoftLimitExceeded { + limit: u64, + consumed: u64, + }, } /// Error for the System pallet @@ -1338,7 +1354,22 @@ impl Pallet { /// Remove temporary "environment" entries in storage, compute the storage root and return the /// resulting header for this block. pub fn finalize() -> T::Header { - log::debug!( + // Check the soft weight limit. + let consumed = BlockWeight::::get(); + if consumed.total().proof_size() > 5 * 1024 * 1024 { + log::warn!( + target: LOG_TARGET, + "Block {:?} consumed more than 5MiB of proof size", + Self::block_number() + ); + // emit an event + Self::deposit_event(Event::PovSoftLimitExceeded { + limit: 5 * 1024 * 1024, + consumed: consumed.total().proof_size(), + }); + } + + /*log::debug!( target: LOG_TARGET, "[{:?}] {} extrinsics, length: {} (normal {}%, op: {}%, mandatory {}%) / normal weight:\ {} ({}%) op weight {} ({}%) / mandatory weight {} ({}%)", @@ -1372,7 +1403,7 @@ impl Pallet { Self::block_weight().get(DispatchClass::Mandatory).ref_time(), T::BlockWeights::get().get(DispatchClass::Mandatory).max_total.unwrap_or(Bounded::max_value()).ref_time() ).deconstruct(), - ); + );*/ ExecutionPhase::::kill(); AllExtrinsicsLen::::kill(); From f0298e53d337f4d6b88c023ec48b0f45908b45f6 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:45:29 +0100 Subject: [PATCH 04/31] WeightLimit: Cleanup and doc Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/limit.rs | 234 ++++++++++++++++++-------------- 1 file changed, 130 insertions(+), 104 deletions(-) diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index b24b6055a188c..d777dbcde4040 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -15,43 +15,33 @@ // See the License for the specific language governing permissions and // limitations under the License. -#![allow(unused_imports)] // FAIL-CI remove use crate::Weight; - -use codec::{CompactAs, Decode, Encode, MaxEncodedLen}; -use core::ops::Mul; +use codec::{Decode, Encode}; use scale_info::TypeInfo; -#[cfg(feature = "std")] -use serde::{Deserialize, Serialize}; -use smallvec::SmallVec; -use sp_arithmetic::{ - traits::{BaseArithmetic, SaturatedConversion, Saturating, Unsigned, Zero}, - Perbill, -}; -use sp_core::Get; use sp_debug_derive::RuntimeDebug; -/// Defines a chromatic, inclusive, upper and optional limit for the [`Weight`] type. +/// Defines a limit for the [`Weight`] type. /// -/// - The limit is *chromatic* since both components can be limited independently. -/// - It is *inclusive*, since its [`Contains`] the limit itself. -/// - It is *upper* in the sense that it is a maximum value. It is not a minimum value. -/// - Is is *optional*, since it can set to `None` in which case it is *unlimited*. -#[derive(codec::Encode, codec::Decode, Copy, Clone, RuntimeDebug, TypeInfo)] -#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +/// The properties of this limit type are: +/// - *chromatic*: both components can be limited independently. +/// - *inclusive*: since the maximum value [`is_within`] the limit. +/// - *optional*: can be set to `None`, in which case it is *unlimited*. +/// - *upper*: semantically it is an upper limit, not a lower. +#[derive(Encode, Decode, Copy, Clone, RuntimeDebug, TypeInfo)] +#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] pub struct WeightLimit { /// An optional upper limit on the ref time component. - pub ref_time: Option, + pub(crate) ref_time: Option, /// An optional upper limit on the proof size component. - pub proof_size: Option, + pub(crate) proof_size: Option, } impl WeightLimit { /// The limit that only contains `Weight::zero()`. pub const NOTHING: WeightLimit = Self { ref_time: Some(0), proof_size: Some(0) }; - /// The limit that contains all possible `Weight`s. + /// The limit that contains all possible `Weight` values. pub const UNLIMITED: WeightLimit = Self { ref_time: None, proof_size: None }; /// The limit that only contains `Weight::zero()`. @@ -59,166 +49,225 @@ impl WeightLimit { Self::NOTHING } + /// The limit that contains all possible `Weight` values. pub const fn unlimited() -> Self { Self::UNLIMITED } + /// Use the provided weight as the limit. + /// + /// Using `u64::MAX` is deprecated in favour of [`Self::from_limits`]. pub const fn from_weight(weight: Weight) -> Self { Self { ref_time: Some(weight.ref_time), proof_size: Some(weight.proof_size) } } - pub const fn from_limits(ref_time: Option, proof_size: Option) -> Self { - Self { ref_time, proof_size } - } - + /// Construct a new [`Self`] with concrete limits per component. + /// + /// Using `u64::MAX` is deprecated in favour of [`Self::from_limits`]. pub const fn from_some_limits(ref_time: u64, proof_size: u64) -> Self { Self { ref_time: Some(ref_time), proof_size: Some(proof_size) } } + /// Construct a new [`Self`] with optional limits per component. + /// + /// `None` means that the component is *unlimited*. + pub const fn from_limits(ref_time: Option, proof_size: Option) -> Self { + Self { ref_time, proof_size } + } + + /// Construct a new [`Self`] with a concrete time limit. The proof size is unlimited. pub const fn from_time_limit(ref_time: u64) -> Self { Self { ref_time: Some(ref_time), proof_size: None } } - // `from_some_time_limit` omitted + /// Construct a new [`Self`] with a concrete proof size limit. The time is unlimited. pub const fn from_proof_limit(proof_size: u64) -> Self { Self { ref_time: None, proof_size: Some(proof_size) } } - // `from_some_proof_limit` omitted - pub fn with_time_limit(mut self, ref_time: u64) -> Self { + /// Set a time limit for `self`. Overrides any existing time limit. + pub const fn with_time_limit(mut self, ref_time: u64) -> Self { self.ref_time = Some(ref_time); self } - pub fn with_proof_limit(mut self, proof_size: u64) -> Self { + /// Set a proof size limit for `self`. Overrides any existing proof size limit. + pub const fn with_proof_limit(mut self, proof_size: u64) -> Self { self.proof_size = Some(proof_size); self } - // FAIL-CI: TODO try if const - pub fn is_unlimited(&self) -> bool { + /// Whether all components are unlimited. + pub const fn is_all_unlimited(&self) -> bool { self.ref_time.is_none() && self.proof_size.is_none() } - pub fn is_all_unlimited(&self) -> bool { - self.ref_time.is_none() && self.proof_size.is_none() + /// Whether all components are limited. + pub const fn is_all_limited(&self) -> bool { + self.ref_time.is_some() && self.proof_size.is_some() + } + + /// Whether any component is unlimited. + pub const fn is_any_unlimited(&self) -> bool { + self.ref_time.is_none() || self.proof_size.is_none() } - pub fn is_any_limited(&self) -> bool { + /// Whether any component is limited. + pub const fn is_any_limited(&self) -> bool { self.ref_time.is_some() || self.proof_size.is_some() } - pub fn is_all_limited(&self) -> bool { - self.ref_time.is_some() && self.proof_size.is_some() + /// Whether the time is unlimited. + pub const fn is_time_unlimited(&self) -> bool { + self.ref_time.is_none() } - /// All components are zero. - pub fn is_nothing(&self) -> bool { - self.ref_time.map_or(false, |t| t.is_zero()) && - self.proof_size.map_or(false, |s| s.is_zero()) + /// Whether the proof is unlimited. + pub const fn is_proof_unlimited(&self) -> bool { + self.proof_size.is_none() } - pub fn is_time_limited(&self) -> bool { + /// Whether the time component is limited. + pub const fn is_time_limited(&self) -> bool { self.ref_time.is_some() } - pub fn is_proof_limited(&self) -> bool { + /// Whether the proof is limited. + pub const fn is_proof_limited(&self) -> bool { self.proof_size.is_some() } - pub fn is_time_unlimited(&self) -> bool { - self.ref_time.is_none() - } - - pub fn is_proof_unlimited(&self) -> bool { - self.proof_size.is_none() - } - - pub fn time_limit(&self) -> Option { + /// Returns the concrete time limit. `None` means unlimited. + pub const fn time_limit(&self) -> Option { self.ref_time } - pub fn proof_limit(&self) -> Option { + /// Returns the concrete proof size limit. `None` means unlimited. + pub const fn proof_limit(&self) -> Option { self.proof_size } + /// Whether all components of `self` are greater-or-equal than the corresponding components of + /// `weight`. pub fn all_gte(&self, weight: &Weight) -> bool { self.ref_time.map_or(true, |limit| limit >= weight.ref_time) && self.proof_size.map_or(true, |limit| limit >= weight.proof_size) } + /// Whether all components of `self` are greater than the corresponding components of `weight`. pub fn all_gt(&self, weight: &Weight) -> bool { self.ref_time.map_or(true, |limit| limit > weight.ref_time) && self.proof_size.map_or(true, |limit| limit > weight.proof_size) } + /// Whether all components of `self` are less-or-equal than the corresponding components of + /// `weight`. pub fn all_lte(&self, weight: &Weight) -> bool { self.ref_time.map_or(true, |limit| limit <= weight.ref_time) && self.proof_size.map_or(true, |limit| limit <= weight.proof_size) } + /// Whether all components of `self` are less than the corresponding components of `weight`. pub fn all_lt(&self, weight: &Weight) -> bool { self.ref_time.map_or(true, |limit| limit < weight.ref_time) && self.proof_size.map_or(true, |limit| limit < weight.proof_size) } + /// Whether any component of `self` is greater-or-equal than the corresponding component of + /// `weight`. + pub fn any_gte(&self, weight: &Weight) -> bool { + self.ref_time.map_or(false, |limit| limit >= weight.ref_time) || + self.proof_size.map_or(false, |limit| limit >= weight.proof_size) + } + + /// Whether any component of `self` is greater than the corresponding component of `weight`. + pub fn any_gt(&self, weight: &Weight) -> bool { + self.ref_time.map_or(false, |limit| limit > weight.ref_time) || + self.proof_size.map_or(false, |limit| limit > weight.proof_size) + } + + /// Whether any component of `self` is less-or-equal than the corresponding component of + /// `weight`. + pub fn any_lte(&self, weight: &Weight) -> bool { + self.ref_time.map_or(false, |limit| limit <= weight.ref_time) || + self.proof_size.map_or(false, |limit| limit <= weight.proof_size) + } + + /// Whether any component of `self` is less than the corresponding component of `weight`. pub fn any_lt(&self, weight: &Weight) -> bool { self.ref_time.map_or(false, |limit| limit < weight.ref_time) || self.proof_size.map_or(false, |limit| limit < weight.proof_size) } - pub fn chromatic_min(&self, other: &WeightLimit) -> Self { - Self { - ref_time: self - .ref_time - .map_or(other.ref_time, |t| Some(t.min(other.ref_time.unwrap_or(u64::MAX)))), - proof_size: self - .proof_size - .map_or(other.proof_size, |s| Some(s.min(other.proof_size.unwrap_or(u64::MAX)))), - } - } - /// Uses the exact value for each *limited* component and `w` for each unlimited one. - pub fn chromatic_limited_or(self, w: Weight) -> Weight { + /// + /// # Example + /// + /// ```rust + /// use sp_weights::{Weight, WeightLimit}; + /// + /// let weight = Weight::from_parts(100, 100); + /// let limit = WeightLimit::from_weight(weight); + /// // Does nothing if there are concrete limits. + /// assert_eq!(weight, limit.limited_or(Weight::MAX)); + /// + /// let limit = WeightLimit::from_time_limit(100); + /// // Uses `u64::MAX` for the proof size, since there is no *limited* proof size. + /// let weight = Weight::from_parts(100, u64::MAX); + /// assert_eq!(weight, limit.limited_or(Weight::MAX)); + /// ``` + pub fn limited_or(self, w: Weight) -> Weight { Weight { ref_time: self.ref_time.unwrap_or(w.ref_time), proof_size: self.proof_size.unwrap_or(w.proof_size), } } + /// Uses the exact value for each *limited* component and `u64::MAX` for each unlimited one. + /// + /// Equivalent to `limited_or(Weight::MAX)`. pub fn limited_or_max(self) -> Weight { - self.chromatic_limited_or(Weight::MAX) + self.limited_or(Weight::MAX) } + /// Uses the exact value for each *limited* component and `0` for each unlimited one. + /// + /// Equivalent to `limited_or(Weight::zero())`. pub fn limited_or_min(self) -> Weight { - self.chromatic_limited_or(Weight::zero()) + self.limited_or(Weight::zero()) } - pub fn saturating_sub(self, other: Weight) -> Self { - Self { - ref_time: self.ref_time.map(|t| t.saturating_sub(other.ref_time)), - proof_size: self.proof_size.map(|s| s.saturating_sub(other.proof_size)), - } + /// Subtract `other` from `self` and saturate. Does nothing for *unlimited* components. + pub fn saturating_sub(mut self, weight: Weight) -> Self { + self.saturating_decrease(weight); + self } - pub fn saturating_decrease(&mut self, other: Self) { - self.ref_time = self.ref_time.map(|t| t.saturating_sub(other.ref_time.unwrap_or(0))); - self.proof_size = self.proof_size.map(|s| s.saturating_sub(other.proof_size.unwrap_or(0))); + /// Decrease `self` by `other` and saturate. Does nothing for *unlimited* components. + pub fn saturating_decrease(&mut self, weight: Weight) { + self.ref_time = self.ref_time.map(|t| t.saturating_sub(weight.ref_time)); + self.proof_size = self.proof_size.map(|s| s.saturating_sub(weight.proof_size)); } - pub fn exact_limits(self) -> Option { + /// Returns the exact weight limits for each component. + /// + /// The exact limit is the larges possible weight that is still *within* the limit. Returns + /// `None` if one of the components is unlimited. + pub const fn exact_limits(self) -> Option { match (self.ref_time, self.proof_size) { (Some(t), Some(s)) => Some(Weight { ref_time: t, proof_size: s }), _ => None, } } + /// Whether `weight` is within the limits of `self`. + pub fn is_within(&self, weight: Weight) -> bool { + self.all_gte(&weight) + } + + /// Check that `weight` is within the limits of `self`. pub fn check_within(&self, weight: Weight) -> Result<(), ()> { - if self.all_gte(&weight) { - Ok(()) - } else { - Err(()) - } + self.is_within(weight).then(|| ()).ok_or(()) } } @@ -227,26 +276,3 @@ impl From for WeightLimit { Self::from_weight(w) } } - -macro_rules! weight_mul_per_impl { - ($($t:ty),* $(,)?) => { - $( - impl Mul for $t { - type Output = WeightLimit; - fn mul(self, b: WeightLimit) -> WeightLimit { - WeightLimit { - ref_time: b.ref_time.map(|t| self * t), - proof_size: b.proof_size.map(|s| self * s), - } - } - } - )* - } -} -weight_mul_per_impl!( - sp_arithmetic::Percent, - sp_arithmetic::PerU16, - sp_arithmetic::Permill, - sp_arithmetic::Perbill, - sp_arithmetic::Perquintill, -); From 7e7d8a40ad30fc7b8e1331b5cca71bf610466620 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:47:16 +0100 Subject: [PATCH 05/31] cleanup system and create constant for PoV limit Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/lib.rs | 65 +++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 3cdc71f61dfb2..7847381b786d3 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -165,6 +165,9 @@ impl SetCode for () { } } +/// Soft limit for the PoV weight of a block. Above this limit a warning will be printed and an [`Event::PovLimitExceeded`] will be emitted. +pub const SOFT_POV_LIMIT_BYTES: u64 = 5 * 1024 * 1024; + /// Numeric limits over the ability to add a consumer ref using `inc_consumers`. pub trait ConsumerLimits { /// The number of consumers over which `inc_consumers` will cease to work. @@ -503,33 +506,22 @@ pub mod pallet { #[pallet::event] pub enum Event { /// An extrinsic completed successfully. - ExtrinsicSuccess { - dispatch_info: DispatchInfo, - }, + ExtrinsicSuccess { dispatch_info: DispatchInfo }, /// An extrinsic failed. - ExtrinsicFailed { - dispatch_error: DispatchError, - dispatch_info: DispatchInfo, - }, + ExtrinsicFailed { dispatch_error: DispatchError, dispatch_info: DispatchInfo }, /// `:code` was updated. CodeUpdated, /// A new account was created. - NewAccount { - account: T::AccountId, - }, + NewAccount { account: T::AccountId }, /// An account was reaped. - KilledAccount { - account: T::AccountId, - }, + KilledAccount { account: T::AccountId }, /// On on-chain remark happened. - Remarked { - sender: T::AccountId, - hash: T::Hash, - }, - PovSoftLimitExceeded { - limit: u64, - consumed: u64, - }, + Remarked { sender: T::AccountId, hash: T::Hash }, + /// This block exceeded its soft PoV limit. + /// + /// - `limit` is the soft PoV limit. + /// - `consumed` is the PoV consumed by the block, which is always greater than `limit`. + PovLimitExceeded { limit: u64, consumed: u64 }, } /// Error for the System pallet @@ -1333,7 +1325,7 @@ impl Pallet { /// Another potential use-case could be for the `on_initialize` and `on_finalize` hooks. pub fn register_extra_weight_unchecked(weight: Weight, class: DispatchClass) { BlockWeight::::mutate(|current_weight| { - current_weight.add(weight, class); + current_weight.saturating_accrue(weight, class); }); } @@ -1354,22 +1346,25 @@ impl Pallet { /// Remove temporary "environment" entries in storage, compute the storage root and return the /// resulting header for this block. pub fn finalize() -> T::Header { - // Check the soft weight limit. - let consumed = BlockWeight::::get(); - if consumed.total().proof_size() > 5 * 1024 * 1024 { + // Check the soft PoV weight limit. + let consumed = BlockWeight::::get().total().proof_size(); + if consumed > SOFT_POV_LIMIT_BYTES { + let percent = (consumed as f32 * 100.0) / (SOFT_POV_LIMIT_BYTES as f32) - 100.0; log::warn!( target: LOG_TARGET, - "Block {:?} consumed more than 5MiB of proof size", - Self::block_number() + "Block {:?} exceeded the PoV limit by {:.2}%. Consumed: {} > limit: {} bytes", + Self::block_number(), + percent, + consumed, + SOFT_POV_LIMIT_BYTES, ); - // emit an event Self::deposit_event(Event::PovSoftLimitExceeded { - limit: 5 * 1024 * 1024, - consumed: consumed.total().proof_size(), + limit: SOFT_POV_LIMIT_BYTES, + consumed, }); } - /*log::debug!( + log::debug!( target: LOG_TARGET, "[{:?}] {} extrinsics, length: {} (normal {}%, op: {}%, mandatory {}%) / normal weight:\ {} ({}%) op weight {} ({}%) / mandatory weight {} ({}%)", @@ -1391,19 +1386,19 @@ impl Pallet { Self::block_weight().get(DispatchClass::Normal), sp_runtime::Percent::from_rational( Self::block_weight().get(DispatchClass::Normal).ref_time(), - T::BlockWeights::get().get(DispatchClass::Normal).max_total.unwrap_or(Bounded::max_value()).ref_time() + T::BlockWeights::get().get(DispatchClass::Normal).max_total.limited_or(Bounded::max_value()).ref_time() ).deconstruct(), Self::block_weight().get(DispatchClass::Operational), sp_runtime::Percent::from_rational( Self::block_weight().get(DispatchClass::Operational).ref_time(), - T::BlockWeights::get().get(DispatchClass::Operational).max_total.unwrap_or(Bounded::max_value()).ref_time() + T::BlockWeights::get().get(DispatchClass::Operational).max_total.limited_or(Bounded::max_value()).ref_time() ).deconstruct(), Self::block_weight().get(DispatchClass::Mandatory), sp_runtime::Percent::from_rational( Self::block_weight().get(DispatchClass::Mandatory).ref_time(), - T::BlockWeights::get().get(DispatchClass::Mandatory).max_total.unwrap_or(Bounded::max_value()).ref_time() + T::BlockWeights::get().get(DispatchClass::Mandatory).max_total.limited_or(Bounded::max_value()).ref_time() ).deconstruct(), - );*/ + ); ExecutionPhase::::kill(); AllExtrinsicsLen::::kill(); From 2dc79da5b354e5e564156142e2263628a86deb29 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:47:40 +0100 Subject: [PATCH 06/31] Fix PerDispatchClass arithmetic Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/dispatch.rs | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index b01f0ce23abcf..77e803daefa98 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -423,37 +423,35 @@ impl PerDispatchClass { impl PerDispatchClass { /// Returns the total weight consumed by all extrinsics in the block. + /// + /// Saturates on overflow. pub fn total(&self) -> Weight { let mut sum = Weight::zero(); for class in DispatchClass::all() { - sum = sum.saturating_add(*self.get(*class)); + sum.saturating_accrue(*self.get(*class)); } sum } - /// Add some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`. - pub fn add(&mut self, weight: Weight, class: DispatchClass) { + /// Add some weight to the given class. Saturates at the numeric bounds. + pub fn saturating_add(mut self, weight: Weight, class: DispatchClass) -> Self { self.saturating_accrue(weight, class); + self } + /// Increase the weight of the given class. Saturates at the numeric bounds. pub fn saturating_accrue(&mut self, weight: Weight, class: DispatchClass) { - let value = self.get_mut(class); - *value = value.saturating_add(weight); + self.get_mut(class).saturating_accrue(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(()) + /// Try to increase the weight of the given class. Saturates at the numeric bounds. + pub fn checked_accrue(&mut self, weight: Weight, class: DispatchClass) -> Result<(), ()> { + self.get_mut(class).checked_accrue(weight).ok_or(()) } - /// 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); + /// Reduce the weight of the given class. Saturates at the numeric bounds. + pub fn saturating_reduce(&mut self, weight: Weight, class: DispatchClass) { + self.get_mut(class).saturating_reduce(weight); } } From aa291ea033663aad5554c45c34df0f8ded68d83f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:47:51 +0100 Subject: [PATCH 07/31] Test PerDispatchClass Signed-off-by: Oliver Tale-Yazdi --- frame/support/src/dispatch.rs | 192 ++++++++++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/frame/support/src/dispatch.rs b/frame/support/src/dispatch.rs index 77e803daefa98..26e8611129bc7 100644 --- a/frame/support/src/dispatch.rs +++ b/frame/support/src/dispatch.rs @@ -3695,3 +3695,195 @@ mod weight_tests { assert_eq!(extract_actual_pays_fee(&Ok((Some(1000), Pays::Yes).into()), &pre), Pays::No); } } + +#[cfg(test)] +mod per_dispatch_class_tests { + use super::*; + use sp_runtime::traits::Zero; + use DispatchClass::*; + + // helper trait + trait IntoWeight { + fn into_weight(self) -> Weight; + } + + impl IntoWeight for u64 { + fn into_weight(self) -> Weight { + Weight::from_all(self) + } + } + + impl IntoWeight for (u64, u64) { + fn into_weight(self) -> Weight { + Weight::from_parts(self.0, self.1) + } + } + + #[test] + fn saturating_add_works() { + let a = PerDispatchClass { + normal: (5, 10).into_weight(), + operational: (20, 30).into_weight(), + mandatory: Weight::MAX, + }; + assert_eq!( + a.clone() + .saturating_add((20, 5).into_weight(), Normal) + .saturating_add((10, 10).into_weight(), Operational) + .saturating_add((u64::MAX, 3).into_weight(), Mandatory), + PerDispatchClass { + normal: (25, 15).into_weight(), + operational: (30, 40).into_weight(), + mandatory: Weight::MAX + } + ); + let b = a + .saturating_add(Weight::MAX, Normal) + .saturating_add(Weight::MAX, Operational) + .saturating_add(Weight::MAX, Mandatory); + assert_eq!( + b, + PerDispatchClass { + normal: Weight::MAX, + operational: Weight::MAX, + mandatory: Weight::MAX + } + ); + assert_eq!(b.total(), Weight::MAX); + } + + #[test] + fn saturating_accrue_works() { + let mut a = PerDispatchClass::default(); + + a.saturating_accrue((10, 15).into_weight(), Normal); + assert_eq!(a.normal, (10, 15).into_weight()); + assert_eq!(a.total(), (10, 15).into_weight()); + + a.saturating_accrue((20, 25).into_weight(), Operational); + assert_eq!(a.operational, (20, 25).into_weight()); + assert_eq!(a.total(), (30, 40).into_weight()); + + a.saturating_accrue((30, 35).into_weight(), Mandatory); + assert_eq!(a.mandatory, (30, 35).into_weight()); + assert_eq!(a.total(), (60, 75).into_weight()); + + a.saturating_accrue((u64::MAX, 10).into_weight(), Operational); + assert_eq!(a.operational, (u64::MAX, 35).into_weight()); + assert_eq!(a.total(), (u64::MAX, 85).into_weight()); + + a.saturating_accrue((10, u64::MAX).into_weight(), Normal); + assert_eq!(a.normal, (20, u64::MAX).into_weight()); + assert_eq!(a.total(), Weight::MAX); + } + + #[test] + fn saturating_reduce_works() { + let mut a = PerDispatchClass { + normal: (10, u64::MAX).into_weight(), + mandatory: (u64::MAX, 10).into_weight(), + operational: (20, 20).into_weight(), + }; + + a.saturating_reduce((5, 100).into_weight(), Normal); + assert_eq!(a.normal, (5, u64::MAX - 100).into_weight()); + assert_eq!(a.total(), (u64::MAX, u64::MAX - 70).into_weight()); + + a.saturating_reduce((15, 5).into_weight(), Operational); + assert_eq!(a.operational, (5, 15).into_weight()); + assert_eq!(a.total(), (u64::MAX, u64::MAX - 75).into_weight()); + + a.saturating_reduce((50, 0).into_weight(), Mandatory); + assert_eq!(a.mandatory, (u64::MAX - 50, 10).into_weight()); + assert_eq!(a.total(), (u64::MAX - 40, u64::MAX - 75).into_weight()); + + a.saturating_reduce((u64::MAX, 100).into_weight(), Operational); + assert!(a.operational.is_zero()); + assert_eq!(a.total(), (u64::MAX - 45, u64::MAX - 90).into_weight()); + + a.saturating_reduce((5, u64::MAX).into_weight(), Normal); + assert!(a.normal.is_zero()); + assert_eq!(a.total(), (u64::MAX - 50, 10).into_weight()); + } + + #[test] + fn checked_accrue_works() { + let mut a = PerDispatchClass::default(); + + a.checked_accrue((1, 2).into_weight(), Normal).unwrap(); + a.checked_accrue((3, 4).into_weight(), Operational).unwrap(); + a.checked_accrue((5, 6).into_weight(), Mandatory).unwrap(); + a.checked_accrue((7, 8).into_weight(), Operational).unwrap(); + a.checked_accrue((9, 0).into_weight(), Normal).unwrap(); + + assert_eq!( + a, + PerDispatchClass { + normal: (10, 2).into_weight(), + operational: (10, 12).into_weight(), + mandatory: (5, 6).into_weight(), + } + ); + + a.checked_accrue((u64::MAX - 10, u64::MAX - 2).into_weight(), Normal).unwrap(); + a.checked_accrue((0, 0).into_weight(), Normal).unwrap(); + a.checked_accrue((1, 0).into_weight(), Normal).unwrap_err(); + a.checked_accrue((0, 1).into_weight(), Normal).unwrap_err(); + + assert_eq!( + a, + PerDispatchClass { + normal: Weight::MAX, + operational: (10, 12).into_weight(), + mandatory: (5, 6).into_weight(), + } + ); + } + + #[test] + fn checked_accrue_does_not_modify_on_error() { + let mut a = PerDispatchClass { + normal: 0.into_weight(), + operational: Weight::MAX / 2 + 2.into_weight(), + mandatory: 10.into_weight(), + }; + + a.checked_accrue(Weight::MAX / 2, Operational).unwrap_err(); + a.checked_accrue(Weight::MAX - 9.into_weight(), Mandatory).unwrap_err(); + a.checked_accrue(Weight::MAX, Normal).unwrap(); // This one works + + assert_eq!( + a, + PerDispatchClass { + normal: Weight::MAX, + operational: Weight::MAX / 2 + 2.into_weight(), + mandatory: 10.into_weight(), + } + ); + } + + #[test] + fn total_works() { + assert!(PerDispatchClass::default().total().is_zero()); + + assert_eq!( + PerDispatchClass { + normal: 0.into_weight(), + operational: (10, 20).into_weight(), + mandatory: (20, u64::MAX).into_weight(), + } + .total(), + (30, u64::MAX).into_weight() + ); + + assert_eq!( + PerDispatchClass { + normal: (u64::MAX - 10, 10).into_weight(), + operational: (3, u64::MAX).into_weight(), + mandatory: (4, u64::MAX).into_weight(), + } + .total(), + (u64::MAX - 3, u64::MAX).into_weight() + ); + } +} From dcedaa882787f31027502bff1a7549796f0650fa Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:50:38 +0100 Subject: [PATCH 08/31] Add some functions to Weight Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/weight_v2.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/primitives/weights/src/weight_v2.rs b/primitives/weights/src/weight_v2.rs index 9c9e64406b349..d81dc0cd2645f 100644 --- a/primitives/weights/src/weight_v2.rs +++ b/primitives/weights/src/weight_v2.rs @@ -125,6 +125,11 @@ impl Weight { Self { ref_time, proof_size } } + /// Construct [`Weight`] from the same weight for all parts. + pub const fn from_all(value: u64) -> Self { + Self { ref_time: value, proof_size: value } + } + /// Saturating [`Weight`] addition. Computes `self + rhs`, saturating at the numeric bounds of /// all fields instead of overflowing. pub const fn saturating_add(self, rhs: Self) -> Self { @@ -175,6 +180,11 @@ impl Weight { *self = self.saturating_add(amount); } + /// Reduce [`Weight`] by `amount` via saturating subtraction. + pub fn saturating_reduce(&mut self, amount: Self) { + *self = self.saturating_sub(amount); + } + /// Checked [`Weight`] addition. Computes `self + rhs`, returning `None` if overflow occurred. pub const fn checked_add(&self, rhs: &Self) -> Option { let ref_time = match self.ref_time.checked_add(rhs.ref_time) { @@ -230,6 +240,16 @@ impl Weight { Some(Self { ref_time, proof_size }) } + /// Try to increase `self` by `amount` via checked addition. + pub fn checked_accrue(&mut self, amount: Self) -> Option<()> { + self.checked_add(&amount).map(|new_self| *self = new_self) + } + + /// Try to reduce `self` by `amount` via checked subtraction. + pub fn checked_reduce(&mut self, amount: Self) -> Option<()> { + self.checked_sub(&amount).map(|new_self| *self = new_self) + } + /// Return a [`Weight`] where all fields are zero. pub const fn zero() -> Self { Self { ref_time: 0, proof_size: 0 } From 7e697d39fbe63befe6a0ba68280666db5fa75053 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:51:27 +0100 Subject: [PATCH 09/31] Use correct arithmetic names in CheckWeight Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/extensions/check_weight.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 0b3a154c0ec73..898405cda3577 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -137,7 +137,7 @@ where all_weight.saturating_accrue(extrinsic_weight.without_proof_size(), info.class) } else { all_weight - .checked_add(extrinsic_weight.without_proof_size(), info.class) + .checked_accrue(extrinsic_weight.without_proof_size(), info.class) .map_err(|_| InvalidTransaction::ExhaustsResources)?; } @@ -147,7 +147,7 @@ where all_weight.saturating_accrue(extrinsic_weight.without_ref_time(), info.class) } else { all_weight - .checked_add(extrinsic_weight.without_ref_time(), info.class) + .checked_accrue(extrinsic_weight.without_ref_time(), info.class) .map_err(|_| InvalidTransaction::ExhaustsResources)?; } @@ -231,7 +231,7 @@ where let unspent = post_info.calc_unspent(info); if unspent.any_gt(Weight::zero()) { crate::BlockWeight::::mutate(|current_weight| { - current_weight.sub(unspent, info.class); + current_weight.saturating_reduce(unspent, info.class); }) } @@ -270,7 +270,7 @@ mod tests { block_weights() .get(DispatchClass::Normal) .max_total // if the class total limit is unlimited, then the block limit is used. - .chromatic_limited_or(block_weights().max_block) + .limited_or(block_weights().max_block) } fn block_weight_limit() -> Weight { @@ -330,10 +330,8 @@ mod tests { fn operational_extrinsic_limited_by_operational_space_limit() { new_test_ext().execute_with(|| { let weights = block_weights(); - let operational_limit = weights - .get(DispatchClass::Operational) - .max_total - .chromatic_limited_or(weights.max_block); + let operational_limit = + weights.get(DispatchClass::Operational).max_total.limited_or(weights.max_block); let base_weight = weights.get(DispatchClass::Operational).base_extrinsic; let weight = operational_limit - base_weight; From 09989f4c3770fdf3a4ffaa6d70fc8ce84182179a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:52:07 +0100 Subject: [PATCH 10/31] Renames Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/limits.rs | 4 ++-- frame/transaction-payment/src/lib.rs | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index d7ead50f6da32..2777a6fa77906 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -252,9 +252,9 @@ impl BlockWeights { weights.max_extrinsic, max_for_class.saturating_sub(base_for_class), ); - // Max extrinsic should have a value for each component. + // Max extrinsic must non be zero in any component. error_assert!( - !weights.max_extrinsic.is_nothing(), + weights.max_extrinsic.all_gt(&Weight::zero()), &mut error, "[{:?}] {:?} (max_extrinsic) must not be 0. Check base cost and average initialization cost.", class, weights.max_extrinsic, diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 17c2988fefef8..98e15cbd80f77 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -196,10 +196,8 @@ where let weights = T::BlockWeights::get(); // the computed ratio is only among the normal class. - let normal_max_weight = weights - .get(DispatchClass::Normal) - .max_total - .chromatic_limited_or(weights.max_block); + let normal_max_weight = + weights.get(DispatchClass::Normal).max_total.limited_or(weights.max_block); let current_block_weight = >::block_weight(); let normal_block_weight = current_block_weight.get(DispatchClass::Normal).min(normal_max_weight); From 66042a63e02ba80bfb0e665c8b915c78f1b571bd Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:52:16 +0100 Subject: [PATCH 11/31] Fix test Signed-off-by: Oliver Tale-Yazdi --- frame/executive/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index b454d1e276cc8..c84b006eab27b 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -1125,7 +1125,8 @@ mod tests { // on_initialize weight + base block execution weight let block_weights = ::BlockWeights::get(); let base_block_weight = Weight::from_ref_time(175) + block_weights.base_block; - let limit = block_weights.get(DispatchClass::Normal).max_total.unwrap() - base_block_weight; + let limit = block_weights.get(DispatchClass::Normal).max_total.exact_limits().unwrap() - + base_block_weight; let num_to_exhaust_block = limit.ref_time() / (encoded_len + 5); t.execute_with(|| { Executive::initialize_block(&Header::new( From bf931df7f5424963eced8d51419090c4c7c78ca8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:52:43 +0100 Subject: [PATCH 12/31] Update kitchensink runtime This is probably what we have to do for all runtimes. Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a9e93a16f0713..d7db1dd9b237a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -190,15 +190,15 @@ parameter_types! { weights.base_extrinsic = ExtrinsicBaseWeight::get(); }) .for_class(DispatchClass::Normal, |weights| { - weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT); + weights.max_total = (NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT).into(); }) .for_class(DispatchClass::Operational, |weights| { - weights.max_total = Some(MAXIMUM_BLOCK_WEIGHT); + weights.max_total = (MAXIMUM_BLOCK_WEIGHT).into(); // Operational transactions have some extra reserved space, so that they // are included even if block reached `MAXIMUM_BLOCK_WEIGHT`. - weights.reserved = Some( + weights.reserved = ( MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT - ); + ).into(); }) .avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO) .build_or_panic(); @@ -614,7 +614,9 @@ parameter_types! { pub const MultiPhaseUnsignedPriority: TransactionPriority = StakingUnsignedPriority::get() - 1u64; pub MinerMaxWeight: Weight = RuntimeBlockWeights::get() .get(DispatchClass::Normal) - .max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed") + .max_extrinsic + .exact_limits() + .expect("Normal extrinsics have a weight limit configured; qed") .saturating_sub(BlockExecutionWeight::get()); // Solution can occupy 90% of normal block size pub MinerMaxLength: u32 = Perbill::from_rational(9u32, 10) * @@ -1187,7 +1189,7 @@ parameter_types! { .per_class .get(DispatchClass::Normal) .max_total - .unwrap_or(RuntimeBlockWeights::get().max_block); + .limited_or(RuntimeBlockWeights::get().max_block); pub Schedule: pallet_contracts::Schedule = Default::default(); } From 9dfa68a8592d219f441da3eb1970bf14570e6010 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 00:53:25 +0100 Subject: [PATCH 13/31] fmt Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 7847381b786d3..b21948b5fcfa3 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -165,7 +165,8 @@ impl SetCode for () { } } -/// Soft limit for the PoV weight of a block. Above this limit a warning will be printed and an [`Event::PovLimitExceeded`] will be emitted. +/// Soft limit for the PoV weight of a block. Above this limit a warning will be printed and an +/// [`Event::PovLimitExceeded`] will be emitted. pub const SOFT_POV_LIMIT_BYTES: u64 = 5 * 1024 * 1024; /// Numeric limits over the ability to add a consumer ref using `inc_consumers`. From 8845e948aa15c3e525f476b180e7ad0337f6a636 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 01:18:09 +0100 Subject: [PATCH 14/31] Nicer print Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index b21948b5fcfa3..517fbe3ccde76 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1353,13 +1353,13 @@ impl Pallet { let percent = (consumed as f32 * 100.0) / (SOFT_POV_LIMIT_BYTES as f32) - 100.0; log::warn!( target: LOG_TARGET, - "Block {:?} exceeded the PoV limit by {:.2}%. Consumed: {} > limit: {} bytes", + "Block {:?} exceeded the PoV limit by {:.2}%; {:.2} > {:.2} MiB", Self::block_number(), percent, - consumed, - SOFT_POV_LIMIT_BYTES, + consumed as f32 / (1024 * 1024) as f32, + SOFT_POV_LIMIT_BYTES as f32 / (1024 * 1024) as f32, ); - Self::deposit_event(Event::PovSoftLimitExceeded { + Self::deposit_event(Event::PovLimitExceeded { limit: SOFT_POV_LIMIT_BYTES, consumed, }); From b6566f99dad49caf6418cd2f5dd96bb6fd457490 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 01:41:55 +0100 Subject: [PATCH 15/31] fix test Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/impls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/impls.rs b/bin/node/runtime/src/impls.rs index b3f58ea5d24ab..2da36cb087918 100644 --- a/bin/node/runtime/src/impls.rs +++ b/bin/node/runtime/src/impls.rs @@ -134,7 +134,7 @@ mod multiplier_tests { BlockWeights::get() .get(DispatchClass::Normal) .max_total - .unwrap_or_else(|| BlockWeights::get().max_block) + .limited_or(BlockWeights::get().max_block) } fn min_multiplier() -> Multiplier { @@ -284,7 +284,7 @@ mod multiplier_tests { // `cargo test congested_chain_simulation -- --nocapture` to get some insight. // almost full. The entire quota of normal transactions is taken. - let block_weight = BlockWeights::get().get(DispatchClass::Normal).max_total.unwrap() - + let block_weight = BlockWeights::get().get(DispatchClass::Normal).max_total.exact_limits().unwrap() - Weight::from_ref_time(100); // Default substrate weight. From 3acdc2cd5199a387431d6c51d92eda72c22c4bc3 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 01:51:35 +0100 Subject: [PATCH 16/31] fmt + doc Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/impls.rs | 5 +++-- frame/system/src/lib.rs | 5 +---- frame/system/src/limits.rs | 1 - primitives/weights/src/limit.rs | 2 +- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/bin/node/runtime/src/impls.rs b/bin/node/runtime/src/impls.rs index 2da36cb087918..5f7b1ec924daa 100644 --- a/bin/node/runtime/src/impls.rs +++ b/bin/node/runtime/src/impls.rs @@ -284,8 +284,9 @@ mod multiplier_tests { // `cargo test congested_chain_simulation -- --nocapture` to get some insight. // almost full. The entire quota of normal transactions is taken. - let block_weight = BlockWeights::get().get(DispatchClass::Normal).max_total.exact_limits().unwrap() - - Weight::from_ref_time(100); + let block_weight = + BlockWeights::get().get(DispatchClass::Normal).max_total.exact_limits().unwrap() - + Weight::from_ref_time(100); // Default substrate weight. let tx_weight = frame_support::weights::constants::ExtrinsicBaseWeight::get(); diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 517fbe3ccde76..89e2a6b2faa50 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1359,10 +1359,7 @@ impl Pallet { consumed as f32 / (1024 * 1024) as f32, SOFT_POV_LIMIT_BYTES as f32 / (1024 * 1024) as f32, ); - Self::deposit_event(Event::PovLimitExceeded { - limit: SOFT_POV_LIMIT_BYTES, - consumed, - }); + Self::deposit_event(Event::PovLimitExceeded { limit: SOFT_POV_LIMIT_BYTES, consumed }); } log::debug!( diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 2777a6fa77906..d54eccb944091 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -417,7 +417,6 @@ impl BlockWeightsBuilder { if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); - // FAIL-CI do this per component? if per_class.max_extrinsic.is_time_unlimited() && init_cost.is_some() { per_class.max_extrinsic = per_class .max_total diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index d777dbcde4040..c7109008ef623 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -24,7 +24,7 @@ use sp_debug_derive::RuntimeDebug; /// /// The properties of this limit type are: /// - *chromatic*: both components can be limited independently. -/// - *inclusive*: since the maximum value [`is_within`] the limit. +/// - *inclusive*: since the maximum value [`Self::is_within`] the limit. /// - *optional*: can be set to `None`, in which case it is *unlimited*. /// - *upper*: semantically it is an upper limit, not a lower. #[derive(Encode, Decode, Copy, Clone, RuntimeDebug, TypeInfo)] From 35c18c24a1099002cc85169359f96b2e5519e9c7 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 13:02:35 +0100 Subject: [PATCH 17/31] Add event test Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/tests.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs index c42131c450228..4477716b52714 100644 --- a/frame/system/src/tests.rs +++ b/frame/system/src/tests.rs @@ -635,6 +635,32 @@ fn events_not_emitted_during_genesis() { }); } +/// Consuming more than `SOFT_POV_LIMIT_BYTES` should result in a `PovLimitExceeded` event. +#[test] +fn event_emitted_when_over_pov_limit() { + new_test_ext().execute_with(|| { + for i in 0..10 { + let consumed = SOFT_POV_LIMIT_BYTES + i - 5; + + System::reset_events(); + System::initialize(&1, &[0u8; 32].into(), &Default::default()); + System::note_finished_extrinsics(); + // Set the consumed PoV weight. + System::set_block_consumed_resources(Weight::from_parts(u64::MAX, consumed), 1); + System::note_finished_extrinsics(); + System::finalize(); + + if consumed > SOFT_POV_LIMIT_BYTES { + System::assert_has_event( + Event::PovLimitExceeded { limit: SOFT_POV_LIMIT_BYTES, consumed }.into(), + ); + } else { + assert!(System::events().is_empty(), "No events"); + } + } + }); +} + #[test] fn extrinsics_root_is_calculated_correctly() { new_test_ext().execute_with(|| { From 4acf078fd9fa4f18b15a6f9e1c93b82c505949ee Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 14:21:51 +0100 Subject: [PATCH 18/31] Test WeightLimit Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/extensions/check_weight.rs | 6 +- primitives/weights/src/limit.rs | 365 +++++++++++++++++++- 2 files changed, 361 insertions(+), 10 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 898405cda3577..650d52f822d62 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -50,7 +50,7 @@ where ) -> Result<(), TransactionValidityError> { let limit = T::BlockWeights::get().get(info.class).max_extrinsic; limit - .check_within(info.weight) + .check_contains(info.weight) .map_err(|_| InvalidTransaction::ExhaustsResources.into()) } @@ -156,7 +156,7 @@ where // Check if we don't exceed per-class allowance limit_per_class .max_total - .check_within(per_class) + .check_contains(per_class) .map_err(|_| InvalidTransaction::ExhaustsResources)?; // In cases total block weight is exceeded, we need to fall back @@ -164,7 +164,7 @@ where if all_weight.total().any_gt(maximum_weight.max_block) { limit_per_class .reserved - .check_within(per_class) + .check_contains(per_class) .map_err(|_| InvalidTransaction::ExhaustsResources)?; } diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index c7109008ef623..457b8bbb33f31 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -24,11 +24,11 @@ use sp_debug_derive::RuntimeDebug; /// /// The properties of this limit type are: /// - *chromatic*: both components can be limited independently. -/// - *inclusive*: since the maximum value [`Self::is_within`] the limit. +/// - *inclusive*: since it [`Self::contains`] the limit. /// - *optional*: can be set to `None`, in which case it is *unlimited*. /// - *upper*: semantically it is an upper limit, not a lower. #[derive(Encode, Decode, Copy, Clone, RuntimeDebug, TypeInfo)] -#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] +#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize, PartialEq, Eq))] pub struct WeightLimit { /// An optional upper limit on the ref time component. pub(crate) ref_time: Option, @@ -239,12 +239,12 @@ impl WeightLimit { /// Subtract `other` from `self` and saturate. Does nothing for *unlimited* components. pub fn saturating_sub(mut self, weight: Weight) -> Self { - self.saturating_decrease(weight); + self.saturating_reduce(weight); self } /// Decrease `self` by `other` and saturate. Does nothing for *unlimited* components. - pub fn saturating_decrease(&mut self, weight: Weight) { + pub fn saturating_reduce(&mut self, weight: Weight) { self.ref_time = self.ref_time.map(|t| t.saturating_sub(weight.ref_time)); self.proof_size = self.proof_size.map(|s| s.saturating_sub(weight.proof_size)); } @@ -261,13 +261,13 @@ impl WeightLimit { } /// Whether `weight` is within the limits of `self`. - pub fn is_within(&self, weight: Weight) -> bool { + pub fn contains(&self, weight: Weight) -> bool { self.all_gte(&weight) } /// Check that `weight` is within the limits of `self`. - pub fn check_within(&self, weight: Weight) -> Result<(), ()> { - self.is_within(weight).then(|| ()).ok_or(()) + pub fn check_contains(&self, weight: Weight) -> Result<(), ()> { + self.contains(weight).then(|| ()).ok_or(()) } } @@ -276,3 +276,354 @@ impl From for WeightLimit { Self::from_weight(w) } } + +#[cfg(test)] +mod tests { + use super::WeightLimit as L; + use crate::Weight; + + #[test] + fn nothing_works() /* pun intended */ + { + assert_eq!(L::NOTHING, L::nothing()); + let nothing = L::NOTHING; + + assert!(!nothing.is_any_unlimited()); + assert!(!nothing.is_all_unlimited()); + assert!(nothing.is_any_limited()); + assert!(nothing.is_all_limited()); + } + + #[test] + fn unlimited_works() { + assert_eq!(L::UNLIMITED, L::unlimited()); + let unlimited = L::UNLIMITED; + + assert!(unlimited.is_any_unlimited()); + assert!(unlimited.is_all_unlimited()); + assert!(!unlimited.is_any_limited()); + assert!(!unlimited.is_all_limited()); + } + + #[test] + fn from_weight_works() { + let l = L::from_weight(Weight::from_parts(u64::MAX, 123)); + assert_eq!(l, L { ref_time: Some(u64::MAX), proof_size: Some(123) }); + assert!(l.is_all_limited()); + } + + #[test] + fn from_some_limits_works() { + let l = L::from_some_limits(u64::MAX, 123); + assert_eq!(l, L { ref_time: Some(u64::MAX), proof_size: Some(123) }); + assert!(l.is_all_limited()); + } + + #[test] + fn from_limits_works() { + let l = L::from_limits(Some(u64::MAX), Some(123)); + assert_eq!(l, L { ref_time: Some(u64::MAX), proof_size: Some(123) }); + assert!(l.is_all_limited()); + + let l = L::from_limits(None, Some(123)); + assert_eq!(l, L { ref_time: None, proof_size: Some(123) }); + assert!(!l.is_all_limited()); + assert!(!l.is_all_unlimited()); + } + + #[test] + fn from_time_limit_works() { + let l = L::from_time_limit(u64::MAX); + assert_eq!(l, L { ref_time: Some(u64::MAX), proof_size: None }); + assert!(l.is_time_limited()); + assert!(l.is_proof_unlimited()); + + let l = L::from_time_limit(0); + assert_eq!(l, L { ref_time: Some(0), proof_size: None }); + assert!(l.is_time_limited()); + assert!(l.is_proof_unlimited()); + } + + #[test] + fn from_proof_limit_works() { + let l = L::from_proof_limit(u64::MAX); + assert_eq!(l, L { ref_time: None, proof_size: Some(u64::MAX) }); + assert!(l.is_time_unlimited()); + assert!(l.is_proof_limited()); + + let l = L::from_proof_limit(0); + assert_eq!(l, L { ref_time: None, proof_size: Some(0) }); + assert!(l.is_time_unlimited()); + assert!(l.is_proof_limited()); + } + + #[test] + fn with_time_limit_works() { + let l = L::UNLIMITED.with_time_limit(u64::MAX).with_time_limit(5); + assert_eq!(l, L::from_time_limit(5)); + + let l = L::UNLIMITED.with_time_limit(2).with_time_limit(u64::MAX); + assert_eq!(l, L::from_time_limit(u64::MAX)); + } + + #[test] + fn with_proof_limit_works() { + let l = L::UNLIMITED.with_proof_limit(u64::MAX).with_proof_limit(5); + assert_eq!(l, L::from_proof_limit(5)); + + let l = L::UNLIMITED.with_proof_limit(2).with_proof_limit(u64::MAX); + assert_eq!(l, L::from_proof_limit(u64::MAX)); + } + + #[test] + fn with_limit_works() { + let l = L::UNLIMITED.with_time_limit(10).with_proof_limit(5); + let m = L::UNLIMITED.with_proof_limit(5).with_time_limit(10); + assert_eq!(l, m, "Order does not matter"); + assert_eq!(m, L::from_some_limits(10, 5)); + } + + #[test] + fn time_limit_works() { + assert_eq!(L::from_time_limit(5).time_limit(), Some(5)); + assert_eq!(L::from_time_limit(u64::MAX).time_limit(), Some(u64::MAX)); + } + + #[test] + fn proof_limit_works() { + assert_eq!(L::from_proof_limit(5).proof_limit(), Some(5)); + assert_eq!(L::from_proof_limit(u64::MAX).proof_limit(), Some(u64::MAX)); + } + + #[test] + fn comparison_basic_works() { + let l = L::from_some_limits(1, 2); + let m = Weight::from_parts(1, 1); + + assert!(l.all_gte(&m)); + assert!(l.any_gte(&m)); + assert!(!l.all_gt(&m)); + assert!(l.any_gt(&m)); + assert!(!l.all_lte(&m)); + assert!(l.any_lte(&m)); + assert!(!l.all_lt(&m)); + assert!(!l.any_lt(&m)); + + let l = l.exact_limits().unwrap(); + let m = L::from_weight(m); + + assert!(!m.all_gte(&l)); + assert!(m.any_gte(&l)); + assert!(!m.all_gt(&l)); + assert!(!m.any_gt(&l)); + assert!(m.all_lte(&l)); + assert!(m.any_lte(&l)); + assert!(!m.all_lt(&l)); + assert!(m.any_lt(&l)); + } + + #[test] + fn comparison_works_same_as_weight() { + for (x, y) in good_values() { + let l = L::from_weight(x); + + assert_eq!(x.all_gte(y), l.all_gte(&y)); + assert_eq!(x.all_gt(y), l.all_gt(&y)); + assert_eq!(x.all_lte(y), l.all_lte(&y)); + assert_eq!(x.all_lt(y), l.all_lt(&y)); + + assert_eq!(x.any_gte(y), l.any_gte(&y)); + assert_eq!(x.any_gt(y), l.any_gt(&y)); + assert_eq!(x.any_lte(y), l.any_lte(&y)); + assert_eq!(x.any_lt(y), l.any_lt(&y)); + } + } + + #[test] + fn limited_or_works() { + assert_eq!(L::NOTHING.limited_or(Weight::from_parts(10, 5)), Weight::zero()); + assert_eq!(L::NOTHING.limited_or(Weight::MAX), Weight::zero()); + + assert_eq!(L::UNLIMITED.limited_or(Weight::from_parts(10, 5)), Weight::from_parts(10, 5)); + assert_eq!(L::UNLIMITED.limited_or(Weight::MAX), Weight::MAX); + + let l = L::from_time_limit(10); + let m = L::from_proof_limit(5); + assert_eq!(l.limited_or(Weight::from_parts(0, 5)), m.limited_or(Weight::from_parts(10, 0))); + } + + #[test] + fn limited_or_max_works() { + assert_eq!(L::UNLIMITED.limited_or_max(), L::UNLIMITED.limited_or(Weight::MAX)); + assert_eq!(L::UNLIMITED.limited_or_max(), Weight::MAX); + assert_eq!(L::NOTHING.limited_or_max(), Weight::zero()); + + let l = L::from_time_limit(5); + assert_eq!(l.limited_or_max(), Weight::from_parts(5, u64::MAX)); + let m = L::from_proof_limit(5); + assert_eq!(m.limited_or_max(), Weight::from_parts(u64::MAX, 5)); + } + + #[test] + fn limited_or_min_works() { + assert_eq!(L::UNLIMITED.limited_or_min(), L::UNLIMITED.limited_or(Weight::zero())); + assert_eq!(L::UNLIMITED.limited_or_min(), Weight::zero()); + assert_eq!(L::NOTHING.limited_or_min(), Weight::zero()); + + let l = L::from_time_limit(5); + assert_eq!(l.limited_or_min(), Weight::from_parts(5, 0)); + let m = L::from_proof_limit(5); + assert_eq!(m.limited_or_min(), Weight::from_parts(0, 5)); + } + + #[test] + fn saturating_sub_works() { + assert_eq!(L::UNLIMITED.saturating_sub(Weight::from_parts(1, 2)), L::UNLIMITED); + assert_eq!(L::UNLIMITED.saturating_sub(Weight::MAX), L::UNLIMITED); + + assert_eq!(L::NOTHING.saturating_sub(Weight::from_parts(1, 2)), L::NOTHING); + assert_eq!(L::NOTHING.saturating_sub(Weight::MAX), L::NOTHING); + + assert_eq!( + L::from_time_limit(10).saturating_sub(Weight::from_parts(5, 10)), + L::from_time_limit(5) + ); + assert_eq!( + L::from_time_limit(10).saturating_sub(Weight::from_parts(50, 0)), + L::from_time_limit(0) + ); + + assert_eq!( + L::from_proof_limit(10).saturating_sub(Weight::from_parts(10, 5)), + L::from_proof_limit(5) + ); + assert_eq!( + L::from_proof_limit(10).saturating_sub(Weight::from_parts(0, 50)), + L::from_proof_limit(0) + ); + + assert_eq!(L::from_weight(Weight::MAX).saturating_sub(Weight::MAX), L::NOTHING); + } + + #[test] + fn saturating_reduce_works() { + let mut l = L::UNLIMITED; + l.saturating_reduce(Weight::from_parts(1, 2)); + assert_eq!(l, L::UNLIMITED); + + let mut l = L::UNLIMITED; + l.saturating_reduce(Weight::MAX); + assert_eq!(l, L::UNLIMITED); + + let mut l = L::NOTHING; + l.saturating_reduce(Weight::from_parts(1, 2)); + assert_eq!(l, L::NOTHING); + let mut l = L::NOTHING; + l.saturating_reduce(Weight::MAX); + assert_eq!(l, L::NOTHING); + + let mut l = L::from_time_limit(10); + l.saturating_reduce(Weight::from_parts(5, 10)); + assert_eq!(l, L::from_time_limit(5)); + let mut l = L::from_time_limit(10); + l.saturating_reduce(Weight::from_parts(50, 0)); + assert_eq!(l, L::from_time_limit(0)); + + let mut l = L::from_proof_limit(10); + l.saturating_reduce(Weight::from_parts(10, 5)); + assert_eq!(l, L::from_proof_limit(5)); + + let mut l = L::from_proof_limit(10); + l.saturating_reduce(Weight::from_parts(0, 50)); + assert_eq!(l, L::from_proof_limit(0)); + + let mut l = L::from_weight(Weight::MAX); + l.saturating_reduce(Weight::MAX); + assert_eq!(l, L::NOTHING); + } + + #[test] + fn exact_limits_works() { + assert!(L::UNLIMITED.exact_limits().is_none()); + assert_eq!(L::NOTHING.exact_limits(), Some(Weight::zero())); + + assert!(L::from_time_limit(5).exact_limits().is_none()); + assert!(L::from_proof_limit(5).exact_limits().is_none()); + + assert_eq!( + L::from_proof_limit(5).with_time_limit(10).exact_limits(), + Some(Weight::from_parts(10, 5)) + ); + assert_eq!(L::from_limits(Some(2), Some(1)).exact_limits(), Some(Weight::from_parts(2, 1))); + assert!(L::from_limits(None, Some(1)).exact_limits().is_none()); + assert!(L::from_limits(Some(2), None).exact_limits().is_none()); + assert_eq!(L::from_some_limits(2, 1).exact_limits(), Some(Weight::from_parts(2, 1))); + } + + #[test] + fn contains_works() { + assert!(L::NOTHING.contains(Weight::zero())); + assert!(!L::NOTHING.contains(Weight::from_parts(1, 0))); + assert!(!L::NOTHING.contains(Weight::from_parts(0, 1))); + assert!(!L::NOTHING.contains(Weight::MAX)); + + assert!(L::UNLIMITED.contains(Weight::zero())); + assert!(L::UNLIMITED.contains(Weight::from_parts(u64::MAX, 0))); + assert!(L::UNLIMITED.contains(Weight::from_parts(0, u64::MAX))); + assert!(L::UNLIMITED.contains(Weight::MAX)); + + assert!(L::from_time_limit(10).contains(Weight::zero())); + assert!(L::from_time_limit(10).contains(Weight::from_parts(1, 200))); + assert!(L::from_time_limit(10).contains(Weight::from_parts(10, u64::MAX))); + assert!(!L::from_time_limit(10).contains(Weight::from_parts(11, 0))); + + assert!(L::from_proof_limit(10).contains(Weight::zero())); + assert!(L::from_proof_limit(10).contains(Weight::from_parts(200, 1))); + assert!(L::from_proof_limit(10).contains(Weight::from_parts(u64::MAX, 10))); + assert!(!L::from_proof_limit(10).contains(Weight::from_parts(0, 11))); + } + + #[test] + fn check_contains_works_same_as_contains() { + for (x, y) in good_values() { + let l = L::from_weight(x); + assert_eq!(l.contains(y), l.check_contains(y).is_ok()); + } + } + + #[test] + fn check_contains_works() { + assert!(L::NOTHING.check_contains(Weight::zero()).is_ok()); + assert!(!L::NOTHING.check_contains(Weight::from_parts(1, 0)).is_ok()); + assert!(!L::NOTHING.check_contains(Weight::from_parts(0, 1)).is_ok()); + assert!(!L::NOTHING.check_contains(Weight::MAX).is_ok()); + + assert!(L::UNLIMITED.check_contains(Weight::zero()).is_ok()); + assert!(L::UNLIMITED.check_contains(Weight::from_parts(u64::MAX, 0)).is_ok()); + assert!(L::UNLIMITED.check_contains(Weight::from_parts(0, u64::MAX)).is_ok()); + assert!(L::UNLIMITED.check_contains(Weight::MAX).is_ok()); + + assert!(L::from_time_limit(10).check_contains(Weight::zero()).is_ok()); + assert!(L::from_time_limit(10).check_contains(Weight::from_parts(1, 200)).is_ok()); + assert!(L::from_time_limit(10).check_contains(Weight::from_parts(10, u64::MAX)).is_ok()); + assert!(!L::from_time_limit(10).check_contains(Weight::from_parts(11, 0)).is_ok()); + + assert!(L::from_proof_limit(10).check_contains(Weight::zero()).is_ok()); + assert!(L::from_proof_limit(10).check_contains(Weight::from_parts(200, 1)).is_ok()); + assert!(L::from_proof_limit(10).check_contains(Weight::from_parts(u64::MAX, 10)).is_ok()); + assert!(!L::from_proof_limit(10).check_contains(Weight::from_parts(0, 11)).is_ok()); + } + + /// Some known-good weight pairs. + fn good_values() -> Vec<(Weight, Weight)> { + let c = vec![0, 1, 1000, u64::MAX - 1, u64::MAX]; + let weights = c + .iter() + .flat_map(|t| c.iter().map(|p| Weight::from_parts(*t, *p))) + .collect::>(); + weights + .iter() + .flat_map(|x| weights.iter().map(|y| (*x, *y))) + .collect::>() + } +} From efcf8e38840a3cb06ac0a1b78830d1f6540e5477 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 21:31:03 +0100 Subject: [PATCH 19/31] Add helpers Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/limit.rs | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index 457b8bbb33f31..3c599dd585d1c 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -122,6 +122,10 @@ impl WeightLimit { self.ref_time.is_none() } + pub fn is_any_zero(&self) -> bool { + self.ref_time == Some(0) || self.proof_size == Some(0) + } + /// Whether the proof is unlimited. pub const fn is_proof_unlimited(&self) -> bool { self.proof_size.is_none() @@ -199,6 +203,23 @@ impl WeightLimit { self.proof_size.map_or(false, |limit| limit < weight.proof_size) } + /// Compare `self` with `other` and return `true` if all limits of `self` are greater-or-equal. + pub fn all_above_or_equal(&self, other: &Self) -> bool { + self.ref_time.map_or(true, |limit| limit >= other.ref_time.unwrap_or(u64::MAX)) && + self.proof_size + .map_or(true, |limit| limit >= other.proof_size.unwrap_or(u64::MAX)) + } + + pub fn all_less_or_equal(&self, other: &Self) -> bool { + self.ref_time.map_or_else( + || other.ref_time.is_none(), + |limit| other.ref_time.map_or(true, |other_limit| limit <= other_limit), + ) && self.proof_size.map_or_else( + || other.proof_size.is_none(), + |limit| other.proof_size.map_or(true, |other_limit| limit <= other_limit), + ) + } + /// Uses the exact value for each *limited* component and `w` for each unlimited one. /// /// # Example @@ -269,6 +290,27 @@ impl WeightLimit { pub fn check_contains(&self, weight: Weight) -> Result<(), ()> { self.contains(weight).then(|| ()).ok_or(()) } + + pub fn map_proof_limit(self, f: F) -> Self + where + F: FnOnce(Option) -> Option, + { + Self { ref_time: self.ref_time, proof_size: f(self.proof_size) } + } + + pub fn map_time_limit(self, f: F) -> Self + where + F: FnOnce(Option) -> Option, + { + Self { ref_time: f(self.ref_time), proof_size: self.proof_size } + } + + pub fn map_limits(self, f: F) -> Self + where + F: Fn(Option) -> Option, + { + Self { ref_time: f(self.ref_time), proof_size: f(self.proof_size) } + } } impl From for WeightLimit { @@ -614,6 +656,18 @@ mod tests { assert!(!L::from_proof_limit(10).check_contains(Weight::from_parts(0, 11)).is_ok()); } + #[test] + fn all_less_or_equal_works() { + assert!(L::UNLIMITED.all_less_or_equal(&L::UNLIMITED)); + assert!(L::NOTHING.all_less_or_equal(&L::UNLIMITED)); + + let l = L::from_some_limits(10, 5); + let m = L::from_some_limits(5, 10); + assert!(l.all_less_or_equal(&l)); + assert!(!l.all_less_or_equal(&m)); + assert!(!m.all_less_or_equal(&l)); + } + /// Some known-good weight pairs. fn good_values() -> Vec<(Weight, Weight)> { let c = vec![0, 1, 1000, u64::MAX - 1, u64::MAX]; From c29a16511ddcbae8881c70f29818227f615268ec Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 21:59:49 +0100 Subject: [PATCH 20/31] Tx-payment pallet: Take Soft limit into account Signed-off-by: Oliver Tale-Yazdi --- frame/transaction-payment/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 98e15cbd80f77..a8b5d58906192 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -405,11 +405,9 @@ pub mod pallet { T::BlockWeights::get() .get(DispatchClass::Normal) .max_total + .with_proof_limit(frame_system::SOFT_POV_LIMIT_BYTES) // FAIL-CI double check .exact_limits() - .expect( - "Setting `max_total` for `Normal` dispatch class is not compatible with \ - `transaction-payment` pallet.", - ); + .expect("Transaction-payment needs exact weight limit; qed"); // add 1 percent; let addition = target / 100; if addition == Weight::zero() { From a34c9458152602ab857e68bbf9acc7aa01cfec22 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 18 Jan 2023 22:01:26 +0100 Subject: [PATCH 21/31] Make stuff work Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/limits.rs | 93 ++++++++++++++++++++++++++------- frame/system/src/mock.rs | 2 +- primitives/weights/src/limit.rs | 4 ++ 3 files changed, 80 insertions(+), 19 deletions(-) diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index d54eccb944091..7d99ffdd8e3a3 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -200,8 +200,12 @@ pub struct WeightsPerClass { #[derive(RuntimeDebug, Clone, codec::Encode, codec::Decode, TypeInfo)] pub struct BlockWeights { /// Base weight of block execution. + /// + /// This is a fixed constant and not a *maximum* value. pub base_block: Weight, /// Maximal total weight consumed by all kinds of extrinsics (without `reserved` space). + /// + /// This is not set manually but calculated by the builder. pub max_block: Weight, /// Weight limits for extrinsics of given dispatch class. pub per_class: PerDispatchClass, @@ -228,14 +232,14 @@ impl BlockWeights { for class in DispatchClass::all() { let weights = self.per_class.get(*class); - let max_for_class = weights.max_total.limited_or_max(); + let max_for_class = weights.max_total; let base_for_class = weights.base_extrinsic; - let reserved = weights.reserved.limited_or_max(); + let reserved = weights.reserved; // Make sure that if total is set it's greater than base_block && // base_for_class error_assert!( - (max_for_class.all_gt(self.base_block) && max_for_class.all_gt(base_for_class)) - || max_for_class == Weight::zero(), + (max_for_class.all_gt(&self.base_block) && max_for_class.all_gt(&base_for_class)) + || max_for_class.is_all_zero(), &mut error, "[{:?}] {:?} (total) has to be greater than {:?} (base block) & {:?} (base extrinsic)", class, max_for_class, self.base_block, base_for_class, @@ -244,24 +248,28 @@ impl BlockWeights { error_assert!( weights .max_extrinsic - .limited_or_min() - .all_lte(max_for_class.saturating_sub(base_for_class)), + .limited_or(Weight::zero()) + .all_lte(max_for_class.saturating_sub(base_for_class).limited_or_max()), + /*weights // FAIL-CI why does this not work? + .max_extrinsic + .all_less_or_equal(&max_for_class.saturating_sub(base_for_class)),*/ &mut error, "[{:?}] {:?} (max_extrinsic) can't be greater than {:?} (max for class)", class, weights.max_extrinsic, max_for_class.saturating_sub(base_for_class), ); - // Max extrinsic must non be zero in any component. + // Max extrinsic must allow for some computation time. Proof size could legitimately + // be 0. error_assert!( - weights.max_extrinsic.all_gt(&Weight::zero()), + weights.max_extrinsic.time_limit().unwrap_or(1) > 0, &mut error, - "[{:?}] {:?} (max_extrinsic) must not be 0. Check base cost and average initialization cost.", + "[{:?}] {:?} (max_extrinsic) time limit must not be 0. Check base cost and average initialization cost.", class, weights.max_extrinsic, ); // Make sure that if reserved is set it's greater than base_for_class. error_assert!( - reserved.all_gt(base_for_class) || reserved == Weight::zero(), + reserved.all_gt(&base_for_class) || reserved.is_any_zero(), &mut error, "[{:?}] {:?} (reserved) has to be greater than {:?} (base extrinsic) if set", class, @@ -270,7 +278,7 @@ impl BlockWeights { ); // Make sure max block is greater than max_total if it's set. error_assert!( - self.max_block.all_gte(weights.max_total.limited_or_min()), + self.max_block.all_gte(weights.max_total.limited_or(Weight::zero())), &mut error, "[{:?}] {:?} (max block) has to be greater than {:?} (max for class)", class, @@ -279,7 +287,8 @@ impl BlockWeights { ); // Make sure we can fit at least one extrinsic. error_assert!( - self.max_block.all_gt(base_for_class + self.base_block), + // FAIL-CI intended logic change + self.max_block.all_gte(base_for_class + self.base_block), &mut error, "[{:?}] {:?} (max block) must fit at least one extrinsic {:?} (base weight)", class, @@ -409,24 +418,33 @@ impl BlockWeightsBuilder { // compute max block size. for class in DispatchClass::all() { - // FAIL-CI why is this not just maxing to unlimited aka `limited_or_max`? + // FAIL-CI why is this min weights.max_block = - weights.per_class.get(*class).max_total.limited_or_min().max(weights.max_block); + weights.max_block.max(weights.per_class.get(*class).max_total.limited_or_min()); } // compute max size of single extrinsic if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); + // FAIL-CI cleanup if per_class.max_extrinsic.is_time_unlimited() && init_cost.is_some() { per_class.max_extrinsic = per_class .max_total .saturating_sub(init_weight.without_proof_size()) .saturating_sub(per_class.base_extrinsic.without_proof_size()); } - if per_class.max_extrinsic.is_proof_unlimited() && init_cost.is_some() { - per_class.max_extrinsic = per_class - .max_total - .saturating_sub(init_weight.without_ref_time()) + if per_class.max_extrinsic.is_proof_unlimited() && + init_cost.is_some() && (per_class.max_extrinsic.is_time_unlimited() && + init_cost.is_some()) + { + per_class.max_extrinsic.saturating_reduce(init_weight.without_ref_time()); + per_class + .max_extrinsic + .saturating_reduce(per_class.base_extrinsic.without_ref_time()); + } else if per_class.max_extrinsic.is_proof_unlimited() && init_cost.is_some() { + per_class.max_extrinsic.saturating_reduce(init_weight.without_ref_time()); + per_class + .max_extrinsic .saturating_sub(per_class.base_extrinsic.without_ref_time()); } } @@ -449,9 +467,48 @@ impl BlockWeightsBuilder { #[cfg(test)] mod tests { use super::*; + use crate::limits::BlockWeights; + use frame_support::weights::WeightLimit; + use sp_runtime::Perbill; #[test] fn default_weights_are_valid() { BlockWeights::default().validate().unwrap(); } + + // Copied from Polkadot. + const BLOCK_WEIGHT_LIMIT: WeightLimit = WeightLimit::from_time_limit(2000000000000); + const AVERAGE_ON_INITIALIZE_RATIO: Perbill = Perbill::from_percent(1); + pub const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75); + + #[test] + fn block_weight_works() { + let weights = BlockWeights::builder() + .base_block(Weight::from_parts(6103588000, 0)) + .for_class(DispatchClass::all(), |weights| { + weights.base_extrinsic = Weight::from_ref_time(95479000); + }) + .for_class(DispatchClass::Normal, |weights| { + weights.max_total = BLOCK_WEIGHT_LIMIT.map_limits(|limit| match limit { + // This case means that the component (time or proof) is not limited. We keep it + // that way by mapping to `None`. + None => None, + // Here we linearly scale the concrete limit down. + Some(limit) => Some(NORMAL_DISPATCH_RATIO * limit), + }); + }) + .for_class(DispatchClass::Operational, |weights| { + weights.max_total = BLOCK_WEIGHT_LIMIT.into(); + // Operational transactions have an extra reserved space, so that they + // are included even if block reached `BLOCK_WEIGHT_LIMIT`. + weights.reserved = BLOCK_WEIGHT_LIMIT.map_limits(|limit| match limit { + None => None, + Some(limit) => Some(limit - NORMAL_DISPATCH_RATIO * limit), + }); + }) + .avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO) + .build_or_panic(); + + assert_eq!(weights.max_block, Weight::from_parts(2000000000000, 0)); + } } diff --git a/frame/system/src/mock.rs b/frame/system/src/mock.rs index b1f3abff3a20f..4a1b00d503698 100644 --- a/frame/system/src/mock.rs +++ b/frame/system/src/mock.rs @@ -41,7 +41,7 @@ frame_support::construct_runtime!( ); const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75); -const MAX_BLOCK_WEIGHT: Weight = Weight::from_ref_time(1024).set_proof_size(u64::MAX); +const MAX_BLOCK_WEIGHT: Weight = Weight::from_parts(1024, u64::MAX); parameter_types! { pub Version: RuntimeVersion = RuntimeVersion { diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index 3c599dd585d1c..36d7068da3460 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -126,6 +126,10 @@ impl WeightLimit { self.ref_time == Some(0) || self.proof_size == Some(0) } + pub fn is_all_zero(&self) -> bool { + self.ref_time == Some(0) || self.proof_size == Some(0) + } + /// Whether the proof is unlimited. pub const fn is_proof_unlimited(&self) -> bool { self.proof_size.is_none() From b30cfdd215121efe162f4d47ed72b5d91b21a5b0 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 16:37:54 +0100 Subject: [PATCH 22/31] Make soft limit configurable Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/lib.rs | 15 +++++++-------- frame/system/src/limits.rs | 26 ++++++++++++++++++++++++-- frame/system/src/mock.rs | 1 + frame/system/src/tests.rs | 11 +++++------ frame/transaction-payment/src/lib.rs | 9 +++------ 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 89e2a6b2faa50..ecada914bc840 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -165,10 +165,6 @@ impl SetCode for () { } } -/// Soft limit for the PoV weight of a block. Above this limit a warning will be printed and an -/// [`Event::PovLimitExceeded`] will be emitted. -pub const SOFT_POV_LIMIT_BYTES: u64 = 5 * 1024 * 1024; - /// Numeric limits over the ability to add a consumer ref using `inc_consumers`. pub trait ConsumerLimits { /// The number of consumers over which `inc_consumers` will cease to work. @@ -1348,18 +1344,21 @@ impl Pallet { /// resulting header for this block. pub fn finalize() -> T::Header { // Check the soft PoV weight limit. + let limits = T::BlockWeights::get(); + let limit = limits.pov_soft_limit.unwrap_or(limits.max_block.proof_size()); let consumed = BlockWeight::::get().total().proof_size(); - if consumed > SOFT_POV_LIMIT_BYTES { - let percent = (consumed as f32 * 100.0) / (SOFT_POV_LIMIT_BYTES as f32) - 100.0; + + if consumed > limit { + let percent = (consumed as f32 * 100.0) / (limit as f32) - 100.0; log::warn!( target: LOG_TARGET, "Block {:?} exceeded the PoV limit by {:.2}%; {:.2} > {:.2} MiB", Self::block_number(), percent, consumed as f32 / (1024 * 1024) as f32, - SOFT_POV_LIMIT_BYTES as f32 / (1024 * 1024) as f32, + limit as f32 / (1024 * 1024) as f32, ); - Self::deposit_event(Event::PovLimitExceeded { limit: SOFT_POV_LIMIT_BYTES, consumed }); + Self::deposit_event(Event::PovLimitExceeded { limit, consumed }); } log::debug!( diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 7d99ffdd8e3a3..352975ebf99a2 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -25,6 +25,7 @@ //! `DispatchClass`. This module contains configuration object for both resources, //! which should be passed to `frame_system` configuration when runtime is being set up. +use crate::limits::constants::WEIGHT_PROOF_SIZE_PER_MB; use frame_support::{ dispatch::{DispatchClass, OneOrMany, PerDispatchClass}, weights::{constants, Weight}, @@ -97,6 +98,7 @@ const DEFAULT_NORMAL_RATIO: Perbill = Perbill::from_percent(75); /// `DispatchClass`-specific weight configuration. #[derive(RuntimeDebug, Clone, codec::Encode, codec::Decode, TypeInfo)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq))] pub struct WeightsPerClass { /// Base weight of single extrinsic of given class. pub base_extrinsic: Weight, @@ -198,6 +200,7 @@ pub struct WeightsPerClass { /// As a consequence of `reserved` space, total consumed block weight might exceed `max_block` /// value, so this parameter should rather be thought of as "target block weight" than a hard limit. #[derive(RuntimeDebug, Clone, codec::Encode, codec::Decode, TypeInfo)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq))] pub struct BlockWeights { /// Base weight of block execution. /// @@ -207,6 +210,8 @@ pub struct BlockWeights { /// /// This is not set manually but calculated by the builder. pub max_block: Weight, + // none means unlimited. + pub pov_soft_limit: Option, /// Weight limits for extrinsics of given dispatch class. pub per_class: PerDispatchClass, } @@ -214,7 +219,7 @@ pub struct BlockWeights { impl Default for BlockWeights { fn default() -> Self { Self::with_sensible_defaults( - Weight::from_parts(constants::WEIGHT_REF_TIME_PER_SECOND, u64::MAX), + Weight::from_parts(constants::WEIGHT_REF_TIME_PER_SECOND, 5 * WEIGHT_PROOF_SIZE_PER_MB), DEFAULT_NORMAL_RATIO, ) } @@ -296,6 +301,14 @@ impl BlockWeights { base_for_class + self.base_block, ); } + // If there is a soft PoV limit, it must not exceed `max_block`. + error_assert!( + self.pov_soft_limit.map_or(true, |l| l <= self.max_block.proof_size()), + &mut error, + "PoV soft limit {:?} must be smaller than max block size {:?}", + self.pov_soft_limit, + self.max_block.proof_size(), + ); if error.has_errors { Err(error) @@ -364,6 +377,7 @@ impl BlockWeights { reserved: initial, } }), + pov_soft_limit: None, }, init_cost: None, } @@ -396,6 +410,14 @@ impl BlockWeightsBuilder { self } + /// Set a soft pov limit. This is the max PoV size that you would *expect* without hard limiting it. + /// + /// Setting this to something higher than `max_block` will error. + pub fn pov_soft_limit(mut self, pov_soft_limit: u64) -> Self { + self.weights.pov_soft_limit = Some(pov_soft_limit); + self + } + /// Set parameters for particular class. /// /// Note: `None` values of `max_extrinsic` will be overwritten in `build` in case @@ -509,6 +531,6 @@ mod tests { .avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO) .build_or_panic(); - assert_eq!(weights.max_block, Weight::from_parts(2000000000000, 0)); + assert_eq!(weights.max_block, Weight::from_parts(2000000000000, u64::MAX)); } } diff --git a/frame/system/src/mock.rs b/frame/system/src/mock.rs index 4a1b00d503698..2ea578dab0748 100644 --- a/frame/system/src/mock.rs +++ b/frame/system/src/mock.rs @@ -74,6 +74,7 @@ parameter_types! { ).into(); }) .avg_block_initialization(Perbill::from_percent(0)) + .pov_soft_limit(5 * 1024 * 1024) .build_or_panic(); pub RuntimeBlockLength: limits::BlockLength = limits::BlockLength::max_with_normal_ratio(1024, NORMAL_DISPATCH_RATIO); diff --git a/frame/system/src/tests.rs b/frame/system/src/tests.rs index 4477716b52714..fbdcda773288b 100644 --- a/frame/system/src/tests.rs +++ b/frame/system/src/tests.rs @@ -635,12 +635,13 @@ fn events_not_emitted_during_genesis() { }); } -/// Consuming more than `SOFT_POV_LIMIT_BYTES` should result in a `PovLimitExceeded` event. +/// Consuming more than the soft PoV limit should result in a `PovLimitExceeded` event. #[test] fn event_emitted_when_over_pov_limit() { + let limit = ::BlockWeights::get().pov_soft_limit.unwrap(); new_test_ext().execute_with(|| { for i in 0..10 { - let consumed = SOFT_POV_LIMIT_BYTES + i - 5; + let consumed = limit + i - 5; System::reset_events(); System::initialize(&1, &[0u8; 32].into(), &Default::default()); @@ -650,10 +651,8 @@ fn event_emitted_when_over_pov_limit() { System::note_finished_extrinsics(); System::finalize(); - if consumed > SOFT_POV_LIMIT_BYTES { - System::assert_has_event( - Event::PovLimitExceeded { limit: SOFT_POV_LIMIT_BYTES, consumed }.into(), - ); + if consumed > limit { + System::assert_has_event(Event::PovLimitExceeded { limit, consumed }.into()); } else { assert!(System::events().is_empty(), "No events"); } diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index a8b5d58906192..0d672cccd3aae 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -401,13 +401,10 @@ pub mod pallet { .unwrap(), ); + // Could be that there is no `max_total` for class `Normal` since it can be unlimited. + // We therefore fall back to `u64::MAX`. let target = T::FeeMultiplierUpdate::target() * - T::BlockWeights::get() - .get(DispatchClass::Normal) - .max_total - .with_proof_limit(frame_system::SOFT_POV_LIMIT_BYTES) // FAIL-CI double check - .exact_limits() - .expect("Transaction-payment needs exact weight limit; qed"); + T::BlockWeights::get().get(DispatchClass::Normal).max_total.limited_or_max(); // add 1 percent; let addition = target / 100; if addition == Weight::zero() { From 0200b34f76711cca1c9a75abf93d72e921393865 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 16:38:55 +0100 Subject: [PATCH 23/31] Exclude Mandatory class from max_block Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/limits.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 352975ebf99a2..6309fcb8f7dd8 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -440,9 +440,15 @@ impl BlockWeightsBuilder { // compute max block size. for class in DispatchClass::all() { - // FAIL-CI why is this min + // This is kind of a hack, but since the mandatory needs to happen *anyway*, we can + // exclude it from the regular `max_block`. The reason is that you normally want + // `unlimited` mandatory, which will always lead to a `max_block` of `(u64::MAX, + // u64::MAX)` as well. + if class == &DispatchClass::Mandatory { + continue + } weights.max_block = - weights.max_block.max(weights.per_class.get(*class).max_total.limited_or_min()); + weights.max_block.max(weights.per_class.get(*class).max_total.limited_or_max()); } // compute max size of single extrinsic if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { From 2aa0d0bb278fd9d3a76a20ab076a0403c6dc325f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 16:43:19 +0100 Subject: [PATCH 24/31] fmt Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/limits.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 6309fcb8f7dd8..8c18caccbaed1 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -410,7 +410,8 @@ impl BlockWeightsBuilder { self } - /// Set a soft pov limit. This is the max PoV size that you would *expect* without hard limiting it. + /// Set a soft pov limit. This is the max PoV size that you would *expect* without hard limiting + /// it. /// /// Setting this to something higher than `max_block` will error. pub fn pov_soft_limit(mut self, pov_soft_limit: u64) -> Self { From b9159adb477468aa8ffdce61c69616c575f19e24 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 18:14:05 +0100 Subject: [PATCH 25/31] cleanup Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/extensions/check_weight.rs | 6 +++--- frame/system/src/limits.rs | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 650d52f822d62..6257dfa29c79b 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -672,11 +672,11 @@ mod tests { .base_block(Weight::zero()) .for_class(DispatchClass::non_mandatory(), |w| { w.base_extrinsic = Weight::zero(); - w.max_total = Weight::from_ref_time(20).set_proof_size(20000).into(); + w.max_total = Weight::from_parts(20, u64::MAX).into(); }) .for_class(DispatchClass::Mandatory, |w| { w.base_extrinsic = Weight::zero(); - w.reserved = Weight::from_ref_time(5).set_proof_size(20000).into(); + w.reserved = Weight::from_parts(5, u64::MAX).into(); w.max_total = WeightLimit::UNLIMITED; }) .build_or_panic(); @@ -685,7 +685,7 @@ mod tests { DispatchClass::Operational => Weight::from_ref_time(10), DispatchClass::Mandatory => Weight::zero(), }); - assert_eq!(maximum_weight.max_block, all_weight.total().set_proof_size(20000)); + assert_eq!(maximum_weight.max_block, all_weight.total().set_proof_size(u64::MAX)); // fits into reserved let mandatory1 = DispatchInfo { diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 8c18caccbaed1..5d058728cdbc0 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -255,9 +255,6 @@ impl BlockWeights { .max_extrinsic .limited_or(Weight::zero()) .all_lte(max_for_class.saturating_sub(base_for_class).limited_or_max()), - /*weights // FAIL-CI why does this not work? - .max_extrinsic - .all_less_or_equal(&max_for_class.saturating_sub(base_for_class)),*/ &mut error, "[{:?}] {:?} (max_extrinsic) can't be greater than {:?} (max for class)", class, From fb344c0c6ef43b3adeac82cb865cf69419b2d8d4 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 20:01:06 +0100 Subject: [PATCH 26/31] fix? Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/limits.rs | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index 5d058728cdbc0..e58096feb5d12 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -452,26 +452,19 @@ impl BlockWeightsBuilder { if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); - // FAIL-CI cleanup - if per_class.max_extrinsic.is_time_unlimited() && init_cost.is_some() { - per_class.max_extrinsic = per_class - .max_total - .saturating_sub(init_weight.without_proof_size()) - .saturating_sub(per_class.base_extrinsic.without_proof_size()); - } - if per_class.max_extrinsic.is_proof_unlimited() && - init_cost.is_some() && (per_class.max_extrinsic.is_time_unlimited() && - init_cost.is_some()) - { - per_class.max_extrinsic.saturating_reduce(init_weight.without_ref_time()); + let before = per_class.max_extrinsic; + if per_class.max_extrinsic.is_any_unlimited() { + per_class.max_extrinsic = per_class.max_total; + + per_class.max_extrinsic.saturating_reduce(init_weight.without_proof_size()); per_class .max_extrinsic - .saturating_reduce(per_class.base_extrinsic.without_ref_time()); - } else if per_class.max_extrinsic.is_proof_unlimited() && init_cost.is_some() { + .saturating_reduce(per_class.base_extrinsic.without_proof_size()); + per_class.max_extrinsic.saturating_reduce(init_weight.without_ref_time()); per_class .max_extrinsic - .saturating_sub(per_class.base_extrinsic.without_ref_time()); + .saturating_reduce(per_class.base_extrinsic.without_ref_time()); } } } From 8053ee4636c72fc25767612ee18184b6bb3ccaff Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 20:09:12 +0100 Subject: [PATCH 27/31] fix Signed-off-by: Oliver Tale-Yazdi --- frame/system/src/extensions/check_weight.rs | 2 +- frame/system/src/limits.rs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 6257dfa29c79b..c3eba1b8ccd11 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -314,7 +314,7 @@ mod tests { .get(DispatchClass::Normal) .max_extrinsic .exact_limits() - .unwrap() + Weight::from_ref_time(1), // FAIL-CI + .unwrap() + Weight::from_ref_time(1), class: DispatchClass::Normal, ..Default::default() }; diff --git a/frame/system/src/limits.rs b/frame/system/src/limits.rs index e58096feb5d12..ee5768659c6be 100644 --- a/frame/system/src/limits.rs +++ b/frame/system/src/limits.rs @@ -104,8 +104,8 @@ pub struct WeightsPerClass { pub base_extrinsic: Weight, /// Maximal weight of single extrinsic. Should NOT include `base_extrinsic` cost. /// - /// FAIL-CI fix docs - /// `UNLIMITED` indicates that this class of extrinsics doesn't have a limit. + /// `UNLIMITED` indicates that this class of extrinsics doesn't have a limit. This will be + /// calculated by the builder if not specified. pub max_extrinsic: WeightLimit, /// Block maximal total weight for all extrinsics of given class. /// @@ -118,7 +118,7 @@ pub struct WeightsPerClass { pub max_total: WeightLimit, /// Block reserved allowance for all extrinsics of a particular class. /// - /// Setting to `None` indicates that extrinsics of that class are allowed + /// Setting to `UNLIMITED` indicates that extrinsics of that class are allowed /// to go over total block weight (but at most `max_total` for that class). /// Setting to `Some(x)` guarantees that at least `x` weight of particular class /// is processed in every block. @@ -289,8 +289,7 @@ impl BlockWeights { ); // Make sure we can fit at least one extrinsic. error_assert!( - // FAIL-CI intended logic change - self.max_block.all_gte(base_for_class + self.base_block), + self.max_block.all_gt(base_for_class + self.base_block), &mut error, "[{:?}] {:?} (max block) must fit at least one extrinsic {:?} (base weight)", class, @@ -452,7 +451,7 @@ impl BlockWeightsBuilder { if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) { for class in DispatchClass::all() { let per_class = weights.per_class.get_mut(*class); - let before = per_class.max_extrinsic; + if per_class.max_extrinsic.is_any_unlimited() { per_class.max_extrinsic = per_class.max_total; From 8d3f6a578f2edb095a503b4bb2f48b861944a8cc Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 22:56:55 +0100 Subject: [PATCH 28/31] Simplify API Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/lib.rs | 16 +++---- primitives/weights/src/limit.rs | 84 ++++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 16 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d7db1dd9b237a..2cf0516d85677 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -41,7 +41,7 @@ use frame_support::{ constants::{ BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight, WEIGHT_REF_TIME_PER_SECOND, }, - ConstantMultiplier, IdentityFee, Weight, + ConstantMultiplier, IdentityFee, Weight, WeightLimit, }, PalletId, RuntimeDebug, }; @@ -176,8 +176,8 @@ const AVERAGE_ON_INITIALIZE_RATIO: Perbill = Perbill::from_percent(10); /// by Operational extrinsics. const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75); /// We allow for 2 seconds of compute with a 6 second average block time, with maximum proof size. -const MAXIMUM_BLOCK_WEIGHT: Weight = - Weight::from_parts(WEIGHT_REF_TIME_PER_SECOND.saturating_mul(2), u64::MAX); +const MAXIMUM_BLOCK_WEIGHT: WeightLimit = + WeightLimit::from_time_limit(2 * WEIGHT_REF_TIME_PER_SECOND); parameter_types! { pub const BlockHashCount: BlockNumber = 2400; @@ -190,15 +190,13 @@ parameter_types! { weights.base_extrinsic = ExtrinsicBaseWeight::get(); }) .for_class(DispatchClass::Normal, |weights| { - weights.max_total = (NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT).into(); + weights.max_total = NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT; }) .for_class(DispatchClass::Operational, |weights| { - weights.max_total = (MAXIMUM_BLOCK_WEIGHT).into(); + weights.max_total = MAXIMUM_BLOCK_WEIGHT; // Operational transactions have some extra reserved space, so that they // are included even if block reached `MAXIMUM_BLOCK_WEIGHT`. - weights.reserved = ( - MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT - ).into(); + weights.reserved = MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT; }) .avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO) .build_or_panic(); @@ -1497,7 +1495,7 @@ parameter_types! { pub const MinBid: Balance = 100 * DOLLARS; pub const MinReceipt: Perquintill = Perquintill::from_percent(1); pub const IntakePeriod: BlockNumber = 10; - pub MaxIntakeWeight: Weight = MAXIMUM_BLOCK_WEIGHT / 10; + pub MaxIntakeWeight: Weight = (MAXIMUM_BLOCK_WEIGHT / 10).limited_or_max(); pub const ThawThrottle: (Perquintill, BlockNumber) = (Perquintill::from_percent(25), 5); pub Target: Perquintill = Perquintill::zero(); pub const NisPalletId: PalletId = PalletId(*b"py/nis "); diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index 36d7068da3460..bf0bd1a03519d 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -17,6 +17,7 @@ use crate::Weight; use codec::{Decode, Encode}; +use core::ops::{Add, Div, Mul, Sub}; use scale_info::TypeInfo; use sp_debug_derive::RuntimeDebug; @@ -241,24 +242,30 @@ impl WeightLimit { /// let weight = Weight::from_parts(100, u64::MAX); /// assert_eq!(weight, limit.limited_or(Weight::MAX)); /// ``` - pub fn limited_or(self, w: Weight) -> Weight { - Weight { - ref_time: self.ref_time.unwrap_or(w.ref_time), - proof_size: self.proof_size.unwrap_or(w.proof_size), - } + pub const fn limited_or(self, w: Weight) -> Weight { + // NOTE: not using `unwrap_or` since it is not `const`. + let time_limit = match self.ref_time { + Some(limit) => limit, + None => w.ref_time, + }; + let proof_size_limit = match self.proof_size { + Some(limit) => limit, + None => w.proof_size, + }; + Weight::from_parts(time_limit, proof_size_limit) } /// Uses the exact value for each *limited* component and `u64::MAX` for each unlimited one. /// /// Equivalent to `limited_or(Weight::MAX)`. - pub fn limited_or_max(self) -> Weight { + pub const fn limited_or_max(self) -> Weight { self.limited_or(Weight::MAX) } /// Uses the exact value for each *limited* component and `0` for each unlimited one. /// /// Equivalent to `limited_or(Weight::zero())`. - pub fn limited_or_min(self) -> Weight { + pub const fn limited_or_min(self) -> Weight { self.limited_or(Weight::zero()) } @@ -315,6 +322,14 @@ impl WeightLimit { { Self { ref_time: f(self.ref_time), proof_size: f(self.proof_size) } } + + /// Only map exact values - `unlimited` stays untouched. + pub fn map_exact_limits(self, f: F) -> Self + where + F: Fn(u64) -> u64, + { + Self { ref_time: self.ref_time.map(&f), proof_size: self.proof_size.map(&f) } + } } impl From for WeightLimit { @@ -685,3 +700,58 @@ mod tests { .collect::>() } } + +macro_rules! impl_weight_limit_mul { + ($($t:ty),* $(,)?) => { + $( + impl Mul for $t { + type Output = WeightLimit; + fn mul(self, b: WeightLimit) -> WeightLimit { + WeightLimit { + ref_time: b.ref_time.map(|x| self * x), + proof_size: b.proof_size.map(|x| self * x), + } + } + } + )* + } +} + +impl_weight_limit_mul!( + sp_arithmetic::Percent, + sp_arithmetic::PerU16, + sp_arithmetic::Permill, + sp_arithmetic::Perbill, + sp_arithmetic::Perquintill, +); + +impl Add for WeightLimit { + type Output = Self; + fn add(self, rhs: Self) -> Self { + Self { + ref_time: self.ref_time.and_then(|x| rhs.ref_time.map(|y| x + y)), + proof_size: self.proof_size.and_then(|x| rhs.proof_size.map(|y| x + y)), + } + } +} + +impl Sub for WeightLimit { + type Output = Self; + fn sub(self, rhs: Self) -> Self { + Self { + ref_time: self.ref_time.and_then(|x| rhs.ref_time.map(|y| x - y)), + proof_size: self.proof_size.and_then(|x| rhs.proof_size.map(|y| x - y)), + } + } +} + +impl Div for WeightLimit +where + u64: Div, + T: Copy, +{ + type Output = Self; + fn div(self, b: T) -> Self { + self.map_exact_limits(|x| x / b) + } +} From dabb1d5971891418927ce00eab0333d589ebf16c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 23:51:56 +0100 Subject: [PATCH 29/31] make stuff work Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/lib.rs | 4 ++++ primitives/weights/src/limit.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 2cf0516d85677..be003e12f332a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -178,6 +178,9 @@ const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75); /// We allow for 2 seconds of compute with a 6 second average block time, with maximum proof size. const MAXIMUM_BLOCK_WEIGHT: WeightLimit = WeightLimit::from_time_limit(2 * WEIGHT_REF_TIME_PER_SECOND); +/// The target Proof size that we aim for. This is not a hard limit and is currently only used to +/// print a warning and emit an event on overflow. +pub const BLOCK_POV_TARGET: u64 = 5 * 1024 * 1024; parameter_types! { pub const BlockHashCount: BlockNumber = 2400; @@ -198,6 +201,7 @@ parameter_types! { // are included even if block reached `MAXIMUM_BLOCK_WEIGHT`. weights.reserved = MAXIMUM_BLOCK_WEIGHT - NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT; }) + .pov_soft_limit(BLOCK_POV_TARGET) .avg_block_initialization(AVERAGE_ON_INITIALIZE_RATIO) .build_or_panic(); } diff --git a/primitives/weights/src/limit.rs b/primitives/weights/src/limit.rs index bf0bd1a03519d..e2a88b4e3413a 100644 --- a/primitives/weights/src/limit.rs +++ b/primitives/weights/src/limit.rs @@ -281,6 +281,19 @@ impl WeightLimit { self.proof_size = self.proof_size.map(|s| s.saturating_sub(weight.proof_size)); } + pub const fn div(self, n: u64) -> Self { + // Note: not using `map` since it is not `const`. + let ref_time = match self.ref_time { + Some(limit) => Some(limit / n), + None => None, + }; + let proof_size = match self.proof_size { + Some(limit) => Some(limit / n), + None => None, + }; + Self { ref_time, proof_size } + } + /// Returns the exact weight limits for each component. /// /// The exact limit is the larges possible weight that is still *within* the limit. Returns From 7b12d0ec22ef6e85c8a52bc87713e3a62c3e35bf Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Thu, 19 Jan 2023 23:58:13 +0100 Subject: [PATCH 30/31] fix Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index be003e12f332a..0b06d3253b38a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -617,8 +617,7 @@ parameter_types! { pub MinerMaxWeight: Weight = RuntimeBlockWeights::get() .get(DispatchClass::Normal) .max_extrinsic - .exact_limits() - .expect("Normal extrinsics have a weight limit configured; qed") + .limited_or_max() .saturating_sub(BlockExecutionWeight::get()); // Solution can occupy 90% of normal block size pub MinerMaxLength: u32 = Perbill::from_rational(9u32, 10) * From 09cf5352e6e8a557cfdb991f24e1a312cf5a9346 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 20 Jan 2023 00:23:56 +0100 Subject: [PATCH 31/31] fix test Signed-off-by: Oliver Tale-Yazdi --- bin/node/runtime/src/impls.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/bin/node/runtime/src/impls.rs b/bin/node/runtime/src/impls.rs index 5f7b1ec924daa..ef96e5e6d9033 100644 --- a/bin/node/runtime/src/impls.rs +++ b/bin/node/runtime/src/impls.rs @@ -443,12 +443,14 @@ mod multiplier_tests { // Some values that are all above the target and will cause an increase. let t = target(); - vec![t + Weight::from_ref_time(100), t * 2, t * 4].into_iter().for_each(|i| { - run_with_system_weight(i, || { - let fm = runtime_multiplier_update(max_fm); - // won't grow. The convert saturates everything. - assert_eq!(fm, max_fm); - }) - }); + vec![t + Weight::from_ref_time(100), t.saturating_mul(2), t.saturating_mul(4), Weight::MAX] + .into_iter() + .for_each(|i| { + run_with_system_weight(i, || { + let fm = runtime_multiplier_update(max_fm); + // won't grow. The convert saturates everything. + assert_eq!(fm, max_fm); + }) + }); } }