From ceb4a2046385a093555f50dd3cefa2f918df1478 Mon Sep 17 00:00:00 2001
From: kdeme <kim.demey@gmail.com>
Date: Mon, 22 Jun 2020 16:46:58 +0200
Subject: [PATCH] Fix depth calculation for bucket splitting

---
 eth/p2p/discoveryv5/routing_table.nim | 15 ++++++++------
 tests/p2p/test_routing_table.nim      | 28 ++++++++++++++++-----------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/eth/p2p/discoveryv5/routing_table.nim b/eth/p2p/discoveryv5/routing_table.nim
index 1016e19..3329a12 100644
--- a/eth/p2p/discoveryv5/routing_table.nim
+++ b/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
diff --git a/tests/p2p/test_routing_table.nim b/tests/p2p/test_routing_table.nim
index 373489c..d9a6ea0 100644
--- a/tests/p2p/test_routing_table.nim
+++ b/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
-    for i in 0..<BUCKET_SIZE:
-      check table.addNode(node.nodeAtDistance(256)) == nil
+    # Add 16 nodes, distance 256 from `node`, but all with 2 bits shared prefix
+    # among themselves.
+    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
-    # both buckets are full.
-    # TODO: Could improve by adding specific nodes for one of the buckets.
-    for i in 0..<BUCKET_SIZE*2:
-      discard table.addNode(node.nodeAtDistance(256))
+    # Add 16 more nodes with only 1 bit shared prefix with previous 16. This
+    # should cause the initial bucket to split and and fill the second bucket
+    # with the 16 new entries.
+    for n in 0..<16:
+      check table.addNode(firstNode.nodeAtDistance(255)) == nil
 
-    # Adding another should fail as both buckets should be full and not be
-    # allowed to split another time
+    # Adding another should fail as both buckets will be full and not be
+    # 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