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	l0rinc, ajtowns, sedited

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:

#34717 (p2p: remove m_getaddr_sent by naiyoma)

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

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 outdated

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…

maflcko commented at 12:58 pm on March 16, 2026:

static constexpr time_point epoch

Sure, I’ll keep it in mind and add it when there is need to.

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

Not sure how that is possible here. The interface for max is const T& max( const T& a, const T& b ), so both values passed to it must have exactly the same type T, otherwise the compiler will fail with: candidate template ignored: deduced conflicting types for parameter '_Tp' ('duration<[...], ratio<[...], den aka 1000000000>>' vs. 'duration<[...], ratio<[...], 1000000>>').

See https://en.cppreference.com/w/cpp/algorithm/max.html

(Previously, specifying the type here was not needed, because a cast was done in another place to ensure both values have the exact same type)

Leaving as-is for now, but I am happy to push any diff that compiles.

ajtowns commented at 2:56 am on March 20, 2026:

0const auto time_diff{current_time - peer.m_addr_token_timestamp};
1const double increment = std::max(Ticks<SecondsDouble>(time_diff), 0.0) * MAX_ADDR_RATE_PER_SECOND;

would work.

l0rinc commented at 3:32 am on March 20, 2026:

+1, would inline time_diff though (to unify = and {} at least) and maybe use division by literal instead (though that changes the type from double to long double which we may not want):

0const auto increment{std::max((current_time - peer.m_addr_token_timestamp) / 1.0s, 0.0L) * MAX_ADDR_RATE_PER_SECOND};

maflcko commented at 6:15 am on March 20, 2026:

would work.

Thx, pushed.

use division by literal instead

I don’t think division is the right tool to count ticks. If it was, then there was no need for Ticks() or count()?

in src/net_processing.cpp:5668 in fa0c1f3ac5 outdated

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.

maflcko commented at 12:58 pm on March 16, 2026:

Yes, this is correct. However I thought it would be easier to review (and read in the future) when using the same type for all of the addr.nTime compare/assignments.

ajtowns commented at 3:06 am on March 20, 2026:

Hmm, I figured it would be easier to read in future with fewer types/conversions; the advantage of the time_point stuff is that conversions between different precision just happen automatically when it’s possible to do that correctly.

l0rinc commented at 3:51 am on March 20, 2026:

You mean like:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1--- a/src/net_processing.cpp	(revision 1c50fde29f43c74a2e26625f4e332562c8c4be68)
 2+++ b/src/net_processing.cpp	(date 1773978487194)
 3@@ -5654,7 +5654,6 @@
 4     const bool rate_limited = !pfrom.HasPermission(NetPermissionFlags::Addr);
 5     uint64_t num_proc = 0;
 6     uint64_t num_rate_limit = 0;
 7-    const auto now_seconds{std::chrono::time_point_cast<std::chrono::seconds>(current_time)};
 8     std::shuffle(vAddr.begin(), vAddr.end(), m_rng);
 9     for (CAddress& addr : vAddr)
10     {
11@@ -5676,8 +5675,8 @@
12         if (!MayHaveUsefulAddressDB(addr.nServices) && !HasAllDesirableServiceFlags(addr.nServices))
13             continue;
14 
15-        if (addr.nTime <= NodeSeconds{100000000s} || addr.nTime > now_seconds + 10min) {
16-            addr.nTime = now_seconds - 5 * 24h;
17+        if (addr.nTime <= NodeSeconds{100000000s} || addr.nTime > current_time + 10min) {
18+            addr.nTime = std::chrono::time_point_cast<std::chrono::seconds>(current_time - 5 * 24h);
19         }
20         AddAddressKnown(peer, addr);
21         if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) {
22@@ -5686,7 +5685,7 @@
23         }
24         ++num_proc;
25         const bool reachable{g_reachable_nets.Contains(addr)};
26-        if (addr.nTime > now_seconds - 10min && !peer.m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {
27+        if (addr.nTime > current_time - 10min && !peer.m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {
28             // Relay to a limited number of other nodes
29             RelayAddress(pfrom.GetId(), addr, reachable);
30         }

I had the same observation.

maflcko commented at 6:17 am on March 20, 2026:

Ok, pushed, since a few reviewers preferred it

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

ACK fa0c1f3ac5b57564ca3f1f36010ea88b001d021e

in src/net_processing.cpp:5608 in facebad6c9 outdated

5603@@ -5683,6 +5604,91 @@ bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer)
5604     return true;
5605 }
5606 
5607+void PeerManagerImpl::ProcessAddrs(std::string_view msg_type, CNode& pfrom, Peer& peer, std::vector<CAddress>&& vAddr, const std::atomic<bool>& interruptMsgProc)
5608+{

sedited commented at 8:41 am on March 6, 2026:

Should we assert on the locks here?

maflcko commented at 12:58 pm on March 16, 2026:

Sure, done!

sedited commented at 8:45 am on March 6, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on Mar 11, 2026

move-only: Extract ProcessAddrs() helper

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

fa55723b8f

maflcko force-pushed on Mar 16, 2026

maflcko commented at 12:58 pm on March 16, 2026: member

Rebased. Should be easy to re-review with range-diff.

DrahtBot removed the label Needs rebase on Mar 16, 2026

sedited approved

sedited commented at 4:33 pm on March 18, 2026: contributor

ACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

DrahtBot requested review from ajtowns on Mar 18, 2026

ajtowns commented at 3:06 am on March 20, 2026: contributor

utACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

in src/net_processing.cpp:5646 in fa932ae9ff outdated

5642@@ -5643,10 +5643,10 @@ void PeerManagerImpl::ProcessAddrs(std::string_view msg_type, CNode& pfrom, Peer
5643     const auto current_a_time{Now<NodeSeconds>()};
5644 
5645     // Update/increment addr rate limiting bucket.
5646-    const auto current_time{GetTime<std::chrono::microseconds>()};
5647+    const auto current_time{NodeClock::now()};

l0rinc commented at 3:31 am on March 20, 2026:

fa932ae refactor: Use NodeClock::time_point for m_addr_token_timestamp:

Note: my understanding is this was microseconds before and is potentially implementation-defined precision now (could be nanoseconds). It shouldn’t matter here as far as I can tell.

in src/net_processing.cpp:1086 in fa55723b8f outdated

1082@@ -1083,6 +1083,9 @@ class PeerManagerImpl final : public PeerManager
1083      */
1084     bool SetupAddressRelay(const CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
1085 
1086+    void ProcessAddrs(std::string_view msg_type, CNode& pfrom, Peer& peer, std::vector<CAddress>&& vAddr, const std::atomic<bool>& interruptMsgProc)

l0rinc commented at 3:55 am on March 20, 2026:

fa55723 move-only: Extract ProcessAddrs() helper:

super-nit: it’s not strictly a move-only, it’s an extract, in other cases we’d call this a refactor. Not important.

maflcko commented at 6:16 am on March 20, 2026:

Yeah I think use use move-only to mean that the dimmed-zebra is mostly dimmed (mod a few lines)

l0rinc approved

l0rinc commented at 4:29 am on March 20, 2026: contributor

ACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

Rebased locally and ran all tests.

refactor: Use NodeClock::time_point for m_addr_token_timestamp

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

3333c5023f

refactor: Use current_time over redundant call to Now()

This also better reflects the meaning and use.

faaea7895f

maflcko force-pushed on Mar 20, 2026

l0rinc commented at 6:16 am on March 20, 2026: contributor

ACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

DrahtBot requested review from sedited on Mar 20, 2026

ajtowns commented at 7:29 am on March 20, 2026: contributor

reACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

sedited approved

sedited commented at 8:29 am on March 20, 2026: contributor

Re-ACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

sedited merged this on Mar 20, 2026

sedited closed this on Mar 20, 2026

maflcko deleted the branch on Mar 20, 2026

refactor: Use NodeClock::time_point for m_addr_token_timestamp #34059

Code Coverage & Benchmarks

Reviews

Conflicts