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.

Sign up for GitHub

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

Jump to bottom

fix: add more checks to handleFindNode #216

Merged
merged 3 commits into from
Nov 2, 2022
Merged

fix: add more checks to handleFindNode #216

merged 3 commits into from
Nov 2, 2022

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Nov 1, 2022

This PR fixes a few things in the FINDNODE request handler:

  • We should filter out invalid/duplicate distances.
  • According to the spec, the recommended result limit is 16, not 15.
  • When returning an empty response, total should be one.

@jtraglia jtraglia requested a review from a team as a code owner November 1, 2022 20:29
wemeetagain
wemeetagain reviewed Nov 2, 2022
View reviewed changes
src/service/service.ts Outdated Show resolved Hide resolved
@wemeetagain
Copy link
Member

When returning an empty response, total should be one

Why is that?

@jtraglia
Copy link
Contributor Author

jtraglia commented Nov 2, 2022

When returning an empty response, total should be one

Why is that?

Well total is the number of packets being sent/received. If you're sending something, it must be at least one, even if the number of nodes is zero. For reassurance, see the following other implementations:

wemeetagain
wemeetagain approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

Thanks @jtraglia

looks good to me

@wemeetagain wemeetagain merged commit b8a09bf into ChainSafe:master Nov 2, 2022
@jtraglia jtraglia deleted the fix-things-in-handle-findnode branch November 2, 2022 16:51
@github-actions github-actions bot mentioned this pull request Nov 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wemeetagain wemeetagain wemeetagain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtraglia @wemeetagain