Vision for cleaner substrate-node's developer-facing interface

[kianenigma]

related to #186 #1337

Vision 1

As it stands now, we take it for granted that that one builds a runtime, then pulls the substrate-node-template (or some similar repository of code), integrate it in a "blind" fashion (using probably nasty trial and error, without really knowing what they are doing) and never look at the node side code ever again.

Taking the same mindset that I talked about for a "substrate-node-builder-cli" here, Imagine an alternative like this:

• You build your pallet and runtime using feat: FRAME umbrella crate. #1337
• Then, you build a new project like:

[dependencies]
my-runtime = "1.0.0"
substrate-node-builder = "1.0.0"

• a main.rs that looks like:

use substrate_node_builder::{Builder, Consensus, Subcommand};
 
let node = Builder::default()
    // wire your node and runtime. All of the random types that the node needs from the runtime should not 
    // be raw re-exports anymore, and should be done via a `trait`.
    // this should also be helpful 
    .runtime(my_runtime::MyRuntime)
    // define your consensus engine. Can only be one of a few options. 
    .consensus(Consensus::ManualSeal)
    // Tweak the list of sub-commands from `Default`
    .add_subcommand(Subcommand::TryRuntime)
    .add_subcommand(Subcommand::Revert)
    .remove_subcommand(Subcommand::Benchmarks)
    // Tweak the list of RPCs endpoints
    .add_rpc(Rpc::Foo)
    .remove_rpc(Rpc::Bar)
    .build()
    .unwrap();

fn main() {
    node.run();
}

This is basically what a "creat-substrate-app" CLI would do for you, but done via code. Once we have this, the CLI would be rather trivial to build, and would merely be something that converts a YAML/JSON file t the above piece of code.

Vision 2

I have little clue about what are the blockers to reach the above. Historically I knew that code in service.rs has been quite a difficult one to play with. But, looking at a very simple node-template with manual seal, I think I can wrap my head around it and conclude that the above is feasible.

But, if the above is not possible, what I would be a good improvement to the existing quo, especially from a DevEx perspective is to replace the need to clone any code for the node software with pure binaries.

That is, once #62 is done, and there is less (hopefully none!) strict dependency between the node and the runtime, the process to learn FRAME and run a basic chain would be:

1. build your runtime, again with feat: FRAME umbrella crate. #1337
2. go to a release/download page where you are presented a list of node software different pre-configurations, such as consensus options.
3. You download the software, for example node and run ./node --runtime path/to/wasm.wasm <rest of substrate cli opts> and the rest should work.

I am not sure if this is actually much simpler than the above, as it would require some checks like seeing which runtime api/rpcs are available to become runtime (os opposed to the current compile-time) checks.

[kianenigma]

(Once I get some initial feedback, unless if this is totally crazy, I would like to post this in the parity forum for other eco-dev related departments to also see.)

[xlc]

I don't see much reason to have custom subcommand.
We don't want custom RPC.
The consensus is the same for all the parachains.
Runtime can be fetched from chain spec.
How about just create a universal client that works with all the parachains?

[arturgontijo]

We don't want custom RPC.

Why not? I mean, why impose that limit to the builders?
I see that the best practice would be calling RuntimeApi but I see no reason to limit it in a development tool like this.

[xlc]

It is all about trade offs. If you believe support custom RPC have more advantages than disadvantages, sure, list them out and we good.

Please keep in mind that Substrate is so flexible to the point it is pain to use and we still haven’t learned lessons from this?

[bkchr]

Yeah, custom RPCs are not really needed. This simple kind of interface should be to onboard people really easy. There would be some advantaged mode where you can still add your custom RPCs and whatever. So, there will be some "hard mode" and some "easy mode".

I already spoke with @kianenigma and I support this for parachains: paritytech/cumulus#2181

How about just create a universal client that works with all the parachains?

Yes, but to make this properly it will require quite a lot of work.

[arturgontijo]

So, there will be some "hard mode" and some "easy mode".

Ok, yeah...makes sense.

[Polkadot-Forum]

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

https://forum.polkadot.network/t/psa-parity-is-currently-working-on-merging-the-polkadot-stack-repositories-into-one-single-repository/2883/14

[gilescope]

@expenses and @gnunicorn were also keen on this happening.

[rphmeier]

Strong disagree on any Consensus subcommand.

There's a lot of stuff baked into consensus that doesn't usually get considered:

• Custom networking protocols
• Fork choice rules
• Many long-running tasks
• Running with multiple consensus engines

Consensus is usually not just one task but many things stitched together.

In my view, we should separate the infrastructure that Substrate (interfaces to libp2p, a block database, maybe stuff like sync or tx-gossip) from everything else. These should be provided as context into long-running futures that all just have absolute freedom in how they use these things.

Also, I don't see why the runtime should factor into this at all. The runtime should be encapsulated entirely by the chain-spec once the native runtime is gone, right? chain-spec is a CLI flag and can be handled internally.

Another goal is that we should not only abstract complexity at the very outer layer, but also make extending these functionalities very simple. The simplest thing is just letting users spawn futures.

For example - starting with an Aura parachain, but then adding a bunch of off-chain logic and networking protocols for e.g. offchain storage or oracles. This should be an easy tweak to the templates we provide.

My ideal here would be something like

// Some "mega object" that gives you access to:
//  - Registering new network protocols
//  - Adding block import hooks and notifications
//  - Spawning tasks
//  - Importing blocks
//  - Finalizing blocks
//  - Generating storage proofs
//  - Calling runtime APIs
//  - Setting the best block
//  - Adding RPCs
//  - Reading blocks and headers
//  - Reading and writing arbitrary items in the DB
//  - Reading state
//  - Executing Wasm
//  - etc.
//   
// It's not intended to be `Clone`d, but will give shared ownership of handles for all these things to avoid leaking a 
// god-object everywhere.
let raw_node: Node = RawNode::<Block>::new(cli_params);

The Raw Node does nothing on its own. It's just a bag of all the raw components you need to do anything interesting. This makes starting consensus logic simple.

Here are some examples (which could be library functions exposed by top-level Substrate/Cumulus crates).

A single function for running a standalone Babe/Grandpa node:

// Example for a standalone Babe/Grandpa node
fn vanilla_substrate() {
  let raw_node = RawNode::new(cli_params);

  // Under the hood, this is setting up new networking protocols, block import hooks, new DB
  spawn(run_grandpa(&raw_node));
  spawn(run_babe(&raw_node));

  // wait...
}

How it might be used in Polkadot or other complex systems with extra subsystems added:

fn polkadot() {
  let raw_node = RawNode::new(cli_params);

  // Under the hood, this is setting up new networking protocols, block import hooks, new DB
  spawn(run_grandpa(&raw_node));
  spawn(run_babe(&raw_node));
  spawn(run_parachain_consensus(&raw_node));

  // wait...
}

How it might be used for a one-size-fits-all parachain with Aura:

fn aura_parachain(cli_params) {
  // This may be starting a Polkadot raw node underneath. Or not. 
  let relay_chain_interface = make_relay_chain_interface(cli_params);

  let parachain_raw_node = RawNode::new(cli_params);

  // Under the hood, this is setting up new networking protocols, block import hooks, new DB
  spawn(run_parachain_general(&raw_node, relay_chain_interface));
  spawn(run_parachain_aura(&raw_node, relay_chain_interface));

  // wait...
}

This is pretty dead-simple and would hide all the complexity of registering network protocols, starting services, etc. inside of specialized crates that aren't exposed to the user except through a simple one or two parameter function.

I could see things getting a little more complex, i.e. if we let people swap in Proposers for consensus algorithms, but it still all stays very simple. Substrate could expose a DefaultProposer as it currently does in basic_authorship, for instance. Or even extracting out "standard" networking protocols like sync and transaction pooling, but having a utility function that spawns these things under the hood.

We need to make the easy stuff easy while keeping the hard stuff possible.

From above:

The stuff that the node provides ... maybe stuff like sync or tx-gossip

It's not clear to me that sync or tx-gossip shouldn't just be long-running futures themselves instead of baked into the service crate. These are also just components that build on top of the raw node.

[rphmeier]

@bkchr I am interested in sketching out some issues related to the above refactoring approach, if it's an agreeable direction. I am coming off of a larger project and would like to spend some time making Substrate more fun to work with for end-users, and this is one of the higher impact things I feel I can reason well about. Furthermore, many of these issues are the result of my own decisions in 2018/19 and I have some personal responsibility to improve the situation as a result.

Some initial suggestions with the goal of creating the struct RawNode:

1. Remove reliance on static dispatch in BlockImport. This is currently one of the main sources of setup hell, as we first set up BABE, GRANDPA, BEEFY, etc. BlockImport implementations and then spawn the tasks. I'd advocate for a simplification of the BlockImport trait and an implementation that wraps a Vec<dyn BlockImport> instead of setting them up like a composable onion.
2. Likewise, we amend the ImportQueue interface to accept an arbitrary amount of custom Verifiers via dynamic dispatch.
3. Remove the Proposer/Environment split. There really should just be one Proposer trait: fn propose(&self, parent_hash) -> Block
4. Create a general-purpose AllRuntimeApis: ProvideRuntimeApi type as described in Remove sp-api requirement on compile time bounds #27 which we can use to statically type the client. This is so the RawNode<Block> can have an internal client: Client<Block, SpecificBackend, SpecificExecutor, AllRuntimeApis>.
5. Decouple any enshrined network protocols from the starting of the network and add utility functions for spawning them.

[bkchr]

While we don't have done that much work on the front of the node refactoring, we have collected ideas etc here: https://www.notion.so/paritytechnologies/Node-Refactoring-937b5770d14c494991903a4b7ce52012

1. Remove reliance on static dispatch in BlockImport. This is currently one of the main sources of setup hell, as we first set up BABE, GRANDPA, BEEFY, etc. BlockImport implementations and then spawn the tasks. I'd advocate for a simplification of the BlockImport trait and an implementation that wraps a Vec<dyn BlockImport> instead of setting them up like a composable onion.

Yes this is already going into the right direction, while I think we should even go a step further. Do we really need a BlockImport trait on the level of BABE or Grandpa? I would say no, we just need both implementations providing functions for doing this. You can still have these traits, but on the level where you stick together these parts. For Polkadot we would may not even need this. We would just need one function block_import that is calling the correct objects in the correct order. The mistake we have done before, the assumption about how other components work should be prevented this way (hopefully).

2. Likewise, we amend the ImportQueue interface to accept an arbitrary amount of custom Verifiers via dynamic dispatch.

The ImportQueue should not even need a verifier. It should be the job of the sync job to verify incoming headers before we request the blocks:

(image taken from the notion above)

The job of the import queue is to import blocks and not to verify the integrity of consensus seals etc.

3. Remove the Proposer/Environment split. There really should just be one Proposer trait: fn propose(&self, parent_hash) -> Block

Makes sense.

Create a general-purpose AllRuntimeApis: ProvideRuntimeApi type as described in #27 which we can use to statically type the client. This is so the RawNode can have an internal client: Client<Block, SpecificBackend, SpecificExecutor, AllRuntimeApis>.

We really want to get rid off the Client. This god object is just wrong and we should slowly untangle it. I'm also against this AllRuntimeApis type as it would serve no purpose. The static checking has no advantage if we just insert some type that implements all runtime apis automatically.

5. Decouple any enshrined network protocols from the starting of the network and add utility functions for spawning them.

💯 and we have done some great work on this. Thanks to @altonen and his team the sync is almost a "free standing" protocol and also all the other protocols got moved out of sc-network. sc-network should just be the tool that provides you with ways for spawning a protocol and not requiring you that every Substrate node is running a particular protocol.

We have developed Substrate with Polkadot in mind, which made sense and was a totally reasonable way to approach the problem we had ahead of us. However, now we need to break the chain ;) between Polkadot and Substrate to make Substrate the generic framework it should be (still in some bounds, but you hopefully get what I mean).

Now to this issue, as I already had said above, this idea of having a builder interface for creating a node is not something we didn't also thought about. I think this will also crucial as I want to have this sc-service being removed (or most of it). These functions inside this crate have all these assumptions on what you want to start and thus pull in most of Substrate, which makes every node Polkadot like in some way. By default Substrate should expose the "expert" way of building a node. This means you just take the components and stick them together in your downstream project as you need them. Expert not in the way that you need to know in every detail how everything works, but expert in the way that you should have an understanding what you are sticking there together. We would use this "expert mode" to build the Polkadot node. It would give us much better ways to reason about on when something is written to the database and thus, it is canonical and when this property doesn't hold. We could also finally have full control on when and in what frequency we want to tell something about imported blocks, because we know all the components that need this information and can send it directly from the "db task". I could probably continue this list for quite some time.

On top of this "expert mode" we would then provide this builder interface. The builder interface would give users a very simple interface to build their own node. However, at some point it would stop its support and would require the user to switch to the "expert mode". This builder interface will also enable to do proper versioning quite easy, as we only need to version the functions that the builder exposes as the rest should be hidden in the implementation. I'm still in favor that we start with this builder pattern like approach for Parachains. We don't need to "waste" time now on standalone blockchains, because that isn't our focus.

[rphmeier]

Do we really need a BlockImport trait on the level of BABE or Grandpa? I would say no, we just need both implementations providing functions for doing this

Sure, there's not much difference between a Vec<Box<dyn BlockImport>> or Vec<Box<dyn Fn>>. Though it's more idiomatic to use the trait and then blanket impl it for Fns so as to expose an interface which is both closure-friendly but doesn't force the user to use a closure.

The job of the import queue is to import blocks and not to verify the integrity of consensus seals etc.

The job of the import queue is 100% to verify the integrity of consensus seals, we just do it in a half-baked way.

In particular, it's theoretically important when performing a full sync from genesis. Doing full sync quickly requires parallel verification of headers which are pending import. Every Ethereum node does this, including parity-ethereum from 2016. There are two phases to import: things which can be done in parallel (PoW checks, signature checks), and things which must be done sequentially. While no Substrate consensus logic currently makes use of this, I don't see a good motivation for removing the possibility. One day, someone will be tasked with making full sync fast.

We really want to get rid off the Client. This god object is just wrong and we should slowly untangle it.

Is it that much of a god object? It combines database access and code execution with some more superfluous stuff like fork choice rules. We might destructure it somewhat but the ability for combining those two things is definitely needed, at the very least in a read-only way.

This builder interface will also enable to do proper versioning quite easy, as we only need to version the functions that the builder exposes as the rest should be hidden in the implementation. I'm still in favor that we start with this builder pattern like approach for Parachains

I'm fine with a builder-like interface as long as it doesn't try to proscribe things like consensus and we expect that things like GRANDPA will have a fn run(&mut Builder).

"builder pattern" vs "expert mode" feels like a false dichotomy. I believe a builder pattern could easily get more complicated than a well-written API for "expert mode" (the RawNode proposal).

i.e. when you spawn GRANDPA you want:

1. import queue logic
2. block import logic
3. custom RPCs
4. custom networking protocol

The builder pattern (without a fn grandpa::run(&mut NodeBuilder)) would require the end user to make calls for each of those instead of encapsulating them.

[bkchr]

Sure, there's not much difference between a Vec<Box<dyn BlockImport>> or Vec<Box<dyn Fn>>. Though it's more idiomatic to use the trait and then blanket impl it for Fns so as to expose an interface which is both closure-friendly but doesn't force the user to use a closure.

I meant this on the BABE/Grandpa crate level. There we probably don't need this trait. You can still introduce a trait on a higher level when needed.

The job of the import queue is 100% to verify the integrity of consensus seals, we just do it in a half-baked way.

In particular, it's theoretically important when performing a full sync from genesis. Doing full sync quickly requires parallel verification of headers which are pending import.

Yes I know that we doesn't support this. Still I don't know how parallel verification is related to block import. You get new header or even blocks from the network. You verify these in parallel and then you pass the blocks in the import queue. The main thing that changes here is that we split the block import from the verification. And by doing this we achieve exactly what you want, we achieve to run verification in parallel and then block import can continue in sequential mode. However, as I would also like to move the seal removal into the runtime, we would not achieve that much from doing it in parallel before. But someone could argue that you pass in a "seal already" checked argument to execute_block which results in just removing the seal, but not verifying it.

Is it that much of a god object? It combines database access and code execution with some more superfluous stuff like fork choice rules

The main problem with the current model is that you bring too much assumptions into the process. Too many weird branches etc to support all the small differences. This works for stuff like ETH where you have a specific implementation of a blockchain. However, it doesn't work for a framework that wants to be generic for all its users. There are tons of "hacks" we added to support custom implementations of X and Y. Fork choice rule would for example something that lives in the database write task. This task in the only task that can give you a transactional view onto the database, as it is the one that writes to it. If you need to know exactly what is the block you want to build on, you need to call into this database task (it would be a channel that communicates with the object). However, things like RPC don't need the strong guarantee and are mainly happy with a good enough guarantee that the block its getting is the best block.

One main problem we also have is that the database is aware of what is the best or finalized block. The database should not know this. The database should store blocks and give you an interface to query them. The fork choice rule should be the one deciding on what is the best block. This would also solve things like the informant showing a wrong best block for the relay chain when we have a dispute until the new fork is seen as the best chain. The finalized block also isn't interesting to the database, the database just needs to expose some interface prune and this is called by the consensus engine when something is finalized.

The builder pattern (without a fn grandpa::run(&mut NodeBuilder)) would require the end user to make calls for each of those instead of encapsulating them.

I would expect that the builder has functions like node_with_grandpa_and_aura or whatever kind of combinations we want to provide. So, we would provide some sort of predefined sets of node configuration. The builder pattern should be seen as highly opinionated interface that is provided by us to help users for a quick start and not more. We should not again try to mix things. We already have done this and look at sc-service and how it looks like.

[rphmeier]

I would expect that the builder has functions like node_with_grandpa_and_aura or whatever kind of combinations we want to provide.

The implementation of that function would look pretty close to this.

fn run_grandpa_and_aura(node: &mut RawNodeForSpawningTasksAndAddingHooksAndMetricsAndWhatever) {
  grandpa::run(node);
  aura::run(node);
}

where those crates handle setting up block import hooks, network protocols, whatever. This does not look too difficult for the average programmer.

Making things simple doesn't require anything more than just exposing the raw functionality that tasks need to run. What I mean is that I don't understand the justification for adding a builder pattern alongside the general refactoring, when just refactoring node startup already makes things easy for the end user.

Basically, I see service.rs files as having two problems which make it unwieldy:

1. multi-phase setup (mostly because of block import implementations and networking protocols)
2. too many parameters passed around (solution: put all the parameters which are handles to the network, database, RPCs, metrics, etc. in one struct that gets passed around).

The main thing that changes here is that we split the block import from the verification

Okay, yes, this is reasonable. There are some complications around runtime upgrades but that can mostly be worked around at higher levels.

Fork choice rule would for example something that lives in the database write task. This task in the only task that can give you a transactional view onto the database, as it is the one that writes to it

Fork choice rules should just be background tasks like anything else. We should not make any assumptions that a fork choice rule is a function that only takes the current known blocks as input. (see https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/node/core/chain-selection/src/lib.rs).

[bkchr]

Fork choice rules should just be background tasks like anything else. We should not make any assumptions that a fork choice rule is a function that only takes the current known blocks as input. (see https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/node/core/chain-selection/src/lib.rs).

I never said that this is an assumption. It is just about the synchronization point between looking into the db an ensuring that the db doesn't change while you determine the best block. That you need to have access to all blocks and more information is clear to me ;)

What I mean is that I don't understand the justification for adding a builder pattern alongside the general refactoring, when just refactoring node startup already makes things easy for the end user.

General refactoring and builder pattern are mostly orthogonal things. I still think that you assume too much knowledge from the user side. I'm looking more on the people wanting to get something up fast for experimentation. However, I also don't have any hard requirements on this stuff as long as we go into the direction I tried to outline above. The things you have proposed are also mainly fine and it is just about small details and we could probably make both work.

[rphmeier]

It is just about the synchronization point between looking into the db an ensuring that the db doesn't change while you determine the best block

The only thing a fork choice rule really needs is that blocks don't disappear from the DB. There might be some necessary CAS logic but that is a lower level concern where I think we can break any coupling (this seems good).

General refactoring and builder pattern are mostly orthogonal things. I still think that you assume too much knowledge from the user side. I'm looking more on the people wanting to get something up fast for experimentation

I don't think I'm assuming any knowledge - some preconfigured templates and single function calls would make things really easy for anyone. It's just an API design philosophy, "make the easy things easy and the hard things possible".

The stack I'm imagining is:

1. The Substrate node infrastructure itself. This is the database, runtime API, libp2p setup, RPCs, metrics etc.
2. Consensus level tasks which use the capabilities of the raw node. Sync, GRANDPA, BABE, fork choice rules, sit at this level.
3. Pre-packaged combinations of tasks. These are small functions that do things like start commonly grouped tasks, or Orchestra implementations.
4. Out of the box "just start a node" functions, where the users don't care at all what happens under the hood

If you really want to target the audience of people who are looking to experiment, we can't just write code to target level (4) and call it a day. Those users will then ask questions like "how do I add a custom RPC?" or "how can I add extra background tasks to the node?". It should be easy to answer those questions without pushing them into level (1). That is, it needs to be easy to move from writing code at level (4) to writing at level (3), (2), or even (1).

A good programmer would be able to figure this out from the docs, as long as the code is explicit enough. They'd see that the fn run_grandpa takes a RawNode as a parameter and that they are explicitly calling run_grandpa in their own user-defined code. Then it's an obvious step to try writing their own function that takes RawNode as a parameter and call that.

My main concern is that shifting the "paradigm" between level 4 and level 3 is not going to be good for the developer community to do anything other than basic demos.

[bkchr]

The only thing a fork choice rule really needs is that blocks don't disappear from the DB. There might be some necessary CAS logic but that is a lower level concern where I think we can break any coupling (this seems good).

Yeah. In general I want to prevent stuff like this. This import_lock should not exist, nor should it be exposed in public api. It works, but it is a hack.

• The Substrate node infrastructure itself. This is the database, runtime API, libp2p setup, RPCs, metrics etc.

• Consensus level tasks which use the capabilities of the raw node. Sync, GRANDPA, BABE, fork choice rules, sit at this level.

• Pre-packaged combinations of tasks. These are small functions that do things like start commonly grouped tasks, or Orchestra implementations.

• Out of the box "just start a node" functions, where the users don't care at all what happens under the hood

I'm with you 100% or better 99.99% ;) I think the main difference being where things are being exposed etc. Nothing that is really blocking IMO.

[Polkadot-Forum]

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

https://forum.polkadot.network/t/developer-experience-must-be-our-1-priority/3957/8

[Polkadot-Forum]

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

https://forum.polkadot.network/t/developer-experience-must-be-our-1-priority/3957/12

[Polkadot-Forum]

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

https://forum.polkadot.network/t/developer-experience-must-be-our-1-priority/3957/47

[Polkadot-Forum]

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

https://forum.polkadot.network/t/developer-experience-must-be-our-1-priority/3957/48

[xlc]

need to make sure this is addressed #2499

[Polkadot-Forum]

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