Skip to content

RoutingTable node no longer removed in replaceNode when replacement cache empty #791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bhartnett wants to merge 3 commits into
base: master
Choose a base branch
from
Open

RoutingTable node no longer removed in replaceNode when replacement cache empty #791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8303a54
Fix replaceNode so that the node will not be removed if the replaceme…
bhartnett May 6, 2025
702c93b
Improve tests.
bhartnett May 7, 2025
4733859
Fix Discv5 test.
bhartnett May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions eth/p2p/discoveryv5/routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -458,20 +460,33 @@ 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.

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
Copy link
Contributor Author

@bhartnett bhartnett May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdeme When marking the node as not seen do you think we should also move the node to the end of the bucket (the least recently seen position)?

return 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))
return true
else:
return false

func getNode*(r: RoutingTable, id: NodeId): Opt[Node] =
## Get the `Node` with `id` as `NodeId` from the routing table.
Expand Down
10 changes: 9 additions & 1 deletion tests/p2p/test_discoveryv5.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand Down
62 changes: 39 additions & 23 deletions tests/p2p/test_routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand All @@ -157,40 +157,55 @@ 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..<BUCKET_SIZE:
bucketNodes[i].seen = true
check table.addNode(bucketNodes[i]) == 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.replaceNode(table.nodeToRevalidate()) == false
table.getNode(bucketNodes[bucketNodes.high].id).get().seen == false

test "Double add":
let node = generateNode(PrivateKey.random(rng[]))
# bitsPerHop = 1 -> 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..<BUCKET_SIZE-1:
check table.addNode(node.nodeAtDistance(rng[], 256)) == Added
var bucketNodes = node.nodesAtDistance(rng[], 256, BUCKET_SIZE)
for i in 0..<BUCKET_SIZE - 1:
bucketNodes[i].seen = true
check table.addNode(bucketNodes[i]) == Added

check table.addNode(node.nodeAtDistance(rng[], 256)) == ReplacementAdded
check table.addNode(bucketNodes[^1]) == ReplacementAdded
# Check when adding again once the bucket is full
check table.addNode(doubleNode) == Existing

# Test if its order is preserved, there is one node in replacement cache
# which is why we run `BUCKET_SIZE` times.
for n in 0..<BUCKET_SIZE:
table.replaceNode(table.nodeToRevalidate())
# The first call to replaceNode should replace the node using the replacement cache
check table.replaceNode(bucketNodes[0]) == true

# The next BUCKET_SIZE - 1 replaceNode calls should only set seen to false
for i in 1..<BUCKET_SIZE:
check:
table.replaceNode(bucketNodes[i]) == false
table.getNode(bucketNodes[i].id).get().seen == false

let res = table.getNode(doubleNode.id)
# Test if its order is preserved
for i in 0..<BUCKET_SIZE - 1:
table.removeNode(table.nodeToRevalidate())

let nodeRes = table.getNode(doubleNode.id)
check:
res.isSome()
res.get() == doubleNode
nodeRes.isSome()
nodeRes.get() == doubleNode
nodeRes.get().seen == true
table.len == 1

test "Double replacement add":
Expand Down Expand Up @@ -236,8 +251,9 @@ suite "Routing Table Tests":
table.setJustSeen(n)

for n in bucketNodes:
table.replaceNode(table.nodeToRevalidate())
check (table.getNode(n.id)).isNone()
check:
table.replaceNode(n) == false
table.getNode(n.id).get().seen == false

test "Just seen replacement":
let node = generateNode(PrivateKey.random(rng[]))
Expand Down Expand Up @@ -286,7 +302,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.
Expand Down Expand Up @@ -386,10 +402,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":
Expand Down