Fix depth calculation for bucket splitting

eth/p2p/discoveryv5/routing_table.nim

@@ -153,7 +153,7 @@ proc binaryGetBucketForNode(buckets: openarray[KBucket],
   if result.isNil:
     raise (ref Defect)(msg: "No bucket found for node with id " & $id)
 
-proc computeSharedPrefixBits(nodes: openarray[Node]): int =
+proc computeSharedPrefixBits(nodes: openarray[NodeId]): int =
   ## Count the number of prefix bits shared by all nodes.
   if nodes.len < 2:
     return ID_SIZE
@@ -163,13 +163,14 @@ proc computeSharedPrefixBits(nodes: openarray[Node]): int =
 
   for i in 1 .. ID_SIZE:
     mask = mask or (one shl (ID_SIZE - i))
-    let reference = nodes[0].id and mask
+    let reference = nodes[0] and mask
     for j in 1 .. nodes.high:
-      if (nodes[j].id and mask) != reference: return i - 1
+      if (nodes[j] and mask) != reference: return i - 1
 
   for n in nodes:
-    echo n.id.toHex()
+    echo n.toHex()
 
+  # Reaching this would mean that all node ids are equal
   doAssert(false, "Unable to calculate number of shared prefix bits")
 
 proc init*(r: var RoutingTable, thisNode: Node, bitsPerHop = 8) {.inline.} =
@@ -199,8 +200,10 @@ proc addNode*(r: var RoutingTable, n: Node): Node =
   if not evictionCandidate.isNil:
     # Split if the bucket has the local node in its range or if the depth is not
     # congruent to 0 mod `bitsPerHop`
-
+    #
-    let depth = computeSharedPrefixBits(bucket.nodes)
+    # Calculate the prefix shared by all nodes in the bucket's range, not the
+    # ones actually in the bucket.
+    let depth = computeSharedPrefixBits(@[bucket.istart, bucket.iend])
     # TODO: Shouldn't the adding to replacement cache be done only if the bucket
     # doesn't get split?
     if bucket.inRange(r.thisNode) or

tests/p2p/test_routing_table.nim

@@ -40,22 +40,28 @@ suite "Routing Table Tests":
     let node = generateNode()
     var table: RoutingTable
 
-    # bitsPerHop = 2, allow not in range branch to split once.
+    # bitsPerHop = 2, allow not in range branch to split once (2 buckets).
     table.init(node, 2)
 
-    # Add 16 nodes, distance 256
+    # Add 16 nodes, distance 256 from `node`, but all with 2 bits shared prefix
-    for i in 0..<BUCKET_SIZE:
+    # among themselves.
-      check table.addNode(node.nodeAtDistance(256)) == nil
+    let firstNode = node.nodeAtDistance(256)
+    check table.addNode(firstNode) == nil
+    for n in 1..<BUCKET_SIZE:
+      check table.addNode(firstNode.nodeAtDistance(254)) == nil
 
-    # Add another 32 nodes, to make the not in range branch split and be be sure
+    # Add 16 more nodes with only 1 bit shared prefix with previous 16. This
-    # both buckets are full.
+    # should cause the initial bucket to split and and fill the second bucket
-    # TODO: Could improve by adding specific nodes for one of the buckets.
+    # with the 16 new entries.
-    for i in 0..<BUCKET_SIZE*2:
+    for n in 0..<16:
-      discard table.addNode(node.nodeAtDistance(256))
+      check table.addNode(firstNode.nodeAtDistance(255)) == nil
 
-    # Adding another should fail as both buckets should be full and not be
+    # Adding another should fail as both buckets will be full and not be
-    # allowed to split another time
+    # allowed to split another time.
     check table.addNode(node.nodeAtDistance(256)) != nil
+    # And also when targetting one of the two specific buckets.
+    check table.addNode(firstNode.nodeAtDistance(255)) != nil
+    check table.addNode(firstNode.nodeAtDistance(254)) != nil
     # This add should be allowed as it is on the branch where the own node's id
     # id belongs to.
     check table.addNode(node.nodeAtDistance(255)) == nil