Skip to content

p2p/discover: restore nextTimeout update in UDPv4 resetTimeout loop - #34878

Merged
fjl merged 1 commit into
ethereum:masterfrom
rayjun:fix/discv4-reset-timeout-stale-ref
May 5, 2026
Merged

p2p/discover: restore nextTimeout update in UDPv4 resetTimeout loop#34878
fjl merged 1 commit into
ethereum:masterfrom
rayjun:fix/discv4-reset-timeout-stale-ref

Conversation

@rayjun

@rayjun rayjun commented May 5, 2026

Copy link
Copy Markdown
Contributor

The refactor from for el := plist.Front(); ...; el = el.Next() to the new iterList iterator in #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.

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.
@rayjun
rayjun requested review from fjl and zsfelfoldi as code owners May 5, 2026 09:50
@fjl fjl self-assigned this May 5, 2026

@fjl fjl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Damn, I misread the code when changing the PR. Thanks for fixing!

@fjl
fjl merged commit 60db25b into ethereum:master May 5, 2026
9 checks passed
@fjl fjl added this to the 1.17.3 milestone May 5, 2026
@rayjun
rayjun deleted the fix/discv4-reset-timeout-stale-ref branch May 6, 2026 04:15
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.

2 participants