CheckWeight: account for extrinsic len as proof size

cumulus/primitives/storage-weight-reclaim/src/lib.rs

@@ -201,7 +201,7 @@ mod tests {
 	use super::*;
 	use frame_support::{
 		assert_ok,
-		dispatch::DispatchClass,
+		dispatch::{DispatchClass, PerDispatchClass},
 		weights::{Weight, WeightMeter},
 	};
 	use frame_system::{BlockWeight, CheckWeight};
@@ -215,7 +215,7 @@ mod tests {
 			pages: 0u64,
 		});
 	const ALICE: AccountId32 = AccountId32::new([1u8; 32]);
-	const LEN: usize = 0;
+	const LEN: usize = 150;
 
 	pub fn new_test_ext() -> sp_io::TestExternalities {
 		let ext: sp_io::TestExternalities = cumulus_test_runtime::RuntimeGenesisConfig::default()
@@ -256,6 +256,10 @@ mod tests {
 		});
 	}
 
+	fn get_storage_weight() -> PerDispatchClass<Weight> {
+		BlockWeight::<Test>::get()
+	}
+
 	#[test]
 	fn basic_refund() {
 		// The real cost will be 100 bytes of storage size
@@ -268,6 +272,9 @@ mod tests {
 			let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
 			let post_info = PostDispatchInfo::default();
 
+			// Should add 500 + 150 (len) to weight.
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
@@ -283,7 +290,7 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 600);
+			assert_eq!(get_storage_weight().total().proof_size(), 1250);
 		})
 	}
 
@@ -299,6 +306,9 @@ mod tests {
 			let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
 			let post_info = PostDispatchInfo::default();
 
+			// Adds 500 + 150 (len) weight
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
@@ -313,7 +323,7 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1000);
+			assert_eq!(get_storage_weight().total().proof_size(), 1650);
 		})
 	}
 
@@ -327,6 +337,9 @@ mod tests {
 			let info = DispatchInfo { weight: Weight::from_parts(0, 100), ..Default::default() };
 			let post_info = PostDispatchInfo::default();
 
+			// Weight added should be 100 + 150 (len)
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
@@ -342,7 +355,10 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1100);
+			assert_eq!(
+				get_storage_weight().total().proof_size(),
+				1100 + LEN as u64 + info.weight.proof_size()
+			);
 		})
 	}
 
@@ -354,6 +370,8 @@ mod tests {
 			let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
 			let post_info = PostDispatchInfo::default();
 
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
@@ -368,7 +386,8 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 0);
+			// Proof size should be exactly equal to extrinsic length
+			assert_eq!(get_storage_weight().total().proof_size(), LEN as u64);
 		});
 	}
 
@@ -382,12 +401,17 @@ mod tests {
 			let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
 			let post_info = PostDispatchInfo::default();
 
+			// Adds 500 + 150 (len) weight, total weight is 1950
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
 			assert_eq!(pre, Some(300));
 
+			// Refund 500 unspent weight according to `post_info`, total weight is now 1650
 			assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
+			// Recorded proof size is negative -200, total weight is now 1450
 			assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
 				Some(pre),
 				&info,
@@ -396,7 +420,7 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 800);
+			assert_eq!(get_storage_weight().total().proof_size(), 1450);
 		});
 	}
 
@@ -416,6 +440,9 @@ mod tests {
 				pays_fee: Default::default(),
 			};
 
+			// Should add 300 + 150 (len) of weight
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
@@ -432,7 +459,8 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 900);
+			// Reclaimed 100
+			assert_eq!(get_storage_weight().total().proof_size(), 1350);
 		})
 	}
 
@@ -451,14 +479,66 @@ mod tests {
 				pays_fee: Default::default(),
 			};
 
+			// Adds 50 + 150 (len) weight, total weight 1200
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
 			assert_eq!(pre, Some(100));
 
 			// The `CheckWeight` extension will refund `actual_weight` from `PostDispatchInfo`
 			// we always need to call `post_dispatch` to verify that they interoperate correctly.
+
+			// Refunds unspent 25 weight according to `post_info`, 1175
 			assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
+			// Adds 200 - 25 (unspent) == 175 weight, total weight 1350
+			assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
+				Some(pre),
+				&info,
+				&post_info,
+				LEN,
+				&Ok(())
+			));
+
+			assert_eq!(get_storage_weight().total().proof_size(), 1350);
+		})
+	}
+
+	#[test]
+	fn test_nothing_relcaimed() {
+		let mut test_ext = setup_test_externalities(&[100, 200]);
+
+		test_ext.execute_with(|| {
+			set_current_storage_weight(0);
+			// Benchmarked storage weight: 100
+			let info = DispatchInfo { weight: Weight::from_parts(100, 100), ..Default::default() };
+
+			// Actual proof size is 100
+			let post_info = PostDispatchInfo {
+				actual_weight: Some(Weight::from_parts(50, 100)),
+				pays_fee: Default::default(),
+			};
+
+			// Adds benchmarked weight 100 + 150 (len), total weight is now 250
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
+			// Weight should go up by 150 len + 100 proof size weight, total weight 250
+			assert_eq!(get_storage_weight().total().proof_size(), 250);
+
+			let pre = StorageWeightReclaim::<Test>(PhantomData)
+				.pre_dispatch(&ALICE, CALL, &info, LEN)
+				.unwrap();
+			// Should return `setup_test_externalities` proof recorder value: 100.
+			assert_eq!(pre, Some(100));
+
+			// The `CheckWeight` extension will refund `actual_weight` from `PostDispatchInfo`
+			// we always need to call `post_dispatch` to verify that they interoperate correctly.
+			// Nothing to refund, unspent is 0, total weight 250
+			assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, LEN, &Ok(())));
+			// `setup_test_externalities` proof recorder value: 200, so this means the extrinsic
+			// actually used 100 proof size.
+			// Nothing to refund or add, weight matches proof recorder
 			assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
 				Some(pre),
 				&info,
@@ -467,7 +547,9 @@ mod tests {
 				&Ok(())
 			));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1150);
+			// Check block len weight was not reclaimed:
+			// 100 weight + 150 extrinsic len == 250 proof size
+			assert_eq!(get_storage_weight().total().proof_size(), 250);
 		})
 	}
 
@@ -487,11 +569,15 @@ mod tests {
 				pays_fee: Default::default(),
 			};
 
+			// Adds 300 + 150 (len) weight, total weight 1450
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
 			assert_eq!(pre, Some(100));
 
+			// This refunds 100 - 50(unspent), total weight is now 1400
 			assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
 				Some(pre),
 				&info,
@@ -504,7 +590,8 @@ mod tests {
 			// we always need to call `post_dispatch` to verify that they interoperate correctly.
 			assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 900);
+			// Above call refunds 50 (unspent), total weight is 1350 now
+			assert_eq!(get_storage_weight().total().proof_size(), 1350);
 		})
 	}
 
@@ -523,11 +610,15 @@ mod tests {
 				pays_fee: Default::default(),
 			};
 
+			// Adds 50 + 150 (len) weight, total weight is 1200
+			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));
+
 			let pre = StorageWeightReclaim::<Test>(PhantomData)
 				.pre_dispatch(&ALICE, CALL, &info, LEN)
 				.unwrap();
 			assert_eq!(pre, Some(100));
 
+			// Adds additional 150 weight recorded
 			assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
 				Some(pre),
 				&info,
@@ -540,7 +631,7 @@ mod tests {
 			// we always need to call `post_dispatch` to verify that they interoperate correctly.
 			assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
 
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1150);
+			assert_eq!(get_storage_weight().total().proof_size(), 1350);
 		})
 	}
 
@@ -644,7 +735,7 @@ mod tests {
 
 			// We reclaimed 3 bytes of storage size!
 			assert_eq!(reclaimed, Some(Weight::from_parts(0, 3)));
-			assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 10);
+			assert_eq!(get_storage_weight().total().proof_size(), 10);
 			assert_eq!(remaining_weight_meter.remaining(), Weight::from_parts(10, 8));
 		}
 	}

prdoc/pr_4765.prdoc

@@ -0,0 +1,18 @@
+title: CheckWeight - account for extrinsic len as proof size
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This changes how CheckWeight extension works. It will now account for the extrinsic length
+      as proof size. When `on_idle` is called, the remaining weight parameter reflects this.
+
+crates:
+  - name: frame-system
+    bump: patch
+  - name: frame-executive
+    bump: none
+  - name: cumulus-primitives-storage-weight-reclaim
+    bump: none
+
+
+

substrate/frame/executive/src/tests.rs

@@ -637,8 +637,8 @@ fn block_weight_limit_enforced() {
 				assert!(res.is_ok());
 				assert_eq!(
 					<frame_system::Pallet<Runtime>>::block_weight().total(),
-					//--------------------- on_initialize + block_execution + extrinsic_base weight
-					Weight::from_parts((encoded_len + 5) * (nonce + 1), 0) + base_block_weight,
+					//--------------------- on_initialize + block_execution + extrinsic_base weight + extrinsic len
+					Weight::from_parts((encoded_len + 5) * (nonce + 1), (nonce + 1)* encoded_len) + base_block_weight,
 				);
 				assert_eq!(
 					<frame_system::Pallet<Runtime>>::extrinsic_index(),
@@ -686,9 +686,10 @@ fn block_weight_and_size_is_stored_per_tx() {
 			<Runtime as frame_system::Config>::BlockWeights::get()
 				.get(DispatchClass::Normal)
 				.base_extrinsic;
+		// Check we account for all extrinsic weight and their len.
 		assert_eq!(
 			<frame_system::Pallet<Runtime>>::block_weight().total(),
-			base_block_weight + 3u64 * extrinsic_weight,
+			base_block_weight + 3u64 * extrinsic_weight + 3u64 * Weight::from_parts(0, len as u64),
 		);
 		assert_eq!(<frame_system::Pallet<Runtime>>::all_extrinsics_len(), 3 * len);