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

Fix Turbine making Port based decisions #7774

Closed
wants to merge 4 commits into from

Conversation

sagar-solana
Copy link
Contributor

Problem

Turbine chooses whether or not to forward a packet to neighbors and children or just children based on which port the packet arrived on. Turbine also drops any packets that arrive on the repair port.

This port based filtering is a problem because it can easily be spoofed and cause selective propagation of packets through a cluster

Summary of Changes

A Validator will now always perform a "wide"(to neighbors and children) retransmit if it was on the critical path for a packet.

Validators can easily check if they are "on the critical path" for a given packet (since the Turbine tree is shuffled per packet) by looking at their index after the shuffle. If their Index is a multiple of the Turbine Fanout, that means they must be on the critical path.

First of many partial fixes for #6672

@sagar-solana
Copy link
Contributor Author

sagar-solana commented Jan 13, 2020

Pending a testnet run + some more reasoning through it to make sure this doesn't break anything.

pgarg66
pgarg66 previously approved these changes Jan 13, 2020
@mergify mergify bot dismissed pgarg66’s stale review January 14, 2020 02:45

Pull request has been modified.

@sagar-solana
Copy link
Contributor Author

Oh awesome, looks like it broke the retransmit bench because Turbine is more secure now and refuses to propagate packets that are not in the critical path.

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #7774 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master   #7774     +/-   ##
========================================
- Coverage    81.8%   81.6%   -0.3%     
========================================
  Files         238     241      +3     
  Lines       50993   50973     -20     
========================================
- Hits        41730   41604    -126     
- Misses       9263    9369    +106

@sagar-solana
Copy link
Contributor Author

This appears to cause some odd behavior on testnet. Need to figure out why. UDP traffic is comparable but I think there's a corner case or something that's preventing data from reaching everyone properly. Going to hold off on merging this till I can figure out if there's a bug in the logic.

@stale
Copy link

stale bot commented Jan 21, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 21, 2020
@sagar-solana sagar-solana removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 21, 2020
@stale
Copy link

stale bot commented Jan 28, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 28, 2020
@sagar-solana sagar-solana removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 30, 2020
@garious
Copy link
Contributor

garious commented Feb 4, 2020

@sagar-solana, I see @aeyakovenko approved this PR. Still working on it or ready to land?

@sagar-solana
Copy link
Contributor Author

@garious It's not working as expected and I haven't been able to identify why. I can take the PR down and re-upload it when it works if you prefer that.

@garious
Copy link
Contributor

garious commented Feb 4, 2020

Nah, you can leave it. Just needed to know its status. Thanks!

@stale
Copy link

stale bot commented Feb 11, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 11, 2020
@sagar-solana sagar-solana removed the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 13, 2020
@stale
Copy link

stale bot commented Feb 19, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 19, 2020
@stale
Copy link

stale bot commented Feb 26, 2020

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants