Bad Edge Case in Recovery Draft

[martinduke]

In OnLossDetectionTimeout():

if (crypto packets are outstanding):
// Crypto retransmission timeout.
RetransmitUnackedCryptoData()
crypto_count++

The literal interpretation is a little loopy. In the case where crypto data is retransmitted and then the acks arrive for the original packets, we have outstanding crypto packets but no unacked crypto data. This pseudocode will result in sending nothing, and then (not shown) starting the Loss Detection timer even though there is nothing in flight, which seems bad.

Two improvements to make here:

1. If the there is no unacked crypto data, it would be better to send any available 1-RTT data.
2. We should only start the loss detection timer if there is something to send.

[ianswett]

Thanks for writing this up, I think this points out that the crypto timer should be enabled when there's crypto data to send, not when crypto packets are in flight.

However, there's an exception to that, which is if there's no crypto data to send on the client side because all Initial packets have been acked, but the client didn't receive the server's Handshake, then you can hit the Anti-amplification attack limit, as you discovered in New York.

This might be awkward to writeup, but I'll give it a try, unless you'd like to?

[martinduke]

Well, in the scenario above the sender sets the crypto timer when there is still unacked data. One could change OnPacketAcked() to cancel the LD timer if there's nothing left in the send queues, I suppose. But I would recommend exactly the two changes listed above.

Yeah the amplification thing is gross.

I am not sure when I'll have time to generate a PR., so please feel free to give it a crack.

[ianswett]

This was fixed by #2590