Merge pull request #64 from libp2p/fix/accurate-buckets

fix: use accurate bucket logic

table.go

@@ -261,18 +261,18 @@ func (rt *RoutingTable) NearestPeers(id ID, count int) []peer.ID {
 	// Add peers from the target bucket (cpl+1 shared bits).
 	pds.appendPeersFromList(rt.buckets[cpl].list)
 
-	// If we're short, add peers from buckets to the right until we have
-	// enough. All buckets to the right share exactly cpl bits (as opposed
-	// to the cpl+1 bits shared by the peers in the cpl bucket).
+	// If we're short, add peers from all buckets to the right. All buckets
+	// to the right share exactly cpl bits (as opposed to the cpl+1 bits
+	// shared by the peers in the cpl bucket).
 	//
-	// Unfortunately, to be completely correct, we can't just take from
-	// buckets until we have enough peers because peers because _all_ of
-	// these peers will be ~2**(256-cpl) from us.
-	//
-	// However, we're going to do that anyways as it's "good enough"
+	// This is, unfortunately, less efficient than we'd like. We will switch
+	// to a trie implementation eventually which will allow us to find the
+	// closest N peers to any target key.
 
-	for i := cpl + 1; i < len(rt.buckets) && pds.Len() < count; i++ {
-		pds.appendPeersFromList(rt.buckets[i].list)
+	if pds.Len() < count {
+		for i := cpl + 1; i < len(rt.buckets); i++ {
+			pds.appendPeersFromList(rt.buckets[i].list)
+		}
 	}
 
 	// If we're still short, add in buckets that share _fewer_ bits. We can

table_test.go

@@ -311,26 +311,13 @@ func TestTableFindMultiple(t *testing.T) {
 	}
 }
 
-func assertSortedPeerIdsEqual(t *testing.T, a, b []peer.ID) {
-	t.Helper()
-	if len(a) != len(b) {
-		t.Fatal("slices aren't the same length")
-	}
-	for i, p := range a {
-		if p != b[i] {
-			t.Fatalf("unexpected peer %d", i)
-		}
-	}
-}
-
 func TestTableFindMultipleBuckets(t *testing.T) {
 	t.Parallel()
 
 	local := test.RandPeerIDFatal(t)
-	localID := ConvertPeerID(local)
 	m := pstore.NewMetrics()
 
-	rt, err := NewRoutingTable(5, localID, time.Hour, m, NoOpThreshold)
+	rt, err := NewRoutingTable(5, ConvertPeerID(local), time.Hour, m, NoOpThreshold)
 	require.NoError(t, err)
 
 	peers := make([]peer.ID, 100)
@@ -339,96 +326,28 @@ func TestTableFindMultipleBuckets(t *testing.T) {
 		rt.TryAddPeer(peers[i], true)
 	}
 
-	targetID := ConvertPeerID(peers[2])
-
-	closest := SortClosestPeers(rt.ListPeers(), targetID)
-	targetCpl := CommonPrefixLen(localID, targetID)
-
-	// split the peers into closer, same, and further from the key (than us).
-	var (
-		closer, same, further []peer.ID
-	)
-	var i int
-	for i = 0; i < len(closest); i++ {
-		cpl := CommonPrefixLen(ConvertPeerID(closest[i]), targetID)
-		if targetCpl >= cpl {
-			break
-		}
-	}
-	closer = closest[:i]
+	closest := SortClosestPeers(rt.ListPeers(), ConvertPeerID(peers[2]))
 
-	var j int
-	for j = len(closer); j < len(closest); j++ {
-		cpl := CommonPrefixLen(ConvertPeerID(closest[j]), targetID)
-		if targetCpl > cpl {
-			break
-		}
-	}
-	same = closest[i:j]
-	further = closest[j:]
+	t.Logf("Searching for peer: '%s'", peers[2])
 
 	// should be able to find at least 30
 	// ~31 (logtwo(100) * 5)
-	found := rt.NearestPeers(targetID, 20)
+	found := rt.NearestPeers(ConvertPeerID(peers[2]), 20)
 	if len(found) != 20 {
 		t.Fatalf("asked for 20 peers, got %d", len(found))
 	}
-
-	// We expect this list to include:
-	// * All peers with a common prefix length > than the CPL between our key
-	//   and the target (peers in the target bucket).
-	// * Then a subset of the peers with the _same_ CPL (peers in buckets to the right).
-	// * Finally, other peers to the left of the target bucket.
-
-	// First, handle the peers that are _closer_ than us.
-	if len(found) <= len(closer) {
-		// All of our peers are "closer".
-		assertSortedPeerIdsEqual(t, found, closer[:len(found)])
-		return
-	}
-	assertSortedPeerIdsEqual(t, found[:len(closer)], closer)
-
-	// We've worked through closer peers, let's work through peers at the
-	// "same" distance.
-	found = found[len(closer):]
-
-	// Next, handle peers that are at the same distance as us.
-	if len(found) <= len(same) {
-		// Ok, all the remaining peers are at the same distance.
-		// unfortunately, that means we may be missing peers that are
-		// technically closer.
-
-		// Make sure all remaining peers are _somewhere_ in the "closer" set.
-		pset := peer.NewSet()
-		for _, p := range same {
-			pset.Add(p)
-		}
-		for _, p := range found {
-			if !pset.Contains(p) {
-				t.Fatalf("unexpected peer %s", p)
-			}
+	for i, p := range found {
+		if p != closest[i] {
+			t.Fatalf("unexpected peer %d", i)
 		}
-		return
 	}
-	assertSortedPeerIdsEqual(t, found[:len(same)], same)
-
-	// We've worked through closer peers, let's work through the further
-	// peers.
-	found = found[len(same):]
-
-	// All _further_ peers will be correctly sorted.
-	assertSortedPeerIdsEqual(t, found, further[:len(found)])
-
-	// Finally, test getting _all_ peers. These should be in the correct
-	// order.
 
 	// Ok, now let's try finding all of them.
 	found = rt.NearestPeers(ConvertPeerID(peers[2]), 100)
 	if len(found) != rt.Size() {
 		t.Fatalf("asked for %d peers, got %d", rt.Size(), len(found))
 	}
 
-	// We should get _all_ peers in sorted order.
 	for i, p := range found {
 		if p != closest[i] {
 			t.Fatalf("unexpected peer %d", i)