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

fadd8b2676f6d68ec87189871461c9a6a6aa3cac 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 fadd8b2676f6d68ec87189871461c9a6a6aa3cac

Tree-SHA512: b6c178fa01161544e5bc76c4cb23e11bcc30391f7b7a64accce864923766647bcfce2e8ae21d36fb1ffc1afa07bc46415aca612405bd8d4cc1f319c92a08498f

9 files changed, 28 insertions, 29 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 857ad73b2f..f16ff2230b 100644
--- a/src/addrman.cpp
+++ b/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;
             }
         }
diff --git a/src/addrman.h b/src/addrman.h
index b70c6a48ad..5099c8c7a3 100644
--- a/src/addrman.h
+++ b/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 @@ public:
      * @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 @@ public:
      * @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);
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
index a73a026940..376e79f49f 100644
--- a/src/addrman_impl.h
+++ b/src/addrman_impl.h
@@ -93,10 +93,10 @@ public:
     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
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index 2600b03022..f14d1f89b6 100644
--- a/src/bench/addrman.cpp
+++ b/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;
     };
diff --git a/src/net.cpp b/src/net.cpp
index c4aaac4986..e87ac079b5 100644
--- a/src/net.cpp
+++ b/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;
             }
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2e11390541..64c2a29245 100644
--- a/src/net_processing.cpp
+++ b/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);
         }
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 0ee905a77a..3b234bc317 100644
--- a/src/rpc/net.cpp
+++ b/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)) {
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index b1372a3e98..b10d32ccec 100644
--- a/src/test/addrman_tests.cpp
+++ b/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);
diff --git a/src/timedata.h b/src/timedata.h
index ed2d8639f7..cfe5111644 100644
--- a/src/timedata.h
+++ b/src/timedata.h
@@ -76,7 +76,6 @@ public:
 /** 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);
 
 /**