-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
p2p/discover: fix bug in checkNodesEqual
#29603
base: master
Are you sure you want to change the base?
Conversation
} else { | ||
return fmt.Errorf("length dismatch: got %d nodes, want %d nodes", len(got), len(want)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative fix: just move the return nil
up one line, into the if-clause. Then the non-equal case would just fall through to the code below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holiman Thanks for reviewing. What you suggest seems more elegant :)
But what more important is that test case TestUDPv5_lookupE2E
is defective, UDPv5.Lookup
actually could not find all expected nodes. So if we merge this bugfix, this test case will never pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, well done then finding a bug in the testcase!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holiman I try to fix TestUDPv5_lookupE2E
, but it seem hard for me now :)
Maybe you guys could fix it, then this PR could get merged.
Yeah, the test doesn't pass and I need to investigate. |
I have investigated. The problem is that the network created in the test is very small, and thus the outcome of the lookup depends on whether the randomly chosen nodes fall into the right buckets. If they don't, the initial query to the bootnode returns empty results and the lookup finishes. |
No description provided.