From e4b89cc50c8d17868d6c8b122f2e156d678c7525 Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Thu, 23 May 2024 21:21:32 +0200 Subject: [PATCH 1/5] Disable total pov size check for mandatory extrinsics --- .../system/src/extensions/check_weight.rs | 126 +++++++++++++++--- 1 file changed, 108 insertions(+), 18 deletions(-) diff --git a/substrate/frame/system/src/extensions/check_weight.rs b/substrate/frame/system/src/extensions/check_weight.rs index 061d543f8c31..a6da11648e1a 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,11 +131,20 @@ 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, +{ + if matches!(info.class, DispatchClass::Mandatory) { + // Allow mandatory extrinsics + return Ok(()); + } + // 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); @@ -146,7 +155,7 @@ pub fn check_combined_proof_size( total_pov_size as f64/1024.0, maximum_weight.max_block.proof_size() as f64/1024.0 ); - return Err(InvalidTransaction::ExhaustsResources.into()) + return 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 + )); } } From 24705a52d7b246b2f7420ea7f8356ff8bfa8b8ce Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 27 May 2024 09:06:50 +0200 Subject: [PATCH 2/5] Still print if mandatory --- .../frame/system/src/extensions/check_weight.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/substrate/frame/system/src/extensions/check_weight.rs b/substrate/frame/system/src/extensions/check_weight.rs index a6da11648e1a..87ab9d5c74fa 100644 --- a/substrate/frame/system/src/extensions/check_weight.rs +++ b/substrate/frame/system/src/extensions/check_weight.rs @@ -140,10 +140,6 @@ pub fn check_combined_proof_size( where Call: Dispatchable, { - if matches!(info.class, DispatchClass::Mandatory) { - // Allow mandatory extrinsics - return Ok(()); - } // This extra check ensures that the extrinsic length does not push the // PoV over the limit. @@ -151,11 +147,16 @@ where 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(()) } From 1e03bdf444649a1dd4d32fb7fbe86317ec80889a Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 27 May 2024 09:17:14 +0200 Subject: [PATCH 3/5] Add prdoc --- prdoc/pr_4571.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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..83b651a50356 --- /dev/null +++ b/prdoc/pr_4571.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/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 From c51ca8010c55bb969c8248f44873d6439e7bcfda Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Mon, 27 May 2024 08:41:36 +0000 Subject: [PATCH 4/5] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/system/src/extensions/check_weight.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/frame/system/src/extensions/check_weight.rs b/substrate/frame/system/src/extensions/check_weight.rs index 87ab9d5c74fa..5d6c68989ed5 100644 --- a/substrate/frame/system/src/extensions/check_weight.rs +++ b/substrate/frame/system/src/extensions/check_weight.rs @@ -140,7 +140,6 @@ pub fn check_combined_proof_size( 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); @@ -155,7 +154,7 @@ where return match info.class { // Allow mandatory extrinsics DispatchClass::Mandatory => Ok(()), - _ => Err(InvalidTransaction::ExhaustsResources.into()) + _ => Err(InvalidTransaction::ExhaustsResources.into()), }; } Ok(()) From 29e70235e900fcab4afaaa342beac733cfff14dc Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 27 May 2024 10:48:28 +0200 Subject: [PATCH 5/5] Also bump umbrella crate --- prdoc/pr_4571.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_4571.prdoc b/prdoc/pr_4571.prdoc index 83b651a50356..b03fee8a5cc8 100644 --- a/prdoc/pr_4571.prdoc +++ b/prdoc/pr_4571.prdoc @@ -15,3 +15,5 @@ doc: crates: - name: frame-system bump: minor + - name: polkadot-sdk + bump: minor