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

Add development chain-spec file for minimal/parachain templates for Omni Node compatibility #6529

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

Conversation

EleisonC
Copy link

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

  1. Overview of Changes:
  • Added development chain specs for use in the minimal and parachain template.
  • Updated zombienet-omni-node.toml and zombinet.toml files in the minimal and parachain templates to include paths to the new dev chain specs.
  1. Integration Guidance:
    NB: Follow the templates' READMEs from the polkadot-SDK master branch. Please build the binaries and runtimes based on the polkadot-SDK master branch.
  • Ensure you have set up your runtimes parachain-template-runtime and minimal-template-runtime
  • Ensure you have installed the nodes required ie parachain-template-node and minimal-template-node
  • Set up Zombinet
  • For running the parachains, you will need to install the polkadot cargo install --path polkadot remember from the polkadot-SDK master branch.
  • Inside the template folders minimal or parachain, run the command to start with Zombienet with Omni Node, Zombienet with minimal-template-node or Zombienet with parachain-template-node

Include your leftover TODOs, if any, here.

  • Test the syncing of chain specs with runtime's code.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Nov 18, 2024

User @EleisonC, please sign the CLA here.

@iulianbarbu iulianbarbu self-requested a review November 19, 2024 15:03
@iulianbarbu iulianbarbu added T17-Templates This PR/Issue is related to templates T11-documentation This PR/Issue is related to documentation. labels Nov 19, 2024
@iulianbarbu iulianbarbu added the R0-silent Changes should not be mentioned in any release notes label Nov 19, 2024
Copy link
Contributor

@iulianbarbu iulianbarbu left a 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?

templates/minimal/zombienet.toml Outdated Show resolved Hide resolved
templates/parachain/zombienet.toml Outdated Show resolved Hide resolved
@EleisonC
Copy link
Author

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?

@iulianbarbu
Copy link
Contributor

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 ChainSpec as described here: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/node/src/chain_spec.rs#L36, and then call as_json -> and then convert it to serde_json::Value, and then read the newly added file and load its content as a serde_json::Value and then compare the two.

@EleisonC
Copy link
Author

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

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Nov 21, 2024

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 runtime initially. I think it makes more sense to place it under runtime so that if anything changes here, running the tests for the crate would fail because the changes will not be inline with dev_chain_spec.json. The node uses the logic defined in the runtime, so it would benefit from the runtime test, but not directly. I don't think we need to add a test on the same lines in the templates' nodes crate either because these nodes aren't depending on the dev_chain_spec.json file. Only OmniNode depends on this file being in sync with the runtime, and that not because it would fail otherwise, but because it would start an outdated version.

@EleisonC
Copy link
Author

EleisonC commented Nov 25, 2024

From the top of my head I thought about adding an integration test. You could build a ChainSpec as described here: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/node/src/chain_spec.rs#L36, and then call as_json -> and then convert it to serde_json::Value, and then read the newly added file and load its content as a serde_json::Value and then compare the two.

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
Is it the value of code? Because the test fails when I compare the code value of the two chain spec files.
let test_runtime_code = test_json_value // .get("genesis") // .expect("failed to extract genesis") // .get("runtimeGenesis") // .expect("failed to extract runtimeGenesis from genesis") // .get("code") // .expect("failed to extract code from runtimeGenesis");
But the code value of test_runtime and the existing file do not match. Hence wondering if it would be the Top value to or maybe I am missing a step of conversion somewhere

@EleisonC
Copy link
Author

From the top of my head I thought about adding an integration test. You could build a ChainSpec as described here: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/node/src/chain_spec.rs#L36, and then call as_json -> and then convert it to serde_json::Value, and then read the newly added file and load its content as a serde_json::Value and then compare the two.

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 Is it the value of code? Because the test fails when I compare the code value of the two chain spec files. let test_runtime_code = test_json_value // .get("genesis") // .expect("failed to extract genesis") // .get("runtimeGenesis") // .expect("failed to extract runtimeGenesis from genesis") // .get("code") // .expect("failed to extract code from runtimeGenesis"); But the code value of test_runtime and the existing file do not match. Hence wondering if it would be the Top value to or maybe I am missing a step of conversion somewhere

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

@iulianbarbu
Copy link
Contributor

From the top of my head I thought about adding an integration test. You could build a ChainSpec as described here: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/node/src/chain_spec.rs#L36, and then call as_json -> and then convert it to serde_json::Value, and then read the newly added file an

d load its content as a serde_json::Value and then compare the two.

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 Is it the value of code? Because the test fails when I compare the code value of the two chain spec files. let test_runtime_code = test_json_value // .get("genesis") // .expect("failed to extract genesis") // .get("runtimeGenesis") // .expect("failed to extract runtimeGenesis from genesis") // .get("code") // .expect("failed to extract code from runtimeGenesis"); But the code value of test_runtime and the existing file do not match. Hence wondering if it would be the Top value to or maybe I am missing a step of conversion somewhere

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

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 omni_node_* tests like this one: https://github.com/paritytech/polkadot-sdk/blob/master/templates/zombienet/tests/smoke.rs#L160. We can update them in a follow up, no need to consider them right now, but it is up to you.

As usual, LMK in case of blockers, happy to take a look.

@EleisonC
Copy link
Author

Hey @iulianbarbu,
I added a test and would appreciate it if you could look at it and approve it before I add the one for the para chain. Additionally, I added the test folder to nodes because I was encountering setup issues.

Copy link
Contributor

@iulianbarbu iulianbarbu left a 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.

templates/minimal/node/tests/validate_dev_chain_spec.rs Outdated Show resolved Hide resolved
templates/minimal/node/tests/validate_dev_chain_spec.rs Outdated Show resolved Hide resolved
templates/minimal/node/tests/validate_dev_chain_spec.rs Outdated Show resolved Hide resolved
templates/minimal/node/Cargo.toml Outdated Show resolved Hide resolved
@iulianbarbu
Copy link
Contributor

iulianbarbu commented Jan 6, 2025

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 polkadot-sdk, in any crate which is a dependency of any of the two runtimes (minimal or parachain). At this point I am leaning towards @skunert idea of dropping the CI job, considering it will bring attention with most PRs and its benefit - ensuring the chain specs that come with the templates are up to date for usage of templates based on polkadot-sdk repo, when we aim in fact for usage from their dedicated GH repos (e.g. https://github.com/paritytech/polkadot-sdk-parachain-template).

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 OmniNode. LMK if you think differently.
I also believe the CI job effort can conclude nicely in this script you're proposing, which can be used anytime we want to update the chain specs for polkadot-sdk (as a nice to have, we can also print some notes right before the script is doing its thing, about the merging of master with the branch for the case of a PR, in case we want to update the chain spec as it would be done in CI), or when we generate the chain specs in the misc-sync-templates action. This script can install srtool if not present and check the environment looks fine (having at least podman installed). The script can be added as a follow up though (but it is your choice, given the misc-sync-templates.yml would contain similar commands, and could very well use a script that can do the same separately from the action).

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.

@EleisonC
Copy link
Author

EleisonC commented Jan 6, 2025

Hey @EleisonC ! First of all, sorry for my silence, it has been the end of the year and I took some needed time off.

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. 😅

Second of all, I have to admit I totally missed the aspect of this CI job failing at every minor change done in polkadot-sdk, in any crate which is a dependency of any of the two runtimes (minimal or parachain). At this point I am leaning towards @skunert idea of dropping the CI job, considering it will bring attention with most PRs and its benefit - ensuring the chain specs that come with the templates are up to date for usage of templates based on polkadot-sdk repo, when we aim in fact for usage from their dedicated GH repos (e.g. https://github.com/paritytech/polkadot-sdk-parachain-template).

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 OmniNode. LMK if you think differently.

Based on this my Next steps are going to be to :

  • - Remove the current CI job that verifies chain specs
  • - Add chain spec generation to the misc-sync-templates.yml workflow
  • - Generate specs only during template synchronization
  • - Update the ReadMe

I also believe the CI job effort can conclude nicely in this script you're proposing, which can be used anytime we want to update the chain specs for polkadot-sdk (as a nice to have, we can also print some notes right before the script is doing its thing, about the merging of master with the branch for the case of a PR, in case we want to update the chain spec as it would be done in CI), or when we generate the chain specs in the misc-sync-templates action. This script can install srtool if not present and check the environment looks fine (having at least podman installed). The script can be added as a follow up though (but it is your choice, given the misc-sync-templates.yml would contain similar commands, and could very well use a script that can do the same separately from the action).

This can be done in another PR. Its still hints at one of the earlier solutions
Create a utility script that:
Installs srtool if not present
Verifies at least podman installation
Generates chain specs on demand
Prints informative notes about branch merging
Can be used both manually and by the sync workflow

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.

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.

@EleisonC
Copy link
Author

Hey @iulianbarbu, I’d love to hear your thoughts on my approach and whether it aligns with the expectations.
cc: @skunert – I would appreciate your perspective as well.

Copy link
Contributor

@iulianbarbu iulianbarbu left a 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.

.github/workflows/misc-sync-templates.yml Outdated Show resolved Hide resolved
templates/minimal/README.md Outdated Show resolved Hide resolved
templates/minimal/README.md Outdated Show resolved Hide resolved
templates/minimal/README.md Outdated Show resolved Hide resolved
templates/minimal/README.md Outdated Show resolved Hide resolved
templates/parachain/README.md Outdated Show resolved Hide resolved
templates/parachain/README.md Outdated Show resolved Hide resolved
@iulianbarbu
Copy link
Contributor

iulianbarbu commented Jan 17, 2025

Other than this I plan to sync the templates with stable2412 release today & hopefully this will work without significant issues.

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 Plan.toml (it was added after the tag was made). We'll need to wait for the stable2412-1 planned for next week, its tag, and then try to sync again.

Copy link
Contributor

@skunert skunert left a 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 :)!

.github/workflows/misc-sync-templates.yml Outdated Show resolved Hide resolved
templates/minimal/README.md Outdated Show resolved Hide resolved
@EleisonC
Copy link
Author

Looking really good, only minor nits!

Thanks again for seeing this through :)!

Thank you for your guidance and feedback! I've addressed the minor nits. 😊

Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looking good to me now! @iulianbarbu please verify the introduced workflow once more when you are back, then we are ready to go!

- name: Prepare upload directory
run: |
mkdir -p artifacts-${{ matrix.template }}
cp dev_chain_spec.json artifacts-${{ matrix.package_name }}/dev_chain_spec.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are using matrix.package_name instead of matrix.template, I think this is a mismatch.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the catch. I have addressed it

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

Just felt we can make the docs a bit clearer, so left a few clarifying comments, besides a few others for the misc-sync-templates.yml. It is mostly there :D

Comment on lines +100 to +110
- uses: actions/checkout@v4
with:
path: polkadot-sdk
ref: "${{ github.event.inputs.stable_release_branch }}"

- name: Download template artifacts
uses: actions/download-artifact@v4
with:
name: artifacts-${{ matrix.template }}
path: templates/${{ matrix.template }}/
if: matrix.template != 'solochain'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the first action checkout here? We're doing it once more below, after setting the git identity. I am thinking we can also move the Download template artifacts below the actions checkout that runs after Configure git identity. What do you think?

- 👤 The template has no consensus configured - it is best for experimenting with a single node network.


- For the most up-to-date chain specification, refer to [staging-chain-spec-builder](https://crates.io/crates/staging-chain-spec-builder)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this note from the intro. It isn't very relevant from an introduction standpoint for the template, and the latest info for the chain spec isn't necessarily part of staging-chain-spec-builder. I would keep the #Intro section as before.

You don't need to add a mention about staging-chain-spec-builder in other place either. We already have it mentioned in the steps below (e.g. under Starting a Minimal Template Chain/Omni Node/Install staging-chain-spec-builder.

Comment on lines +156 to +158
To simplify the process of starting the minimal template with zombienet and Omni Node, we've added a pre-configured
development chain spec (dev_chain_spec.json) to the minimal template. The zombienet-omni-node.toml file of this template
points to it, but you can update it to an updated path to the new chain spec generated on your machine, as shown below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To simplify the process of starting the minimal template with zombienet and Omni Node, we've added a pre-configured
development chain spec (dev_chain_spec.json) to the minimal template. The zombienet-omni-node.toml file of this template
points to it, but you can update it to an updated path to the new chain spec generated on your machine, as shown below.
To simplify the process of starting the minimal template with zombienet and Omni Node, we've added a pre-configured
development chain spec (dev_chain_spec.json) to the minimal template. The zombienet-omni-node.toml file of this template
points to it, but you can update it to an updated path to the new chain spec generated on your machine.

My initial suggestion was to move this right into the section: Update zombienet-omni-node.toml with a valid chain spec path. It can be placed right at the beginning of the section, but then I would change the next paragraph: Before starting... by removing the first sentence.

Comment on lines +150 to +153

To simplify the process of using the parachain-template with zombienet and Omni Node, we've added a pre-configured
development chain spec (dev_chain_spec.json) to the parachain template. The zombienet-omni-node.toml file of this
template points to it, but you can update it to an updated chain spec generated on your machine, as shown below:
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly the same as for minimal

- template: parachain
package_name: 'parachain-template-runtime'
runtime_path: './templates/parachain/runtime'
runtime_wasm_path: 'minimal-template-runtime/minimal_template_runtime.compact.compressed.wasm'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
runtime_wasm_path: 'minimal-template-runtime/minimal_template_runtime.compact.compressed.wasm'
runtime_wasm_path: 'parachain-template-runtime/parachain_template_runtime.compact.compressed.wasm'

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 T11-documentation This PR/Issue is related to documentation. T17-Templates This PR/Issue is related to templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants