WIP: implement StorageWeightReclaim as wrapping transaction extension with weight #5234

gui1117 · 2024-08-05T07:07:29Z

Implement StorageWeightReclaim as a transaction extension that wraps around all the transaction extensions.

This is because transaction extension weight is not part of base extrinsic weight. So we should trigger before all extensions and reclaim after all extensions to get accurate post info.

old StorageWeightReclaim is deprecated

TODO:

fix benchmarks so that ProofSizeExt is available or that the worst-case code is covered in some way
maybe modify benchmark scripts
execute benchmarks and link weights.
can be rewritten using 2 extesions: record and reclaim if prefered
prdoc

skunert · 2024-08-05T09:50:06Z

Very nice!

can be rewritten using 2 extesions: record and reclaim if prefered

How would the two communicate? Would we need a new StorageItem for them to read and write?
From the top of my head I currently don't see a benefit from splitting them, do you have anything in mind?

gui1117 · 2024-08-05T10:12:32Z

can be rewritten using 2 extesions: record and reclaim if prefered

How would the two communicate? Would we need a new StorageItem for them to read and write? From the top of my head I currently don't see a benefit from splitting them, do you have anything in mind?

extending the dispatch context to transaction extension also
https://paritytech.github.io/polkadot-sdk/master/frame_support/dispatch_context/index.html

or using a storage item.

But wrapping type seems ok.

cumulus/pallets/weight-reclaim-tx/src/benchmarks.rs

s0me0ne-unkn0wn · 2024-08-05T10:36:37Z

Having a storage item and writing into it in pre_dispatch would affect the proof size by itself (a classical "the observer influences what he observes"). It can be mitigated by adjusting the calculations by a benchmarked proof size of that operation, but the wrapper solution looks cleaner in that sense, anyway.

gui1117 · 2024-08-05T13:10:15Z

Having a storage item and writing into it in pre_dispatch would affect the proof size by itself (a classical "the observer influences what he observes"). It can be mitigated by adjusting the calculations by a benchmarked proof size of that operation, but the wrapper solution looks cleaner in that sense, anyway.

I thought writing into a storage without reading it before would not leak anything into the proof.

georgepisaltu

First round of review, looking good so far 👍

georgepisaltu · 2024-08-05T16:53:44Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

@@ -945,6 +945,10 @@ impl pallet_xcm_bridge_hub_router::Config<ToWestendXcmRouterInstance> for Runtim
 	type FeeAsset = xcm_config::bridging::XcmBridgeHubRouterFeeAssetId;
 }

+impl cumulus_pallet_weight_reclaim_tx::Config for Runtime {
+	type WeightInfo = ();


Would be nice to run a bot bench on this new pallet and get some weights once we're set on a final version of the code. Same for other runtimes.

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/tests/tests.rs

georgepisaltu · 2024-08-05T16:59:30Z

docs/sdk/src/guides/enable_pov_reclaim.rs

-//! add [`StorageWeightReclaim`](cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim)
-//! to that list. For maximum efficiency, make sure that `StorageWeightReclaim` is last in the list.
+//! set [`StorageWeightReclaim`](cumulus_pallet_weight_reclaim_tx::StorageWeightReclaim)
+//! as a warpper of that list.


I'd add something like "in order to catch the whole PoV size, it's required that this extension wraps all the other transaction extensions of the runtime.

done in ea81a0f

cumulus/pallets/weight-reclaim-tx/src/lib.rs

georgepisaltu · 2024-08-05T17:18:04Z

cumulus/pallets/weight-reclaim-tx/src/lib.rs

+	}
+}
+
+// Make this extension "invisible" from the outside (ie metadata type information)


Why would we not want this extension to show up in the metadata? Especially since this along with CheckMetadataHash would alert us if something changes to the extension pipeline, this extension included.

I don't know how UI code is written and if it expects a list of transaction extension or if it can handle more complex structure.

I copied the design from SkipIfCheckFeeless which is also invisible from the outside.

I don't mind anything, but we must make sure UI knows how to handle nested transaction extensions.

Until we can confirm the UI doesn't break or we get support in the UI, let's keep it invisible 👍

How would an extension that has no implication on the signed and raw payload the tx break any UI?

As in, with our without this extension, the same bytes are a valid tx, so...?

How would an extension that has no implication on the signed and raw payload the tx break any UI?

As in, with our without this extension, the same bytes are a valid tx, so...?

I expect UI frameworks to do some match on the transaction extension identifiers (which weight reclaim implements in an invisible way), but if they are doing some match on the transaction extension type to get information about what needs to be provided then it will break.

As far as I'm aware, client libraries (like Subxt) mostly just match on the identifiers, since they usually need to implement some sort of custom logic for how to handle each extension anyway. So if this extension is "invisible" in that way then I don't see it causing an issue offhand!

I fixed the implementation, it is still invisible, but should be correctly implemented. 375543d

georgepisaltu · 2024-08-05T17:20:04Z

cumulus/pallets/weight-reclaim-tx/src/lib.rs

+		// Trade-off: we could move it to `prepare` but better be accurate on reclaim than fast on
+		// `validate`


cumulus/pallets/weight-reclaim-tx/src/lib.rs

georgepisaltu · 2024-08-05T17:43:00Z

cumulus/pallets/weight-reclaim-tx/src/lib.rs

+		let inner_weight =
+			S::post_dispatch_details(inner_pre, info, post_info, len, result, context)?;


This should be computed using post_dispatch. The post_dispatch_details function should not be used explicitly because, unlike post_dispatch, it does not carry over the reduced weight through a mutable reference to the PostDispatchInfo. It would return the correct actual weight used, but it wouldn't update the post_info along the way, and some extensions rely on it, it's basically their weight meter.

For more info into the way it works, look at the TransactionExtension impl at substrate/primitives/runtime/src/traits/transaction_extension/mod.rs:498.

I wanted to make post_dispatch_details take a mutable reference to post_info as well, but I figured I would only encourage people to use it the unintended way.

The suggestion below can probably be nicer with a match, but the inner_weight calculation will change anyway once the post dispatch weight subtraction instead of accrual is implemented.

Suggested change

let inner_weight =

S::post_dispatch_details(inner_pre, info, post_info, len, result, context)?;

let mut actual_post_info = *post_info;

S::post_dispatch(inner_pre, info, &mut actual_post_info, len, result, context)?;

let inner_weight = actual_post_info

.actual_weight

.map(|actual_weight| actual_weight.saturating_sub(post_info.actual_weight.unwrap()));

I see, ~~maybe the post_dispatch_details implementation for tuples should log a warning, or have a debug error.~~

Implementing post_dispatch_details prevent us from returning the correct pays_fee if a transaction extension modified it.

It may be more accurate to implement post_dispatch I did at: 4d99a45

georgepisaltu · 2024-08-05T19:03:15Z

cumulus/pallets/weight-reclaim-tx/src/tests.rs

+}
+
+#[derive_impl(frame_system::config_preludes::TestDefaultConfig)]
+impl frame_system::Config for Test {


It probably needs a custom MockWeights: frame_system::ExtensionsWeightInfo that you can control and assert in tests for CheckWeight's post_dispatch. To make the tests more accurate, some mock weights can be used for this pallet as well, instead of ().

I set up configuration for better tests in eb0cdff but I will wait for the deccumulating/refining post dispatch info implementation.

Co-authored-by: georgepisaltu <[email protected]>

This reverts commit 4d99a45.

…gui-storage-proof-size-reclaim-more-accurate

paritytech-cicd-pr · 2024-08-10T05:17:43Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6960444

I propose to move PoV-reclaim to the next release. It can happen that the amount to reclaim is overestimated paritytech/polkadot-sdk#5229. Improvements (paritytech/polkadot-sdk#5281, paritytech/polkadot-sdk#5273) and a proper fix (paritytech/polkadot-sdk#5234) are in the works, but should be tested again. I expect the chances that the system-chains run into this scenario to be low, but it does not look like activity is so high that the inclusion of reclaim is urgent. --------- Co-authored-by: Bastian Köcher <[email protected]>

…gui-storage-proof-size-reclaim-more-accurate

gui1117 · 2024-10-19T16:54:51Z

somehow I can't reopen it. Moved to #6140

impl

76d3373

gui1117 force-pushed the gui-storage-proof-size-reclaim-more-accurate branch from b05e68c to 76d3373 Compare August 5, 2024 07:09

gui1117 mentioned this pull request Aug 5, 2024

PoV reclaim underestimates proof size #5229

Open

gui1117 commented Aug 5, 2024

View reviewed changes

cumulus/pallets/weight-reclaim-tx/src/benchmarks.rs Outdated Show resolved Hide resolved

fix missing benchmark config

ec44063

georgepisaltu reviewed Aug 5, 2024

View reviewed changes

gui1117 and others added 8 commits August 6, 2024 08:18

refactor

46e26c7

Co-authored-by: georgepisaltu <[email protected]>

remove from + refactor

fd01c34

doc

ea81a0f

fix warning in CI

51d91b9

Update cumulus/pallets/weight-reclaim-tx/src/lib.rs

30f8f4e

Co-authored-by: georgepisaltu <[email protected]>

Fix post_dispatch_details

4d99a45

Initial setup for more complete tests

eb0cdff

fmt

84a924b

s0me0ne-unkn0wn linked an issue Aug 6, 2024 that may be closed by this pull request

PoV reclaim underestimates proof size #5229

Open

gui1117 added 7 commits August 7, 2024 10:34

Revert "Fix post_dispatch_details"

1f937e6

This reverts commit 4d99a45.

fix post_dispatch_details

d9cf306

Merge remote-tracking branch 'origin/george/restore-gav-tx-ext' into …

ad4c64e

…gui-storage-proof-size-reclaim-more-accurate

Fix reclaim code and some test

16d87c2

fixup

413e0b2

Merge remote-tracking branch 'origin/george/restore-gav-tx-ext' into …

458df34

…gui-storage-proof-size-reclaim-more-accurate

fixup

c168718

skunert mentioned this pull request Aug 8, 2024

Remove pov-reclaim from kusama system-chains polkadot-fellows/runtimes#423

Merged

gui1117 added 3 commits August 10, 2024 10:58

Merge remote-tracking branch 'origin/george/restore-gav-tx-ext' into …

2752c44

…gui-storage-proof-size-reclaim-more-accurate

better naming

4e46cf1

fixup and fmt

93eca1c

gui1117 added 6 commits August 10, 2024 12:03

pallet default weight using template benchmark

c8c90e8

support old rustc version

b004477

fmt

b393b62

fmt and typo

e9e9125

fix licences

49b97e4

fix test service

076aa20

peterwht mentioned this pull request Aug 20, 2024

chore: upgrade v1.14.0 r0gue-io/pop-node#198

Merged

3 tasks

gui1117 mentioned this pull request Sep 18, 2024

FRAME: Reintroduce TransactionExtension as a replacement for SignedExtension #3685

Merged

gui1117 added 12 commits October 2, 2024 15:41

Merge remote-tracking branch 'origin/george/restore-gav-tx-ext' into …

ed9599c

…gui-storage-proof-size-reclaim-more-accurate

fix update

bb0c547

fix errors

ddb199d

fix impl

dd7e6b2

implement it similar to tuples

375543d

fix import

6b0ea4f

warnings

61cd608

prdoc

a05d6be

fix tests

c417a79

cleanup unused dependency and allow deprecated in some tests

09512ea

fix benchmark

16b8e2e

Merge remote-tracking branch 'origin/george/restore-gav-tx-ext' into …

b38a2f0

…gui-storage-proof-size-reclaim-more-accurate

gui1117 added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 5, 2024

gui1117 added 3 commits October 5, 2024 16:43

remove readme for new crate

0e0c0af

add dependency in doc crate

3b15c61

fmt

a005518

georgepisaltu deleted the branch paritytech:george/restore-gav-tx-ext October 18, 2024 18:29

georgepisaltu closed this Oct 18, 2024

gui1117 mentioned this pull request Oct 19, 2024

Implement cumulus StorageWeightReclaim as wrapping transaction extension + frame system ReclaimWeight #6140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: implement StorageWeightReclaim as wrapping transaction extension with weight #5234

WIP: implement StorageWeightReclaim as wrapping transaction extension with weight #5234

gui1117 commented Aug 5, 2024 •

edited

Loading

skunert commented Aug 5, 2024

gui1117 commented Aug 5, 2024

s0me0ne-unkn0wn commented Aug 5, 2024

gui1117 commented Aug 5, 2024

georgepisaltu left a comment

georgepisaltu Aug 5, 2024

georgepisaltu Aug 5, 2024

gui1117 Aug 6, 2024

georgepisaltu Aug 5, 2024

gui1117 Aug 5, 2024

georgepisaltu Aug 6, 2024

kianenigma Aug 6, 2024

gui1117 Aug 7, 2024

jsdw Aug 14, 2024

gui1117 Oct 2, 2024

georgepisaltu Aug 5, 2024

georgepisaltu Aug 5, 2024

gui1117 Aug 5, 2024 •

edited

Loading

gui1117 Aug 6, 2024

georgepisaltu Aug 5, 2024

gui1117 Aug 6, 2024

paritytech-cicd-pr commented Aug 10, 2024

gui1117 commented Oct 19, 2024 •

edited

Loading

		// Trade-off: we could move it to `prepare` but better be accurate on reclaim than fast on
		// `validate`

		let inner_weight =
		S::post_dispatch_details(inner_pre, info, post_info, len, result, context)?;

-		let inner_weight =
-			S::post_dispatch_details(inner_pre, info, post_info, len, result, context)?;
+		let mut actual_post_info = *post_info;
+		S::post_dispatch(inner_pre, info, &mut actual_post_info, len, result, context)?;
+		let inner_weight = actual_post_info
+			.actual_weight
+			.map(|actual_weight| actual_weight.saturating_sub(post_info.actual_weight.unwrap()));

WIP: implement StorageWeightReclaim as wrapping transaction extension with weight #5234

WIP: implement StorageWeightReclaim as wrapping transaction extension with weight #5234

Conversation

gui1117 commented Aug 5, 2024 • edited Loading

skunert commented Aug 5, 2024

gui1117 commented Aug 5, 2024

s0me0ne-unkn0wn commented Aug 5, 2024

gui1117 commented Aug 5, 2024

georgepisaltu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gui1117 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Aug 10, 2024

gui1117 commented Oct 19, 2024 • edited Loading

gui1117 commented Aug 5, 2024 •

edited

Loading

gui1117 Aug 5, 2024 •

edited

Loading

gui1117 commented Oct 19, 2024 •

edited

Loading