Omni-node: add solochain with manual consensus logic #5027
Conversation
Also moved some more primitives to common
serban300
added
R0-silent
| #[cfg(not(feature = "runtime-benchmarks"))] | ||
| pub type HostFunctions = cumulus_client_service::ParachainHostFunctions; | ||
|
|
||
| #[cfg(feature = "runtime-benchmarks")] |
There was a problem hiding this comment.
Please keep an eye on the end goal that everything to do with benchmark should ideally be removed from this and all other nodes someday.
I think I've asked this in another PR, but have likely missed the answer 🙈: @ggwpez can you chime in, if there are any functional features that we now miss in the omni-bencher, before we can kiss and say goodbye to the benchmark subcommand here?
There was a problem hiding this comment.
Yea we should delete the benchmarking sub-commands here. Omni bencher will be extended for all of them.
| use substrate_frame_rpc_system::{System, SystemApiServer}; | ||
|
|
||
| type BlockNumber = u32; | ||
| type AccountId = AccountId32; |
There was a problem hiding this comment.
So these are hardcoded assumptions? I am actually not sure. This will probably work with a runtime that also uses Multi* account id types.
In any case, Let's move all of the types that used to come from the hardcoded runtime, and no longer are, to a assumptions.rs (or similar name) and document exactly what assumptions exist, and why.
| @@ -0,0 +1,263 @@ | |||
| // Copyright (C) Parity Technologies (UK) Ltd. | |||
There was a problem hiding this comment.
I would call this TestNode or DevNode or something like that, as a solo chain is usually associated with a full-fledged one with babe and gradpa and all that stuff.
| select_chain, | ||
| commands_stream: Box::pin(commands_stream), | ||
| consensus_data_provider: None, | ||
| create_inherent_data_providers: move |_, ()| async move { |
There was a problem hiding this comment.
This is tied to the runtime, as in it is also part of the assumptions and need to ideally be tested for. For example, will this work with a runtime that doesn't have pallet-timestamp?
@gupnik when I talk about "opinionated frame system super pallet", this is the type of things that I mean. All normal FRAME-based runtimes must have the time timestamp pallet. Why should this not already be part of frame_system, or an opinionated flavor of it?
A project that you can generally start to complement the omni-node is an frame-omni-node-system, a variant of frame_system and defaults that is meant to work with omni-node.
There was a problem hiding this comment.
The code here looks great, I really like the NodeSpec direction to separate the node configs of parachains and solochains etc.
First, I don't (yet) think we need to add anything that we used to call solochain in here. So I suggest renaming some of your code to avoid confusion. The use cases we want is:
- A node that runs the default parachain consensus. To connect the parachain to Paseo, and RCs.
- A node that runs no consensus, only for testing your runtime locally, with PJS and such.
What we seem to have here is no CLI integration, and unclear what should happen. I have some opinions here, they are a bit disruptive, and I invite others to also share:
-
First, I would remove
--chainand rename it to--chain-specto be more clear that this flag only accept s chain-spec file. Indeed, this means to no longer--chain <magic-system-para-string>, and then we can finally remove all the embedded runtimes from here as well. This can be done in a backwards compat way to give everyone heads up about it. This is not an anymore, and must be provided -
Crucially, this also removed
--chain devas well.--devcan still remain, but it should not influence--chainat all. It should only influence--tmp --aliceand other things.
Then, the issue we face is broadly described as: Omni node can be configured to do different things, among few axes. One that we see now, is consensus. Possibly, in the future, there could be more of these axes that can be configured, for example, we have had the request to make it be EVM-compatible. This is another axis where at some place, we have to boot the node with a declaration of "be EVM compatible or not".
The challenge is, how do we describe these new parameters. I broadly see two ways:
- We extend chain-spec, through extensions, with fields that parameterize things.
- For each, there is a default, and a CLI (+ env variable) option that can override it.
I don't fully understand chain-spec, what should and shoult not be in it, and therefore find it hard to make a decision about it. If we want to take the second approach here, we would:
- add
--consensus. Default ispara-aura, so everyone already running this will not feel any difference. You can set this CLI flag tomanual-sealto enable to code path of this PR.
|
Closing this PR as discussed offline since we decided to keep a separate binary for the manual seal |
Related to #5026
service.rsfile partially