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: use accurate bucket logic #64

Merged
merged 1 commit into from
Apr 2, 2020
Merged

fix: use accurate bucket logic #64

merged 1 commit into from
Apr 2, 2020

Conversation

Stebalien
Copy link
Member

This reverts an optimization where we fudged the closer peers when returning responses to users when we were within a power of two to the target.

@aarshkshah1992
Copy link
Contributor

@Stebalien What's the motivation on this i.e. what do we gain by not adding peers to the left ?

@Stebalien
Copy link
Member Author

We're adding peers from all buckets. The change is that, instead of adding peers from buckets from the right until we have enough peers, we just add all peers from buckets to the right, then sort them based on their distance to the target, then trim back down to K peers.

This is strictly more accurate as all peers to the right are the same order of magnitude (same CPL).

@aarshkshah1992 aarshkshah1992 self-requested a review April 1, 2020 12:52
@aarshkshah1992
Copy link
Contributor

@Stebalien LGTM. Please can you merge this ?

Copy link

@petar petar left a comment

Choose a reason for hiding this comment

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

LGTM


for i := cpl + 1; i < len(rt.buckets) && pds.Len() < count; i++ {
pds.appendPeersFromList(rt.buckets[i].list)
if pds.Len() < count {
Copy link

Choose a reason for hiding this comment

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

Yep. This is more accurate. And sorting happens later, I assume.

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Apr 1, 2020
@Stebalien
Copy link
Member Author

I'm blocking this on #66. Once we land that, we should store the pre-hashed peer ID in the routing table. Otherwise, if we're missing a single peer in the closest bucket, we'll have to hash all the peers to the right when sorting them.

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented Apr 2, 2020

@Stebalien

PR to store pre-hashed peerId and re-use them in rt.NearestPeers at #67.

@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label Apr 2, 2020
This reverts an optimization where we fudged the closer peers when returning
responses to users when we were within a power of two to the target.
@Stebalien Stebalien merged commit 3d3bf8c into master Apr 2, 2020
@Stebalien Stebalien deleted the fix/accurate-buckets branch April 2, 2020 22:37
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.

3 participants