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

Properly set the max proof size weight on defaults and tests #12383

Merged
merged 16 commits into from
Sep 29, 2022

Conversation

KiChjang
Copy link
Contributor

Part of paritytech/polkadot-sdk#256.

This PR sets the default proof size weight correctly, so that it passes unit tests as intended.

The default max proof size is set at u64::MAX, because not all substrate-based chains require submitting PoV blocks to the relay -- standalone sovereign chains can safely ignore this weight component.

For parachains however, the proper parameter to set for the max proof size comes from the relay chain, and is stored a field called max_pov_size in this struct: https://github.com/paritytech/cumulus/blob/032540dbf9afc9f5a0d5db7279fc81048f3a168b/pallets/parachain-system/src/lib.rs#L526.

@KiChjang KiChjang added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 28, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Much needed, thanks.

You chose u64::MAX as a default since it would get rejected by any sensible relay-chain config (if used in a para-chain)?

@KiChjang
Copy link
Contributor Author

You chose u64::MAX as a default since it would get rejected by any sensible relay-chain config?

Because it allows chains that don't care about PoV sizes to safely ignore this weight component. Maybe I should write this in the docs somewhere so that people know that they should use u64::MAX for the proof size if their chain doesn't need to care about it.

@shawntabrizi
Copy link
Member

if their chain doesn't need to care about it.

I guess this brings to mind that maybe this should be an Option value then... but i think for simplicity we don't need that.

But generally speaking, this kind of thing is what Rust is good at describing with explicit types, so just want to call that out.

@bkchr
Copy link
Member

bkchr commented Sep 28, 2022

But generally speaking, this kind of thing is what Rust is good at describing with explicit types, so just want to call that out.

Exactly what I said on element :P

@KiChjang
Copy link
Contributor Author

I guess this brings to mind that maybe this should be an Option value then... but i think for simplicity we don't need that.

I thought about this, but it actually doesn't help much, because there's actually two default values: one defaults to max where it's being used to determine the maximum proof size for a block, and the other defaults to 0 where it's sensible for measuring the proof size of an actual operation.

Even if we create new types for this, we can't escape the fact that whenever you define the max block for a chain, you'd still have to manually define the max proof size, rather than relying on the 0 default. And if we try and do it the other way around and make the default u64::MAX instead, then we need to be very careful and weigh extrinsics appropriately, otherwise they would never be able to be placed in a block due to it exceeding max block.

/// We allow for 2 seconds of compute with a 6 second average block time.
const MAXIMUM_BLOCK_WEIGHT: Weight = WEIGHT_PER_SECOND.saturating_mul(2);
/// We allow for 2 seconds of compute with a 6 second average block time, with maximum proof size.
const MAXIMUM_BLOCK_WEIGHT: Weight = WEIGHT_PER_SECOND.saturating_mul(2).set_proof_size(u64::MAX);
Copy link
Member

Choose a reason for hiding this comment

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

In a general note, this will not work for Parachains. Parachains have the max proof size defined by the relay chain and this value could change any block (it isn't but it is a value that is changeable on the relay chain). So, we would need to have a dynamic way of getting the max 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.

@bkchr getting the weight of the pov from the validation data is a solution here right? Because, this "could" change from the relay side dynamically, or can we just assume that MAX_POV_SIZE from the primitives is always good to use statically?

@bkchr
Copy link
Member

bkchr commented Sep 29, 2022

Even if we create new types for this, we can't escape the fact that whenever you define the max block for a chain, you'd still have to manually define the max proof size, rather than relying on the 0 default. And if we try and do it the other way around and make the default u64::MAX instead, then we need to be very careful and weigh extrinsics appropriately, otherwise they would never be able to be placed in a block due to it exceeding max block.

Maybe we should get rid off the Default implementation for Weight at all. This would probably change a lot of code, but would it make more obvious to give these values some sensible values.

@KiChjang
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 61b9a4d into master Sep 29, 2022
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/proper-proof-size branch September 29, 2022 15:48
ggwpez added a commit that referenced this pull request Sep 30, 2022
bkontur pushed a commit that referenced this pull request Oct 3, 2022
* Properly set the max proof size weight on defaults and tests

* cargo fmt

* Set proper max proof size for contracts pallet tests

* Properly set max proof size for node

* Properly set max proof size for frame system mock

* Update test expectations

* Update test expectations

* Properly set max proof size for balances mock

* Update test expectations

* Update test expectations

* Properly set max proof size for democracy mock

* Properly set max proof size for scheduler mock

* Properly set max proof size for fast unstake mock

* Properly set max proof size for tx payment mock

* Properly set max proof size for elections phragmen mock

* Properly set max proof size for node template
coderobe added a commit that referenced this pull request Oct 4, 2022
ordian added a commit that referenced this pull request Oct 5, 2022
* master: (42 commits)
  Adapt `pallet-contracts` to WeightV2 (#12421)
  Improved election pallet testing (#12327)
  Bump prost to 0.11+ (#12419)
  Use saturating add for alliance::disband witness data (#12418)
  [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416)
  client/beefy: small code improvements (#12414)
  BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
  Add @koute to `docs/CODEOWNERS` and update stale paths (#12408)
  docs/CODEOWNERS: add @acatangiu as MMR owner (#12406)
  Remove unnecessary Clone trait bounds on CountedStorageMap (#12402)
  Fix `Weight::is_zero` (#12396)
  Beefy on-demand justifications as a custom RequestResponse protocol (#12124)
  Remove contracts RPCs (#12358)
  pallet-mmr: generate historical proofs (#12324)
  unsafe_pruning flag removed (#12385)
  Carry over where clauses defined in Config to Call and Hook (#12388)
  Properly set the max proof size weight on defaults and tests (#12383)
  BEEFY: impl TypeInfo for SignedCommitment (#12382)
  bounding staking: `BoundedElectionProvider` trait (#12362)
  New Pallet: Root offences (#11943)
  ...
coderobe added a commit that referenced this pull request Oct 6, 2022
* Revert "Add storage size component to weights (#12277)"

This reverts commit 17c07af.

* Revert "Properly set the max proof size weight on defaults and tests (#12383)"

This reverts commit 61b9a4d.
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ech#12383)

* Properly set the max proof size weight on defaults and tests

* cargo fmt

* Set proper max proof size for contracts pallet tests

* Properly set max proof size for node

* Properly set max proof size for frame system mock

* Update test expectations

* Update test expectations

* Properly set max proof size for balances mock

* Update test expectations

* Update test expectations

* Properly set max proof size for democracy mock

* Properly set max proof size for scheduler mock

* Properly set max proof size for fast unstake mock

* Properly set max proof size for tx payment mock

* Properly set max proof size for elections phragmen mock

* Properly set max proof size for node template
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants