-
Notifications
You must be signed in to change notification settings - Fork 696
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
Change the chain to Rococo in the parachain template Zombienet config #5279
Conversation
@pepoviola what is the easiest way we can write a "test" for a zombienet config to make sure they are correct, and producing blocks? |
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!
@rzadp have you confirmed that end to end, the other templates you have created work? Like just simply following the readme that is included in the template is something that people will be able to do successfully? This is all being triggered because I simply TRIED to use what you all are putting out there... seems like for the first time anyone actually tried to use these templates. |
@kianenigma @rzadp this is actually not good enough. There needs to be at least 2 validators on Polkadot for us to prorgress with parachain blocks. So the final template needs to look like something like: [relaychain]
default_command = "polkadot"
chain = "rococo-local"
[[relaychain.nodes]]
name = "alice"
validator = true
ws_port = 9944
[[relaychain.nodes]]
name = "bob"
validator = true
ws_port = 9955
[[parachains]]
id = 1000
[parachains.collator]
name = "charlie"
ws_port = 9988
command = "./target/release/parachain-template-node" BTW note here that i changed This seems to make much more sense to me than:
I really don't know why we would recommend to |
Here is the error with just 1 validator:
You can see it says |
Hi @kianenigma, did you want to run the test in our CI or the idea is to provide a nice example test to run by endusers? Since with the context we can work on diff options availables. Thx! |
I have updated the PR with this config with 2 validators. Also sent a quick fix to the target repo.
Just seemed consistent with the other binaries/commands. I have changed this (and the readme) to not export this one.
Evidently, I haven't done a good job making sure they are in a proper condition. Besides the immediate fixes, I'm adding a CI check to make sure relaychain blocks are produced, and we shall work on a check for parachain blocks. |
@@ -63,14 +63,9 @@ You still need a relaychain node - you can download the `polkadot` | |||
(and the accompanying `polkadot-prepare-worker` and `polkadot-execute-worker`) | |||
binaries from [Polkadot SDK releases](https://github.com/paritytech/polkadot-sdk/releases/latest). |
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.
Download binaries (of polkadot) only works on linux (x86_64), we should add a comment to compile those in macOS or arm64.
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.
I prefer we solve like this: #802
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.
Totally agree, we should push to include those as part of the release process to improve dx.
templates/parachain/README.md
Outdated
|
||
This way, we can conveniently use them in the following steps. | ||
Make sure to bring `zombienet` and the relaychain binaries | ||
(`polkadot`, `polkadot-prepare-worker`, `polkadot-execute-worker`) |
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.
(`polkadot`, `polkadot-prepare-worker`, `polkadot-execute-worker`) | |
(`polkadot`, `polkadot-prepare-worker`, `polkadot-execute-worker`, `parachain-template-node`) |
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.
Maybe keeping the export PATH
example is a good idea.
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.
really though? seems super inconvenient?
compared to just having the path in the zombienet config itself?
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 then imo to run cargo install
so that the binary is placed into the cargo bin
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.
There is one more argument supporting the PATH path.
The user could be trying out the readme, having downloaded the parachain-template-node
binary from a release instead of building it.
Then, the hardcoded path in zombienet wouldn't make sense, but instructions to have it in PATH would still hold.
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 then imo to run
cargo install
so that the binary is placed into thecargo bin
OK, I've added a step with cargo install
for the parachain binary.
And, I left an example with export PATH
for the other binaries.
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.
Should we merge this after #5342, which seems to be the testing needed to ensure these changes are correct?
Not sure about that, those tests don't ensure these changes are correct (raised some thoughts here). Specifically: This PR here is mostly about adding a second validator Bob, but the I've added CI with |
* master: (36 commits) Bump the ci_dependencies group across 1 directory with 2 updates (#5401) Remove deprecated calls in cumulus-parachain-system (#5439) Make the PR template a default for new PRs (#5462) Only log the propagating transactions when they are not empty (#5424) [CI] Fix SemVer check base commit (#5361) Sync status refactoring (#5450) Add build options to the srtool build step (#4956) `MaybeConsideration` extension trait for `Consideration` (#5384) Skip slot before creating inherent data providers during major sync (#5344) Add symlinks for code of conduct and contribution guidelines (#5447) pallet-collator-selection: correctly register weight in `new_session` (#5430) Derive `Clone` on `EncodableOpaqueLeaf` (#5442) Moving `Find FAIL-CI` check to GHA (#5377) Remove panic, as proof is invalid. (#5427) Reactive syncing metrics (#5410) [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006) Change the chain to Rococo in the parachain template Zombienet config (#5279) Improve the appearance of crates on `crates.io` (#5243) Add initial version of `pallet_revive` (#5293) Update OpenZeppelin template documentation (#5398) ...
Following this: paritytech/polkadot-sdk-parachain-template#11