address review comments

Author: sukunrt

config/config.go

@@ -439,8 +439,6 @@ func (cfg *Config) newBasicHost(swrm *swarm.Swarm, eventBus event.Bus) (*bhost.B
 		DisableIdentifyAddressDiscovery: cfg.DisableIdentifyAddressDiscovery,
 		EnableAutoNATv2:                 cfg.EnableAutoNATv2,
 		AutoNATv2Dialer:                 autonatv2Dialer,
-		EnableAutoRelay:                 cfg.EnableAutoRelay,
-		AutoRelayOpts:                   cfg.AutoRelayOpts,
 	})
 	if err != nil {
 		return nil, err
@@ -534,7 +532,7 @@ func (cfg *Config) NewNode() (host.Host, error) {
 	}
 	fxopts = append(fxopts, transportOpts...)
 
-	// Configure routing and autorelay
+	// Configure routing
 	if cfg.Routing != nil {
 		fxopts = append(fxopts,
 			fx.Provide(cfg.Routing),
@@ -544,6 +542,28 @@ func (cfg *Config) NewNode() (host.Host, error) {
 		)
 	}
 
+	// enable autorelay
+	fxopts = append(fxopts,
+		fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) error {
+			if cfg.EnableAutoRelay {
+				if !cfg.DisableMetrics {
+					mt := autorelay.WithMetricsTracer(
+						autorelay.NewMetricsTracer(autorelay.WithRegisterer(cfg.PrometheusRegisterer)))
+					mtOpts := []autorelay.Option{mt}
+					cfg.AutoRelayOpts = append(mtOpts, cfg.AutoRelayOpts...)
+				}
+
+				ar, err := autorelay.NewAutoRelay(h, cfg.AutoRelayOpts...)
+				if err != nil {
+					return err
+				}
+				lifecycle.Append(fx.StartStopHook(ar.Start, ar.Close))
+				return nil
+			}
+			return nil
+		}),
+	)
+
 	var bh *bhost.BasicHost
 	fxopts = append(fxopts, fx.Invoke(func(bho *bhost.BasicHost) { bh = bho }))
 	fxopts = append(fxopts, fx.Invoke(func(h *bhost.BasicHost, lifecycle fx.Lifecycle) {
@@ -558,10 +578,12 @@ func (cfg *Config) NewNode() (host.Host, error) {
 	fxopts = append(fxopts, cfg.UserFxOptions...)
 
 	app := fx.New(fxopts...)
-	if app.Err() != nil {
-		return nil, fmt.Errorf("failed to create host: %w", app.Err())
+	if err := app.Start(context.Background()); err != nil {
+		return nil, err
 	}
+
 	if err := cfg.addAutoNAT(bh); err != nil {
+		app.Stop(context.Background())
 		if cfg.Routing != nil {
 			rh.Close()
 		} else {
@@ -570,10 +592,6 @@ func (cfg *Config) NewNode() (host.Host, error) {
 		return nil, err
 	}
 
-	if err := app.Start(context.Background()); err != nil {
-		return nil, err
-	}
-
 	if cfg.Routing != nil {
 		return &closableRoutedHost{App: app, RoutedHost: rh}, nil
 	}

core/event/addrs.go

@@ -83,6 +83,6 @@ type EvtLocalAddressesUpdated struct {
 }
 
 // EvtAutoRelayAddrsUpdated is sent by the autorelay when the node's relay addresses are updated
-type EvtAutoRelayAddrs struct {
+type EvtAutoRelayAddrsUpdated struct {
 	RelayAddrs []ma.Multiaddr
 }

libp2p_test.go

@@ -466,7 +466,6 @@ func TestDialCircuitAddrWithWrappedResourceManager(t *testing.T) {
 		),
 		peerstore.TempAddrTTL,
 	)
-	require.NoError(t, err)
 
 	require.Eventually(t, func() bool {
 		ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)

p2p/host/autorelay/autorelay.go

@@ -10,7 +10,6 @@ import (
 	"github.com/libp2p/go-libp2p/core/host"
 	"github.com/libp2p/go-libp2p/core/network"
 	"github.com/libp2p/go-libp2p/p2p/host/eventbus"
-	ma "github.com/multiformats/go-multiaddr"
 
 	logging "github.com/ipfs/go-log/v2"
 )
@@ -54,10 +53,6 @@ func NewAutoRelay(host host.Host, opts ...Option) (*AutoRelay, error) {
 	return r, nil
 }
 
-func (r *AutoRelay) RelayAddrs() []ma.Multiaddr {
-	return r.relayFinder.RelayAddrs()
-}
-
 func (r *AutoRelay) Start() {
 	r.refCount.Add(1)
 	go func() {
@@ -82,7 +77,6 @@ func (r *AutoRelay) background() {
 			if !ok {
 				return
 			}
-			// TODO: push changed addresses
 			evt := ev.(event.EvtLocalReachabilityChanged)
 			switch evt.Reachability {
 			case network.ReachabilityPrivate, network.ReachabilityUnknown:

p2p/host/autorelay/autorelay_test.go

@@ -19,6 +19,7 @@ import (
 	circuitv2_proto "github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/proto"
 
 	ma "github.com/multiformats/go-multiaddr"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -524,13 +525,14 @@ func TestNoBusyLoop0MinInterval(t *testing.T) {
 }
 func TestAutoRelayAddrsEvent(t *testing.T) {
 	cl := newMockClock()
-	r1, r2 := newRelay(t), newRelay(t)
+	relays := []host.Host{newRelay(t), newRelay(t), newRelay(t), newRelay(t), newRelay(t)}
 	t.Cleanup(func() {
-		r1.Close()
-		r2.Close()
+		for _, r := range relays {
+			r.Close()
+		}
 	})
 
-	relayFromP2PAddr := func(a ma.Multiaddr) peer.ID {
+	relayIDFromP2PAddr := func(a ma.Multiaddr) peer.ID {
 		r, c := ma.SplitLast(a)
 		if c.Protocol().Code != ma.P_CIRCUIT {
 			return ""
@@ -541,47 +543,61 @@ func TestAutoRelayAddrsEvent(t *testing.T) {
 		return ""
 	}
 
-	checkPeersExist := func(addrs []ma.Multiaddr, peers ...peer.ID) bool {
+	checkAddrsContainsPeersAsRelay := func(addrs []ma.Multiaddr, peers ...peer.ID) bool {
 		for _, p := range peers {
-			if !slices.ContainsFunc(addrs, func(a ma.Multiaddr) bool { return relayFromP2PAddr(a) == p }) {
+			if !slices.ContainsFunc(addrs, func(a ma.Multiaddr) bool { return relayIDFromP2PAddr(a) == p }) {
 				return false
 			}
 		}
 		return true
 	}
-	peerChan := make(chan peer.AddrInfo, 3)
+	peerChan := make(chan peer.AddrInfo, 5)
 	h := newPrivateNode(t,
 		func(context.Context, int) <-chan peer.AddrInfo {
 			return peerChan
 		},
 		autorelay.WithClock(cl),
 		autorelay.WithMinCandidates(1),
 		autorelay.WithMaxCandidates(10),
-		autorelay.WithNumRelays(3),
+		autorelay.WithNumRelays(5),
 		autorelay.WithBootDelay(1*time.Second),
 		autorelay.WithMinInterval(time.Hour),
 	)
 	defer h.Close()
 
-	sub, err := h.EventBus().Subscribe(new(event.EvtAutoRelayAddrs))
+	sub, err := h.EventBus().Subscribe(new(event.EvtAutoRelayAddrsUpdated))
 	require.NoError(t, err)
 
-	peerChan <- peer.AddrInfo{ID: r1.ID(), Addrs: r1.Addrs()}
+	peerChan <- peer.AddrInfo{ID: relays[0].ID(), Addrs: relays[0].Addrs()}
 	cl.AdvanceBy(time.Second)
 
-	require.Eventually(t, func() bool {
+	require.EventuallyWithT(t, func(collect *assert.CollectT) {
 		e := <-sub.Out()
-		if !checkPeersExist(e.(event.EvtAutoRelayAddrs).RelayAddrs, r1.ID()) {
-			return false
+		evt := e.(event.EvtAutoRelayAddrsUpdated)
+		if !checkAddrsContainsPeersAsRelay(evt.RelayAddrs, relays[0].ID()) {
+			collect.Errorf("expected %s to be in %v", relays[0].ID(), evt.RelayAddrs)
 		}
-		if checkPeersExist(e.(event.EvtAutoRelayAddrs).RelayAddrs, r2.ID()) {
-			return false
+		if checkAddrsContainsPeersAsRelay(evt.RelayAddrs, relays[1].ID()) {
+			collect.Errorf("expected %s to not be in %v", relays[1].ID(), evt.RelayAddrs)
 		}
-		return true
 	}, 5*time.Second, 50*time.Millisecond)
-	peerChan <- peer.AddrInfo{ID: r2.ID(), Addrs: r2.Addrs()}
-	require.Eventually(t, func() bool {
+	for _, r := range relays[1:] {
+		peerChan <- peer.AddrInfo{ID: r.ID(), Addrs: r.Addrs()}
+	}
+	require.EventuallyWithT(t, func(c *assert.CollectT) {
 		e := <-sub.Out()
-		return checkPeersExist(e.(event.EvtAutoRelayAddrs).RelayAddrs, r1.ID(), r2.ID())
+		evt := e.(event.EvtAutoRelayAddrsUpdated)
+		relayIds := []peer.ID{}
+		for _, r := range relays[1:] {
+			relayIds = append(relayIds, r.ID())
+		}
+		if !checkAddrsContainsPeersAsRelay(evt.RelayAddrs, relayIds...) {
+			c.Errorf("expected %s to be in %v", relayIds, evt.RelayAddrs)
+		}
 	}, 5*time.Second, 50*time.Millisecond)
+	select {
+	case e := <-sub.Out():
+		t.Fatal("expected no more events after all reservations obtained; got: ", e.(event.EvtAutoRelayAddrsUpdated))
+	case <-time.After(1 * time.Second):
+	}
 }