StorageWeightReclaim: set to node pov size if higher #5281

skunert · 2024-08-08T09:43:24Z

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

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

gui1117 · 2024-08-08T10:01:43Z

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

+				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}. Setting to node-side proof size."
+				);


sounds good indeed.

I also think we could formalize the concept of weight reservation. So that on_initialize can reserve some weight for on_finalize.
I believe that extrinsic should also be able to reserve some weight if they trigger some logic in on_finalize.

I also think we could formalize the concept of weight reservation. So that on_initialize can reserve some weight for on_finalize.
I believe that extrinsic should also be able to reserve some weight if they trigger some logic in on_finalize.

I don't have a full overview of all the implications of this. But for sure it would help making the math here more accurate. This is one of the reasons why we need to work with the diffs instead of just setting the proof size to the node variant in the first place.

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]>

skunert · 2024-08-13T13:57:39Z

bot fmt

command-bot · 2024-08-13T13:57:45Z

@skunert https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6991121 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 12-ac3bd57e-e67b-40b4-87bd-f9dbc0b9a677 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-08-13T13:59:34Z

@skunert Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6991121 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6991121/artifacts/download.

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

lexnv

LGTM based on my limited knowledge in this area!

@gui1117

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 055eb53)

Backports #5273 & #5281 --------- Co-authored-by: Guillaume Thiolliere <[email protected]>

* master: (35 commits) Fix OurViewChange small race (#5356) Make ticket non-optional and add ensure_successful method to Consideration trait (#5359) [tests] dedup test code, add more tests, improve naming and docs (#5338) Stop running the wishlist workflow on forks (#5297) Migrate foreign assets v3::Location to v4::Location (#4129) Minor clean up (#5284) [Pools] Ensure members can always exit the pool gracefully (#4998) StorageWeightReclaim: set to node pov size if higher (#5281) [Bot] Add prdoc generation (#5331) Small nits found accidentally along the way (#5341) Create subsystem-benchmarks.yml (#5325) Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232) Bump authoring duration for async backing to 2s. (#5195) Fix spelling issues (#5206) Bump the known_good_semver group across 1 directory with 3 updates (#5315) `polkadot-node-core-pvf-common`: Fix test compilation error (#5310) ci: Paused `cmd-action` commenter (#5287) Remove unnecessary mut (#5318) chain-spec: minor clarification on the genesis config patch (#5324) fix av-distribution Jaeger spans mem leak (#5321) ...

* master: (167 commits) Upgrade accidentally downgraded deps (#5365) [Pools] Fix issues with member migration to `DelegateStake` (#4822) Unify `no_genesis` check (#5360) [CI] Fix prdoc command (#5358) Beefy: add benchmarks for `report_fork_voting()` (#5188) Fix OurViewChange small race (#5356) Make ticket non-optional and add ensure_successful method to Consideration trait (#5359) [tests] dedup test code, add more tests, improve naming and docs (#5338) Stop running the wishlist workflow on forks (#5297) Migrate foreign assets v3::Location to v4::Location (#4129) Minor clean up (#5284) [Pools] Ensure members can always exit the pool gracefully (#4998) StorageWeightReclaim: set to node pov size if higher (#5281) [Bot] Add prdoc generation (#5331) Small nits found accidentally along the way (#5341) Create subsystem-benchmarks.yml (#5325) Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232) Bump authoring duration for async backing to 2s. (#5195) Fix spelling issues (#5206) Bump the known_good_semver group across 1 directory with 3 updates (#5315) ...

@gui1117

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 055eb53)

Backports #5273 & #5281 --------- Co-authored-by: Guillaume Thiolliere <[email protected]>

skunert added 2 commits August 8, 2024 11:12

Set runtime proof size to node side proof size if higher

1221c09

Wording

7cc3b60

skunert added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Aug 8, 2024

skunert requested a review from s0me0ne-unkn0wn August 8, 2024 09:43

skunert requested a review from a team as a code owner August 8, 2024 09:43

skunert mentioned this pull request Aug 8, 2024

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

Merged

s0me0ne-unkn0wn reviewed Aug 8, 2024

View reviewed changes

cumulus/primitives/storage-weight-reclaim/src/lib.rs Outdated Show resolved Hide resolved

prdoc

1ac18d3

gui1117 reviewed Aug 8, 2024

View reviewed changes

skunert added 2 commits August 9, 2024 09:31

Update prdoc

1eddfe7

Merge branch 'master' into skunert/pov-reclaim-safeguard

6b290de

skunert requested a review from bkchr August 12, 2024 16:16

Improve tests, take into account proof size after reclaim

29ad78c

".git/.scripts/commands/fmt/fmt.sh"

b825be2

dmitry-markin approved these changes Aug 13, 2024

View reviewed changes

cumulus/primitives/storage-weight-reclaim/src/lib.rs Outdated Show resolved Hide resolved

lexnv approved these changes Aug 13, 2024

View reviewed changes

skunert added 2 commits August 13, 2024 19:45

Improve test

e1a777b

Merge branch 'master' into skunert/pov-reclaim-safeguard

824d14f

skunert enabled auto-merge August 13, 2024 17:49

bkchr approved these changes Aug 13, 2024

View reviewed changes

skunert added this pull request to the merge queue Aug 13, 2024

Merged via the queue into master with commit 055eb53 Aug 13, 2024
174 of 175 checks passed

skunert deleted the skunert/pov-reclaim-safeguard branch August 13, 2024 20:26

skunert mentioned this pull request Aug 13, 2024

[stable2407] Backport PoV-reclaim fixes (#5273, #5281) #5351

Merged

EgorPopelyaev pushed a commit that referenced this pull request Aug 14, 2024

[stable2407] Backport PoV-reclaim fixes (#5273, #5281) (#5351)

b754e0b

Backports #5273 & #5281 --------- Co-authored-by: Guillaume Thiolliere <[email protected]>

skunert mentioned this pull request Aug 15, 2024

[1.14] Backport PoV-reclaim fixes (#5273, #5281) #5371

Merged

EgorPopelyaev pushed a commit that referenced this pull request Aug 15, 2024

[1.14] Backport PoV-reclaim fixes (#5273, #5281) (#5371)

21a1bf8

Backports #5273 & #5281 --------- Co-authored-by: Guillaume Thiolliere <[email protected]>

skunert mentioned this pull request Aug 16, 2024

PoV reclaim underestimates proof size #5229

Open

github-actions bot mentioned this pull request Oct 8, 2024

Update polkadot-sdk from stable2407 to stable2409 moonbeam-foundation/moonbeam#2994

Closed

skunert mentioned this pull request Oct 24, 2024

Enable PoV reclaim in all system chains polkadot-fellows/runtimes#494

Open

gui1117 added a commit to gui1117/polkadot-sdk that referenced this pull request Nov 6, 2024

Keep accrue when node proof size is bigger: paritytech#5281

1ad6e56

github-actions bot mentioned this pull request Dec 5, 2024

Update polkadot-sdk from stable2407 to stable2409-2 moonbeam-foundation/moonbeam#3075

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StorageWeightReclaim: set to node pov size if higher #5281

StorageWeightReclaim: set to node pov size if higher #5281

skunert commented Aug 8, 2024

gui1117 Aug 8, 2024

skunert Aug 8, 2024

skunert commented Aug 13, 2024

command-bot bot commented Aug 13, 2024 •

edited

Loading

command-bot bot commented Aug 13, 2024

lexnv left a comment

StorageWeightReclaim: set to node pov size if higher #5281

StorageWeightReclaim: set to node pov size if higher #5281

Conversation

skunert commented Aug 8, 2024

gui1117 Aug 8, 2024

Choose a reason for hiding this comment

skunert Aug 8, 2024

Choose a reason for hiding this comment

skunert commented Aug 13, 2024

command-bot bot commented Aug 13, 2024 • edited Loading

command-bot bot commented Aug 13, 2024

lexnv left a comment

Choose a reason for hiding this comment

command-bot bot commented Aug 13, 2024 •

edited

Loading