diff --git a/prdoc/pr_4326.prdoc b/prdoc/pr_4326.prdoc new file mode 100644 index 000000000000..b448bd7e52e7 --- /dev/null +++ b/prdoc/pr_4326.prdoc @@ -0,0 +1,16 @@ +# 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: CheckWeight checks for combined extrinsic length and proof size + +doc: + - audience: Runtime Dev + description: | + The `CheckWeight` `SignedExtension` will now perform an additional check. The extension was verifying the extrinsic length and + weight limits individually. However, the proof size dimension of the weight and extrinsic length together are bound by the PoV size limit. + The `CheckWeight` extension will now check that the combined size of the proof and the extrinsic lengths will not + exceed the PoV size limit. + +crates: + - name: frame-system + bump: minor diff --git a/substrate/frame/system/src/extensions/check_weight.rs b/substrate/frame/system/src/extensions/check_weight.rs index 70d1e7563327..061d543f8c31 100644 --- a/substrate/frame/system/src/extensions/check_weight.rs +++ b/substrate/frame/system/src/extensions/check_weight.rs @@ -64,17 +64,6 @@ where } } - /// Checks if the current extrinsic can fit into the block with respect to block weight limits. - /// - /// Upon successes, it returns the new block weight as a `Result`. - fn check_block_weight( - info: &DispatchInfoOf, - ) -> Result { - let maximum_weight = T::BlockWeights::get(); - let all_weight = Pallet::::block_weight(); - calculate_consumed_weight::(maximum_weight, all_weight, info) - } - /// Checks if the current extrinsic can fit into the block with respect to block length limits. /// /// Upon successes, it returns the new block length as a `Result`. @@ -113,7 +102,12 @@ where len: usize, ) -> Result<(), TransactionValidityError> { let next_len = Self::check_block_length(info, len)?; - let next_weight = Self::check_block_weight(info)?; + + let all_weight = Pallet::::block_weight(); + 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)?; Self::check_extrinsic_weight(info)?; crate::AllExtrinsicsLen::::put(next_len); @@ -136,8 +130,32 @@ where } } +/// Check that the combined extrinsic length and proof size together do not exceed the PoV limit. +pub fn check_combined_proof_size( + maximum_weight: &BlockWeights, + next_len: u32, + next_weight: &crate::ConsumedWeight, +) -> Result<(), TransactionValidityError> { + // 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", + total_pov_size as f64/1024.0, + maximum_weight.max_block.proof_size() as f64/1024.0 + ); + return Err(InvalidTransaction::ExhaustsResources.into()) + } + Ok(()) +} + +/// Checks if the current extrinsic can fit into the block with respect to block weight limits. +/// +/// Upon successes, it returns the new block weight as a `Result`. pub fn calculate_consumed_weight( - maximum_weight: BlockWeights, + maximum_weight: &BlockWeights, mut all_weight: crate::ConsumedWeight, info: &DispatchInfoOf, ) -> Result @@ -742,17 +760,90 @@ mod tests { // when assert_ok!(calculate_consumed_weight::<::RuntimeCall>( - maximum_weight.clone(), + &maximum_weight, all_weight.clone(), - &mandatory1 + &mandatory1, )); assert_err!( calculate_consumed_weight::<::RuntimeCall>( - maximum_weight, + &maximum_weight, all_weight, - &mandatory2 + &mandatory2, ), InvalidTransaction::ExhaustsResources ); } + + #[test] + fn maximum_proof_size_includes_length() { + let maximum_weight = BlockWeights::builder() + .base_block(Weight::zero()) + .for_class(DispatchClass::non_mandatory(), |w| { + w.base_extrinsic = Weight::zero(); + w.max_total = Some(Weight::from_parts(20, 10)); + }) + .for_class(DispatchClass::Mandatory, |w| { + w.base_extrinsic = Weight::zero(); + w.reserved = Some(Weight::from_parts(5, 10)); + w.max_total = None; + }) + .build_or_panic(); + + assert_eq!(maximum_weight.max_block, Weight::from_parts(20, 10)); + + // 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), + DispatchClass::Operational => Weight::from_parts(0, 0), + DispatchClass::Mandatory => Weight::zero(), + }); + + // 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_err!( + check_combined_proof_size(&maximum_weight, 6, &next_weight), + InvalidTransaction::ExhaustsResources + ); + + // We have 10 reftime and 0 proof size left over. + let next_weight = crate::ConsumedWeight::new(|class| match class { + DispatchClass::Normal => Weight::from_parts(10, 10), + DispatchClass::Operational => Weight::from_parts(0, 0), + DispatchClass::Mandatory => Weight::zero(), + }); + assert_ok!(check_combined_proof_size(&maximum_weight, 0, &next_weight)); + assert_err!( + check_combined_proof_size(&maximum_weight, 1, &next_weight), + InvalidTransaction::ExhaustsResources + ); + + // We have 10 reftime and 2 proof size left over. + // Used weight is spread across dispatch classes this time. + let next_weight = crate::ConsumedWeight::new(|class| match class { + DispatchClass::Normal => Weight::from_parts(10, 5), + 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_err!( + check_combined_proof_size(&maximum_weight, 3, &next_weight), + InvalidTransaction::ExhaustsResources + ); + + // Ref time is over the limit. Should not happen, but we should make sure that it is + // ignored. + let next_weight = crate::ConsumedWeight::new(|class| match class { + DispatchClass::Normal => Weight::from_parts(30, 5), + 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_err!( + check_combined_proof_size(&maximum_weight, 6, &next_weight), + InvalidTransaction::ExhaustsResources + ); + } }