Collator node workflow #280
Conversation
polkadot/cli/src/lib.rs
Outdated
| @@ -117,6 +120,26 @@ fn base_path(matches: &clap::ArgMatches) -> PathBuf { | |||
| .unwrap_or_else(default_base_path) | |||
| } | |||
|
|
|||
| /// Additional application logic making use of the ndoe, to run asynchronously before shutdown. | |||
There was a problem hiding this comment.
making use of the node (typo)
polkadot/collator/src/lib.rs
Outdated
|
|
||
| /// Parachain context needed for collation. | ||
| /// | ||
| /// This can be implemented through an externally attached service or a stub. | ||
| pub trait ParachainContext { | ||
| pub trait ParachainContext: Clone { |
There was a problem hiding this comment.
Should we add something like this to the comment?
This is expected to be a lightweight, shared type like an Arc.
There was a problem hiding this comment.
yeah, I will add it
polkadot/collator/src/lib.rs
Outdated
| /// Produce a candidate, given the latest ingress queue information. | ||
| fn produce_candidate<I: IntoIterator<Item=(ParaId, Message)>>( | ||
| &self, | ||
| last_head: HeadData, |
There was a problem hiding this comment.
Is this deserves a doc update?
polkadot/src/main.rs
Outdated
| quick_main!(run); | ||
|
|
||
| fn run() -> cli::error::Result<()> { | ||
| cli::run(::std::env::args()) | ||
| cli::run(::std::env::args(), Application) |
There was a problem hiding this comment.
I think cli::Application is a little confusing name... Here is my experience
When I checked this file for the first time I had an impression that Application provides a main logic of, well, the application. Only when I've found that work returns future::empty() for the Application I realized that my hypothesis was wrong.
There was a problem hiding this comment.
I think of it as "application on top of the node". Since the normal operation mode is to do no additional work with the node then the work future is empty. But with the collator node the implementation of Work will be to listen to import notifications from the node and to push collated blocks to validators on the network.
I can't really think of a better name for it though.
There was a problem hiding this comment.
Yeah, figured it out pretty quick after that, so maybe not a big deal
I honestly was hoping that you will be able to come up with a better name : )
There was a problem hiding this comment.
(renaming to Worker on suggestion of @gnunicorn )
|
HatApplication. |
polkadot/collator/src/lib.rs
Outdated
| fn exit(&mut self) -> Self::Exit { | ||
| match self.exit.take() { | ||
| Some(exit) => future::Either::A(exit), | ||
| None => future::Either::B(future::empty()), |
There was a problem hiding this comment.
isn't this the return value that is then passed into the separate thread, which then performs an exit.wait() before exiting? With using future::empty() this would prevent the process from exiting ever, right? Is that intentional? If so, why do we want to prevent an exit? Or did you mean to use future::ok() to have it resolve immediately, which would make more sense to me...
There was a problem hiding this comment.
in practice exit will only get called once, and I wasn't quite sure what to do if exit is called a second time. I favored returning a future that never resolves over returning one that resolves immediately, but the other options I considered were to panic or alter Worker to return an error. No strong preference.
polkadot/collator/src/lib.rs
Outdated
| future::Either::B(future::ok(None)) | ||
| } | ||
| Err(e) => { | ||
| info!("Could not collate for parachain {:?}: {:?}", id, e); |
There was a problem hiding this comment.
shouldn't that be at least of level warn!? I feel consuming an error without actually recovering and not being verbose about it, could easily cover up important issues to be addressed.
There was a problem hiding this comment.
it might get rather spammy, but yes
gnunicorn
added
A6-mustntgrumble
and removed
A0-please_review
rphmeier
added
A0-please_review
rphmeier
added
A8-looksgood
and removed
A0-please_review
* update polkadot/types * update test dependencies as well
* Rename ValidatorLedger.total to total_nomination * Move DECIMALS to xpallet-protocol * . * . * Remove duplicate consts in runtime * .
A collator is a special polkadot full node which is attached to a particular parachain and "collates" proposals for blocks on that parachain to give to validators to consider as part of the consensus. This PR creates a futures-based workflow for a collator node, with actual collation logic encapsulated by a trait.
After #173, will pass collations to validators for agreement