How to handle backpressure for inbound substreams?

[rkuhn]

Since the implementation of #2796 outbound substreams may fail to negotiate due to an excess of inbound substreams on the remote end. Unfortunately the signal that I see in libp2p-bitswap is only this:

Machine-#0  WARN libp2p_yamux: dropping (Stream 9be48fe7/172) because buffer is full
Machine-#1 DEBUG libp2p_bitswap::behaviour: bitswap outbound failure 12D3KooWDpJ7As7BWAwRMfu1VU2WCqNjvq387JEYKDBj4kx6nXTN 63 UnsupportedProtocols
Machine-#1 TRACE ipfs_embed::net: sync progress: Ready(Some(Complete(Err(Failed to retrieve block bafyr4ihgonob4lcjjeapoafaiyly5bxzhkullpfgruybprgpctf4zqwyli.))))

It would be great to get a better diagnostic than “UnsupportedProtocols”. But even so, it is unclear to me how to correctly implement backpressure in this environment:

• how many requests may I send?
• when can I send more?
• what is my substream quota in the context of a larger network behaviour?

My current impression — hopefully misguided — is that I need to do something akin to reimplementing TCP on top of UDP. Until then I’ll probably have to set the inbound substream limit to an extremely high value.

[thomaseizinger]

Realistically, what you need is a muxer that allows back-pressure on the number of streams. QUIC can do that for example or qmux.

In the absence of a back-pressure mechanism (yamux doesn't have one), we can only drop incoming streams.

There are two ways to deal with a full queue: drop packets, or throttle incoming packets (backpressure).

Source: https://apenwarr.ca/log/20170814

I am not sure if it is possible to implement a backwards-compatible mechanism to signal that we are intentionally dropping the stream.

As part of #3014 I want to change yamux such that we can observe a stream reset without having written a single byte to the stream. That would cause the stream to be aborted with ConnectionReset which we could use as a heuristic that we are hammering the node too much.

[thomaseizinger]

I just had an idea. I think we can extend rust-yamux with a configuration option for the maximum number of not ACK'ed streams. That is backwards compatible with the spec and allows an application to not flood another peer with new streams if that one is slow in ACK-ing them.

I know how to do that on top of libp2p/rust-yamux#142 once the Control API is gone:

Assuming a poll_outbound API as we have it in StreamMuxer today, we can keep a record of opened and ACK'ed streams and if the difference hits the configured limit, we register a waker and wake that one on the next ACK of an already opened stream.

[rkuhn]

Given that currently deployed libp2p software cannot recognise this type of backpressure, my assessment is that this change broke network compatibility and needs to be reverted. Adding backpressure is a worthy goal, but it needs to be done such that existing peers expect it and can handle it. Since yamux has no reliable tools for this task, my proposal would be to

• revert this change in yamux
• pick one (or more) new muxers that support backpressure
• use them in libp2p in a fashion that has proper APIs and/or backpressure behaviour from day 1 of their usage
• describe an upgrade path for existing software to run both yamux and the new muxer in parallel, with backpressure only working between new peers

/cc @mxinden

[thomaseizinger]

Damn, that breaking change was not anticipated. For reference, it happened as part of #2861.

I'd like to avoid reverting the entire PR. @mxinden would it be acceptable to remove the buffer bound here until we have a better solution? Existing deployments rely on the unbounded buffer which is unfortunate but the reality.

I'd propose to:

• Remove the bound on the buffer
• Put a fat TODO next to it that we need to fix this
• Open an issue to track it
• Continue working on rust-yamux to see how we can deploy a version that can enforce some amount of back-pressure.

[rkuhn]

Yes, this would unblock me, thanks! I’ll be happy to contribute to the design and implementation of better back-pressure schemes — sorry for not recognising the issue during PR review (mostly because effecting some active behaviour using Drop is a rather oblique path that requires a lot of contextual knowledge to understand).

[mxinden]

Thanks @rkuhn for tracing this all the way back. Sorry for this not being easier to discover.

First off, I would like to differentiate two types of inbound streams:

1. Negotiating inbound streams
   • Inbound streams that have not yet read or written from/to
   • Inbound streams that currently run multistream-select
   • Inbound streams that currently run the UpgradeInbound provided by the ConnectionHandler
2. Negotiated inbound streams

Also, just to avoid confusion, all the limits we are talking about here are per connection, i.e. between two endpoints. Not for all connections of a given peer.

Since the implementation of #2796 outbound substreams may fail to negotiate due to an excess of inbound substreams on the remote end.

The limit on inbound negotiating streams was introduced with #2697. Without this limit a remote can open a large amount of streams towards the local node. Given that the local node allocates some memory for each of these streams, the local node eventually runs out of memory.

I would not refer to this mechanism as a backpressure mechanism, but as a last-resort DOS prevention mechanism.

#2796 and #2861 did not introduce this limit, but instead moved this limit. Instead of dropping inbound streams within the HandlerWrapper or Conection, we now no longer poll the stream muxer for new inbound streams once the limit is reached. Thus we are forcing the stream muxer to eventually drop the streams (or backpressure but none of the existing muxers support that today).

Given that Yamux breaks the backpressure chain, i.e. does not communicate the limit to the remote but drops streams, we don't actually enforce backpressure with Yamux. #2796 does pave the way for proper backpressure on streams for QUIC and the future qmux.

I just had an idea. I think we can extend rust-yamux with a configuration option for the maximum number of not ACK'ed streams. That is backwards compatible with the spec and allows an application to not flood another peer with new streams if that one is slow in ACK-ing them.

First we would have to agree on a magic number. Second this would need to be rolled out to a live network in two steps, given that these limits are never communicated in-band. First this needs to be rolled out for the stream outbound side and then the stream inbound side. I think it makes more sense for us to invest in QUIC and sometime in the future into qmux.

Given that currently deployed libp2p software cannot recognise this type of backpressure, my assessment is that this change broke network compatibility and needs to be reverted.

Note that you can set a very high limit and thus achieve the old behavior (SwarmBuilder::max_negotiating_inbound_streams). Note however that you are prone to the above DOS attack.

My suggestion moving forward here:

• In case your environment (trusted) allows, @rkuhn you set a high limit.
• In your bitswap implementation you set a reasonable limit on the number of outbound streams. A guiding factor can be the amount of parallelism you expect any remote can handle. Long term, I would really like us to own this part, see protocols/bitswap: Add BitSwap implementation #2632.
• We roll out QUIC and make sure backpressure on the number of inbound streams works as expected (//CC @elenaf9).
• We introduce a mechanism to ConnectionHandler that enforces outbound stream backpressure. Today a ConnectionHandler can emit as many ConnectionHandlerEvent::OutboundSubstreamRequest as they like.

Yes, this would unblock me, thanks! I’ll be happy to contribute to the design and implementation of better back-pressure schemes 

I am very eager to take you up on this offer. I think we can learn a lot from your experience here. If you are up for it, I would suggest to start with a call with @thomaseizinger and me. I will try to schedule something out-of-band.

[rkuhn]

• In case your environment (trusted) allows, @rkuhn you set a high limit.

Yes, that’ll be my path once #3041 is resolved (because the limit I’m currently hitting is not configurable).

• In your bitswap implementation … . Long term, I would really like us to own this part …

Yes, I do think that bitswap should receive some tender loving care. I am hesitant to construct workarounds, though, since I think the back-pressure aware core API will require a rather different approach (like new substream requests being pulled from bitswap only when there is room to send them).

---

After some brainstorming we should create discussions and issues here as we gain more clarity on how to proceed in the big picture.

[thomaseizinger]

• In case your environment (trusted) allows, @rkuhn you set a high limit.

Yes, that’ll be my path once #3041 is resolved (because the limit I’m currently hitting is not configurable).

As a hotfix, we can expose a configuration option for this when constructing the yamux multiplexer. Thoughts on that @mxinden @rkuhn ? We can make a patch release for that so it is immediately available.