Skip to content
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

Discv5 Routing Table: Add support for banning nodes #768

Merged
merged 7 commits into from
Jan 24, 2025
Merged

Discv5 Routing Table: Add support for banning nodes #768

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c0c3ba0
Add banned nodes to routing table.
bhartnett Jan 20, 2025
59b17d1
Add tests.
bhartnett Jan 21, 2025
6cdd9cd
Fix test.
bhartnett Jan 21, 2025
e6a542c
Reduce sleep time in test.
bhartnett Jan 22, 2025
52b72df
Cleanup expired bans from banned nodes table when adding a new ban.
bhartnett Jan 22, 2025
f672d72
Make cleanupExpiredBans public so it can be called independently of b…
bhartnett Jan 22, 2025
1e8110c
Init bannedNodes field.
bhartnett Jan 23, 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
45 changes: 44 additions & 1 deletion eth/p2p/discoveryv5/routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import
../../net/utils,
"."/[node, random2, enr]

export results
export results, chronos.timer

declareGauge routing_table_nodes,
"Discovery routing table nodes", labels = ["state"]
Expand Down Expand Up @@ -47,6 +47,10 @@ type
## replacement caches.
distanceCalculator: DistanceCalculator
rng: ref HmacDrbgContext
bannedNodes: Table[NodeId, chronos.Moment] ## Nodes can be banned from the
## routing table for a period until the timeout is reached. Banned nodes
## are removed from the routing table and not allowed to be included again
## until the timeout expires.

KBucket = ref object
istart, iend: NodeId ## Range of NodeIds this KBucket covers. This is not a
Expand Down Expand Up @@ -95,6 +99,7 @@ type
ReplacementAdded
ReplacementExisting
NoAddress
Banned

# xor distance functions
func distance*(a, b: NodeId): UInt256 =
Expand Down Expand Up @@ -189,6 +194,41 @@ func ipLimitDec(r: var RoutingTable, b: KBucket, n: Node) =
b.ipLimits.dec(ip)
r.ipLimits.dec(ip)

func getNode*(r: RoutingTable, id: NodeId): Opt[Node]
proc replaceNode*(r: var RoutingTable, n: Node)

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
## from the routing table and replaced using a node from the replacement cache.

let banTimeout = now(chronos.Moment) + period

if r.bannedNodes.contains(nodeId):
let existingTimeout = r.bannedNodes.getOrDefault(nodeId)
if existingTimeout < banTimeout:
r.bannedNodes[nodeId] = banTimeout
else:
r.bannedNodes[nodeId] = banTimeout

# Remove the node from the routing table
let node = r.getNode(nodeId)
if node.isSome():
r.replaceNode(node.get())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we replace the node as I've done here or simply remove it without using the replacement cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace is the way to go 👍

In general we never really use remove AFAIK (maybe in testing?), always use the replacement cache as that cache is not actively used to search for nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok great, I'll leave it as it is then.


proc isBanned*(r: var RoutingTable, nodeId: NodeId): bool =
if not r.bannedNodes.contains(nodeId):
return false

let
currentTime = now(chronos.Moment)
banTimeout = r.bannedNodes.getOrDefault(nodeId)
if currentTime < banTimeout:
return true

# Peer is in bannedNodes table but the time period has expired
r.bannedNodes.del(nodeId)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is sufficient for banned nodes clean-up.

A node could be started, send 1 invalid message and then disappear from the network? Repeat this continuously and there is a small but increasing mem leak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good point. Maybe a better implementation would be to clean up expired nodes for the entire routing table inside the call to addNode.

false

proc add(k: KBucket, n: Node) =
k.nodes.add(n)
routing_table_nodes.inc()
Expand Down Expand Up @@ -343,6 +383,9 @@ proc addNode*(r: var RoutingTable, n: Node): NodeStatus =
if n == r.localNode:
return LocalNode

if r.isBanned(n.id):
return Banned

let bucket = r.bucketForNode(n.id)

## Check if the node is already present. If so, check if the record requires
Expand Down
69 changes: 69 additions & 0 deletions tests/p2p/test_routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{.used.}

import
std/os,
unittest2,
../../eth/common/keys, ../../eth/p2p/discoveryv5/[routing_table, node, enr],
./discv5_test_helper
Expand Down Expand Up @@ -561,3 +562,71 @@ suite "Routing Table Tests":
# there may be more than one node at provided distance
check len(neighboursAtLogDist) >= 1
check neighboursAtLogDist.contains(n)

test "Banned nodes: banned node cannot be added":
let
localNode = generateNode(PrivateKey.random(rng[]))
node1 = generateNode(PrivateKey.random(rng[]))
node2 = generateNode(PrivateKey.random(rng[]))

var table = RoutingTable.init(localNode, 1, DefaultTableIpLimits, rng = rng)

# Can add a node that is not banned
check:
table.contains(node1) == false
table.isBanned(node1.id) == false
table.addNode(node1) == Added
table.contains(node1) == true
table.isBanned(node1.id) == false

# Can ban a node that exists in the routing table
table.banNode(node1.id, 1.minutes)
check:
table.contains(node1) == false # the node is removed when banned
table.isBanned(node1.id) == true
table.addNode(node1) == Banned # the node cannot be added while banned
table.contains(node1) == false
table.getNode(node1.id).isNone()
table.isBanned(node1.id) == true

# Can ban a node that doesn't yet exist in the routing table
check:
table.contains(node2) == false
table.isBanned(node2.id) == false

table.banNode(node2.id, 1.minutes)
check:
table.contains(node2) == false
table.isBanned(node2.id) == true
table.addNode(node2) == Banned # the node cannot be added while banned
table.contains(node2) == false
table.getNode(node2.id).isNone()
table.isBanned(node2.id) == true

test "Banned nodes: nodes with expired bans can be added":
let
localNode = generateNode(PrivateKey.random(rng[]))
node1 = generateNode(PrivateKey.random(rng[]))
node2 = generateNode(PrivateKey.random(rng[]))

var table = RoutingTable.init(localNode, 1, DefaultTableIpLimits, rng = rng)

check table.addNode(node1) == Added
table.banNode(node1.id, 1.nanoseconds)
table.banNode(node2.id, 1.nanoseconds)

sleep(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is 100 ms right? can be just value of 1 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes true, I'll update. I guess it will speed up the test.


# Can add nodes for which the ban has expired
check:
table.contains(node1) == false
table.isBanned(node1.id) == false
table.addNode(node1) == Added
table.contains(node1) == true
table.isBanned(node1.id) == false

table.contains(node2) == false
table.isBanned(node2.id) == false
table.addNode(node2) == Added
table.contains(node2) == true
table.isBanned(node2.id) == false
Loading