feat(dot/sync): prevent block sync request spamming #3953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EclesioMeloJunior
requested review from
edwardmack,
timwu20,
jimjbrettj,
kishansagathiya,
dimartiro and
axaysagathiya
as code owners
EclesioMeloJunior
added
C-simple
jimjbrettj
reviewed
May 16, 2024
jimjbrettj
approved these changes
May 16, 2024
timwu20
reviewed
May 23, 2024
|
|
||
| numOfRequests := s.seenBlockSyncRequests.Get(requestHash) | ||
|
|
||
| if numOfRequests > maxNumberOfSameRequestPerPeer { |
There was a problem hiding this comment.
Suggested change
| if numOfRequests > maxNumberOfSameRequestPerPeer { | |
| if numOfRequests + 1 > maxNumberOfSameRequestPerPeer { |
Shouldn't we increment this now to report the peer and exit early instead of waiting for the 3rd message spam to do this?
There was a problem hiding this comment.
but then Gossamer will punish w/o reason, I'm mean, if someone reach the limit that is not a bad thing so should not be punished, however if it goes beyond the limit then it should be punished and have its reputation decreased. That's why we should wait for the third message imo
timwu20
removed request for
dimartiro,
edwardmack,
kishansagathiya and
axaysagathiya
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
/sync/2protocol IDsyncpackage, which is responsible for responding any sync requests takes the peer ID as well as the request message and hash them and insert in a LRU caching layer with the number of this peer and request was seen, if it reaches the max number then the peer is reported and the message is ignoredTests
WIP
Issues
Primary Reviewer
@timwu20