Merge bitcoin/bitcoin#23713: refactor, test: refactor addrman_tried_collisions test to directly check for collisions

caac999 refactor: remove dependence on AddrManTest (josibake)
f961c47 refactor: check Good() in tried_collisions test (josibake)
207f1c8 refactor: make AddrMan::Good return bool (josibake)

Pull request description:

  Previously, the `addrman_tried_collisions` test behaved in the following way:

  1. add an address to addrman
  2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
  3. verify that `num_addrs` matched `size()` to check for collisions in the new table

  `AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.

  While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.

  ### solution

  To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.

  Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945).

  ### followup
  As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:

  * https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/rpc/net.cpp#L945
  * https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net.cpp#L2067
  * https://github.com/bitcoin/bitcoin/blob/a06364741358feae04813050e4225eb43fc386e3/src/net_processing.cpp#L2708

ACKs for top commit:
  jnewbery:
    utACK caac999
  mzumsande:
    Code review ACK caac999

Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99

Author: MarcoFalke

src/addrman.cpp

@@ -614,7 +614,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
     return fInsert;
 }
 
-void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
+bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
 {
     AssertLockHeld(cs);
 
@@ -625,8 +625,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     AddrInfo* pinfo = Find(addr, &nId);
 
     // if not found, bail out
-    if (!pinfo)
-        return;
+    if (!pinfo) return false;
 
     AddrInfo& info = *pinfo;
 
@@ -638,13 +637,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     // currently-connected peers.
 
     // if it is already in the tried set, don't do anything else
-    if (info.fInTried)
-        return;
+    if (info.fInTried) return false;
 
     // if it is not in new, something bad happened
-    if (!Assume(info.nRefCount > 0)) {
-        return;
-    }
+    if (!Assume(info.nRefCount > 0)) return false;
+
 
     // which tried bucket to move the entry to
     int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
@@ -661,11 +658,13 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
                  colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
                  addr.ToString(),
                  m_tried_collisions.size());
+        return false;
     } else {
         // move nId to the tried tables
         MakeTried(info, nId);
         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);
+        return true;
     }
 }
 
@@ -1049,12 +1048,13 @@ bool AddrManImpl::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source
     return ret;
 }
 
-void AddrManImpl::Good(const CService& addr, int64_t nTime)
+bool AddrManImpl::Good(const CService& addr, int64_t nTime)
 {
     LOCK(cs);
     Check();
-    Good_(addr, /* test_before_evict */ true, nTime);
+    auto ret = Good_(addr, /*test_before_evict=*/true, nTime);
     Check();
+    return ret;
 }
 
 void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -1157,9 +1157,9 @@ bool AddrMan::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, in
     return m_impl->Add(vAddr, source, nTimePenalty);
 }
 
-void AddrMan::Good(const CService& addr, int64_t nTime)
+bool AddrMan::Good(const CService& addr, int64_t nTime)
 {
-    m_impl->Good(addr, nTime);
+    return m_impl->Good(addr, nTime);
 }
 
 void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)

src/addrman.h

@@ -81,8 +81,14 @@ class AddrMan
      * @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".
-    void Good(const CService& addr, int64_t nTime = GetAdjustedTime());
+    /**
+     * Mark an address record as accessible and attempt to move it to addrman's tried table.
+     *
+     * @param[in] addr            Address record to attempt to move to tried table.
+     * @param[in] nTime           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, int64_t nTime = GetAdjustedTime());
 
     //! Mark an entry as connection attempted to.
     void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime());

src/addrman_impl.h

@@ -115,7 +115,7 @@ class AddrManImpl
     bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    void Good(const CService& addr, int64_t nTime)
+    bool Good(const CService& addr, int64_t nTime)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     void Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -248,7 +248,7 @@ class AddrManImpl
      *  @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 Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 

src/test/addrman_tests.cpp

@@ -268,32 +268,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
 
 BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
 {
-    AddrManTest addrman;
+    auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /*deterministic=*/true, /*consistency_check_ratio=*/100);
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
     uint32_t num_addrs{0};
 
-    BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
+    BOOST_CHECK_EQUAL(addrman->size(), num_addrs);
 
-    while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1
+    while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1
         CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
-        BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source));
-        addrman.Good(CAddress(addr, NODE_NONE));
+        BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source));
+
+        // Test: Add to tried without collision
+        BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE)));
 
-        // Test: No collision in tried table yet.
-        BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
     }
 
-    // Test: tried table collision!
+    // Test: Unable to add to tried table due to 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_EQUAL(addrman.size(), num_addrs - collisions);
+    BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
+    BOOST_CHECK(!addrman->Good(CAddress(addr1, NODE_NONE)));
 
+    // Test: Add the next address to tried without collision
     CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
-    BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source));
-    BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
+    BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source));
+    BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE)));
 }
 
 BOOST_AUTO_TEST_CASE(addrman_find)