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

fix: reuse the quic port in webtransport #2935

Closed
wants to merge 1 commit into from
Closed

Conversation

2color
Copy link
Contributor

@2color 2color commented Aug 22, 2024

This sets the ListenOrder for WebTransport so that it starts listening after QUIC, thereby allowing port reuse when not listening on an explicit port.

Related bugs

Related to #2913 and fixes a bug whereby a libp2p peer doesn't register an observed public IP, because the outgoing connection uses the WebTransport port which is different to the QUIC port.

@2color 2color requested a review from sukunrt August 22, 2024 14:20
@sukunrt
Copy link
Member

sukunrt commented Aug 22, 2024

We don't need this change. The current behavior of using different ports when we listen on

/ip4/0.0.0.0/udp/0/quic-v1
/ip4/0.0.0.0/udp/0/quic-v1/webtransport

is the expected behavior and also one that on changing might break existing users.

To reuse the same port, either pass a port explicitly or listen on port 0 for quic first and then listen on webtransport with the port allocated to the quic listener.

@sukunrt sukunrt requested a review from MarcoPolo August 22, 2024 15:06
@2color
Copy link
Contributor Author

2color commented Aug 22, 2024

Fair enough.

If we allow WebTransport and QUIC to be allocated different ports, we still need to fix the bug whereby go-libp2p fails to correctly verify an observed ip as observed in this comment, because the request goes out from the WebTransport port which then doesn't match the QUIC one (as we also noticed in our testing @sukunrt)

Any thoughts on that? Should we open a separate issue for that?

@MarcoPolo
Copy link
Collaborator

we still need to fix the bug whereby go-libp2p fails to correctly verify an observed ip as observed in this comment, because the request goes out from the WebTransport port which then doesn't match the QUIC one

Fixed in #2936

@2color
Copy link
Contributor Author

2color commented Aug 23, 2024

To reuse the same port, either pass a port explicitly or listen on port 0 for quic first and then listen on webtransport with the port allocated to the quic listener.

Naive question, if this change is not necessary, why do we define ListenOrder for the WebRTC transport?

func (t *WebRTCTransport) ListenOrder() int {
return libp2pquic.ListenOrder + 1 // We want to listen after QUIC listens so we can possibly reuse the same port.
}

In other words, isn't this PR required for situations when you explicitly use the same port for the WebTransport and QUIC listeners?

@sukunrt
Copy link
Member

sukunrt commented Aug 23, 2024

Naive question, if this change is not necessary, why do we define ListenOrder for the WebRTC transport?

You can do (webtransport followed by quic)

swarm.Listen(ma.StringCast("/ip4/0.0.0.0/udp/4001/quic-v1/webtransport")
swarm.Listen(ma.StringCast("/ip4/0.0.0.0/udp/4001/quic-v1")

But you cannot do (webrtc-direct followed by quic):

swarm.Listen(ma.StringCast("/ip4/0.0.0.0/udp/4001/webrtc-direct")
swarm.Listen(ma.StringCast("/ip4/0.0.0.0/udp/4001/quic-v1")

This is to ensure that the udp socket used for port 4001 is correctly shared between webrtc-direct and quic.
When listening on webrtc-direct before quic we assume that there wont be any quic listener for the same port and give the socket ownership to the webrtc transport. So if you listen on quic after that, it will error.

This is not a problem for webtransport because it's easy to share ownership there since it's all using quic.Transport (quic-go).

@2color
Copy link
Contributor Author

2color commented Aug 23, 2024

Got it. So this can be closed?

@MarcoPolo
Copy link
Collaborator

is the expected behavior and also one that on changing might break existing users.

One more thought on this. I think we can change this, but we should just do it deliberately through an option this is default off, and at some point default on. (like all breaking changes).

@MarcoPolo MarcoPolo closed this Aug 23, 2024
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.

3 participants