Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Optional PoV block limits #13164

Closed
wants to merge 31 commits into from
Closed

Optional PoV block limits #13164

wants to merge 31 commits into from

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 17, 2023

This MR makes it possible to have optional block weight limits for Ref Time and Proof Size.
Additionally to this it will emit a PovLimitExceeded and print a warning if a block consumes more than 5 MiB of proof size.

🚨 breaking changes:

  • WeightsPerClass changed the type of max_extrinsic, max_total and reserved from Option<Weight> to WeightLimit.
  • The types for the BlockWeights thereby slightly changed from Option<Weight> to WeightLimit.
  • Make the CheckWeight signed extension consider optional weight limits.
  • Fix arithmetical semantic of PerDispatchClass:: since the naming was off.
    • add renamed to saturating_accrue
    • checked_add renamed to checked_accrue
    • sub renamed to saturating_reduce

Non breaking additions:

  • sp_weights::WeightLimit which can be used to express an upper limit for the consumption of some weight.
  • frame_system::SOFT_POV_LIMIT_BYTES and Warning + Event for blocks that consume more weight.
  • sp_weights::Weight::{from_all, without_ref_time, without_proof_size, saturating_reduce, checked_accrue, checked_reduce}

Integration Guide (For all chains)

Change your MAXIMUM_BLOCK_WEIGHT from type Weight to WeightLimit and use it in the BlockWeights::builder().
The difference is here that before we had to use u64::MAX, whereas now we can make it actually unlimited.

// Old Code using a `Some`
.for_class(DispatchClass::Normal, |weights| {
	weights.max_total = Some(NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT);
})

// Cew Code using an `into()`
.for_class(DispatchClass::Normal, |weights| {
	weights.max_total = NORMAL_DISPATCH_RATIO * MAXIMUM_BLOCK_WEIGHT;
})

Before and after example from the kitchensink of this MR:
image
And the block weight builder:
image

You can now also specify a soft PoV limit by calling .pov_soft_limit(…) on the builder. Polkadot et al uses 5 MiB for this limit.

Other errors can arise from testing code.

  • If you call expect on the optional limits: Use .exact_limits().expect() instead.
  • If you call unwrap_or(Something) on on the optional limits: Use .limited_or(Something), .limited_or_max() or .limited_or_min() instead.

TODO:

  • Extend MR description
  • Add tests for WeightLimit
  • Integration guide
  • Companions

Polkadot companion: paritytech/polkadot#6580
Cumulus companion: paritytech/cumulus#2117

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 17, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
This is probably what we have to do for all runtimes.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review January 18, 2023 00:42
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 18, 2023
@ggwpez ggwpez added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 18, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Comment on lines +440 to +446
// This is kind of a hack, but since the mandatory needs to happen *anyway*, we can
// exclude it from the regular `max_block`. The reason is that you normally want
// `unlimited` mandatory, which will always lead to a `max_block` of `(u64::MAX,
// u64::MAX)` as well.
if class == &DispatchClass::Mandatory {
continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The compromise that i had to make.

@ggwpez ggwpez added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 19, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2290397

@ggwpez
Copy link
Member Author

ggwpez commented Jan 20, 2023

Okay this is not really what we want.
The two things I can still salvage from it:

  • The WeightLimit type since it uses unlimited instead of the hacky u64::MAX
  • The PoV limit printing on > 5 MiB

@ggwpez ggwpez closed this Jan 20, 2023
@ggwpez ggwpez deleted the oty-optional-pov-limit branch April 24, 2023 12:44
@ggwpez ggwpez restored the oty-optional-pov-limit branch April 24, 2023 12:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants