Overseer

[montekki]

A very early draft PR of the Overseer implementation.

Essentially what it does is runs async tasks and lets them communicate with each other using message passing. For more motivations behind implementing such an Overseer consult the implementors-guide (linked above).

The code contains reasoning behind the current architectural approach (can be viewed with cargo doc --no-deps --open in the overseer dir) however given that:

• It is like around my third attempt in a week at designing it
• Turns out that designing convenient async APIs is not easy
  It is likely to change a lot.

To the moment have thought about this component in the most possible general way, it looks like a large portion of what we will end up with can be abstracted away in another crate/repo and implement a very generic Overseer that can be removed, it would probably be great to make it happen.

Fixes #1144

[expenses]

I had a quick skim and have made a few comments.

[drahnr]

it feels slightly misplaced to do all of this here, should not the subsystem take care of processing individual Messages?

[montekki]

The spec stresses out that the overseer itself does all the message routing to "provide a single source of heartbeat in the system" or I don't remember the exact words, but that piece of doc contains some motivation

[rphmeier]

The heartbeat is about doing this rough thing

let new_chain_head = chain_events.next().await;
future::join_all(subsystems.iter().map(|s| s.send(StartWork(...)))).await;
// thus the `StartWork` message is buffered for all subsystems before we send any further message to subsystems

[drahnr]

not a fan of start, spawn / block_on/run tend to give a better idea, of what it does, but that might be a personal pref

[montekki]

block_on is probably not the best choice since we are not blocking here, but spawn works for me

[rphmeier]

Some general comments on executors and type-erasure. I was not generally expecting the overseer to act as an executor for all subsystems and spawned tasks, but rather to just do initial spawning and provide communication for them.

Also I have some opinions on codebase structure:

polkadot
├── node
│   ├── core
│   ├── messages
│   ├── network
│   └── overseer
├── primitives
└── runtime

• Primitives are for common types between node and runtime and maybe runtime APIs
• node/messages defines all message types, probably in single crate
• runtime defines all the runtime modules
• node/core holds all core subsystem implementations.
• node/network holds all network subsystem implementations
• node/overseer holds the overseer that glues them together

[mxinden]

👍 Thanks for the well structured and well documented design. Made it easy to review!

I have a couple of ideas. As I am not very familiar with the Polkadot codebase please take these with a grain of salt!

[mxinden]

Why make this an async function without any awaits? Why not return Poll<Option<M>> here? Why deviate from the ecosystem standard that something returns Poll::Pending when not yet ready?

Suggested change

	pub async fn try_recv(&mut self) -> Result<Option<M>, ()> {
	match poll!(self.rx.next()) {
	Poll::Ready(Some(msg)) => Ok(Some(msg)),
	Poll::Ready(None) => Err(()),
	Poll::Pending => Ok(None),
	}
	}
	pub fn try_recv(&mut self, cx: Context<_>) -> Poll<Option<M>> {
	self.rx.poll_next_unpin(cx)
	}

[montekki]

We want to call it from the async context:

async {
    ctx.try_recv().await;
}

So we don't actually want the user to deal with Context since that's not something that's being passed around, rather we have to somehow get it from the environment; so that is hidden by the poll! macro and we in turn hide it's usage from the user by this method. But the method needs to be async for poll! to work.

Also it would make no sense to use Poll in the async context.

Does this explanation make any sense at all? But the return type probably could be made a bit better

[mxinden]

As a user I would be confused to await a function that explicitly returns directly (try prefix). Instead I would find poll!(ctx.try_recv()) easier to understand as it implies that the call logically returns directly.

Anyways, this is not a big deal, if no one else feels the same this should stay as it is.

[mxinden]

As a user I would be confused to await a function that explicitly returns directly (try prefix). Instead I would find poll!(ctx.try_recv()) easier to understand as it implies that the call logically returns directly.

Anyways, this is not a big deal, if no one else feels the same this should stay as it is.

[montekki]

@rphmeier I attempted to solve the problem of communication with the outside world in 564964c (the latest commit atm) and added forwarding of timer events from outside world to the example code. I think that the results is not that bad actually.
So now I think that feature-wise it's ready for re-reviewing as I intend to add no new features to this PR. After this eventually passes the review I will re-arrange the directory structure to avoid a mess.

[montekki]

When the subsystems communicate with each other they use the api of the context, so to send message from one subsystem to another subsystem they use

	/// Send a direct message to some other `Subsystem` you know the `I`d of.
	pub async fn send_msg(&mut self, to: I, msg: M) -> SubsystemResult<()> {

When another subsystem receives this message it will receive

enum FromOverseer {
..,
	/// Some other `Subsystem`'s message.
	Communication {
		msg: M,
		from: Option<I>,
	},
}

By looking at the from field of the message a subsystem knows if it was received from another subystem (Some) or from the outside world. In the first case it may use send_msg to answer back (request-response interaction) or not (just a notification case).

Now when a subsystem wants to communicate to someone outside the overseer it uses

	/// Send a message to some entity that resides outside of the `Overseer`.
	pub async fn send_msg_outside(&mut self, msg: M) -> SubsystemResult<()> {
		self.tx.send(ToOverseer::SubsystemMessage {

Which will eventually be exiting the Overseer via OverseerHandler and the outside code has to know how to route this message.

When the outside code wants to send a message to one of the Subsystems in the Overseer it uses

	/// Send some message to one of the `Subsystem`s.
	pub async fn send_to_subsystem(&mut self, to: I, msg: M) -> SubsystemResult<()> {
		self.events_tx.send(Event::MsgToSubsystem {

api of the OverseerHandler. So the outside code is assumed to have knowledge of what I we are being generic over but it would be the same if we just used pre-defined big struct.

[rphmeier]

The send_message_outside thing can't differentiate between multiple requests, though.

the outside code has to know how to route this message.

Wouldn't the overseer have to know how to route the message?

In the first case it may use send_msg to answer back (request-response interaction) or not (just a notification case).

If both A and B want to send ValidationSubsystem::ValidateBlock. A and B both need a BlockValidatedMessage variant, right? Or is this the idea behind the accepts_message thing, that you'd have a ValidationSubsystemResponse message type, and both A and B also accept that message type?

Also I guess we need some kind of unique RequestId for all requests, so we can coordinate requests and responses. And all subsystems will need to implement this logic, of matching

pending_requests: HashMap<RequestId, RequestKindsThisSubsystemMakes>

against incoming messages.

The overseer will also have to do this, but it'll have to have a big enum of all possible response types, which is some of the difficulty I was mentioning.

In the first case it may use send_msg to answer back (request-response interaction) or not (just a notification case).

On the other hand, we'd have oneshot::Receiver.await which does not require the addressing scheme.

So, in short, we are running into the exact issues that I predicted at every step...

[rphmeier]

Generally looks good, just some style nits.

I like the use of RemoteHandle in detecting early exit and in the Stop routine.