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

Moved presets to the testnet runtimes #5327

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Aug 12, 2024

It is a first step for switching to the frame-omni-bencher for CI.

This PR includes several changes related to generating chain specs plus:

Closes: #5680

Follow-ups

@bkontur bkontur added T12-benchmarks This PR/Issue is related to benchmarking and weights. T14-system_parachains This PR/Issue is related to system parachains. R0-silent Changes should not be mentioned in any release notes labels Aug 12, 2024
@bkontur bkontur force-pushed the bko-get_preset-for-testnets branch from 63d05be to 932e369 Compare August 13, 2024 08:54
@bkontur
Copy link
Contributor Author

bkontur commented Aug 30, 2024

@michalkucharczyk I just saw your branch origin/mku-asset-hub-rococo-preset and your PR, I think you are doing the same as I here, but in this PR I've already refactored some of the testnet AssetHubs/BridgeHubs.

What if I update this branch to actual master and then we could merge your PR's changes here and then you could also continue with other runtimes here? wdyt?

@bkontur bkontur force-pushed the bko-get_preset-for-testnets branch from 932e369 to 80475ff Compare August 30, 2024 13:56
@michalkucharczyk
Copy link
Contributor

@michalkucharczyk I just saw your branch origin/mku-asset-hub-rococo-preset and your PR, I think you are doing the same as I here, but in this PR I've already refactored some of the testnet AssetHubs/BridgeHubs.

What if I update this branch to actual master and then we could merge your PR's changes here and then you could also continue with other runtimes here? wdyt?

Can we just merge my PR to master (as it is ready to be merged) and then you can merge master to this PR?
I am not sure who will work on reworking rest of runtimes as I am still invoved in #4639 .

testnet_parachains_constants::westend::currency::UNITS * 1_000_000,
1000.into(),
))
.with_genesis_config_patch(asset_hub_westend_development_genesis(para_id.into()))
Copy link
Contributor

@michalkucharczyk michalkucharczyk Aug 30, 2024

Choose a reason for hiding this comment

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

You shall use: with_genesis_config_preset_name.

This would allow to fetch the preset from the wasm blob, w/o necessity to natively compile the runtime.

I noticed the same in polkadot-fellows/runtimes#379 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I saw that fellows comment, it is still in my TODOs :)

@bkontur
Copy link
Contributor Author

bkontur commented Aug 30, 2024

@michalkucharczyk I just saw your branch origin/mku-asset-hub-rococo-preset and your PR, I think you are doing the same as I here, but in this PR I've already refactored some of the testnet AssetHubs/BridgeHubs.
What if I update this branch to actual master and then we could merge your PR's changes here and then you could also continue with other runtimes here? wdyt?

Can we just merge my PR to master (as it is ready to be merged) and then you can merge master to this PR? I am not sure who will work on reworking rest of runtimes as I am still invoved in #4639 .

ok, let's merge your and I will adjust and continue here with other testnets

@michalkucharczyk
Copy link
Contributor

@michalkucharczyk I just saw your branch origin/mku-asset-hub-rococo-preset and your PR, I think you are doing the same as I here, but in this PR I've already refactored some of the testnet AssetHubs/BridgeHubs.
What if I update this branch to actual master and then we could merge your PR's changes here and then you could also continue with other runtimes here? wdyt?

Can we just merge my PR to master (as it is ready to be merged) and then you can merge master to this PR? I am not sure who will work on reworking rest of runtimes as I am still invoved in #4639 .

ok, let's merge your and I will adjust and continue here with other testnets

Thank you

@michalkucharczyk
Copy link
Contributor

Not sure if you are interested - there is one more I plan to merge soon: #4739

@bkontur bkontur force-pushed the bko-get_preset-for-testnets branch 2 times, most recently from 89133a0 to 493ad6b Compare September 1, 2024 21:00
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7209375

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7209377

@bkontur
Copy link
Contributor Author

bkontur commented Sep 2, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Sep 2, 2024

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7214774 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". 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-3568b7dd-c695-4ae7-a3d4-30e9d1fb4ca3 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 2, 2024

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7214774 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7214774/artifacts/download.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team September 13, 2024 11:23
@bkontur
Copy link
Contributor Author

bkontur commented Sep 13, 2024

why not in GHA?

@mordamax You mean why not using short benchmarks from GHA with frame-omni-bencher?
Because we need to first merge: #5655 and then enabled shorts GHA - it is a follow up

@bkontur bkontur requested a review from a team as a code owner September 13, 2024 11:31
Copy link
Contributor

@mordamax mordamax left a comment

Choose a reason for hiding this comment

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

understood, although I think we could migrate rococo/westend already to omni anyways, as only some of the bridge ones fail until those fix?

@bkontur
Copy link
Contributor Author

bkontur commented Sep 13, 2024

Basically we need sth like this:

Let's create an issue for this. We can discuss it there if we want/need this.

-> #5700

@michalkucharczyk I could possibly also revert using RuntimeGenesisConfig and revert back json for the main level, and we can switch to RuntimeGenesisConfig as a part of #5700, wdyt revert or leave it as it is?

@michalkucharczyk
Copy link
Contributor

Basically we need sth like this:

Let's create an issue for this. We can discuss it there if we want/need this.

-> #5700

@michalkucharczyk I could possibly also revert using RuntimeGenesisConfig and revert back json for the main level, and we can switch to RuntimeGenesisConfig as a part of #5700, wdyt revert or leave it as it is?

Honestly, I don't know.
We never assumed any kind of compatibility between presets across different versions - so small chances that sth will be broken. With this in mind I think we can leave them as is, and try to align everything in #5700.

Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

LGTM.

Would be nice to add some tests verifying that presets are actually convertible to genesis storage - sample approach given here:

fn build_config_from_json_works() {

But this could be done in follow-up, especially in context of #5700.

@bkontur bkontur changed the title Add get_preset to the testnet runtimes Moved presets to the testnet runtimes Sep 18, 2024
@@ -66,6 +66,10 @@ pub type PresetId = sp_runtime::RuntimeString;
/// [`GenesisBuilder`] interface.
pub const DEV_RUNTIME_PRESET: &'static str = "development";

/// The default `local-testnet` preset used to communicate with the runtime via
/// [`GenesisBuilder`] interface.
pub const LOCAL_TESTNET_RUNTIME_PRESET: &'static str = "local_testnet";
Copy link
Contributor

@michalkucharczyk michalkucharczyk Sep 18, 2024

Choose a reason for hiding this comment

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

Sorry for nit-picking: maybe it would good to add some note on which preset is mandatory and which is optional? (Having these predefined names maybe somewhat confusing for runtime's maintainer in terms what is actually needed and how it will be used).

Seems that we kinda require "development" (e.g. for benchmarking, perhaps for omni-node - not sure what is the status there?).

And "local_testnet" is just for "our" runtimes? Not really required anywhere in generic tools?

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 see those constants as a unified good common "examples". It is hard to say which is mandatory or optional here.
I mean, the module sp-genesis-builder does not even know that some frame-omni-bencher exists and uses those constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T12-benchmarks This PR/Issue is related to benchmarking and weights. T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rococo benchmarks not working for polkadot_runtime_parachains::paras_inherent
9 participants