Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infer public webtransport addrs from quic-v1 addrs. #2251

Merged
merged 7 commits into from
Apr 12, 2023

Conversation

MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented Apr 11, 2023

[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/webtransport

And we observe that our public IP addresses include

  • /ip4/2.3.4.5/udp/1234/quic-v1

We 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.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 MarcoPolo changed the base branch from master to marco/fix-obsaddr-listenAddr-comparison April 11, 2023 15:51
@MarcoPolo MarcoPolo force-pushed the marco/webtransport-addrs-from-quic branch from d541dce to 7fd64c3 Compare April 11, 2023 18:08
@MarcoPolo
Copy link
Collaborator Author

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.

@MarcoPolo MarcoPolo marked this pull request as ready for review April 11, 2023 18:18
@MarcoPolo
Copy link
Collaborator Author

Note that this PR is based on #2250. We want both of these.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling a bit to wrap my head around the logic here. I think there's a bug here if the input contains duplicates.

By the way, the code would be sooo much cleaner with the new multiaddr API.

Comment on lines +1008 to +1021
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it? I thought it just pointed to slices?

p2p/host/basic/basic_host.go Show resolved Hide resolved
p2p/host/basic/basic_host.go Show resolved Hide resolved
@MarcoPolo MarcoPolo changed the base branch from marco/fix-obsaddr-listenAddr-comparison to master April 12, 2023 15:18
@MarcoPolo MarcoPolo force-pushed the marco/webtransport-addrs-from-quic branch from 70b49fb to b63155f Compare April 12, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants