quic: prioritise listen connections for reuse

[sukunrt]

Currently there is a race in kubo between the first dial made by a node on startup and starting to listen on a quic address.

If the dial happens first, this places an entry in the reuse.global map. This entry will stay in the map till we get a period of 30 seconds where we make no new dials. In my experiments this entry is removed after 15 minutes of starting the node. This dial happens before listening 5-10% of the times on my machine.

The impact of this is on holepunching. While this entry is in the map, the function reuse.Dial in p2p/transport/quicreuse/reuse.go can return either this connection that we dialed out of but are not listening on, or it can return the correct connection the one on which we are listening.

[marten-seemann]

Does this PR fix #1534?

[marten-seemann]

What happens in the following scenario?

1. We dial a QUIC connection to a remote node. We don't have any existing connections, so we listen on :0, and get assinged :port1.
2. Now we start a listener on 0.0.0.0:port2.

With this PR, we'd ignore port2, and just reuse the connection that's listening on port1, right?

[sukunrt]

We dial a QUIC connection to a remote node. We don't have any existing connections, so we listen on :0, and get assinged :port1.

The :port1 will go in to globalDial globalFallback map

Now we start a listener on 0.0.0.0:port2.

The :port2 will go in to global map

With this PR, we'd ignore port2, and just reuse the connection that's listening on port1, right?

No, we will prioritise :port2 because it is in the global map which is checked before globalDial globalFallback map

I added a test case for this. 8efdc6a (#2262)

[sukunrt]

Does this PR fix #1534?

I think so. comments below

Ideally, we'd mark this as a fallback connection so we can stop using it when we get a real global connection.
libp2p/go-libp2p-quic-transport#63 (comment)

Yes this is exactly what i've implemented. The fallback map is globalDial globalFallback

The listening part of this is still a bit racy, if the user decides to listen on an ephemeral port that was picked for a Dial call before. We should reuse connections when Listen is called after Dial (and the port happens to be the same) as well. Planning to fix this in a separate PR.

In the original description

This is fixed by this PR. First we check globalDial globalFallback to see if there's a connection. If there is we reuse that for listening.

Update:
have renamed globalDial to globalFallback. It's a more suggestive name

[marten-seemann]

I've tried this out using the following code: https://gist.github.com/marten-seemann/c221eed357fc14e15f26d066380c5c44

It seems to work! Just left some comments regarding naming, and a question regarding listening on port 0.

[marten-seemann]

Do we need to handle listening on port 0?

[sukunrt]

I don't think so. An entry in the map(listeners or dialers) will never have port 0 because we add the finally assigned listener port to the map and not 0. So if a listen call happens with port 0 and there is an entry in globalDialers map, we will not find a match and then net.ListenUDP will give use a new port which is not in the globalDialers map.

Do you think this makes sense?

[marten-seemann]

So if a listen call happens with port 0 and there is an entry in globalDialers map, we will not find a match and then net.ListenUDP will give use a new port which is not in the globalDialers map.

That's what I meant. If the port is 0, the user doesn't care which port is assigned. So we might as well reuse a connection that we used before, right?

[sukunrt]

oh, that's a nice idea. will fix.

[sukunrt]

fixed this. updated the test TestReuseConnectionWhenDialBeforeListen

[marten-seemann]

Logic LGTM. Just suggesting to add another comment, to make the code easier to understand when we come back to it later.