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 1 commit
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
39 changes: 28 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,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
Copy link
Contributor

@kdeme kdeme May 6, 2025

Choose a reason for hiding this comment

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

nit: I find it slightly cleaner to just return the resulting bool directly in each if/else clause

Copy link
Contributor Author

@bhartnett bhartnett May 6, 2025

Choose a reason for hiding this comment

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

Sure, will update.


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)?


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.
Expand Down
16 changes: 8 additions & 8 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 @@ -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()
Copy link
Contributor

@kdeme kdeme May 6, 2025

Choose a reason for hiding this comment

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

could add a check on the n.seen = false here.

Copy link
Contributor Author

@bhartnett bhartnett May 6, 2025

Choose a reason for hiding this comment

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

Good point, will do.


test "Double add":
let node = generateNode(PrivateKey.random(rng[]))
Expand Down Expand Up @@ -191,7 +191,7 @@ suite "Routing Table Tests":
check:
res.isSome()
res.get() == doubleNode
table.len == 1
table.len == 16
Copy link
Contributor

@kdeme kdeme May 6, 2025

Choose a reason for hiding this comment

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

There was a part of this test that checks if order of revalidations is preserved by checking at the end if the latest added node (doubleNode) is still there (the one not revalidated and thus replaced).

As now all nodes remain, this getNode will for sure always pass.

I think same test can be achieved by checking the seen value for each node.


test "Double replacement add":
let node = generateNode(PrivateKey.random(rng[]))
Expand Down Expand Up @@ -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()
Copy link
Contributor

@kdeme kdeme May 6, 2025

Choose a reason for hiding this comment

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

similar item as above, the getNode is no longer a good test. I think we need to check seen value also.


test "Just seen replacement":
let node = generateNode(PrivateKey.random(rng[]))
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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":
Expand Down
Loading