merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging

src/addrman.cpp

@@ -552,6 +552,83 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
     info.fInTried = true;
 }
 
+bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
+{
+    AssertLockHeld(cs);
+
+    if (!addr.IsRoutable())
+        return false;
+
+    int nId;
+    AddrInfo* pinfo = Find(addr, &nId);
+
+    // Do not set a penalty for a source's self-announcement
+    if (addr == source) {
+        nTimePenalty = 0;
+    }
+
+    if (pinfo) {
+        // periodically update nTime
+        bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60);
+        int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60);
+        if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) {
+            pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty);
+        }
+
+        // add services
+        pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices);
+
+        // do not update if no new information is present
+        if (addr.nTime <= pinfo->nTime) {
+            return false;
+        }
+
+        // do not update if the entry was already in the "tried" table
+        if (pinfo->fInTried)
+            return false;
+
+        // do not update if the max reference count is reached
+        if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS)
+            return false;
+
+        // stochastic test: previous nRefCount == N: 2^N times harder to increase it
+        int nFactor = 1;
+        for (int n = 0; n < pinfo->nRefCount; n++)
+            nFactor *= 2;
+        if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0))
+            return false;
+    } else {
+        pinfo = Create(addr, source, &nId);
+        pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty);
+        nNew++;
+    }
+
+    int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap);
+    int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket);
+    bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
+    if (vvNew[nUBucket][nUBucketPos] != nId) {
+        if (!fInsert) {
+            AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]];
+            if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) {
+                // Overwrite the existing new table entry.
+                fInsert = true;
+            }
+        }
+        if (fInsert) {
+            ClearNew(nUBucket, nUBucketPos);
+            pinfo->nRefCount++;
+            vvNew[nUBucket][nUBucketPos] = nId;
+            LogPrint(BCLog::ADDRMAN, "Added %s mapped to AS%i to new[%i][%i]\n",
+                     addr.ToString(), addr.GetMappedAS(m_asmap), nUBucket, nUBucketPos);
+        } else {
+            if (pinfo->nRefCount == 0) {
+                Delete(nId);
+            }
+        }
+    }
+    return fInsert;
+}
+
 void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
 {
     AssertLockHeld(cs);
@@ -615,83 +692,16 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     }
 }
 
-bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
+bool AddrManImpl::Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty)
 {
-    AssertLockHeld(cs);
+    int added{0};
-
+    for (std::vector<CAddress>::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) {
-    if (!addr.IsRoutable())
+        added += AddSingle(*it, source, nTimePenalty) ? 1 : 0;
-        return false;
-
-    bool fNew = false;
-    int nId;
-    AddrInfo* pinfo = Find(addr, &nId);
-
-    // Do not set a penalty for a source's self-announcement
-    if (addr == source) {
-        nTimePenalty = 0;
     }
-
+    if (added > 0) {
-    if (pinfo) {
+        LogPrint(BCLog::ADDRMAN, "Added %i addresses (of %i) from %s: %i tried, %i new\n", added, vAddr.size(), source.ToString(), nTried, nNew);
-        // periodically update nTime
-        bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60);
-        int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60);
-        if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) {
-            pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty);
-        }
-
-        // add services
-        pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices);
-
-        // do not update if no new information is present
-        if (addr.nTime <= pinfo->nTime) {
-            return false;
-        }
-
-        // do not update if the entry was already in the "tried" table
-        if (pinfo->fInTried)
-            return false;
-
-        // do not update if the max reference count is reached
-        if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS)
-            return false;
-
-        // stochastic test: previous nRefCount == N: 2^N times harder to increase it
-        int nFactor = 1;
-        for (int n = 0; n < pinfo->nRefCount; n++)
-            nFactor *= 2;
-        if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0))
-            return false;
-    } else {
-        pinfo = Create(addr, source, &nId);
-        pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty);
-        nNew++;
-        fNew = true;
     }
-
+    return added > 0;
-    int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap);
-    int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket);
-    if (vvNew[nUBucket][nUBucketPos] != nId) {
-        bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
-        if (!fInsert) {
-            AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]];
-            if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) {
-                // Overwrite the existing new table entry.
-                fInsert = true;
-            }
-        }
-        if (fInsert) {
-            ClearNew(nUBucket, nUBucketPos);
-            pinfo->nRefCount++;
-            vvNew[nUBucket][nUBucketPos] = nId;
-            LogPrint(BCLog::ADDRMAN, "Added %s mapped to AS%i to new[%i][%i]\n",
-                     addr.ToString(), addr.GetMappedAS(m_asmap), nUBucket, nUBucketPos);
-        } else {
-            if (pinfo->nRefCount == 0) {
-                Delete(nId);
-            }
-        }
-    }
-    return fNew;
 }
 
 void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -1087,15 +1097,10 @@ size_t AddrManImpl::size() const
 bool AddrManImpl::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
 {
     LOCK(cs);
-    int nAdd = 0;
     Check();
-    for (std::vector<CAddress>::const_iterator it = vAddr.begin(); it != vAddr.end(); it++)
+    auto ret = Add_(vAddr, source, nTimePenalty);
-        nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0;
     Check();
-    if (nAdd) {
+    return ret;
-        LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew);
-    }
-    return nAdd > 0;
 }
 
 void AddrManImpl::Good(const CService& addr, int64_t nTime)

src/addrman.h

@@ -70,7 +70,15 @@ public:
     //! Return the number of (unique) addresses in all tables.
     size_t size() const;
 
-    //! Add addresses to addrman's new table.
+    /**
+     * Attempt to add one or more addresses to addrman's new table.
+     *
+     * @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
+     *                            sends us an address record with nTime=n, then we'll add it to our
+     *                            addrman with nTime=(n - nTimePenalty).
+     * @return    true if at least one address is successfully added. */
     bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0);
 
     //! Mark an entry as accessible, possibly moving it from "new" to "tried".

src/addrman_impl.h

@@ -248,9 +248,13 @@ private:
     //! Move an entry from the "new" table(s) to the "tried" table
     void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
+    /** 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);
+
     void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    bool Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
 

src/test/addrman_tests.cpp

@@ -374,7 +374,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
     //Test: tried table collision!
     CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
     uint32_t collisions{1};
-    BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source));
+    BOOST_CHECK(!addrman.Add({CAddress(addr1, NODE_NONE)}, source));
     BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
 
     CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));

test/functional/p2p_addr_relay.py

@@ -148,7 +148,6 @@ class AddrTest(BitcoinTestFramework):
         msg = self.setup_addr_msg(num_ipv4_addrs)
         with self.nodes[0].assert_debug_log(
             [
-                'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs),
                 'received: addr (301 bytes) peer=1',
             ]
         ):

test/functional/p2p_addrv2_relay.py

@@ -74,9 +74,6 @@ class AddrTest(BitcoinTestFramework):
         addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
         msg.addrs = ADDRS
         with self.nodes[0].assert_debug_log([
-                # The I2P address is not added to node's own addrman because it has no
-                # I2P reachability (thus 10 - 1 = 9).
-                'Added 9 addresses from 127.0.0.1: 0 tried',
                 'received: addrv2 (159 bytes) peer=1',
         ]):
             addr_source.send_and_ping(msg)