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

pallet-glutton: over-unity consumption #14338

Merged
merged 10 commits into from
Jun 13, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jun 9, 2023

The PoV benchmarking is over-estimating the actual proof size of storage reads.

Now after testing the pallet on Versi, we saw that it consumed 19% (compressed) PoV when requested to use 50%.
To make it possible to eventually consume the full 100%; over-unity values are needed for the proof and computation weight.

This is done by changing Perbill to FixedU64 with a range of [0, 10] that will be interpreted as [0, 1000]%.
As next value to archive 50% PoV consumption we could try a proof limit of 1.31 since 50/(19/50) = 131%.

Changes:

  • 🚨 Change pallet_glutton::{set_compute, set_storage} args from Perbill to FixedU64
  • 🚨 Same for GenesisConfig
  • Fix re-export of FixedU64 in sp-runtime and sp-arithmetic
  • Some pallet code cleanup.

👉 Pallet glutton is not (and never will be) advised for deployment on value-bearing chains. Note that there is no storage migration provided. Test chains using this need to be re-genesised.
Cumulus companion: paritytech/cumulus#2730

@NachoPal @Szegoo please let me know if you see something amiss.

ggwpez added 4 commits June 9, 2023 17:59
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 added A3-in_progress Pull request is in progress. No review needed at this stage. 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. T1-runtime This PR/Issue is related to the topic “runtime”. labels Jun 10, 2023
ggwpez added 2 commits June 10, 2023 16:38
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review June 10, 2023 16:42
@ggwpez ggwpez requested review from a team and NachoPal June 10, 2023 16:42
@Szegoo
Copy link
Contributor

Szegoo commented Jun 10, 2023

The changes seem good to me. The only question I have may be a bit off-topic.
Wouldn't it be a good idea if we had per_thing implementations that are not limited to 100%? If we had that we wouldn't have to use FixedU64 in situations like this.

frame/glutton/src/tests.rs Outdated Show resolved Hide resolved
frame/glutton/src/tests.rs Outdated Show resolved Hide resolved
frame/glutton/src/tests.rs Outdated Show resolved Hide resolved
frame/glutton/src/tests.rs Outdated Show resolved Hide resolved
Co-authored-by: Guillaume Yu Thiolliere <gui.thiolliere@gmail.com>
@ggwpez
Copy link
Member Author

ggwpez commented Jun 11, 2023

The changes seem good to me. The only question I have may be a bit off-topic.
Wouldn't it be a good idea if we had per_thing implementations that are not limited to 100%? If we had that we wouldn't have to use FixedU64 in situations like this.

The whole reason to have per_thing is to be in range [0, 1] with maximum precision. We just need this very often to express ratios.
If we enlarge the range, then it will be the same as FixedU*. There is nothing wrong with using FixedU64 here AFAIK, it has the same encoded size and should still be sufficiently precise in the range [0, 10].

@ggwpez ggwpez added the E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. label Jun 11, 2023
@NachoPal
Copy link
Contributor

@ggwpez Where did you see it is consuming only 19% of the proof_size?

@sandreim
Copy link
Contributor

@ggwpez Where did you see it is consuming only 19% of the proof_size?

https://grafana.parity-mgmt.parity.io/goto/tJ1Pf9l4g?orgId=1

2023-06-13 07:59:00.543  INFO tokio-runtime-worker cumulus-collator: [Parachain] PoV size { header: 0.1396484375kb, extrinsics: 3.22265625kb, storage_proof: 980.1328125kb } 

@ggwpez
Copy link
Member Author

ggwpez commented Jun 13, 2023

bot bench $ pallet dev pallet-glutton

@command-bot
Copy link

command-bot bot commented Jun 13, 2023

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2978867 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet-glutton. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-a1a7a114-3e34-491b-9c92-ed0c9bf8efd2 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jun 13, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet-glutton has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2978867 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2978867/artifacts/download.

@ggwpez
Copy link
Member Author

ggwpez commented Jun 13, 2023

bot bench $ pallet dev pallet-glutton

@command-bot
Copy link

command-bot bot commented Jun 13, 2023

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2979580 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet-glutton. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-86b3b6e3-b33c-4ed4-a418-0cddc89f0da0 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jun 13, 2023

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet-glutton has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2979580 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2979580/artifacts/download.

@ggwpez
Copy link
Member Author

ggwpez commented Jun 13, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e976964 into master Jun 13, 2023
@paritytech-processbot paritytech-processbot bot deleted the oty-glutton-overunity branch June 13, 2023 15:57
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* pallet-glutton: over-unity consumption

* Add hard limit

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Cleanup

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Highlight warning

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix docs

* Review test fixes

Co-authored-by: Guillaume Yu Thiolliere <gui.thiolliere@gmail.com>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet-glutton

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet-glutton

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Guillaume Yu Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants