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

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Aug 31, 2022

This PR provides a mechanism for pushing digest items into the benchmark overhead and benchmark extrinsic pipelines. It is a potential fix for #12142. The rationale for it is thoroughly described there, but briefly: this solves the dependency problem where an inherent requires a pre-runtime digest to be present.

Note that I tested a similar fix through Moonbeam against an older version of Substrate (v0.9.26) but have not tested the code in this PR. I'll be happy to do so if there is support for it in the first place.

Companions

TODO

  • no-op companion PRs
  • test - can be accomplished by implementing in nimbus which is where this all started
  • any documentation required (minor breaking changes)

polkadot companion: paritytech/polkadot#5959

@bkchr bkchr requested a review from ggwpez August 31, 2022 20:14
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.

Nice.

It still needs companions and a fmt. In the future we can think about putting all of these args in a config struct to have fewer companion requirements.

@ggwpez ggwpez added A0-please_review Pull request needs code review. B5-clientnoteworthy 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit and removed D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Sep 1, 2022
@notlesh
Copy link
Contributor Author

notlesh commented Sep 1, 2022

I am working on implementing benchmark overhead/extrinsic in nimbus because I'll be able to test this change there (the problematic inherent/digest pair lives in that repo).

However, I believe this is mergeable as-is.
Nevermind, Cumulus needs a companion apparently.

I believe this will build if CI will run against my polkadot companion (am I missing something?)

@ggwpez
Copy link
Member

ggwpez commented Sep 1, 2022

I believe this will build if CI will run against my polkadot companion (am I missing something?)

Looks like it. But it also needs a Cumulus comp, since we also expose these commands there…

@notlesh
Copy link
Contributor Author

notlesh commented Sep 1, 2022

I believe this will build if CI will run against my polkadot companion (am I missing something?)

Looks like it. But it also needs a Cumulus comp, since we also expose these commands there…

I don't think it does unless it's compiling a branch other than master (I can't tell from the CI log). Cumulus master looks like it doesn't implement BenchmarkCmd::Overhead or BenchmarkCmd::Extrinsic.

It looks to me like the CI failure here is coming out of polkadot, example:

/builds/parity/mirrors/substrate/extra_dependencies/polkadot/cli/src/command.rs:555:14
1950    |
1951555 | ...                   cmd.run(client.clone(), inherent_data, &ext_factory)
1952    |                           ^^^ --------------  -------------  ------------ supplied 3 arguments
1953    |                           |
1954    |                           expected 4 arguments

Is cumulus not being built against my polkadot companion?

@ggwpez
Copy link
Member

ggwpez commented Sep 1, 2022

bot merge

I don't think it does unless it's compiling a branch other than master…

Yea you are right. Cumulus just ignores these commands (currently).
PS: You need to tick a bot in the comp MR.

@paritytech-processbot
Copy link

Error: Github API says "Allow edits from maintainers" is not enabled for paritytech/polkadot#5958. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

@notlesh
Copy link
Contributor Author

notlesh commented Sep 1, 2022

I recreated my companion PR from my personal repo so I could enable edits from maintainers. Sorry for the mess...

New PR: paritytech/polkadot#5959

@bkchr
Copy link
Member

bkchr commented Sep 2, 2022

bot merge

@paritytech-processbot
Copy link

Error: Github API says #12159 is not mergeable

@ggwpez
Copy link
Member

ggwpez commented Sep 2, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 1c0d008 into paritytech:master Sep 2, 2022
@notlesh notlesh deleted the notlesh-add-digest-to-benchmark-overhead branch September 3, 2022 16:13
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add benchmarking support for digest items

* fmt
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. 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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants