Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

StorageWeightReclaim: set to node pov size if higher #5281

Merged
merged 9 commits into from
Aug 13, 2024
Merged

StorageWeightReclaim: set to node pov size if higher #5281

merged 9 commits into from
Aug 13, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Aug 8, 2024

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

@skunert skunert added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Aug 8, 2024
@skunert skunert requested a review from a team as a code owner August 8, 2024 09:43
@skunert skunert mentioned this pull request Aug 8, 2024
Merged
gui1117
gui1117 reviewed Aug 8, 2024
View reviewed changes
cumulus/primitives/storage-weight-reclaim/src/lib.rs Outdated
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."
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@skunert skunert requested a review from bkchr August 12, 2024 16:16
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Aug 13, 2024
@skunert @bkchr
Remove pov-reclaim from kusama system-chains (#423)
b2ae4df 
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 <git@kchr.de>
@skunert
Copy link
Contributor Author

skunert commented Aug 13, 2024

bot fmt

@command-bot
Copy link

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

@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
Copy link

command-bot bot commented Aug 13, 2024

@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.

lexnv
lexnv approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on my limited knowledge in this area!

@skunert skunert enabled auto-merge August 13, 2024 17:49
@skunert 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 skunert deleted the skunert/pov-reclaim-safeguard branch August 13, 2024 20:26
skunert added a commit that referenced this pull request Aug 13, 2024
@skunert
StorageWeightReclaim: set to node pov size if higher (#5281)
81a36ec 
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)
@skunert skunert mentioned this pull request Aug 13, 2024
Merged
EgorPopelyaev pushed a commit that referenced this pull request Aug 14, 2024
@skunert @gui1117
[stable2407] Backport PoV-reclaim fixes (#5273, #5281) (#5351)
b754e0b 
Backports #5273 & #5281

---------

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
ordian added a commit that referenced this pull request Aug 14, 2024
@ordian
Merge branch 'master' into ao-enact-candidate-weight
ef78697 
* 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)
  ...
ordian added a commit that referenced this pull request Aug 14, 2024
@ordian
Merge branch 'master' into ao-fix-parainclusion-weight-overestimation
cbe0b3f 
* 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)
  ...
ordian added a commit that referenced this pull request Aug 15, 2024
@ordian
Merge branch 'master' into ao-fix-backing-filtering-on-disputes
55f5782 
* 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)
  ...
skunert added a commit that referenced this pull request Aug 15, 2024
@skunert
StorageWeightReclaim: set to node pov size if higher (#5281)
fa32f16 
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)
@skunert skunert mentioned this pull request Aug 15, 2024
Merged
EgorPopelyaev pushed a commit that referenced this pull request Aug 15, 2024
@skunert @gui1117
[1.14] Backport PoV-reclaim fixes (#5273, #5281) (#5371)
21a1bf8 
Backports #5273 &
#5281

---------

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@skunert skunert mentioned this pull request Aug 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gui1117 gui1117 gui1117 left review comments

@s0me0ne-unkn0wn s0me0ne-unkn0wn s0me0ne-unkn0wn left review comments

@bkchr bkchr bkchr approved these changes

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@lexnv lexnv lexnv approved these changes

Assignees
No one assigned
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
SDK Node
Status: done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@skunert @bkchr @dmitry-markin @gui1117 @s0me0ne-unkn0wn @lexnv