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

check-weight: Disable total pov size check for mandatory extrinsics #4571

Merged
merged 6 commits into from
May 27, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented May 24, 2024

So in some pallets we like here we use max_block as return value for on_initialize (ideally we would not).

This means the block is already full when we try to apply the inherents, which lead to the error seen in #4559 because we are unable to include the required inherents. This was not erroring before #4326 because we were running into this branch:

// There is either no limit in reserved pool (`None`),
// or we are below the limit.
_ => {},

The inherents are of DispatchClass::Mandatory and therefore have a reserved value of None in all runtimes I have inspected. So they will always pass the normal check.

So in this PR I adjust the check_combined_proof_size to return an early Ok(()) for mandatory extrinsics.

If we agree on this PR I will backport it to the 1.12.0 branch.

closes #4559

@skunert skunert requested review from bkchr and ggwpez May 24, 2024 16:25
@skunert skunert requested a review from a team as a code owner May 24, 2024 16:25
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

One nitpick, otherwise it looks good.

substrate/frame/system/src/extensions/check_weight.rs Outdated Show resolved Hide resolved
@bkchr bkchr added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label May 24, 2024
@svyatonik
Copy link
Contributor

bot fmt

@command-bot
Copy link

command-bot bot commented May 27, 2024

@svyatonik https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6305487 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 2-09b6ac02-d579-4b11-ba34-0de7f22762fa to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented May 27, 2024

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

@skunert skunert enabled auto-merge May 27, 2024 09:13
bkchr pushed a commit that referenced this pull request May 27, 2024
…tory extrinsics (#4592)

Backport of #4571

---------

Co-authored-by: command-bot <>
@skunert skunert added this pull request to the merge queue May 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 27, 2024
@skunert skunert enabled auto-merge May 27, 2024 16:41
@skunert skunert added this pull request to the merge queue May 27, 2024
Merged via the queue into master with commit 70dd67a May 27, 2024
141 of 152 checks passed
@skunert skunert deleted the skunert/check-weight-ignore-mandatory branch May 27, 2024 17:39
EgorPopelyaev pushed a commit that referenced this pull request May 30, 2024
…tory extrinsics (#4592)

Backport of #4571

---------

Co-authored-by: command-bot <>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
…aritytech#4571)

So in some pallets we like
[here](https://github.com/paritytech/polkadot-sdk/blob/5dc522d02fe0b53be1517f8b8979176e489a388b/substrate/frame/session/src/lib.rs#L556)
we use `max_block` as return value for `on_initialize` (ideally we would
not).

This means the block is already full when we try to apply the inherents,
which lead to the error seen in paritytech#4559 because we are unable to include
the required inherents. This was not erroring before paritytech#4326 because we
were running into this branch:

https://github.com/paritytech/polkadot-sdk/blob/e4b89cc50c8d17868d6c8b122f2e156d678c7525/substrate/frame/system/src/extensions/check_weight.rs#L222-L224

The inherents are of `DispatchClass::Mandatory` and therefore have a
`reserved` value of `None` in all runtimes I have inspected. So they
will always pass the normal check.

So in this PR I adjust the `check_combined_proof_size` to return an
early `Ok(())` for mandatory extrinsics.

If we agree on this PR I will backport it to the 1.12.0 branch.

closes paritytech#4559

---------

Co-authored-by: command-bot <>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…aritytech#4571)

So in some pallets we like
[here](https://github.com/paritytech/polkadot-sdk/blob/5dc522d02fe0b53be1517f8b8979176e489a388b/substrate/frame/session/src/lib.rs#L556)
we use `max_block` as return value for `on_initialize` (ideally we would
not).

This means the block is already full when we try to apply the inherents,
which lead to the error seen in paritytech#4559 because we are unable to include
the required inherents. This was not erroring before paritytech#4326 because we
were running into this branch:

https://github.com/paritytech/polkadot-sdk/blob/e4b89cc50c8d17868d6c8b122f2e156d678c7525/substrate/frame/system/src/extensions/check_weight.rs#L222-L224

The inherents are of `DispatchClass::Mandatory` and therefore have a
`reserved` value of `None` in all runtimes I have inspected. So they
will always pass the normal check.

So in this PR I adjust the `check_combined_proof_size` to return an
early `Ok(())` for mandatory extrinsics.

If we agree on this PR I will backport it to the 1.12.0 branch.

closes paritytech#4559

---------

Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asset-hub-rococo: Extrinsic exceeds total pov size
5 participants