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.
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
Go IDONTWANT #553
base: master
Are you sure you want to change the base?
Go IDONTWANT #553
Conversation
Since we want to implement a priority queue later, we need to replace the normal sending channels with the new smart structures first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far looking good overall.
However, I think using a slice to back the queue is problematic at many levels as it might leak space with a growing array, and also hang on to too much space.
I would very much prefer using a dequeue for this.
} | ||
rpc, err := outgoing.Pop(ctx) | ||
if err != nil { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to log the error (at very low priority) if it is not a closed indicator.
UrgentPush allows you to push an rpc packet to the front of the queue so that it will be popped out fast.
Most importantly, this commit adds a new method called PreValidation to the interface PubSubRouter, which will be called right before validating the gossipsub message. In GossipSubRouter, PreValidation will send the IDONTWANT controll messages to all the mesh peers of the topics of the received messages.
The unused space will be freed eventually https://stackoverflow.com/a/26863706 |
This reverts commit dfd81e8.
I'm not really sure if we need dfd81e8 or not. I guess adding a non-standard flag to |
Yeah, let's not break compatibility with existing code, we don't need it. Any progress other than that? |
I haven't implemented the second half. Does the first half looks good to you? |
yes, looks reasonable so far. |
When receiving IDONTWANTs, the host should remember the message ids contained in IDONTWANTs using a hash map. When receiving messages with those ids, it shouldn't forward them to the peers who already sent the IDONTWANTs. When the maximum number of IDONTWANTs is reached for any particular peer, the host should ignore any excessive IDONTWANTs from that peer.
@vyzo I finished everything already. Sorry for the delay. I was in vacation and was a bit busy lately. Thank you for your patience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, but lets make the counter clear less aggressive.
@@ -56,7 +56,9 @@ var ( | |||
GossipSubGraftFloodThreshold = 10 * time.Second | |||
GossipSubMaxIHaveLength = 5000 | |||
GossipSubMaxIHaveMessages = 10 | |||
GossipSubMaxIDontWantMessages = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt that too low?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't think enough about this. I just copied it from GossipSubMaxIHaveMessages
. I think I will change it to 1,000 which will makes the IDONTWANT cache grows by 60,000 per minute (because GossipSubHeartbeatInterval
is 1 second)
@@ -1617,6 +1693,13 @@ func (gs *GossipSubRouter) clearIHaveCounters() { | |||
} | |||
} | |||
|
|||
func (gs *GossipSubRouter) clearIDontWantCounters() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats too aggressive and can clear the counters at unfortunate times.
Lets keep them for heartbeat or two, maybe 3 to match the cache sliding window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. When is the unfortunate time?
This only clear the counters not the cache that keeps the message ids from IDONTWANTs. So technically, the slower you clear the cache, the more you prevent the DoS attack. The faster you clear the cache, the more efficiency you have (because you process more IDONTWANTs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets say you just got an IDW, heartbeat clear, and then the message arrives... and you send it even though it is not wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and you send it even though it is not wanted.
This is not true.
The heartbeat clear only clears the counter, not the cache. The IDONTWANT is still in the cache so the message is not forwarded. In other words, the heartbeat clear clears peerdontwant
, while only unwanted
is used to decide whether to forward it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when is the cache cleared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you misunderatand.
We need to keep the IDW for some heartbeats and then clear them.
It is both for controlling memory use and for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reagardless, an LRA (least recently added) cache for IDW could work and keep memory bounded and it is possibly simpler.
However we need to consider cache sizes carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the problem here is a sybil attack, and we get to keep all the garbage indefinitely.
So no, this is not the correct solution.
We need to explicitly clean old IDWs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact we should go one step further and use a global limit for IDWs across all peers in the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is both for controlling memory use and for correctness.
Alright, got it. Thanks.
The message ids from IDONTWANT can be very large, so hashing it before keeping it in memory quite makes sense. The same thing is already implemented in nim-libp2p vacp2p/nim-libp2p#1090 |
sure, but it needs to be cleared, otherwise its memroy leak and dos waiting to happen. Or at best the feature breaks itself for long running nodes. I suggest for each IDW keep a counter of how many heartbeats it survived, and after 3 (to match the IHAVE ads) it should be cleared. |
Sending IDONTWANT
Handling IDONTWANT
max_idontwant_messages
(ignore the IDONWANT packets if the max is reached)Spec: libp2p/specs#548