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

Query distances from left/right of target distance #164

Closed
wants to merge 4 commits into from

Conversation

jtraglia
Copy link
Contributor

PR Description

So the lookup section of the spec isn't super clear in this regard, but I think findNodes isn't querying the ideal distances. Given a target distance of 10, it will query 10, 11, and 12. I think it should query 9, 10, 11. 9 is closer to 10 than 12. Slight distinction but it may help. Also, while I don't think it's common, the previous implementation doesn't work very well with a target distance of 255 and 256; it will only query one or two distances, instead of the typical three. Thoughts?

@jtraglia
Copy link
Contributor Author

I just realized that the previous implementation would query 4 distances, instead of 3 like I thought. I'll wait for your initial review before changing NUM_DISTANCES_TO_QUERY to 4, assuming that's what you want.

@ajsutton
Copy link
Contributor

I don't think that bit of the spec is actual relevant to the closest distances we're finding here. The spec is talking about selecting some number of nodes to query that are closest to the node we're looking for. At this point we've already done that selection and are now creating the query to send to that node. In theory we should be able to request neighbours from just a single target distance from our selected peer (the distance that would equate to the k-bucket the selected peer would have our target node in). However because some buckets can be fairly sparse, we actually send a FINDNODES request with three target distances, starting from the target distance and then moving further away (more distant buckets are more likely to have nodes in them).

You're right that the current algorithm is suboptimal when the target distance is close to the maximum as we will request fewer distances, although those more distant buckets are more likely to be full anyway because of the log based distance.

I suspect we want to keep prioritising higher distances in the request over lower ones because lower distances are less likely to contain nodes. It would be reasonable to add lower distances if we've reached MAX_DISTANCE though.

@jtraglia
Copy link
Contributor Author

Thanks for the detailed response. Makes sense to me. I'll close this.

@jtraglia jtraglia closed this Oct 31, 2022
@jtraglia jtraglia deleted the find-closer-nodes branch November 3, 2022 15:44
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.

2 participants