Infer public webtransport addrs from quic-v1 addrs. #2251
Conversation
There was a problem hiding this comment.
Strongly opposed to this PR. While I appreciate the problem it’s trying to solve, I don’t think we should solve it in this way.
I’ve started early work on the address pipeline, and part of that is moving cruft out of the basic host (speaking of, can I get a review on #2248 please?). This PR adds more technical debt to the basic host, and will make implementing the address pipeline harder.
MarcoPolo
changed the base branch from
master
to
marco/fix-obsaddr-listenAddr-comparison
MarcoPolo
force-pushed
the
marco/webtransport-addrs-from-quic
branch
from
d541dce
to
7fd64c3
Compare
|
Summarizing sync convo: We both agreed the previous approach was too hacky. We also agreed there was value in having this fix out so that we can start seeing webtransport addrs in the wild. We came up with an alternate approach that makes this a pure function that is pretty well contained. When we fix this with the address pipeline, users won't notice the difference. Things will just keep working. I've updated the PR to use the less hacky approach, and added some table tests. I've also verified this surfaces my public webtransport addrs on Kubo. |
|
Note that this PR is based on #2250. We want both of these. |
| // inferWebtransportAddrsFromQuic infers more webtransport addresses from QUIC addresses. | ||
| // This is useful when we discover our public QUIC address, but haven't discovered our public WebTransport addrs. | ||
| // If we see that we are listening on the same port for QUIC and WebTransport, | ||
| // we can be pretty sure that the WebTransport addr will be reachable if the | ||
| // QUIC one is. |
There was a problem hiding this comment.
Great explanation, I wish every function we have was that well documented!
| // We need to check if we are listening on the same ip+port for QUIC and WebTransport. | ||
| // If not, there's nothing to do since we can't infer anything. | ||
|
|
||
| // Count the number of QUIC addrs, this will let us allocate just once at the beginning. |
There was a problem hiding this comment.
Not sure if the plan to reduce allocs actually works out, SplitLast also allocates pretty heavily. No need to change it though, as this is temporary anyway.
There was a problem hiding this comment.
Does it? I thought it just pointed to slices?
MarcoPolo
changed the base branch from
marco/fix-obsaddr-listenAddr-comparison
to
master
MarcoPolo
force-pushed
the
marco/webtransport-addrs-from-quic
branch
from
70b49fb
to
b63155f
Compare
This reverts commit b63155f.
MarcoPolo
force-pushed
the
marco/webtransport-addrs-from-quic
branch
from
2c5b057
to
f640d2d
Compare
[edited]
Adds a pure function to infer public webtransport addrs from quic addrs if we see that we are using the same port for both quic and webtransport addrs.
For example say we know we are listening on:
/ip4/0.0.0.0/udp/1234/quic-v1/ip4/0.0.0.0/udp/1234/quic-v1/webtransportAnd we observe that our public IP addresses include
/ip4/2.3.4.5/udp/1234/quic-v1We see that we're listening on the same address for quic-v1 and webtransport, so we should be able to say pretty confidently that we are also reachable via
/ip4/2.3.4.5/udp/1234/quic-v1/webtransport.That's what this PR does.