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

CheckWeight SE: Check for extrinsic length + proof size combined #4326

Merged
merged 7 commits into from
May 13, 2024

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Apr 29, 2024

Currently the CheckWeight SignedExtension was tracking the size of the proof and the extrinsic length separately. But in reality we need one more check that ensures we don't hit the PoV limit with both combined.

The rest of the logic remains unchanged. One scenario where the changes make a difference is when we enter this branch:

match limit_per_class.reserved {
// We are over the limit in reserved pool.
Some(reserved) if per_class.any_gt(reserved) => {
log::debug!(
target: LOG_TARGET,
"Total block weight is exceeded.",
);
return Err(InvalidTransaction::ExhaustsResources.into())
},
// There is either no limit in reserved pool (`None`),
// or we are below the limit.
_ => {},
}

This was previously allowing to some extrinsics that is exceeding the block limit but are withing the reserved area of BlockWeights. This will now be caught by the later check I introduced. I think the new behaviour makes sense, since the proof size dimension is designed for parachains and they don't want to go over the limit and get rejected.

In the long run we should maybe get rid of RuntimeBlockLength alltogether, however that would require a deprecation process and can come at a later point.

@skunert skunert added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 29, 2024
@skunert skunert requested a review from bkchr April 29, 2024 12:53
@skunert skunert requested a review from a team as a code owner April 29, 2024 12:53
@skunert skunert requested a review from ggwpez May 6, 2024 13:43
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.

Thank you!

@skunert skunert enabled auto-merge May 13, 2024 08:36
@skunert skunert added this pull request to the merge queue May 13, 2024
Merged via the queue into master with commit 6d3a6d8 May 13, 2024
146 of 148 checks passed
@skunert skunert deleted the skunert/unify-length-tracking branch May 13, 2024 09:28
ordian added a commit that referenced this pull request May 14, 2024
* master:
  improve MockValidationDataInherentDataProvider to support async backing (#4442)
  Bump `proc-macro-crate` to the latest version (#4409)
  [ci] Run check-runtime-migration in GHA (#4441)
  prospective-parachains rework (#4035)
  [ci] Add forklift to GHA ARC (#4372)
  `CheckWeight` SE: Check for extrinsic length + proof size combined (#4326)
  Add generate and verify logic for `AncestryProof` (#4430)
  Rococo AH: undeploy trie migration (#4414)
  Remove `substrate-frame-cli` (#4403)
  migrations: `take()`should consume read and write operation weight (#4302)
  `remote-externalities`: store block header in snapshot (#4349)
  xcm-emlator: Use `BlockNumberFor` instead of `parachains_common::BlockNumber=u32` (#4434)
  Remove `pallet::getter` usage from authority-discovery pallet (#4091)
  Remove pallet::getter usage from pallet-contracts-mock-network (#4417)
  Add docs to request_core_count (#4423)
github-merge-queue bot pushed a commit that referenced this pull request May 27, 2024
…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 #4559 because we are unable to include
the required inherents. This was not erroring before #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 #4559

---------

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

Currently the `CheckWeight` `SignedExtension` was tracking the size of
the proof and the extrinsic length separately. But in reality we need
one more check that ensures we don't hit the PoV limit with both
combined.

The rest of the logic remains unchanged. One scenario where the changes
make a difference is when we enter this branch:

https://github.com/paritytech/polkadot-sdk/blob/f34d8e3cf033e2a22a41b505c437972a5dc83d78/substrate/frame/system/src/extensions/check_weight.rs#L185-L198

This was previously allowing to some extrinsics that is exceeding the
block limit but are withing the reserved area of `BlockWeights`. This
will now be caught by the later check I introduced. I think the new
behaviour makes sense, since the proof size dimension is designed for
parachains and they don't want to go over the limit and get rejected.


In the long run we should maybe get rid of `RuntimeBlockLength`
alltogether, however that would require a deprecation process and can
come at a later point.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
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 <>
liuchengxu pushed a commit to liuchengxu/polkadot-sdk that referenced this pull request Jun 19, 2024
…aritytech#4326)

Currently the `CheckWeight` `SignedExtension` was tracking the size of
the proof and the extrinsic length separately. But in reality we need
one more check that ensures we don't hit the PoV limit with both
combined.

The rest of the logic remains unchanged. One scenario where the changes
make a difference is when we enter this branch:

https://github.com/paritytech/polkadot-sdk/blob/f34d8e3cf033e2a22a41b505c437972a5dc83d78/substrate/frame/system/src/extensions/check_weight.rs#L185-L198

This was previously allowing to some extrinsics that is exceeding the
block limit but are withing the reserved area of `BlockWeights`. This
will now be caught by the later check I introduced. I think the new
behaviour makes sense, since the proof size dimension is designed for
parachains and they don't want to go over the limit and get rejected.


In the long run we should maybe get rid of `RuntimeBlockLength`
alltogether, however that would require a deprecation process and can
come at a later point.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…aritytech#4326)

Currently the `CheckWeight` `SignedExtension` was tracking the size of
the proof and the extrinsic length separately. But in reality we need
one more check that ensures we don't hit the PoV limit with both
combined.

The rest of the logic remains unchanged. One scenario where the changes
make a difference is when we enter this branch:

https://github.com/paritytech/polkadot-sdk/blob/f34d8e3cf033e2a22a41b505c437972a5dc83d78/substrate/frame/system/src/extensions/check_weight.rs#L185-L198

This was previously allowing to some extrinsics that is exceeding the
block limit but are withing the reserved area of `BlockWeights`. This
will now be caught by the later check I introduced. I think the new
behaviour makes sense, since the proof size dimension is designed for
parachains and they don't want to go over the limit and get rejected.


In the long run we should maybe get rid of `RuntimeBlockLength`
alltogether, however that would require a deprecation process and can
come at a later point.

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
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
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants