From 25217838bf8538e4aabe6f5a72081544106a51fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 19 Jun 2020 14:46:55 +0200 Subject: [PATCH 1/2] Fix weight limit for operational transactions. --- frame/system/src/lib.rs | 65 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index b64b5d58f73d1..c5679b74d6457 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1394,8 +1394,10 @@ impl CheckWeight where info: &DispatchInfoOf, ) -> Result<(), TransactionValidityError> { match info.class { - // Mandatory and Operational transactions does not - DispatchClass::Mandatory | DispatchClass::Operational => Ok(()), + // Mandatory transactions are included in a block unconditionally, so + // we don't verify weight. + DispatchClass::Mandatory => Ok(()), + // Normal transactions must not exceed `MaximumExtrinsicWeight`. DispatchClass::Normal => { let maximum_weight = T::MaximumExtrinsicWeight::get(); let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); @@ -1404,7 +1406,19 @@ impl CheckWeight where } else { Ok(()) } - } + }, + // For operational transactions we make sure it doesn't exceed + // the space alloted for `Operational` class. + DispatchClass::Operational => { + let maximum_weight = T::MaximumBlockWeight::get(); + let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); + if extrinsic_weight > operational_limit { + Err(InvalidTransaction::ExhaustsResources.into()) + } else { + Ok(()) + } + }, } } @@ -1483,9 +1497,11 @@ impl CheckWeight where fn get_priority(info: &DispatchInfoOf) -> TransactionPriority { match info.class { DispatchClass::Normal => info.weight.into(), - DispatchClass::Operational => Bounded::max_value(), + // Don't use up the whole priority space, to allow things like `tip` + // to be taken into account as well. + DispatchClass::Operational => TransactionPriority::max_value() / 2, // Mandatory extrinsics are only for inherents; never transactions. - DispatchClass::Mandatory => Bounded::min_value(), + DispatchClass::Mandatory => TransactionPriority::min_value(), } } @@ -2448,6 +2464,39 @@ pub(crate) mod tests { }); } + #[test] + fn operational_extrinsic_limited_by_operational_space_limit() { + new_test_ext().execute_with(|| { + let operational_limit = CheckWeight::::get_dispatch_limit_ratio( + DispatchClass::Operational + ) * ::MaximumBlockWeight::get(); + let base_weight = ::ExtrinsicBaseWeight::get(); + let okay = DispatchInfo { + weight: operational_limit - base_weight, + class: DispatchClass::Operational, + ..Default::default() + }; + let max = DispatchInfo { + weight: operational_limit - base_weight + 1, + class: DispatchClass::Operational, + ..Default::default() + }; + let len = 0_usize; + + assert_eq!( + CheckWeight::::do_validate(&okay, len), + Ok(ValidTransaction { + priority: CheckWeight::::get_priority(&okay), + ..Default::default() + }) + ); + assert_noop!( + CheckWeight::::do_validate(&max, len), + InvalidTransaction::ExhaustsResources + ); + }); + } + #[test] fn register_extra_weight_unchecked_doesnt_care_about_limits() { new_test_ext().execute_with(|| { @@ -2475,6 +2524,8 @@ pub(crate) mod tests { assert_ok!(CheckWeight::::do_pre_dispatch(&rest_operational, len)); assert_eq!(::MaximumBlockWeight::get(), 1024); assert_eq!(System::block_weight().total(), ::MaximumBlockWeight::get()); + // Checking single extrinsic should not take current block weight into account. + assert_eq!(CheckWeight::::check_extrinsic_weight(&rest_operational), Ok(())); }); } @@ -2510,6 +2561,8 @@ pub(crate) mod tests { assert_ok!(CheckWeight::::do_pre_dispatch(&dispatch_operational, len)); // Not too much though assert_noop!(CheckWeight::::do_pre_dispatch(&dispatch_operational, len), InvalidTransaction::ExhaustsResources); + // Even with full block, validity of single transaction should be correct. + assert_eq!(CheckWeight::::check_extrinsic_weight(&dispatch_operational), Ok(())); }); } @@ -2555,7 +2608,7 @@ pub(crate) mod tests { .validate(&1, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, u64::max_value()); + assert_eq!(priority, u64::max_value() / 2); }) } From 447d224dbce07e147759ed1d8c999996a3141b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 19 Jun 2020 16:14:51 +0200 Subject: [PATCH 2/2] Include BlockExecutionWeight. --- frame/system/src/lib.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 2e4f90ed98b66..517b420f16f07 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1411,7 +1411,10 @@ impl CheckWeight where // the space alloted for `Operational` class. DispatchClass::Operational => { let maximum_weight = T::MaximumBlockWeight::get(); - let operational_limit = Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let operational_limit = + Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight; + let operational_limit = + operational_limit.saturating_sub(T::BlockExecutionWeight::get()); let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get()); if extrinsic_weight > operational_limit { Err(InvalidTransaction::ExhaustsResources.into()) @@ -2474,13 +2477,16 @@ pub(crate) mod tests { DispatchClass::Operational ) * ::MaximumBlockWeight::get(); let base_weight = ::ExtrinsicBaseWeight::get(); + let block_base = ::BlockExecutionWeight::get(); + + let weight = operational_limit - base_weight - block_base; let okay = DispatchInfo { - weight: operational_limit - base_weight, + weight, class: DispatchClass::Operational, ..Default::default() }; let max = DispatchInfo { - weight: operational_limit - base_weight + 1, + weight: weight + 1, class: DispatchClass::Operational, ..Default::default() };