Rank Dial addresses #212

aarshkshah1992 · 2020-05-05T14:29:43Z

A dial address is FD consuming when:
- For non-relay addresses, the transport is TCP or WebSocket.
- For relay addresses, the transport of the Relay Server is TCP or Websocket.
The limiter ONLY consumes FD's for non-relay addresses to avoid double counting.
For dialling:
- We partition the addresses into FD consuming and Non-FD consuming addresses.
- We then sort each of two sets in descending order of priority such that private addresses have the highest priority followed by non-relay public addresses followed by relay addresses.
We first attempt to dial all the sorted Non-FD consuming addresses and ONLY after/if we exhaust all of them without getting a connection, we attempt to dial the sorted FD consuming addresses.

swarm_dial.go

Stebalien · 2020-05-06T06:23:25Z

swarm_dial.go

+	var errNonFd *DialError
+	var dialErr *DialError
+
+	connC, errNonFd = s.dialAddrs(ctx, p, addrsToChanFnc(nonFdAddrs))


We can just get rid of the channel on this interface to simplify it.

Agreed. This is done.

Stebalien · 2020-05-06T06:26:50Z

swarm_dial.go

+		var errFd *DialError
+		connC, errFd = s.dialAddrs(ctx, p, addrsToChanFnc(fdAddrs))
+		if errFd != nil {
+			dialErr = &DialError{Peer: p, DialErrors: append(errNonFd.DialErrors, errFd.DialErrors...),


We should drop additional errors if we're over the maxDialDialErrors limit (not sure what I did with that variable name). We might want to just add a "combine" method to the DialError type.

This is done.

Stebalien · 2020-05-06T06:29:00Z

swarm_dial.go

+	rankAddrsFnc := func(addrs []ma.Multiaddr) []ma.Multiaddr {
+		var localAddrs []ma.Multiaddr
+		var relayAddrs []ma.Multiaddr
+		var others []ma.Multiaddr


I'd pre-allocate and over-allocate (i.e., with length len(addrs)) all of these.

Note: as the rules here grow more complicated, it may be simpler to use sort.Slice().

(can be changed later if necessary)

@Stebalien Using sort.Slice now and got rid of the allocs.

swarm_dial.go

Stebalien · 2020-05-06T06:35:40Z

swarm_dial.go

+			}
+		}
+		return false
+	}


Hm. I take back what I said before. Let's just simplify this:

If the transport is a "proxy" transport, don't consume a file descriptor token.

Otherwise, just call addrutil.IsFDCostlyTransport.

@Stebalien Please see above comment.

For dialing in the limiter, we do NOT consume fd's for proxy addrs.
For classifying an address as FD or Non-FD consuming so we can dial all the latter ones before attempting the former, we look at the address of the relay server.

Ah, I see. You're right, we do need to do that.

In that case, I'd still simplify this (given that we only really support circuit addresses anyways):

Modify addrutil.IsFDCostlyTransport to ignore circuit addresses.

When sorting between relay connections when dialing, split on the P_CIRCUIT protocol and call addrutil.IsFDCostlyTransport on the relay part.

That way:

We don't need to look at the swarm's transports. You're right in that we should probably expose this information on the transport itself, but we don't do that yet anyways.

Can just go "full special case" for circuit addresses.

Does it sound like that will simplify this?

@aarshkshah1992 thoughts on this?

@Stebalien I'd replied earlier at #212 (comment).

Reproducing it here:

"We shouldn't leak how our dial uses the addrutil.IsFDCostlyTransport utility into the
shared utility itself. I've documented the code better for now and have filed an issue(#214 ) to simplify/fix this later.
".

I still think this code needs to be simpler:

We're checking to see if the address is a "proxy" address, but we only actually support circuit addresses. We should just split on the circuit address and be done with it.

We're hard coding rules for specific transport protocols when we should be able to just say "if protocol X appears in the target multiaddr (before any proxy protocols), the address must consume a file descriptor". That is, we know that the tcp and unix protocols both must consume a file descriptor per connection while udp, memory, etc. protocols don't.

Basically, this code is trying to be general purpose and abstract over transports, but we can't actually do that so it's hard coding a bunch of stuff. Sitting halfway in between is just confusing.

aarshkshah1992 · 2020-05-07T09:12:52Z

@Stebalien Have addressed your review with some open comments. Please take a look.

Stebalien · 2020-05-07T21:37:23Z

dial_error.go

+	}
+
+	return cbd
+}


We should be able to do this with two allocations (slice + error), two appends, some some slicing, and no for loops.

@Stebalien Turns out we need only one alloc. This is done.

swarm_dial.go

Stebalien · 2020-05-07T21:44:54Z

swarm_dial.go

+			}
+		}
+		return false
+	}


Ah, I see. You're right, we do need to do that.

Stebalien · 2020-05-07T21:53:26Z

swarm_dial.go

+			}
+		}
+		return false
+	}


In that case, I'd still simplify this (given that we only really support circuit addresses anyways):

Modify addrutil.IsFDCostlyTransport to ignore circuit addresses.

When sorting between relay connections when dialing, split on the P_CIRCUIT protocol and call addrutil.IsFDCostlyTransport on the relay part.

That way:

We don't need to look at the swarm's transports. You're right in that we should probably expose this information on the transport itself, but we don't do that yet anyways.

Can just go "full special case" for circuit addresses.

Does it sound like that will simplify this?

aarshkshah1992 · 2020-05-11T06:34:57Z

@Stebalien We shouldn't leak how our dial uses the addrutil.IsFDCostlyTransport utility into the shared utility itself.

I've documented the code better for now and have filed an issue(#214 ) to simplify/fix this later.

Have made the other changes. Please take a look when you can.

raulk · 2020-05-12T15:32:15Z

swarm_dial.go

+	var errNonFd *DialError
+	var dialErr *DialError
+
+	connC, errNonFd = s.dialAddrs(ctx, p, nonFdAddrs)


I don't think this is a good idea. QUIC is a UDP-based protocol. UDP is unreliable, it has no connection establishment flow like TCP, the feedback from lack of connectivity/port closed is not immediate like with TCP. If we dialled an IP that was routable but a bad/closed port, I suspect we'd have to wait for the full connection timeout to notice (5 seconds?), whereas with TCP we'd notice during the TCP handshake (much sooner). Same applies if firewalls/routers are dropping UDP packets.

What does this buy us? If the ultimate goal is to select a succeeding a QUIC connection over a succeeding TCP connection, sequentialising the dials is going to cause more pain than gain IMO.

The TCP handshake is a layer below TCP reliability. It's all IP at that point:

The feedback for a UDP packet sent to a closed port is the same as the feedback for TCP: an ICMP unreachable packet. Unfortunately, many home firewalls will just drop packets so you'll get no feedback either way.

If a SYN gets dropped, it will be resent eventually (2s?). I assume QUIC does the same.

The real question is whether or not the ICMP unreachable packet this is exposed to the reader. @marten-seemann, any ideas on this?

What does this buy us?

It:

Avoids tying up slots in the file descriptor dial limiter needed by peers that only support TCP.

Avoids spraying SYN packets (NATs don't like that).

However, it's not critical. I'm fine landing a version of this patch that simply prioritizes UDP connections.

The TCP handshake is a layer below TCP reliability. It's all IP at that point

That is correct.

If a SYN gets dropped, it will be resent eventually (2s?). I assume QUIC does the same.

This is the order of magnitute that the TCP RFC recommends, but most implementations use a much shorter value. Similarly, the QUIC draft was changed after discussing with the TCP folks to use the same value, but not many implementations are willing to take that performance hit. quic-go retransmits two copies of the Initial packet after 200ms (with exponential backoff after that).

The real question is whether or not the ICMP unreachable packet this is exposed to the reader. @marten-seemann, any ideas on this?

For UDP, the kernel will only deliver ICMP packets if you're using a connected socket.

You can't really rely on ICMP packets anyway, since they're frequently dropped (or mis-routed). Furthermore, as ICMP packets are not authenticated, it would be inadvisable to take any action, except for the very early stages of the handshake.

aarshkshah1992 · 2020-05-14T15:07:47Z

@steb @raulk We no longer wait for UDP addresses to finish before dialling TCP addresses. Please take a look at the PR when you can.

Stebalien · 2020-05-15T06:16:37Z

swarm_dial.go

-	// try to get a connection to any addr
-	connC, dialErr := s.dialAddrs(ctx, p, goodAddrsChan)
+	// sorts addresses in descending order of preference for dialing
+	// Private UDP > Private TCP > Public UDP > Public TCP > UDP Relay server > TCP Relay server


I'd actually say public UDP goes before private TCP as TCP dials can get blocked on the fd limiter.

That makes sense. Have made the change.

Stebalien · 2020-05-15T06:17:59Z

swarm_dial.go

+			}
+		}
+		return false
+	}


@aarshkshah1992 thoughts on this?

aarshkshah1992 · 2020-05-15T10:36:41Z

@Stebalien Have addressed your review and we now rank Public UDP addresses above private TCP addresses. Have also replied to your comment about simplifying the FD consuming address func at:

#212 (comment)

Please take a look when you can.

aarshkshah1992 · 2020-05-18T12:26:06Z

ping @Stebalien for one last look.

Stebalien · 2020-05-19T04:29:35Z

swarm_dial.go

+var fdConsumingTptProtos = map[int]struct{}{
+	ma.P_WS:  struct{}{},
+	ma.P_TCP: struct{}{},
+}


I'd strongly prefer to just check if the address contains tcp and/or unix (i.e., a fd consuming transports) before any proxy addresses. Otherwise, we're going to miss transports (e.g., WSS).

I think I grok what you mean here and in #212 (comment) and it makes sense. I've made the changes.

Stebalien · 2020-05-19T04:37:22Z

swarm_dial.go

+			}
+		}
+		return false
+	}


I still think this code needs to be simpler:

We're checking to see if the address is a "proxy" address, but we only actually support circuit addresses. We should just split on the circuit address and be done with it.

We're hard coding rules for specific transport protocols when we should be able to just say "if protocol X appears in the target multiaddr (before any proxy protocols), the address must consume a file descriptor". That is, we know that the tcp and unix protocols both must consume a file descriptor per connection while udp, memory, etc. protocols don't.

Basically, this code is trying to be general purpose and abstract over transports, but we can't actually do that so it's hard coding a bunch of stuff. Sitting halfway in between is just confusing.

Stebalien · 2020-05-19T04:41:30Z

swarm_dial.go

+				return 4
+			}
+			return 6
+		}


nit: Numeric scores are hard to read and easy to get wrong. Honestly, now that I see this, I'd go back to sorting these addresses into separate slices then concatenating them like you were doing before (sorry for going back and forth on this).

Note: This is fine as-is, just a bit confusing.

I agree. I think that way is easier to read. This is done.

aarshkshah1992 · 2020-05-19T15:43:19Z

@Stebalien I've addressed all your concerns and it looks simpler/cleaner now. Please take a look.

Stebalien

One small change. Otherwise, LGTM!

Stebalien · 2020-05-20T05:43:54Z

swarm_dial.go

+		var othersFd []ma.Multiaddr     // public fd consuming
+
+		for _, a := range addrs {
+			if manet.IsPrivateAddr(a) {


I think this will include circuit addrs. We should probably check for that first.

Good catch. This is done.

Stebalien suggested changes May 6, 2020

View reviewed changes

Stebalien reviewed May 7, 2020

View reviewed changes

aarshkshah1992 mentioned this pull request May 25, 2022

swarm: simplify how we classify address as FD consuming libp2p/go-libp2p#1548

Open

aarshkshah1992 requested a review from raulk May 12, 2020 14:47

raulk suggested changes May 12, 2020

View reviewed changes

aarshkshah1992 changed the title ~~[WIP] Swarm Dial Priorities~~ Rank Dial addresses May 14, 2020

Stebalien reviewed May 15, 2020

View reviewed changes

aarshkshah1992 added 7 commits May 18, 2020 17:51

swarm dial priorities

2dd727b

non fd address test

c0a570e

changes as per review

2a891c8

changes as per review

8cd8c89

prioritize non fd addrs

b13443e

change ranking of addrs

5494652

resolve conflicts

da0e646

aarshkshah1992 force-pushed the feat/dial-priorities branch from 1f9d2a7 to da0e646 Compare May 18, 2020 12:25

Stebalien reviewed May 19, 2020

View reviewed changes

changes as per review

180072c

Stebalien approved these changes May 20, 2020

View reviewed changes

changes as per review

771501b

aarshkshah1992 merged commit fbfe382 into master May 20, 2020

aarshkshah1992 deleted the feat/dial-priorities branch May 20, 2020 09:04

Stebalien mentioned this pull request May 26, 2020

Release 0.6.0 ipfs/kubo#7347

Closed

77 tasks

Rank Dial addresses #212

Rank Dial addresses #212

Conversation

aarshkshah1992 commented May 5, 2020 • edited Loading

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

Stebalien May 6, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 7, 2020

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

aarshkshah1992 commented May 11, 2020 • edited Loading

raulk May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 15, 2020 • edited Loading

aarshkshah1992 commented May 18, 2020

Choose a reason for hiding this comment

aarshkshah1992 May 19, 2020 • edited Loading

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

aarshkshah1992 commented May 19, 2020

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented May 5, 2020 •

edited

Loading

Stebalien May 6, 2020 •

edited

Loading

aarshkshah1992 commented May 11, 2020 •

edited

Loading

raulk May 12, 2020 •

edited

Loading

aarshkshah1992 commented May 15, 2020 •

edited

Loading

aarshkshah1992 May 19, 2020 •

edited

Loading