swarm: fix selection of transport for dialing #1653

marten-seemann · 2022-07-10T14:08:46Z

With WebTransport's /webtransport/certhash/xyz addresses, the assumption that the last component of a multiaddr identifies the transport to use for dialing doesn't hold any more.

Note that WebRTC will probably also use the certhash multiaddr component to encode its certificate hashes.

This is a draft until we actually move the webtransport transport into this repo (#1737), but I'm asking for reviews nonetheless. Please review carefully, this has the potential to break us in painful ways.

marten-seemann · 2022-07-10T14:10:44Z

p2p/net/swarm/swarm_addr_test.go

@@ -57,3 +67,34 @@ func TestAddressesWithoutListening(t *testing.T) {
 	require.NoError(t, err)
 	require.Empty(t, a1, "expected to be listening on no addresses")
 }
+
+func TestDialAddressSelection(t *testing.T) {


I'm slightly concerned about the amount of dependencies that this test adds. Not that much of a problem, now that we have a mono-repo, but maybe this would be nicer if we used a mock transport.Transport interface?

marten-seemann · 2022-07-10T14:11:29Z

p2p/net/swarm/swarm_transport.go

-		}
-		if transport.Proxy() {
-			return transport
+	if isRelayAddr(a) {


I don't really like this logic here. Instead, we could have transports reject /p2p-circuit address. wdyt?

Let's not change this just yet since that would involve changing every transport. Let's revisit after discussing multiformats/multiaddr#140

marten-seemann · 2022-07-10T15:04:59Z

Side node: We might want to introduce a CanListen for transports, to avoid relying on the same assumption.

With WebTransport's /webtransport/certhash/xyz addresses, the assumption that the last component of a multiaddr identifies the transport to use for dialing doesn't hold any more. Note that WebRTC will probably also use the certhash multiaddr component to encode its certificate hashes.

marten-seemann · 2022-09-07T15:23:44Z

Rebased the PR. This should be ready for review now.

MarcoPolo

I think this logic should change a bit after we discuss the multiaddr interpretation strategy. But at the very least I think we should be traversing the multiaddr backwards when looking for a transport. i.e. we don't want quic to match before webtransport in .../quic/webtransport.

MarcoPolo · 2022-09-07T16:30:15Z

p2p/net/swarm/swarm_transport.go

+	if isRelayAddr(a) {
+		return s.transports.m[ma.P_CIRCUIT]
+	}
+	for _, t := range s.transports.m {


I think in general this needs to go the other way. We should be interpreting this right to left.

I don't understand this comment. We're iterating over a map[int]transport.Transport here. Inverting the order of iteration shouldn't have any effect, since map iterations are (pseudo-) randomized anyway.

I misunderstood. I thought we were iterating of the multiaddr components. I see now that we're iterating over all registered transports.

I think this isn't ideal because:

we have to make sure every registered transport doesn't intersect with any other transport. It would be tricky behavior if two transports return CanDial -> true for the same a since we would randomly pick one of the two. I think simply a guaranteed ordering here would make it better.

This means the transport owns the interpretation of the full multiaddr. This doesn't compose well. Imagine we have a percentencode component and a lenprefixencode component, we don't want every transport to have to know how to read the encoded data from the multiaddr. Instead they should be given the decoded data from something else and deal with their part of the multiaddr. This relates to Interpreting multiaddrs multiformats/multiaddr#140.

I'll draft a patch of what I'm thinking here and how it relates to multiformats/multiaddr#140

Here's concretely what I mean: https://github.com/libp2p/go-libp2p/compare/fix-dial-transport-selection...marco/layered-transports?expand=1#diff-1d9e588fd7c4fedee40f7f985be2346b02ba540389251f4603cbc13e8f8fc371R94

Notice this gives us the benefit of removing the "extract certhash" logic out of webtransport completely both in the Dial and the CanDial method (I didn't actually do this refactor, since this is a demo).

MarcoPolo · 2022-09-07T16:49:56Z

p2p/net/swarm/swarm_transport.go

 		}
 	}
-
-	return s.transports.m[protocols[len(protocols)-1].Code]


What if we have a TransportForComponent function that takes the component and the rest of the multiaddr (and maybe the list of registered transports) and returns the transport to use. This fits with the encapsulation idea of multiaddrs.

This function could be a method of an interface and components could implement this interface. The certhash component would simply return the next transport in the multiaddr along with the certhash parameters.

That would be a layer violation, wouldn't it?
Components are a multiaddr concept. Multiaddr doesn't know anything about transports.

marten-seemann · 2022-09-08T11:28:13Z

I think this logic should change a bit after we discuss the multiaddr interpretation strategy. But at the very least I think we should be traversing the multiaddr backwards when looking for a transport.

I agree that this is a mess and should be changed, but can we focus on getting something landed to unblock WebTransport for v0.23.0?
I'm not really sure what a concrete proposal would look like. Interpreting everything right-to-left won't solve our problem, since we still have /p2p-circuit to deal with.

MarcoPolo · 2022-09-14T21:55:38Z

I agree that this is a mess and should be changed, but can we focus on getting something landed to unblock WebTransport for v0.23.0?

Ok. Then maybe it's worth asking if anything here will make things worse than than they already are, or is there something that will make backwards compatibility harder? I don't think so. This has the same behavior as before if no two transports return candial=true for any given multiaddr (if this wasn't true, this would introduce a non-deterministic bug).

I'm not really sure what a concrete proposal would look like.

I hope my patch clarifies this, but let me know if things are still unclear.

we still have /p2p-circuit to deal with.

Yeah, this is the one multiaddr format that doesn't match up with how the rest work. We can still deal with it following the same principles but it would involve some changes (see multiformats/multiaddr#140). Right now we deal with this with a special case. We could keep doing that too, but it would be better if we can unify the interpretation of multiaddrs.

MarcoPolo

I don't think this changes the old behavior, and it unblocks webtransport. This code needs refactoring but I don't want to block webtransport. Let's revisit after discussing multiformats/multiaddr#140

MarcoPolo · 2022-09-14T21:56:35Z

p2p/net/swarm/swarm_transport.go

-		}
-		if transport.Proxy() {
-			return transport
+	if isRelayAddr(a) {


Let's not change this just yet since that would involve changing every transport. Let's revisit after discussing multiformats/multiaddr#140

marten-seemann · 2022-09-16T05:52:39Z

Merging this PR, with the understanding that this will get refactored very soon.

marten-seemann requested review from Stebalien and MarcoPolo July 10, 2022 14:08

marten-seemann commented Jul 10, 2022

View reviewed changes

marten-seemann requested a review from vyzo July 10, 2022 14:10

marten-seemann commented Jul 10, 2022

View reviewed changes

marten-seemann mentioned this pull request Aug 23, 2022

[Tracking Issue] Experimental WebTransport Transport #1717

Closed

10 tasks

marten-seemann force-pushed the fix-dial-transport-selection branch from b1219fd to bc8df9a Compare September 7, 2022 15:23

marten-seemann marked this pull request as ready for review September 7, 2022 15:23

MarcoPolo requested changes Sep 7, 2022

View reviewed changes

MarcoPolo approved these changes Sep 14, 2022

View reviewed changes

marten-seemann merged commit 8b7d151 into master Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm: fix selection of transport for dialing #1653

swarm: fix selection of transport for dialing #1653

marten-seemann commented Jul 10, 2022 •

edited

Loading

marten-seemann Jul 10, 2022

marten-seemann Jul 10, 2022

MarcoPolo Sep 14, 2022

marten-seemann commented Jul 10, 2022

marten-seemann commented Sep 7, 2022

MarcoPolo left a comment

MarcoPolo Sep 7, 2022

marten-seemann Sep 8, 2022

MarcoPolo Sep 12, 2022

MarcoPolo Sep 12, 2022

MarcoPolo Sep 7, 2022

marten-seemann Sep 8, 2022

marten-seemann commented Sep 8, 2022

MarcoPolo commented Sep 14, 2022

MarcoPolo left a comment

MarcoPolo Sep 14, 2022

marten-seemann commented Sep 16, 2022

swarm: fix selection of transport for dialing #1653

swarm: fix selection of transport for dialing #1653

Conversation

marten-seemann commented Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Jul 10, 2022

marten-seemann commented Sep 7, 2022

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Sep 8, 2022

MarcoPolo commented Sep 14, 2022

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Sep 16, 2022

marten-seemann commented Jul 10, 2022 •

edited

Loading