Discv5 Routing Table: Add support for banning nodes #768
Conversation
|
@kdeme FYI, I didn't implement any filter in the |
| # Remove the node from the routing table | ||
| let node = r.getNode(nodeId) | ||
| if node.isSome(): | ||
| r.replaceNode(node.get()) |
There was a problem hiding this comment.
Should we replace the node as I've done here or simply remove it without using the replacement cache?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok great, I'll leave it as it is then.
| # Remove the node from the routing table | ||
| let node = r.getNode(nodeId) | ||
| if node.isSome(): | ||
| r.replaceNode(node.get()) |
There was a problem hiding this comment.
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.
tests/p2p/test_routing_table.nim
Outdated
| table.banNode(node1.id, 1.nanoseconds) | ||
| table.banNode(node2.id, 1.nanoseconds) | ||
|
|
||
| sleep(100) |
There was a problem hiding this comment.
This is 100 ms right? can be just value of 1 then?
There was a problem hiding this comment.
Yes true, I'll update. I guess it will speed up the test.
| # Peer is in bannedNodes table but the time period has expired | ||
| r.bannedNodes.del(nodeId) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes good point. Maybe a better implementation would be to clean up expired nodes for the entire routing table inside the call to addNode.
|
@kdeme I've updated this PR based on your suggestions. Ready for another review. |
| r.replaceNode(node.get()) | ||
|
|
||
| # Remove all expired bans from the banned nodes table | ||
| r.cleanupExpiredBans() |
There was a problem hiding this comment.
I think having the cleanup here is potentially worse than the actual mem leak.
Allowing this to run in the banNode call effectively puts it in an attacker's power to run this cleanup. While at the same time the attacker can make it more expensive by "adding bans".
I think that this type of clean-up call should always be run independently of any peer node actions.
There was a problem hiding this comment.
I see so the concern in this case is the computational overhead of looping over the table rather than the list growing?
Did you have a suggestion for where you think the cleanup should happen?
There was a problem hiding this comment.
I've now removed the call to cleanupExpiredBans from the banPeer function and made it public so it can be called at a high layer.
This PR adds support for banning nodes from the Discv5 routing table. This feature will be used by both Discv5 and the portal subnetworks in Fluffy. See the related task here: status-im/nimbus-eth1#2809
The updates to the Discv5 protocol to implement banning of peers will be included in a separate PR.