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

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

Closed
bkontur opened this issue Apr 2, 2024 · 23 comments
Closed

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

bkontur opened this issue Apr 2, 2024 · 23 comments

Comments

@bkontur
Copy link
Contributor

bkontur commented Apr 2, 2024

TBD

Relates to: #1984
Relates to: #2714

@bkontur
Copy link
Contributor Author

bkontur commented Apr 3, 2024

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
Copy link
Contributor Author

bkontur commented Apr 4, 2024

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
Copy link
Contributor

seadanda commented Apr 4, 2024

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
Copy link
Contributor Author

bkontur commented Apr 4, 2024

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

I think benchmarking can go away with omni-bencher

@kianenigma
Copy link
Contributor

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
Copy link
Contributor

#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 GenesisConfig presets for runtime #2714 (comment) for some proposal.

@bkontur
Copy link
Contributor Author

bkontur commented Apr 5, 2024

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
Copy link
Contributor

michalkucharczyk commented Apr 5, 2024

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
Copy link
Contributor

seadanda commented Apr 5, 2024

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
Copy link
Contributor

michalkucharczyk commented Apr 5, 2024

Some draft idea on how to move genesis config presets into the runtime: #3996

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

@kianenigma
Copy link
Contributor

kianenigma commented Apr 5, 2024

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 #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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

eskimor commented Apr 10, 2024

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
  • 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
Copy link
Member

bkchr commented Apr 11, 2024

  • 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
Copy link
Member

eskimor commented Apr 17, 2024

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:

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

@bkchr
Copy link
Member

bkchr commented Apr 18, 2024

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
Copy link
Contributor

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
Copy link
Member

bkchr commented May 9, 2024

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
Copy link
Member

eskimor commented May 10, 2024

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 🥳 So the only actual missing ingredient for doing omninode --paraid paraid is actually having bootnodes on the DHT? Nice!

@Polkadot-Forum
Copy link

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
Copy link

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

@kianenigma
Copy link
Contributor

@bkontur this can be closed now?

github-merge-queue bot pushed a commit that referenced this issue Aug 30, 2024
Gensis config presets moved from `polkadot-parachain` binary into
`asset-hub-rococo` runtime.

relates to: #3944

---------

Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants