Merge bitcoin/bitcoin#23826: test: Make AddrMan unit tests use public interface, extend coverage

ea4c9fd test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb46 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb test: Remove unused AddrManTest class (Martin Zumsande)
b696d78 test: Remove tests for internal helper functions (Martin Zumsande)
0538520 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d42 test: Inline SimConnFail function (Martin Zumsande)
5b7aac3 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74 test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f76 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)

Pull request description:

  This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
  This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.

  This includes the following steps:
  * Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried).  Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
  * Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
  * Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
  * Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.

  All in all, this PR increases the unit test coverage of AddrMan by a bit.

ACKs for top commit:
  jnewbery:
    ACK ea4c9fd
  josibake:
    reACK bitcoin/bitcoin@ea4c9fd

Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447

Author: fanquake

src/addrman.cpp

@@ -930,6 +930,29 @@ std::pair<CAddress, int64_t> AddrManImpl::SelectTriedCollision_()
     return {info_old, info_old.nLastTry};
 }
 
+std::optional<AddressPosition> AddrManImpl::FindAddressEntry_(const CAddress& addr)
+{
+    AssertLockHeld(cs);
+
+    AddrInfo* addr_info = Find(addr);
+
+    if (!addr_info) return std::nullopt;
+
+    if(addr_info->fInTried) {
+        int bucket{addr_info->GetTriedBucket(nKey, m_asmap)};
+        return AddressPosition(/*tried=*/true,
+                               /*multiplicity=*/1,
+                               /*bucket=*/bucket,
+                               /*position=*/addr_info->GetBucketPosition(nKey, false, bucket));
+    } else {
+        int bucket{addr_info->GetNewBucket(nKey, m_asmap)};
+        return AddressPosition(/*tried=*/false,
+                               /*multiplicity=*/addr_info->nRefCount,
+                               /*bucket=*/bucket,
+                               /*position=*/addr_info->GetBucketPosition(nKey, true, bucket));
+    }
+}
+
 void AddrManImpl::Check() const
 {
     AssertLockHeld(cs);
@@ -1116,6 +1139,15 @@ void AddrManImpl::SetServices(const CService& addr, ServiceFlags nServices)
     Check();
 }
 
+std::optional<AddressPosition> AddrManImpl::FindAddressEntry(const CAddress& addr)
+{
+    LOCK(cs);
+    Check();
+    auto entry = FindAddressEntry_(addr);
+    Check();
+    return entry;
+}
+
 const std::vector<bool>& AddrManImpl::GetAsmap() const
 {
     return m_asmap;
@@ -1201,3 +1233,8 @@ const std::vector<bool>& AddrMan::GetAsmap() const
 {
     return m_impl->GetAsmap();
 }
+
+std::optional<AddressPosition> AddrMan::FindAddressEntry(const CAddress& addr)
+{
+    return m_impl->FindAddressEntry(addr);
+}

src/addrman.h

@@ -22,6 +22,31 @@ class AddrManImpl;
 /** Default for -checkaddrman */
 static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
 
+/** Test-only struct, capturing info about an address in AddrMan */
+struct AddressPosition {
+    // Whether the address is in the new or tried table
+    const bool tried;
+
+    // Addresses in the tried table should always have a multiplicity of 1.
+    // Addresses in the new table can have multiplicity between 1 and
+    // ADDRMAN_NEW_BUCKETS_PER_ADDRESS
+    const int multiplicity;
+
+    // If the address is in the new table, the bucket and position are
+    // populated based on the first source who sent the address.
+    // In certain edge cases, this may not be where the address is currently
+    // located.
+    const int bucket;
+    const int position;
+
+    bool operator==(AddressPosition other) {
+        return std::tie(tried, multiplicity, bucket, position) ==
+               std::tie(other.tried, other.multiplicity, other.bucket, other.position);
+    }
+    explicit AddressPosition(bool tried_in, int multiplicity_in, int bucket_in, int position_in)
+        : tried{tried_in}, multiplicity{multiplicity_in}, bucket{bucket_in}, position{position_in} {}
+};
+
 /** Stochastic address manager
  *
  * Design goals:
@@ -142,6 +167,15 @@ class AddrMan
     void SetServices(const CService& addr, ServiceFlags nServices);
 
     const std::vector<bool>& GetAsmap() const;
+
+    /** Test-only function
+     * Find the address record in AddrMan and return information about its
+     * position.
+     * @param[in] addr       The address record to look up.
+     * @return               Information about the address record in AddrMan
+     *                       or nullopt if address is not found.
+     */
+    std::optional<AddressPosition> FindAddressEntry(const CAddress& addr);
 };
 
 #endif // BITCOIN_ADDRMAN_H

src/addrman_impl.h

@@ -137,9 +137,11 @@ class AddrManImpl
     void SetServices(const CService& addr, ServiceFlags nServices)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
+    std::optional<AddressPosition> FindAddressEntry(const CAddress& addr)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
+
     const std::vector<bool>& GetAsmap() const;
 
-    friend class AddrManTest;
     friend class AddrManDeterministic;
 
 private:
@@ -266,6 +268,8 @@ class AddrManImpl
 
     std::pair<CAddress, int64_t> SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
+    std::optional<AddressPosition> FindAddressEntry_(const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(cs);
+
     //! Consistency check, taking into account m_consistency_check_ratio.
     //! Will std::abort if an inconsistency is detected.
     void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs);