Add Runtime::OmniNode variant to polkadot-parachain
#4805
Conversation
serban300
force-pushed
the
polkadot-parachain-omni-node
branch
3 times, most recently
from
b06a92c to
0c13a9a
Compare
|
Why are you adding all these chain specs here? |
serban300
force-pushed
the
polkadot-parachain-omni-node
branch
2 times, most recently
from
6206313 to
c8b1be8
Compare
Also we can't have the banner printed all the time, because it leads to printing malformed chain specs.
serban300
force-pushed
the
polkadot-parachain-omni-node
branch
from
c8b1be8 to
16b759d
Compare
serban300
force-pushed
the
polkadot-parachain-omni-node
branch
from
16b759d to
84b1d32
Compare
the chain specs are only for manual testing, we should remove them 🙈 unless if they are used in CI tests. |
+ small fixes
Done |
| @@ -47,7 +47,7 @@ short-benchmark-westend: &short-bench | |||
| tags: | |||
| - benchmark | |||
| script: | |||
| - ./artifacts/polkadot-parachain benchmark pallet --chain $RUNTIME_CHAIN --pallet "*" --extrinsic "*" --steps 2 --repeat 1 | |||
| - ./artifacts/polkadot-parachain-omni-node benchmark pallet --chain $RUNTIME_CHAIN --pallet "*" --extrinsic "*" --steps 2 --repeat 1 | |||
There was a problem hiding this comment.
@ggwpez the days for this command is also numbered, is there a ticket to move this to the omni-bencher?
Side question: can the omni-node do benchmarking as well? what are the runtime-dependent parts of the benchmarking code that live in the node side? I suppose there are dependencies that led to the omni-bencher being created, but idk what they are.
| ``` | ||
| cargo run --release --bin polkadot-parachain --features runtime-benchmarks -- benchmark machine --base-path /mnt/scratch/benchmark | ||
| cargo run --release --bin polkadot-parachain-omni-node --features runtime-benchmarks -- benchmark machine --base-path /mnt/scratch/benchmark |
There was a problem hiding this comment.
From my understanding, all node benchmark commands should eventually be moved to omni-bencher CLI. I am not sure if the tool is ready yet, but good to know what is the end goal.
|
|
||
| https://paritytech.github.io/chainspecs | ||
|
|
||
| <bold>polkadot-parachain-omni-node --chain asset-hub-polkadot --sync warp -- --chain polkadot --sync warp</> |
There was a problem hiding this comment.
The meat of the omni node is knowing how to use the --chain. But this flag is coming from the depth of the substrate's CLI types. I wish there was a way to customize the doc you get from --chain to, for example, demonstrate the list of options for you.
There was a problem hiding this comment.
I'm not sure if this is possible. I will check.
There was a problem hiding this comment.
I couldn't find a way to do this so far
| Please refer to the following resources to learn more about the omni-node: | ||
| - https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback/7823/4 | ||
| - https://github.com/paritytech/polkadot-sdk/issues/5 | ||
| - sdk-docs |
There was a problem hiding this comment.
I would like to work on a small set of ref-docs and push them to this PR about how to use the omni-node.
There was a problem hiding this comment.
IMO this entire text is spam. What is the need to spam every node launch with this stuff? You don't need to mention the issues etc...
Also --help is like available on every CLI program on this planet.
Formerly know as polkadot-parachain, now called polkadot-parachain-omni-node, equipped with running more parachains in the Polkadot networks.
If you run the binary, which was renamed, you already know that it was renamed.
There was a problem hiding this comment.
I am trying to gain more attention to this. How about we let it spam all CLIs for a few versions, then move it to --help?
I generally agree it is spammy. We can also move it to --help as of now.
There was a problem hiding this comment.
I removed it for the moment since it doesn't fit if we keep the old name. I can add it back if you decide that it's needed.
How about we let it spam all CLIs for a few versions, then move it to --help ?
If we show it, we need to show it only just with --help. Otherwise for example the chain-spec generation commands will show it, resulting in malformed chain specs.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
serban300
changed the title
Runtime::OmniNode variant to polkadot-parachain
|
The CI is passing and I reverted the renaming. The PR only adds the |
| info!("Parachain Account: {}", parachain_account); | ||
| info!("Is collating: {}", if config.role.is_authority() { "yes" } else { "no" }); | ||
| info!("🪪 Parachain id: {:?}", id); | ||
| info!("🧾 Parachain Account: {}", parachain_account); |
There was a problem hiding this comment.
| info!("🧾 Parachain Account: {}", parachain_account); |
Not sure we need this anymore.
There was a problem hiding this comment.
Why not though? it seems like a nice user friendly addition to me. As a noob, it would be nice to see it.
There was a problem hiding this comment.
I also think it's nice to have it.
| // rococo actually uses aura import and consensus, unlike most system chains that use | ||
| // relay to aura. |
There was a problem hiding this comment.
"relay to aura" is generic that it also works if you just have an aura chain from gensis.
There was a problem hiding this comment.
yeah, just that it is needlessly checking some if statement, so it is a miniscule slower, but it indeed works.
There was a problem hiding this comment.
Not sure if I understood correctly. Are you suggesting to use start_generic_aura_lookahead_node here ?
|
|
||
| (norm_id, orig_id, para.map(Into::into)) | ||
| (id, id, None) |
There was a problem hiding this comment.
Hmm. Is this also a breaking change?
There was a problem hiding this comment.
It shouldn't be. This is just cosmetics. The logic should be the same as before.
What? What user interface are you talking about here? What are the changes to this user interface you are talking about here? I don't see them. Please show them to me. |
For example, we agreed that as the next step, we want to add the ability to dummy-run a runtime without consensus ( tbh I originally agreed more with Rob (+this would also allow us to rename things as we wished, and actually call the the omno node binary |
For sure this will require a new CLI flag. However, you could have a bug everywhere.
TBH, I know how all this code works. I don't get why you are afraid or whatever. |
18a6a56
) Adding `Runtime::OmniNode` variant + small changes --------- Co-authored-by: kianenigma <[email protected]>
) Adding `Runtime::OmniNode` variant + small changes --------- Co-authored-by: kianenigma <[email protected]>
) Adding `Runtime::OmniNode` variant + small changes --------- Co-authored-by: kianenigma <[email protected]>
Adding
Runtime::OmniNodevariant + small changes