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

fix: spamming peer is disconnected and seen cache doesn't grow indefinitely #1139

Closed
wants to merge 2 commits into from
Closed

fix: spamming peer is disconnected and seen cache doesn't grow indefinitely #1139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1298f9
fix: spamming peer is disconnected and seen cache doesn't grow indefi…
diegomrsantos Jun 26, 2024
40ffd9e
formatting
diegomrsantos Jun 26, 2024
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
2 changes: 1 addition & 1 deletion libp2p/protocols/pubsub/gossipsub.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ proc init*(
behaviourPenaltyWeight = -1.0,
behaviourPenaltyDecay = 0.999,
directPeers = initTable[PeerId, seq[MultiAddress]](),
disconnectBadPeers = false,
disconnectBadPeers = true,
Copy link
Collaborator

@kaiserd kaiserd Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the default in other implementations?
(I agree we should set this to true.)

Copy link
Contributor

Choose a reason for hiding this comment

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

The disconnection feature was a temporary hack because disconnection of a bad peer typically belongs in a different layer than gossipsub - what should be happening is that an event is raised to which the application reacts by taking appropriate action.

Usually this will involve not just disconnecting the peer but also adding them to a temporary ban list to prevent them from reconnecting within a short period of time (anything from a minute to an hour depending on the setup, doesn't greatly matter) - it might also involve judging the peer on other metrics than gossipsub.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in terms of working on this issue, it would involve also working on the primary users of libp2p and making sure that the strategy of handling poor peers is integrated holistically with accompanying changes in nimbus-eth2 etc - I don't think making this change in libp2p on its own without making sure it fits a bigger picture makes much sense, ie the merit of the change should be evaluated in the context of how it gets used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you very much. Both Nimbus and Waku set disconnectBadPeers to true

Copy link
Contributor Author

@diegomrsantos diegomrsantos Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

to prevent them from reconnecting within a short period of time

This has been implemented here https://github.com/vacp2p/nim-libp2p/pull/909/files#diff-abbf987f19a24c9940cd01bde79a1b1e0819f1c3b6d7ba2efa04ad680d022c78

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to close this PR in favor of #1141

enablePX = false,
bandwidthEstimatebps = 100_000_000, # 100 Mbps or 12.5 MBps
overheadRateLimit = Opt.none(tuple[bytes: int, interval: Duration]),
Expand Down
3 changes: 3 additions & 0 deletions libp2p/protocols/pubsub/timedcache.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,8 @@ func addedAt*[K](t: var TimedCache[K], k: K): Moment =

default(Moment)

func len*(t: TimedCache): int =
t.entries.len

func init*[K](T: type TimedCache[K], timeout: Duration = Timeout): T =
T(timeout: timeout)
59 changes: 58 additions & 1 deletion tests/pubsub/testgossipsub.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

{.used.}

import sequtils, options, tables, sets, sugar
import sequtils, options, tables, sets, sugar, random
import chronos, stew/byteutils, chronos/ratelimit
import chronicles
import metrics
Expand Down Expand Up @@ -1138,3 +1138,60 @@ suite "GossipSub":
await allFuturesThrowing(node0.switch.stop(), node1.switch.stop())

await allFuturesThrowing(nodesFut.concat())

asyncTest "spamming peer is disconnected and seen cache doesn't grow indefinitely":
proc dumbMsgIdProvider(m: Message): Result[MessageId, ValidationResult] =
ok(m.data)

let nodes = generateNodes(2, gossip = true, msgIdProvider = dumbMsgIdProvider)

discard await allFinished(nodes[0].switch.start(), nodes[1].switch.start())

await subscribeNodes(nodes)

proc handle(topic: string, data: seq[byte]) {.async.} =
discard

let gossip0 = GossipSub(nodes[0])
let gossip1 = GossipSub(nodes[1])

gossip0.subscribe("foobar", handle)
gossip1.subscribe("foobar", handle)
await waitSubGraph(nodes, "foobar")

# Avoid being disconnected by failing signature verification
gossip0.verifySignature = false
gossip1.verifySignature = false

let topic = "foobar"
proc execValidator(
topic: string, message: messages.Message
): Future[ValidationResult] {.raises: [].} =
let res = newFuture[ValidationResult]()
res.complete(ValidationResult.Reject)
res

gossip0.addValidator(topic, execValidator)
gossip1.addValidator(topic, execValidator)

randomize()
let peer = gossip0.mesh[topic].toSeq[0]
while gossip0.switch.isConnected(peer.peerId):
var data = newSeq[byte](32)
for i in 0 ..< data.len:
data[i] = rand(256).uint8
# creating a random message and returning it in dumbMsgIdProvider will make the id unique
let msg = RPCMsg(messages: @[Message(topic: topic, data: data)])
gossip0.broadcast(@[peer], msg, isHighPriority = true)
await sleepAsync(1.milliseconds)

# we also check that the spamming peer can't connect to us again
expect(DialFailedError):
await nodes[0].switch.connect(
nodes[1].switch.peerInfo.peerId, nodes[1].switch.peerInfo.addrs
)

check gossip0.switch.isConnected(peer.peerId) == false
check gossip1.seen.len < 1000

await stopNodes(nodes)
Loading