Change sc_network::protocol::generic_proto::behaviour to occupy inbound peerset slots for substreams and not connections

[tomaka]

At the moment, receiving an incoming connection results in behaviour.rs requesting an inbound slot from the peerset.
If the peerset refuses the node, any incoming substream is immediately shut down.
If the peerset accepts the node, incoming notification substreams are expected. If no such substream arrives, the connection is killed after a 60 seconds timeout. During these 60 seconds, the peerset slot is reserved.

The problem with that scheme is that if a node connects for example only for Kademlia-related purposes and has a no intention of opening notification substreams, a peerset slot will also get reserved.

Instead, a slot should be queried from the peerset only after the remote has attempt to open notification substreams.

This change requires a lot of tweaking in group.rs as well in order for the NotifsHandler struct to signal that the remote desires the substreams to be open.
As such, this issue is blocked upon some parts of #5670. See #5670 for details about how this issue integrates with the rest of the changes.

[tomaka]

I've been trying to tackle this issue for around two weeks now, and I've been having a lot of troubles redesigning the NotifsHandlerIn and NotifsHandlerOut structs.

Opening a notifications protocol substream is done by opening a substream, sending a handshake, then waiting for the remote to accept or reject that handshake. Assuming the handshake is accepted, it is then possible to send notifications. The remote might at some point close their writing side of the substream, which is a hint that we should close the substream as soon as possible.
Substreams are unidirectional, meaning that the remote has to open its own substream in order to send notifications to us.

In other words, each substream thus has four possible states:

• Closed
• Handshaking
• Open
• OpenButCloseDesired

Each notifications protocol allows up to two substreams active at a time: one outbound and one inbound.

The state transitions that a node is allowed to perform are:

• Outbound substream: Closed -> Handshaking
• Outbound substream: Handshaking -> Closed
• Outbound substream: Open -> Closed
• Outbound substream: OpenButCloseDesired -> Closed
• Inbound substream: Handshaking -> Closed
• Inbound substream: Handshaking -> Open
• Inbound substream: Open -> OpenButCloseDesired

It should be possible to design a communication layer where the NotifsHandlerIn contains one message per possible state transition to perform, and NotifsHandlerOut one event per state transition that the remote performed.

This however looks very complicated to implement in behaviour.rs, and I've been trying to simplify the scheme to look closer to the current state (with Enabled/Disabled).

[tomaka]

I believe it should be possible to model the interactions like that:

• The behaviour can ask the handler to open the substream. The handler later responds with either Success (if accepted) or Failure (if rejected).
• The behaviour can ask the handler to close the substream. The handler responds when this is done.
• If the remote tries to open the substream, the handler notifies the behaviour. This encourages the behaviour to send either an open or close request.
• If the remote wants to close the substream, the handler notifies the behaviour as well. The behaviour acknowledges by sending a close request.

The reason why the handler needs to confirm the closing is so that the behaviour knows whether a "remote has opened the substream" message has taken into account the close message it has potentially sent earlier.

[arkpar]

Wouldn't allowing unlimited incoming TCP connections open the door for DOS attacks?
There should be a limit on how many incoming TCP connections are allowed, regardless of the purpose.

[tomaka]

Yes, we need a hard limit on the number of incoming TCP connections.
Unfortunately, whatever we do, it will always be possible for an attacker to fill all the inbound connections or slots of a node. All we can ensure is that the node doesn't crash (because of memory or ulimits).

This issue is mostly about making sure that TCP connections used only for discovery or for collation don't use the peerset slots that are supposed to be only for relay chain full nodes.