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 <>
paritytech · May 27, 2024 · 70dd67a · 70dd67a
1 parent 16887b6
commit 70dd67a
Show file tree

Hide file tree

Showing 2 changed files with 129 additions and 20 deletions.
diff --git a/prdoc/pr_4571.prdoc 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
@@ -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::<T::RuntimeCall>(&maximum_weight, all_weight, info)?;
-		check_combined_proof_size(&maximum_weight, next_len, &next_weight)?;
+		check_combined_proof_size::<T::RuntimeCall>(info, &maximum_weight, next_len, &next_weight)?;
 		Self::check_extrinsic_weight(info)?;
 
 		crate::AllExtrinsicsLen::<T>::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<Call>(
+	info: &DispatchInfoOf<Call>,
 	maximum_weight: &BlockWeights,
 	next_len: u32,
 	next_weight: &crate::ConsumedWeight,
-) -> Result<(), TransactionValidityError> {
+) -> Result<(), TransactionValidityError>
+where
+	Call: Dispatchable<Info = DispatchInfo, PostInfo = PostDispatchInfo>,
+{
 	// 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,24 +810,61 @@ 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::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			0,
+			&next_weight
+		));
+		assert_ok!(check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			5,
+			&next_weight
+		));
 		assert_err!(
-			check_combined_proof_size(&maximum_weight, 6, &next_weight),
+			check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+				&info,
+				&maximum_weight,
+				6,
+				&next_weight
+			),
 			InvalidTransaction::ExhaustsResources
 		);
+		assert_ok!(check_combined_proof_size::<<Test as Config>::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 {
 			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_ok!(check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			0,
+			&next_weight
+		));
 		assert_err!(
-			check_combined_proof_size(&maximum_weight, 1, &next_weight),
+			check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+				&info,
+				&maximum_weight,
+				1,
+				&next_weight
+			),
 			InvalidTransaction::ExhaustsResources
 		);
+		assert_ok!(check_combined_proof_size::<<Test as Config>::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::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			0,
+			&next_weight
+		));
+		assert_ok!(check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			2,
+			&next_weight
+		));
 		assert_err!(
-			check_combined_proof_size(&maximum_weight, 3, &next_weight),
+			check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+				&info,
+				&maximum_weight,
+				3,
+				&next_weight
+			),
 			InvalidTransaction::ExhaustsResources
 		);
+		assert_ok!(check_combined_proof_size::<<Test as Config>::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::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			0,
+			&next_weight
+		));
+		assert_ok!(check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+			&info,
+			&maximum_weight,
+			5,
+			&next_weight
+		));
 		assert_err!(
-			check_combined_proof_size(&maximum_weight, 6, &next_weight),
+			check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+				&info,
+				&maximum_weight,
+				6,
+				&next_weight
+			),
 			InvalidTransaction::ExhaustsResources
 		);
+		assert_ok!(check_combined_proof_size::<<Test as Config>::RuntimeCall>(
+			&mandatory,
+			&maximum_weight,
+			6,
+			&next_weight
+		));
 	}
 }