refactor: Use NodeClock::time_point for m_addr_token

maflcko commented at 2:15 pm on December 12, 2025: member

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.

Fix one place m_addr_token_timestamp to use NodeClock::time_point.

Also:

Extract a ProcessAddrs helper, similar to the other Process*() helpers, to cut down the ProcessMessage with a massive scope.
Rename the confusing current_a_time to now_seconds. (The a in this context refers to the removed “adjusted” time, see commit fadd8b2676f6d68ec87189871461c9a6a6aa3cac, which removed adjusted time here)

DrahtBot renamed this:
~~refactor: Use NodeClock::time_point for m_addr_token_timestamp~~
refactor: Use NodeClock::time_point for m_addr_token_timestamp
on Dec 12, 2025

DrahtBot added the label Refactoring on Dec 12, 2025

DrahtBot commented at 2:15 pm on December 12, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34059.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ajtowns

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34181 (refactor: [p2p] Make ProcessMessage private again, Use references when non-null by maflcko)
#33854 (fix assumevalid is ignored during reindex by Eunovo)
#32278 (doc: better document NetEventsInterface and the deletion of “CNode"s by vasild)
#31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ajtowns commented at 11:45 pm on December 12, 2025: contributor

What do you think of getting all the peer->foo to peer.foo noise done first, rather than doing it bit by bit? Here’s a scripted-diff approach that might be easy to review: https://github.com/ajtowns/bitcoin/commits/202512-peer-by-ref/

Approach ACK otherwise

maflcko marked this as a draft on Dec 15, 2025

maflcko force-pushed on Dec 16, 2025

maflcko marked this as ready for review on Dec 16, 2025

maflcko force-pushed on Dec 16, 2025

DrahtBot added the label CI failed on Dec 16, 2025

maflcko force-pushed on Dec 16, 2025

maflcko commented at 9:40 am on December 16, 2025: member

What do you think of getting all the peer->foo to peer.foo noise done first, rather than doing it bit by bit? Here’s a scripted-diff approach that might be easy to review: https://github.com/ajtowns/bitcoin/commits/202512-peer-by-ref/

Nice branch. I re-created it to avoid refactoring real code to accommodate tests, which are better served by just using the pre-existing ConnmanTestMsg helpers.

DrahtBot removed the label CI failed on Dec 16, 2025

DrahtBot added the label Needs rebase on Dec 16, 2025

maflcko force-pushed on Dec 16, 2025

DrahtBot removed the label Needs rebase on Dec 16, 2025

DrahtBot added the label CI failed on Dec 16, 2025

DrahtBot removed the label CI failed on Dec 16, 2025

maflcko marked this as a draft on Dec 30, 2025

DrahtBot added the label Needs rebase on Jan 12, 2026

maflcko marked this as ready for review on Feb 9, 2026

move-only: Extract ProcessAddrs() helper

This can be reviewed via the git options:
--color-moved=dimmed-zebra  --color-moved-ws=ignore-all-space

facebad6c9

refactor: Use NodeClock::time_point for m_addr_token_timestamp

This refactor makes the field a bit more type-safe.

faa9f191ba

refactor: Rename current_a_time to now_seconds

This better reflects the meaning and use.

fa0c1f3ac5

maflcko force-pushed on Feb 9, 2026

maflcko commented at 12:52 pm on February 9, 2026: member

Rebased on the preparatory work in commit 6f68e0c8b760046cc793d172f9bdbe5a78fd323d. Should be easy to review now.

DrahtBot removed the label Needs rebase on Feb 9, 2026

in src/net_processing.cpp:5628 in faa9f191ba

5621@@ -5622,10 +5622,10 @@ void PeerManagerImpl::ProcessAddrs(std::string_view msg_type, CNode& pfrom, Peer
5622     const auto current_a_time{Now<NodeSeconds>()};
5623 
5624     // Update/increment addr rate limiting bucket.
5625-    const auto current_time{GetTime<std::chrono::microseconds>()};
5626+    const auto current_time{NodeClock::now()};
5627     if (peer.m_addr_token_bucket < MAX_ADDR_PROCESSING_TOKEN_BUCKET) {
5628         // Don't increment bucket if it's already full
5629-        const auto time_diff = std::max(current_time - peer.m_addr_token_timestamp, 0us);
5630+        const auto time_diff{std::max<NodeClock::duration>(current_time - peer.m_addr_token_timestamp, 0us)};

ajtowns commented at 8:37 pm on February 9, 2026:

Might be nice to define class NodeClock { static constexpr time_point epoch{duration{0}}; ... } with static_assert(NodeClock::epoch.time_since_epoch().count() == 0); ... } in util/time.cpp so that we can write std::max(c_t - a_t_t, NodeClock::epoch) instead?

We should be able to use the new modern time_point types without having to have clumsy casts back and forwards from durations…

in src/net_processing.cpp:5668 in fa0c1f3ac5

5664@@ -5665,7 +5665,7 @@ void PeerManagerImpl::ProcessAddrs(std::string_view msg_type, CNode& pfrom, Peer
5665         }
5666         ++num_proc;
5667         const bool reachable{g_reachable_nets.Contains(addr)};
5668-        if (addr.nTime > current_a_time - 10min && !peer.m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {
5669+        if (addr.nTime > now_seconds - 10min && !peer.m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {

ajtowns commented at 8:56 pm on February 9, 2026:

I believe we could use current_time in both the comparisons (causing addr.nTime to gain precision rather than now_seconds to lose it), and only do the time_point_cast when assigning to addr.nTime.

ajtowns commented at 9:01 pm on February 9, 2026: contributor

ACK fa0c1f3ac5b57564ca3f1f36010ea88b001d021e

refactor: Use NodeClock::time_point for m_addr_token_timestamp #34059

Code Coverage & Benchmarks

Reviews

Conflicts