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

quic: QUIC listener doesn't respect the QUIC version of the listen address #1904

Closed
Tracked by #1766
marten-seemann opened this issue Nov 19, 2022 · 4 comments · Fixed by #1923
Closed
Tracked by #1766

quic: QUIC listener doesn't respect the QUIC version of the listen address #1904

marten-seemann opened this issue Nov 19, 2022 · 4 comments · Fixed by #1923
Assignees
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP

Comments

@marten-seemann
Copy link
Contributor

func TestListenOnlyOnOneVersion(t *testing.T) {
	t.Run("only one draft-29", func(t *testing.T) {
		h1, err := libp2p.New(
			libp2p.Transport(libp2pquic.NewTransport),
			libp2p.ListenAddrStrings(
				"/ip4/127.0.0.1/udp/12345/quic", // QUIC draft-29
			),
		)
		require.NoError(t, err)
		defer h1.Close()

		addrs := h1.Addrs()
		require.Len(t, addrs, 1)
		require.Equal(t, ma.P_QUIC, getQUICMultiaddrCode(addrs[0]))
	})
}

This test fails, as the listener is listening on both versions.

@marten-seemann marten-seemann added kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP labels Nov 19, 2022
@marten-seemann marten-seemann mentioned this issue Nov 19, 2022
34 tasks
@marten-seemann
Copy link
Contributor Author

I found this when adding an integration test for QUIC version support as part of #1905. The test above should be added as an integration test.
It probably makes the most sense to work on a fix for this on top of #1905.

@MarcoPolo
Copy link
Collaborator

I agree this is a bug. I think we can fix this in a patch release.

@MarcoPolo
Copy link
Collaborator

On more discussion. This shouldn't be a patch release because it will change the behavior of the public api

@MarcoPolo
Copy link
Collaborator

As I started fixing this I realized that I was writing special logic in the swarm to handle listeners returning multiple (possibly changing over time) multiaddrs. This felt error-prone and like a leaky abstraction. I'm thinking now that we shouldn't have Listeners return multiple multiaddrs. This leaks the implementation detail of us using the same listener for multiple multiaddrs to the rest of the stack. This problem is highlighted in this issue because we want the Listener to only return one multiaddr (quic-draft29) until we ask the transport to give us another listener for the other multiaddr (quic-v1).

I also think this is a similar problem to the quic conn-reuse across the quic transport and WebTransport. And in that case we didn't (and shouldn't) have the quic Listener also return the webtransport multiaddr.

Because this involves potentially undoing the breaking API change we made here: #1881 that isn't released yet this is blocking issue for this next release. If we aren't sure we want this breaking change, we should avoid it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP
Projects
None yet
2 participants