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

proper executor/block type for benchmarks and try-runtime #2771

Merged
merged 4 commits into from
Mar 31, 2021
Merged

proper executor/block type for benchmarks and try-runtime #2771

merged 4 commits into from
Mar 31, 2021

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Mar 30, 2021 •
edited
Loading

closes #2516

What I don't get is that when I run these two commands with --chain=polkadot and inspect the executor logs, I get polkadot-0 as the spec-version, but when I do --chain=polkadot-dev I get polkadot-30. Is this why we also run benchmarks always with -dev variants? And why is this?

Either way, that coded needed refactoring.

@kianenigma kianenigma 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Mar 30, 2021
@shawntabrizi
Copy link
Member

shawntabrizi commented Mar 31, 2021 •
edited by kianenigma
Loading

I think there is a typo in your post. Do you mean --chain=polkadot-dev? (FIXED)

--chain=polkadot uses the hardcoded genesis with a specific wasm file which DOES NOT contain benchmarking or your try-runtime apis. So --chain=polkadot will not work for this kind of stuff, and is a pretty common error. --chain=polkadot-dev on the other hand uses a compile / execution time generated genesis which will be modified based on your compiler flags.

shawntabrizi
shawntabrizi reviewed Mar 31, 2021
View reviewed changes
cli/src/command.rs Outdated Show resolved Hide resolved
shawntabrizi
shawntabrizi reviewed Mar 31, 2021
View reviewed changes
cli/src/command.rs Outdated
.map_err(|e| Error::SubstrateCli(e))
})?)
} else {
panic!("can only use benchmarks with --chain value of [polkadot, kusama, westend], got {}", spec_name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
panic!("can only use benchmarks with --chain value of [polkadot, kusama, westend], got {}", spec_name);
panic!("can only use benchmarks with --chain value of [polkadot-dev, kusama-dev, westend-dev], got {}", spec_name);

shawntabrizi
shawntabrizi approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

👍

@kianenigma
Copy link
Contributor Author

--chain=polkadot uses the hardcoded genesis with a specific wasm file which DOES NOT contain benchmarking or your try-runtime apis

Which hardcoded genesis exactly?

I see, so my observation was expected, but I still don't get why we cannot compile the real polkadot node+runtime, with these additional apis piggybacked?

@shawntabrizi
Copy link
Member

shawntabrizi commented Mar 31, 2021 •
edited
Loading

Which hardcoded genesis exactly?

The genesis of the real Polkadot blockchain :)

https://github.com/paritytech/polkadot/tree/master/node/service/res

@kianenigma
Copy link
Contributor Author

Which hardcoded genesis exactly?

The genesis of the real Polkadot blockchain :)

https://github.com/paritytech/polkadot/tree/master/node/service/res

Huh, I did not know that existed :D

bkchr
bkchr suggested changes Mar 31, 2021
View reviewed changes
cli/src/command.rs Outdated Show resolved Hide resolved
@kianenigma kianenigma requested a review from bkchr March 31, 2021 12:50
@bkchr bkchr merged commit 6da658b into master Mar 31, 2021
@bkchr bkchr deleted the kiz-executor-try-runtime branch March 31, 2021 18:40
ordian added a commit that referenced this pull request Mar 31, 2021
@ordian
Merge branch 'master' into ao-fix-race-in-statement-distribution
fc506dd 
* master:
  proper executor/block type for benchmarks and try-runtime (#2771)
  Fix future-polling loop in availability and add a better early-exit (#2779)
ordian added a commit that referenced this pull request Mar 31, 2021
@ordian
Merge branch 'master' into ao-do-no-try-to-connect-if-we-are-no-valid…
f78992a 
…ators

* master:
  statement-distribution: do not use OurViewChange (#2790)
  Better timeout values now that we are going to be connected to all nodes. (#2778)
  proper executor/block type for benchmarks and try-runtime (#2771)
  Fix future-polling loop in availability and add a better early-exit (#2779)
@kianenigma kianenigma mentioned this pull request Apr 18, 2021
Merged
@kianenigma kianenigma mentioned this pull request Jul 20, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use correct block and executor type for benchmarking and testing CLI
3 participants
@kianenigma @shawntabrizi @bkchr