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

eth: fix hang in waitSnapExtension #25677

Closed
wants to merge 1 commit into from
Closed

Conversation

kaber2
Copy link

@kaber2 kaber2 commented Sep 4, 2022

This patch fixes peers hanging in state 'eth: "handshake", snap: "handshake"' forever. On my nodes, I had a huge amount of these peers, which might indicate another issue, but this one clearly appears to be a bug.

Since this is my first Github PR, I'm not sure if this description will actually be the commit message as it pre-populated the PR text with my commit message, which is:

When a peer announcing snap support connects without actually starting
the snap protocol, waitSnapExtension() will wait forever.

Add a timer to stop waiting after 5s.

When a peer announcing snap support connects without actually starting
the snap protocol, waitSnapExtension() will wait forever.

Add a timer to stop waiting after 5s.
@kaber2
Copy link
Author

kaber2 commented Sep 4, 2022

Small addendum: it turns out the root cause for the large amount of occurences on my nodes were caused by local changes.

The fix itself still seems valid though.

@fjl
Copy link
Contributor

fjl commented Sep 9, 2022

I have discussed this with @karalabe, and we reached the following conclusion: The timeout is not necessary here because we are already checking whether "eth" is enabled earlier in waitSnapExtension:

if !peer.RunningCap(eth.ProtocolName, eth.ProtocolVersions) {
return errSnapWithoutEth
}

If the eth is enabled, its handler will be launched, and so there is no need to wait with a timeout, it's more correct to rely on it sending on the channel.

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.

4 participants