fix: spamming peer is disconnected and seen cache doesn't grow indefinitely #1139
Conversation
| @@ -96,7 +96,7 @@ proc init*( | |||
| behaviourPenaltyWeight = -1.0, | |||
| behaviourPenaltyDecay = 0.999, | |||
| directPeers = initTable[PeerId, seq[MultiAddress]](), | |||
| disconnectBadPeers = false, | |||
| disconnectBadPeers = true, | |||
There was a problem hiding this comment.
What is the default in other implementations?
(I agree we should set this to true.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you very much. Both Nimbus and Waku set disconnectBadPeers to true
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm going to close this PR in favor of #1141
This part does not seem to be in the code? Edit: I just saw I suggest changing the commit message when merging to: fix: disconnect bad peers, add test incl check seen cache size is limited |
IMO this implies a fixed limit in the cache more than the current title. There's no limit to the number of messages sent in the test and thus the size of the cache, it is checking there's some mechanism that prevents this from keep going indefinitely. |
|
The spec https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#score-thresholds doesn't say a peer should be disconnected in this case, but rather that its RPC messages should be ignored. It's not GossipSub purpose to disconnect peers. If GossipSub thinks a peer isn't following the protocol, it should not interact or limit interaction with the peer, but not disconnect the peer completely. There might also be other protocols in the system. Disconnecting the peer seems a view where GossipSub is the most important protocol in the system and if a peer isn't following it, the peer isn't useful. Pragmatically, it might be true for our current systems, but I'm not sure it's right for the nim-libp2p lib in general. We can merge this PR, but revisit it later. |
That is OK. My point is mainly making it explicit that this is regarding the testing, and not limiting the seen cache. |
Yes. Thanks you. |
|
Closing this PR in favor of #1141 |
true.