p2p/discover: restore nextTimeout update in UDPv4 resetTimeout loop - #34878
Merged
Conversation
The refactor from `for el := plist.Front(); ...; el = el.Next()` to the new `iterList` iterator in ethereum#34743 silently dropped two things needed by resetTimeout: 1. `nextTimeout = el.Value.(*replyMatcher)` at the top of the loop. This assignment is what gives `nextTimeout` its documented meaning ("head of plist when timeout was last reset"), and what makes the early-return optimization at the top of resetTimeout work. Without it, nextTimeout is only ever written to nil, so `nextTimeout == plist.Front().Value` is always false and the optimization is dead. 2. `nextTimeout.errc <- errClockWarp` in the clock-warp branch now reads a stale or nil pointer. Prior to the refactor, the inner assignment kept nextTimeout pointing at the current matcher so its errc was the right channel to receive the errClockWarp signal. After the refactor, on first entry into the clock-warp branch nextTimeout is nil, which panics the UDPv4 loop goroutine with a nil pointer deref and takes discv4 down. Re-assign `nextTimeout = p` at the head of the loop (restoring the documented invariant) and send the clock-warp error on `p.errc` rather than the now-stale `nextTimeout.errc`. The clock-warp branch triggers only when the system clock jumps backward after a deadline is assigned (deadline - time.Now() >= 2*respTimeout, i.e. at least ~500ms backward jump), which is why this regression slipped past CI - it is not exercised by any existing unit test, and writing one would require plumbing a clock through the loop.
fjl
approved these changes
May 5, 2026
fjl
left a comment
Contributor
There was a problem hiding this comment.
Damn, I misread the code when changing the PR. Thanks for fixing!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The refactor from
for el := plist.Front(); ...; el = el.Next()to the newiterListiterator in #34743 silently dropped two things needed by resetTimeout:nextTimeout = el.Value.(*replyMatcher)at the top of the loop. This assignment is what givesnextTimeoutits documented meaning ("head of plist when timeout was last reset"), and what makes the early-return optimization at the top of resetTimeout work. Without it, nextTimeout is only ever written to nil, sonextTimeout == plist.Front().Valueis always false and the optimization is dead.nextTimeout.errc <- errClockWarpin the clock-warp branch now reads a stale or nil pointer. Prior to the refactor, the inner assignment kept nextTimeout pointing at the current matcher so its errc was the right channel to receive the errClockWarp signal. After the refactor, on first entry into the clock-warp branch nextTimeout is nil, which panics the UDPv4 loop goroutine with a nil pointer deref and takes discv4 down.Re-assign
nextTimeout = pat the head of the loop (restoring the documented invariant) and send the clock-warp error onp.errcrather than the now-stalenextTimeout.errc.The clock-warp branch triggers only when the system clock jumps backward after a deadline is assigned (deadline - time.Now() >= 2*respTimeout, i.e. at least ~500ms backward jump), which is why this regression slipped past CI - it is not exercised by any existing unit test, and writing one would require plumbing a clock through the loop.