#35315 refactor: Use NodeClock::time_point in more places

ReviewabilityReviewsAgreementSize
#35315 maflcko refactor: Use NodeClock::time_point in more placesAuthor silent 103d3 (+2)Strong148 + 35 tests
  • Standardize absolute time tracking across P2P and networking code using strongly typed time points
  • Eliminate type confusion and second-level truncation that causes arbitrary tie-breaking in peer eviction
  • Needs rebase and author response after 92 days of inactivity
  • Strong support with all objections resolved (ryanofsky, stickies-v)
  • Strong support for type safety and clearer time representation (ryanofsky, stickies-v)
  • Verified by running unit and P2P functional test suites (seduless, jeanpablojp)
  • Rebase requested (sedited)
  • 148 lines added or modified outside tests
  • 35 lines added or modified in tests
  • 181 lines removed in total
  • 24 files, 10 commits
P2P#56 of 71
P3 · cleanup
  • P3 because it improves type safety and readability across peer-to-peer networking modules
  • Increases precision so peer eviction tie-breaking relies on exact timing rather than arbitrary peer IDs
  • Ranking pass: Mostly type hygiene with only a marginal eviction-precision gain.
Ranking notes
Utilities (logging, arguments, libraries)#55 of 59
P4 · cleanup
  • P4 because it adds minor shared utility helpers and standard sentinels in util/time.h
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Replaces deprecated GetTime() calls and duration types with strongly typed NodeClock::time_point and NodeSeconds across networking and P2P modules. The changes span net_processing, txrequest, txdownloadman, and node eviction tracking, while standardizing sentinel values on NodeClock::epoch, time_point::min(), and time_point::max().

Problem: P2P and networking code inconsistently uses durations, raw int64_t timestamps, and deprecated GetTime() alongside NodeClock time points to represent absolute times. This inconsistency causes type confusion and leads to second-level truncation in peer tracking fields, causing eviction sorting logic to fall back to arbitrary peer IDs during same-second ties.

Evidence of importance:

  • PR description: "It is a bit confusing to have some code use the deprecated GetTime, which returns a duration and not a time point, and other code to use NodeClock time points."
  • Commit fab33fdd92: "With the precision increase from seconds to nanoseconds, SelectNodeToEvict will be more precise picking the protected peers with CompareNodeBlockTime and CompareNodeTXTime."
  • Commit fafd1e3312: "Changing the time points to be more precise may change the sorting of the worst peers when they all announced a block in the same second. In this case it should be fine and preferable, to use the more accurate sorting from the exact block announce time for the worst peers, than to fall back to peer with the highest id."

Scope: Modifies time parameter types in public and internal interfaces across several P2P subsystems (txrequest, txdownloadman, eviction, net_processing), requiring coordinated updates across unit tests and fuzz harnesses.

Reviewability

Stale · Author silent 103d

No author activity for 103 days; a review may wait a long time for a response.

Waiting on (from the thread read):

Agreement

Strong

substantive support, no open objection (ryanofsky, ryanofsky, stickies-v, ryanofsky, ryanofsky, jeanpablojp)

Thread notes: Added suggestions from seduless on peerman_tests and ryanofsky on type aliases found during schema 1 re-check. The PR is waiting on a rebase requested by sedited. / Re-read under schema 2: reclassified stickies-v's sentinel antipattern comment and ryanofsky's commit message comment as suggestions, and recorded suggestions for type aliases, peerman_tests follow-up, and TicksSeconds. The PR is waiting on the author for a rebase.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
seduless (contributor)suggestion, scopeMigrate remaining straightforward places in peerman_tests to NodeSecondsaccepted · author preferred to keep PR scope to net_processing and defer peerman_tests to follow-up; seduless agreedansweredyes2026-06-24: 'peerman_tests still has a few straightforward migrations left. Non-blocking, happy to send a follow-up if you'd rather not fold it in here.'
Status from: 2026-06-25: seduless: 'Perhaps it's useful for the next follow-up to avoid clashing with the scoped clock proposed for the kernel in #35557?'
ryanofsky (member)suggestion, approachUse application-specific type aliases (such as NetworkTime, MempoolTime) instead of hardcoding NodeClock::time_point everywhereagreed_to_disagree · author and sedited preferred not adding aliases; ryanofsky agreed to disagreeansweredyes2026-06-29: 'could use application-specific type aliases like `using MempoolTime = NodeClock::time_point;`, `using NetworkTime = NodeClock::time_point;`, `using BlockTime = NodeSeconds;`'
Status from: 2026-07-07: ryanofsky: 'Seems fine to agree to disagree on this, though.'
ryanofsky (member)objection, maintenanceExpanding uses of the nonstandard NodeClock::epoch constant encourages bad code unnecessarily tied to the unix epoch and promotes magic-constant sentinelsfixed · fix pushed · author adjusted commit message and added warning docstring to epoch; ryanofsky ACKedansweredyes2026-06-29: 'This change seems kludgy and would seem to encourage writing bad code.'
Status from: 2026-07-28: ryanofsky: 'Code review ACK fab4bd7f2bf8eb78efff00a6d1abcb28187e0063. Changes since last review: tweaking `epoch` comment and commit'
ryanofsky (member)objection, correctnessSwitching from second to nanosecond precision can alter comparison outcomes (e.g., peer eviction and block announcement tie-breaking), potentially exposing new corner cases without being documentedfixed · fix pushed · author clarified commit messages and prefixed precision-changing commits; ryanofsky ACKedansweredyes2026-06-29: 'Changing behavior should be ok and probably even an improvement, but commit message should clarify whether it is changing or not.'
Status from: 2026-07-28: ryanofsky: 'Code review ACK fab4bd7f2bf8eb78efff00a6d1abcb28187e0063.'
stickies-v (member)suggestion, safetyUse TicksSeconds in ApproximateBestBlockDepth to guard against changes to m_best_block_time duration typefixed · fix pushed · author addressed in rebase with PowTargetSpacing replacementansweredyes2026-07-16: 'would be good to use `TicksSeconds` here to guard against errors when `m_best_block_time` changes its duration type?'
Status from: 2026-07-21: maflcko: 'Thx, done, but with a different patch'
stickies-v (member)objection, interfaceRelying on time_point::min and time_point::max sentinels creates nonsensical time values and obscures semantic states compared to std::optionalaccepted · stickies-v accepted keeping it as-is to avoid review churn on a non-regressionansweredyes2026-07-20: 'I find relying on sentinels (`if (State(staller)->m_stalling_since == NodeClock::time_point::max())`) to be an antipattern, and would have found it more clear if this were a `std::optional`.'
Status from: 2026-07-22: stickies-v: 'I'm happy to keep it as-is, it's not a regression vs master and the PR has too many other benefits to be spending too mu'

Support:

  • w0xlt (contributor) (Concept ACK, 2026-06-14): Concept ACK [not substantive]
  • sedited (member) (ACK, 2026-06-24): ACK fa14855ff1bd527cbbd56080eb2cfdcd74d780da [not substantive]
  • seduless (contributor) (Tested ACK, 2026-06-24) [tested]: Tested ACK fa14855ff1bd527cbbd56080eb2cfdcd74d780da [not substantive]
  • ryanofsky (member) (ACK, 2026-06-29) [read]: NodeClock::time_point is an improvement over using integer or duration types to represent time points.
  • ryanofsky (member) (ACK, 2026-07-09) [read]: IMO, this is much improved since the last version, and the last version was already a nice cleanup
  • stickies-v (member) (Approach ACK, 2026-07-15) [read]: improved type safety in time handling, this makes the code easier to read and harder to make mistakes.
  • ryanofsky (member) (ACK, 2026-07-16) [read]: Nice improvement using the time point type to represent times, instead of duration types or integers.
  • stickies-v (member) (ACK, 2026-07-23): ACK fab4bd7f2bf8eb78efff00a6d1abcb28187e0063 [not substantive]
  • ryanofsky (member) (ACK, 2026-07-28) [read]: Code review ACK fab4bd7f2bf8eb78efff00a6d1abcb28187e0063. Changes since last review: tweaking `epoch` comment and commit message as suggested, replacing nPowTargetSpacing with PowTargetSpacing a few p
  • jeanpablojp (contributor) (Tested ACK, 2026-09-08) [tested]: Built and ran the unit suites and the p2p functional tests.

Participants:

  • w0xlt (contributor, support, 1 statements 2026-06-14 to 2026-06-14): Concept ACK
  • sedited (member, support, 3 statements 2026-06-24 to 2026-10-21): ACKed; opposed type aliases; pinged author for rebase
  • seduless (contributor, support, 2 statements 2026-06-24 to 2026-06-25): Tested ACK; suggested follow-up cleanups in peerman_tests
  • ryanofsky (member, objection, 34 statements 2026-06-29 to 2026-07-28): ACKed; raised concerns regarding NodeClock::epoch usage, precision changes, and suggested type aliases
  • stickies-v (member, objection, 11 statements 2026-07-15 to 2026-07-23): Approach ACK and ACK; raised concern preferring std::optional over time_point sentinels
  • jeanpablojp (contributor, support, 4 statements 2026-09-08 to 2026-09-08): Tested ACK with minor nits

Files

70 lines under test/bench/ci.

  • src/net_processing.cpp +86/-91
  • src/txrequest.cpp +12/-12
  • src/test/txrequest_tests.cpp +11/-11
  • src/test/fuzz/txrequest.cpp +8/-8
  • src/util/time.h +12/-1
  • src/node/txdownloadman_impl.cpp +6/-6
  • src/test/net_peer_eviction_tests.cpp +6/-6
  • src/addrman.cpp +4/-4
  • src/init.cpp +4/-4
  • src/net.h +4/-4
  • src/txrequest.h +4/-4
  • src/net_processing.h +4/-3
  • src/node/txdownloadman_impl.h +3/-3
  • src/node/eviction.h +3/-2
  • src/qt/rpcconsole.cpp +2/-3
  • src/qt/rpcconsole.h +0/-5
  • src/node/txdownloadman.h +2/-2
  • src/rpc/net.cpp +2/-2
  • src/test/denialofservice_tests.cpp +2/-2
  • src/test/fuzz/node_eviction.cpp +2/-2
  • src/test/fuzz/txdownloadman.cpp +2/-2
  • src/test/util/net.cpp +2/-2
  • src/test/peerman_tests.cpp +1/-1
  • src/test/txdownload_tests.cpp +1/-1

Card

The short text the ranking pass compares across PRs.

Replaces deprecated GetTime calls and duration types with NodeClock::time_point and NodeSeconds across P2P subsystems, including net_processing, txrequest, txdownloadman, and peer eviction logic. Inconsistent use of raw integer durations creates type confusion and truncates event timestamps to whole seconds. The author notes that higher timestamp precision improves peer eviction decisions in SelectNodeToEvict and EvictExtraOutboundPeers by sorting based on actual event order rather than falling back to peer IDs during ties. This is primarily an internal refactoring with minor peer sorting precision improvements.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-22T01:16, confidence high, input hash 2f2e97e1d8b92023
thread read through 2026-10-21 (99 statements), head fab4bd7f2b
labels: Refactoring, Needs rebase

Categories

P2P

P3 · cleanup (ranking pass: Mostly type hygiene with only a marginal eviction-precision gain.)

Adopting `NodeClock::time_point` and `NodeSeconds` across P2P subsystems improves type safety and readability. Additionally, increasing precision to nanoseconds makes peer eviction sorting more accurate during same-second ties instead of falling back to arbitrary peer IDs.

Membership: Refactors time types across net_processing, txrequest, txdownloadman, and peer eviction handling.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1

Utilities (logging, arguments, libraries)

P4 · cleanup

Provides minor shared utility additions in util/time.h to support type-safe time representations across the codebase.

Membership: Extends src/util/time.h to introduce NodeClock::epoch and standardized sentinel time points.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1