From 70dd67a5d129745da6a05bce958824504a4c9d83 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 27 May 2024 19:12:46 +0200 Subject: [PATCH] check-weight: Disable total pov size check for mandatory extrinsics (#4571) So in some pallets we like [here](https://github.com/paritytech/polkadot-sdk/blob/5dc522d02fe0b53be1517f8b8979176e489a388b/substrate/frame/session/src/lib.rs#L556) we use `max_block` as return value for `on_initialize` (ideally we would not). This means the block is already full when we try to apply the inherents, which lead to the error seen in #4559 because we are unable to include the required inherents. This was not erroring before #4326 because we were running into this branch: https://github.com/paritytech/polkadot-sdk/blob/e4b89cc50c8d17868d6c8b122f2e156d678c7525/substrate/frame/system/src/extensions/check_weight.rs#L222-L224 The inherents are of `DispatchClass::Mandatory` and therefore have a `reserved` value of `None` in all runtimes I have inspected. So they will always pass the normal check. So in this PR I adjust the `check_combined_proof_size` to return an early `Ok(())` for mandatory extrinsics. If we agree on this PR I will backport it to the 1.12.0 branch. closes #4559 --------- Co-authored-by: command-bot <> --- prdoc/pr_4571.prdoc | 19 +++ .../system/src/extensions/check_weight.rs | 130 +++++++++++++++--- 2 files changed, 129 insertions(+), 20 deletions(-) create mode 100644 prdoc/pr_4571.prdoc diff --git a/prdoc/pr_4571.prdoc b/prdoc/pr_4571.prdoc new file mode 100644 index 000000000000..b03fee8a5cc8 --- /dev/null +++ b/prdoc/pr_4571.prdoc @@ -0,0 +1,19 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://github.com/raw/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Ignore mandatory extrinsics in total PoV size check + +doc: + - audience: Runtime Dev + description: | + The `CheckWeight` extension is checking that extrinsic length and used storage proof + weight together do not exceed the PoV size limit. This lead to problems when + the PoV size was already reached before mandatory extrinsics were applied.The `CheckWeight` + extension will now allow extrinsics of `DispatchClass::Mandatory` to be applied even if + the limit is reached. + +crates: + - name: frame-system + bump: minor + - name: polkadot-sdk + bump: minor diff --git a/substrate/frame/system/src/extensions/check_weight.rs b/substrate/frame/system/src/extensions/check_weight.rs index 061d543f8c31..5d6c68989ed5 100644 --- a/substrate/frame/system/src/extensions/check_weight.rs +++ b/substrate/frame/system/src/extensions/check_weight.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::{limits::BlockWeights, Config, Pallet, LOG_TARGET}; +use crate::{limits::BlockWeights, Config, DispatchClass, Pallet, LOG_TARGET}; use codec::{Decode, Encode}; use frame_support::{ dispatch::{DispatchInfo, PostDispatchInfo}, @@ -107,7 +107,7 @@ where let maximum_weight = T::BlockWeights::get(); let next_weight = calculate_consumed_weight::(&maximum_weight, all_weight, info)?; - check_combined_proof_size(&maximum_weight, next_len, &next_weight)?; + check_combined_proof_size::(info, &maximum_weight, next_len, &next_weight)?; Self::check_extrinsic_weight(info)?; crate::AllExtrinsicsLen::::put(next_len); @@ -131,22 +131,31 @@ where } /// Check that the combined extrinsic length and proof size together do not exceed the PoV limit. -pub fn check_combined_proof_size( +pub fn check_combined_proof_size( + info: &DispatchInfoOf, maximum_weight: &BlockWeights, next_len: u32, next_weight: &crate::ConsumedWeight, -) -> Result<(), TransactionValidityError> { +) -> Result<(), TransactionValidityError> +where + Call: Dispatchable, +{ // This extra check ensures that the extrinsic length does not push the // PoV over the limit. let total_pov_size = next_weight.total().proof_size().saturating_add(next_len as u64); if total_pov_size > maximum_weight.max_block.proof_size() { log::debug!( target: LOG_TARGET, - "Extrinsic exceeds total pov size: {}kb, limit: {}kb", + "Extrinsic exceeds total pov size. Still including if mandatory. size: {}kb, limit: {}kb, is_mandatory: {}", total_pov_size as f64/1024.0, - maximum_weight.max_block.proof_size() as f64/1024.0 + maximum_weight.max_block.proof_size() as f64/1024.0, + info.class == DispatchClass::Mandatory ); - return Err(InvalidTransaction::ExhaustsResources.into()) + return match info.class { + // Allow mandatory extrinsics + DispatchClass::Mandatory => Ok(()), + _ => Err(InvalidTransaction::ExhaustsResources.into()), + }; } Ok(()) } @@ -190,7 +199,7 @@ where "Exceeded the per-class allowance.", ); - return Err(InvalidTransaction::ExhaustsResources.into()) + return Err(InvalidTransaction::ExhaustsResources.into()); }, // There is no `max_total` limit (`None`), // or we are below the limit. @@ -208,7 +217,7 @@ where "Total block weight is exceeded.", ); - return Err(InvalidTransaction::ExhaustsResources.into()) + return Err(InvalidTransaction::ExhaustsResources.into()); }, // There is either no limit in reserved pool (`None`), // or we are below the limit. @@ -791,6 +800,8 @@ mod tests { assert_eq!(maximum_weight.max_block, Weight::from_parts(20, 10)); + let info = DispatchInfo { class: DispatchClass::Normal, ..Default::default() }; + let mandatory = DispatchInfo { class: DispatchClass::Mandatory, ..Default::default() }; // We have 10 reftime and 5 proof size left over. let next_weight = crate::ConsumedWeight::new(|class| match class { DispatchClass::Normal => Weight::from_parts(10, 5), @@ -799,12 +810,33 @@ mod tests { }); // Simple checks for the length - assert_ok!(check_combined_proof_size(&maximum_weight, 0, &next_weight)); - assert_ok!(check_combined_proof_size(&maximum_weight, 5, &next_weight)); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 0, + &next_weight + )); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 5, + &next_weight + )); assert_err!( - check_combined_proof_size(&maximum_weight, 6, &next_weight), + check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 6, + &next_weight + ), InvalidTransaction::ExhaustsResources ); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &mandatory, + &maximum_weight, + 6, + &next_weight + )); // We have 10 reftime and 0 proof size left over. let next_weight = crate::ConsumedWeight::new(|class| match class { @@ -812,11 +844,27 @@ mod tests { DispatchClass::Operational => Weight::from_parts(0, 0), DispatchClass::Mandatory => Weight::zero(), }); - assert_ok!(check_combined_proof_size(&maximum_weight, 0, &next_weight)); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 0, + &next_weight + )); assert_err!( - check_combined_proof_size(&maximum_weight, 1, &next_weight), + check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 1, + &next_weight + ), InvalidTransaction::ExhaustsResources ); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &mandatory, + &maximum_weight, + 1, + &next_weight + )); // We have 10 reftime and 2 proof size left over. // Used weight is spread across dispatch classes this time. @@ -825,12 +873,33 @@ mod tests { DispatchClass::Operational => Weight::from_parts(0, 3), DispatchClass::Mandatory => Weight::zero(), }); - assert_ok!(check_combined_proof_size(&maximum_weight, 0, &next_weight)); - assert_ok!(check_combined_proof_size(&maximum_weight, 2, &next_weight)); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 0, + &next_weight + )); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 2, + &next_weight + )); assert_err!( - check_combined_proof_size(&maximum_weight, 3, &next_weight), + check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 3, + &next_weight + ), InvalidTransaction::ExhaustsResources ); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &mandatory, + &maximum_weight, + 3, + &next_weight + )); // Ref time is over the limit. Should not happen, but we should make sure that it is // ignored. @@ -839,11 +908,32 @@ mod tests { DispatchClass::Operational => Weight::from_parts(0, 0), DispatchClass::Mandatory => Weight::zero(), }); - assert_ok!(check_combined_proof_size(&maximum_weight, 0, &next_weight)); - assert_ok!(check_combined_proof_size(&maximum_weight, 5, &next_weight)); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 0, + &next_weight + )); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 5, + &next_weight + )); assert_err!( - check_combined_proof_size(&maximum_weight, 6, &next_weight), + check_combined_proof_size::<::RuntimeCall>( + &info, + &maximum_weight, + 6, + &next_weight + ), InvalidTransaction::ExhaustsResources ); + assert_ok!(check_combined_proof_size::<::RuntimeCall>( + &mandatory, + &maximum_weight, + 6, + &next_weight + )); } }