#35315 refactor: Use NodeClock::time_point in more places
https://github.com/bitcoin/bitcoin/pull/35315 · · +183/-181 in 24 files, 10 commits · labels: Refactoring, Needs rebase
Goal
- Unify time handling across P2P code to eliminate confusion from mixing raw integers and time points
- Remove deprecated time helpers to make timing mistakes harder in net processing
Replaces legacy integer time tracking and deprecated GetTime calls with NodeClock::time_point and NodeSeconds throughout net_processing, txdownloadman, and txrequest. It also adjusts peer eviction structures and stats reporting to store time points rather than raw durations, and documents the usage of NodeClock::epoch.
Problem: Net processing code mixed raw integer seconds, durations, and NodeClock time points, creating confusion and relying on deprecated time helpers.
Category: P2P (#54 of 65)
P3 · cleanup
- P3 because it is an internal cleanup that improves type safety and removes deprecated time helpers in P2P
- Provides minor tie-breaking precision improvements when evaluating peer eviction candidates
P3 because this is an internal cleanup that improves type safety and removes deprecated GetTime usage from P2P logic. As stickies-v noted, 'Approach ACK for improved type safety in time handling, this makes the code easier to read and harder to make mistakes.' It also provides a minor internal tie-breaking enhancement in eviction candidate sorting by using sub-second precision.
Membership: Refactors timestamp storage and time calculation in net_processing.cpp, net.h, eviction.h, txdownloadman, and txrequest.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Category: Utilities (logging, arguments, libraries) (#59 of 66)
P4 · cleanup
- P4 because changes to time utilities are cosmetic refinements and docstring additions
- Does not substantially alter underlying time primitives
P4 because the change to src/util/time.h is a cosmetic refinement to allow NodeClock::epoch in NodeSeconds contexts accompanied by explanatory docstrings, without altering time primitives substantially.
Membership: Modifies NodeClock::epoch definition in src/util/time.h and adds guidance on time point defaults.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with master
- Author has been silent for nearly two months with recent nits unaddressed
The PR has merge conflicts with master and carries the 'Needs rebase' label.
Author status: Addressing review comments intermittently; silent for 58 days, with recent reviewer nits left unaddressed.
Resolved concerns:
- ryanofsky objected to expanding uses of NodeClock::epoch as a sentinel and suggested alternatives such as std::optional or default-constructed time points; maflcko refined the docstrings and reduced epoch usage in response.
- stickies-v favored std::optional over time_point::min and time_point::max sentinels, but withdrew the objection as nonblocking given the overall improvements.
- Minor behavior changes from higher precision in peer eviction and block announcement tie-breaking were documented in commit messages as requested by ryanofsky.
Agreement: Positive w/ caveats
- Broad support for type safety improvements across time handling
- Approves the code while noting expanding epoch sentinel usage is suboptimal (ryanofsky)
- Withdrew nonblocking preference for std::optional given overall improvements (stickies-v)
- Verified by testing with minor nits (jeanpablojp)
- Concept approval without stated reasons (w0xlt)
Multiple ACKs, but ryanofsky and stickies-v noted standing reservations about sentinel time values and NodeClock::epoch
Reviewers broadly support the type safety improvements, with multiple ACKs from regular contributors. However, criticisms regarding the use of NodeClock::epoch and sentinel values instead of std::optional were resolved by agreeing to disagree rather than full architectural changes.
- stickies-v: Approach ACK and later full ACK fab4bd7f2b.
- ryanofsky: Code review ACK fab4bd7f2b after commit docstring refinements, while maintaining that expanded NodeClock::epoch usage is suboptimal.
- jeanpablojp: tACK fab4bd7f2b with minor nits.
- w0xlt: Concept ACK.
Review verdicts (DrahtBot): 3 (+2)
- ACK: stickies-v, ryanofsky, jeanpablojp
- Stale ACK: sedited, seduless
- Concept ACK: w0xlt
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
PR #35315 migrates timestamp tracking across P2P net processing, eviction logic, and txdownloadman from deprecated GetTime and raw integer durations to NodeClock::time_point and NodeSeconds. This resolves confusing mixed time representations and slightly improves peer eviction sorting precision. Multiple reviewers have ACKed the change for improved type safety, although discussions around sentinel values versus std::optional were resolved by agreeing to disagree. The PR is currently in a merge-conflict state and requires a rebase before merging.