Implement Network Bridge

[rphmeier]

Closes #1243

This is a concrete implementation of the network bridge subsystem using sc_network.

It also extracts the Subsystem machinery to a new polkadot_node_subsystem crate which absorbs the previous polkadot_node_messages crate. At the same time, it decouples Subsystems from the overseer by making the SubsystemContext a trait. This is done with the async-trait crate, which will lead us to incur an addition Box and corresponding virtual call to each subsystem context method. If this proves to be a performance hit, we'll have do something more verbose to work around it.

These changes to the Subsystem trait make things more mockable, so this also includes a mock subsystem context that can be used to test other subsystem implementations as well.

[mxinden]

I looked a bit into why Network was required to be Sync even though there is no obvious need for sending a reference across threads. This follows up on #1280 (comment).

Why I find this important

The reason I find this discussion important is the fact that Sync is infectious as in if an upper component requires it, all lower components do so as well. This can have larger architectural impacts and might force a type to be wrapped in a Mutex without any need for the additional synchronization.

The compiler error

When removing Sync from Network multiple similar compile failures within node/network/bridge/src/lib.rs run_network occur. Let's look into the simplest one:

(Make sure to use trait bounds instead of `impl` for better compiler error messages.)

diff --git a/node/network/bridge/src/lib.rs b/node/network/bridge/src/lib.rs
index 1bd13f9a..c8e339f8 100644
--- a/node/network/bridge/src/lib.rs
+++ b/node/network/bridge/src/lib.rs
@@ -236,7 +236,7 @@ async fn update_view(
        Some(NetworkBridgeEvent::OurViewChange(local_view.clone()))
 }
 
-async fn run_network(net: impl Network, mut ctx: impl SubsystemContext<Message=NetworkBridgeMessage>) {
+async fn run_network<N: Network>(net: N, mut ctx: impl SubsystemContext<Message=NetworkBridgeMessage>) {
        let mut event_stream = net.event_stream().fuse();
 
        // Most recent heads are at the back.

polkadot/node/network/bridge/src/lib.rs

Lines 282 to 284 in 8ac6269

	Action::ReportPeer(peer, rep) => {
	net.report_peer(peer, rep).await
	}

error: future cannot be sent between threads safely
   --> node/network/bridge/src/lib.rs:116:53
    |
116 |         SpawnedSubsystem(run_network(self.0.clone(), ctx).boxed())
    |                                                           ^^^^^ future returned by `run_network` is not `Send`
    |
    = help: the trait `std::marker::Sync` is not implemented for `N`
note: future is not `Send` as this value is used across an await
   --> node/network/bridge/src/lib.rs:283:5
    |
283 |                 net.report_peer(peer, rep).await
    |                 ---^^^^^^^^^^^^^^^^^^^^^^^
    |                 |
    |                 await occurs here, with `net` maybe used later
    |                 has type `&N` which is not `Send`
284 |             }
    |             - `net` is later dropped here
help: consider moving this into a `let` binding to create a shorter lived borrow
   --> node/network/bridge/src/lib.rs:283:5
    |
283 |                 net.report_peer(peer, rep).await
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
help: consider further restricting type parameter `N`
    |
113 |     where C: SubsystemContext<Message=NetworkBridgeMessage>, N: std::marker::Sync

Explanation

net.report_peer borrows net. Given the await of the returned future this borrow lives across await points. At the same time, as it is an immutable and not a mutable reference, net might be borrowed another time. In the case of two borrows and the fact that async/await futures need to be Send, there is now the possibility of a shared reference of net being send across threads and thus net needs to be Sync. (Please correct me in case I am missing something.)

let binding fix

Applying the first suggestion from the compile error fixes the issue. By using a let binding one can create a shorter lived borrow of net and thus there is no need to require Sync.

Action::ReportPeer(peer, rep) => {
	let report_fut = net.report_peer(peer, rep);
	report_fut.await;
}

&mut self fix

An alternative approach is described in rust-lang/rust#71010. Currently methods on Network require &self to be passed. Changing this to be a &mut self prevents the issue of multiple possible references to net as multiple mutable references are not allowed in the first place.

---

Discussion

I see three ways moving forward:

1. Keep the requirement on Network to be Sync. This would prevent any types that have interior mutability in a non-thread-safe from implementing Network.

2. Use the above let binding trick each time one awaits a Future returned from a Network method.

3. Having all methods of Network take a &mut self instead of a &self. This would enable us in the future to have a simple futures::channel::mpsc::Sender implement the Network trait.

I am in favor of (3) as (a) it allows more implementations of the Network trait (non Sync, simple Sender, ...) and thus makes future changes easy and (b) it is not bloating the code like the let binding trick.

@rphmeier what do you think?

Also adding @tomaka as I am curious if you have an alternative solution.

[mxinden]

What is the reason for sending a message to all but the first one to only then send the same message to the first one?

[rphmeier]

It prevents a clone if we are sending only to one peer, and these messages can be pretty large in practical situations.

[mxinden]

I was curious whether this loop unrolling would be more performant than the optimizations rustc and llvm do already.

I wrote a small benchmark https://github.com/mxinden/clone-trick/blob/master/benches/bench.rs distributing a message of 1 KB, 10 KB, 100 KB and 1 MB among 20 peers.

The manual loop unrolling does have a performance impact, but a small one. While the simple implementation takes 3.9 us to distribute a 10 KB message to 20 peers the optimized implementation takes 3.6 us (on a i7-8550U).

I would expect that if write_notification accepts some type that allows sharing of the underlying allocation (e.g. Bytes or Arc<Vec<u8>> like we do for NotificationsReceived the entire memory allocation overhead would vanish.

@rphmeier in case you want to keep the optimization in here (e.g. for the impact it has in case there is only a single peer and thus no clone needed), would you mind adding a comment describing why this is not a single for loop?

[rphmeier]

It's a memory optimization, not a performance one. Computers are pretty fast, but if I'm sending a 10MiB message to 20 peers I'd rather keep memory usage down by 5% while this is buffered. This would be solved by having the lower-level APIs take Bytes, BTW, and the comment I added yesterday (but didn't push) notes that.

[rphmeier]

It would also be addressed by the lower level APIs accepting a set of peers to send a message to. I don't view this optimization as premature. PoVs are likely to be in the range of 1-10 MiB and we gossip them to all our peers (because peer set management and validator authentication still isn't in a state where we can do something more targeted).

The underlying network and discovery deficiencies are the forest for the trees on this optimization - get those working, and we won't need to use tricks like this. Unfortunately, rustc and LLVM are not smart enough to do that for us...

[mxinden]

but if I'm sending a 10MiB message to 20 peers I'd rather keep memory usage down by 5% while this is buffered

Are you suggesting that this optimization keeps the resident set size in memory low? How does it do that?

Say we send a message to 20 peers. With the un-optimized implementation the message would be cloned 20 times, thus for a short amount of time the message would exist 21 times in memory. But this 21st (useless) copy of the message is dropped right at the end of the function scope, thus this optimization only reduces the total resident set size for the duration of the function scope, which I would guess is negligible.

[rphmeier]

Well, every little bit counts. Given that we're awaiting here before dropping the last clone, there's no guarantee how long that will take.

[tomaka]

Having all methods of Network take a &mut self instead of a &self. This would enable us in the future to have a simple futures::channel::mpsc::Sender implement the Network trait.

The choice of &mut self vs &self shouldn't be based on whether or not the code compiles, but whether or not it is logically fundamentally correct to allow multiple simultaneous borrows.

If you require &mut self, you avoid possible race conditions and potentially make the API more simple, but you limit your ability to share the network in multiple places.

[coriolinus]

It looks like there's some kind of sorting precondition for live_heads; that would be good to see documented in a comment.

[rphmeier]

The ordering is that they are sorted by the order StartWork was received.

[drahnr]

Looks good as far as I can see