← Watch feed
refactor: Use span instead of vector for data in util/asmap
Public commit record
97/100 · Strong
What the developer wrote
Authored by Fabian Jahr
refactor: Use span instead of vector for data in util/asmap
This prevents holding the asmap data in memory twice.
The version hash changes due to spans being serialized without their size-prefix (unlike vectors).
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This is a code cleanup (refactor) change in Bitcoin Core. It replaces internal use of std::vector with std::span for asmap data so the same data isn't kept in memory twice, and updates the version-hash test value because the serialization format changed slightly. There is no security bug being fixed here.
Recommended action
No security action needed; treat as ordinary code-quality/performance refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit refactors util/asmap and NetGroupManager to use std::span
Changed components
src/util/asmap.cppsrc/util/asmap.hsrc/netgroup.hsrc/netgroup.cppsrc/init.cppInspect captured patch +87 / −51
diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp
index 3f900b36..d28030e4 100644
--- a/src/bench/addrman.cpp
+++ b/src/bench/addrman.cpp
@@ -24,7 +24,7 @@
static constexpr size_t NUM_SOURCES = 64;
static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
-static NetGroupManager EMPTY_NETGROUPMAN{{}};
+static auto EMPTY_NETGROUPMAN{NetGroupManager::NoAsmap()};
static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0};
static std::vector<CAddress> g_sources;
diff --git a/src/init.cpp b/src/init.cpp
index 198c07ae..6bd055f7 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1560,9 +1560,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
ApplyArgsManOptions(args, peerman_opts);
{
-
- // Read asmap file if configured
- std::vector<std::byte> asmap;
+ // Read asmap file if configured and initialize
+ // Netgroupman with or without it
+ assert(!node.netgroupman);
if (args.IsArgSet("-asmap") && !args.IsArgNegated("-asmap")) {
fs::path asmap_path = args.GetPathArg("-asmap");
if (asmap_path.empty()) {
@@ -1576,21 +1576,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
InitError(strprintf(_("Could not find asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
return false;
}
- asmap = DecodeAsmap(asmap_path);
+ std::vector<std::byte> asmap{DecodeAsmap(asmap_path)};
if (asmap.size() == 0) {
InitError(strprintf(_("Could not parse asmap file %s"), fs::quoted(fs::PathToString(asmap_path))));
return false;
}
- const uint256 asmap_version = AsmapVersion(asmap);;
+ const uint256 asmap_version = AsmapVersion(asmap);
+ node.netgroupman = std::make_unique<NetGroupManager>(NetGroupManager::WithLoadedAsmap(std::move(asmap)));
LogInfo("Using asmap version %s for IP bucketing", asmap_version.ToString());
} else {
+ node.netgroupman = std::make_unique<NetGroupManager>(NetGroupManager::NoAsmap());
LogInfo("Using /16 prefix for IP bucketing");
}
- // 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…"));
diff --git a/src/netgroup.cpp b/src/netgroup.cpp
index 5a81cd6b..9b0d4d8d 100644
--- a/src/netgroup.cpp
+++ b/src/netgroup.cpp
@@ -6,6 +6,7 @@
#include <hash.h>
#include <logging.h>
+#include <uint256.h>
#include <util/asmap.h>
#include <cstddef>
diff --git a/src/netgroup.h b/src/netgroup.h
index 7f08fef7..2482d66a 100644
--- a/src/netgroup.h
+++ b/src/netgroup.h
@@ -16,9 +16,22 @@
*/
class NetGroupManager {
public:
- explicit NetGroupManager(std::vector<std::byte>&& asmap)
- : m_asmap{std::move(asmap)}
- {}
+ NetGroupManager(const NetGroupManager&) = delete;
+ NetGroupManager(NetGroupManager&&) = default;
+ NetGroupManager& operator=(const NetGroupManager&) = delete;
+ NetGroupManager& operator=(NetGroupManager&&) = delete;
+
+ static NetGroupManager WithEmbeddedAsmap(std::span<const std::byte> asmap) {
+ return NetGroupManager(asmap, {});
+ }
+
+ static NetGroupManager WithLoadedAsmap(std::vector<std::byte>&& asmap) {
+ return NetGroupManager(std::span{asmap}, std::move(asmap));
+ }
+
+ static NetGroupManager NoAsmap() {
+ return NetGroupManager({}, {});
+ }
/** Get the asmap version, a checksum identifying the asmap being used. */
uint256 GetAsmapVersion() const;
@@ -53,7 +66,10 @@ public:
bool UsingASMap() const;
private:
- /** Compressed IP->ASN mapping, loaded from a file when a node starts.
+ /** Compressed IP->ASN mapping.
+ *
+ * Data may be loaded from a file when a node starts or embedded in the
+ * binary.
*
* 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
@@ -70,8 +86,19 @@ private:
* re-bucketed.
*
* This is initialized in the constructor, const, and therefore is
- * thread-safe. */
- const std::vector<std::byte> m_asmap;
+ * thread-safe. m_asmap can either point to m_loaded_asmap which holds
+ * data loaded from an external file at runtime or it can point to embedded
+ * asmap data.
+ */
+ const std::span<const std::byte> m_asmap;
+ std::vector<std::byte> m_loaded_asmap;
+
+ explicit NetGroupManager(std::span<const std::byte> embedded_asmap, std::vector<std::byte>&& loaded_asmap)
+ : m_asmap{embedded_asmap},
+ m_loaded_asmap{std::move(loaded_asmap)}
+ {
+ assert(m_loaded_asmap.empty() || m_asmap.data() == m_loaded_asmap.data());
+ }
};
#endif // BITCOIN_NETGROUP_H
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 9cd667ba..b70d97ee 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -24,7 +24,7 @@ using namespace std::literals;
using node::NodeContext;
using util::ToString;
-static NetGroupManager EMPTY_NETGROUPMAN{{}};
+static auto EMPTY_NETGROUPMAN{NetGroupManager::NoAsmap()};
static const bool DETERMINISTIC{true};
static int32_t GetCheckRatio(const NodeContext& node_ctx)
@@ -584,8 +584,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy)
// 101.8.0.0/16 AS8
BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket)
{
- std::vector<std::byte> asmap(test::data::asmap.begin(), test::data::asmap.end());
- NetGroupManager ngm_asmap{std::move(asmap)};
+ auto ngm_asmap{NetGroupManager::WithEmbeddedAsmap(test::data::asmap)};
CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE);
CAddress addr2 = CAddress(ResolveService("250.1.1.1", 9999), NODE_NONE);
@@ -638,8 +637,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket)
BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket)
{
- std::vector<std::byte> asmap(test::data::asmap.begin(), test::data::asmap.end());
- NetGroupManager ngm_asmap{std::move(asmap)};
+ auto ngm_asmap{NetGroupManager::WithEmbeddedAsmap(test::data::asmap)};
CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE);
CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE);
@@ -716,8 +714,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket)
BOOST_AUTO_TEST_CASE(addrman_serialization)
{
- std::vector<std::byte> asmap1(test::data::asmap.begin(), test::data::asmap.end());
- NetGroupManager netgroupman{std::move(asmap1)};
+ auto netgroupman{NetGroupManager::WithEmbeddedAsmap(test::data::asmap)};
const auto ratio = GetCheckRatio(m_node);
auto addrman_asmap1 = std::make_unique<AddrMan>(netgroupman, DETERMINISTIC, ratio);
diff --git a/src/test/fuzz/asmap.cpp b/src/test/fuzz/asmap.cpp
index 40dde196..edf78338 100644
--- a/src/test/fuzz/asmap.cpp
+++ b/src/test/fuzz/asmap.cpp
@@ -29,7 +29,7 @@ FUZZ_TARGET(asmap)
if (buffer.size() < size_t(1 + asmap_size + addr_size)) return;
std::vector<std::byte> asmap = ipv6 ? IPV6_PREFIX_ASMAP : IPV4_PREFIX_ASMAP;
std::ranges::copy(std::as_bytes(buffer.subspan(1, asmap_size)), std::back_inserter(asmap));
- if (!SanityCheckASMap(asmap, 128)) return;
+ if (!CheckStandardAsmap(asmap)) return;
const uint8_t* addr_data = buffer.data() + 1 + asmap_size;
CNetAddr net_addr;
@@ -42,6 +42,6 @@ FUZZ_TARGET(asmap)
memcpy(&ipv4, addr_data, addr_size);
net_addr.SetIP(CNetAddr{ipv4});
}
- NetGroupManager netgroupman{std::move(asmap)};
+ auto netgroupman{NetGroupManager::WithEmbeddedAsmap(asmap)};
(void)netgroupman.GetMappedAS(net_addr);
}
diff --git a/src/test/fuzz/p2p_handshake.cpp b/src/test/fuzz/p2p_handshake.cpp
index aec7387e..a56e1e5f 100644
--- a/src/test/fuzz/p2p_handshake.cpp
+++ b/src/test/fuzz/p2p_handshake.cpp
@@ -48,7 +48,7 @@ FUZZ_TARGET(p2p_handshake, .init = ::initialize)
chainman.ResetIbd();
node::Warnings warnings{};
- NetGroupManager netgroupman{{}};
+ auto netgroupman{NetGroupManager::NoAsmap()};
AddrMan addrman{netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0};
auto peerman = PeerManager::make(connman, addrman,
/*banman=*/nullptr, chainman,
diff --git a/src/test/fuzz/util/net.h b/src/test/fuzz/util/net.h
index 93aabcc3..2c169392 100644
--- a/src/test/fuzz/util/net.h
+++ b/src/test/fuzz/util/net.h
@@ -236,8 +236,10 @@ public:
[[nodiscard]] inline NetGroupManager ConsumeNetGroupManager(FuzzedDataProvider& fuzzed_data_provider) noexcept
{
std::vector<std::byte> asmap{ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider)};
- if (!SanityCheckASMap(asmap, 128)) asmap.clear();
- return NetGroupManager(std::move(asmap));
+ if (!CheckStandardAsmap(asmap)) {
+ return NetGroupManager::NoAsmap();
+ }
+ return NetGroupManager::WithLoadedAsmap(std::move(asmap));
}
inline CSubNet ConsumeSubNet(FuzzedDataProvider& fuzzed_data_provider) noexcept
diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index 37997845..24f3fd80 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -325,7 +325,7 @@ BOOST_AUTO_TEST_CASE(subnet_test)
BOOST_AUTO_TEST_CASE(netbase_getgroup)
{
- NetGroupManager netgroupman{{}}; // use /16
+ auto netgroupman{NetGroupManager::NoAsmap()}; // use /16
BOOST_CHECK(netgroupman.GetGroup(ResolveIP("127.0.0.1")) == std::vector<unsigned char>({0})); // Local -> !Routable()
BOOST_CHECK(netgroupman.GetGroup(ResolveIP("257.0.0.1")) == std::vector<unsigned char>({0})); // !Valid -> !Routable()
BOOST_CHECK(netgroupman.GetGroup(ResolveIP("10.0.0.1")) == std::vector<unsigned char>({0})); // RFC1918 -> !Routable()
@@ -631,7 +631,7 @@ BOOST_AUTO_TEST_CASE(asmap_test_vectors)
"63dc33d28f757a4a5e15d6a08"_hex};
// Construct NetGroupManager with this data.
- NetGroupManager netgroup{std::vector(ASMAP_DATA.begin(), ASMAP_DATA.end())};
+ auto netgroup{NetGroupManager::WithEmbeddedAsmap(ASMAP_DATA)};
BOOST_CHECK(netgroup.UsingASMap());
// Check some randomly-generated IPv6 addresses in it (biased towards the very beginning and
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 454f7a74..6f962a17 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -344,7 +344,7 @@ TestingSetup::TestingSetup(
if (!opts.setup_net) return;
- m_node.netgroupman = std::make_unique<NetGroupManager>(/*asmap=*/std::vector<std::byte>{});
+ m_node.netgroupman = std::make_unique<NetGroupManager>(NetGroupManager::NoAsmap());
m_node.addrman = std::make_unique<AddrMan>(*m_node.netgroupman,
/*deterministic=*/false,
m_node.args->GetIntArg("-checkaddrman", 0));
diff --git a/src/util/asmap.cpp b/src/util/asmap.cpp
index 1993103a..a40a0615 100644
--- a/src/util/asmap.cpp
+++ b/src/util/asmap.cpp
@@ -17,6 +17,7 @@
#include <cassert>
#include <cstddef>
#include <cstdio>
+#include <span>
#include <utility>
#include <vector>
@@ -38,7 +39,7 @@ inline bool ConsumeBitBE(uint8_t& bitpos, std::span<const std::byte> bytes) noex
return bit;
}
-uint32_t DecodeBits(size_t& bitpos, const std::vector<std::byte>& data, uint8_t minval, const std::vector<uint8_t>& bit_sizes)
+uint32_t DecodeBits(size_t& bitpos, const std::span<const std::byte> data, uint8_t minval, const std::span<const uint8_t> bit_sizes)
{
uint32_t val = minval;
bool bit;
@@ -71,35 +72,33 @@ enum class Instruction : uint32_t
DEFAULT = 3,
};
-const std::vector<uint8_t> TYPE_BIT_SIZES{0, 0, 1};
-Instruction DecodeType(size_t& bitpos, const std::vector<std::byte>& data)
+constexpr uint8_t TYPE_BIT_SIZES[]{0, 0, 1};
+Instruction DecodeType(size_t& bitpos, const std::span<const std::byte> data)
{
return Instruction(DecodeBits(bitpos, data, 0, TYPE_BIT_SIZES));
}
-const std::vector<uint8_t> ASN_BIT_SIZES{15, 16, 17, 18, 19, 20, 21, 22, 23, 24};
-uint32_t DecodeASN(size_t& bitpos, const std::vector<std::byte>& data)
+constexpr uint8_t ASN_BIT_SIZES[]{15, 16, 17, 18, 19, 20, 21, 22, 23, 24};
+uint32_t DecodeASN(size_t& bitpos, const std::span<const std::byte> data)
{
return DecodeBits(bitpos, data, 1, ASN_BIT_SIZES);
}
-
-const std::vector<uint8_t> MATCH_BIT_SIZES{1, 2, 3, 4, 5, 6, 7, 8};
-uint32_t DecodeMatch(size_t& bitpos, const std::vector<std::byte>& data)
+constexpr uint8_t MATCH_BIT_SIZES[]{1, 2, 3, 4, 5, 6, 7, 8};
+uint32_t DecodeMatch(size_t& bitpos, const std::span<const std::byte> data)
{
return DecodeBits(bitpos, data, 2, MATCH_BIT_SIZES);
}
-
-const std::vector<uint8_t> JUMP_BIT_SIZES{5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30};
-uint32_t DecodeJump(size_t& bitpos, const std::vector<std::byte>& data)
+constexpr uint8_t JUMP_BIT_SIZES[]{5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30};
+uint32_t DecodeJump(size_t& bitpos, const std::span<const std::byte> data)
{
return DecodeBits(bitpos, data, 17, JUMP_BIT_SIZES);
}
}
-uint32_t Interpret(const std::vector<std::byte>& asmap, const std::vector<std::byte>& ip)
+uint32_t Interpret(const std::span<const std::byte> asmap, const std::span<const std::byte> ip)
{
size_t pos{0};
const size_t endpos{asmap.size() * 8};
@@ -143,7 +142,7 @@ uint32_t Interpret(const std::vector<std::byte>& asmap, const std::vector<std::b
return 0; // 0 is not a valid ASN
}
-bool SanityCheckASMap(const std::vector<std::byte>& asmap, int bits)
+bool SanityCheckASMap(const std::span<const std::byte> asmap, int bits)
{
size_t pos{0};
const size_t endpos{asmap.size() * 8};
@@ -204,6 +203,15 @@ bool SanityCheckASMap(const std::vector<std::byte>& asmap, int bits)
return false; // Reached EOF without RETURN instruction
}
+bool CheckStandardAsmap(const std::span<const std::byte> data)
+{
+ if (!SanityCheckASMap(data, 128)) {
+ LogWarning("Sanity check of asmap data failed\n");
+ return false;
+ }
+ return true;
+}
+
std::vector<std::byte> DecodeAsmap(fs::path path)
{
FILE *filestr = fsbridge::fopen(path, "rb");
@@ -218,7 +226,7 @@ std::vector<std::byte> DecodeAsmap(fs::path path)
std::vector<std::byte> buffer(length);
file.read(buffer);
- if (!SanityCheckASMap(buffer, 128)) {
+ if (!CheckStandardAsmap(buffer)) {
LogWarning("Sanity check of asmap file %s failed", fs::quoted(fs::PathToString(path)));
return {};
}
@@ -226,7 +234,7 @@ std::vector<std::byte> DecodeAsmap(fs::path path)
return buffer;
}
-uint256 AsmapVersion(const std::vector<std::byte>& data)
+uint256 AsmapVersion(const std::span<const std::byte> data)
{
if (data.empty()) return {};
diff --git a/src/util/asmap.h b/src/util/asmap.h
index 722aeec7..1d5f6e81 100644
--- a/src/util/asmap.h
+++ b/src/util/asmap.h
@@ -10,15 +10,18 @@
#include <cstddef>
#include <cstdint>
+#include <span>
#include <vector>
-uint32_t Interpret(const std::vector<std::byte>& asmap, const std::vector<std::byte>& ip);
+uint32_t Interpret(std::span<const std::byte> asmap, std::span<const std::byte> ip);
-bool SanityCheckASMap(const std::vector<std::byte>& asmap, int bits);
+bool SanityCheckASMap(std::span<const std::byte> asmap, int bits);
+/** Check standard asmap data (128 bits for IPv6) */
+bool CheckStandardAsmap(std::span<const std::byte> data);
-/** Read asmap from provided binary file */
+/** Read and check asmap from provided binary file */
std::vector<std::byte> DecodeAsmap(fs::path path);
/** Calculate the asmap version, a checksum identifying the asmap being used. */
-uint256 AsmapVersion(const std::vector<std::byte>& data);
+uint256 AsmapVersion(std::span<const std::byte> data);
#endif // BITCOIN_UTIL_ASMAP_H
diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py
index 762f5a14..8ad59c82 100755
--- a/test/functional/feature_asmap.py
+++ b/test/functional/feature_asmap.py
@@ -18,7 +18,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal
ASMAP = 'src/test/data/asmap.raw' # path to unit test skeleton asmap
-VERSION = '6dfbc157b8a97b6e9fc7fc08d4e43d30247bbf62055eaa1098f46db9885855e3'
+VERSION = 'bafc9da308f45179443bd1d22325400ac9104f741522d003e3fac86700f68895'
def expected_messages(filename):
return [f'Opened asmap file "{filename}" (59 bytes) from disk',
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.