From 0d420ee4d81079067bf7c7c4275af4f7f036f8c2 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 df0b55621d39cc3f892aa4337063d71d9baa87fb 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 9a51bf1d0cedd028b784719ce9994a5e9acde8ad 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 2992b24f317754ad6da6b0b86d3a62856b57554b 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 897ca7478d556a4a8c26a59403da068649c881ce Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Mon, 27 May 2024 10:56:32 +0200 Subject: [PATCH 5/5] Move prdoc into 1.12.0 release directory. --- prdoc/{ => 1.12.0}/pr_4571.prdoc | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename prdoc/{ => 1.12.0}/pr_4571.prdoc (100%) diff --git a/prdoc/pr_4571.prdoc b/prdoc/1.12.0/pr_4571.prdoc similarity index 100% rename from prdoc/pr_4571.prdoc rename to prdoc/1.12.0/pr_4571.prdoc