basichost: don't advertise unreachable addrs.

Author: sukunrt

p2p/host/basic/addrs_manager.go

@@ -317,13 +317,13 @@ func (a *addrsManager) updateAddrs(prevHostAddrs hostAddrs, relayAddrs []ma.Mult
 		currReachableAddrs, currUnreachableAddrs, currUnknownAddrs = a.getConfirmedAddrs(localAddrs)
 	}
 	relayAddrs = slices.Clone(relayAddrs)
-	currAddrs := a.getAddrs(slices.Clone(localAddrs), relayAddrs)
+	currAddrs := a.getDialableAddrs(localAddrs, currReachableAddrs, currUnreachableAddrs, relayAddrs)
+	currAddrs = a.applyAddrsFactory(currAddrs)
 
 	if areAddrsDifferent(prevHostAddrs.addrs, currAddrs) {
 		_, _, removed := diffAddrs(prevHostAddrs.addrs, currAddrs)
 		a.updatePeerStore(currAddrs, removed)
 	}
-
 	a.addrsMx.Lock()
 	a.currentAddrs = hostAddrs{
 		addrs:            append(a.currentAddrs.addrs[:0], currAddrs...),
@@ -410,25 +410,35 @@ func (a *addrsManager) notifyAddrsUpdated(emitter event.Emitter, localAddrsEmitt
 // the node's relay addresses and private network addresses.
 func (a *addrsManager) Addrs() []ma.Multiaddr {
 	a.addrsMx.RLock()
-	directAddrs := slices.Clone(a.currentAddrs.localAddrs)
-	relayAddrs := slices.Clone(a.currentAddrs.relayAddrs)
+	addrs := a.getDialableAddrs(a.currentAddrs.localAddrs, a.currentAddrs.reachableAddrs, a.currentAddrs.unreachableAddrs, a.currentAddrs.relayAddrs)
 	a.addrsMx.RUnlock()
-	return a.getAddrs(directAddrs, relayAddrs)
+	// don't hold the lock while applying addrs factory
+	return a.applyAddrsFactory(addrs)
 }
 
-// getAddrs returns the node's dialable addresses. Mutates localAddrs
-func (a *addrsManager) getAddrs(localAddrs []ma.Multiaddr, relayAddrs []ma.Multiaddr) []ma.Multiaddr {
-	addrs := localAddrs
-	rch := a.hostReachability.Load()
-	if rch != nil && *rch == network.ReachabilityPrivate {
-		// Delete public addresses if the node's reachability is private, and we have relay addresses
-		if len(relayAddrs) > 0 {
+// getDialableAddrs returns the node's dialable addrs. Doesn't mutate any argument.
+func (a *addrsManager) getDialableAddrs(localAddrs, reachableAddrs, unreachableAddrs, relayAddrs []ma.Multiaddr) []ma.Multiaddr {
+	// remove known unreachable addrs
+	addrs := removeInSource(slices.Clone(localAddrs), unreachableAddrs)
+	// If we have no confirmed reachable addresses, add the relay addresses
+	if a.addrsReachabilityTracker != nil {
+		if len(reachableAddrs) == 0 {
+			addrs = append(addrs, relayAddrs...)
+		}
+	} else {
+		// If we're only using autonatv1, remove public addrs and add relay addrs
+		if len(relayAddrs) > 0 && *a.hostReachability.Load() == network.ReachabilityPrivate {
 			addrs = slices.DeleteFunc(addrs, manet.IsPublicAddr)
 			addrs = append(addrs, relayAddrs...)
 		}
 	}
-	// Make a copy. Consumers can modify the slice elements
-	addrs = slices.Clone(a.addrsFactory(addrs))
+	return addrs
+}
+
+func (a *addrsManager) applyAddrsFactory(addrs []ma.Multiaddr) []ma.Multiaddr {
+	af := a.addrsFactory(addrs)
+	// Copy to our slice in case addrsFactory returns its own same slice always.
+	addrs = append(addrs[:0], af...)
 	// Add certhashes for the addresses provided by the user via address factory.
 	addrs = a.addCertHashes(ma.Unique(addrs))
 	slices.SortFunc(addrs, func(a, b ma.Multiaddr) int { return a.Compare(b) })
@@ -880,6 +890,33 @@ func removeNotInSource(addrs, source []ma.Multiaddr) []ma.Multiaddr {
 	return addrs[:i]
 }
 
+// removeInSource removes items from addrs that are present in source.
+// Modifies the addrs slice in place
+// addrs and source must be sorted using multiaddr.Compare.
+func removeInSource(addrs, source []ma.Multiaddr) []ma.Multiaddr {
+	j := 0
+	// mark entries in source as nil
+	for i, a := range addrs {
+		// move right in source as long as a > source[j]
+		for j < len(source) && a.Compare(source[j]) > 0 {
+			j++
+		}
+		// a is in source,  mark nil
+		if j < len(source) && a.Compare(source[j]) == 0 {
+			addrs[i] = nil
+		}
+	}
+	// j is the current element, i is the lowest index nil element
+	i := 0
+	for j := range len(addrs) {
+		if addrs[j] != nil {
+			addrs[i], addrs[j] = addrs[j], addrs[i]
+			i++
+		}
+	}
+	return addrs[:i]
+}
+
 type multiCloser []io.Closer
 
 func (mc *multiCloser) Close() error {

p2p/host/basic/addrs_manager_test.go

@@ -3,7 +3,6 @@ package basichost
 import (
 	"context"
 	"crypto/rand"
-	"errors"
 	"fmt"
 	"slices"
 	"sync/atomic"
@@ -406,10 +405,10 @@ func TestAddrsManagerReachabilityEvent(t *testing.T) {
 			F: func(_ context.Context, reqs []autonatv2.Request) (autonatv2.Result, error) {
 				if reqs[0].Addr.Equal(publicQUIC) {
 					return autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityPublic}, nil
-				} else if reqs[0].Addr.Equal(publicTCP) || reqs[0].Addr.Equal(publicQUIC2) {
+				} else if reqs[0].Addr.Equal(publicQUIC2) {
 					return autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityPrivate}, nil
 				}
-				return autonatv2.Result{}, errors.New("invalid")
+				return autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityUnknown, AllAddrsRefused: true}, nil
 			},
 		},
 	})
@@ -429,17 +428,20 @@ func TestAddrsManagerReachabilityEvent(t *testing.T) {
 
 	// Wait for probes to complete and addresses to be classified
 	reachableAddrs := []ma.Multiaddr{publicQUIC}
-	unreachableAddrs := []ma.Multiaddr{publicTCP, publicQUIC2}
+	unreachableAddrs := []ma.Multiaddr{publicQUIC2}
+	unknownAddrs := []ma.Multiaddr{publicTCP}
 	select {
 	case e := <-sub.Out():
 		evt := e.(event.EvtHostReachableAddrsChanged)
-		require.ElementsMatch(t, reachableAddrs, evt.Reachable)
-		require.ElementsMatch(t, unreachableAddrs, evt.Unreachable)
-		require.Empty(t, evt.Unknown)
+		matest.AssertMultiaddrsMatch(t, reachableAddrs, evt.Reachable)
+		matest.AssertMultiaddrsMatch(t, unreachableAddrs, evt.Unreachable)
+		matest.AssertMultiaddrsMatch(t, unknownAddrs, evt.Unknown)
 		reachable, unreachable, unknown := am.ConfirmedAddrs()
-		require.ElementsMatch(t, reachable, reachableAddrs)
-		require.ElementsMatch(t, unreachable, unreachableAddrs)
-		require.Empty(t, unknown)
+		matest.AssertMultiaddrsMatch(t, reachableAddrs, reachable)
+		matest.AssertMultiaddrsMatch(t, unreachableAddrs, unreachable)
+		matest.AssertMultiaddrsMatch(t, unknownAddrs, unknown)
+		// unreachable addrs should be removed
+		matest.AssertMultiaddrsMatch(t, []ma.Multiaddr{publicQUIC, publicTCP}, am.Addrs())
 	case <-time.After(5 * time.Second):
 		t.Fatal("expected final event for reachability change after probing")
 	}