Merge bitcoin/bitcoin#22911: [net] Minor cleanups to asmap

853c4edb70 [net] Remove asmap argument from CNode::CopyStats() (John Newbery)
9fd5618610 [asmap] Make DecodeAsmap() a utility function (John Newbery)
bfdf4ef334 [asmap] Remove SanityCheckASMap() from netaddress (John Newbery)
07a9eccb60 [net] Remove CConnman::Options.m_asmap (John Newbery)

Pull request description:

  These small cleanups to the asmap code are the first 4 commits from #22910. They're minor improvements that are independently useful whether or not 22910 is merged.

ACKs for top commit:
  naumenkogs:
    ACK 853c4edb70
  theStack:
    Concept and code-review ACK 853c4edb70 🗺️
  fanquake:
    ACK 853c4edb70

Tree-SHA512: 64783743182592ac165df6ff8d18870b63861e9204ed722c207fca6938687aac43232a5ac4d8228cf8b92130ab0349de1b410a2467bb5a9d60dd9a7221b3b85b

src/addrman.cpp

@@ -1009,30 +1009,3 @@ CAddrInfo CAddrMan::SelectTriedCollision_()
 
     return mapInfo[id_old];
 }
-
-std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
-{
-    std::vector<bool> bits;
-    FILE *filestr = fsbridge::fopen(path, "rb");
-    CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
-    if (file.IsNull()) {
-        LogPrintf("Failed to open asmap file from disk\n");
-        return bits;
-    }
-    fseek(filestr, 0, SEEK_END);
-    int length = ftell(filestr);
-    LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length);
-    fseek(filestr, 0, SEEK_SET);
-    uint8_t cur_byte;
-    for (int i = 0; i < length; ++i) {
-        file >> cur_byte;
-        for (int bit = 0; bit < 8; ++bit) {
-            bits.push_back((cur_byte >> bit) & 1);
-        }
-    }
-    if (!SanityCheckASMap(bits)) {
-        LogPrintf("Sanity check of asmap file %s failed\n", path);
-        return {};
-    }
-    return bits;
-}

src/addrman.h

@@ -142,9 +142,6 @@ static constexpr int ADDRMAN_BUCKET_SIZE{1 << ADDRMAN_BUCKET_SIZE_LOG2};
 class CAddrMan
 {
 public:
-    // Read asmap from provided binary file
-    static std::vector<bool> DecodeAsmap(fs::path path);
-
     template <typename Stream>
     void Serialize(Stream& s_) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
 

src/init.cpp

@@ -1189,7 +1189,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
                 InitError(strprintf(_("Could not find asmap file %s"), asmap_path));
                 return false;
             }
-            asmap = CAddrMan::DecodeAsmap(asmap_path);
+            asmap = DecodeAsmap(asmap_path);
             if (asmap.size() == 0) {
                 InitError(strprintf(_("Could not parse asmap file %s"), asmap_path));
                 return false;

src/net.cpp

@@ -554,14 +554,13 @@ Network CNode::ConnectedThroughNetwork() const
 
 #undef X
 #define X(name) stats.name = name
-void CNode::CopyStats(CNodeStats& stats, const std::vector<bool>& asmap)
+void CNode::CopyStats(CNodeStats& stats)
 {
     stats.nodeid = this->GetId();
     X(nServices);
     X(addr);
     X(addrBind);
     stats.m_network = ConnectedThroughNetwork();
-    stats.m_mapped_as = addr.GetMappedAS(asmap);
     if (m_tx_relay != nullptr) {
         LOCK(m_tx_relay->cs_filter);
         stats.fRelayTxes = m_tx_relay->fRelayTxes;
@@ -2805,7 +2804,8 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) const
     vstats.reserve(vNodes.size());
     for (CNode* pnode : vNodes) {
         vstats.emplace_back();
-        pnode->CopyStats(vstats.back(), addrman.GetAsmap());
+        pnode->CopyStats(vstats.back());
+        vstats.back().m_mapped_as = pnode->addr.GetMappedAS(addrman.GetAsmap());
     }
 }
 

src/net.h

@@ -651,7 +651,7 @@ public:
 
     void CloseSocketDisconnect();
 
-    void CopyStats(CNodeStats& stats, const std::vector<bool>& asmap);
+    void CopyStats(CNodeStats& stats);
 
     ServiceFlags GetLocalServices() const
     {
@@ -767,7 +767,6 @@ public:
         bool m_use_addrman_outgoing = true;
         std::vector<std::string> m_specified_outgoing;
         std::vector<std::string> m_added_nodes;
-        std::vector<bool> m_asmap;
         bool m_i2p_accept_incoming;
     };
 

src/netaddress.cpp

@@ -1242,8 +1242,3 @@ bool operator<(const CSubNet& a, const CSubNet& b)
 {
     return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16) < 0));
 }
-
-bool SanityCheckASMap(const std::vector<bool>& asmap)
-{
-    return SanityCheckASMap(asmap, 128); // For IP address lookups, the input is 128 bits
-}

src/netaddress.h

@@ -567,6 +567,4 @@ public:
     }
 };
 
-bool SanityCheckASMap(const std::vector<bool>& asmap);
-
 #endif // BITCOIN_NETADDRESS_H

src/test/fuzz/addrman.cpp

@@ -221,7 +221,7 @@ public:
 [[nodiscard]] inline std::vector<bool> ConsumeAsmap(FuzzedDataProvider& fuzzed_data_provider) noexcept
 {
     std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-    if (!SanityCheckASMap(asmap)) asmap.clear();
+    if (!SanityCheckASMap(asmap, 128)) asmap.clear();
     return asmap;
 }
 

src/test/fuzz/asmap.cpp

@@ -4,6 +4,7 @@
 
 #include <netaddress.h>
 #include <test/fuzz/fuzz.h>
+#include <util/asmap.h>
 
 #include <cstdint>
 #include <vector>
@@ -42,7 +43,7 @@ FUZZ_TARGET(asmap)
             asmap.push_back((buffer[1 + i] >> j) & 1);
         }
     }
-    if (!SanityCheckASMap(asmap)) return;
+    if (!SanityCheckASMap(asmap, 128)) return;
 
     const uint8_t* addr_data = buffer.data() + 1 + asmap_size;
     CNetAddr net_addr;

src/test/fuzz/net.cpp

@@ -14,6 +14,7 @@
 #include <test/fuzz/util.h>
 #include <test/util/net.h>
 #include <test/util/setup_common.h>
+#include <util/asmap.h>
 
 #include <cstdint>
 #include <optional>
@@ -38,12 +39,8 @@ FUZZ_TARGET_INIT(net, initialize_net)
                 node.CloseSocketDisconnect();
             },
             [&] {
-                const std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
-                if (!SanityCheckASMap(asmap)) {
-                    return;
-                }
                 CNodeStats stats;
-                node.CopyStats(stats, asmap);
+                node.CopyStats(stats);
             },
             [&] {
                 const CNode* add_ref_node = node.AddRef();

src/util/asmap.cpp

@@ -2,10 +2,16 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <util/asmap.h>
+
+#include <clientversion.h>
+#include <crypto/common.h>
+#include <logging.h>
+#include <streams.h>
+
+#include <cassert>
 #include <map>
 #include <vector>
-#include <assert.h>
-#include <crypto/common.h>
 
 namespace {
 
@@ -183,3 +189,31 @@ bool SanityCheckASMap(const std::vector<bool>& asmap, int bits)
     }
     return false; // Reached EOF without RETURN instruction
 }
+
+std::vector<bool> DecodeAsmap(fs::path path)
+{
+    std::vector<bool> bits;
+    FILE *filestr = fsbridge::fopen(path, "rb");
+    CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
+    if (file.IsNull()) {
+        LogPrintf("Failed to open asmap file from disk\n");
+        return bits;
+    }
+    fseek(filestr, 0, SEEK_END);
+    int length = ftell(filestr);
+    LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length);
+    fseek(filestr, 0, SEEK_SET);
+    uint8_t cur_byte;
+    for (int i = 0; i < length; ++i) {
+        file >> cur_byte;
+        for (int bit = 0; bit < 8; ++bit) {
+            bits.push_back((cur_byte >> bit) & 1);
+        }
+    }
+    if (!SanityCheckASMap(bits, 128)) {
+        LogPrintf("Sanity check of asmap file %s failed\n", path);
+        return {};
+    }
+    return bits;
+}
+

src/util/asmap.h

@@ -5,11 +5,16 @@
 #ifndef BITCOIN_UTIL_ASMAP_H
 #define BITCOIN_UTIL_ASMAP_H
 
-#include <stdint.h>
+#include <fs.h>
+
+#include <cstdint>
 #include <vector>
 
 uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip);
 
 bool SanityCheckASMap(const std::vector<bool>& asmap, int bits);
 
+/** Read asmap from provided binary file */
+std::vector<bool> DecodeAsmap(fs::path path);
+
 #endif // BITCOIN_UTIL_ASMAP_H