Merge subspace-executor executable into subspace-node #366
Conversation
`RelayChainCli` will be evolved into `SecondaryChainCli` later.
It's mandatory to run the embedded subspace node in authorty mode for now, I was stucked for a while due to the CLI flag `--validator` was missed, so it makes sense to me to forcibly enforce this flag.
liuchengxu
force-pushed
the
merge-executor
branch
from
b638803
to
3a05f5c
Compare
| @@ -19,6 +19,7 @@ include = [ | |||
| targets = ["x86_64-unknown-linux-gnu"] | |||
|
|
|||
| [dependencies] | |||
| cirrus-node = { version = "0.1.0", path = "../../cumulus/node" } | |||
There was a problem hiding this comment.
Should we add it to runtime-benchmarks feature below?
There was a problem hiding this comment.
Yes, we should, but runtime-benchmarks is actually unused at present. It also reminds me that we might also want to actually integrate try-runtime feature for convenient runtime upgrade testing.
| @@ -74,7 +74,39 @@ fn main() -> std::result::Result<(), Error> { | |||
| let cli = Cli::from_args(); | |||
|
|
|||
| if !cli.secondarychain_args.is_empty() { | |||
| println!("Unimplemented: Run an executor with an embedded primary full node"); | |||
| let secondary_chain_cli = | |||
There was a problem hiding this comment.
I don't understand why is this all before match statement, shouldn't it be in None => { branch os match statement instead?
Also Subcommand::PurgeChain command wasn't updated the way it behaved in subspace-executor accordingly and probably some others that I don't recall immediately.
I was expecting that primary chain would start just like before and in its .map(|full| { we'd check for cli.secondarychain_args and run it before calling full.network_starter.start_network();. Right now it doesn't seem like networking is started at all.
There was a problem hiding this comment.
I don't understand why is this all before match statement, shouldn't it be in None => { branch os match statement instead?
That's reasonable, updated.
Also Subcommand::PurgeChain command wasn't updated the way it behaved in subspace-executor accordingly and probably some others that I don't recall immediately.
All executor-related sub-commands are TODO for now.
I was expecting that primary chain would start just like before and in its .map(|full| { we'd check for cli.secondarychain_args and run it before calling full.network_starter.start_network();. Right now it doesn't seem like networking is started at all.
When running an executor, the runner has to be created using SecondaryChainCli in which the primary node will be started instead of creating a runner using the primary node arguments.
Primary networking is started here:
https://github.com/subspace/subspace/blob/fba21ad1355e475a0ddf63fb747464cc04a524be/cumulus/node/src/service.rs#L308
There was a problem hiding this comment.
I see. In future we'll probably want to have it the other way around and start networking outside of that.
| ```bash | ||
| $ ./target/release/subspace-node build-spec --chain=dev --raw --disable-default-bootnode > dev.json | ||
| ``` | ||
|
|
||
| ### Spin up a local testnet | ||
|
|
||
| 1. Run a primary node. | ||
|
|
||
| ```bash | ||
| $ ./target/release/subspace-node --dev -d tmp --log=txpool=trace,gossip::executor=trace |
There was a problem hiding this comment.
Do you need to start it separately now BTW?
There was a problem hiding this comment.
It's a standalone consensus node, which will always be started separately.
There was a problem hiding this comment.
Hm, but we can combine it with executor node for simpler testing, right?
There was a problem hiding this comment.
Hmm, I don't see what's the benefit of combining it, this standalone primary full node plays the role of the consensus layer which keeps producing primary blocks, then the executor will process each of them to produce secondary blocks, it's not complex and clear regrading the actual work flow.
There was a problem hiding this comment.
The benefit is that you wouldn't need to build it first, then running one command, looking at node ID and running second command. You could just use cargo run and have everything working in one step.
There was a problem hiding this comment.
The benefit is that you wouldn't need to build it first, then running one command, looking at node ID and running second command. You could just use cargo run and have everything working in one step.
Sorry that I still don't understand how you can run the standalone primary node and executor node with just one command.
I have been used to building everything first in the release mode to save some disk space as I used an old MBP with 512GB only :D.
There was a problem hiding this comment.
Executor node has perfectly capable primary node in it already (minus RPC endpoint that is currently disabled, but I'm working on re-enabling among other things). I don't see why you can't use it as RPC endpoint for farmer. It already fires slot info notifications, just connect farmer to it and go!
There was a problem hiding this comment.
Okay, you mean making the embedded primary node able to produce blocks by connecting to some farmer just like the regular node? Not sure it's a better way as in practice the executor node does not have to be a farmer, the embedded primary node should be just transparent, in the future, the embedded primary full node might be eliminated and replaced by a WS client, all the communications between the primary node and secondary node will be achieved by the RPC requests, apart from the benefits mentioned in this thread, it also makes sense to me due to the executor node can be a bit lightweight as running a primary full node comes with a cost in any way, can be great if that cost can be saved.
There was a problem hiding this comment.
the embedded primary node should be just transparent, in the future, the embedded primary full node might be eliminated and replaced by a WS client
As far as I can see it will not. They are tightly coupled and it is no longer primary node embedded in secondary node, it is secondary node as an optional feature in the primary node and I don't see them being separate going forward.
all the communications between the primary node and secondary node will be achieved by the RPC requests, apart from the benefits mentioned in paritytech/cumulus#545, it also makes sense to me due to the executor node can be a bit lightweight as running a primary full node comes with a cost in any way, can be great if that cost can be saved
We don't have the same problem as Cumulus has where some independent upstream project forces you to update. We also don't have a use case for running multiple "parachain" nodes with one "relay chain" node, we always have 1 to 1 mapping. As to overhead, the overhead of primary chain can be considered negligible comparing to the load executor will have, hence we don't need to worry about it. Also RPC layer introduces a whole bunch of networking-related edge cases, memory copied, serialization/deserialization and other complexities.
I agree it makes a lot of sense for Polkadot+Cumulus, but I don't see that it is relevant for Primary+Secondary chain in Subspace.
There was a problem hiding this comment.
I don't see that it is relevant for Primary+Secondary chain in Subspace.
Well, I have to admit that I don't know Primary+Secondary chain in Subspace then :P
This PR finally abandons the subspace-executor binary and merges it into subspace-node, the executor sub-commands are still a TODO though, honestly, I haven't got a good solution for that. Some code can be cleaned up further, but I'd do it later.