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

omni node without embedded chainspecs #5210

Closed
xlc opened this issue Aug 1, 2024 · 15 comments
Closed

omni node without embedded chainspecs #5210

xlc opened this issue Aug 1, 2024 · 15 comments
Assignees

Comments

@xlc
Copy link
Contributor

xlc commented Aug 1, 2024

I would like to delete all the node related code from Acala repo and start using omni node but got some blockers:

  • I need to ensure the CLI compatibility. i.e. --chain acala should still work. So I need a way to build a binary with minimal change to embed the Acala and Karura chainspec to the binary.
  • I want to keep the binary size in control. i.e. I don't want to embed other chain spec, which are large.

Suggestion: make a new crate, move all the logic in polkadot-parachain-bin to there, except all the chainspec for various networks. Update polkadot-parachain-bin to use this new crate. And then I will be able to do similar thing at Acala.

@kianenigma
Copy link
Contributor

  • I would expect the chain-specs to be removed soon in any case.
  • Don't you have any custom EVM related code on the node side? will everything else work?

@xlc
Copy link
Contributor Author

xlc commented Aug 2, 2024

I would expect the chain-specs to be removed soon in any case.

Good. Will it happen within 4 weeks? If yes I can wait, otherwise I will prefer to do it myself if that can speed it up.

Don't you have any custom EVM related code on the node side? will everything else work?

We don't have any custom EVM related code in our node. Our EVM solution works with Chopsticks which only implements standard RPC.

@kianenigma
Copy link
Contributor

Removing the chain-specs from our side is a few days worth of work at most, but it might also break a lot of things in our CI etc, because things like --chain sys-chain will also not work now 🙈

Let me loop in @bkchr and @PierreBesson to see what they think. I kinda regret not forking polkadot-parachain in the past and making a new binary that we can freely break, instead of building things into polkadot-parachain. Still not too late to do that.

@bkchr
Copy link
Member

bkchr commented Aug 2, 2024

Not sure why it should take multiple days of work. I never advocated to keep them part of the binary. Parity CI or any other random parity internals should never be a blocker for these kind of changes.

However I can see the argument for externals. As we have now "stable" releases. We could remove the code on master and add a pr to stable that prints warnings that people should stop using these and switch over to providing the chain specs. Then in 3 months we can remove them entirely in the stable release.

@bkchr
Copy link
Member

bkchr commented Aug 2, 2024

I need to ensure the CLI compatibility. i.e. --chain acala should still work. So I need a way to build a binary with minimal change to embed the Acala and Karura chainspec to the binary.

@kianenigma did you actually read this? Because that was not really planned? Maybe what we could do until we have bootnodes on the relay chain dht, there exists some central registry to resolve these names to an actual chain spec. So, we would download this file and then final chain spec on the fly.

@PierreBesson
Copy link

@kianenigma In any case removing the embedded chainspecs is a big breaking change and should be clearly communicated to users with release notes/print warnings etc. Although users can always adjust their configurations, we don't want to break our promise of stability so we must minimize disruptions to existing flows including binary upgrades.

In terms of backward compatibility, I like the approach that @xlc wants to take with Acala which is to have one binary with embedded chainspec and another without. This allows to keep 100% backward compatibility and provide a migration path for parachains who are already distributing their node binaries and want to switch to the omni-node. If we were to take this approach for system parachains, we could have eg. polkadot-parachain for the system parachain binary and polkadot-omni for the omni-node.

Could we facilitate that process somehow by creating an omni-node "extension template" where you simply have to drop your chainspecs in a folder ? I think this could go a long way to foster adoption of the omni-node. We would of course use this ourselves for our system parachains binary.

As a caveat, it could also be argued that doing it this way kind of defeat the philosophy of the omni-node as it will multiply the number of node binaries in use in the wild.

@xlc
Copy link
Contributor Author

xlc commented Aug 2, 2024

Can we do it step by step? I only need step 1 to unblock me.

  1. Make a new create to contain most of the logic of the omni-node
  2. Use this create to reimplement polkadot-parachain binary
  3. Do whatever needed like making a breaking change or add a new binary or make deprecate warnings

I think we need step 1 done regardless the whole plan so let's do it?

@serban300
Copy link
Contributor

serban300 commented Aug 5, 2024

The idea of separating the common logic sounds good to me. And we can definitely do it within 4 weeks if there are no concerns about it. I can prioritize this. I just want to try a bit to see if we can do it without a separate crate as a first step. For example just to make polkadot-parachain compilable both as a bin and as a library and maybe also add a feature flag.

LE: But, just a heads-up. This common logic is still under development. We still plan to do some more refactoring and to make some things more generic, so the interfaces might change during the following months.

@xlc
Copy link
Contributor Author

xlc commented Aug 5, 2024

the public interface should be very simple: take a list of chain specs. I don’t think you will be able to break such simple interface with whatever refactoring that may happen.

and that’s the main point. you are free to change the node implementation and it won’t impact downstream users

@kianenigma
Copy link
Contributor

This public interface should be kept at the very very bare minimum, because the point of it is its stability. If we break this interface, the underlying goal of the whole thing is compromised.

For now we can try with a simple interface that allows you to build your own omni-node with custom --chain.

I am generally a fan of moving the omni-node towards being a library as well (I have called it omni-node-builder in the past), so I support @serban300 working on this. That being said, I want to see if I can convince @xlc that --chain acala vs. ---chain ./path/to/acala.json is such a small difference, that I hope you can already move the Acala nodes to omni node without needing to wait for this.

@xlc
Copy link
Contributor Author

xlc commented Aug 6, 2024

The main thing is I don't want to break compatibility. Also it will be more work to update the deployment process to add a json file alongside with a docker image / binary. Having one or two teams to update is fine but there are exchanges, RPC providers, collator runners etc all need to update their setup and they will have different setup. I just want to save people's time.

@serban300
Copy link
Contributor

Opened the following PR to address this issue: #5288

@xlc @kianenigma can you PTAL ?

@serban300
Copy link
Contributor

serban300 commented Aug 21, 2024

Merged PR #5288 which separates polkadot-parachain-bin into a lib and a bin. Listing here the items that still remain to be done:

@xlc if there are other remaining items related to this, please feel free to add them here

@kianenigma
Copy link
Contributor

@PierreBesson sorry I missed your comment about compatibility.

Could we facilitate that process somehow by creating an omni-node "extension template" where you simply have to drop your chainspecs in a folder ? I think this could go a long way to foster adoption of the omni-node. We would of course use this ourselves for our system parachains binary.

Sounds like a great idea, we will work on it.

#5288 generally solves the issue of users who wish to use polkadot-parachain-lib. But the fork in the road for parity ops running system chains is still a tiny time-bomb 🙈

We could indefinitely keep these chain-specs in polkadot-parachain, but it is not an elegant setup.

Given the comments in #5269 (comment), we can, upon one breaking release:

  1. rename the existing polkadot-parachain to polkadot-omni-parachain or something else that doesn't have these chain-specs. This binary, as noted above, can also contain the chain-spec builder and benchmarking tool again.
  2. make polkadot-parachain be one new binary that is created using polkadot-parachian-lib, and is merely a main.rs file that embeds multiple chain-specs.

@PierreBesson
Copy link

PierreBesson commented Aug 26, 2024

@kianenigma I would also fully endorse approach. Keep polkadot-parachain with all current features and as backward-compatible as possible with all system chains and introduce a new polkadot-omni-parachain that is more generic and include all tools for testing and running your "runtime-only" parachain networks.

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

5 participants