-
Notifications
You must be signed in to change notification settings - Fork 12
ETCM-416: Limit entries from the same subnet in the K-table #109
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.
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
Conversation
| // Find any Peer records that correspond to this ID. | ||
| val peers = | ||
| nodeMap.get(peerId).map(node => Peer(node.id, toAddress(node.address))).toSeq ++ | ||
| lastPongTimestampMap.keys.filter(_.id == peerId).toSeq ++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems wasteful that we are itereating lastPongTimestampMap and bondingResultsMap maps two times here when finding all records of peer, and later when fitering this peer out. From what i known this collections can be fairly large, maybe we could do it in one pass ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can change that. But this method won't be actually called, I just kept it for backwards compatibility with KRouter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| val blc = bucketLevelCounts |+| Map(idx -> Map(ip -> 1)) | ||
|
|
||
| val overTheLimit = | ||
| limits.prefixLength > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could add some comments about each case or export this to separate funcion with some docs as this logic is little dense ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, split it out into several methods.
| case counts if counts(ip) <= 0 => counts - ip | ||
| case counts => counts | ||
| } | ||
| val blc = bucketLevelCounts |+| Map(idx -> Map(ip -> -1)) match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could add some comments about each case or export this to separate funcion with some docs as this logic is little dense ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, split it out into several methods.
The PR implements what
gethdoes to limit the entries in the K-table from the same subnet, which is a variation of Countermeasure 2 from the eclipse attack paper. In particular, by default it will not add a peer to the k-table if doing so would violate the limits. The default limits are that there cannot be more than 2 peers from the same subnet in any k-bucket, or more than 10 across all buckets, where the subnet is defined as the fist 24 bits of the IP address. All these values are configurable.Note that the nodes and ENR records are still saved by the service, the only difference is that these values will not be inserted into the k-table, which should prevent an attacker from filling up the k-table with nodes running on the same machine, or the same subnet, thus hijacking all
FindNoderequests, making our node unable to discover honest peers. Because of this, Mantis should also have some measures to limit the number of TCP connections going to the same IP or subnet, because if it usesgetNodesit will still get every discovered peer.