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.

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

Make on_idle WeightMeter factor in block size #4743

Closed
skunert opened this issue Jun 10, 2024 · 0 comments · Fixed by #4765
Closed

Make on_idle WeightMeter factor in block size #4743

skunert opened this issue Jun 10, 2024 · 0 comments · Fixed by #4765
Assignees
Labels
I2-bug The node fails to follow expected behavior.

Comments

@skunert
Copy link
Contributor

skunert commented Jun 10, 2024

We want to get rid of the current defensive limit on pov size defined here:

(validation_data.max_pov_size / 2) as usize,

With #4326 in place we are already close.
There is one more scenario we need to fix before we can safely enable the full size there. It can happen that on_idle consumes more weight than what is available and exceed the PoV size.

Example for this scenario:

  • The check introduced in CheckWeight SE: Check for extrinsic length + proof size combined #4326 reports that that no more extrinsics can be included in a block because weight + block length exceed the PoV size.
  • One or more pallets have an on_idle hook defined that gets called with the rest weight. However, since block length and storage proof weight are tracked separately, the remaining weight passed to on_idle will be too high.

Solutions:

  1. We unify the tracking of block length and storage proof weight. This needs adjustment of the check introduced in CheckWeight SE: Check for extrinsic length + proof size combined #4326. If we go this route one needs to double check the pov-reclaim logic for adjustments.
  2. We subtract the block length in frame-executor before we calculate the remaining weight to be passed in to the hook. This is more of a quick fix, while 1. would tackle the problem globally.
@skunert skunert added the I2-bug The node fails to follow expected behavior. label Jun 10, 2024
@skunert skunert added this to SDK Node Jun 10, 2024
@github-project-automation github-project-automation bot moved this to backlog in SDK Node Jun 10, 2024
@sandreim sandreim self-assigned this Jun 11, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 14, 2024
Fix #4743 which allows
us to remove the defensive limit on pov size in Cumulus after relay
chain gets upgraded with these changes. Also add unit test to ensure
`CheckWeight` - `StorageWeightReclaim` integration works.

TODO:
- [x] PRDoc
- [x] Add a len to all the other tests in storage weight reclaim and
call `CheckWeight::pre_dispatch`

---------

Signed-off-by: Andrei Sandu <[email protected]>
@github-project-automation github-project-automation bot moved this from backlog to done in SDK Node Jun 14, 2024
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Fix paritytech#4743 which allows
us to remove the defensive limit on pov size in Cumulus after relay
chain gets upgraded with these changes. Also add unit test to ensure
`CheckWeight` - `StorageWeightReclaim` integration works.

TODO:
- [x] PRDoc
- [x] Add a len to all the other tests in storage weight reclaim and
call `CheckWeight::pre_dispatch`

---------

Signed-off-by: Andrei Sandu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior.
Projects
Status: done
Development

Successfully merging a pull request may close this issue.

2 participants