Resolve open Peer Starving TODO

[mpguerra]

Details

The following open TODO item from src/peer/connection.rs is noted:

zebra/zebra-network/src/peer/connection.rs

Lines 598 to 613 in a4cb835

	// CORRECTNESS
	//
	// Currently, select prefers the first future if multiple
	// futures are ready.
	//
	// The peer can starve client requests if it sends an
	// uninterrupted series of messages. But this is unlikely in
	// practice, due to network delays.
	//
	// If both futures are ready, there's no particular reason
	// to prefer one over the other.
	//
	// TODO: use `futures::select!`, which chooses a ready future
	// at random, avoiding starvation
	// (To use `select!`, we'll need to map the different
	// results to a new enum types.)

This would seem to be an important item to prioritize for resolution. While NCC Group concurs that it is unlikely in practice, it nevertheless could compound with other factors to strengthen network-level attacks. It is further noted that in the case of attackers with extremely influential positions on the network, mitigating factors such as network latency
may have less of a mitigating influence than originally expected.

It is noted that if the great majority of outstanding messages originate from attackers, then randomly chosen messages will likely be attacker-sent, and will still be prioritized over honest ones; it may be preferable to respond to requests in the order they are received, in order to ensure all requests are (eventually) answered. This may result in worse behavior in
the case where the incoming message rate exceeds the message processing rate and a backlog develops (since in this case response times would be monotonically increasing); however, the solution to this is likely not random choice but rather rate-limiting.

Resolution

• Update existing comment to specify that the current existing behaviour is what is desired

[teor2345]

This TODO is about the prioritisation of a single peer's messages:

• outbound requests from Zebra services to each individual peer
• inbound requests from the peer to Zebra

It is noted that if the great majority of outstanding messages originate from attackers, then randomly chosen messages will likely be attacker-sent, and will still be prioritized over honest ones; it may be preferable to respond to requests in the order they are received, in order to ensure all requests are (eventually) answered.

This is what we do in the existing implementation: requests from different peers are processed concurrently, multiple requests from the same peer are processed in order.

This may result in worse behavior in the case where the incoming message rate exceeds the message processing rate and a backlog develops (since in this case response times would be monotonically increasing); however, the solution to this is likely not random choice but rather rate-limiting.

In the existing implementation, if a peer does this, it will fill its individual message pipeline, and its keepalive or other
messages will time out, so Zebra will disconnect that peer.

Messages to other peers will continue to be processed concurrently, but individual services might be delayed slightly while the peer times out, if a request to that peer is sent by the service, and the service blocks until the request completes (or times out).

[teor2345]

I think the current behaviour is what we want, and we should update the TODO comment to say that.

The reverse behaviour would be worse, because a peer sending an endless stream of messages would never time out its keep alive task.

[teor2345]

There's also another reason to prefer the current implementation: If an inbound peer message arrives at a ready peer that is just being sent a request from Zebra, we want to process the peer's message first.

If we process the Zebra request first:

• we could misinterpret the inbound peer message as a response when we process it
• if we put the peer in the "awaiting response" state if the peer message is a request to Zebra, then we'll correctly ignore the simultaneous Zebra request (Zebra services make multiple requests or do retries, so this is ok)

[mpguerra]

Sounds like the fix for this is to just update the comment to remove the TODO and justify the current behaviour?

[mpguerra]

@teor2345 can you please add a size estimate for this issue?