Improve spam protections #284
Conversation
vyzo
force-pushed
the
feat/spam-protections
branch
2 times, most recently
from
de9ed51 to
cb1c181
Compare
vyzo
force-pushed
the
feat/spam-protections
branch
from
cb1c181 to
dfb15de
Compare
| // to protect from IHAVE floods. You should adjust this value from the default if your | ||
| // system is pushing more than 5000 messages in GossipSubHistoryGossip heartbeats; with the | ||
| // defaults this is 1666 messages/s. | ||
| GossipSubMaxIHaveLength = 5000 |
There was a problem hiding this comment.
Related question: do we track inflight IWANTs? My rationale is that if we are poorly connected to the mesh and we recurrently only hear about a subset of messages via gossip, we might end up asking many peers for the same message at once, unless we deduplicate inflight requests.
If we don't, we'd be selfishly causing more traffic than necessary. However, I acknowledge this is tricky logic to implement, because it needs to factor in many concerns:
- What if the peer we ask never sends back the message?
- Do we queue up other candidates we know we can fall back on?
- What is the grace period? And how does it relate to window sliding? (by the time we fall back to the next candidate, the message might have slid away from the cache)
- Memory footprint of this new state to maintain.
There was a problem hiding this comment.
No, we are not tracking in flight IWANTs.
I agree that we do want to track them, and it will make it much easier to respond to distributed IHAVE floods as well.
I'll open an issue so that we can tackle this, as it is rather complex and I want to think a bit about it.
| gossip: make(map[peer.ID][]*pb.ControlIHave), | ||
| control: make(map[peer.ID]*pb.ControlMessage), | ||
| backoff: make(map[string]map[peer.ID]time.Time), | ||
| peerhave: make(map[peer.ID]int), |
There was a problem hiding this comment.
note for reviewers: peerhave is what was added here. GitHub diffs don't make it easy to hunt down the change :D
| // refresh the backoff | ||
| gs.addBackoff(p, topic) | ||
| // check the flood cutoff -- is the GRAFT coming too fast? | ||
| floodCutoff := expire.Add(GossipSubGraftFloodThreshold - GossipSubPruneBackoff) |
There was a problem hiding this comment.
Another (possibly more correct) way of implementing this could be sending back the desired backoff time to a peer at the time of PRUNE, and penalising peers who do not honour it?
There was a problem hiding this comment.
Yeah, it would be nice of us to send the backoff period and it makes things more robust for peers with different values of it.
It will take a protocol change however, so let me consider a bit and do it in a follow up PR.
There was a problem hiding this comment.
LGTM 👍
In Bitswap we try to "keep peers busy" when serving blocks, meaning we send messages to whichever peer has the least amount of data in its send queue. It may make sense to follow an analogous approach here, whereby when deciding how many messages to send to each peer we prioritize them by score, and then round-robin
Currently we return a PRUNE for every GRAFT we are rejecting, even if the GRAFTs are coming too fast, in flood style. Similarly, we are asking for every (unknown) message advertised in an IHAVE even if these are too many, and potentially a flood.
This patch adds rudimentary protections for both, closing the (single node) flood holes:
GossipSubMaxIHaveLength; default is 5000 message ids, which would take some 200KB.We also only accept up to
GossipSubMaxIHaveMessagesIHAVE messages per heartbeat from each peer.GossipSubGraftFloodThresholdparameter. We also add an extended backoff penalty to avoid GRAFTING ourselves on this peer for a while.Also merges in #283 which tests the spam protections.