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

Allow registering parathreads in pallet_registrar genesis #632

Merged
merged 11 commits into from
Jul 31, 2024

Conversation

tmpolaczyk
Copy link
Contributor

To actually add them we will need to modify the chain spec manually, probably using a javascript script

@tmpolaczyk tmpolaczyk marked this pull request as draft July 19, 2024 09:54
Copy link
Contributor

github-actions bot commented Jul 22, 2024

Coverage Report

(master)

@@                           Coverage Diff                            @@
##           master   tomasz-genesis-registrar-parathreads      +/-   ##
========================================================================
- Coverage   66.71%                                 66.63%   -0.08%     
+ Files         260                                    261       +1     
- Lines       44818                                  44717     -101     
========================================================================
- Hits        29897                                  29794     -103     
+ Misses      14921                                  14923       +2     
Files Changed Coverage
/pallets/invulnerables/src/lib.rs 85.59% (-0.90%) 🔽
/pallets/registrar/src/lib.rs 88.73% (-0.09%) 🔽

Coverage generated Wed Jul 31 09:11:44 UTC 2024

@tmpolaczyk tmpolaczyk marked this pull request as ready for review July 22, 2024 13:52
@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes labels Jul 22, 2024
@@ -217,7 +217,7 @@ fn testnet_genesis(

let para_ids: Vec<_> = para_ids
.into_iter()
.map(|(para_id, genesis_data, _boot_nodes)| (para_id, genesis_data))
.map(|(para_id, genesis_data, _boot_nodes)| (para_id, genesis_data, None))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should find a way to pass them as parathreads. Maybe the cleanest is with a new command line argument dont you think? Or alternatively, something in the spec that indicates whether it is a parathread..

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

I think the PR is good but I would love to see a way for us injecting parathreads without going through manually changing the spec. However the only way of doing is I think is by:

  • A chain-spec container file from which we can read the parathread-params from some attribute
  • A different command that asks for two arguments, the json file and the parathread params for parathreads.

I am not sure how feasible is the latter (or even, how user-friendly)

@tmpolaczyk
Copy link
Contributor Author

I feel that parity wants to move all the genesis stuff out of the node binary and use the chain_spec_builder binary instead. I don't know how that binary works yet so not sure if that's a good idea.

But with that in mind, I was thinking of deprecating the --add-container-chain flag, and instead using a javascript script. Then we could have that script accept a genesis json patch file as input, and optionally the container chain paths like we do now. Because having to add a new flag to the node binary every time we add a new field in a genesis config feels wrong. We still will have to add some flags, but for most of them they will be defined in the patch json file instead. So in the parathread example, the parathread params should be part of the json patch and not the cli.

And also we could merge all the build-spec-*.sh scripts into a single one with subcommands or args to toggle dancebox/flashbox and num collators.

But I don't want to do any of this in this PR, as it would be a big change and probably break our zombienet tests.

@girazoki
Copy link
Collaborator

sounds fair enough yes. I think what you want to do is something similar to what zombienet does right? In that case I am fine with it yes. Take into account that #629 adds the command line flag for starlight too, so please wait until it is merged to apply changes over it

@tmpolaczyk tmpolaczyk merged commit 7d2caa7 into master Jul 31, 2024
37 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-genesis-registrar-parathreads branch July 31, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants