config: fix AddrFactory for AutoNAT (#2868)

---------

Co-authored-by: Marco Munizaga <git@marcopolo.io>

config/config.go

@@ -407,9 +407,9 @@ func (cfg *Config) newBasicHost(swrm *swarm.Swarm, eventBus event.Bus) (*bhost.B
 		// addresses by default.
 		//
 		// TODO: We shouldn't be doing this here.
-		oldFactory := h.AddrsFactory
+		originalAddrFactory := h.AddrsFactory
 		h.AddrsFactory = func(addrs []ma.Multiaddr) []ma.Multiaddr {
-			return oldFactory(autorelay.Filter(addrs))
+			return originalAddrFactory(autorelay.Filter(addrs))
 		}
 	}
 	return h, nil
@@ -486,8 +486,18 @@ func (cfg *Config) NewNode() (host.Host, error) {
 		)
 	}
 
-	// Note: h.AddrsFactory may be changed by relayFinder, but non-relay version is
+	// originalAddrFactory is the AddrFactory before it's modified by autorelay
-	// used by AutoNAT below.
+	// we need this for checking reachability via autonat
+	originalAddrFactory := func(addrs []ma.Multiaddr) []ma.Multiaddr {
+		return addrs
+	}
+
+	// enable autorelay
+	fxopts = append(fxopts,
+		fx.Invoke(func(h *bhost.BasicHost) {
+			originalAddrFactory = h.AddrsFactory
+		}),
+		fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) error {
 			if cfg.EnableAutoRelay {
 				if !cfg.DisableMetrics {
 					mt := autorelay.WithMetricsTracer(
@@ -495,17 +505,17 @@ func (cfg *Config) NewNode() (host.Host, error) {
 					mtOpts := []autorelay.Option{mt}
 					cfg.AutoRelayOpts = append(mtOpts, cfg.AutoRelayOpts...)
 				}
-		fxopts = append(fxopts,
+
-			fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) (*autorelay.AutoRelay, error) {
 				ar, err := autorelay.NewAutoRelay(h, cfg.AutoRelayOpts...)
 				if err != nil {
-					return nil, err
+					return err
 				}
 				lifecycle.Append(fx.StartStopHook(ar.Start, ar.Close))
-				return ar, nil
+				return nil
+			}
+			return nil
 		}),
 	)
-	}
 
 	var bh *bhost.BasicHost
 	fxopts = append(fxopts, fx.Invoke(func(bho *bhost.BasicHost) { bh = bho }))
@@ -523,7 +533,7 @@ func (cfg *Config) NewNode() (host.Host, error) {
 		return nil, err
 	}
 
-	if err := cfg.addAutoNAT(bh); err != nil {
+	if err := cfg.addAutoNAT(bh, originalAddrFactory); err != nil {
 		app.Stop(context.Background())
 		if cfg.Routing != nil {
 			rh.Close()
@@ -539,8 +549,7 @@ func (cfg *Config) NewNode() (host.Host, error) {
 	return &closableBasicHost{App: app, BasicHost: bh}, nil
 }
 
-func (cfg *Config) addAutoNAT(h *bhost.BasicHost) error {
+func (cfg *Config) addAutoNAT(h *bhost.BasicHost, addrF AddrsFactory) error {
-	addrF := h.AddrsFactory
 	autonatOpts := []autonat.Option{
 		autonat.UsingAddresses(func() []ma.Multiaddr {
 			return addrF(h.AllAddrs())

libp2p_test.go

@@ -465,3 +465,26 @@ func TestDialCircuitAddrWithWrappedResourceManager(t *testing.T) {
 	require.NoError(t, res.Error)
 	defer cancel()
 }
+
+func TestHostAddrsFactoryAddsCerthashes(t *testing.T) {
+	addr := ma.StringCast("/ip4/1.2.3.4/udp/1/quic-v1/webtransport")
+	h, err := New(
+		AddrsFactory(func(m []ma.Multiaddr) []ma.Multiaddr {
+			return []ma.Multiaddr{addr}
+		}),
+	)
+	require.NoError(t, err)
+	require.Eventually(t, func() bool {
+		addrs := h.Addrs()
+		for _, a := range addrs {
+			first, last := ma.SplitFunc(a, func(c ma.Component) bool {
+				return c.Protocol().Code == ma.P_CERTHASH
+			})
+			if addr.Equal(first) && last != nil {
+				return true
+			}
+		}
+		return false
+	}, 5*time.Second, 50*time.Millisecond)
+	h.Close()
+}

p2p/host/basic/basic_host.go

@@ -284,6 +284,20 @@ func NewHost(n network.Network, opts *HostOpts) (*BasicHost, error) {
 	if opts.AddrsFactory != nil {
 		h.AddrsFactory = opts.AddrsFactory
 	}
+	// This is a terrible hack.
+	// We want to use this AddrsFactory for autonat. Wrapping AddrsFactory here ensures
+	// that autonat receives addresses with the correct certhashes.
+	//
+	// This logic cannot be in Addrs method as autonat cannot use the Addrs method directly.
+	// The autorelay package updates AddrsFactory to only provide p2p-circuit addresses when
+	// reachability is Private.
+	//
+	// Wrapping it here allows us to provide the wrapped AddrsFactory to autonat before
+	// autorelay updates it.
+	addrFactory := h.AddrsFactory
+	h.AddrsFactory = func(addrs []ma.Multiaddr) []ma.Multiaddr {
+		return h.addCertHashes(addrFactory(addrs))
+	}
 
 	if opts.NATManager != nil {
 		h.natmgr = opts.NATManager(n)
@@ -804,47 +818,13 @@ func (h *BasicHost) ConnManager() connmgr.ConnManager {
 // Addrs returns listening addresses that are safe to announce to the network.
 // The output is the same as AllAddrs, but processed by AddrsFactory.
 func (h *BasicHost) Addrs() []ma.Multiaddr {
-	// This is a temporary workaround/hack that fixes #2233. Once we have a
+	// We don't need to append certhashes here, the user provided addrsFactory was
-	// proper address pipeline, rework this. See the issue for more context.
+	// wrapped with addCertHashes in the constructor.
-	type transportForListeninger interface {
-		TransportForListening(a ma.Multiaddr) transport.Transport
-	}
-
-	type addCertHasher interface {
-		AddCertHashes(m ma.Multiaddr) (ma.Multiaddr, bool)
-	}
-
 	addrs := h.AddrsFactory(h.AllAddrs())
-
+	// Make a copy. Consumers can modify the slice elements
-	s, ok := h.Network().(transportForListeninger)
+	res := make([]ma.Multiaddr, len(addrs))
-	if !ok {
+	copy(res, addrs)
-		return addrs
+	return res
-	}
-
-	// Copy addrs slice since we'll be modifying it.
-	addrsOld := addrs
-	addrs = make([]ma.Multiaddr, len(addrsOld))
-	copy(addrs, addrsOld)
-
-	for i, addr := range addrs {
-		wtOK, wtN := libp2pwebtransport.IsWebtransportMultiaddr(addr)
-		webrtcOK, webrtcN := libp2pwebrtc.IsWebRTCDirectMultiaddr(addr)
-		if (wtOK && wtN == 0) || (webrtcOK && webrtcN == 0) {
-			t := s.TransportForListening(addr)
-			tpt, ok := t.(addCertHasher)
-			if !ok {
-				continue
-			}
-			addrWithCerthash, added := tpt.AddCertHashes(addr)
-			if !added {
-				log.Debugf("Couldn't add certhashes to multiaddr: %s", addr)
-				continue
-			}
-			addrs[i] = addrWithCerthash
-		}
-	}
-
-	return addrs
 }
 
 // NormalizeMultiaddr returns a multiaddr suitable for equality checks.
@@ -864,8 +844,9 @@ func (h *BasicHost) NormalizeMultiaddr(addr ma.Multiaddr) ma.Multiaddr {
 	return addr
 }
 
-// AllAddrs returns all the addresses of BasicHost at this moment in time.
+// AllAddrs returns all the addresses the host is listening on except circuit addresses.
-// It's ok to not include addresses if they're not available to be used now.
+// The output has webtransport addresses inferred from quic addresses.
+// All the addresses have the correct
 func (h *BasicHost) AllAddrs() []ma.Multiaddr {
 	listenAddrs := h.Network().ListenAddresses()
 	if len(listenAddrs) == 0 {
@@ -959,10 +940,50 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
 	}
 	finalAddrs = ma.Unique(finalAddrs)
 	finalAddrs = inferWebtransportAddrsFromQuic(finalAddrs)
-
 	return finalAddrs
 }
 
+func (h *BasicHost) addCertHashes(addrs []ma.Multiaddr) []ma.Multiaddr {
+	// This is a temporary workaround/hack that fixes #2233. Once we have a
+	// proper address pipeline, rework this. See the issue for more context.
+	type transportForListeninger interface {
+		TransportForListening(a ma.Multiaddr) transport.Transport
+	}
+
+	type addCertHasher interface {
+		AddCertHashes(m ma.Multiaddr) (ma.Multiaddr, bool)
+	}
+
+	s, ok := h.Network().(transportForListeninger)
+	if !ok {
+		return addrs
+	}
+
+	// Copy addrs slice since we'll be modifying it.
+	addrsOld := addrs
+	addrs = make([]ma.Multiaddr, len(addrsOld))
+	copy(addrs, addrsOld)
+
+	for i, addr := range addrs {
+		wtOK, wtN := libp2pwebtransport.IsWebtransportMultiaddr(addr)
+		webrtcOK, webrtcN := libp2pwebrtc.IsWebRTCDirectMultiaddr(addr)
+		if (wtOK && wtN == 0) || (webrtcOK && webrtcN == 0) {
+			t := s.TransportForListening(addr)
+			tpt, ok := t.(addCertHasher)
+			if !ok {
+				continue
+			}
+			addrWithCerthash, added := tpt.AddCertHashes(addr)
+			if !added {
+				log.Debugf("Couldn't add certhashes to multiaddr: %s", addr)
+				continue
+			}
+			addrs[i] = addrWithCerthash
+		}
+	}
+	return addrs
+}
+
 var wtComponent = ma.StringCast("/webtransport")
 
 // inferWebtransportAddrsFromQuic infers more webtransport addresses from QUIC addresses.