-
Notifications
You must be signed in to change notification settings - Fork 779
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
Add development chain-spec file for minimal/parachain templates for Omni Node compatibility #6529
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @EleisonC ! Left a few comments for now.
Looking forward to the README updates & btw, have you clarified how to make a rust test for the chain specs?
Hey @iulianbarbu that is what I am looking into today. Do you have any pointers you'd like me to know as I move forward? |
From the top of my head I thought about adding an integration test. You could build a |
Hey @iulianbarbu one last question it's okay if I placed the test folder/file under the nodes folder/file. if not is there a better place |
Hmm, thought about placing it under |
Hey @iulianbarbu regarding comparing the two I am a bit stuck on what to compare. From my research I would think it's other the code or value of top |
I figured out my problem was around decoding a Hex String to bytes. I can now properly traverse the values of the Top section and find the code for that spec file |
d load its content as a
Happy to hear you've sorted it out! I've noticed that in some tests we don't compare the code portions of the generated chain specs, and that's mainly because they are not super relevant for the test case. For this test though I think that it is relevant, to ensure generated chain specs correspond to the runtimes in the CI. Some other good news is that once we'll have these chain specs we'll be able to easily run in the CI the As usual, LMK in case of blockers, happy to take a look. |
Signed-off-by: EleisonC <[email protected]>
Hey @iulianbarbu, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please regenerate the dev_chain_spec.json
for minimal (for now) after building the minimal/runtime
with cargo build
? I wonder if you built the runtime with --release
and then generated the dev_chain_spec.json
. It makes sense according to the README, but we run the tests with the testnet
profile, which enables the debug symbols, and I think the chain spec code
section differs based on the profile. I would be curious to run the test again after you regenerate the dev_chain_spec.json
, and also see if regenerating on my machine results in a different code section, or chain spec altogheter.
I am getting the same as your posted snippet from CI:
Did you maybe mix up your build command? I am using |
sorry should have added the commands |
Hmm at this point its not really possible for me to say why you are ending up with a different hash. Have you merged master here in the PR but not pulled to your local machine, so that you are on a different commit? Or your have local changes? These issues aside, after thinking about it more I am against introducing this CI job. Everytime there is a minor change in any of the runtime related code of polkadot-sdk, the generated WASM blob will change, forcing developers to update this chain-spec constantly (because the job will fail). A better way to handle this IMO is to just have the chainspec committed and update it manually. For sure this is annoying and the spec could potentially go out of sync. But having an extra CI job for it seems like overkill. I am sorry for the work you have already put into the CI job. Can you please remove it? |
…adot-sdk into add-dev-chain-spec
Hey @skunert cc: @iulianbarbu . I appreciate the help and the time you were able to allocate to this. I want to first address this issue ☝️, After thorough testing, I can confirm that this issue has been resolved and the workflows run okay Workflow Run Personal Fork. The CI job and local development environments now consistently produce the same Blake2_256 hash for the runtime WASM blob, ensuring alignment with the Polkadot SDK master's Runtime determined using the Srtool.
I completely understand your concern regarding the added maintenance burden of a CI job that enforces frequent updates to the chain spec due to minor runtime changes. Your suggestion to commit the chain spec manually and update it as needed is reasonable; however, I'd like to propose an enhancement that maintains the benefits of automation while keeping the workflow lightweight and developer-friendly.
|
Hey, the problem with (1) is that this will not break now and then when there is a change to the parachain template runtime, that would be okay. But polkadot-sdk is merging lots of commits per week. Imagine someone is creating a pull request to modify some log line here for example. In their pull request the CI job would fail because that code is a dependency of parachain-template-runtime and they would need to regenerate the chain-spec. Whe question is also what the benefit of keeping the chain-spec up to date in polkadot-sdk really is. Once per release these two template repositories are synchronized with the current state of polkadot-sdk:
This is where I expect new users to go to try out these templates. Ideally, the job that synchronizes the templates with the polkadot-sdk would generate the chain-specs from the current runtime. This way, the chain-specs in the template repositories would be correct on every release. I like your proposal of (2), having a script in the repo to generate these chainspecs. The flow could look like this:
This way the chain-spec in the template repositories is always up-to-date and we don't have the burden of this constantly failing CI job. |
Hey @EleisonC ! First of all, sorry for my silence, it has been the end of the year and I took some needed time off. Second of all, I have to admit I totally missed the aspect of this CI job failing at every minor change done in Generating the chain specs once in this job: https://github.com/paritytech/polkadot-sdk/blob/master/.github/workflows/misc-sync-templates.yml should be enough IMO, from the point of view of ensuring up to date chain specs (in relation to the code found in their repo), and ease the use when it comes to Lastly, wanted to say that your work here was a great learning experience for me too. Would be happy to assist and review any followup changes if you can invest some more time here. LMK your thoughts. |
Hey @iulianbarbu I agree the time-off it is needed. I did the same. It was Christmas I bet it was enough haha. Information overload can happen quickly. 😅
Based on this my Next steps are going to be to :
This can be done in another PR. Its still hints at one of the earlier solutions
This is work I am proud of and definitely committed to seeing through. Also, I appreciate your continued guidance and support it makes the process easier. |
Hey @iulianbarbu, I’d love to hear your thoughts on my approach and whether it aligns with the expectations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @EleisonC ! This is very nice and looks about done, except a few things that can be improved.
I plan to sync the templates with stable2412 release today & hopefully this will work without significant issues. Once that's done you can read every template readme as if it would be part of the dedicated template repos. This is just as FYI, I don't think you need to change more than what I suggested with this review, for this PR.
Other than this, would be great to open an issue related to the script we've discussed about, where we'll apply all this learnings about how to generate reproducible chain specs, if needed.
templates/minimal/README.md
Outdated
For the most accurate chain spec generation, `staging-chain-spec-builder` or other mentioned binaries and runtimes, | ||
please try to build them based on the Polkadot-sdk most updated master branch an example: | ||
```bash | ||
cargo install --path substrate/bin/utils/chain-spec-builder | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good note, but I wouldn't add it anymore. Latest staging-chain-spec-builder
is good enough for what this readme needs: https://crates.io/crates/staging-chain-spec-builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need --locked
here. Please doublecheck all the install commands. cargo install
by default does not use the lockfile.
templates/minimal/README.md
Outdated
@@ -81,15 +95,18 @@ be used to run the minimal template's runtime. `polkadot-omni-node` binary crate | |||
|
|||
Please see installation section on [crates.io/omni-node](https://crates.io/crates/polkadot-omni-node). | |||
|
|||
#### Build `minimal-template-runtime` | |||
#### Build `minimal-template-runtime` (A reminder to build on the most updated version of polkadot-sdk master) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the reminder addition. We still waant people to refer to the dedicated minimal-template repository, so building out of polkadot-sdk
can be done, but isn't the user flow we're looking. This readme will end up in this minimal-template dedicated repo, so we shouldn't refer back to polkadot-sdk
from there.
templates/minimal/README.md
Outdated
```bash | ||
cargo install --path substrate/bin/utils/chain-spec-builder | ||
``` | ||
|
||
Please see the installation section at [`crates.io/staging-chain-spec-builder`](https://crates.io/crates/staging-chain-spec-builder). | ||
Or Please see the installation section at [`crates.io/staging-chain-spec-builder`](https://crates.io/crates/staging-chain-spec-builder). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to revert this because we will assume this readme lives in https://github.com/paritytech/polkadot-sdk-minimal-template.
templates/parachain/README.md
Outdated
- 🔧 To simplify the process of using Zombienet with Omni Node, we’ve added a pre-configured | ||
development chain spec (dev_chain_spec.json) to the parachain template. Additionally, | ||
the zombienet-omni-node.toml file has been updated to include a valid chain spec path | ||
pointing to dev_chain_spec.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for minimal template
templates/parachain/README.md
Outdated
- For the most accurate chain spec generation, `staging-chain-spec-builder` or other mentioned | ||
binaries and runtimes,please try to build them based on the Polkadot-sdk most updated master branch | ||
an example: | ||
```bash | ||
cargo install --path substrate/bin/utils/chain-spec-builder | ||
``` | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as for minimal template
Can't be done yet. Sync job is still failing because it tries to update the templates' repos Cargo.toml deps versions based on the stable2412 tag, which doesn't have a commited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good, only minor nits!
Thanks again for seeing this through :)!
run: | | ||
sudo apt-get update | ||
sudo apt-get install -y protobuf-compiler | ||
cargo install --git https://github.com/chevdor/srtool-cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add --locked
to these git install commands.
templates/minimal/README.md
Outdated
For the most accurate chain spec generation, `staging-chain-spec-builder` or other mentioned binaries and runtimes, | ||
please try to build them based on the Polkadot-sdk most updated master branch an example: | ||
```bash | ||
cargo install --path substrate/bin/utils/chain-spec-builder | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need --locked
here. Please doublecheck all the install commands. cargo install
by default does not use the lockfile.
Co-authored-by: Iulian Barbu <[email protected]>
Co-authored-by: Iulian Barbu <[email protected]>
no need
Thank you for your guidance and feedback! I've addressed the minor nits. 😊 |
Description
This PR adds development chain specs for the minimal and parachain templates. #6334
Integration
This PR adds development chain specs for the minimal and para chain template runtimes, ensuring synchronization with runtime code. It updates zombienet-omni-node.toml, zombinet.toml files to include valid chain spec paths, simplifying configuration for zombienet in the parachain and minimal template.
Review Notes
NB: Follow the templates' READMEs from the polkadot-SDK master branch. Please build the binaries and runtimes based on the polkadot-SDK master branch.
parachain-template-runtime
andminimal-template-runtime
parachain-template-node
andminimal-template-node
cargo install --path polkadot
remember from the polkadot-SDK master branch.Zombienet with Omni Node
,Zombienet with minimal-template-node
orZombienet with parachain-template-node
Include your leftover TODOs, if any, here.