Skip to content

Commit 42aa212

Browse files
committed
fix(basichost): sort confirmed addrs
probeManager.AppendConfirmedAddrs emits primary addrs followed by secondary addrs. Each half is sorted, the concatenation is not, and both consumers require input sorted by Multiaddr.Compare: addrsManager.getConfirmedAddrs feeds the buckets to removeNotInSource, and Addrs() subtracts the unreachable bucket via removeInSource. Unsorted input makes removeNotInSource silently drop confirmed addrs: a webrtc-direct secondary sorts before its quic-v1 primary and gets nil'd, so ConfirmedAddrs() under-reports secondary transports even though the tracker confirmed them, defeating the reachability inheritance added in #3435. removeInSource fails the other way and retains confirmed-unreachable addrs in Addrs(). Sort the three buckets before returning them.
1 parent ec408fc commit 42aa212

3 files changed

Lines changed: 82 additions & 0 deletions

File tree

p2p/host/basic/addrs_manager_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,40 @@ func TestAddrsManagerReachabilityEvent(t *testing.T) {
449449
}
450450
}
451451

452+
func TestAddrsManagerConfirmedAddrsIncludesSecondaryTransports(t *testing.T) {
453+
// A node listening on every transport kubo enables by default. The
454+
// secondary transports (ws, webrtc-direct, webtransport) inherit Public
455+
// from their thin-waist primary, and all of them must survive into
456+
// ConfirmedAddrs: getConfirmedAddrs feeds these slices to
457+
// removeNotInSource, which drops entries when its input is not sorted.
458+
tcp := ma.StringCast("/ip4/1.2.3.4/tcp/4001")
459+
wsSNI := ma.StringCast("/ip4/1.2.3.4/tcp/4001/tls/sni/*.example.net/ws")
460+
quic := ma.StringCast("/ip4/1.2.3.4/udp/4001/quic-v1")
461+
webrtc := ma.StringCast("/ip4/1.2.3.4/udp/4001/webrtc-direct")
462+
wt := ma.StringCast("/ip4/1.2.3.4/udp/4001/quic-v1/webtransport")
463+
listenAddrs := []ma.Multiaddr{tcp, wsSNI, quic, webrtc, wt}
464+
465+
am := newAddrsManagerTestCase(t, addrsManagerArgs{
466+
ListenAddrs: func() []ma.Multiaddr { return listenAddrs },
467+
AutoNATClient: mockAutoNATClient{
468+
F: func(_ context.Context, reqs []autonatv2.Request) (autonatv2.Result, error) {
469+
return autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityPublic}, nil
470+
},
471+
},
472+
})
473+
defer am.Close()
474+
475+
require.Eventually(t, func() bool {
476+
reachable, _, _ := am.ConfirmedAddrs()
477+
return len(reachable) == len(listenAddrs)
478+
}, 5*time.Second, 50*time.Millisecond, "expected all listen addrs to become confirmed reachable")
479+
480+
reachable, unreachable, unknown := am.ConfirmedAddrs()
481+
matest.AssertMultiaddrsMatch(t, listenAddrs, reachable)
482+
require.Empty(t, unreachable)
483+
require.Empty(t, unknown)
484+
}
485+
452486
func TestAddrsManagerPeerstoreUpdated(t *testing.T) {
453487
quic1 := ma.StringCast("/ip4/1.2.3.4/udp/1234/quic-v1")
454488
quic2 := ma.StringCast("/ip4/1.2.3.5/udp/1/quic-v1")

p2p/host/basic/addrs_reachability_tracker.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,6 +396,9 @@ func newProbeManager(now func() time.Time) *probeManager {
396396
}
397397

398398
// AppendConfirmedAddrs appends the current confirmed reachable and unreachable addresses.
399+
// The returned slices are sorted by Multiaddr.Compare: addrsManager.getConfirmedAddrs
400+
// passes them to removeNotInSource, and Addrs passes the unreachable set to
401+
// removeInSource, both of which require sorted input.
399402
func (m *probeManager) AppendConfirmedAddrs(reachable, unreachable, unknown []ma.Multiaddr) (reachableAddrs, unreachableAddrs, unknownAddrs []ma.Multiaddr) {
400403
m.mx.Lock()
401404
defer m.mx.Unlock()
@@ -425,6 +428,16 @@ func (m *probeManager) AppendConfirmedAddrs(reachable, unreachable, unknown []ma
425428
unknown = append(unknown, a)
426429
}
427430
}
431+
432+
// primaryAddrs and secondaryAddrs are each sorted, but interleave in the
433+
// buckets above (a secondary like webrtc-direct sorts before its quic-v1
434+
// primary). Unsorted output makes removeNotInSource silently drop
435+
// confirmed addrs, and makes removeInSource fail to filter unreachable
436+
// addrs out of Addrs().
437+
cmp := func(a, b ma.Multiaddr) int { return a.Compare(b) }
438+
slices.SortFunc(reachable, cmp)
439+
slices.SortFunc(unreachable, cmp)
440+
slices.SortFunc(unknown, cmp)
428441
return reachable, unreachable, unknown
429442
}
430443

p2p/host/basic/addrs_reachability_tracker_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,41 @@ func TestProbeManager(t *testing.T) {
294294
})
295295
}
296296

297+
func TestProbeManagerConfirmedAddrsSorted(t *testing.T) {
298+
// Secondary transports can sort before their primary: webrtc-direct
299+
// (protocol code 280) sorts before quic-v1 (461) on the same UDP socket.
300+
// AppendConfirmedAddrs iterates primaries then secondaries, so without a
301+
// final sort the buckets interleave; removeNotInSource in addrsManager
302+
// then drops confirmed addrs, and removeInSource fails to filter
303+
// unreachable addrs out of Addrs().
304+
tcp := ma.StringCast("/ip4/1.2.3.4/tcp/4001")
305+
wsSNI := ma.StringCast("/ip4/1.2.3.4/tcp/4001/tls/sni/*.example.net/ws")
306+
quic := ma.StringCast("/ip4/1.2.3.4/udp/4001/quic-v1")
307+
webrtc := ma.StringCast("/ip4/1.2.3.4/udp/4001/webrtc-direct")
308+
wt := ma.StringCast("/ip4/1.2.3.4/udp/4001/quic-v1/webtransport")
309+
addrs := []ma.Multiaddr{tcp, wsSNI, quic, webrtc, wt}
310+
311+
cl := clock.NewMock()
312+
pm := newProbeManager(cl.Now)
313+
pm.UpdateAddrs(slices.Clone(addrs))
314+
315+
for {
316+
reqs := pm.GetProbe()
317+
if len(reqs) == 0 {
318+
break
319+
}
320+
pm.MarkProbeInProgress(reqs)
321+
pm.CompleteProbe(reqs, autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityPublic}, nil)
322+
}
323+
324+
reachable, unreachable, unknown := pm.AppendConfirmedAddrs(nil, nil, nil)
325+
require.Empty(t, unreachable)
326+
require.Empty(t, unknown)
327+
matest.AssertMultiaddrsMatch(t, addrs, reachable)
328+
require.True(t, slices.IsSortedFunc(reachable, func(a, b ma.Multiaddr) int { return a.Compare(b) }),
329+
"reachable addrs not sorted: %v", reachable)
330+
}
331+
297332
type mockAutoNATClient struct {
298333
F func(context.Context, []autonatv2.Request) (autonatv2.Result, error)
299334
}

0 commit comments

Comments
 (0)