Merge bitcoin/bitcoin#24697: refactor address relay time

fa64dd6 refactor: Use type-safe std::chrono for addrman time (MarcoFalke)
fa2ae37 Add type-safe AdjustedTime() getter to timedata (MarcoFalke)
fa5103a Add ChronoFormatter to serialize (MarcoFalke)
fa253d3 util: Add HoursDouble (MarcoFalke)
fa21fc6 scripted-diff: Rename addrman time symbols (MarcoFalke)
fa9284c refactor: Remove not needed std::max (MacroFake)

Pull request description:

  Those refactors are overlapping with, but otherwise largely unrelated to #24662.

ACKs for top commit:
  naumenkogs:
    utACK fa64dd6
  dergoegge:
    Code review ACK fa64dd6

Tree-SHA512: a50625e78036e7220a11997e6d9b6c6b317cb38ce02b1835fb41cbee2d8bfb1faf29b29d8990be78d6b5e15e9a9d8dec33bf25fa439b47610ef708950969724b

Author: fanquake

src/addrman.cpp

@@ -11,6 +11,7 @@
 #include <protocol.h>
 #include <streams.h>
 #include <timedata.h>
+#include <util/time.h>
 
 #include <cstdint>
 #include <memory>
@@ -107,23 +108,23 @@ class AddrMan
      *
      * @param[in] vAddr           Address records to attempt to add.
      * @param[in] source          The address of the node that sent us these addr records.
-     * @param[in] nTimePenalty    A "time penalty" to apply to the address record's nTime. If a peer
+     * @param[in] time_penalty    A "time penalty" to apply to the address record's nTime. If a peer
      *                            sends us an address record with nTime=n, then we'll add it to our
-     *                            addrman with nTime=(n - nTimePenalty).
+     *                            addrman with nTime=(n - time_penalty).
      * @return    true if at least one address is successfully added. */
-    bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0);
+    bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty = 0s);
 
     /**
      * Mark an address record as accessible and attempt to move it to addrman's tried table.
      *
      * @param[in] addr            Address record to attempt to move to tried table.
-     * @param[in] nTime           The time that we were last connected to this peer.
+     * @param[in] time            The time that we were last connected to this peer.
      * @return    true if the address is successfully moved from the new table to the tried table.
      */
-    bool Good(const CService& addr, int64_t nTime = GetAdjustedTime());
+    bool Good(const CService& addr, NodeSeconds time = AdjustedTime());
 
     //! Mark an entry as connection attempted to.
-    void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime());
+    void Attempt(const CService& addr, bool fCountFailure, NodeSeconds time = AdjustedTime());
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     void ResolveCollisions();
@@ -133,18 +134,18 @@ class AddrMan
      * attempting to evict.
      *
      * @return CAddress The record for the selected tried peer.
-     *         int64_t  The last time we attempted to connect to that peer.
+     *         seconds  The last time we attempted to connect to that peer.
      */
-    std::pair<CAddress, int64_t> SelectTriedCollision();
+    std::pair<CAddress, NodeSeconds> SelectTriedCollision();
 
     /**
      * Choose an address to connect to.
      *
      * @param[in] newOnly  Whether to only select addresses from the new table.
      * @return    CAddress The record for the selected peer.
-     *            int64_t  The last time we attempted to connect to that peer.
+     *            seconds  The last time we attempted to connect to that peer.
      */
-    std::pair<CAddress, int64_t> Select(bool newOnly = false) const;
+    std::pair<CAddress, NodeSeconds> Select(bool newOnly = false) const;
 
     /**
      * Return all or many randomly selected addresses, optionally by network.
@@ -166,9 +167,9 @@ class AddrMan
      *  not leak information about currently connected peers.
      *
      * @param[in]   addr     The address of the peer we were connected to
-     * @param[in]   nTime    The time that we were last connected to this peer
+     * @param[in]   time     The time that we were last connected to this peer
      */
-    void Connected(const CService& addr, int64_t nTime = GetAdjustedTime());
+    void Connected(const CService& addr, NodeSeconds time = AdjustedTime());
 
     //! Update an entry's service bits.
     void SetServices(const CService& addr, ServiceFlags nServices);

src/addrman.h

@@ -11,7 +11,9 @@
 #include <protocol.h>
 #include <serialize.h>
 #include <sync.h>
+#include <timedata.h>
 #include <uint256.h>
+#include <util/time.h>
 
 #include <cstdint>
 #include <optional>
@@ -38,16 +40,16 @@ class AddrInfo : public CAddress
 {
 public:
     //! last try whatsoever by us (memory only)
-    int64_t nLastTry{0};
+    NodeSeconds m_last_try{0s};
 
     //! last counted attempt (memory only)
-    int64_t nLastCountAttempt{0};
+    NodeSeconds m_last_count_attempt{0s};
 
     //! where knowledge about this address first came from
     CNetAddr source;
 
     //! last successful connection by us
-    int64_t nLastSuccess{0};
+    NodeSeconds m_last_success{0s};
 
     //! connection attempts since last successful attempt
     int nAttempts{0};
@@ -64,7 +66,7 @@ class AddrInfo : public CAddress
     SERIALIZE_METHODS(AddrInfo, obj)
     {
         READWRITEAS(CAddress, obj);
-        READWRITE(obj.source, obj.nLastSuccess, obj.nAttempts);
+        READWRITE(obj.source, Using<ChronoFormatter<int64_t>>(obj.m_last_success), obj.nAttempts);
     }
 
     AddrInfo(const CAddress &addrIn, const CNetAddr &addrSource) : CAddress(addrIn), source(addrSource)
@@ -91,10 +93,10 @@ class AddrInfo : public CAddress
     int GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const;
 
     //! Determine whether the statistics about this entry are bad enough so that it can just be deleted
-    bool IsTerrible(int64_t nNow = GetAdjustedTime()) const;
+    bool IsTerrible(NodeSeconds now = AdjustedTime()) const;
 
     //! Calculate the relative chance this entry should be given when selecting nodes to connect to
-    double GetChance(int64_t nNow = GetAdjustedTime()) const;
+    double GetChance(NodeSeconds now = AdjustedTime()) const;
 };
 
 class AddrManImpl
@@ -112,26 +114,26 @@ class AddrManImpl
 
     size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
+    bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    bool Good(const CService& addr, int64_t nTime)
+    bool Good(const CService& addr, NodeSeconds time)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    void Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
+    void Attempt(const CService& addr, bool fCountFailure, NodeSeconds time)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    std::pair<CAddress, int64_t> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    std::pair<CAddress, NodeSeconds> SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    std::pair<CAddress, int64_t> Select(bool newOnly) const
+    std::pair<CAddress, NodeSeconds> Select(bool newOnly) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    void Connected(const CService& addr, int64_t nTime)
+    void Connected(const CService& addr, NodeSeconds time)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     void SetServices(const CService& addr, ServiceFlags nServices)
@@ -202,7 +204,7 @@ class AddrManImpl
     int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
 
     //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.
-    int64_t nLastGood GUARDED_BY(cs){1};
+    NodeSeconds m_last_good GUARDED_BY(cs){1s};
 
     //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
     std::set<int> m_tried_collisions;
@@ -233,25 +235,25 @@ class AddrManImpl
 
     /** Attempt to add a single address to addrman's new table.
      *  @see AddrMan::Add() for parameters. */
-    bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    bool AddSingle(const CAddress& addr, const CNetAddr& source, std::chrono::seconds time_penalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    bool Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    bool Good_(const CService& addr, bool test_before_evict, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    bool Add_(const std::vector<CAddress>& vAddr, const CNetAddr& source, std::chrono::seconds time_penalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void Attempt_(const CService& addr, bool fCountFailure, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    std::pair<CAddress, int64_t> Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::pair<CAddress, NodeSeconds> Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void Connected_(const CService& addr, NodeSeconds time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     void SetServices_(const CService& addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    std::pair<CAddress, int64_t> SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
+    std::pair<CAddress, NodeSeconds> SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     std::optional<AddressPosition> FindAddressEntry_(const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 

src/addrman_impl.h

@@ -43,7 +43,7 @@ static void CreateAddresses()
 
         CAddress ret(CService(addr, port), NODE_NETWORK);
 
-        ret.nTime = GetAdjustedTime();
+        ret.nTime = AdjustedTime();
 
         return ret;
     };

src/bench/addrman.cpp

@@ -187,15 +187,15 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
     // it'll get a pile of addresses with newer timestamps.
     // Seed nodes are given a random 'last seen time' of between one and two
     // weeks ago.
-    const int64_t nOneWeek = 7*24*60*60;
+    const auto one_week{7 * 24h};
     std::vector<CAddress> vSeedsOut;
     FastRandomContext rng;
     CDataStream s(vSeedsIn, SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT);
     while (!s.eof()) {
         CService endpoint;
         s >> endpoint;
         CAddress addr{endpoint, GetDesirableServiceFlags(NODE_NONE)};
-        addr.nTime = GetTime() - rng.randrange(nOneWeek) - nOneWeek;
+        addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - one_week, -one_week);
         LogPrint(BCLog::NET, "Added hardcoded seed: %s\n", addr.ToString());
         vSeedsOut.push_back(addr);
     }
@@ -452,10 +452,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
         }
     }
 
-    /// debug print
     LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "trying connection %s lastseen=%.1fhrs\n",
-                  pszDest ? pszDest : addrConnect.ToString(),
-                  pszDest ? 0.0 : (double)(GetAdjustedTime() - addrConnect.nTime) / 3600.0);
+        pszDest ? pszDest : addrConnect.ToString(),
+        Ticks<HoursDouble>(pszDest ? 0h : AdjustedTime() - addrConnect.nTime));
 
     // Resolve
     const uint16_t default_port{pszDest != nullptr ? Params().GetDefaultPort(pszDest) :
@@ -1469,9 +1468,8 @@ void CConnman::ThreadDNSAddressSeed()
             unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
             if (LookupHost(host, vIPs, nMaxIPs, true)) {
                 for (const CNetAddr& ip : vIPs) {
-                    int nOneDay = 24*3600;
                     CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
-                    addr.nTime = GetTime() - 3*nOneDay - rng.randrange(4*nOneDay); // use a random age between 3 and 7 days old
+                    addr.nTime = rng.rand_uniform_delay(Now<NodeSeconds>() - 3 * 24h, -4 * 24h); // use a random age between 3 and 7 days old
                     vAdd.push_back(addr);
                     found++;
                 }
@@ -1737,7 +1735,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 
         addrman.ResolveCollisions();
 
-        int64_t nANow = GetAdjustedTime();
+        const auto nANow{AdjustedTime()};
         int nTries = 0;
         while (!interruptNet)
         {
@@ -1760,7 +1758,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 break;
 
             CAddress addr;
-            int64_t addr_last_try{0};
+            NodeSeconds addr_last_try{0s};
 
             if (fFeeler) {
                 // First, try to get a tried table collision address. This returns
@@ -1800,8 +1798,9 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                 continue;
 
             // only consider very recently tried nodes after 30 failed attempts
-            if (nANow - addr_last_try < 600 && nTries < 30)
+            if (nANow - addr_last_try < 10min && nTries < 30) {
                 continue;
+            }
 
             // for non-feelers, require all the services we'll want,
             // for feelers, only require they be a full node (only because most

src/net.cpp

@@ -29,6 +29,7 @@
 #include <scheduler.h>
 #include <streams.h>
 #include <sync.h>
+#include <timedata.h>
 #include <tinyformat.h>
 #include <txmempool.h>
 #include <txorphanage.h>
@@ -2929,7 +2930,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             // indicate to the peer that we will participate in addr relay.
             if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload())
             {
-                CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, (uint32_t)GetAdjustedTime()};
+                CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, AdjustedTime()};
                 FastRandomContext insecure_rand;
                 if (addr.IsRoutable())
                 {
@@ -3134,8 +3135,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         // Store the new addresses
         std::vector<CAddress> vAddrOk;
-        int64_t nNow = GetAdjustedTime();
-        int64_t nSince = nNow - 10 * 60;
+        const auto current_a_time{AdjustedTime()};
 
         // Update/increment addr rate limiting bucket.
         const auto current_time{GetTime<std::chrono::microseconds>()};
@@ -3171,16 +3171,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             if (!MayHaveUsefulAddressDB(addr.nServices) && !HasAllDesirableServiceFlags(addr.nServices))
                 continue;
 
-            if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
-                addr.nTime = nNow - 5 * 24 * 60 * 60;
+            if (addr.nTime <= NodeSeconds{100000000s} || addr.nTime > current_a_time + 10min) {
+                addr.nTime = current_a_time - 5 * 24h;
+            }
             AddAddressKnown(*peer, addr);
             if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) {
                 // Do not process banned/discouraged addresses beyond remembering we received them
                 continue;
             }
             ++num_proc;
             bool fReachable = IsReachable(addr);
-            if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {
+            if (addr.nTime > current_a_time - 10min && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) {
                 // Relay to a limited number of other nodes
                 RelayAddress(pfrom.GetId(), addr, fReachable);
             }
@@ -3193,7 +3194,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d\n",
                  vAddr.size(), num_proc, num_rate_limit, pfrom.GetId());
 
-        m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60);
+        m_addrman.Add(vAddrOk, pfrom.addr, 2h);
         if (vAddr.size() < 1000) peer->m_getaddr_sent = false;
 
         // AddrFetch: Require multiple addresses to avoid disconnecting on self-announcements
@@ -4685,7 +4686,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
             peer.m_addr_known->reset();
         }
         if (std::optional<CService> local_service = GetLocalAddrForPeer(node)) {
-            CAddress local_addr{*local_service, peer.m_our_services, (uint32_t)GetAdjustedTime()};
+            CAddress local_addr{*local_service, peer.m_our_services, AdjustedTime()};
             FastRandomContext insecure_rand;
             PushAddress(peer, local_addr, insecure_rand);
         }

src/net_processing.cpp

@@ -36,7 +36,6 @@
 #include <shutdown.h>
 #include <support/allocators/secure.h>
 #include <sync.h>
-#include <timedata.h>
 #include <txmempool.h>
 #include <uint256.h>
 #include <univalue.h>

src/node/interfaces.cpp

@@ -11,6 +11,7 @@
 #include <serialize.h>
 #include <streams.h>
 #include <uint256.h>
+#include <util/time.h>
 
 #include <cstdint>
 #include <limits>
@@ -352,7 +353,7 @@ static inline bool MayHaveUsefulAddressDB(ServiceFlags services)
 /** A CService with information about it as peer */
 class CAddress : public CService
 {
-    static constexpr uint32_t TIME_INIT{100000000};
+    static constexpr std::chrono::seconds TIME_INIT{100000000};
 
     /** Historically, CAddress disk serialization stored the CLIENT_VERSION, optionally OR'ed with
      *  the ADDRV2_FORMAT flag to indicate V2 serialization. The first field has since been
@@ -382,7 +383,7 @@ class CAddress : public CService
 public:
     CAddress() : CService{} {};
     CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
-    CAddress(CService ipIn, ServiceFlags nServicesIn, uint32_t nTimeIn) : CService{ipIn}, nTime{nTimeIn}, nServices{nServicesIn} {};
+    CAddress(CService ipIn, ServiceFlags nServicesIn, NodeSeconds time) : CService{ipIn}, nTime{time}, nServices{nServicesIn} {};
 
     SERIALIZE_METHODS(CAddress, obj)
     {
@@ -415,7 +416,7 @@ class CAddress : public CService
             use_v2 = s.GetVersion() & ADDRV2_FORMAT;
         }
 
-        READWRITE(obj.nTime);
+        READWRITE(Using<LossyChronoFormatter<uint32_t>>(obj.nTime));
         // nServices is serialized as CompactSize in V2; as uint64_t in V1.
         if (use_v2) {
             uint64_t services_tmp;
@@ -430,8 +431,8 @@ class CAddress : public CService
         SerReadWriteMany(os, ser_action, ReadWriteAsHelper<CService>(obj));
     }
 
-    //! Always included in serialization.
-    uint32_t nTime{TIME_INIT};
+    //! Always included in serialization. The behavior is unspecified if the value is not representable as uint32_t.
+    NodeSeconds nTime{TIME_INIT};
     //! Serialized as uint64_t in V1, and as CompactSize in V2.
     ServiceFlags nServices{NODE_NONE};