Merge #6243: backport: merge bitcoin#22226, #22831, #23084, #22872, #23477, #23492, #23713, #23780, #23826, #23373, #24201, #24665, #22910, partial bitcoin#23025 (addrman backports: part 3)

f032119456 merge bitcoin#22910: Encapsulate asmap in NetGroupManager (Kittywhiskers Van Gogh)
8020bfa8c1 merge bitcoin#24665: document clang tidy named args (Kittywhiskers Van Gogh)
40a22e457a merge bitcoin#24201: Avoid InitError when downgrading peers.dat (Kittywhiskers Van Gogh)
cdcaf2278c merge bitcoin#23373: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change (Kittywhiskers Van Gogh)
b30f0fa441 test: remove `connman` local from `BasicTestingSetup` (Kittywhiskers Van Gogh)
df43565464 merge bitcoin#23826: Make AddrMan unit tests use public interface, extend coverage (Kittywhiskers Van Gogh)
c14a54089f merge bitcoin#23780: update `addrman_tests.cpp` to use output from `AddrMan::Good()` (Kittywhiskers Van Gogh)
8b2db6bce4 merge bitcoin#23713: refactor addrman_tried_collisions test to directly check for collisions (Kittywhiskers Van Gogh)
5b5dd39f45 merge bitcoin#23492: tidy up addrman unit tests (Kittywhiskers Van Gogh)
aba0ebd400 merge bitcoin#23477: tidy up unit tests (Kittywhiskers Van Gogh)
cdc8321c4d merge bitcoin#22872: improve checkaddrman logging with duration in milliseconds (Kittywhiskers Van Gogh)
8d22fe9945 merge bitcoin#23084: avoid non-determinism in asmap-addrman test (Kittywhiskers Van Gogh)
ba4696718e partial bitcoin#23025: update nanobench add `-min_time` (Kittywhiskers Van Gogh)
c28b05c5ca merge bitcoin#22831: add addpeeraddress "tried", test addrman checks on restart with asmap (Kittywhiskers Van Gogh)
c4fe6085c8 merge bitcoin#22226: add unittest core dump instructions (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * In [bitcoin#22831](https://github.com/bitcoin/bitcoin/pull/22831), when restarting the node in `rpc_net.py`'s `test_addpeeraddress()`, existing commands need to be appended to `extra_args` to ensure they're retained when the node is restarted (default behavior is to overwrite the argument list with `extra_args`) to prevent the test from hanging and then failing due to missing fast DIP3 activation params from the arguments list.

    Missing arguments result in a block database sanity check failure on restart due to `bad-qc-premature` arising from the activation height being higher than the height of a block with a quorum commitment.

  * `NodeContext` was moved from `TestingSetup` to `BasicTestingSetup` in [bitcoin#18571](https://github.com/bitcoin/bitcoin/pull/18571) ([dash#4844](https://github.com/dashpay/dash/pull/4844)) but `connman` as a `BasicTestingSetup` variable still stuck around (despite `NodeContext`'s presence making this unnecessary).

    To prepare for [bitcoin#22910](https://github.com/bitcoin/bitcoin/pull/22910), the remnant variable has been replaced with `m_node.connman` and adjustments have been made to that effect.

  ## Breaking Changes

  None observed.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK f032119456
  PastaPastaPasta:
    utACK f032119456

Tree-SHA512: b29c292ecda54cda8301ea804b433f80476a1cdbb72bd48740cc9b2e885a4ff52350e5e42f112426856282bd6d961f0e37f1b23020c52f07238413070bbc504a

doc/developer-notes.md

@@ -160,11 +160,57 @@ public:
 } // namespace foo
 ```
 
+Coding Style (C++ named arguments)
+------------------------------
+
+When passing named arguments, use a format that clang-tidy understands. The
+argument names can otherwise not be verified by clang-tidy.
+
+For example:
+
+```c++
+void function(Addrman& addrman, bool clear);
+
+int main()
+{
+    function(g_addrman, /*clear=*/false);
+}
+```
+
+### Running clang-tidy
+
+To run clang-tidy on Ubuntu/Debian, install the dependencies:
+
+```sh
+apt install clang-tidy bear clang
+```
+
+Then, pass clang as compiler to configure, and use bear to produce the `compile_commands.json`:
+
+```sh
+./autogen.sh && ./configure CC=clang CXX=clang++
+make clean && bear make -j $(nproc)     # For bear 2.x
+make clean && bear -- make -j $(nproc)  # For bear 3.x
+```
+
+To run clang-tidy on all source files:
+
+```sh
+( cd ./src/ && run-clang-tidy  -j $(nproc) )
+```
+
+To run clang-tidy on the changed source lines:
+
+```sh
+git diff | ( cd ./src/ && clang-tidy-diff -p2 -j $(nproc) )
+```
+
 Coding Style (Python)
 ---------------------
 
 Refer to [/test/functional/README.md#style-guidelines](/test/functional/README.md#style-guidelines).
 
+
 Coding Style (Doxygen-compatible comments)
 ------------------------------------------
 

doc/fuzzing.md

@@ -64,6 +64,15 @@ block^@M-^?M-^?M-^?M-^?M-^?nM-^?M-^?
 
 In this case the fuzzer managed to create a `block` message which when passed to `ProcessMessage(...)` increased coverage.
 
+It is possible to specify `dashd` arguments to the `fuzz` executable.
+Depending on the test, they may be ignored or consumed and alter the behavior
+of the test. Just make sure to use double-dash to distinguish them from the
+fuzzer's own arguments:
+
+```sh
+$ FUZZ=address_deserialize_v2 src/test/fuzz/fuzz -runs=1 fuzz_seed_corpus/address_deserialize_v2 --checkaddrman=5 --printtoconsole=1
+```
+
 ## Fuzzing corpora
 
 The project's collection of seed corpora is found in the [`bitcoin-core/qa-assets`](https://github.com/bitcoin-core/qa-assets) repo.

src/Makefile.am

@@ -266,6 +266,7 @@ BITCOIN_CORE_H = \
   netaddress.h \
   netbase.h \
   netfulfilledman.h \
+  netgroup.h \
   netmessagemaker.h \
   node/blockstorage.h \
   node/coin.h \
@@ -492,6 +493,7 @@ libbitcoin_server_a_SOURCES = \
   miner.cpp \
   net.cpp \
   netfulfilledman.cpp \
+  netgroup.cpp \
   net_processing.cpp \
   node/blockstorage.cpp \
   node/coin.cpp \

src/addrdb.cpp

@@ -12,6 +12,7 @@
 #include <hash.h>
 #include <logging/timer.h>
 #include <netbase.h>
+#include <netgroup.h>
 #include <random.h>
 #include <streams.h>
 #include <tinyformat.h>
@@ -182,10 +183,10 @@ void ReadFromStream(AddrMan& addr, CDataStream& ssPeers)
     DeserializeDB(ssPeers, addr, false);
 }
 
-std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman)
+std::optional<bilingual_str> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman)
 {
     auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
-    addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+    addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
 
     int64_t nStart = GetTimeMillis();
     const auto path_addr{gArgs.GetDataDirNet() / "peers.dat"};
@@ -194,7 +195,7 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
         LogPrintf("Loaded %i addresses from peers.dat  %dms\n", addrman->size(), GetTimeMillis() - nStart);
     } catch (const DbNotFoundError&) {
         // Addrman can be in an inconsistent state after failure, reset it
-        addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
         LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
         DumpPeerAddresses(args, *addrman);
     } catch (const DbInconsistentError& e) {
@@ -203,9 +204,18 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
         // with frequent corruption, we are restoring old behaviour that does the same, silently.
         //
         // TODO: Evaluate cause and fix, revert this change at some point.
-        addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
+        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
         LogPrintf("Creating peers.dat because of invalid or corrupt file (%s)\n", e.what());
         DumpPeerAddresses(args, *addrman);
+    } catch (const InvalidAddrManVersionError&) {
+        if (!RenameOver(path_addr, (fs::path)path_addr + ".bak")) {
+            addrman = nullptr;
+            return strprintf(_("Failed to rename invalid peers.dat file. Please move or delete it and try again."));
+        }
+        // Addrman can be in an inconsistent state after failure, reset it
+        addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
+        LogPrintf("Creating new peers.dat because the file version was not compatible (%s). Original backed up to peers.dat.bak\n", fs::quoted(fs::PathToString(path_addr)));
+        DumpPeerAddresses(args, *addrman);
     } catch (const std::exception& e) {
         addrman = nullptr;
         return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),

src/addrdb.h

@@ -18,6 +18,7 @@ class ArgsManager;
 class AddrMan;
 class CAddress;
 class CDataStream;
+class NetGroupManager;
 struct bilingual_str;
 
 bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
@@ -49,7 +50,7 @@ public:
 };
 
 /** Returns an error string on failure */
-std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
+std::optional<bilingual_str> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
 
 /**
  * Dump the anchor IP address database (anchors.dat)

src/addrman.cpp

@@ -7,6 +7,8 @@
 #include <addrman_impl.h>
 
 #include <hash.h>
+#include <logging.h>
+#include <logging/timer.h>
 #include <netaddress.h>
 #include <protocol.h>
 #include <random.h>
@@ -41,17 +43,17 @@ static constexpr size_t ADDRMAN_SET_TRIED_COLLISION_SIZE{10};
 /** The maximum time we'll spend trying to resolve a tried table collision, in seconds */
 static constexpr int64_t ADDRMAN_TEST_WINDOW{40*60}; // 40 minutes
 
-int AddrInfo::GetTriedBucket(const uint256& nKey, const std::vector<bool>& asmap) const
+int AddrInfo::GetTriedBucket(const uint256& nKey, const NetGroupManager& netgroupman) const
 {
     uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetKey()).GetCheapHash();
-    uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetCheapHash();
+    uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << netgroupman.GetGroup(*this) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetCheapHash();
     return hash2 % ADDRMAN_TRIED_BUCKET_COUNT;
 }
 
-int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std::vector<bool>& asmap) const
+int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const NetGroupManager& netgroupman) const
 {
-    std::vector<unsigned char> vchSourceGroupKey = src.GetGroup(asmap);
+    std::vector<unsigned char> vchSourceGroupKey = netgroupman.GetGroup(src);
-    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetCheapHash();
+    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << netgroupman.GetGroup(*this) << vchSourceGroupKey).GetCheapHash();
     uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << vchSourceGroupKey << (hash1 % ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP)).GetCheapHash();
     return hash2 % ADDRMAN_NEW_BUCKET_COUNT;
 }
@@ -99,11 +101,11 @@ double AddrInfo::GetChance(int64_t nNow) const
     return fChance;
 }
 
-AddrManImpl::AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio)
+AddrManImpl::AddrManImpl(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio)
     : insecure_rand{deterministic}
     , nKey{deterministic ? uint256{1} : insecure_rand.rand256()}
     , m_consistency_check_ratio{consistency_check_ratio}
-    , m_asmap{std::move(asmap)}
+    , m_netgroupman{netgroupman}
 {
     for (auto& bucket : vvNew) {
         for (auto& entry : bucket) {
@@ -218,11 +220,7 @@ void AddrManImpl::Serialize(Stream& s_) const
     }
     // Store asmap checksum after bucket entries so that it
     // can be ignored by older clients for backward compatibility.
-    uint256 asmap_checksum;
+    s << m_netgroupman.GetAsmapChecksum();
-    if (m_asmap.size() != 0) {
-        asmap_checksum = SerializeHash(m_asmap);
-    }
-    s << asmap_checksum;
 }
 
 template <typename Stream>
@@ -248,7 +246,7 @@ void AddrManImpl::Unserialize(Stream& s_)
     s >> compat;
     const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
     if (lowest_compatible > FILE_FORMAT) {
-        throw std::ios_base::failure(strprintf(
+        throw InvalidAddrManVersionError(strprintf(
             "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
             "but the maximum supported by this version of %s is %u.",
             uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
@@ -292,7 +290,7 @@ void AddrManImpl::Unserialize(Stream& s_)
     for (int n = 0; n < nTried; n++) {
         AddrInfo info;
         s >> info;
-        int nKBucket = info.GetTriedBucket(nKey, m_asmap);
+        int nKBucket = info.GetTriedBucket(nKey, m_netgroupman);
         int nKBucketPos = info.GetBucketPosition(nKey, false, nKBucket);
         if (info.IsValid()
                 && vvTried[nKBucket][nKBucketPos] == -1) {
@@ -329,10 +327,7 @@ void AddrManImpl::Unserialize(Stream& s_)
     // If the bucket count and asmap checksum haven't changed, then attempt
     // to restore the entries to the buckets/positions they were in before
     // serialization.
-    uint256 supplied_asmap_checksum;
+    uint256 supplied_asmap_checksum{m_netgroupman.GetAsmapChecksum()};
-    if (m_asmap.size() != 0) {
-        supplied_asmap_checksum = SerializeHash(m_asmap);
-    }
     uint256 serialized_asmap_checksum;
     if (format >= Format::V2_ASMAP) {
         s >> serialized_asmap_checksum;
@@ -365,7 +360,7 @@ void AddrManImpl::Unserialize(Stream& s_)
         } else {
             // In case the new table data cannot be used (bucket count wrong or new asmap),
             // try to give them a reference based on their primary source address.
-            bucket = info.GetNewBucket(nKey, m_asmap);
+            bucket = info.GetNewBucket(nKey, m_netgroupman);
             bucket_position = info.GetBucketPosition(nKey, true, bucket);
             if (vvNew[bucket][bucket_position] == -1) {
                 vvNew[bucket][bucket_position] = entry_index;
@@ -389,7 +384,7 @@ void AddrManImpl::Unserialize(Stream& s_)
         LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost);
     }
 
-    const int check_code{ForceCheckAddrman()};
+    const int check_code{CheckAddrman()};
     if (check_code != 0) {
         throw DbInconsistentError(strprintf(
             "Corrupt data. Consistency check failed with code %s",
@@ -487,7 +482,7 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
     AssertLockHeld(cs);
 
     // remove the entry from all new buckets
-    const int start_bucket{info.GetNewBucket(nKey, m_asmap)};
+    const int start_bucket{info.GetNewBucket(nKey, m_netgroupman)};
     for (int n = 0; n < ADDRMAN_NEW_BUCKET_COUNT; ++n) {
         const int bucket{(start_bucket + n) % ADDRMAN_NEW_BUCKET_COUNT};
         const int pos{info.GetBucketPosition(nKey, true, bucket)};
@@ -502,7 +497,7 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
     assert(info.nRefCount == 0);
 
     // which tried bucket to move the entry to
-    int nKBucket = info.GetTriedBucket(nKey, m_asmap);
+    int nKBucket = info.GetTriedBucket(nKey, m_netgroupman);
     int nKBucketPos = info.GetBucketPosition(nKey, false, nKBucket);
 
     // first make space to add it (the existing tried entry there is moved to new, deleting whatever is there).
@@ -518,7 +513,7 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
         nTried--;
 
         // find which new bucket it belongs to
-        int nUBucket = infoOld.GetNewBucket(nKey, m_asmap);
+        int nUBucket = infoOld.GetNewBucket(nKey, m_netgroupman);
         int nUBucketPos = infoOld.GetBucketPosition(nKey, true, nUBucket);
         ClearNew(nUBucket, nUBucketPos);
         assert(vvNew[nUBucket][nUBucketPos] == -1);
@@ -588,7 +583,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
         nNew++;
     }
 
-    int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap);
+    int nUBucket = pinfo->GetNewBucket(nKey, source, m_netgroupman);
     int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket);
     bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
     if (vvNew[nUBucket][nUBucketPos] != nId) {
@@ -604,7 +599,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
             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);
+                     addr.ToString(), m_netgroupman.GetMappedAS(addr), nUBucket, nUBucketPos);
         } else {
             if (pinfo->nRefCount == 0) {
                 Delete(nId);
@@ -614,7 +609,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 +620,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)
+    if (!pinfo) return false;
-        return;
 
     AddrInfo& info = *pinfo;
 
@@ -638,16 +632,14 @@ 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)
+    if (info.fInTried) return false;
-        return;
 
     // if it is not in new, something bad happened
-    if (!Assume(info.nRefCount > 0)) {
+    if (!Assume(info.nRefCount > 0)) return false;
-        return;
+
-    }
 
     // which tried bucket to move the entry to
-    int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
+    int tried_bucket = info.GetTriedBucket(nKey, m_netgroupman);
     int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);
 
     // Will moving this address into tried evict another entry?
@@ -663,13 +655,15 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
                      addr.ToString(),
                      m_tried_collisions.size());
         }
+        return false;
     } 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);
+                     addr.ToString(), m_netgroupman.GetMappedAS(addr), tried_bucket, tried_bucket_pos);
         }
+        return true;
     }
 }
 
@@ -873,7 +867,7 @@ void AddrManImpl::ResolveCollisions_()
             AddrInfo& info_new = mapInfo[id_new];
 
             // Which tried bucket to move the entry to.
-            int tried_bucket = info_new.GetTriedBucket(nKey, m_asmap);
+            int tried_bucket = info_new.GetTriedBucket(nKey, m_netgroupman);
             int tried_bucket_pos = info_new.GetBucketPosition(nKey, false, tried_bucket);
             if (!info_new.IsValid()) { // id_new may no longer map to a valid address
                 erase_collision = true;
@@ -941,13 +935,36 @@ std::pair<CAddress, int64_t> AddrManImpl::SelectTriedCollision_()
     const AddrInfo& newInfo = mapInfo[id_new];
 
     // which tried bucket to move the entry to
-    int tried_bucket = newInfo.GetTriedBucket(nKey, m_asmap);
+    int tried_bucket = newInfo.GetTriedBucket(nKey, m_netgroupman);
     int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket);
 
     const AddrInfo& info_old = mapInfo[vvTried[tried_bucket][tried_bucket_pos]];
     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_netgroupman)};
+        return AddressPosition(/*tried_in=*/true,
+                               /*multiplicity_in=*/1,
+                               /*bucket_in=*/bucket,
+                               /*position_in=*/addr_info->GetBucketPosition(nKey, false, bucket));
+    } else {
+        int bucket{addr_info->GetNewBucket(nKey, m_netgroupman)};
+        return AddressPosition(/*tried_in=*/false,
+                               /*multiplicity_in=*/addr_info->nRefCount,
+                               /*bucket_in=*/bucket,
+                               /*position_in=*/addr_info->GetBucketPosition(nKey, true, bucket));
+    }
+}
+
 void AddrManImpl::Check() const
 {
     AssertLockHeld(cs);
@@ -956,18 +973,19 @@ void AddrManImpl::Check() const
     if (m_consistency_check_ratio == 0) return;
     if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return;
 
-    const int err{ForceCheckAddrman()};
+    const int err{CheckAddrman()};
     if (err) {
         LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
         assert(false);
     }
 }
 
-int AddrManImpl::ForceCheckAddrman() const
+int AddrManImpl::CheckAddrman() const
 {
     AssertLockHeld(cs);
 
-    LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size());
+    LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
+        strprintf("new %i, tried %i, total %u", nNew, nTried, vRandom.size()), BCLog::ADDRMAN);
 
     std::unordered_set<int> setTried;
     std::unordered_map<int, int> mapNew;
@@ -1014,7 +1032,7 @@ int AddrManImpl::ForceCheckAddrman() const
                 if (!setTried.count(vvTried[n][i]))
                     return -11;
                 const auto it{mapInfo.find(vvTried[n][i])};
-                if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_asmap) != n) {
+                if (it == mapInfo.end() || it->second.GetTriedBucket(nKey, m_netgroupman) != n) {
                     return -17;
                 }
                 if (it->second.GetBucketPosition(nKey, false, n) != i) {
@@ -1047,7 +1065,6 @@ int AddrManImpl::ForceCheckAddrman() const
     if (nKey.IsNull())
         return -16;
 
-    LogPrint(BCLog::ADDRMAN, "Addrman checks completed successfully\n");
     return 0;
 }
 
@@ -1066,12 +1083,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)
@@ -1133,6 +1151,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;
+}
+
 AddrInfo AddrManImpl::GetAddressInfo(const CService& addr)
 {
     AddrInfo addrRet;
@@ -1145,13 +1172,8 @@ AddrInfo AddrManImpl::GetAddressInfo(const CService& addr)
     return addrRet;
 }
 
-const std::vector<bool>& AddrManImpl::GetAsmap() const
+AddrMan::AddrMan(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio)
-{
+    : m_impl(std::make_unique<AddrManImpl>(netgroupman, deterministic, consistency_check_ratio)) {}
-    return m_asmap;
-}
-
-AddrMan::AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
-    : m_impl(std::make_unique<AddrManImpl>(std::move(asmap), deterministic, consistency_check_ratio)) {}
 
 AddrMan::~AddrMan() = default;
 
@@ -1185,9 +1207,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)
@@ -1230,7 +1252,7 @@ AddrInfo AddrMan::GetAddressInfo(const CService& addr)
     return m_impl->GetAddressInfo(addr);
 }
 
-const std::vector<bool>& AddrMan::GetAsmap() const
+std::optional<AddressPosition> AddrMan::FindAddressEntry(const CAddress& addr)
 {
-    return m_impl->GetAsmap();
+    return m_impl->FindAddressEntry(addr);
 }

src/addrman.h

@@ -7,6 +7,7 @@
 #define BITCOIN_ADDRMAN_H
 
 #include <netaddress.h>
+#include <netgroup.h>
 #include <protocol.h>
 #include <streams.h>
 #include <timedata.h>
@@ -17,12 +18,6 @@
 #include <utility>
 #include <vector>
 
-class AddrInfo;
-class AddrManImpl;
-
-/** Default for -checkaddrman */
-static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0};
-
 class DbInconsistentError : public std::exception
 {
     using std::exception::exception;
@@ -33,6 +28,43 @@ public:
     const char* what() const noexcept override { return error.c_str(); }
 };
 
+class InvalidAddrManVersionError : public std::ios_base::failure
+{
+public:
+    InvalidAddrManVersionError(std::string msg) : std::ios_base::failure(msg) { }
+};
+
+class AddrInfo;
+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:
@@ -64,10 +96,11 @@ public:
  */
 class AddrMan
 {
+protected:
     const std::unique_ptr<AddrManImpl> m_impl;
 
 public:
-    explicit AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio);
+    explicit AddrMan(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio);
 
     ~AddrMan();
 
@@ -91,8 +124,14 @@ public:
      * @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());
@@ -148,10 +187,14 @@ public:
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
     AddrInfo GetAddressInfo(const CService& addr);
 
-    const std::vector<bool>& GetAsmap() const;
+    /** Test-only function
-
+     * Find the address record in AddrMan and return information about its
-    friend class AddrManTest;
+     * position.
-    friend class AddrManDeterministic;
+     * @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

@@ -6,6 +6,7 @@
 #define BITCOIN_ADDRMAN_IMPL_H
 
 #include <logging.h>
+#include <logging/timer.h>
 #include <netaddress.h>
 #include <protocol.h>
 #include <serialize.h>
@@ -75,15 +76,15 @@ public:
     }
 
     //! Calculate in which "tried" bucket this entry belongs
-    int GetTriedBucket(const uint256 &nKey, const std::vector<bool> &asmap) const;
+    int GetTriedBucket(const uint256& nKey, const NetGroupManager& netgroupman) const;
 
     //! Calculate in which "new" bucket this entry belongs, given a certain source
-    int GetNewBucket(const uint256 &nKey, const CNetAddr& src, const std::vector<bool> &asmap) const;
+    int GetNewBucket(const uint256& nKey, const CNetAddr& src, const NetGroupManager& netgroupman) const;
 
     //! Calculate in which "new" bucket this entry belongs, using its default source
-    int GetNewBucket(const uint256 &nKey, const std::vector<bool> &asmap) const
+    int GetNewBucket(const uint256& nKey, const NetGroupManager& netgroupman) const
     {
-        return GetNewBucket(nKey, source, asmap);
+        return GetNewBucket(nKey, source, netgroupman);
     }
 
     //! Calculate in which position of a bucket to store this entry.
@@ -99,7 +100,7 @@ public:
 class AddrManImpl
 {
 public:
-    AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio);
+    AddrManImpl(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio);
 
     ~AddrManImpl();
 
@@ -114,7 +115,7 @@ public:
     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)
@@ -136,11 +137,12 @@ public:
     void SetServices(const CService& addr, ServiceFlags nServices)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    AddrInfo GetAddressInfo(const CService& addr);
+    std::optional<AddressPosition> FindAddressEntry(const CAddress& addr)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    const std::vector<bool>& GetAsmap() const;
+    AddrInfo GetAddressInfo(const CService& addr)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    friend class AddrManTest;
     friend class AddrManDeterministic;
 
 private:
@@ -211,21 +213,8 @@ private:
     /** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */
     const int32_t m_consistency_check_ratio;
 
-    // Compressed IP->ASN mapping, loaded from a file when a node starts.
+    /** Reference to the netgroup manager. netgroupman must be constructed before addrman and destructed after. */
-    // Should be always empty if no file was provided.
+    const NetGroupManager& m_netgroupman;
-    // This mapping is then used for bucketing nodes in Addrman.
-    //
-    // If asmap is provided, nodes will be bucketed by
-    // AS they belong to, in order to make impossible for a node
-    // to connect to several nodes hosted in a single AS.
-    // This is done in response to Erebus attack, but also to generally
-    // diversify the connections every node creates,
-    // especially useful when a large fraction of nodes
-    // operate under a couple of cloud providers.
-    //
-    // If a new asmap was provided, the existing records
-    // would be re-bucketed accordingly.
-    const std::vector<bool> m_asmap;
 
     //! Find an entry.
     AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -249,7 +238,7 @@ private:
      *  @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);
 
@@ -269,12 +258,15 @@ private:
 
     std::pair<CAddress, int64_t> SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected.
+    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);
 
     //! Perform consistency check, regardless of m_consistency_check_ratio.
     //! @returns an error code or zero.
-    int ForceCheckAddrman() const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    int CheckAddrman() const EXCLUSIVE_LOCKS_REQUIRED(cs);
 };
 
 #endif // BITCOIN_ADDRMAN_IMPL_H

src/bench/addrman.cpp

@@ -4,6 +4,7 @@
 
 #include <addrman.h>
 #include <bench/bench.h>
+#include <netgroup.h>
 #include <random.h>
 #include <util/time.h>
 
@@ -14,6 +15,9 @@
 static constexpr size_t NUM_SOURCES = 64;
 static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
 
+static NetGroupManager EMPTY_NETGROUPMAN{std::vector<bool>()};
+static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0};
+
 static std::vector<CAddress> g_sources;
 static std::vector<std::vector<CAddress>> g_addresses;
 
@@ -72,14 +76,14 @@ static void AddrManAdd(benchmark::Bench& bench)
     CreateAddresses();
 
     bench.run([&] {
-        AddrMan addrman{/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0};
+        AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
         AddAddressesToAddrMan(addrman);
     });
 }
 
 static void AddrManSelect(benchmark::Bench& bench)
 {
-    AddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
 
     FillAddrMan(addrman);
 
@@ -91,7 +95,7 @@ static void AddrManSelect(benchmark::Bench& bench)
 
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
-    AddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
 
     FillAddrMan(addrman);
 
@@ -101,40 +105,33 @@ static void AddrManGetAddr(benchmark::Bench& bench)
     });
 }
 
-static void AddrManGood(benchmark::Bench& bench)
+static void AddrManAddThenGood(benchmark::Bench& bench)
 {
-    /* Create many AddrMan objects - one to be modified at each loop iteration.
-     * This is necessary because the AddrMan::Good() method modifies the
-     * object, affecting the timing of subsequent calls to the same method and
-     * we want to do the same amount of work in every loop iteration. */
-
-    bench.epochs(5).epochIterations(1);
-    const size_t addrman_count{bench.epochs() * bench.epochIterations()};
-
-    std::vector<std::unique_ptr<AddrMan>> addrmans(addrman_count);
-    for (size_t i{0}; i < addrman_count; ++i) {
-        addrmans[i] = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
-        FillAddrMan(*addrmans[i]);
-    }
-
     auto markSomeAsGood = [](AddrMan& addrman) {
         for (size_t source_i = 0; source_i < NUM_SOURCES; ++source_i) {
             for (size_t addr_i = 0; addr_i < NUM_ADDRESSES_PER_SOURCE; ++addr_i) {
-                if (addr_i % 32 == 0) {
+                addrman.Good(g_addresses[source_i][addr_i]);
-                    addrman.Good(g_addresses[source_i][addr_i]);
-                }
             }
         }
     };
 
-    uint64_t i = 0;
+    CreateAddresses();
+
     bench.run([&] {
-        markSomeAsGood(*addrmans.at(i));
+        // To make the benchmark independent of the number of evaluations, we always prepare a new addrman.
-        ++i;
+        // This is necessary because AddrMan::Good() method modifies the object, affecting the timing of subsequent calls
+        // to the same method and we want to do the same amount of work in every loop iteration.
+        //
+        // This has some overhead (exactly the result of AddrManAdd benchmark), but that overhead is constant so improvements in
+        // AddrMan::Good() will still be noticeable.
+        AddrMan addrman{EMPTY_NETGROUPMAN, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
+        AddAddressesToAddrMan(addrman);
+
+        markSomeAsGood(addrman);
     });
 }
 
 BENCHMARK(AddrManAdd);
 BENCHMARK(AddrManSelect);
 BENCHMARK(AddrManGetAddr);
-BENCHMARK(AddrManGood);
+BENCHMARK(AddrManAddThenGood);

src/bench/bench.cpp

@@ -16,6 +16,8 @@
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
+
 namespace {
 
 void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& file, const char* tpl)

src/bench/nanobench.h

@@ -33,7 +33,7 @@
 // see https://semver.org/
 #define ANKERL_NANOBENCH_VERSION_MAJOR 4 // incompatible API changes
 #define ANKERL_NANOBENCH_VERSION_MINOR 3 // backwards-compatible changes
-#define ANKERL_NANOBENCH_VERSION_PATCH 4 // backwards-compatible bug fixes
+#define ANKERL_NANOBENCH_VERSION_PATCH 6 // backwards-compatible bug fixes
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 // public facing api - as minimal as possible
@@ -88,13 +88,15 @@
         } while (0)
 #endif
 
-#if defined(__linux__) && defined(PERF_EVENT_IOC_ID) && defined(PERF_COUNT_HW_REF_CPU_CYCLES) && defined(PERF_FLAG_FD_CLOEXEC) && \
+#define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 0
-    !defined(ANKERL_NANOBENCH_DISABLE_PERF_COUNTERS)
+#if defined(__linux__) && !defined(ANKERL_NANOBENCH_DISABLE_PERF_COUNTERS)
-// only enable perf counters on kernel 3.14 which seems to have all the necessary defines. The three PERF_... defines are not in
+#    include <linux/version.h>
-// kernel 2.6.32 (all others are).
+#    if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 14, 0)
-#    define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 1
+// PERF_COUNT_HW_REF_CPU_CYCLES only available since kernel 3.3
-#else
+// PERF_FLAG_FD_CLOEXEC since kernel 3.14
-#    define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 0
+#        undef ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS
+#        define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 1
+#    endif
 #endif
 
 #if defined(__clang__)
@@ -2210,20 +2212,20 @@ struct IterationLogic::Impl {
             columns.emplace_back(10, 1, "err%", "%", rErrorMedian * 100.0);
 
             double rInsMedian = -1.0;
-            if (mResult.has(Result::Measure::instructions)) {
+            if (mBench.performanceCounters() && mResult.has(Result::Measure::instructions)) {
                 rInsMedian = mResult.median(Result::Measure::instructions);
                 columns.emplace_back(18, 2, "ins/" + mBench.unit(), "", rInsMedian / mBench.batch());
             }
 
             double rCycMedian = -1.0;
-            if (mResult.has(Result::Measure::cpucycles)) {
+            if (mBench.performanceCounters() && mResult.has(Result::Measure::cpucycles)) {
                 rCycMedian = mResult.median(Result::Measure::cpucycles);
                 columns.emplace_back(18, 2, "cyc/" + mBench.unit(), "", rCycMedian / mBench.batch());
             }
             if (rInsMedian > 0.0 && rCycMedian > 0.0) {
                 columns.emplace_back(9, 3, "IPC", "", rCycMedian <= 0.0 ? 0.0 : rInsMedian / rCycMedian);
             }
-            if (mResult.has(Result::Measure::branchinstructions)) {
+            if (mBench.performanceCounters() && mResult.has(Result::Measure::branchinstructions)) {
                 double rBraMedian = mResult.median(Result::Measure::branchinstructions);
                 columns.emplace_back(17, 2, "bra/" + mBench.unit(), "", rBraMedian / mBench.batch());
                 if (mResult.has(Result::Measure::branchmisses)) {
@@ -2402,6 +2404,14 @@ public:
         return (a + divisor / 2) / divisor;
     }
 
+    ANKERL_NANOBENCH_NO_SANITIZE("integer", "undefined")
+    static inline uint32_t mix(uint32_t x) noexcept {
+        x ^= x << 13;
+        x ^= x >> 17;
+        x ^= x << 5;
+        return x;
+    }
+
     template <typename Op>
     ANKERL_NANOBENCH_NO_SANITIZE("integer", "undefined")
     void calibrate(Op&& op) {
@@ -2441,15 +2451,10 @@ public:
             uint64_t const numIters = 100000U + (std::random_device{}() & 3);
             uint64_t n = numIters;
             uint32_t x = 1234567;
-            auto fn = [&]() {
-                x ^= x << 13;
-                x ^= x >> 17;
-                x ^= x << 5;
-            };
 
             beginMeasure();
             while (n-- > 0) {
-                fn();
+                x = mix(x);
             }
             endMeasure();
             detail::doNotOptimizeAway(x);
@@ -2459,8 +2464,8 @@ public:
             beginMeasure();
             while (n-- > 0) {
                 // we now run *twice* so we can easily calculate the overhead
-                fn();
+                x = mix(x);
-                fn();
+                x = mix(x);
             }
             endMeasure();
             detail::doNotOptimizeAway(x);

src/init.cpp

@@ -36,6 +36,7 @@
 #include <net_permissions.h>
 #include <net_processing.h>
 #include <netbase.h>
+#include <netgroup.h>
 #include <node/blockstorage.h>
 #include <node/context.h>
 #include <node/ui_interface.h>
@@ -287,6 +288,7 @@ void PrepareShutdown(NodeContext& node)
     node.connman.reset();
     node.banman.reset();
     node.addrman.reset();
+    node.netgroupman.reset();
 
     if (node.mempool && node.mempool->IsLoaded() && node.args->GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
         DumpMempool(*node.mempool);
@@ -1523,8 +1525,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
 
     {
-        // Initialize addrman
-        assert(!node.addrman);
 
         // Read asmap file if configured
         std::vector<bool> asmap;
@@ -1551,8 +1551,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             LogPrintf("Using /16 prefix for IP bucketing\n");
         }
 
+        // Initialize netgroup manager
+        assert(!node.netgroupman);
+        node.netgroupman = std::make_unique<NetGroupManager>(std::move(asmap));
+
+        // Initialize addrman
+        assert(!node.addrman);
         uiInterface.InitMessage(_("Loading P2P addresses…").translated);
-        if (const auto error{LoadAddrman(asmap, args, node.addrman)}) {
+        if (const auto error{LoadAddrman(*node.netgroupman, args, node.addrman)}) {
             return InitError(*error);
         }
     }
@@ -1560,7 +1566,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     assert(!node.banman);
     node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);
-    node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), *node.addrman, args.GetBoolArg("-networkactive", true));
+    node.connman = std::make_unique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()),
+                                              GetRand(std::numeric_limits<uint64_t>::max()),
+                                              *node.addrman, *node.netgroupman, args.GetBoolArg("-networkactive", true));
 
     assert(!node.fee_estimator);
     // Don't initialize fee estimation with old data if we don't relay transactions,

src/logging/timer.h

@@ -27,10 +27,12 @@ public:
     Timer(
         std::string prefix,
         std::string end_msg,
-        BCLog::LogFlags log_category = BCLog::LogFlags::ALL) :
+        BCLog::LogFlags log_category = BCLog::LogFlags::ALL,
+        bool msg_on_completion = true) :
             m_prefix(std::move(prefix)),
             m_title(std::move(end_msg)),
-            m_log_category(log_category)
+            m_log_category(log_category),
+            m_message_on_completion(msg_on_completion)
     {
         this->Log(strprintf("%s started", m_title));
         m_start_t = GetTime<std::chrono::microseconds>();
@@ -38,7 +40,11 @@ public:
 
     ~Timer()
     {
-        this->Log(strprintf("%s completed", m_title));
+        if (m_message_on_completion) {
+            this->Log(strprintf("%s completed", m_title));
+        } else {
+            this->Log("completed");
+        }
     }
 
     void Log(const std::string& msg)
@@ -74,14 +80,17 @@ private:
     std::chrono::microseconds m_start_t{};
 
     //! Log prefix; usually the name of the function this was created in.
-    const std::string m_prefix{};
+    const std::string m_prefix;
 
     //! A descriptive message of what is being timed.
-    const std::string m_title{};
+    const std::string m_title;
 
     //! Forwarded on to LogPrint if specified - has the effect of only
     //! outputting the timing log when a particular debug= category is specified.
-    const BCLog::LogFlags m_log_category{};
+    const BCLog::LogFlags m_log_category;
+
+    //! Whether to output the message again on completion.
+    const bool m_message_on_completion;
 };
 
 } // namespace BCLog
@@ -91,6 +100,8 @@ private:
     BCLog::Timer<std::chrono::microseconds> UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category)
 #define LOG_TIME_MILLIS_WITH_CATEGORY(end_msg, log_category) \
     BCLog::Timer<std::chrono::milliseconds> UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category)
+#define LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(end_msg, log_category) \
+    BCLog::Timer<std::chrono::milliseconds> UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category, /* msg_on_completion=*/false)
 #define LOG_TIME_SECONDS(end_msg) \
     BCLog::Timer<std::chrono::seconds> UNIQUE_NAME(logging_timer)(__func__, end_msg)
 

src/net.cpp

@@ -2688,7 +2688,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                     case ConnectionType::BLOCK_RELAY:
                     case ConnectionType::ADDR_FETCH:
                     case ConnectionType::FEELER:
-                        setConnected.insert(pnode->addr.GetGroup(addrman.GetAsmap()));
+                        setConnected.insert(m_netgroupman.GetGroup(pnode->addr));
                 } // no default case, so the compiler can warn about missing cases
             }
         }
@@ -2778,7 +2778,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
                 m_anchors.pop_back();
                 if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) ||
                     !HasAllDesirableServiceFlags(addr.nServices) ||
-                    setConnected.count(addr.GetGroup(addrman.GetAsmap()))) continue;
+                    setConnected.count(m_netgroupman.GetGroup(addr))) continue;
                 addrConnect = addr;
                 LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToString());
                 break;
@@ -2822,12 +2822,12 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
             bool isMasternode = dmn != nullptr;
 
             // Require outbound connections, other than feelers, to be to distinct network groups
-            if (!fFeeler && setConnected.count(addr.GetGroup(addrman.GetAsmap()))) {
+            if (!fFeeler && setConnected.count(m_netgroupman.GetGroup(addr))) {
                 break;
             }
 
             // if we selected an invalid address, restart
-            if (!addr.IsValid() || setConnected.count(addr.GetGroup(addrman.GetAsmap())))
+            if (!addr.IsValid() || setConnected.count(m_netgroupman.GetGroup(addr)))
                 break;
 
             // don't try to connect to masternodes that we already have a connection to (most likely inbound)
@@ -3488,8 +3488,12 @@ void CConnman::SetNetworkActive(bool active, CMasternodeSync* const mn_sync)
     uiInterface.NotifyNetworkActiveChanged(fNetworkActive);
 }
 
-CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, bool network_active) :
+CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in,
-        addrman(addrman_in), nSeed0(nSeed0In), nSeed1(nSeed1In)
+                   const NetGroupManager& netgroupman, bool network_active)
+    : addrman(addrman_in)
+    , m_netgroupman{netgroupman}
+    , nSeed0(nSeed0In)
+    , nSeed1(nSeed1In)
 {
     SetTryNewOutboundPeer(false);
 
@@ -4069,7 +4073,7 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) const
         }
         vstats.emplace_back();
         pnode->CopyStats(vstats.back());
-        vstats.back().m_mapped_as = pnode->addr.GetMappedAS(addrman.GetAsmap());
+        vstats.back().m_mapped_as = m_netgroupman.GetMappedAS(pnode->addr);
     }
 }
 
@@ -4374,9 +4378,9 @@ CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id) const
     return CSipHasher(nSeed0, nSeed1).Write(id);
 }
 
-uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& ad) const
+uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& address) const
 {
-    std::vector<unsigned char> vchNetGroup(ad.GetGroup(addrman.GetAsmap()));
+    std::vector<unsigned char> vchNetGroup(m_netgroupman.GetGroup(address));
 
     return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
 }

src/net.h

@@ -18,6 +18,7 @@
 #include <net_permissions.h>
 #include <netaddress.h>
 #include <netbase.h>
+#include <netgroup.h>
 #include <policy/feerate.h>
 #include <protocol.h>
 #include <random.h>
@@ -991,7 +992,9 @@ public:
         m_onion_binds = connOptions.onion_binds;
     }
 
-    CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true);
+    CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, const NetGroupManager& netgroupman,
+             bool network_active = true);
+
     ~CConnman();
     bool Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync,
                CScheduler& scheduler, const Options& options)
@@ -1505,6 +1508,7 @@ private:
     std::atomic<bool> fNetworkActive{true};
     bool fAddressesInitialized{false};
     AddrMan& addrman;
+    const NetGroupManager& m_netgroupman;
     std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
     Mutex m_addr_fetches_mutex;
     std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);

src/netaddress.cpp

@@ -10,7 +10,6 @@
 #include <hash.h>
 #include <prevector.h>
 #include <tinyformat.h>
-#include <util/asmap.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 
@@ -728,107 +727,6 @@ Network CNetAddr::GetNetClass() const
     return m_net;
 }
 
-uint32_t CNetAddr::GetMappedAS(const std::vector<bool> &asmap) const {
-    uint32_t net_class = GetNetClass();
-    if (asmap.size() == 0 || (net_class != NET_IPV4 && net_class != NET_IPV6)) {
-        return 0; // Indicates not found, safe because AS0 is reserved per RFC7607.
-    }
-    std::vector<bool> ip_bits(128);
-    if (HasLinkedIPv4()) {
-        // For lookup, treat as if it was just an IPv4 address (IPV4_IN_IPV6_PREFIX + IPv4 bits)
-        for (int8_t byte_i = 0; byte_i < 12; ++byte_i) {
-            for (uint8_t bit_i = 0; bit_i < 8; ++bit_i) {
-                ip_bits[byte_i * 8 + bit_i] = (IPV4_IN_IPV6_PREFIX[byte_i] >> (7 - bit_i)) & 1;
-            }
-        }
-        uint32_t ipv4 = GetLinkedIPv4();
-        for (int i = 0; i < 32; ++i) {
-            ip_bits[96 + i] = (ipv4 >> (31 - i)) & 1;
-        }
-    } else {
-        // Use all 128 bits of the IPv6 address otherwise
-        assert(IsIPv6());
-        for (int8_t byte_i = 0; byte_i < 16; ++byte_i) {
-            uint8_t cur_byte = m_addr[byte_i];
-            for (uint8_t bit_i = 0; bit_i < 8; ++bit_i) {
-                ip_bits[byte_i * 8 + bit_i] = (cur_byte >> (7 - bit_i)) & 1;
-            }
-        }
-    }
-    uint32_t mapped_as = Interpret(asmap, ip_bits);
-    return mapped_as;
-}
-
-/**
- * Get the canonical identifier of our network group
- *
- * The groups are assigned in a way where it should be costly for an attacker to
- * obtain addresses with many different group identifiers, even if it is cheap
- * to obtain addresses with the same identifier.
- *
- * @note No two connections will be attempted to addresses with the same network
- *       group.
- */
-std::vector<unsigned char> CNetAddr::GetGroup(const std::vector<bool> &asmap) const
-{
-    std::vector<unsigned char> vchRet;
-    uint32_t net_class = GetNetClass();
-    // If non-empty asmap is supplied and the address is IPv4/IPv6,
-    // return ASN to be used for bucketing.
-    uint32_t asn = GetMappedAS(asmap);
-    if (asn != 0) { // Either asmap was empty, or address has non-asmappable net class (e.g. TOR).
-        vchRet.push_back(NET_IPV6); // IPv4 and IPv6 with same ASN should be in the same bucket
-        for (int i = 0; i < 4; i++) {
-            vchRet.push_back((asn >> (8 * i)) & 0xFF);
-        }
-        return vchRet;
-    }
-
-    vchRet.push_back(net_class);
-    int nBits{0};
-
-    if (IsLocal()) {
-        // all local addresses belong to the same group
-    } else if (IsInternal()) {
-        // all internal-usage addresses get their own group
-        nBits = ADDR_INTERNAL_SIZE * 8;
-    } else if (!IsRoutable()) {
-        // all other unroutable addresses belong to the same group
-    } else if (HasLinkedIPv4()) {
-        // IPv4 addresses (and mapped IPv4 addresses) use /16 groups
-        uint32_t ipv4 = GetLinkedIPv4();
-        vchRet.push_back((ipv4 >> 24) & 0xFF);
-        vchRet.push_back((ipv4 >> 16) & 0xFF);
-        return vchRet;
-    } else if (IsTor() || IsI2P()) {
-        nBits = 4;
-    } else if (IsCJDNS()) {
-        // Treat in the same way as Tor and I2P because the address in all of
-        // them is "random" bytes (derived from a public key). However in CJDNS
-        // the first byte is a constant 0xfc, so the random bytes come after it.
-        // Thus skip the constant 8 bits at the start.
-        nBits = 12;
-    } else if (IsHeNet()) {
-        // for he.net, use /36 groups
-        nBits = 36;
-    } else {
-        // for the rest of the IPv6 network, use /32 groups
-        nBits = 32;
-    }
-
-    // Push our address onto vchRet.
-    const size_t num_bytes = nBits / 8;
-    vchRet.insert(vchRet.end(), m_addr.begin(), m_addr.begin() + num_bytes);
-    nBits %= 8;
-    // ...for the last byte, push nBits and for the rest of the byte push 1's
-    if (nBits > 0) {
-        assert(num_bytes < m_addr.size());
-        vchRet.push_back(m_addr[num_bytes] | ((1 << (8 - nBits)) - 1));
-    }
-
-    return vchRet;
-}
-
 std::vector<unsigned char> CNetAddr::GetAddrBytes() const
 {
     if (IsAddrV1Compatible()) {

src/netaddress.h

@@ -211,12 +211,6 @@ public:
     //! Whether this address has a linked IPv4 address (see GetLinkedIPv4()).
     bool HasLinkedIPv4() const;
 
-    // The AS on the BGP path to the node we use to diversify
-    // peers in AddrMan bucketing based on the AS infrastructure.
-    // The ip->AS mapping depends on how asmap is constructed.
-    uint32_t GetMappedAS(const std::vector<bool> &asmap) const;
-
-    std::vector<unsigned char> GetGroup(const std::vector<bool> &asmap) const;
     std::vector<unsigned char> GetAddrBytes() const;
     int GetReachabilityFrom(const CNetAddr *paddrPartner = nullptr) const;
 

src/netgroup.cpp

@@ -0,0 +1,111 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <netgroup.h>
+
+#include <hash.h>
+#include <util/asmap.h>
+
+uint256 NetGroupManager::GetAsmapChecksum() const
+{
+    if (!m_asmap.size()) return {};
+
+    return SerializeHash(m_asmap);
+}
+
+std::vector<unsigned char> NetGroupManager::GetGroup(const CNetAddr& address) const
+{
+    std::vector<unsigned char> vchRet;
+    // If non-empty asmap is supplied and the address is IPv4/IPv6,
+    // return ASN to be used for bucketing.
+    uint32_t asn = GetMappedAS(address);
+    if (asn != 0) { // Either asmap was empty, or address has non-asmappable net class (e.g. TOR).
+        vchRet.push_back(NET_IPV6); // IPv4 and IPv6 with same ASN should be in the same bucket
+        for (int i = 0; i < 4; i++) {
+            vchRet.push_back((asn >> (8 * i)) & 0xFF);
+        }
+        return vchRet;
+    }
+
+    vchRet.push_back(address.GetNetClass());
+    int nStartByte{0};
+    int nBits{0};
+
+    if (address.IsLocal()) {
+        // all local addresses belong to the same group
+    } else if (address.IsInternal()) {
+        // All internal-usage addresses get their own group.
+        // Skip over the INTERNAL_IN_IPV6_PREFIX returned by CAddress::GetAddrBytes().
+        nStartByte = INTERNAL_IN_IPV6_PREFIX.size();
+        nBits = ADDR_INTERNAL_SIZE * 8;
+    } else if (!address.IsRoutable()) {
+        // all other unroutable addresses belong to the same group
+    } else if (address.HasLinkedIPv4()) {
+        // IPv4 addresses (and mapped IPv4 addresses) use /16 groups
+        uint32_t ipv4 = address.GetLinkedIPv4();
+        vchRet.push_back((ipv4 >> 24) & 0xFF);
+        vchRet.push_back((ipv4 >> 16) & 0xFF);
+        return vchRet;
+    } else if (address.IsTor() || address.IsI2P()) {
+        nBits = 4;
+    } else if (address.IsCJDNS()) {
+        // Treat in the same way as Tor and I2P because the address in all of
+        // them is "random" bytes (derived from a public key). However in CJDNS
+        // the first byte is a constant 0xfc, so the random bytes come after it.
+        // Thus skip the constant 8 bits at the start.
+        nBits = 12;
+    } else if (address.IsHeNet()) {
+        // for he.net, use /36 groups
+        nBits = 36;
+    } else {
+        // for the rest of the IPv6 network, use /32 groups
+        nBits = 32;
+    }
+
+    // Push our address onto vchRet.
+    auto addr_bytes = address.GetAddrBytes();
+    const size_t num_bytes = nBits / 8;
+    vchRet.insert(vchRet.end(), addr_bytes.begin() + nStartByte, addr_bytes.begin() + nStartByte + num_bytes);
+    nBits %= 8;
+    // ...for the last byte, push nBits and for the rest of the byte push 1's
+    if (nBits > 0) {
+        assert(num_bytes < addr_bytes.size());
+        vchRet.push_back(addr_bytes[num_bytes] | ((1 << (8 - nBits)) - 1));
+    }
+
+    return vchRet;
+}
+
+uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
+{
+    uint32_t net_class = address.GetNetClass();
+    if (m_asmap.size() == 0 || (net_class != NET_IPV4 && net_class != NET_IPV6)) {
+        return 0; // Indicates not found, safe because AS0 is reserved per RFC7607.
+    }
+    std::vector<bool> ip_bits(128);
+    if (address.HasLinkedIPv4()) {
+        // For lookup, treat as if it was just an IPv4 address (IPV4_IN_IPV6_PREFIX + IPv4 bits)
+        for (int8_t byte_i = 0; byte_i < 12; ++byte_i) {
+            for (uint8_t bit_i = 0; bit_i < 8; ++bit_i) {
+                ip_bits[byte_i * 8 + bit_i] = (IPV4_IN_IPV6_PREFIX[byte_i] >> (7 - bit_i)) & 1;
+            }
+        }
+        uint32_t ipv4 = address.GetLinkedIPv4();
+        for (int i = 0; i < 32; ++i) {
+            ip_bits[96 + i] = (ipv4 >> (31 - i)) & 1;
+        }
+    } else {
+        // Use all 128 bits of the IPv6 address otherwise
+        assert(address.IsIPv6());
+        auto addr_bytes = address.GetAddrBytes();
+        for (int8_t byte_i = 0; byte_i < 16; ++byte_i) {
+            uint8_t cur_byte = addr_bytes[byte_i];
+            for (uint8_t bit_i = 0; bit_i < 8; ++bit_i) {
+                ip_bits[byte_i * 8 + bit_i] = (cur_byte >> (7 - bit_i)) & 1;
+            }
+        }
+    }
+    uint32_t mapped_as = Interpret(m_asmap, ip_bits);
+    return mapped_as;
+}

src/netgroup.h

@@ -0,0 +1,66 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_NETGROUP_H
+#define BITCOIN_NETGROUP_H
+
+#include <netaddress.h>
+#include <uint256.h>
+
+#include <vector>
+
+/**
+ * Netgroup manager
+ */
+class NetGroupManager {
+public:
+    explicit NetGroupManager(std::vector<bool> asmap)
+        : m_asmap{std::move(asmap)}
+    {}
+
+    /** Get a checksum identifying the asmap being used. */
+    uint256 GetAsmapChecksum() const;
+
+    /**
+     * Get the canonical identifier of the network group for address.
+     *
+     * The groups are assigned in a way where it should be costly for an attacker to
+     * obtain addresses with many different group identifiers, even if it is cheap
+     * to obtain addresses with the same identifier.
+     *
+     * @note No two connections will be attempted to addresses with the same network
+     *       group.
+     */
+    std::vector<unsigned char> GetGroup(const CNetAddr& address) const;
+
+    /**
+     *  Get the autonomous system on the BGP path to address.
+     *
+     *  The ip->AS mapping depends on how asmap is constructed.
+     */
+    uint32_t GetMappedAS(const CNetAddr& address) const;
+
+private:
+    /** Compressed IP->ASN mapping, loaded from a file when a node starts.
+     *
+     * This mapping is then used for bucketing nodes in Addrman and for
+     * ensuring we connect to a diverse set of peers in Connman. The map is
+     * empty if no file was provided.
+     *
+     * If asmap is provided, nodes will be bucketed by AS they belong to, in
+     * order to make impossible for a node to connect to several nodes hosted
+     * in a single AS. This is done in response to Erebus attack, but also to
+     * generally diversify the connections every node creates, especially
+     * useful when a large fraction of nodes operate under a couple of cloud
+     * providers.
+     *
+     * If a new asmap is provided, the existing addrman records are
+     * re-bucketed.
+     *
+     * This is initialized in the constructor, const, and therefore is
+     * thread-safe. */
+    const std::vector<bool> m_asmap;
+};
+
+#endif // BITCOIN_NETGROUP_H

src/node/context.cpp

@@ -22,6 +22,7 @@
 #include <net.h>
 #include <netfulfilledman.h>
 #include <net_processing.h>
+#include <netgroup.h>
 #include <policy/fees.h>
 #include <scheduler.h>
 #include <spork.h>

src/node/context.h

@@ -29,6 +29,7 @@ class CScheduler;
 class CSporkManager;
 class CTxMemPool;
 class CMNHFManager;
+class NetGroupManager;
 class PeerManager;
 struct CJContext;
 struct LLMQContext;
@@ -59,6 +60,7 @@ struct NodeContext {
     std::unique_ptr<AddrMan> addrman;
     std::unique_ptr<CConnman> connman;
     std::unique_ptr<CTxMemPool> mempool;
+    std::unique_ptr<const NetGroupManager> netgroupman;
     std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
     std::unique_ptr<PeerManager> peerman;
     std::unique_ptr<ChainstateManager> chainman;

src/qt/test/test_main.cpp

@@ -23,6 +23,7 @@
 #include <QApplication>
 #include <QObject>
 #include <QTest>
+#include <functional>
 
 #if defined(QT_STATICPLUGIN)
 #include <QtPlugin>
@@ -40,6 +41,8 @@ Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin);
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
+
 // This is all you need to run all the tests
 int main(int argc, char* argv[])
 {

src/rpc/client.cpp

@@ -225,6 +225,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "upgradetohd", 3, "rescan"},
     { "getnodeaddresses", 0, "count"},
     { "addpeeraddress", 1, "port"},
+    { "addpeeraddress", 2, "tried"},
     { "stop", 0, "wait" },
     { "verifychainlock", 2, "blockHeight" },
     { "verifyislock", 3, "maxHeight" },

src/rpc/net.cpp

@@ -970,6 +970,7 @@ static RPCHelpMan addpeeraddress()
         {
             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address of the peer"},
             {"port", RPCArg::Type::NUM, RPCArg::Optional::NO, "The port of the peer"},
+            {"tried", RPCArg::Type::BOOL, /* default */ "false", "If true, attempt to add the peer to the tried addresses table"},
         },
         RPCResult{
             RPCResult::Type::OBJ, "", "",
@@ -978,8 +979,8 @@ static RPCHelpMan addpeeraddress()
             },
         },
         RPCExamples{
-            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999")
+            HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999 true")
-    + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999")
+    + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999, true")
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
@@ -991,6 +992,7 @@ static RPCHelpMan addpeeraddress()
 
     const std::string& addr_string{request.params[0].get_str()};
     const uint16_t port = request.params[1].get_int();
+    const bool tried{request.params[2].isTrue()};
 
     UniValue obj(UniValue::VOBJ);
     CNetAddr net_addr;
@@ -1001,7 +1003,13 @@ static RPCHelpMan addpeeraddress()
         address.nTime = GetAdjustedTime();
         // The source address is set equal to the address. This is equivalent to the peer
         // announcing itself.
-        if (node.addrman->Add({address}, address)) success = true;
+        if (node.addrman->Add({address}, address)) {
+            success = true;
+            if (tried) {
+                // Attempt to move the address to the tried addresses table.
+                node.addrman->Good(address);
+            }
+        }
     }
 
     obj.pushKV("success", success);

src/test/README.md

@@ -33,19 +33,31 @@ the `src/qt/test/test_main.cpp` file.
 
 ### Running individual tests
 
-`test_dash` has some built-in command-line arguments; for
+`test_dash` accepts the command line arguments from the boost framework.
-example, to run just the `getarg_tests` verbosely:
+For example, to run just the `getarg_tests` suite of tests:
 
-    test_dash --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT
+```bash
+test_dash --log_level=all --run_test=getarg_tests
+```
 
 `log_level` controls the verbosity of the test framework, which logs when a
-test case is entered, for example. The `DEBUG_LOG_OUT` after the two dashes
+test case is entered, for example. `test_dash` also accepts the command
-redirects the debug log, which would normally go to a file in the test datadir
+line arguments accepted by `dashd`. Use `--` to separate both types of
+arguments:
+
+```bash
+test_dash --log_level=all --run_test=getarg_tests -- -printtoconsole=1
+```
+
+The `-printtoconsole=1` after the two dashes redirects the debug log, which
+would normally go to a file in the test datadir
 (`BasicTestingSetup::m_path_root`), to the standard terminal output.
 
 ... or to run just the doubledash test:
 
-    test_dash --run_test=getarg_tests/doubledash
+```bash
+test_dash --run_test=getarg_tests/doubledash
+```
 
 Run `test_dash --help` for the full list.
 
@@ -68,9 +80,35 @@ on failure. For running individual tests verbosely, refer to the section
 To write to logs from unit tests you need to use specific message methods
 provided by Boost. The simplest is `BOOST_TEST_MESSAGE`.
 
-For debugging you can launch the `test_dash` executable with `gdb`or `lldb` and
+For debugging you can launch the `test_dash` executable with `gdb` or `lldb` and
 start debugging, just like you would with any other program:
 
 ```bash
 gdb src/test/test_dash
 ```
+
+#### Segmentation faults
+
+If you hit a segmentation fault during a test run, you can diagnose where the fault
+is happening by running `gdb ./src/test/test_dash` and then using the `bt` command
+within gdb.
+
+Another tool that can be used to resolve segmentation faults is
+[valgrind](https://valgrind.org/).
+
+If for whatever reason you want to produce a core dump file for this fault, you can do
+that as well. By default, the boost test runner will intercept system errors and not
+produce a core file. To bypass this, add `--catch_system_errors=no` to the
+`test_dash` arguments and ensure that your ulimits are set properly (e.g. `ulimit -c
+unlimited`).
+
+Running the tests and hitting a segmentation fault should now produce a file called `core`
+(on Linux platforms, the file name will likely depend on the contents of
+`/proc/sys/kernel/core_pattern`).
+
+You can then explore the core dump using
+```bash
+gdb src/test/test_dash core
+
+(gbd) bt  # produce a backtrace for where a segfault occurred
+```

src/test/denialofservice_tests.cpp

@@ -137,7 +137,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 {
     NodeId id{0};
     const CChainParams& chainparams = Params();
-    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
@@ -217,7 +217,7 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
 {
     NodeId id{0};
     const CChainParams& chainparams = Params();
-    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
@@ -284,7 +284,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
 
     const CChainParams& chainparams = Params();
     auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
@@ -389,7 +389,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
 
     const CChainParams& chainparams = Params();
     auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,

src/test/fuzz/addrman.cpp

@@ -11,8 +11,10 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
 #include <time.h>
 #include <util/asmap.h>
+#include <util/system.h>
 
 #include <cassert>
 #include <cstdint>
@@ -20,16 +22,34 @@
 #include <string>
 #include <vector>
 
+namespace {
+const BasicTestingSetup* g_setup;
+
+int32_t GetCheckRatio()
+{
+    return std::clamp<int32_t>(g_setup->m_node.args->GetArg("-checkaddrman", 0), 0, 1000000);
+}
+} // namespace
+
 void initialize_addrman()
 {
-    SelectParams(CBaseChainParams::REGTEST);
+    static const auto testing_setup = MakeNoLogFileContext<>(CBaseChainParams::REGTEST);
+    g_setup = testing_setup.get();
+}
+
+[[nodiscard]] NetGroupManager ConsumeNetGroupManager(FuzzedDataProvider& fuzzed_data_provider) noexcept
+{
+    std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
+    if (!SanityCheckASMap(asmap, 128)) asmap.clear();
+    return NetGroupManager(asmap);
 }
 
 FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider);
-    AddrMan addr_man(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
+    AddrMan addr_man(netgroupman, /*deterministic=*/false, GetCheckRatio());
     try {
         ReadFromStream(addr_man, data_stream);
     } catch (const std::exception&) {
@@ -112,8 +132,8 @@ void FillAddrman(AddrMan& addrman, FuzzedDataProvider& fuzzed_data_provider)
 class AddrManDeterministic : public AddrMan
 {
 public:
-    explicit AddrManDeterministic(std::vector<bool> asmap, FuzzedDataProvider& fuzzed_data_provider)
+    explicit AddrManDeterministic(const NetGroupManager& netgroupman, FuzzedDataProvider& fuzzed_data_provider)
-        : AddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0)
+        : AddrMan(netgroupman, /*deterministic=*/true, GetCheckRatio())
     {
         WITH_LOCK(m_impl->cs, m_impl->insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)});
     }
@@ -211,19 +231,12 @@ public:
     }
 };
 
-[[nodiscard]] inline std::vector<bool> ConsumeAsmap(FuzzedDataProvider& fuzzed_data_provider) noexcept
-{
-    std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-    if (!SanityCheckASMap(asmap, 128)) asmap.clear();
-    return asmap;
-}
-
 FUZZ_TARGET_INIT(addrman, initialize_addrman)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    std::vector<bool> asmap = ConsumeAsmap(fuzzed_data_provider);
+    NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
-    auto addr_man_ptr = std::make_unique<AddrManDeterministic>(asmap, fuzzed_data_provider);
+    auto addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider);
     if (fuzzed_data_provider.ConsumeBool()) {
         const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)};
         CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION);
@@ -232,7 +245,7 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
         try {
             ds >> *addr_man_ptr;
         } catch (const std::ios_base::failure&) {
-            addr_man_ptr = std::make_unique<AddrManDeterministic>(asmap, fuzzed_data_provider);
+            addr_man_ptr = std::make_unique<AddrManDeterministic>(netgroupman, fuzzed_data_provider);
         }
     }
     AddrManDeterministic& addr_man = *addr_man_ptr;
@@ -304,9 +317,9 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman)
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     SetMockTime(ConsumeTime(fuzzed_data_provider));
 
-    std::vector<bool> asmap = ConsumeAsmap(fuzzed_data_provider);
+    NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
-    AddrManDeterministic addr_man1{asmap, fuzzed_data_provider};
+    AddrManDeterministic addr_man1{netgroupman, fuzzed_data_provider};
-    AddrManDeterministic addr_man2{asmap, fuzzed_data_provider};
+    AddrManDeterministic addr_man2{netgroupman, fuzzed_data_provider};
 
     CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
 

src/test/fuzz/asmap.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <netaddress.h>
+#include <netgroup.h>
 #include <test/fuzz/fuzz.h>
 #include <util/asmap.h>
 
@@ -56,5 +57,6 @@ FUZZ_TARGET(asmap)
         memcpy(&ipv4, addr_data, addr_size);
         net_addr.SetIP(CNetAddr{ipv4});
     }
-    (void)net_addr.GetMappedAS(asmap);
+    NetGroupManager netgroupman{asmap};
+    (void)netgroupman.GetMappedAS(net_addr);
 }

src/test/fuzz/connman.cpp

@@ -11,22 +11,31 @@
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <test/util/setup_common.h>
+#include <util/system.h>
 #include <util/translation.h>
 
 #include <cstdint>
 #include <vector>
 
+namespace {
+const TestingSetup* g_setup;
+} // namespace
+
 void initialize_connman()
 {
-    static const auto testing_setup = MakeNoLogFileContext<>();
+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    g_setup = testing_setup.get();
 }
 
 FUZZ_TARGET_INIT(connman, initialize_connman)
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    AddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
-    CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman};
+                     fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
+                     *g_setup->m_node.addrman,
+                     *g_setup->m_node.netgroupman,
+                     fuzzed_data_provider.ConsumeBool()};
     CNetAddr random_netaddr;
     CNode random_node = ConsumeNode(fuzzed_data_provider);
     CSubNet random_subnet;

src/test/fuzz/deserialize.cpp

@@ -15,13 +15,16 @@
 #include <merkleblock.h>
 #include <net.h>
 #include <netbase.h>
+#include <netgroup.h>
 #include <node/utxo_snapshot.h>
 #include <primitives/block.h>
 #include <protocol.h>
 #include <psbt.h>
 #include <script/sign.h>
 #include <streams.h>
+#include <test/util/setup_common.h>
 #include <undo.h>
+#include <util/system.h>
 #include <version.h>
 
 #include <exception>
@@ -32,8 +35,14 @@
 
 #include <test/fuzz/fuzz.h>
 
+namespace {
+const BasicTestingSetup* g_setup;
+} // namespace
+
 void initialize_deserialize()
 {
+    static const auto testing_setup = MakeNoLogFileContext<>();
+    g_setup = testing_setup.get();
 }
 
 #define FUZZ_TARGET_DESERIALIZE(name, code)                \
@@ -190,7 +199,10 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, {
     BlockMerkleRoot(block, &mutated);
 })
 FUZZ_TARGET_DESERIALIZE(addrman_deserialize, {
-    AddrMan am(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    NetGroupManager netgroupman{std::vector<bool>()};
+    AddrMan am(netgroupman,
+               /*deterministic=*/false,
+               g_setup->m_node.args->GetArg("-checkaddrman", 0));
     DeserializeFromFuzzingInput(buffer, am);
 })
 FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, {

src/test/fuzz/fuzz.cpp

@@ -16,6 +16,7 @@
 #include <cstdint>
 #include <exception>
 #include <fstream>
+#include <functional>
 #include <memory>
 #include <string>
 #include <unistd.h>
@@ -23,6 +24,29 @@
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+/**
+ * A copy of the command line arguments that start with `--`.
+ * First `LLVMFuzzerInitialize()` is called, which saves the arguments to `g_args`.
+ * Later, depending on the fuzz test, `G_TEST_COMMAND_LINE_ARGUMENTS()` may be
+ * called by `BasicTestingSetup` constructor to fetch those arguments and store
+ * them in `BasicTestingSetup::m_node::args`.
+ */
+static std::vector<const char*> g_args;
+
+static void SetArgs(int argc, char** argv) {
+    for (int i = 1; i < argc; ++i) {
+        // Only take into account arguments that start with `--`. The others are for the fuzz engine:
+        // `fuzz -runs=1 fuzz_seed_corpus/address_deserialize_v2 --checkaddrman=5`
+        if (strlen(argv[i]) > 2 && argv[i][0] == '-' && argv[i][1] == '-') {
+            g_args.push_back(argv[i]);
+        }
+    }
+}
+
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS = []() {
+    return g_args;
+};
+
 std::map<std::string_view, std::tuple<TypeTestOneInput, TypeInitialize, TypeHidden>>& FuzzTargets()
 {
     static std::map<std::string_view, std::tuple<TypeTestOneInput, TypeInitialize, TypeHidden>> g_fuzz_targets;
@@ -132,6 +156,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
 // This function is used by libFuzzer
 extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv)
 {
+    SetArgs(*argc, *argv);
     initialize();
     return 0;
 }

src/test/fuzz/net.cpp

@@ -21,11 +21,24 @@
 #include <string>
 #include <vector>
 
+namespace {
+const BasicTestingSetup* g_setup;
+
+int32_t GetCheckRatio()
+{
+    return std::clamp<int32_t>(g_setup->m_node.args->GetArg("-checkaddrman", 0), 0, 1000000);
+}
+} // namespace
+
 void initialize_net()
 {
     static const auto testing_setup = MakeNoLogFileContext<>(CBaseChainParams::MAIN);
+    g_setup = testing_setup.get();
 }
 
+// From src/test/fuzz/addrman.cpp
+extern NetGroupManager ConsumeNetGroupManager(FuzzedDataProvider& fuzzed_data_provider) noexcept;
+
 FUZZ_TARGET_INIT(net, initialize_net)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
@@ -37,8 +50,9 @@ FUZZ_TARGET_INIT(net, initialize_net)
         CallOneOf(
             fuzzed_data_provider,
             [&] {
-                AddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+                NetGroupManager netgroupman{ConsumeNetGroupManager(fuzzed_data_provider)};
-                CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman};
+                AddrMan addrman(netgroupman, /*deterministic=*/false, GetCheckRatio());
+                CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, netgroupman};
                 node.CloseSocketDisconnect(&connman);
             },
             [&] {

src/test/main.cpp

@@ -11,6 +11,7 @@
 
 #include <test/util/setup_common.h>
 
+#include <functional>
 #include <iostream>
 
 /** Redirect debug log to unit_test.log files */
@@ -24,3 +25,17 @@ const std::function<void(const std::string&)> G_TEST_LOG_FUN = [](const std::str
     if (!should_log) return;
     std::cout << s;
 };
+
+/**
+ * Retrieve the command line arguments from boost.
+ * Allows usage like:
+ * `test_dash --run_test="net_tests/cnode_listen_port" -- -checkaddrman=1 -printtoconsole=1`
+ * which would return `["-checkaddrman=1", "-printtoconsole=1"]`.
+ */
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS = []() {
+    std::vector<const char*> args;
+    for (int i = 1; i < boost::unit_test::framework::master_test_suite().argc; ++i) {
+        args.push_back(boost::unit_test::framework::master_test_suite().argv[i]);
+    }
+    return args;
+};

src/test/netbase_tests.cpp

@@ -6,6 +6,7 @@
 #include <net_permissions.h>
 #include <netaddress.h>
 #include <netbase.h>
+#include <netgroup.h>
 #include <protocol.h>
 #include <serialize.h>
 #include <streams.h>
@@ -316,22 +317,22 @@ BOOST_AUTO_TEST_CASE(subnet_test)
 
 BOOST_AUTO_TEST_CASE(netbase_getgroup)
 {
-    std::vector<bool> asmap; // use /16
+    NetGroupManager netgroupman{std::vector<bool>()}; // use /16
-    BOOST_CHECK(ResolveIP("127.0.0.1").GetGroup(asmap) == std::vector<unsigned char>({0})); // Local -> !Routable()
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("127.0.0.1")) == std::vector<unsigned char>({0})); // Local -> !Routable()
-    BOOST_CHECK(ResolveIP("257.0.0.1").GetGroup(asmap) == std::vector<unsigned char>({0})); // !Valid -> !Routable()
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("257.0.0.1")) == std::vector<unsigned char>({0})); // !Valid -> !Routable()
-    BOOST_CHECK(ResolveIP("10.0.0.1").GetGroup(asmap) == std::vector<unsigned char>({0})); // RFC1918 -> !Routable()
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("10.0.0.1")) == std::vector<unsigned char>({0})); // RFC1918 -> !Routable()
-    BOOST_CHECK(ResolveIP("169.254.1.1").GetGroup(asmap) == std::vector<unsigned char>({0})); // RFC3927 -> !Routable()
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("169.254.1.1")) == std::vector<unsigned char>({0})); // RFC3927 -> !Routable()
-    BOOST_CHECK(ResolveIP("1.2.3.4").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // IPv4
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("1.2.3.4")) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // IPv4
-    BOOST_CHECK(ResolveIP("::FFFF:0:102:304").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC6145
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("::FFFF:0:102:304")) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC6145
-    BOOST_CHECK(ResolveIP("64:FF9B::102:304").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC6052
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("64:FF9B::102:304")) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC6052
-    BOOST_CHECK(ResolveIP("2002:102:304:9999:9999:9999:9999:9999").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC3964
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("2002:102:304:9999:9999:9999:9999:9999")) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC3964
-    BOOST_CHECK(ResolveIP("2001:0:9999:9999:9999:9999:FEFD:FCFB").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC4380
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("2001:0:9999:9999:9999:9999:FEFD:FCFB")) == std::vector<unsigned char>({(unsigned char)NET_IPV4, 1, 2})); // RFC4380
-    BOOST_CHECK(ResolveIP("2001:470:abcd:9999:9999:9999:9999:9999").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV6, 32, 1, 4, 112, 175})); //he.net
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("2001:470:abcd:9999:9999:9999:9999:9999")) == std::vector<unsigned char>({(unsigned char)NET_IPV6, 32, 1, 4, 112, 175})); //he.net
-    BOOST_CHECK(ResolveIP("2001:2001:9999:9999:9999:9999:9999:9999").GetGroup(asmap) == std::vector<unsigned char>({(unsigned char)NET_IPV6, 32, 1, 32, 1})); //IPv6
+    BOOST_CHECK(netgroupman.GetGroup(ResolveIP("2001:2001:9999:9999:9999:9999:9999:9999")) == std::vector<unsigned char>({(unsigned char)NET_IPV6, 32, 1, 32, 1})); //IPv6
 
     // baz.net sha256 hash: 12929400eb4607c4ac075f087167e75286b179c693eb059a01774b864e8fe505
     std::vector<unsigned char> internal_group = {NET_INTERNAL, 0x12, 0x92, 0x94, 0x00, 0xeb, 0x46, 0x07, 0xc4, 0xac, 0x07};
-    BOOST_CHECK(CreateInternal("baz.net").GetGroup(asmap) == internal_group);
+    BOOST_CHECK(netgroupman.GetGroup(CreateInternal("baz.net")) == internal_group);
 }
 
 // Since CNetAddr (un)ser is tested separately in net_tests.cpp here we only

src/test/util/setup_common.cpp

@@ -67,6 +67,7 @@
 #include <evo/mnhftx.h>
 #include <evo/specialtx.h>
 
+#include <stdexcept>
 #include <memory>
 
 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
@@ -140,7 +141,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
       m_args{}
 {
     m_node.args = &gArgs;
-    const std::vector<const char*> arguments = Cat(
+    std::vector<const char*> arguments = Cat(
         {
             "dummy",
             "-printtoconsole=0",
@@ -152,6 +153,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
             "-debugexclude=leveldb",
         },
         extra_args);
+    if (G_TEST_COMMAND_LINE_ARGUMENTS) {
+        arguments = Cat(arguments, G_TEST_COMMAND_LINE_ARGUMENTS());
+    }
     util::ThreadRename("test");
     fs::create_directories(m_path_root);
     m_args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
@@ -160,9 +164,10 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     {
         SetupServerArgs(*m_node.args);
         std::string error;
-        const bool success{m_node.args->ParseParameters(arguments.size(), arguments.data(), error)};
+        if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
-        assert(success);
+            m_node.args->ClearArgs();
-        assert(error.empty());
+            throw std::runtime_error{error};
+        }
     }
     SelectParams(chainName);
     SeedInsecureRand();
@@ -177,8 +182,14 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     SetupNetworking();
     InitSignatureCache();
     InitScriptExecutionCache();
-    m_node.addrman = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
     m_node.chain = interfaces::MakeChain(m_node);
+
+    m_node.netgroupman = std::make_unique<NetGroupManager>(/*asmap=*/std::vector<bool>());
+    m_node.addrman = std::make_unique<AddrMan>(*m_node.netgroupman,
+                                               /*deterministic=*/false,
+                                               m_node.args->GetArg("-checkaddrman", 0));
+    m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); // Deterministic randomness for tests.
+
     // while g_wallet_init_interface is init here at very early stage
     // we can't get rid of unique_ptr from wallet/contex.h
     // TODO: remove unique_ptr from wallet/context.h after bitcoin/bitcoin#22219
@@ -186,7 +197,6 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     fCheckBlockIndex = true;
     m_node.evodb = std::make_unique<CEvoDB>(1 << 20, true, true);
     m_node.mnhf_manager = std::make_unique<CMNHFManager>(*m_node.evodb);
-    connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
     llmq::quorumSnapshotManager.reset(new llmq::CQuorumSnapshotManager(*m_node.evodb));
     m_node.cpoolman = std::make_unique<CCreditPoolManager>(*m_node.evodb);
     static bool noui_connected = false;
@@ -200,11 +210,13 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
 BasicTestingSetup::~BasicTestingSetup()
 {
     SetMockTime(0s); // Reset mocktime for following tests
-    connman.reset();
-    llmq::quorumSnapshotManager.reset();
     m_node.cpoolman.reset();
+    llmq::quorumSnapshotManager.reset();
     m_node.mnhf_manager.reset();
     m_node.evodb.reset();
+    m_node.connman.reset();
+    m_node.addrman.reset();
+    m_node.netgroupman.reset();
 
     LogInstance().DisconnectTestLogger();
     fs::remove_all(m_path_root);
@@ -227,8 +239,6 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
     m_node.chainman = std::make_unique<ChainstateManager>();
     m_node.chainman->m_blockman.m_block_tree_db = std::make_unique<CBlockTreeDB>(1 << 20, true);
 
-    m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
-
     m_node.mn_metaman = std::make_unique<CMasternodeMetaMan>();
     m_node.netfulfilledman = std::make_unique<CNetFulfilledRequestManager>();
     m_node.sporkman = std::make_unique<CSporkManager>();
@@ -252,8 +262,6 @@ ChainTestingSetup::~ChainTestingSetup()
     m_node.sporkman.reset();
     m_node.netfulfilledman.reset();
     m_node.mn_metaman.reset();
-    m_node.connman.reset();
-    m_node.addrman.reset();
     m_node.args = nullptr;
     m_node.mempool.reset();
     m_node.scheduler.reset();

src/test/util/setup_common.h

@@ -18,6 +18,7 @@
 #include <util/string.h>
 #include <util/vector.h>
 
+#include <functional>
 #include <stdexcept>
 #include <type_traits>
 #include <vector>
@@ -27,6 +28,9 @@ class CChainParams;
 /** This is connected to the logger. Can be used to redirect logs to any other log */
 extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;
 
+/** Retrieve the command line arguments. */
+extern const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS;
+
 // Enable BOOST_CHECK_EQUAL for enum class types
 namespace std {
 template <typename T>
@@ -88,7 +92,6 @@ struct BasicTestingSetup {
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();
 
-    std::unique_ptr<CConnman> connman;
     const fs::path m_path_root;
     ArgsManager m_args;
 };

test/functional/feature_addrman.py

@@ -67,17 +67,16 @@ class AddrmanTest(BitcoinTestFramework):
             self.start_node(0, extra_args=["-checkaddrman=1"])
         assert_equal(self.nodes[0].getnodeaddresses(), [])
 
-        self.log.info("Check that addrman from future cannot be read")
+        self.log.info("Check that addrman from future is overwritten with new addrman")
         self.stop_node(0)
         write_addrman(peers_dat, lowest_compatible=111)
-        self.nodes[0].assert_start_raises_init_error(
+        assert_equal(os.path.exists(peers_dat + ".bak"), False)
-            expected_msg=init_error(
+        with self.nodes[0].assert_debug_log([
-                "Unsupported format of addrman database: 1. It is compatible with "
+                f'Creating new peers.dat because the file version was not compatible ("{peers_dat}"). Original backed up to peers.dat.bak',
-                "formats >=111, but the maximum supported by this version of "
+        ]):
-                f"{self.config['environment']['PACKAGE_NAME']} is 4.: (.+)"
+            self.start_node(0)
-            ),
+        assert_equal(self.nodes[0].getnodeaddresses(), [])
-            match=ErrorMatch.FULL_REGEX,
+        assert_equal(os.path.exists(peers_dat + ".bak"), True)
-        )
 
         self.log.info("Check that corrupt addrman cannot be read (EOF)")
         self.stop_node(0)

test/functional/feature_asmap.py

@@ -14,9 +14,11 @@ Verify node behaviour and debug log when launching dashd in these cases:
 
 4. `dashd -asmap/-asmap=` with no file specified, using the default asmap
 
-5. `dashd -asmap` with no file specified and a missing default asmap file
+5. `dashd -asmap` restart with an addrman containing new and tried entries
 
-6. `dashd -asmap` with an empty (unparsable) default asmap file
+6. `dashd -asmap` with no file specified and a missing default asmap file
+
+7. `dashd -asmap` with an empty (unparsable) default asmap file
 
 The tests are order-independent.
 
@@ -37,6 +39,12 @@ def expected_messages(filename):
 class AsmapTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
+        self.extra_args = [["-checkaddrman=1"]]  # Do addrman checks on all operations.
+
+    def fill_addrman(self, node_id):
+        """Add 1 tried address to the addrman, followed by 1 new address."""
+        for addr, tried in [[0, True], [1, False]]:
+            self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333)
 
     def test_without_asmap_arg(self):
         self.log.info('Test dashd with no -asmap arg passed')
@@ -72,6 +80,22 @@ class AsmapTest(BitcoinTestFramework):
                 self.start_node(0, [arg])
         os.remove(self.default_asmap)
 
+    def test_asmap_interaction_with_addrman_containing_entries(self):
+        self.log.info("Test dashd -asmap restart with addrman containing new and tried entries")
+        self.stop_node(0)
+        shutil.copyfile(self.asmap_raw, self.default_asmap)
+        self.start_node(0, ["-asmap", "-checkaddrman=1"])
+        self.fill_addrman(node_id=0)
+        self.restart_node(0, ["-asmap", "-checkaddrman=1"])
+        with self.node.assert_debug_log(
+            expected_msgs=[
+                "CheckAddrman: new 1, tried 1, total 2 started",
+                "CheckAddrman: completed",
+            ]
+        ):
+            self.node.getnodeaddresses()  # getnodeaddresses re-runs the addrman checks
+        os.remove(self.default_asmap)
+
     def test_default_asmap_with_missing_file(self):
         self.log.info('Test dashd -asmap with missing default map file')
         self.stop_node(0)
@@ -97,6 +121,7 @@ class AsmapTest(BitcoinTestFramework):
         self.test_asmap_with_absolute_path()
         self.test_asmap_with_relative_path()
         self.test_default_asmap()
+        self.test_asmap_interaction_with_addrman_containing_entries()
         self.test_default_asmap_with_missing_file()
         self.test_empty_asmap()
 

test/functional/rpc_net.py

@@ -257,7 +257,16 @@ class NetTest(DashTestFramework):
         assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo")
 
     def test_addpeeraddress(self):
+        """RPC addpeeraddress sets the source address equal to the destination address.
+        If an address with the same /16 as an existing new entry is passed, it will be
+        placed in the same new bucket and have a 1/64 chance of the bucket positions
+        colliding (depending on the value of nKey in the addrman), in which case the
+        new address won't be added.  The probability of collision can be reduced to
+        1/2^16 = 1/65536 by using an address from a different /16.  We avoid this here
+        by first testing adding a tried table entry before testing adding a new table one.
+        """
         self.log.info("Test addpeeraddress")
+        self.restart_node(1, self.extra_args[1] + ["-checkaddrman=1"])
         node = self.nodes[1]
 
         self.log.debug("Test that addpeerinfo is a hidden RPC")
@@ -269,17 +278,25 @@ class NetTest(DashTestFramework):
         assert_equal(node.addpeeraddress(address="", port=8333), {"success": False})
         assert_equal(node.getnodeaddresses(count=0), [])
 
-        self.log.debug("Test that adding a valid address succeeds")
+        self.log.debug("Test that adding a valid address to the tried table succeeds")
-        assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": True})
+        assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True})
-        addrs = node.getnodeaddresses(count=0)
+        with node.assert_debug_log(expected_msgs=["CheckAddrman: new 0, tried 1, total 1 started"]):
-        assert_equal(len(addrs), 1)
+            addrs = node.getnodeaddresses(count=0)  # getnodeaddresses re-runs the addrman checks
-        assert_equal(addrs[0]["address"], "1.2.3.4")
+            assert_equal(len(addrs), 1)
-        assert_equal(addrs[0]["port"], 8333)
+            assert_equal(addrs[0]["address"], "1.2.3.4")
+            assert_equal(addrs[0]["port"], 8333)
 
-        self.log.debug("Test that adding the same address again when already present fails")
+        self.log.debug("Test that adding an already-present tried address to the new and tried tables fails")
-        assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": False})
+        for value in [True, False]:
+            assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False})
         assert_equal(len(node.getnodeaddresses(count=0)), 1)
 
+        self.log.debug("Test that adding a second address, this time to the new table, succeeds")
+        assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), {"success": True})
+        with node.assert_debug_log(expected_msgs=["CheckAddrman: new 1, tried 1, total 2 started"]):
+            addrs = node.getnodeaddresses(count=0)  # getnodeaddresses re-runs the addrman checks
+            assert_equal(len(addrs), 2)
+
 
 if __name__ == '__main__':
     NetTest().main()