Skip to content
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

refactor(share/shrex-nd): blacklist peers that send invalid data #2231

Merged
merged 4 commits into from May 25, 2023

Conversation

walldiss
Copy link
Member

Overview

Shrex should blacklist peers that send bad data.

@walldiss walldiss added area:p2p kind:refactor Attached to refactoring PRs labels May 19, 2023
@walldiss walldiss requested a review from renaynay as a code owner May 19, 2023 12:01
@walldiss walldiss self-assigned this May 19, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2231 (2db5afe) into main (e8ffd64) will increase coverage by 0.11%.
The diff coverage is 42.10%.

@@            Coverage Diff             @@
##             main    #2231      +/-   ##
==========================================
+ Coverage   55.81%   55.92%   +0.11%     
==========================================
  Files         216      216              
  Lines       14130    14141      +11     
==========================================
+ Hits         7886     7909      +23     
+ Misses       5454     5442      -12     
  Partials      790      790              
Impacted Files Coverage Δ
share/availability/discovery/discovery.go 73.85% <0.00%> (-2.40%) ⬇️
share/getters/shrex.go 77.12% <0.00%> (-1.55%) ⬇️
share/p2p/shrexnd/client.go 69.42% <ø> (+1.42%) ⬆️
share/availability/discovery/metrics.go 47.05% <57.14%> (+0.63%) ⬆️
share/availability/discovery/backoff.go 86.79% <100.00%> (+1.07%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also very good because now, if we have malicious peer who sends us bad data, it won't short-circuit the request, it'll just blacklist and keep going until it gets good shares. However, there's an issue with the err inside of GetSharesByNamespace in shrex getter: it's only ever returned in the case of not being able to get a peer from peer manager, but not in the case of a context timeout. Why is this the case? Shouldn't we errors.Join the context err as well during timeout and return it? esp since we doing have any checks for ctx deadline exceeded in cascade getter

Also this would be a very good candidate for swamp test.

@walldiss
Copy link
Member Author

However, there's an issue with the err inside of GetSharesByNamespace in shrex getter: it's only ever returned in the case of not being able to get a peer from peer manager, but not in the case of a context timeout. Why is this the case? Shouldn't we errors.Join the context err as well during timeout and return it? esp since we doing have any checks for ctx deadline exceeded in cascade getter

Nice find, we used to have all errors joined before relatively recent hotfix this one I'll open another PR to fix it, since it is unrelated to this change and there is also same problem in GetEDS

@renaynay
Copy link
Member

@walldiss needs rebase

Wondertan
Wondertan previously approved these changes May 23, 2023
@walldiss walldiss enabled auto-merge (squash) May 25, 2023 09:54
@walldiss walldiss merged commit cf1c2ca into celestiaorg:main May 25, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:p2p kind:refactor Attached to refactoring PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants