refactor: Use NodeClock::time_point for m_addr_token_timestamp #34059

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2512-net-less-GetTime changing 1 files +92 −84
  1. 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)
  2. 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
  3. DrahtBot added the label Refactoring on Dec 12, 2025
  4. DrahtBot commented at 2:15 PM on December 12, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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 <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. 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

  6. maflcko marked this as a draft on Dec 15, 2025
  7. maflcko force-pushed on Dec 16, 2025
  8. maflcko marked this as ready for review on Dec 16, 2025
  9. maflcko force-pushed on Dec 16, 2025
  10. DrahtBot added the label CI failed on Dec 16, 2025
  11. maflcko force-pushed on Dec 16, 2025
  12. 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.

  13. DrahtBot removed the label CI failed on Dec 16, 2025
  14. DrahtBot added the label Needs rebase on Dec 16, 2025
  15. maflcko force-pushed on Dec 16, 2025
  16. DrahtBot removed the label Needs rebase on Dec 16, 2025
  17. DrahtBot added the label CI failed on Dec 16, 2025
  18. DrahtBot removed the label CI failed on Dec 16, 2025
  19. maflcko marked this as a draft on Dec 30, 2025
  20. DrahtBot added the label Needs rebase on Jan 12, 2026
  21. maflcko marked this as ready for review on Feb 9, 2026
  22. maflcko force-pushed on Feb 9, 2026
  23. 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.

  24. DrahtBot removed the label Needs rebase on Feb 9, 2026
  25. 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: 
    const auto time_diff{current_time - peer.m_addr_token_timestamp};
const 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):

    const 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()?

  26. 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:

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

    I had the same observation.

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

    Ok, pushed, since a few reviewers preferred it

  27. ajtowns commented at 9:01 PM on February 9, 2026: contributor

    ACK fa0c1f3ac5b57564ca3f1f36010ea88b001d021e

  28. 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!

  29. sedited commented at 8:45 AM on March 6, 2026: contributor

    Concept ACK

  30. DrahtBot added the label Needs rebase on Mar 11, 2026
  31. move-only: Extract ProcessAddrs() helper
    This can be reviewed via the git options:
--color-moved=dimmed-zebra  --color-moved-ws=ignore-all-space
    fa55723b8f
  32. maflcko force-pushed on Mar 16, 2026
  33. maflcko commented at 12:58 PM on March 16, 2026: member

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

  34. DrahtBot removed the label Needs rebase on Mar 16, 2026
  35. sedited approved
  36. sedited commented at 4:33 PM on March 18, 2026: contributor

    ACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

  37. DrahtBot requested review from ajtowns on Mar 18, 2026
  38. ajtowns commented at 3:06 AM on March 20, 2026: contributor

    utACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

  39. 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.

  40. 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)

  41. l0rinc approved
  42. l0rinc commented at 4:29 AM on March 20, 2026: contributor

    ACK fa1111e69afdbe42bf062a7fb61ce6bd748a8241

    Rebased locally and ran all tests.

  43. refactor: Use NodeClock::time_point for m_addr_token_timestamp
    This refactor makes the field a bit more type-safe.
    3333c5023f
  44. refactor: Use current_time over redundant call to Now()
    This also better reflects the meaning and use.
    faaea7895f
  45. maflcko force-pushed on Mar 20, 2026
  46. l0rinc commented at 6:16 AM on March 20, 2026: contributor

    ACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

  47. DrahtBot requested review from sedited on Mar 20, 2026
  48. ajtowns commented at 7:29 AM on March 20, 2026: contributor

    reACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

  49. sedited approved
  50. sedited commented at 8:29 AM on March 20, 2026: contributor

    Re-ACK faaea7895fd4341ebb81a6f3c7e0919bf9d29bf9

  51. sedited merged this on Mar 20, 2026
  52. sedited closed this on Mar 20, 2026
  53. maflcko deleted the branch on Mar 20, 2026
Contributors
maflckoDrahtBotajtownsl0rincsedited


ajtowns

Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me