autorelay: curtail addrsplosion #598

vyzo · 2019-04-13T19:37:24Z

Relays, as discovered in the DHT, are addrsploded. The current (pre-)computation of advertised relay addrs fails to account for this fact, resulting in the advertisement of many dead addresses. In addition, many relays discovered in the DHT are actually dead.

This patch adds logic to deal with the situation

relay addrs are dynamically recomputed by the address factory using the relay addrs present in the peerstore instead of pre-computing based on what was advertised.
a special clean up pass is done on the relay address set to remove addrsploded addrs.
a retry pass is added to findRelays that deals with the case of no live relays in the discovered set.
we also increase the relay advertisement boot delay to 15 min so that some minimum node stability is ensured.

Note: the patch was originally much more extensive, adding a complex relay selection logic that tried to parallelize to deal with dead relays. That proved to be contentious, and biased against dedicated relays, so it has been reverted.

vyzo · 2019-04-14T07:47:34Z

I think we might want to try parallel connects, as many of the relays in the discovered set (I have 183) appear to be dead.

vyzo · 2019-04-14T07:59:01Z

If not parallel connects, at least parallel FindPeers.

vyzo · 2019-04-14T09:22:55Z

Added a relay address set clean up phase to curtail addrsplosion.

vyzo · 2019-04-14T09:57:50Z

Maybe 15s to FindPeer and Connect is too aggressive though, might be better served with a 30s timeout.

vyzo · 2019-04-14T09:58:10Z

I will revert it to 30s.

vyzo · 2019-04-14T11:11:31Z

Reworked the relay selection logic to perform FindPeer in parallel.

Stebalien

Almost there. Please ping me on IRC/slack when you want me to review this again (trying to shorten my review cycles a bit..).

p2p/host/relay/addrsplosion.go

Stebalien · 2019-04-19T05:48:16Z

p2p/host/relay/addrsplosion.go

+	ma.ForEach(a, func(c ma.Component) bool {
+		switch c.Protocol().Code {
+		case ma.P_TCP, ma.P_UDP:
+			port = int(binary.BigEndian.Uint16(c.RawValue()))


Ideally, this would only bother with the first port (although multiple ports isn't currently an issue except for relay addresses which we should be ignoring anyways).

p2p/host/relay/addrsplosion.go

p2p/host/relay/autorelay.go

Stebalien · 2019-04-19T05:56:31Z

p2p/host/relay/autorelay.go

+		case qr := <-resultCh:
+			rcount++
+			if qr.err == nil {
+				pi := cleanupAddressSet(qr.pi)


Ah, you mean we won't advertise addresses like /ip4/.../tcp/ADDRSPLODE/p2p/QmRelay/p2p-circuit/p2p/QmMe? Got it.

Stebalien · 2019-04-19T05:58:51Z

p2p/host/relay/autorelay.go

+		case qr := <-resultCh:
+			rcount++
+			if qr.err == nil {
+				pi := cleanupAddressSet(qr.pi)


Although, if we're going through all this trouble for that, I really wish we could just wait for the relay to tell us its addresses.

Note: If we actually had a reserve, the relay would return the addresses it wants us to use in response to the reserve command.

(but oh well)

…ss set findRelays cleans up address sets now for addrsplosion, and this removes private addrs as well.

Stebalien · 2019-04-19T20:30:51Z

WRT #598 (comment)

The bad peer addresses will already get put into the peerstore anyways so this patch won't actually help fix dialing relays. This patch will allow us to trim down the address list auto-relay users announce but a better solution might be to re-calculate /p2p-cirucit addresses dynamically from the peerstore. That way, we get the latest addresses sent by our relay's identify service.

In terms of this code, @vyzo and I discussed generalizing it and using it from within either the dialer and/or the peerstore. Specifically, we can use the addrsplosion detection logic to trim down the set of addresses we're dialing. However, the current heuristics around circuit and private addresses obviously won't generalize (although we should drop addresses of the form /ip4/private/tcp/.../p2p-circuit/...).

So, the question is, is merging this patch worth it? IMO, not really given that it doesn't affect dialing and there's an (IMO) better option on the advertising end.

vyzo · 2019-04-19T20:42:52Z

So the patch fixes two problems with the current code:

It fixes the addrsplosion in the announced addrs, which is a real problem as most of the address sets returned by discovery are addrsploded (this has been experimentally determined)
It fixes the handling of dead peers in the discovery set, which are processed serially and could result to very long times in determining a relay to use.

vyzo · 2019-04-19T20:47:30Z

Wrt to moving forward, we can stop tracking the relay addrs discovered by FindPeer and use the set returned by identify after connecting (that is when constructing the address set in updateAddrs).
We will still have to clean it up though, as we might have garbage in the peerstore from the queries.

vyzo · 2019-04-19T21:56:55Z

I have updated the code to dynamically compute the relay address set on demand.

- select 25 of 50 relays instead of 20 - increase connect timeout to 30s

vyzo · 2019-04-20T10:23:47Z

I have significantly simplified the patch, removing the contentious parallel query selection logic.
The behaviour wrt to relay selection and dialing matches the old behaviour now.

the presence of stashed query results from discovery in the peerstore _biases_ the selection towards fully DHT nodes, which penalizes dedicated relays.

Stebalien

This looks correct.

increase autorelay discovery limit to 1k

ffd4981

ghost assigned vyzo Apr 13, 2019

ghost added the status/in-progress In progress label Apr 13, 2019

vyzo requested a review from Stebalien April 13, 2019 19:37

reduce relay find peer and connect timeout to 30s

a1ebc4d

vyzo changed the title ~~Increase autorelay discovery limit to 1k~~ Increase autorelay discovery limit to 1k and curtail addrsplosion Apr 14, 2019

clean up relay address sets to curtail addrsplosion

aa03a9b

vyzo force-pushed the feat/moar-relays branch from 5359653 to aa03a9b Compare April 14, 2019 09:58

rework relay selection logic

9863e22

vyzo changed the title ~~Increase autorelay discovery limit to 1k and curtail addrsplosion~~ autorelay: rework relay selection logic to deal with dead relays and increasing relay set size Apr 14, 2019

vyzo changed the title ~~autorelay: rework relay selection logic to deal with dead relays and increasing relay set size~~ autorelay: rework relay selection logic to deal with dead relays and curtail addrsplosion Apr 14, 2019

vyzo added 5 commits April 14, 2019 14:13

select 50 relays

765ed0e

fix selectRelays to return promptly when there are query errors

39f2b7a

shuffle relay set before queries

becb89a

only determine default port if it is in a public addr

1138fb6

increase relay advertising boot delay to 15min

31dfaf6

vyzo requested a review from raulk April 14, 2019 11:54

vyzo added 7 commits April 14, 2019 15:06

increase FindPeer timeout to 30s

c9f627e

don't drop ephemeral ports in address set clean up

45d2888

fix and reinstate autorelay test

da65fd7

also check private addresses when looking for 4001

8c7da83

small tweak in autorelay test

081bb0f

log and ignore relay peers with empty address sets

4a6e767

retry to find relays if we fail to connect to any

376379b

Stebalien requested changes Apr 19, 2019

View reviewed changes

vyzo added 5 commits April 19, 2019 13:48

rewrite isDNSAddr to use ma.SplitFirst

9d7f6b8

filter relay addrs in address set cleanup

528c473

test for privately bound port first when cleaning up addrsplosion

21c4e1d

some more addrsplosion tests

ff4b98a

use addresses from the peerstore if available

a8d14f9

vyzo requested a review from Stebalien April 19, 2019 11:14

vyzo added 2 commits April 19, 2019 19:48

used named constants for dns address protocols

9b2731e

remove redundant private addr check when constructing our relay addre…

bd22c49

…ss set findRelays cleans up address sets now for addrsplosion, and this removes private addrs as well.

vyzo added 2 commits April 20, 2019 00:04

don't track relay addrs, use the peerstore

f4f924e

compute relay address set dynamically

c097172

vyzo added 2 commits April 20, 2019 01:40

don't preallocate result array, we don't know how long it will be

4727d5b

pacify the race detector

5c9299a

vyzo force-pushed the feat/moar-relays branch from 31c8d32 to 5c9299a Compare April 19, 2019 22:53

vyzo added 4 commits April 20, 2019 06:25

reduce scope of the lock, pre-allocate result slice in relayAddrs

1a81119

gate max number of retries in findRelays

f9e182f

some tweaks

4629431

- select 25 of 50 relays instead of 20 - increase connect timeout to 30s

add ignore list to account for connection failures

35e805d

kill the parallel query logic in selectRelays; let it be random

8d073ce

the presence of stashed query results from discovery in the peerstore _biases_ the selection towards fully DHT nodes, which penalizes dedicated relays.

vyzo force-pushed the feat/moar-relays branch from 96bcbd2 to 8d073ce Compare April 20, 2019 10:34

vyzo changed the title ~~autorelay: rework relay selection logic to deal with dead relays and curtail addrsplosion~~ autorelay: curtail addrsplosion Apr 20, 2019

Stebalien approved these changes Apr 20, 2019

View reviewed changes

Stebalien merged commit 9fc5c96 into master Apr 20, 2019

ghost removed the status/in-progress In progress label Apr 20, 2019

Stebalien deleted the feat/moar-relays branch April 20, 2019 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autorelay: curtail addrsplosion #598

autorelay: curtail addrsplosion #598

vyzo commented Apr 13, 2019 •

edited

Loading

vyzo commented Apr 14, 2019 •

edited

Loading

vyzo commented Apr 14, 2019 •

edited

Loading

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

Stebalien left a comment

Stebalien Apr 19, 2019

Stebalien Apr 19, 2019

Stebalien Apr 19, 2019

Stebalien commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 20, 2019

Stebalien left a comment

autorelay: curtail addrsplosion #598

autorelay: curtail addrsplosion #598

Conversation

vyzo commented Apr 13, 2019 • edited Loading

vyzo commented Apr 14, 2019 • edited Loading

vyzo commented Apr 14, 2019 • edited Loading

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

vyzo commented Apr 14, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Apr 19, 2019

Choose a reason for hiding this comment

Stebalien Apr 19, 2019

Choose a reason for hiding this comment

Stebalien Apr 19, 2019

Choose a reason for hiding this comment

Stebalien commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 19, 2019

vyzo commented Apr 20, 2019

Stebalien left a comment

Choose a reason for hiding this comment

vyzo commented Apr 13, 2019 •

edited

Loading

vyzo commented Apr 14, 2019 •

edited

Loading

vyzo commented Apr 14, 2019 •

edited

Loading