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

parachain-omni-node binary #3597

Closed
wants to merge 19 commits into from

Conversation

kianenigma
Copy link
Contributor

Related to #62, #5, #1984 and possibly more.

This node now should be able to take any runtime that has no consensus related expectations (ie. data being in header digests etc), and run it with its own manual consensus. It should be ideal for a scenario like a light tutorial.

But moreover, if the approach here is good, it can already be used to build solo-chain-omni-node and parachain-omni-node etc.

@kianenigma kianenigma added the T0-node This PR/Issue is related to the topic “node”. label Mar 6, 2024
properties.insert("tokenDecimals".to_string(), 0.into());
properties.insert("tokenSymbol".to_string(), "MINI".into());

// `.with_genesis_config(Default::default)` won't work, but should.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michalkucharczyk this was my main chain-spec related issue so far, I would have expected it to work without this. Or perhaps the Default::default should do this for me.

Copy link
Contributor

@michalkucharczyk michalkucharczyk Mar 6, 2024

Choose a reason for hiding this comment

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

This API is under some development. The idea is here: #1984 and some PoC: #2714.

This would help here (fetching default is trivial to add).

What I propose is to move predefined genesis configs into runtime (and call them presets), and extend GenesisBuilder runtime API to fetch them.
This would allow for functionality mentioned in #2714 (comment).

The reasoning for this is that hand-crafting chain-spec files maybe difficult as the serialization of types can be tricky to guess. Also default RuntimeGenesisConfig is typically useless (e.g. does not contains keys).

Copy link
Contributor

@michalkucharczyk michalkucharczyk Mar 6, 2024

Choose a reason for hiding this comment

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

Actually, .with_genesis_config_patch(json!({})) should do the job. (which means empty patch for default config).

pub struct FakeRuntime;
use crate::types::{AccountId, Header, Nonce, OpaqueBlock as Block};

sp_api::impl_runtime_apis! {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also manually implement this stuff without this macro, but this macro already does the job easily.

@kianenigma kianenigma requested a review from a team as a code owner March 22, 2024 15:09
@bkchr
Copy link
Member

bkchr commented Mar 24, 2024

parachain-omni-node

polkadot-parachains should be this binary. No need to have another special binary for this IMO.

@kianenigma
Copy link
Contributor Author

parachain-omni-node

polkadot-parachains should be this binary. No need to have another special binary for this IMO.

Agreed, I didn't know it is already a node capable of this.

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/parity-tech-update-for-march/7226/1

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/rfc-should-we-launch-a-thousand-cores-program/7604/1

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/paritytech-update-for-april/7646/1

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/stabilizing-polkadot/7175/20

@xlc
Copy link
Contributor

xlc commented May 8, 2024

What is blocking this (assuming it is blocked)?

Upgrade the client to the new polkadot-sdk is always a big overhead and error-prune process and for no real benefits. Omni node is the solution to allow parachain teams only focus on the runtime. This is a super critical work and I would like to see it released ASAP.

// same anyways. Possibly requires a big refactor to move Block to an associated type instead of
// generic in a few places.

pub type ParachainClient<Block, RuntimeApi, HostFns> =
Copy link
Contributor Author

@kianenigma kianenigma May 8, 2024

Choose a reason for hiding this comment

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

What I am trying with this crate is as follows:

this is the "cumulus service builder" helpers. There should be one single source of truth for how to create eg. an aura with async backing import queue. At the moment we copy paste this in multiple places even in our repo.

Almost everything in this crate is generic over 3 things:

  1. Block
  2. RuntimeApi
  3. HostFunctions

So a user of this crate should provide these 3 things, and then they can use the helper functions for

  1. building the import queue
  2. building consensus

For each of:

  1. relay consensus
  2. aura consensus
  3. async backing
  4. relay -> aura (we should get rid of this one :/)

If we learn we need customization for database (for EVM we 99% need it), we add it here in a similar fashion.

The first milestone I see is as follows:

Finish this crate, and make sure the same underlying code is being used in polkadot-parachian and /template/parachain. As noted, for example, one function to build aura consensus with async backing, not two instances copy-pasted.

Then we see where go from there :)

}
use whatever::*;

sp_api::impl_runtime_apis! {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should ideally move to cumulus_service now. I think one set of fake runtime impls should be enough.

.with_name("Development")
.with_id("dev")
.with_chain_type(ChainType::Development)
.with_genesis_config_patch(testnet_genesis(
Copy link
Contributor

@michalkucharczyk michalkucharczyk May 8, 2024

Choose a reason for hiding this comment

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

Don't know if this is final or temporary proposal. I'd suggest fetching "local_testnet"/"development" genesis from runtime here as omni-node shall make no assumptions about runtime's internals.

If omni-node is to provide two hardcoded chainspecs ("development", "local_testnet") then we could require runtime's author to provided corresponding presets. If runtime does not provide those presets we could fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, it is not actually used...

let extensions = Extensions { relay_chain: "rococo-local".into(), para_id: 2000 };

let tmp = sc_chain_spec::GenesisConfigBuilderRuntimeCaller::<'_, ()>::new(&code);
let genesis = tmp.get_default_config()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: default runtime config may not be runnable - some crucial components may not be properly initialized in default runtime genesis.

@kianenigma
Copy link
Contributor Author

kianenigma commented May 9, 2024

What is blocking this (assuming it is blocked)?

Upgrade the client to the new polkadot-sdk is always a big overhead and error-prune process and for no real benefits. Omni node is the solution to allow parachain teams only focus on the runtime. This is a super critical work and I would like to see it released ASAP.

There should be no blocker for syncing normal parachains as far as I can say. I have synced hydra, interlay and frequency at 40fbf47 and it all works. Although, as additional tests, we should also test:

  1. If the DApp of the corresponding parachain can work with the omni-node (missing RPCs)
  2. If the node can collate blocks (important, else this is not useful)

What seems like a blocker to me is syncing system chains that do relay -> aura. Tbh I wish to not need to keep the specialized code for this use case around. Maybe it is a reasonable compromise to drop support for this type of consensus in the omni-node. If you want to sync the old versions of eg. AH, you should use a non-omni-legacy-node or something.

The code in this commit already provides some soft degree of configurability through a builder pattern as well. This can either remain a builder or become CLI flags.

fn main() -> sc_cli::Result<()> {
let node = Builder::default()
// .parachain_consensus(builder::ParachainConsensus::Relay(12))
.parachain_consensus(builder::ParachainConsensus::Aura(12))
.build()?;
node.run()
}

@kianenigma kianenigma changed the title WIP/DNM: Turn minimal-template-node into omni-node parachain-omni-node binary May 9, 2024
@xlc
Copy link
Contributor

xlc commented May 9, 2024

let’s get a simple version out that only supports the most common scenario and then we can iteratively make it compatible with more chains

@Polkadot-Forum
Copy link

This pull request 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

@kianenigma
Copy link
Contributor Author

#4369 is a pretty similar PR to what is being done here, and can be reviewed and merged as a first step.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6265821

@kianenigma
Copy link
Contributor Author

This branch will likely be closed in favor of s slimmer version brought to you by @serban300. I will leave open for visibility for now, but feel free to close when a replacemebt comes by.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants