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

Beefy: add benchmarks for report_fork_voting() #5188

Merged
merged 21 commits into from
Aug 14, 2024

Conversation

serban300
Copy link
Contributor

@serban300 serban300 commented Jul 30, 2024

Related to #4523

This PR adds benchmarks for report_fork_voting().

Important: Even though the benchmarks are now available, we still use Weight::MAX. That's because I realized while working on this PR that there's still one missing piece. We should also check that the ancestry proof is optimal. I plan to do this in a future PR, hopefully the last one related to #4523.

@serban300 serban300 added the T15-bridges This PR/Issue is related to bridges. label Jul 30, 2024
@serban300 serban300 self-assigned this Jul 30, 2024
Otherwise the tests will fail with errors related to acessing the
offchain DB. That's related to the changes in pre previous commits. I
couldn't find another way to fix this.
@acatangiu acatangiu added R0-silent Changes should not be mentioned in any release notes and removed R0-silent Changes should not be mentioned in any release notes labels Aug 7, 2024
substrate/frame/beefy/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
@serban300
Copy link
Contributor Author

bot bench substrate-pallet --pallet=beefy_mmr

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=beefy_mmr (https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981779) was cancelled in #5188 (comment)

@serban300
Copy link
Contributor Author

bot cancel 1-3315d102-6309-4f16-98bd-90ba72271042

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=beefy_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981779 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981779/artifacts/download.

@serban300
Copy link
Contributor Author

bot bench polkadot-pallet --pallet=pallet_beefy_mmr --runtime=rococo
bot bench polkadot-pallet --pallet=pallet_beefy_mmr --runtime=westend

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981937 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=pallet_beefy_mmr. 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-34fa7219-8a87-4c39-b971-8be6f3ad18c3 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981938 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_beefy_mmr. 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 3-35692c93-9f08-4b64-9f6f-714703d53885 to cancel this command or bot cancel to cancel all commands in this pull request.

@serban300
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_beefy_mmr

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981942 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_beefy_mmr. 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 4-48f7d192-e508-4b8e-b84f-6a2a2884ad97 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=pallet_beefy_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981937 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981937/artifacts/download.

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_beefy_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981938 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981938/artifacts/download.

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_beefy_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981942 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6981942/artifacts/download.

@serban300
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_beefy_mmr
bot bench polkadot-pallet --pallet=pallet_beefy_mmr --runtime=rococo
bot bench polkadot-pallet --pallet=pallet_beefy_mmr --runtime=westend

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 option '--runtime ' argument 'rococo' is invalid. Allowed choices are dev.

@serban300
Copy link
Contributor Author

bot bench substrate-pallet --pallet=pallet_beefy_mmr

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988941 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_beefy_mmr. 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 11-577ae898-98ea-48d2-ad18-b7c1443b5e49 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 13, 2024

@serban300 Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_beefy_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988941 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6988941/artifacts/download.

@serban300
Copy link
Contributor Author

@acatangiu , @bkontur ,

Please, could you take another look ? I just want to check if there are any concerns related to this commit: 9fab84a . I needed a way to know whether we're in offchain context or not.

maybe also if someone from @paritytech/frame-coders could take a look it would be great

if sp_io::offchain::is_offchain_db_ext_available() {
sp_io::offchain::local_storage_set(StorageKind::PERSISTENT, key, value);
} else {
sp_io::offchain_index::set(key, value);
Copy link
Contributor

@acatangiu acatangiu Aug 14, 2024

Choose a reason for hiding this comment

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

Doesn't sp_io::offchain_index::set(key, value); check if extension is available internally?

What does it even do when offchain_db ext is not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIU sp_io::offchain_index::set(key, value); writes to a buffer, which is commited to offchain storage if the transaction succeeds and if the offchain_db ext is available.

The problem is that when running benchmarks, the values are not commited to offchain storage. But good point, I'll check why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's because the benchmark initialization code is outside of the extrinsic, and the changes that happen there are not commited anywhere. Anyway, rolled back 9fab84a and added a helper storage variable, only for runtime-benchmarks instead.

@serban300 serban300 added this pull request to the merge queue Aug 14, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2024
Related to #4523 

This PR adds benchmarks for `report_fork_voting()`.

**Important: Even though the benchmarks are now available, we still use
`Weight::MAX`. That's because I realized while working on this PR that
there's still one missing piece. We should also check that the ancestry
proof is optimal. I plan to do this in a future PR, hopefully the last
one related to #4523.**

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: command-bot <>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 14, 2024
@serban300 serban300 added this pull request to the merge queue Aug 14, 2024
Merged via the queue into paritytech:master with commit 81d8f0c Aug 14, 2024
172 of 175 checks passed
@serban300 serban300 deleted the beefy-equivocation-runtime branch August 14, 2024 19:00
ordian added a commit that referenced this pull request Aug 15, 2024
* master: (167 commits)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
  Fix OurViewChange small race (#5356)
  Make ticket non-optional and add ensure_successful method to Consideration trait (#5359)
  [tests] dedup test code, add more tests, improve naming and docs (#5338)
  Stop running the wishlist workflow on forks (#5297)
  Migrate foreign assets v3::Location to v4::Location (#4129)
  Minor clean up (#5284)
  [Pools] Ensure members can always exit the pool gracefully (#4998)
  StorageWeightReclaim: set to node pov size if higher (#5281)
  [Bot] Add prdoc generation (#5331)
  Small nits found accidentally along the way (#5341)
  Create subsystem-benchmarks.yml (#5325)
  Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232)
  Bump authoring duration for async backing to 2s. (#5195)
  Fix spelling issues (#5206)
  Bump the known_good_semver group across 1 directory with 3 updates (#5315)
  ...
ordian added a commit that referenced this pull request Aug 16, 2024
* master:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
ordian added a commit that referenced this pull request Aug 16, 2024
…ct-candidate-weight

* ao-fix-parainclusion-weight-overestimation:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants