Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[GossipSub 1.2] IDONTWANT control message #548
[GossipSub 1.2] IDONTWANT control message #548
Changes from 10 commits
c75e81a
4fafe66
c6cfac9
9f9314c
9ba2b9c
7064203
31706a8
5c946ef
b9202a8
1fdaeb8
c7dfbe0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 guess this probably needs to be per-topic per heartbeat, rather than a total per heartbeat.
It seems it could be tied in with the scoring for mesh message delivery rate. I.e the more messages we are expecting per topic, the more IDONTWANT messages we would expect to receive.
One thought would be to add a behaviour penalty, similar to broken promises, if the number of IDONTWANT messages received from a peer exceeds the mesh message delivery rate.
We intend to implement this fairly soon. Perhaps we can leave the scoring penalty here for a future PR if we dont want to specify it now.
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.
Tracking
IDONTWANT
by topics would be perfect, however you don't know a message topic by its ID unless you already received this message.IDONTWANT
message is almost semantically equivalent toIHAVE
message, so probably should have a similar anti-spam protection mechanism?Probably it could make sense to set the 'reasonable default' to
maxIHaveLength * maxIHaveMessages
?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.
Is it a good reason to include topics to
IDONTWANT
and not care whatIHAVE
andIWANT
are doing?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.
Including a topic would increase
IDONTWANT
traffic 2-3 times (messageID is 20 bytes, topic could be up to around 40 bytes). Not sure if it's worth just to enable more advanced spam protection...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.
Please also consider that
IHAVE
is sent on heartbeat with a batch of messages which may be grouped by topics and the topic overhead here could not be that significant.IDONTWANT
on the other hand is intended to be flushed immediately and would most likely contain just a single messageID so the topic overhead would be more significantThere 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 suspect it might be valuable to specify it - ie effectively, after mcache expires, the message should no longer exist and implementations should be able to rely on them being "resent" if they resurface after that time - this more faithfully keeps the protocol consistent in this aspect
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.
@ppopth I was meaning this HashDoS attack.
The messageIDs in
IDONTWANT
are not validated: neither upon receive nor later (as forIHAVE
). Thus an adversary may generate and send significant amount of messageIDs which yields the same hashCode in the context ofunwanted
hash set. That would result in a DoS on the receiver's side.Actually this attack vector could be addressed on the implementation side pretty easily. I just wanted to add a 'warning note' for implementers into the spec
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.
@Nashatyrev Got it. Thank you so much.
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.
@arnetheduck Good point!
I believe as far as the
messageId
function is application specific, the validation of amessageId
is left for an application responsibility. I believe Ethereum clients should all have such validationThere 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'm not convinced this is the case, ie at least in nimbus, we don't validate message id:s (only messages). There's a custom message id generation feature, but this again does not validate message id:s themselves.
This PR represents the first time we receive message id:s that we're expected to store / keep track of - all others are either generated from actual messages or ephemeral.
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.
Concerns about spam attacks triggering amplified IDONTWANT spam?
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.
This doesn't look like a feasible attack vector to me:
IDONTWANT
is primarily intended for larger messages, so the cumulative size of resultingIDONTWANT
messages is expected to be significantly smaller than the original messageIDONTWANT
spamming it would be pretty quickly banned due to negative scoringmax_idontwant_messages
limit as the last resort