review comments

Author: sukunrt

.golangci.yml

@@ -8,6 +8,8 @@ linters:
     - revive
     - unused
     - prealloc
+  disable:
+    - errcheck
 
   settings:
     revive:

p2p/host/basic/addrs_manager.go

@@ -34,6 +34,7 @@ type hostAddrs struct {
 	localAddrs       []ma.Multiaddr
 	reachableAddrs   []ma.Multiaddr
 	unreachableAddrs []ma.Multiaddr
+	relayAddrs       []ma.Multiaddr
 }
 
 type addrsManager struct {
@@ -56,11 +57,8 @@ type addrsManager struct {
 
 	hostReachability atomic.Pointer[network.Reachability]
 
-	addrsMx      sync.RWMutex // protects fields below
+	addrsMx      sync.RWMutex
 	currentAddrs hostAddrs
-	// relayAddrs are the host's relay addresses. Kept separate from hostAddrs as we
-	// update them differently from hostAddrs.
-	relayAddrs []ma.Multiaddr
 
 	wg        sync.WaitGroup
 	ctx       context.Context
@@ -140,7 +138,7 @@ func (a *addrsManager) NetNotifee() network.Notifiee {
 }
 
 func (a *addrsManager) triggerAddrsUpdate() {
-	a.updateAddrs()
+	a.updateAddrs(false, nil)
 	select {
 	case a.triggerAddrsUpdateChan <- struct{}{}:
 	default:
@@ -177,11 +175,12 @@ func (a *addrsManager) startBackgroundWorker() error {
 		return errors.Join(err, err1, err2)
 	}
 
+	var relayAddrs []ma.Multiaddr
 	// update relay addrs in case we're private
 	select {
 	case e := <-autoRelayAddrsSub.Out():
 		if evt, ok := e.(event.EvtAutoRelayAddrsUpdated); ok {
-			a.updateRelayAddrs(evt.RelayAddrs)
+			relayAddrs = slices.Clone(evt.RelayAddrs)
 		}
 	default:
 	}
@@ -195,24 +194,23 @@ func (a *addrsManager) startBackgroundWorker() error {
 	}
 	// update addresses before starting the worker loop. This ensures that any address updates
 	// before calling addrsManager.Start are correctly reported after Start returns.
-	a.updateAddrs()
+	a.updateAddrs(true, relayAddrs)
+
 	a.wg.Add(1)
-	go func() {
-		defer a.wg.Done()
-		a.background(autoRelayAddrsSub, autonatReachabilitySub, emitter)
-	}()
+	go a.background(autoRelayAddrsSub, autonatReachabilitySub, emitter, relayAddrs)
 	return nil
 }
 
-func (a *addrsManager) background(autoRelayAddrsSub, autonatReachabilitySub event.Subscription, emitter event.Emitter) {
+func (a *addrsManager) background(autoRelayAddrsSub, autonatReachabilitySub event.Subscription,
+	emitter event.Emitter, relayAddrs []ma.Multiaddr,
+) {
+	defer a.wg.Done()
 	defer func() {
 		err := autoRelayAddrsSub.Close()
 		if err != nil {
 			log.Warnf("error closing auto relay addrs sub: %s", err)
 		}
-	}()
-	defer func() {
-		err := autonatReachabilitySub.Close()
+		err = autonatReachabilitySub.Close()
 		if err != nil {
 			log.Warnf("error closing autonat reachability sub: %s", err)
 		}
@@ -222,7 +220,7 @@ func (a *addrsManager) background(autoRelayAddrsSub, autonatReachabilitySub even
 	defer ticker.Stop()
 	var previousAddrs hostAddrs
 	for {
-		currAddrs := a.updateAddrs()
+		currAddrs := a.updateAddrs(true, relayAddrs)
 		a.notifyAddrsChanged(emitter, previousAddrs, currAddrs)
 		previousAddrs = currAddrs
 		select {
@@ -231,7 +229,7 @@ func (a *addrsManager) background(autoRelayAddrsSub, autonatReachabilitySub even
 		case <-a.triggerReachabilityUpdate:
 		case e := <-autoRelayAddrsSub.Out():
 			if evt, ok := e.(event.EvtAutoRelayAddrsUpdated); ok {
-				a.updateRelayAddrs(evt.RelayAddrs)
+				relayAddrs = slices.Clone(evt.RelayAddrs)
 			}
 		case e := <-autonatReachabilitySub.Out():
 			if evt, ok := e.(event.EvtLocalReachabilityChanged); ok {
@@ -243,7 +241,9 @@ func (a *addrsManager) background(autoRelayAddrsSub, autonatReachabilitySub even
 	}
 }
 
-func (a *addrsManager) updateAddrs() hostAddrs {
+// updateAddrs updates the addresses of the host and returns the new updated
+// addrs
+func (a *addrsManager) updateAddrs(updateRelayAddrs bool, relayAddrs []ma.Multiaddr) hostAddrs {
 	// Must lock while doing both recompute and update as this method is called from
 	// multiple goroutines.
 	a.addrsMx.Lock()
@@ -254,29 +254,31 @@ func (a *addrsManager) updateAddrs() hostAddrs {
 	if a.addrsReachabilityTracker != nil {
 		currReachableAddrs, currUnreachableAddrs = a.getConfirmedAddrs(localAddrs)
 	}
-	currAddrs := a.getAddrs(slices.Clone(localAddrs), a.getRelayAddrsUnlocked())
+	if !updateRelayAddrs {
+		relayAddrs = a.currentAddrs.relayAddrs
+	} else {
+		// Copy the callers slice
+		relayAddrs = slices.Clone(relayAddrs)
+	}
+	currAddrs := a.getAddrs(slices.Clone(localAddrs), relayAddrs)
 
 	a.currentAddrs = hostAddrs{
 		addrs:            append(a.currentAddrs.addrs[:0], currAddrs...),
 		localAddrs:       append(a.currentAddrs.localAddrs[:0], localAddrs...),
 		reachableAddrs:   append(a.currentAddrs.reachableAddrs[:0], currReachableAddrs...),
 		unreachableAddrs: append(a.currentAddrs.unreachableAddrs[:0], currUnreachableAddrs...),
+		relayAddrs:       append(a.currentAddrs.relayAddrs[:0], relayAddrs...),
 	}
 
 	return hostAddrs{
 		localAddrs:       localAddrs,
 		addrs:            currAddrs,
 		reachableAddrs:   currReachableAddrs,
 		unreachableAddrs: currUnreachableAddrs,
+		relayAddrs:       relayAddrs,
 	}
 }
 
-func (a *addrsManager) updateRelayAddrs(addrs []ma.Multiaddr) {
-	a.addrsMx.Lock()
-	defer a.addrsMx.Unlock()
-	a.relayAddrs = append(a.relayAddrs[:0], addrs...)
-}
-
 func (a *addrsManager) notifyAddrsChanged(emitter event.Emitter, previous, current hostAddrs) {
 	if areAddrsDifferent(previous.localAddrs, current.localAddrs) {
 		log.Debugf("host local addresses updated: %s", current.localAddrs)
@@ -308,7 +310,11 @@ func (a *addrsManager) notifyAddrsChanged(emitter event.Emitter, previous, curre
 // If autorelay is enabled and node reachability is private, it returns
 // the node's relay addresses and private network addresses.
 func (a *addrsManager) Addrs() []ma.Multiaddr {
-	return a.getAddrs(a.DirectAddrs(), a.RelayAddrs())
+	a.addrsMx.RLock()
+	directAddrs := slices.Clone(a.currentAddrs.localAddrs)
+	relayAddrs := slices.Clone(a.currentAddrs.relayAddrs)
+	a.addrsMx.RUnlock()
+	return a.getAddrs(directAddrs, relayAddrs)
 }
 
 // getAddrs returns the node's dialable addresses. Mutates localAddrs
@@ -358,48 +364,9 @@ func (a *addrsManager) ReachableAddrs() []ma.Multiaddr {
 	return slices.Clone(a.currentAddrs.reachableAddrs)
 }
 
-// RelayAddrs returns all the relay addresses of the host.
-func (a *addrsManager) RelayAddrs() []ma.Multiaddr {
-	a.addrsMx.RLock()
-	defer a.addrsMx.RUnlock()
-	return a.getRelayAddrsUnlocked()
-}
-
-func (a *addrsManager) getRelayAddrsUnlocked() []ma.Multiaddr {
-	return slices.Clone(a.relayAddrs)
-}
-
 func (a *addrsManager) getConfirmedAddrs(localAddrs []ma.Multiaddr) (reachableAddrs, unreachableAddrs []ma.Multiaddr) {
 	reachableAddrs, unreachableAddrs = a.addrsReachabilityTracker.ConfirmedAddrs()
-	return removeIfNotInSource(reachableAddrs, localAddrs), removeIfNotInSource(unreachableAddrs, localAddrs)
-}
-
-// removeIfNotInSource removes items from addrs that are not present in source.
-// Modifies the addrs slice in place
-// addrs and source must be sorted using multiaddr.Compare.
-func removeIfNotInSource(addrs, source []ma.Multiaddr) []ma.Multiaddr {
-	j := 0
-	// mark entries not in source as nil
-	for i, a := range addrs {
-		// move right till a is greater
-		for j < len(source) && a.Compare(source[j]) > 0 {
-			j++
-		}
-		// a is not in source if we've reached the end, or a is lesser
-		if j == len(source) || a.Compare(source[j]) < 0 {
-			addrs[i] = nil
-		}
-		// a is in source, nothing to do
-	}
-	// j is the current element, i is the lowest index nil element
-	i := 0
-	for j := 0; j < len(addrs); j++ {
-		if addrs[j] != nil {
-			addrs[i], addrs[j] = addrs[j], addrs[i]
-			i++
-		}
-	}
-	return addrs[:i]
+	return removeNotInSource(reachableAddrs, localAddrs), removeNotInSource(unreachableAddrs, localAddrs)
 }
 
 var p2pCircuitAddr = ma.StringCast("/p2p-circuit")
@@ -701,3 +668,31 @@ func (i *interfaceAddrsCache) updateUnlocked() {
 		}
 	}
 }
+
+// removeNotInSource removes items from addrs that are not present in source.
+// Modifies the addrs slice in place
+// addrs and source must be sorted using multiaddr.Compare.
+func removeNotInSource(addrs, source []ma.Multiaddr) []ma.Multiaddr {
+	j := 0
+	// mark entries not in source as nil
+	for i, a := range addrs {
+		// move right till a is greater
+		for j < len(source) && a.Compare(source[j]) > 0 {
+			j++
+		}
+		// a is not in source if we've reached the end, or a is lesser
+		if j == len(source) || a.Compare(source[j]) < 0 {
+			addrs[i] = nil
+		}
+		// a is in source, nothing to do
+	}
+	// 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]
+}

p2p/host/basic/addrs_manager_test.go

@@ -34,7 +34,7 @@ func TestAppendNATAddrs(t *testing.T) {
 			// nat mapping success, obsaddress ignored
 			Listen: ma.StringCast("/ip4/0.0.0.0/udp/1/quic-v1"),
 			Nat:    ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1"),
-			ObsAddrFunc: func(m ma.Multiaddr) []ma.Multiaddr {
+			ObsAddrFunc: func(_ ma.Multiaddr) []ma.Multiaddr {
 				return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/udp/100/quic-v1")}
 			},
 			Expected: []ma.Multiaddr{ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1")},
@@ -120,7 +120,7 @@ func TestAppendNATAddrs(t *testing.T) {
 		t.Run(tc.Name, func(t *testing.T) {
 			as := &addrsManager{
 				natManager: &mockNatManager{
-					GetMappingFunc: func(addr ma.Multiaddr) ma.Multiaddr {
+					GetMappingFunc: func(_ ma.Multiaddr) ma.Multiaddr {
 						return tc.Nat
 					},
 				},
@@ -139,7 +139,7 @@ type mockNatManager struct {
 	GetMappingFunc func(addr ma.Multiaddr) ma.Multiaddr
 }
 
-func (m *mockNatManager) Close() error {
+func (*mockNatManager) Close() error {
 	return nil
 }
 
@@ -150,7 +150,7 @@ func (m *mockNatManager) GetMapping(addr ma.Multiaddr) ma.Multiaddr {
 	return m.GetMappingFunc(addr)
 }
 
-func (m *mockNatManager) HasDiscoveredNAT() bool {
+func (*mockNatManager) HasDiscoveredNAT() bool {
 	return true
 }
 
@@ -336,7 +336,7 @@ func TestAddrsManager(t *testing.T) {
 		}
 		am := newAddrsManagerTestCase(t, addrsManagerArgs{
 			ObservedAddrsManager: &mockObservedAddrs{
-				ObservedAddrsForFunc: func(addr ma.Multiaddr) []ma.Multiaddr {
+				ObservedAddrsForFunc: func(_ ma.Multiaddr) []ma.Multiaddr {
 					return quicAddrs
 				},
 			},
@@ -352,7 +352,7 @@ func TestAddrsManager(t *testing.T) {
 	t.Run("public addrs removed when private", func(t *testing.T) {
 		am := newAddrsManagerTestCase(t, addrsManagerArgs{
 			ObservedAddrsManager: &mockObservedAddrs{
-				ObservedAddrsForFunc: func(addr ma.Multiaddr) []ma.Multiaddr {
+				ObservedAddrsForFunc: func(_ ma.Multiaddr) []ma.Multiaddr {
 					return []ma.Multiaddr{publicQUIC}
 				},
 			},
@@ -394,7 +394,7 @@ func TestAddrsManager(t *testing.T) {
 				return nil
 			},
 			ObservedAddrsManager: &mockObservedAddrs{
-				ObservedAddrsForFunc: func(addr ma.Multiaddr) []ma.Multiaddr {
+				ObservedAddrsForFunc: func(_ ma.Multiaddr) []ma.Multiaddr {
 					return []ma.Multiaddr{publicQUIC}
 				},
 			},
@@ -414,7 +414,7 @@ func TestAddrsManager(t *testing.T) {
 	t.Run("updates addresses on signaling", func(t *testing.T) {
 		updateChan := make(chan struct{})
 		am := newAddrsManagerTestCase(t, addrsManagerArgs{
-			AddrsFactory: func(addrs []ma.Multiaddr) []ma.Multiaddr {
+			AddrsFactory: func(_ []ma.Multiaddr) []ma.Multiaddr {
 				select {
 				case <-updateChan:
 					return []ma.Multiaddr{publicQUIC}
@@ -451,7 +451,7 @@ func TestAddrsManagerReachabilityEvent(t *testing.T) {
 		// currently they aren't being passed to the reachability tracker
 		ListenAddrs: func() []ma.Multiaddr { return []ma.Multiaddr{publicQUIC, publicQUIC2, publicTCP} },
 		AutoNATClient: mockAutoNATClient{
-			F: func(ctx context.Context, reqs []autonatv2.Request) (autonatv2.Result, error) {
+			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) {
@@ -496,7 +496,7 @@ func TestRemoveIfNotInSource(t *testing.T) {
 	}
 	for i, tc := range cases {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
-			addrs := removeIfNotInSource(tc.addrs, tc.source)
+			addrs := removeNotInSource(tc.addrs, tc.source)
 			require.ElementsMatch(t, tc.expected, addrs, "%s\n%s", tc.expected, tc.addrs)
 		})
 	}
@@ -524,6 +524,6 @@ func BenchmarkRemoveIfNotInSource(b *testing.B) {
 	b.ReportAllocs()
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		removeIfNotInSource(slices.Clone(addrs[:5]), addrs[:])
+		removeNotInSource(slices.Clone(addrs[:5]), addrs[:])
 	}
 }