bkontur commented 3 months ago

TBD

handle unknown id as Runtime::default() https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/polkadot-parachain/src/command.rs#L90-L133, e.g. coretime-kusama causes problem...
maybe remove enum Runtime
remove chain specific stuff from polkadot-parachain-bin and create chain-spec-generator for testnets?

Relates to: https://github.com/paritytech/polkadot-sdk/issues/1984 Relates to: https://github.com/paritytech/polkadot-sdk/pull/2714

bkontur commented 3 months ago

Some hints from: https://github.com/paritytech/polkadot-sdk/pull/3961/files#r1549575119

I think, polkadot-parachain-bin should be maybe chain-agnostic and we should provide just chain-spec json file. So we should remove all these chain-specific stuff from polkadot-parachain and extract that to new crate chain-spec-generator for testnets as we have for fellows. And also, for controlling what setup to run start_generic_aura_node vs start_generic_aura_lookahead_node vs start_asset_hub_node vs start_asset_hub_lookahead_node, we could add new command parameter (or maybe attribute to chain-spec?).

bkontur commented 3 months ago

also we should remove testnet runtimes from dependencies, the more testnet runtimes we add to polkadot-sdk, the bigger the polkadot-parachain binary will be.

Maybe very easy fix would be to add testnet runtimes behind features.

seadanda commented 3 months ago

I agree totally with this.

I think that ideally we would keep polkadot-parachain-bin very light, with no chainspec bytes or runtime dependencies (by removing the local configs completely). This would mean that chain specs are passed as arguments and any other information that is needed for a specific runtime to run is passed as an argument (e.g. key type = ed25519 for asset hub polkadot).

I also agree that the testnets being feature gated would be great, and separated into a testnet-chain-spec-generator for the local chainspecs, and pass the runtime wasm as an argument (or just pass a chainspec which includes the WASM).

I'd love to remove the RuntimeApi dependencies, and actually wonder whether we need the benchmarking in this tool as well.

"Do one thing and do it well"

bkontur commented 3 months ago

and actually wonder whether we need the benchmarking in this tool as well.

I think benchmarking can go away with omni-bencher

kianenigma commented 3 months ago

Aligned with #3597 as well. This crate can end up being a parachain-omni-node that can collate for all parachains so long as they don't have a lot of custom RPCs and such.

Once the revamp of chain-spec by @michalkucharczyk is done, we are one step closer to being able to remove all the runtime related things from this crate.

michalkucharczyk commented 3 months ago

2714 should definitely help with removing runtime deps. With work done there the entire initial genesis config can be queried from the runtime (for different flavors, e.g. -dev, -local), so we should be able to get rid of all runtime deps.

Theoretically it should be possible to run the node with just runtime blob and preset name.

However there are still two open questions:

where the other pre-defined chain-spec properties shall be placed (e.g. chain-name, chain-type, tokenName, tokenDecimals, para_id, relay_chain, etc...). They are currently hard-coded in binary-builtin-files json files for production or into the code for dev/local specs (e.g. here).
where production raw genesis config (intiial state) shall be placed? For now it is also hard-coded into binary. See this comment https://github.com/paritytech/polkadot-sdk/pull/2714#issuecomment-2015125305 for some proposal.

bkontur commented 3 months ago

And there is also another question, polkadot-parachain can start different "aura node setups": start_generic_aura_node start_generic_aura_lookahead_node // <- for async backing start_asset_hub_node start_asset_hub_lookahead_node start_basic_lookahead_node

__Note: I don't know what are exact differences, but start_generic_aura_lookahead_node and start_basic_lookahead_node maybe could be the same one?__

Now this is switched/decided by Runtime enum which is parsed from chain_spec.id, e.g.:

        chain_spec::bridge_hubs::BridgeHubRuntimeType::KusamaLocal =>
                crate::service::start_generic_aura_node(config, polkadot_config, collator_options, id, hwbench)
...
        chain_spec::bridge_hubs::BridgeHubRuntimeType::Westend =>
                crate::service::start_generic_aura_lookahead_node(config,

If we want to polkadot-parachain be really generic/omni, we should change that and introduce e.g. new parameter --node-type to the RunCmd or to the Cli backed by enum, e.g.:

pub enum SupportedNodeType {
        #[default]
        GenericAura,                // -> start_generic_aura_node (as default)
        GenericLookaheadAura,       // -> start_generic_aura_lookahead_node
        AssetHubAura,               // -> start_asset_hub_node
        AssetHubLookaheadAura       // -> start_asset_hub_lookahead_node,
        BasicLookaheadAura          // -> start_basic_lookahead_node
}

So you would be able to run whatever chain-spec/runtime with whatever aura setup, e.g.:

# without async backing
./polkadot-parachain --chain-spec whatever-chain.json
# with async backing
./polkadot-parachain --chain-spec whatever-chain.json --node-type generic-lookahead-aura

or maybe easily just with runtime wasm:

# without async backing
./polkadot-parachain --runtime whatever-chain.wasm
# with async backing
./polkadot-parachain --runtime whatever-chain.wasm --node-type generic-lookahead-aura

I think this is pretty easy and independent change to polkadot-parachain

michalkucharczyk commented 3 months ago

pub enum SupportedNodeType

This could be provided by runtime-side. We could provide new runtime API to provide some node-side config, or parse runtime metadata, or maybe there is another approach. It was already briefly discussed here and there...

The goal would be to minimize the knowledge required to run the node. Ideally just provide the runtime blob, and that's all.

seadanda commented 3 months ago

start_generic_aura_node
start_generic_aura_lookahead_node // <- for async backing
start_asset_hub_node
start_asset_hub_lookahead_node
start_basic_lookahead_node
Note: I don't know what are exact differences, but start_generic_aura_lookahead_node and start_basic_lookahead_node maybe could be the same one?

start_basic_lookahead_node is to support glutton which has no transaction payment api among other things, naively I'd expect this to be able to be handled by making the generic node more general. It might even work already, but glutton's chain spec seems to be missing from the repo.

start_generic_aura_node and start_asset_hub_node are no longer needed for system parachains when they all shift to async backing in the next release, but if we're going for an omni-node approach then we'd still need start_generic_aura_node and a cli flag to enable async backing. I agree that this is a sensible default.

IIRC The asset_hub flavours exist because they started just using relay chain consensus and transitioned to aura runtimes at some point, so the logic is in there for that transition as they didn't have the AuraApi initially. Sounds like @michalkucharczyk's suggestion could allow that to be combined into one function too.

michalkucharczyk commented 3 months ago

Some draft idea on how to move genesis config presets into the runtime: https://github.com/paritytech/polkadot-sdk/pull/3996

(Hopefully the last thing that keeps dep to asset_hub_rococo_runtime is asset_hub_rococo_runtime::WASM_BINARY.)

kianenigma commented 3 months ago

I think something like the type of the aura/consensus, among other parameters, should be exposed as configurations to a builder pattern crate that lets you easily build a node software with fewer lines of code, as per https://github.com/paritytech/polkadot-sdk/issues/5

CLI flag would also do, but we already have a million of them. Moreover, let's not forget that part of the goal here is to also reduce the footprint of the code in ./node for all teams, including ourselves. A builder pattern will help with that, a CLI flag while keeping all the code in service.rs intact not so much.

michalkucharczyk commented 3 months ago

afaiu the builder pattern is orthogonal to what we do with omni-node, right? What I mean is that it allows to reduce the LOC in omni-node, but will not solve a problem how to handle different consensus (or other chain-specific configuration options). We would still need to provide some means of execution time customization.

kianenigma commented 3 months ago

afaiu the builder pattern is orthogonal to what we do with omni-node, right? What I mean is that it allows to reduce the LOC in omni-node, but will not solve a problem how to handle different consensus (or other chain-specific configuration options). We would still need to provide some means of execution time customization.

Your comment is generally right, but through prototyping, I have come to the conclusion that building a node-builder could be a very good exercise to build the omni-node with.

The builder pattern would force us to think in a structured way about all thing that need to be parameterized in the node side. These parameters could be exposed as a combination of either builder functions, or CLI arguments. The final omni-node would then merely be a pre-configured and pre-compiled version of the node-builder.

I'd say that the ideal course of action is to build the node-builder first, and then omni-node based on that. But it is also reasonable to fast-track directly to the omni-node in favor of time. I don't have a strong opinion on this yet, but further prototyping might help.

eskimor commented 2 months ago

As a potential followup. The ideal user story is:

I want to spawn a full node for any Polkadot parachain by simply running

omninode --paraid PARAID

This could be achieved by:

[ ] parachain boot nodes on DHT
[x] Having the genesis hash on relaychain (already) - to be able to communicate with boot nodes.
[ ] Fetch chain specs from collators: Ok downside, for collators nodes at least one would need to have gotten the chain spec from somewhere else. That's a fair bootstrapping requirement though.
[ ] Fetch the para runtime from relay chain state
[ ] Profit: Parachain full node up and running

As a side it has been raised, that it would be nice if omninode were suitable for benchmarks as well. Having a separate binary as discussed above is likely a descent solution too though.

TL;DR: Ideally the relay chain would be self sufficient to launch nodes for any parachain

bkchr commented 2 months ago

parachain boot nodes on DHT

This is the only suitable solution for this and nothing else of what you listed there. With this information the node can warp sync.

eskimor commented 2 months ago

This is the only suitable solution for this and nothing else of what you listed there. With this information the node can warp sync.

The listed bullet points are not options, but overall requirements needed for this feature (from my understanding).

The only two that are actually missing to my knowledge:

Bootnodes on DHT.
Somewhere to fetch the chain spec from.

bkchr commented 2 months ago

2. Somewhere to fetch the chain spec from.

You don't need to fetch the chain spec, if you have the bootnodes on chain. Or what do you mean by this?

kianenigma commented 2 months ago

I wonder if this patter of using relay -> aura parachain consensus is something that the ecosystem also uses, or is it just us? also, is it worth the effort? Why don't we launch parachains from genesis with aura? is there no way to do this?

it seems like quite an inefficiency to indefinitely maintain this duality of "aura or relay consensus" just for the sake of the initial blocks of a parachain's lifecycle.

bkchr commented 1 month ago

I wonder if this patter of using relay -> aura parachain consensus is something that the ecosystem also uses, or is it just us?

Also others have done this. However, we also don't do this anymore.

it seems like quite an inefficiency to indefinitely maintain this duality of "aura or relay consensus" just for the sake of the initial blocks of a parachain's lifecycle.

What is inefficient? You also still need this to be able to sync.

eskimor commented 1 month ago

You don't need to fetch the chain spec, if you have the bootnodes on chain. Or what do you mean by this?

I was told we need it, I have not yet checked myself. If just boot nodes suffice - even better :partying_face: So the only actual missing ingredient for doing omninode --paraid paraid is actually having bootnodes on the DHT? Nice!

Polkadot-Forum commented 1 month ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback/7823/1

Polkadot-Forum commented 3 weeks ago

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-parachain-omni-node-gathering-ideas-and-feedback/7823/15

paritytech / polkadot-sdk

Fix `polkadot-parachain` to work just with chain-spec #3944

2714 should definitely help with removing runtime deps. With work done there the entire initial genesis config can be queried from the runtime (for different flavors, e.g. -dev, -local), so we should be able to get rid of all runtime deps.