From 8303a54cd70b65072033736f69c152a520dfde39 Mon Sep 17 00:00:00 2001 From: bhartnett Date: Tue, 6 May 2025 14:19:21 +0800 Subject: [PATCH 1/3] Fix replaceNode so that the node will not be removed if the replacement cache is empty. --- eth/p2p/discoveryv5/routing_table.nim | 39 +++++++++++++++++++-------- tests/p2p/test_routing_table.nim | 16 +++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/eth/p2p/discoveryv5/routing_table.nim b/eth/p2p/discoveryv5/routing_table.nim index 773c42c4..afcc83ae 100644 --- a/eth/p2p/discoveryv5/routing_table.nim +++ b/eth/p2p/discoveryv5/routing_table.nim @@ -195,7 +195,8 @@ func ipLimitDec(r: var RoutingTable, b: KBucket, n: Node) = r.ipLimits.dec(ip) func getNode*(r: RoutingTable, id: NodeId): Opt[Node] -proc replaceNode*(r: var RoutingTable, n: Node) {.gcsafe.} +proc removeNode*(r: var RoutingTable, n: Node) {.gcsafe.} +proc replaceNode*(r: var RoutingTable, n: Node): bool {.discardable, gcsafe.} proc banNode*(r: var RoutingTable, nodeId: NodeId, period: chronos.Duration) = ## Ban a node from the routing table for the given period. The node is removed @@ -214,8 +215,9 @@ proc banNode*(r: var RoutingTable, nodeId: NodeId, period: chronos.Duration) = # Remove the node from the routing table let node = r.getNode(nodeId) - if node.isSome(): - r.replaceNode(node.get()) + if node.isSome() and not r.replaceNode(node.get()): + # Remove the node if the node was not replaced due to an empty replacement cache + r.removeNode(node.get()) proc isBanned*(r: RoutingTable, nodeId: NodeId): bool = if not r.bannedNodes.contains(nodeId): @@ -458,20 +460,35 @@ proc removeNode*(r: var RoutingTable, n: Node) = if b.remove(n): ipLimitDec(r, b, n) -proc replaceNode*(r: var RoutingTable, n: Node) = +proc replaceNode*(r: var RoutingTable, n: Node): bool {.discardable.} = ## Replace node `n` with last entry in the replacement cache. If there are - ## no entries in the replacement cache, node `n` will simply be removed. - # TODO: Kademlia paper recommends here to not remove nodes if there are no + ## no entries in the replacement cache, node `n` will simply be marked as + ## not seen. Returns bool indictating whether or not the node was successfully + ## replaced. + + # Note: Kademlia paper recommends here to not remove nodes if there are no # replacements. However, that would require a bit more complexity in the # revalidation as you don't want to try pinging that node all the time. + + var replaced = false + let b = r.bucketForNode(n.id) - if b.remove(n): + + if b.replacementCache.len == 0: + let idx = b.nodes.find(n) + if idx >= 0 and n.seen: + b.nodes[idx].seen = false + + elif b.remove(n): ipLimitDec(r, b, n) - if b.replacementCache.len > 0: - # Nodes in the replacement cache are already included in the ip limits. - b.add(b.replacementCache[high(b.replacementCache)]) - b.replacementCache.delete(high(b.replacementCache)) + # Nodes in the replacement cache are already included in the ip limits. + b.add(b.replacementCache[high(b.replacementCache)]) + b.replacementCache.delete(high(b.replacementCache)) + replaced = true + + return replaced + func getNode*(r: RoutingTable, id: NodeId): Opt[Node] = ## Get the `Node` with `id` as `NodeId` from the routing table. diff --git a/tests/p2p/test_routing_table.nim b/tests/p2p/test_routing_table.nim index 652262ce..cc6c5c84 100644 --- a/tests/p2p/test_routing_table.nim +++ b/tests/p2p/test_routing_table.nim @@ -148,7 +148,7 @@ suite "Routing Table Tests": table.replaceNode(generateNode(PrivateKey.random(rng[]))) check table.len == 1 - table.replaceNode(addedNode) + table.removeNode(addedNode) check table.len == 0 test "Empty replacement cache": @@ -162,8 +162,8 @@ suite "Routing Table Tests": check table.addNode(n) == Added table.replaceNode(table.nodeToRevalidate()) - # This node should still be removed - check (table.getNode(bucketNodes[bucketNodes.high].id)).isNone() + # This node should not be removed + check (table.getNode(bucketNodes[bucketNodes.high].id)).isSome() test "Double add": let node = generateNode(PrivateKey.random(rng[])) @@ -191,7 +191,7 @@ suite "Routing Table Tests": check: res.isSome() res.get() == doubleNode - table.len == 1 + table.len == 16 test "Double replacement add": let node = generateNode(PrivateKey.random(rng[])) @@ -237,7 +237,7 @@ suite "Routing Table Tests": for n in bucketNodes: table.replaceNode(table.nodeToRevalidate()) - check (table.getNode(n.id)).isNone() + check (table.getNode(n.id)).isSome() test "Just seen replacement": let node = generateNode(PrivateKey.random(rng[])) @@ -286,7 +286,7 @@ suite "Routing Table Tests": check table.addNode(anotherSameIpNode) == IpLimitReached # Remove one and try add again - table.replaceNode(table.nodeToRevalidate()) + table.removeNode(table.nodeToRevalidate()) check table.addNode(anotherSameIpNode) == Added # Further fill the bucket with nodes with different ip. @@ -386,10 +386,10 @@ suite "Routing Table Tests": table.getNode(anotherSameIpNode2.id).isNone() - block: # Replace again to see if the first one never becomes available + block: # Replace again to see if the first one becomes available table.replaceNode(table.nodeToRevalidate()) check: - table.getNode(anotherSameIpNode1.id).isNone() + table.getNode(anotherSameIpNode1.id).isSome() table.getNode(anotherSameIpNode2.id).isNone() test "Ip limits on replacement cache: deletion": From 702c93b13af5d75ac3334fa64bdffa770ce36d55 Mon Sep 17 00:00:00 2001 From: bhartnett Date: Wed, 7 May 2025 11:11:31 +0800 Subject: [PATCH 2/3] Improve tests. --- eth/p2p/discoveryv5/routing_table.nim | 12 +++--- tests/p2p/test_routing_table.nim | 54 +++++++++++++++++---------- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/eth/p2p/discoveryv5/routing_table.nim b/eth/p2p/discoveryv5/routing_table.nim index afcc83ae..b7f0bc4d 100644 --- a/eth/p2p/discoveryv5/routing_table.nim +++ b/eth/p2p/discoveryv5/routing_table.nim @@ -470,25 +470,23 @@ proc replaceNode*(r: var RoutingTable, n: Node): bool {.discardable.} = # replacements. However, that would require a bit more complexity in the # revalidation as you don't want to try pinging that node all the time. - var replaced = false - let b = r.bucketForNode(n.id) if b.replacementCache.len == 0: let idx = b.nodes.find(n) + if idx >= 0 and n.seen: b.nodes[idx].seen = false - + return false elif b.remove(n): ipLimitDec(r, b, n) # Nodes in the replacement cache are already included in the ip limits. b.add(b.replacementCache[high(b.replacementCache)]) b.replacementCache.delete(high(b.replacementCache)) - replaced = true - - return replaced - + return true + else: + return false func getNode*(r: RoutingTable, id: NodeId): Opt[Node] = ## Get the `Node` with `id` as `NodeId` from the routing table. diff --git a/tests/p2p/test_routing_table.nim b/tests/p2p/test_routing_table.nim index cc6c5c84..c91a346f 100644 --- a/tests/p2p/test_routing_table.nim +++ b/tests/p2p/test_routing_table.nim @@ -157,41 +157,56 @@ suite "Routing Table Tests": var table = RoutingTable.init(node, 1, ipLimits, rng = rng) # create a full bucket TODO: no need to store bucketNodes - let bucketNodes = node.nodesAtDistance(rng[], 256, BUCKET_SIZE) - for n in bucketNodes: - check table.addNode(n) == Added + var bucketNodes = node.nodesAtDistance(rng[], 256, BUCKET_SIZE) + for i in 0.. Split only the branch in range of own id var table = RoutingTable.init(node, 1, ipLimits, rng = rng) - let doubleNode = node.nodeAtDistance(rng[], 256) + var doubleNode = node.nodeAtDistance(rng[], 256) + doubleNode.seen = true + # Try to add the node twice check table.addNode(doubleNode) == Added check table.addNode(doubleNode) == Existing - for n in 0.. Date: Wed, 7 May 2025 11:34:21 +0800 Subject: [PATCH 3/3] Fix Discv5 test. --- tests/p2p/test_discoveryv5.nim | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index ac4ae265..c1b014ba 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/tests/p2p/test_discoveryv5.nim @@ -339,7 +339,9 @@ suite "Discovery v5.1 Tests": n.isSome() n.get().id == targetId n.get().record.seqNum == targetSeqNum - # Node will be removed because of failed findNode request. + + # Remove the target node from the main node routing table + mainNode.routingTable.removeNode(targetNode.localNode) # Bring target back online, update seqNum in ENR, check if we get the # updated ENR. @@ -389,6 +391,12 @@ suite "Discovery v5.1 Tests": await targetNode.closeWait() check mainNode.addNode(lookupNode.localNode.record) + + # Remove the target node from the mainNode routing table + # so that in the call to resolve below the lookupNode node + # is used to fetch the updated ENR. + mainNode.routingTable.removeNode(targetNode.localNode) + let n = await mainNode.resolve(targetId) check: n.isSome()