sc_network::NetworkService::write_notifications should have proper back-pressure

[tomaka]

We need an answer to the question: what happens if someone runs the code below?

for _ in 0..4000000000 {
    network_service.write_notification(some_peer, some_proto, b"hello world".to_vec());
}

Right now we would buffer up 256 of these notifications and then discard the 257th and above.
While it could be a sensible option (UDP-style), GrandPa in particular isn't robust to messages being silently dropped.

I can see a couple of solutions:

• Turn write_notification into an asynchronous function whose future is ready once the remote has accepted our message. This is the way that this problem would normally be solved, but would require changes in GrandPa.

• write_notification returns an error if the queue is full. Note that this might be difficult to implement because the queue of messages is actually in a background task, but if that's the solution I will accommodate for it.

• If the buffer is full, we drop all pending messages then report on the API that we have disconnected and reconnected. GrandPa (and others) need to account for random disconnections anyway, and by making a "full buffer" look like a disconnection, we use the same code for both. This might however cause an infinite loop if we immediately send messages as a response to a reconnection.

• Make GrandPa robust to messages being silently dropped.

• Just increase the size of the buffer if we notice that the limit gets reached.

cc @rphmeier @andresilva @mxinden

[mxinden]

• Turn write_notification into an asynchronous function whose future is ready once the remote has accepted our message. This is the way that this problem would normally be solved, but would require changes in GrandPa.

While intrusive, I am in favor of making this call asynchronous. It would allow for proper backpressure. Whenever the queue is full for a given peer, we need to make sure to reduce the reputation of the peer and eventually disconnect, as otherwise a peer could halt all of Grandpa.

• write_notification returns an error if the queue is full. Note that this might be difficult to implement because the queue of messages is actually in a background task, but if that's the solution I will accommodate for it.

This sounds like a lot more work than the suggestion above it.

• If the buffer is full, we drop all pending messages then report on the API that we have disconnected and reconnected. GrandPa (and others) need to account for random disconnections anyway, and by making a "full buffer" look like a disconnection, we use the same code for both. This might however cause an infinite loop if we immediately send messages as a response to a reconnection.

Agree to the complexity this might cause.

• Make GrandPa robust to messages being silently dropped.

From what I understand this is already partially the case. While probably a good property to have anyways, there is no feedback back to Grandpa, thus it has no clue that messages are being dropped and why messages are being dropped. Idea (1) would give that feedback to Grandpa.

• Just increase the size of the buffer if we notice that the limit gets reached.

I think this could be an interim solution until (1) is in place.

[gavofyork]

Isn't it the case that some later messages can supersede former messages? If so, then rather than spitting out these notifications and expecting them all to be delivered, maybe instead have some sort of channel system so that grandpa can express which messages still actually need to be delivered at any given time. So e.g. if you send a notification with a channel 42 and then at some time later, send another with the same channel 42, then networking won't bother trying to send the first.

[rphmeier]

Make GrandPa robust to messages being silently dropped.

This just doesn't work - the grandpa paper & the asynchronous network model can provide more information. However, it is more ok to drop messages to a single peer. The issue is when messages are completely dropped and are not delivered at all.
I'm with Max that backpressure is the right solution and we should just increase the buffer size until then.

Isn't it the case that some later messages can supersede former messages?

In some rare cases, but not for the majority of message. This would specifically apply for COMMIT messages which are fine to drop anyway.

[tomaka]

While intrusive, I am in favor of making this call asynchronous.

I'm with Max that backpressure is the right solution

Maybe I'm overestimating the difficulty of the change, but I really don't see this as easy.

You can't just do write_notification(...).await everywhere, otherwise a single non-responsive node would totally freeze GrandPa.

However I also don't personally know enough about GP's code to know what to change to properly handle that.

[rphmeier]

What timeout behavior are you thinking for write_notification calls? And if a node times out, does that mean disconnect is imminent?

Grandpa relays messages using substrate-network-gossip and doesn't do write_notification directly. awaiting on single write_notification calls is probably not a good idea as you say.

For network-gossip, the send-message API could join internally all the write-notification calls under a FuturesUnordered, Grandpa doesn't care about messages to individual peers being dropped due to shaky conection, but it does care about a message to all peers being dropped. The behavior I think we want from network-gossip is to make sure that if a message is not successfully sent to a peer, that the peer's known_messages map doesn't contain that message. And then we could look into scheduling retries within network_gossip

The only exception is for things like neighbor packets where we really want them to be delivered to specific peers before we can proceed. How we handle these failures depends on what happens to a peer when they are unresponsive.

[rphmeier]

My last comment is mostly about how to handle failures to deliver messages. Since we don't handle this right now, what we mostly care about as an incremental change is failures to buffer messages.

So the initial .await from write_notification should probably just return when the write_notification task has been successfully enqueued (in this case, I guess sucessfully sent via a bounded channel). It should be fine to turn all write_notification into write_notification.await then, as long as the corresponding Receiver is being pulled from regularly.

[tomaka]

Just to have data, here is a graph of the maximum size that the buffer reached in the past 24 hours.
Since its limit is 256, any time we reach the top of the graph we discarded some messages.

(the first peak corresponds to an era change and the second to a session change, but that's irrelevant for this issue)

[arkpar]

We need an answer to the question: what happens if someone runs the code below?

for _ in 0..4000000000 {
network_service.write_notification(some_peer, some_proto, b"hello world".to_vec());
}

My opinion is that write_notification should result in synchronous call to send on the peer socket. And return an error if the transport can't handle it. Leving the callling code with the option to handle backpressure. There's zero reason for introducing additional async buffer here. OS already has one internally.