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

Send repairman shreds to the repair socket #6671

Merged

Conversation

sagar-solana
Copy link
Contributor

Problem

Repairman protocol sends shreds to a node's TVU. The node then sends those shreds through Turbine causing unnecessary packet duplication and traffic.

Summary of Changes

Advertise the repair socket in Contact Info. Send repairman responses to the repair socket so that Validators don't send them through turbine again.

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #6671 into master will increase coverage by 2%.
The diff coverage is 78.2%.

@@           Coverage Diff            @@
##           master   #6671     +/-   ##
========================================
+ Coverage    68.1%   70.2%     +2%     
========================================
  Files         220     213      -7     
  Lines       47864   46480   -1384     
========================================
+ Hits        32638   32656     +18     
+ Misses      15226   13824   -1402

@aeyakovenko
Copy link
Member

@sagar-solana this is easily broken. Any validators can change this code and send the shreds to a different socket. We need to do a turbine path check and mark the packet as sent.

@sagar-solana
Copy link
Contributor Author

@sagar-solana this is easily broken. Any validators can change this code and send the shreds to a different socket. We need to do a turbine path check and mark the packet as sent.

Yeah this obviously doesn't fix the security aspect that we're already aware of. It also doesn't make it worse. It does however help with some of the networking issues we've had. I feel like we need this now and when we have the turbine path lookup we can fix it.

Also repairman clearly needs its own thing since it's targeted. It's only for this node and should not be turbine bound in anyway. For now since we already filter out repair responses, the fix was to send repairman shreds to the same addr.

@aeyakovenko
Copy link
Member

Is there a security issue tracking it? Can you make it a blocker for slp1? It would be trivial to dos the network otherwise and no way to detect it.

@sagar-solana
Copy link
Contributor Author

@aeyakovenko done!
#6672

@sagar-solana
Copy link
Contributor Author

Does this look ok for the time being @aeyakovenko ?

@sagar-solana sagar-solana merged commit 2d67962 into solana-labs:master Nov 1, 2019
@sagar-solana sagar-solana deleted the fix_repairman_forwards branch November 1, 2019 01:23
mergify bot pushed a commit that referenced this pull request Nov 1, 2019
solana-grimes pushed a commit that referenced this pull request Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants