[Yamux] Allow max ACK backlog of 256 streams

[StefanBratanov]

https://github.com/libp2p/specs/blob/master/yamux/README.md#ack-backlog--backpressure

Implements SHOULD at most allow an ACK backlog of 256 streams.

• Added new parameter ackBacklogLimit when initializing Yamux
• Introduced notion of acknowledged when initializing yamux stream
• throw and don't create new stream if more than 256 streams are not acknowledged

[mxinden]

//CC @thomaseizinger who pushed a lot for this to land in rust-libp2p and specs.

[Nashatyrev]

Just to make sure I'm getting the Yamux spec correctly:

There could be two kinds of unacknowledged streams:

• outbound: local peer opens a stream, sends SYN, but hasn't received ACK yet
• inbound: in our current implementation we are sending back ACK immediately upon receiving a SYN from a remote party. In that case there is no way to have unacknowledged inbound streams. However the spec states the following: MAY delay acknowledging new streams until the application has received or is about to send the first DATA frame. If this statement is implemented at some point then a remotely opened stream may also be unacknowledged. I.e. until the first inbound data frame is received or a local app starts sending a data.

So the questions:

1. should we account both types as a sum and treat them equally?
2. In the case of exceeding unacknowledged streams threshold what should we do when ... opening a new stream locally? Should we throw MuxerWriteException? (i think yes) Should we also abort the connection? (probably no)
3. In the case of exceeding unacknowledged streams threshold what should we do when ... receiving SYN from remote? Abort the stream (by sending back RST)? Abort the connection?

[mxinden]

should we account both types as a sum and treat them equally?

They should be accounted separately. A should not ACK inbound streams from B, in case A's application layer has not yet read or written to a stream. B should not open a new stream to A in case 256 outbound streams to A have not yet been ACKed. Same vice versa for streams from A to B.

[mxinden]

In the case of exceeding unacknowledged streams threshold what should we do when ... opening a new stream locally? Should we throw MuxerWriteException? (i think yes) Should we also abort the connection? (probably no)

Do you have any way to backpressure the application? In Rust and I am assuming in Go as well, we suspend the green thread.

[mxinden]

In the case of exceeding unacknowledged streams threshold what should we do when ... receiving SYN from remote? Abort the stream (by sending back RST)? Abort the connection?

In rust-yamux today it is best effort only, enforced at the outbound end for now.

https://github.com/libp2p/rust-yamux/blob/93b70626f7f3d83f97951d9cd5c2951500088f0d/yamux/src/connection.rs#L435-L439

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

[Nashatyrev]

Do you have any way to backpressure the application?

Not with the present API

In Rust and I am assuming in Go as well, we suspend the green thread.

Yeah, that's a good solution.

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

If both peers has the same ACK_BACKLOG_ALLOWANCE then this situation should be treated as the protocol violation. Am I getting it right? In that case even connection abort would be appropriate

[mxinden]

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

If both peers has the same ACK_BACKLOG_ALLOWANCE then this situation should be treated as the protocol violation. Am I getting it right?

Correct. Though I am not aware of a single libp2p based network out there today that supports the yamux ack backlog feature across all nodes. Thus treating it as best effort everywhere.

[Nashatyrev]

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later? Else the PR looks good to me.

[StefanBratanov]

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later? Else the PR looks good to me.

Working on it now. Think it makes sense to be in this PR as well.

[StefanBratanov]

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later? Else the PR looks good to me.

Working on it now. Think it makes sense to be in this PR as well.

Done.