-
Notifications
You must be signed in to change notification settings - Fork 20k
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
eth: continue after whitelist check #24210
Conversation
Without the PR, on shutdown, a lot of peers are stuck on
This is due (I think) to the peer threads being stuck waiting for the res to return here:
|
Good catch. @holiman Are the hangs before or after this PR? |
The hangs are without this PR (edited to clarify) |
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.
LGTM
eth: continue after whitelist check
For inclusivity should we rename it to allowlist? |
I definitely agree a more inclusive (and descriptive!) name would be better, I'll start a PR that deprecates the current name and chooses a new one as soon as I can, will link it here when ready. |
The whitelist mechanism got broken in #23576, afaict, making it so that if the whitelist check succeeds, the code stalls. The peer is left somehow neither accepted nor rejected, in a state where it's marked as shutting down but not actually kicked out.
This is the cause behind #24202 .
Also, aside from not being able to sync, it also (on my test) causes failure to shut down.
With this fix, I was able to start syncing with peers after the check completes.