rcmgr: Connections and Streams should also reserve memory

[MarcoPolo]

What

Reserving a connection/stream should reserve a roughly accurate amount of memory.

Why

This lets connections/streams still be limited by the memory limits. This enables a use case where a user may only want to think in terms of FDs and memory. They can set connections/streams to unlimited and only limit FDs and memory.

Why in go-libp2p

... as opposed to folks wrapping the resource manager.

go-libp2p has the most context to give a good approximation for the resource cost of a connection for each transport. e.g. a QUIC connection won't consume a FD but a TCP connection will.

Kubo folks (especially @ajnavarro) brought this up as helpful simplification of the knobs in the resource manager.

[marten-seemann]

We kind of do this already, since the muxer applied on top of the connection will reserve memory. And in the case of QUIC, the QUIC transport takes care of this.

I'm not sure how useful memory tracking actually is though, except for muxers, so https://github.com/libp2p/go-libp2p/issues/1708 might be an alternative.

[MarcoPolo]

We kind of do this already, since the muxer applied on top of the connection will reserve memory.

Yeah. I think reserving some memory for the connection itself also makes sense. This way a connection (even if it doesn't have any streams) still has a weight to it.

I'm not sure how useful memory tracking actually is though

It's probably the most useful thing to track. The thing that will kill our process is running out of memory. I'd argue that limiting connections and streams is just a proxy for limiting memory (and CPU time).

It also simpler to think about just memory (and FDs) than to think about connection counts and streams at the various levels.

[BigLep]

@BigLep has been in Kubo standup and go-libp2p triage. Here's the Kubo perspective that I want to address ASAP:

Protect against a script-kiddie being able to easily take down a Kubo node by simply opening many connections or stream.

The way I'd like to accomplish this is with default configuration in https://github.com/ipfs/kubo/pull/9587 . We can discuss specifics there.

In order for this work, established (non-transient) connections will need register memory usage with the resource manager/accountant, ideally across all transports and muxers. If an established libp2p connection doesn't increase memory accounted for by the resource manager/accountant, then I believe the work in this issue will be needed. I'm trying to avoid needing to go-libp2p code changes.

[marten-seemann]

In order for this work, established (non-transient) connections will need register memory usage with the resource manager/accountant, ideally across all transports and muxers. If an established libp2p connection doesn't increase memory accounted for by the resource manager/accountant, then I believe the work in this issue will be needed. I'm trying to avoid needing to go-libp2p code changes.

It makes sense to reserve some amount of memory (1 MB maybe?) per connection, once a connection has been accepted by the swarm. This would be justifiable since a connection does impose a certain overhead (storing it in the swarm, tracking addresses and protocols in the peer store etc.). This amount of memory would be independent of (and in addition to) the memory reserved by the stream multiplexer. We might also want to consider doing the same for every stream, with a much smaller amount (1 kB?).

Memory reservations should use different priorities for incoming and outgoing connections / streams, otherwise an attacker will be able to saturate all available memory for incoming connections, preventing the node from dialing new outgoing connections.

[MarcoPolo]

Thanks Marten. I agree with all those points.

To answer Steve's question of "Can I use this config today safely?". We take into account the memory usage of streams already.

I believe all but tcp+yamux connections reserve memory already as a byproduct of the above. TCP+Yamux connections will reserve memory once streams are opened. If there are no streams, these connections are still bounded by file descriptor limits.

Therefore, I think that config is safe to use. However I'm still for making this change. I just don't think it's a blocker.

[BigLep]

Cool. I think we can do this in Kubo for now and let it roll out in go-libp2p naturally. I updated https://github.com/ipfs/kubo/pull/9587 with the intent of setting System.ConnsInbound to equal 1 inbound connection per MB so we're protected from the TCP+Yamux connection case. That can be removed when go-libp2p does more memory accounting.