search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

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

Open diegomrsantos opened 4 days ago

diegomrsantos commented 4 days ago
kaiserd commented 4 days ago

seen cache doesn't grow indefinitely

This part does not seem to be in the code?

Edit: I just saw check gossip1.seen.len < 1000 in the test. I assumed the title indicates this PR also generally limits he seen cache size.

I suggest changing the commit message when merging to:

fix: disconnect bad peers, add test incl check seen cache size is limited

diegomrsantos commented 4 days ago

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.

diegomrsantos commented 4 days ago

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.

kaiserd commented 3 days ago

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.

That is OK. My point is mainly making it explicit that this is regarding the testing, and not limiting the seen cache. Right now the title reads as: this PR fixes the fact that the seen cache grows indefinitely

kaiserd commented 3 days ago

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.

Yes. Thanks you. Let's add a comment to the "disconnectBadPeers = true" line.