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

Author: kwvg

src/addrman.cpp

@@ -552,77 +552,13 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
     info.fInTried = true;
 }
 
-void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
-{
-    AssertLockHeld(cs);
-
-    int nId;
-
-    nLastGood = nTime;
-
-    AddrInfo* pinfo = Find(addr, &nId);
-
-    // if not found, bail out
-    if (!pinfo)
-        return;
-
-    AddrInfo& info = *pinfo;
-
-    // check whether we are talking about the exact same CService (including same port)
-    if (!m_discriminate_ports && info != addr)
-        return;
-
-    // update info
-    info.nLastSuccess = nTime;
-    info.nLastTry = nTime;
-    info.nAttempts = 0;
-    // nTime is not updated here, to avoid leaking information about
-    // currently-connected peers.
-
-    // if it is already in the tried set, don't do anything else
-    if (info.fInTried)
-        return;
-
-    // if it is not in new, something bad happened
-    if (!Assume(info.nRefCount > 0)) {
-        return;
-    }
-
-    // which tried bucket to move the entry to
-    int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
-    int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);
-
-    // Will moving this address into tried evict another entry?
-    if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
-        if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
-            m_tried_collisions.insert(nId);
-        }
-        // Output the entry we'd be colliding with, for debugging purposes
-        auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]);
-        if (fLogIPs) {
-            LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n",
-                     colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
-                     addr.ToString(),
-                     m_tried_collisions.size());
-        }
-    } else {
-        // move nId to the tried tables
-        MakeTried(info, nId);
-        if (fLogIPs) {
-            LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n",
-                     addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos);
-        }
-    }
-}
-
-bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
+bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
 {
     AssertLockHeld(cs);
 
     if (!addr.IsRoutable())
         return false;
 
-    bool fNew = false;
     int nId;
     AddrInfo* pinfo = Find(addr, &nId);
 
@@ -665,13 +601,12 @@ bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTi
         pinfo = Create(addr, source, &nId);
         pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty);
         nNew++;
-        fNew = true;
     }
 
     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) {
-        bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
         if (!fInsert) {
             AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]];
             if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) {
@@ -691,7 +626,82 @@ bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTi
             }
         }
     }
-    return fNew;
+    return fInsert;
+}
+
+void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
+{
+    AssertLockHeld(cs);
+
+    int nId;
+
+    nLastGood = nTime;
+
+    AddrInfo* pinfo = Find(addr, &nId);
+
+    // if not found, bail out
+    if (!pinfo)
+        return;
+
+    AddrInfo& info = *pinfo;
+
+    // check whether we are talking about the exact same CService (including same port)
+    if (!m_discriminate_ports && info != addr)
+        return;
+
+    // update info
+    info.nLastSuccess = nTime;
+    info.nLastTry = nTime;
+    info.nAttempts = 0;
+    // nTime is not updated here, to avoid leaking information about
+    // currently-connected peers.
+
+    // if it is already in the tried set, don't do anything else
+    if (info.fInTried)
+        return;
+
+    // if it is not in new, something bad happened
+    if (!Assume(info.nRefCount > 0)) {
+        return;
+    }
+
+    // which tried bucket to move the entry to
+    int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
+    int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);
+
+    // Will moving this address into tried evict another entry?
+    if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
+        if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
+            m_tried_collisions.insert(nId);
+        }
+        // Output the entry we'd be colliding with, for debugging purposes
+        auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]);
+        if (fLogIPs) {
+            LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n",
+                     colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
+                     addr.ToString(),
+                     m_tried_collisions.size());
+        }
+    } else {
+        // move nId to the tried tables
+        MakeTried(info, nId);
+        if (fLogIPs) {
+            LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n",
+                     addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos);
+        }
+    }
+}
+
+bool AddrManImpl::Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty)
+{
+    int added{0};
+    for (std::vector<CAddress>::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) {
+        added += AddSingle(*it, source, nTimePenalty) ? 1 : 0;
+    }
+    if (added > 0) {
+        LogPrint(BCLog::ADDRMAN, "Added %i addresses (of %i) from %s: %i tried, %i new\n", added, vAddr.size(), source.ToString(), nTried, nNew);
+    }
+    return added > 0;
 }
 
 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++)
-        nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0;
+    auto ret = Add_(vAddr, source, nTimePenalty);
     Check();
-    if (nAdd) {
-        LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew);
-    }
-    return nAdd > 0;
+    return ret;
 }
 
 void AddrManImpl::Good(const CService& addr, int64_t nTime)

src/addrman.h

@@ -70,7 +70,15 @@ class AddrMan
     //! 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 @@ class AddrManImpl
     //! 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 @@ def relay_tests(self):
         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 @@ def run_test(self):
         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)