[addrman] Add Add_() inner function, fix Add() return semantics

Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.

Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.

p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).

6 files changed, 21 insertions, 17 deletions

diff --git a/src/addrman.cpp b/src/addrman.cpp
index b0555898e2..0222f625be 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -599,7 +599,6 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
     if (!addr.IsRoutable())
         return false;
 
-    bool fNew = false;
     int nId;
     AddrInfo* pinfo = Find(addr, &nId);
 
@@ -640,13 +639,12 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
         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)) {
@@ -666,7 +664,19 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
             }
         }
     }
-    return fNew;
+    return fInsert;
+}
+
+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)
@@ -1031,15 +1041,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 += AddSingle(*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)
diff --git a/src/addrman.h b/src/addrman.h
index 1fbc827183..a9f697f66f 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -76,7 +76,8 @@ public:
      * @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). */
+     *                            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".
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
index 2d01881f2e..c8eb73027e 100644
--- a/src/addrman_impl.h
+++ b/src/addrman_impl.h
@@ -248,6 +248,8 @@ private:
      *  @see AddrMan::Add() for parameters. */
     bool AddSingle(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);
 
     std::pair<CAddress, int64_t> Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs);
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 991bfa5efc..eabc11c467 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -347,7 +347,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));
diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py
index 15b90fa61f..9df74ad3a0 100755
--- a/test/functional/p2p_addr_relay.py
+++ b/test/functional/p2p_addr_relay.py
@@ -152,7 +152,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',
             ]
         ):
diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py
index 3833c58680..f4be893d2c 100755
--- a/test/functional/p2p_addrv2_relay.py
+++ b/test/functional/p2p_addrv2_relay.py
@@ -72,9 +72,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=0',
                 'sending addrv2 (159 bytes) peer=1',
         ]):