Skip to content

eth/protocols/eth: fix handshake timeout metrics classification#33539

Merged
lightclient merged 2 commits intoethereum:masterfrom
ANtutov:eth/fix-handshake-timeout-metrics
Feb 25, 2026
Merged

eth/protocols/eth: fix handshake timeout metrics classification#33539
lightclient merged 2 commits intoethereum:masterfrom
ANtutov:eth/fix-handshake-timeout-metrics

Conversation

@ANtutov
Copy link
Copy Markdown
Contributor

@ANtutov ANtutov commented Jan 6, 2026

Previously, handshake timeouts were recorded as generic peer errors instead of timeout errors. waitForHandshake passed a raw p2p.DiscReadTimeout into markError, but markError classified errors only via errors.Unwrap(err), which returns nil for non-wrapped errors. As a result, the timeoutError meter was never incremented and all such failures fell into the peerError bucket.

This change makes markError switch on the base error, using errors.Unwrap(err) when available and falling back to the original error otherwise. With this adjustment, p2p.DiscReadTimeout is correctly mapped to timeoutError, while existing behaviour for the other wrapped sentinel errors remains unchanged

@ANtutov ANtutov requested a review from rjl493456442 as a code owner January 6, 2026 16:13
Copy link
Copy Markdown
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM, its very weird that unwrap behaves in this way

Copy link
Copy Markdown
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Switched to use errors.Is since it is more idiomatic and can handle multiple layers of error wrapping.

@lightclient lightclient added this to the 1.17.1 milestone Feb 24, 2026
@lightclient lightclient merged commit 2a45272 into ethereum:master Feb 25, 2026
7 of 8 checks passed
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