From 4984effed72c4e85569fd88bec807c682c6e8821 Mon Sep 17 00:00:00 2001 From: Guillaume Thiolliere Date: Sat, 10 Aug 2024 00:06:59 +0900 Subject: [PATCH 1/2] StorageWeightReclaim: Fix issue when underestimating refund. (#5273) The code do reduce or increase the weight by comparing `benchmarked_weight` and `consumed_weight`. But `benchmarked_weight` is the pre dispatch weight. not the post dispatch weight that is actually written into the block weight by `CheckWeight`. So in case the consumed weight was: `pre dispatch weight > consumed weight > post dispatch weight` then the reclaim code was reducing the block weight instead of increasing it. Might explain this issue even better https://github.com/paritytech/polkadot-sdk/issues/5229 @skunert @s0me0ne-unkn0wn (cherry picked from commit 862860ecc89e077331b72eb66cd7305b931fb9e8) --- .../storage-weight-reclaim/src/lib.rs | 48 +++++++++++++++++-- prdoc/pr_5273.prdoc | 10 ++++ 2 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 prdoc/pr_5273.prdoc diff --git a/cumulus/primitives/storage-weight-reclaim/src/lib.rs b/cumulus/primitives/storage-weight-reclaim/src/lib.rs index f48dd927ee96..5984fa77a2c5 100644 --- a/cumulus/primitives/storage-weight-reclaim/src/lib.rs +++ b/cumulus/primitives/storage-weight-reclaim/src/lib.rs @@ -165,15 +165,14 @@ where ); return Ok(()) }; - let benchmarked_weight = info.weight.proof_size(); - let consumed_weight = post_dispatch_proof_size.saturating_sub(pre_dispatch_proof_size); - // Unspent weight according to the `actual_weight` from `PostDispatchInfo` // This unspent weight will be refunded by the `CheckWeight` extension, so we need to // account for that. let unspent = post_info.calc_unspent(info).proof_size(); - let storage_size_diff = - benchmarked_weight.saturating_sub(unspent).abs_diff(consumed_weight as u64); + let benchmarked_weight = info.weight.proof_size().saturating_sub(unspent); + let consumed_weight = post_dispatch_proof_size.saturating_sub(pre_dispatch_proof_size); + + let storage_size_diff = benchmarked_weight.abs_diff(consumed_weight as u64); // This value will be reclaimed by [`frame_system::CheckWeight`], so we need to calculate // that in. @@ -294,6 +293,45 @@ mod tests { }) } + #[test] + fn underestimating_refund() { + // We fixed a bug where `pre dispatch info weight > consumed weight > post info weight` + // resulted in error. + + // The real cost will be 100 bytes of storage size + let mut test_ext = setup_test_externalities(&[0, 100]); + + test_ext.execute_with(|| { + set_current_storage_weight(1000); + + // Benchmarked storage weight: 500 + let info = DispatchInfo { weight: Weight::from_parts(0, 101), ..Default::default() }; + let post_info = PostDispatchInfo { + actual_weight: Some(Weight::from_parts(0, 99)), + pays_fee: Default::default(), + }; + + assert_ok!(CheckWeight::::do_pre_dispatch(&info, LEN)); + + let pre = StorageWeightReclaim::(PhantomData) + .pre_dispatch(&ALICE, CALL, &info, LEN) + .unwrap(); + assert_eq!(pre, Some(0)); + + assert_ok!(CheckWeight::::post_dispatch(None, &info, &post_info, 0, &Ok(()))); + // We expect an accrue of 1 + assert_ok!(StorageWeightReclaim::::post_dispatch( + Some(pre), + &info, + &post_info, + LEN, + &Ok(()) + )); + + assert_eq!(get_storage_weight().total().proof_size(), 1250); + }) + } + #[test] fn does_nothing_without_extension() { let mut test_ext = new_test_ext(); diff --git a/prdoc/pr_5273.prdoc b/prdoc/pr_5273.prdoc new file mode 100644 index 000000000000..981172c6c13f --- /dev/null +++ b/prdoc/pr_5273.prdoc @@ -0,0 +1,10 @@ +title: Fix storage weight reclaim bug. + +doc: + - audience: Runtime Dev + description: | + A bug in storage weight reclaim signed extension is fixed. The bug was causing an underestimate of the proof size when the post dispatch info was underestimating the proof size and the pre dispatch info was overestimating the proof size at the same time. + +crates: + - name: cumulus-primitives-storage-weight-reclaim + bump: patch From 81a36ec692c3ae5468f2269f7553e30a1647233f Mon Sep 17 00:00:00 2001 From: Sebastian Kunert Date: Tue, 13 Aug 2024 21:57:23 +0200 Subject: [PATCH 2/2] StorageWeightReclaim: set to node pov size if higher (#5281) This PR adds an additional defensive check to the reclaim SE. Since it can happen that we miss some storage accesses on other SEs pre-dispatch, we should double check that the bookkeeping of the runtime stays ahead of the node-side pov-size. If we discover a mismatch and the node-side pov-size is indeed higher, we should set the runtime bookkeeping to the node-side value. In cases such as #5229, we would stop including extrinsics and not run `on_idle` at least. cc @gui1117 --------- Co-authored-by: command-bot <> (cherry picked from commit 055eb5377da43eaced23647ed4348a816bfeb8f4) --- .../storage-weight-reclaim/src/lib.rs | 96 ++++++++++++++++++- prdoc/pr_5281.prdoc | 17 ++++ substrate/frame/system/src/lib.rs | 2 +- 3 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 prdoc/pr_5281.prdoc diff --git a/cumulus/primitives/storage-weight-reclaim/src/lib.rs b/cumulus/primitives/storage-weight-reclaim/src/lib.rs index 5984fa77a2c5..a557e881e26b 100644 --- a/cumulus/primitives/storage-weight-reclaim/src/lib.rs +++ b/cumulus/primitives/storage-weight-reclaim/src/lib.rs @@ -174,6 +174,9 @@ where let storage_size_diff = benchmarked_weight.abs_diff(consumed_weight as u64); + let extrinsic_len = frame_system::AllExtrinsicsLen::::get().unwrap_or(0); + let node_side_pov_size = post_dispatch_proof_size.saturating_add(extrinsic_len.into()); + // This value will be reclaimed by [`frame_system::CheckWeight`], so we need to calculate // that in. frame_system::BlockWeight::::mutate(|current| { @@ -190,6 +193,19 @@ where ); current.reduce(Weight::from_parts(0, storage_size_diff), info.class) } + + // If we encounter a situation where the node-side proof size is already higher than + // what we have in the runtime bookkeeping, we add the difference to the `BlockWeight`. + // This prevents that the proof size grows faster than the runtime proof size. + let block_weight_proof_size = current.total().proof_size(); + let missing_from_node = node_side_pov_size.saturating_sub(block_weight_proof_size); + if missing_from_node > 0 { + log::warn!( + target: LOG_TARGET, + "Node-side PoV size higher than runtime proof size weight. node-side: {node_side_pov_size} extrinsic_len: {extrinsic_len} runtime: {block_weight_proof_size}, missing: {missing_from_node}. Setting to node-side proof size." + ); + current.accrue(Weight::from_parts(0, missing_from_node), info.class); + } }); Ok(()) } @@ -332,6 +348,82 @@ mod tests { }) } + #[test] + fn sets_to_node_storage_proof_if_higher() { + // The storage proof reported by the proof recorder is higher than what is stored on + // the runtime side. + { + let mut test_ext = setup_test_externalities(&[1000, 1005]); + + test_ext.execute_with(|| { + // Stored in BlockWeight is 5 + set_current_storage_weight(5); + + // Benchmarked storage weight: 10 + let info = DispatchInfo { weight: Weight::from_parts(0, 10), ..Default::default() }; + let post_info = PostDispatchInfo::default(); + + assert_ok!(CheckWeight::::do_pre_dispatch(&info, LEN)); + + let pre = StorageWeightReclaim::(PhantomData) + .pre_dispatch(&ALICE, CALL, &info, LEN) + .unwrap(); + assert_eq!(pre, Some(1000)); + + assert_ok!(CheckWeight::::post_dispatch(None, &info, &post_info, 0, &Ok(()))); + assert_ok!(StorageWeightReclaim::::post_dispatch( + Some(pre), + &info, + &post_info, + LEN, + &Ok(()) + )); + + // We expect that the storage weight was set to the node-side proof size (1005) + + // extrinsics length (150) + assert_eq!(get_storage_weight().total().proof_size(), 1155); + }) + } + + // In this second scenario the proof size on the node side is only lower + // after reclaim happened. + { + let mut test_ext = setup_test_externalities(&[175, 180]); + test_ext.execute_with(|| { + set_current_storage_weight(85); + + // Benchmarked storage weight: 100 + let info = + DispatchInfo { weight: Weight::from_parts(0, 100), ..Default::default() }; + let post_info = PostDispatchInfo::default(); + + // After this pre_dispatch, the BlockWeight proof size will be + // 85 (initial) + 100 (benched) + 150 (tx length) = 335 + assert_ok!(CheckWeight::::do_pre_dispatch(&info, LEN)); + + let pre = StorageWeightReclaim::(PhantomData) + .pre_dispatch(&ALICE, CALL, &info, LEN) + .unwrap(); + assert_eq!(pre, Some(175)); + + assert_ok!(CheckWeight::::post_dispatch(None, &info, &post_info, 0, &Ok(()))); + + // First we will reclaim 95, which leaves us with 240 BlockWeight. This is lower + // than 180 (proof size hf) + 150 (length), so we expect it to be set to 330. + assert_ok!(StorageWeightReclaim::::post_dispatch( + Some(pre), + &info, + &post_info, + LEN, + &Ok(()) + )); + + // We expect that the storage weight was set to the node-side proof weight + assert_eq!(get_storage_weight().total().proof_size(), 330); + }) + } + } + #[test] fn does_nothing_without_extension() { let mut test_ext = new_test_ext(); @@ -545,7 +637,7 @@ mod tests { #[test] fn test_nothing_relcaimed() { - let mut test_ext = setup_test_externalities(&[100, 200]); + let mut test_ext = setup_test_externalities(&[0, 100]); test_ext.execute_with(|| { set_current_storage_weight(0); @@ -568,7 +660,7 @@ mod tests { .pre_dispatch(&ALICE, CALL, &info, LEN) .unwrap(); // Should return `setup_test_externalities` proof recorder value: 100. - assert_eq!(pre, Some(100)); + assert_eq!(pre, Some(0)); // The `CheckWeight` extension will refund `actual_weight` from `PostDispatchInfo` // we always need to call `post_dispatch` to verify that they interoperate correctly. diff --git a/prdoc/pr_5281.prdoc b/prdoc/pr_5281.prdoc new file mode 100644 index 000000000000..60feab412aff --- /dev/null +++ b/prdoc/pr_5281.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: PoV-Reclaim - Set `BlockWeight` to node-side PoV size if mismatch is detected + +doc: + - audience: Runtime Dev + description: | + After this change, the `StorageWeightReclaim` `SignedExtension` will check the node-side PoV size after every + extrinsic. If we detect a case where the returned proof size is higher than the `BlockWeight` value of the + runtime, we set `BlockWeight` to the size returned from the node. + +crates: + - name: cumulus-primitives-storage-weight-reclaim + bump: patch + - name: frame-system + bump: minor diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 0c6ff2cb8ddb..abacfa7b62cc 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -917,7 +917,7 @@ pub mod pallet { /// Total length (in bytes) for all extrinsics put together, for the current block. #[pallet::storage] - pub(super) type AllExtrinsicsLen = StorageValue<_, u32>; + pub type AllExtrinsicsLen = StorageValue<_, u32>; /// Map of block numbers to block hashes. #[pallet::storage]