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

Jump to bottom

Fluffy: Portal subnetwork peer ban list #3007

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

Fluffy: Portal subnetwork peer ban list #3007

wants to merge 3 commits into from

Conversation

bhartnett
Copy link
Contributor

@bhartnett bhartnett commented Jan 17, 2025 •
edited
Loading

This PR relates to #2809 and implements a per subnetwork peer ban list in the PortalProtocol type.

Changes completed:

  • Banned peers table added to portal protocol used to record banned peers until a given timeout.
  • Nodes can be banned for a given duration from the current time by nodeId.
  • The ban list is checked before returning neighbours from the routing table, and banned nodes are filtered out so that we don't attempt to connect to banned nodes.
  • The node lookup is updated to also filter out banned nodes when fetching nodes from peers.
  • The contentLookup and traceContentLookup is updated to filter out banned nodes when fetching nodes from peers for the case when no content is found and we return the closest nodes to the content.
  • When handling/receiving a portal message we check the ban list before processing it. If the sending peer is in the ban list then we don't process the message.
  • Added helpers templates to ban a node when offer validation or content lookup validation fails.
  • Ban nodes when offer and content lookup validations fail in the state network

Remaining work:

  • Ban nodes when validations fail in the beacon and history networks.
  • Ban misbehaving nodes for other scenarios and conditions in portal protocol.
  • Review and decide on the best ban timeouts for each type of ban reason.

bhartnett
bhartnett commented Jan 17, 2025
View reviewed changes
fluffy/network/wire/portal_protocol.nim
p: PortalProtocol, id: NodeId, k: int = BUCKET_SIZE, seenOnly = false
): seq[Node] =
# TODO: Maybe banned peers should be in the routing table and filtered out at that level
p.routingTable.neighbours(id, k, seenOnly).filterIt(not p.isBanned(it.id))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdeme Any thoughts on whether it would be a good idea to add the ban list into the routing table code in nim-eth? I believe the same routing table is used by the discv5 code and so it might be a good way to apply something similar at the discv5 level.

Copy link
Contributor

@kdeme kdeme Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it can be either in the routing table object or in PortalProtocol and DiscoveryProtocol. The latter would require a few more wrappers like you did now for neighbours.

What is more important I think is that banned nodes do not stay in the routing table. They should be removed from the routing table and not be allowed to re-added. That way the routing table gives space to valid nodes and a neighbours call gives better results.

I think achieving this might be slightly simpler when the ban list is part of routing table, not sure.

But yes, the ban list would also be useful for discv5 I think.

kdeme
kdeme reviewed Jan 17, 2025
View reviewed changes
fluffy/network/state/state_network.nim
@@ -109,6 +109,7 @@ proc getContent(
continue

validateRetrieval(key, contentValue).isOkOr:
n.portalProtocol.contentLookupFailedValidation(lookupRes.receivedFrom.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think when code reading it is clearer what this does when it would just be p.banPeer(nodeId, PeerBanDurationContentLookupFailedValidation). I feel the templates (or their names) mask to much which action is done here.

fluffy/network/wire/portal_protocol.nim
Comment on lines +129 to +131
# Ban durations for the banned peers table
PeerBanDurationContentLookupFailedValidation = 30.minutes
PeerBanDurationOfferFailedValidation = 60.minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

These could probably be the same duration always? Similar type of protocol violation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdeme kdeme kdeme left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bhartnett @kdeme