Merge bitcoin/bitcoin#24662: addrman: Use system time instead of adjusted network time

fadd8b2 addrman: Use system time instead of adjusted network time (MarcoFalke)

Pull request description:

  This changes addrman to use system time for address relay instead of the network adjusted time.

  This is an improvement, because network time has multiple issues:

  * It is non-monotonic, even if the system time is monotonic.
  * It may be wrong, even if the system time is correct.
  * It may be wrong, if the system time is wrong. For example, when the node has limited number of connections (`4`), or the system time is wrong by too much (more than +-70 minutes), or the system time only got wrong after timedata collected more than half of the entries while the time was correct, ...)

  This may slightly degrade addr relay for nodes where timedata successfully adjusted the time. Addr relay can already deal with minor offsets of up to 10 minutes. Offsets larger than this should still allow addr relay and not result in a DoS.

ACKs for top commit:
  dergoegge:
    Code review ACK fadd8b2

Tree-SHA512: b6c178fa01161544e5bc76c4cb23e11bcc30391f7b7a64accce864923766647bcfce2e8ae21d36fb1ffc1afa07bc46415aca612405bd8d4cc1f319c92a08498f

Author: fanquake

src/addrman.cpp

@@ -14,10 +14,10 @@
 #include <random.h>
 #include <serialize.h>
 #include <streams.h>
-#include <timedata.h>
 #include <tinyformat.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/time.h>
 
 #include <cmath>
 #include <optional>
@@ -560,7 +560,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
 
     if (pinfo) {
         // periodically update nTime
-        const bool currently_online{AdjustedTime() - addr.nTime < 24h};
+        const bool currently_online{NodeClock::now() - addr.nTime < 24h};
         const auto update_interval{currently_online ? 1h : 24h};
         if (pinfo->nTime < addr.nTime - update_interval - time_penalty) {
             pinfo->nTime = std::max(NodeSeconds{0s}, addr.nTime - time_penalty);
@@ -788,7 +788,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
     }
 
     // gather a list of random nodes, skipping those of low quality
-    const auto now{AdjustedTime()};
+    const auto now{Now<NodeSeconds>()};
     std::vector<CAddress> addresses;
     for (unsigned int n = 0; n < vRandom.size(); n++) {
         if (addresses.size() >= nNodes)
@@ -874,7 +874,7 @@ void AddrManImpl::ResolveCollisions_()
                 int id_old = vvTried[tried_bucket][tried_bucket_pos];
                 AddrInfo& info_old = mapInfo[id_old];
 
-                const auto current_time{AdjustedTime()};
+                const auto current_time{Now<NodeSeconds>()};
 
                 // Has successfully connected in last X hours
                 if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
@@ -898,7 +898,7 @@ void AddrManImpl::ResolveCollisions_()
                     erase_collision = true;
                 }
             } else { // Collision is not actually a collision anymore
-                Good_(info_new, false, AdjustedTime());
+                Good_(info_new, false, Now<NodeSeconds>());
                 erase_collision = true;
             }
         }

src/addrman.h

@@ -10,7 +10,6 @@
 #include <netgroup.h>
 #include <protocol.h>
 #include <streams.h>
-#include <timedata.h>
 #include <util/time.h>
 
 #include <cstdint>
@@ -121,10 +120,10 @@ class AddrMan
      * @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, NodeSeconds time = AdjustedTime());
+    bool Good(const CService& addr, NodeSeconds time = Now<NodeSeconds>());
 
     //! Mark an entry as connection attempted to.
-    void Attempt(const CService& addr, bool fCountFailure, NodeSeconds time = AdjustedTime());
+    void Attempt(const CService& addr, bool fCountFailure, NodeSeconds time = Now<NodeSeconds>());
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     void ResolveCollisions();
@@ -169,7 +168,7 @@ class AddrMan
      * @param[in]   addr     The address of the peer we were connected to
      * @param[in]   time     The time that we were last connected to this peer
      */
-    void Connected(const CService& addr, NodeSeconds time = AdjustedTime());
+    void Connected(const CService& addr, NodeSeconds time = Now<NodeSeconds>());
 
     //! Update an entry's service bits.
     void SetServices(const CService& addr, ServiceFlags nServices);

src/addrman_impl.h

@@ -93,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(NodeSeconds now = AdjustedTime()) const;
+    bool IsTerrible(NodeSeconds now = Now<NodeSeconds>()) const;
 
     //! Calculate the relative chance this entry should be given when selecting nodes to connect to
-    double GetChance(NodeSeconds now = AdjustedTime()) const;
+    double GetChance(NodeSeconds now = Now<NodeSeconds>()) const;
 };
 
 class AddrManImpl

src/bench/addrman.cpp

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

src/net.cpp

@@ -454,7 +454,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 
     LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "trying connection %s lastseen=%.1fhrs\n",
         pszDest ? pszDest : addrConnect.ToString(),
-        Ticks<HoursDouble>(pszDest ? 0h : AdjustedTime() - addrConnect.nTime));
+        Ticks<HoursDouble>(pszDest ? 0h : Now<NodeSeconds>() - addrConnect.nTime));
 
     // Resolve
     const uint16_t default_port{pszDest != nullptr ? Params().GetDefaultPort(pszDest) :
@@ -1735,7 +1735,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
 
         addrman.ResolveCollisions();
 
-        const auto nANow{AdjustedTime()};
+        const auto current_time{NodeClock::now()};
         int nTries = 0;
         while (!interruptNet)
         {
@@ -1798,7 +1798,7 @@ 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 < 10min && nTries < 30) {
+            if (current_time - addr_last_try < 10min && nTries < 30) {
                 continue;
             }
 

src/net_processing.cpp

@@ -2930,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, AdjustedTime()};
+                CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, Now<NodeSeconds>()};
                 FastRandomContext insecure_rand;
                 if (addr.IsRoutable())
                 {
@@ -3135,7 +3135,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         // Store the new addresses
         std::vector<CAddress> vAddrOk;
-        const auto current_a_time{AdjustedTime()};
+        const auto current_a_time{Now<NodeSeconds>()};
 
         // Update/increment addr rate limiting bucket.
         const auto current_time{GetTime<std::chrono::microseconds>()};
@@ -4683,7 +4683,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, AdjustedTime()};
+            CAddress local_addr{*local_service, peer.m_our_services, Now<NodeSeconds>()};
             FastRandomContext insecure_rand;
             PushAddress(peer, local_addr, insecure_rand);
         }

src/rpc/net.cpp

@@ -23,6 +23,7 @@
 #include <timedata.h>
 #include <util/strencodings.h>
 #include <util/string.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <version.h>
@@ -945,7 +946,7 @@ static RPCHelpMan addpeeraddress()
 
     if (LookupHost(addr_string, net_addr, false)) {
         CAddress address{{net_addr, port}, ServiceFlags{NODE_NETWORK | NODE_WITNESS}};
-        address.nTime = AdjustedTime();
+        address.nTime = Now<NodeSeconds>();
         // The source address is set equal to the address. This is equivalent to the peer
         // announcing itself.
         if (node.addrman->Add({address}, address)) {

src/test/addrman_tests.cpp

@@ -225,7 +225,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_multiplicity)
 {
     auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node));
     CAddress addr{CAddress(ResolveService("253.3.3.3", 8333), NODE_NONE)};
-    const auto start_time{AdjustedTime()};
+    const auto start_time{Now<NodeSeconds>()};
     addr.nTime = start_time;
 
     // test that multiplicity stays at 1 if nTime doesn't increase
@@ -295,15 +295,15 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
     BOOST_CHECK_EQUAL(vAddr1.size(), 0U);
 
     CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE);
-    addr1.nTime = AdjustedTime(); // Set time so isTerrible = false
+    addr1.nTime = Now<NodeSeconds>(); // Set time so isTerrible = false
     CAddress addr2 = CAddress(ResolveService("250.251.2.2", 9999), NODE_NONE);
-    addr2.nTime = AdjustedTime();
+    addr2.nTime = Now<NodeSeconds>();
     CAddress addr3 = CAddress(ResolveService("251.252.2.3", 8333), NODE_NONE);
-    addr3.nTime = AdjustedTime();
+    addr3.nTime = Now<NodeSeconds>();
     CAddress addr4 = CAddress(ResolveService("252.253.3.4", 8333), NODE_NONE);
-    addr4.nTime = AdjustedTime();
+    addr4.nTime = Now<NodeSeconds>();
     CAddress addr5 = CAddress(ResolveService("252.254.4.5", 8333), NODE_NONE);
-    addr5.nTime = AdjustedTime();
+    addr5.nTime = Now<NodeSeconds>();
     CNetAddr source1 = ResolveIP("250.1.2.1");
     CNetAddr source2 = ResolveIP("250.2.3.3");
 
@@ -329,7 +329,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
         CAddress addr = CAddress(ResolveService(strAddr), NODE_NONE);
 
         // Ensure that for all addrs in addrman, isTerrible == false.
-        addr.nTime = AdjustedTime();
+        addr.nTime = Now<NodeSeconds>();
         addrman->Add({addr}, ResolveIP(strAddr));
         if (i % 8 == 0)
             addrman->Good(addr);
@@ -822,7 +822,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
     // Ensure test of address fails, so that it is evicted.
     // Update entry in tried by setting last good connection in the deep past.
     BOOST_CHECK(!addrman->Good(info, NodeSeconds{1s}));
-    addrman->Attempt(info, /*fCountFailure=*/false, AdjustedTime() - 61s);
+    addrman->Attempt(info, /*fCountFailure=*/false, Now<NodeSeconds>() - 61s);
 
     // Should swap 36 for 19.
     addrman->ResolveCollisions();
@@ -966,7 +966,7 @@ BOOST_AUTO_TEST_CASE(addrman_update_address)
     CNetAddr source{ResolveIP("252.2.2.2")};
     CAddress addr{CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE)};
 
-    const auto start_time{AdjustedTime() - 10000s};
+    const auto start_time{Now<NodeSeconds>() - 10000s};
     addr.nTime = start_time;
     BOOST_CHECK(addrman->Add({addr}, source));
     BOOST_CHECK_EQUAL(addrman->size(), 1U);

src/timedata.h

@@ -76,7 +76,6 @@ class CMedianFilter
 /** Functions to keep track of adjusted P2P time */
 int64_t GetTimeOffset();
 int64_t GetAdjustedTime();
-inline NodeSeconds AdjustedTime() { return Now<NodeSeconds>() + std::chrono::seconds{GetTimeOffset()}; }
 void AddTimeData(const CNetAddr& ip, int64_t nTime);
 
 /**