Merge bitcoin/bitcoin#22910: net: Encapsulate asmap in NetGroupManager

36f814c [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7 [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b22681 [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
1943156 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4 [init] Add netgroupman to node.context (John Newbery)
9b38367 [build] Add netgroup.cpp|h (John Newbery)

Pull request description:

  The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.

  This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.

ACKs for top commit:
  mzumsande:
    Code Review ACK 36f814c
  jnewbery:
    CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed bitcoin/bitcoin@36f814c, so I've reverted to that.
  dergoegge:
    Code review ACK 36f814c

Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175

Author: fanquake

src/Makefile.am

@@ -180,6 +180,7 @@ BITCOIN_CORE_H = \
   net_types.h \
   netaddress.h \
   netbase.h \
+  netgroup.h \
   netmessagemaker.h \
   node/blockstorage.h \
   node/caches.h \
@@ -352,6 +353,7 @@ libbitcoin_node_a_SOURCES = \
   init.cpp \
   mapport.cpp \
   net.cpp \
+  netgroup.cpp \
   net_processing.cpp \
   node/blockstorage.cpp \
   node/caches.cpp \

src/addrdb.cpp

@@ -13,6 +13,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.GetIntArg("-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{args.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 InvalidAddrManVersionError&) {
@@ -203,7 +204,7 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
             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>(asmap, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
+        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) {

src/addrdb.h

@@ -17,6 +17,7 @@ class ArgsManager;
 class AddrMan;
 class CAddress;
 class CDataStream;
+class NetGroupManager;
 struct bilingual_str;
 
 bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
@@ -48,7 +49,7 @@ class CBanDB
 };
 
 /** 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

@@ -43,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);
-    uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetCheapHash();
+    std::vector<unsigned char> vchSourceGroupKey = netgroupman.GetGroup(src);
+    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 +99,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 +218,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;
-    if (m_asmap.size() != 0) {
-        asmap_checksum = SerializeHash(m_asmap);
-    }
-    s << asmap_checksum;
+    s << m_netgroupman.GetAsmapChecksum();
 }
 
 template <typename Stream>
@@ -298,7 +294,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) {
@@ -335,10 +331,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;
-    if (m_asmap.size() != 0) {
-        supplied_asmap_checksum = SerializeHash(m_asmap);
-    }
+    uint256 supplied_asmap_checksum{m_netgroupman.GetAsmapChecksum()};
     uint256 serialized_asmap_checksum;
     if (format >= Format::V2_ASMAP) {
         s >> serialized_asmap_checksum;
@@ -371,7 +364,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;
@@ -495,7 +488,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)};
@@ -510,7 +503,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).
@@ -526,7 +519,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);
@@ -594,7 +587,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) {
@@ -610,7 +603,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);
@@ -650,7 +643,7 @@ bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
 
 
     // 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?
@@ -669,7 +662,7 @@ bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
         // move nId to the tried tables
         MakeTried(info, nId);
         LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n",
-                 addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos);
+                 addr.ToString(), m_netgroupman.GetMappedAS(addr), tried_bucket, tried_bucket_pos);
         return true;
     }
 }
@@ -863,7 +856,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;
@@ -929,7 +922,7 @@ 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]];
@@ -945,13 +938,13 @@ std::optional<AddressPosition> AddrManImpl::FindAddressEntry_(const CAddress& ad
     if (!addr_info) return std::nullopt;
 
     if(addr_info->fInTried) {
-        int bucket{addr_info->GetTriedBucket(nKey, m_asmap)};
+        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_asmap)};
+        int bucket{addr_info->GetNewBucket(nKey, m_netgroupman)};
         return AddressPosition(/*tried_in=*/false,
                                /*multiplicity_in=*/addr_info->nRefCount,
                                /*bucket_in=*/bucket,
@@ -1026,7 +1019,7 @@ int AddrManImpl::CheckAddrman() 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) {
@@ -1154,13 +1147,8 @@ std::optional<AddressPosition> AddrManImpl::FindAddressEntry(const CAddress& add
     return entry;
 }
 
-const std::vector<bool>& AddrManImpl::GetAsmap() const
-{
-    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(const NetGroupManager& netgroupman, bool deterministic, int32_t consistency_check_ratio)
+    : m_impl(std::make_unique<AddrManImpl>(netgroupman, deterministic, consistency_check_ratio)) {}
 
 AddrMan::~AddrMan() = default;
 
@@ -1235,11 +1223,6 @@ void AddrMan::SetServices(const CService& addr, ServiceFlags nServices)
     m_impl->SetServices(addr, nServices);
 }
 
-const std::vector<bool>& AddrMan::GetAsmap() const
-{
-    return m_impl->GetAsmap();
-}
-
 std::optional<AddressPosition> AddrMan::FindAddressEntry(const CAddress& addr)
 {
     return m_impl->FindAddressEntry(addr);

src/addrman.h

@@ -7,6 +7,7 @@
 #define BITCOIN_ADDRMAN_H
 
 #include <netaddress.h>
+#include <netgroup.h>
 #include <protocol.h>
 #include <streams.h>
 #include <timedata.h>
@@ -88,7 +89,7 @@ class AddrMan
     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();
 
@@ -172,8 +173,6 @@ class AddrMan
     //! Update an entry's service bits.
     void SetServices(const CService& addr, ServiceFlags nServices);
 
-    const std::vector<bool>& GetAsmap() const;
-
     /** Test-only function
      * Find the address record in AddrMan and return information about its
      * position.

src/addrman_impl.h

@@ -76,15 +76,15 @@ class AddrInfo : public CAddress
     }
 
     //! 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.
@@ -100,7 +100,7 @@ class AddrInfo : public CAddress
 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();
 
@@ -140,8 +140,6 @@ class AddrManImpl
     std::optional<AddressPosition> FindAddressEntry(const CAddress& addr)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    const std::vector<bool>& GetAsmap() const;
-
     friend class AddrManDeterministic;
 
 private:
@@ -212,21 +210,8 @@ class AddrManImpl
     /** 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.
-    // Should be always empty if no file was provided.
-    // 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;
+    /** Reference to the netgroup manager. netgroupman must be constructed before addrman and destructed after. */
+    const NetGroupManager& m_netgroupman;
 
     //! Find an entry.
     AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);