From 50fd77045e2f858a53486b5e02e1798c92ab946c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 24 Aug 2021 09:49:01 +0100 Subject: [PATCH 1/6] [init] Read/decode asmap before constructing addrman Commit 181a1207 introduced an initialization order bug: CAddrMan's m_asmap must be set before deserializing peers.dat. Restore that ordering. review hint: use `git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space` --- src/init.cpp | 62 ++++++++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d4a3c891b1..78025517f0 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1171,10 +1171,39 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) fDiscover = args.GetBoolArg("-discover", true); const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; - assert(!node.addrman); - auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); - node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); { + // Initialize addrman + assert(!node.addrman); + + // Read asmap file if configured + std::vector asmap; + if (args.IsArgSet("-asmap")) { + fs::path asmap_path = fs::path(args.GetArg("-asmap", "")); + if (asmap_path.empty()) { + asmap_path = DEFAULT_ASMAP_FILENAME; + } + if (!asmap_path.is_absolute()) { + asmap_path = gArgs.GetDataDirNet() / asmap_path; + } + if (!fs::exists(asmap_path)) { + InitError(strprintf(_("Could not find asmap file %s"), asmap_path)); + return false; + } + asmap = CAddrMan::DecodeAsmap(asmap_path); + if (asmap.size() == 0) { + InitError(strprintf(_("Could not parse asmap file %s"), asmap_path)); + return false; + } + const uint256 asmap_version = SerializeHash(asmap); + LogPrintf("Using asmap version %s for IP bucketing\n", asmap_version.ToString()); + } else { + LogPrintf("Using /16 prefix for IP bucketing\n"); + } + + auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); + node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); + node.addrman->m_asmap = asmap; + // Load addresses from peers.dat uiInterface.InitMessage(_("Loading P2P addresses…").translated); int64_t nStart = GetTimeMillis(); @@ -1184,10 +1213,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } else { // Addrman can be in an inconsistent state after failure, reset it node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); + node.addrman->m_asmap = asmap; LogPrintf("Recreating peers.dat\n"); adb.Write(*node.addrman); } } + assert(!node.banman); node.banman = std::make_unique(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); @@ -1292,31 +1323,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return InitError(ResolveErrMsg("externalip", strAddr)); } - // Read asmap file if configured - if (args.IsArgSet("-asmap")) { - fs::path asmap_path = fs::path(args.GetArg("-asmap", "")); - if (asmap_path.empty()) { - asmap_path = DEFAULT_ASMAP_FILENAME; - } - if (!asmap_path.is_absolute()) { - asmap_path = gArgs.GetDataDirNet() / asmap_path; - } - if (!fs::exists(asmap_path)) { - InitError(strprintf(_("Could not find asmap file %s"), asmap_path)); - return false; - } - std::vector asmap = CAddrMan::DecodeAsmap(asmap_path); - if (asmap.size() == 0) { - InitError(strprintf(_("Could not parse asmap file %s"), asmap_path)); - return false; - } - const uint256 asmap_version = SerializeHash(asmap); - node.connman->SetAsmap(std::move(asmap)); - LogPrintf("Using asmap version %s for IP bucketing\n", asmap_version.ToString()); - } else { - LogPrintf("Using /16 prefix for IP bucketing\n"); - } - #if ENABLE_ZMQ g_zmq_notification_interface = CZMQNotificationInterface::Create(); From 593247872decd6d483a76e96d79433247226ad14 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 24 Aug 2021 11:31:01 +0100 Subject: [PATCH 2/6] [net] Remove CConnMan::SetAsmap() CAddrMan::m_asmap is now set directly in AppInitMain() so CConnMan::SetAsmap() is no longer required. --- src/net.h | 2 -- src/test/fuzz/connman.cpp | 6 ------ 2 files changed, 8 deletions(-) diff --git a/src/net.h b/src/net.h index 28cd635976..316425e677 100644 --- a/src/net.h +++ b/src/net.h @@ -949,8 +949,6 @@ public: */ std::chrono::microseconds PoissonNextSendInbound(std::chrono::microseconds now, std::chrono::seconds average_interval); - void SetAsmap(std::vector asmap) { addrman.m_asmap = std::move(asmap); } - /** Return true if we should disconnect the peer for failing an inactivity check. */ bool ShouldRunInactivityChecks(const CNode& node, std::optional now=std::nullopt) const; diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 0e323ddc20..4116d5f343 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -103,12 +103,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) [&] { connman.RemoveAddedNode(random_string); }, - [&] { - const std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (SanityCheckASMap(asmap)) { - connman.SetAsmap(asmap); - } - }, [&] { connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); }, From f572f2b2048994b3b50f4cfd5de19e40b1acfb22 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 24 Aug 2021 11:27:05 +0100 Subject: [PATCH 3/6] [addrman] Set m_asmap in CAddrMan initializer list This allows us to make it const. --- src/addrman.cpp | 5 +++-- src/addrman.h | 4 ++-- src/bench/addrman.cpp | 8 ++++---- src/init.cpp | 6 ++---- src/test/addrman_tests.cpp | 13 ++++++------- src/test/fuzz/addrman.cpp | 30 +++++++++++++++++------------- src/test/fuzz/connman.cpp | 2 +- src/test/fuzz/data_stream.cpp | 2 +- src/test/fuzz/deserialize.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- 10 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index edcf97f846..03818213fe 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -77,8 +77,9 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } -CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio) - : insecure_rand{deterministic} +CAddrMan::CAddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio) + : m_asmap{std::move(asmap)} + , insecure_rand{deterministic} , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} , m_consistency_check_ratio{consistency_check_ratio} { diff --git a/src/addrman.h b/src/addrman.h index e2cb60b061..f9c12ba3f9 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -195,7 +195,7 @@ public: // // If a new asmap was provided, the existing records // would be re-bucketed accordingly. - std::vector m_asmap; + const std::vector m_asmap; // Read asmap from provided binary file static std::vector DecodeAsmap(fs::path path); @@ -471,7 +471,7 @@ public: Check(); } - explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio); + explicit CAddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio); ~CAddrMan() { diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index d69a651811..8fbb68c04c 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -73,14 +73,14 @@ static void AddrManAdd(benchmark::Bench& bench) CreateAddresses(); bench.run([&] { - CAddrMan addrman{/* deterministic */ false, /* consistency_check_ratio */ 0}; + CAddrMan addrman{/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0}; AddAddressesToAddrMan(addrman); }); } static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -92,7 +92,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -114,7 +114,7 @@ static void AddrManGood(benchmark::Bench& bench) std::vector> addrmans(addrman_count); for (size_t i{0}; i < addrman_count; ++i) { - addrmans[i] = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ 0); + addrmans[i] = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(*addrmans[i]); } diff --git a/src/init.cpp b/src/init.cpp index 78025517f0..b744298667 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1201,8 +1201,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); - node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); - node.addrman->m_asmap = asmap; + node.addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); // Load addresses from peers.dat uiInterface.InitMessage(_("Loading P2P addresses…").translated); @@ -1212,8 +1211,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("Loaded %i addresses from peers.dat %dms\n", node.addrman->size(), GetTimeMillis() - nStart); } else { // Addrman can be in an inconsistent state after failure, reset it - node.addrman = std::make_unique(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); - node.addrman->m_asmap = asmap; + node.addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); LogPrintf("Recreating peers.dat\n"); adb.Write(*node.addrman); } diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index cd5dc2370f..e1b5df9502 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -26,7 +26,7 @@ public: virtual void Serialize(CDataStream& s) const = 0; CAddrManSerializationMock() - : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 100) + : CAddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) {} }; @@ -82,10 +82,9 @@ private: public: explicit CAddrManTest(bool makeDeterministic = true, std::vector asmap = std::vector()) - : CAddrMan(makeDeterministic, /* consistency_check_ratio */ 100) + : CAddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) { deterministic = makeDeterministic; - m_asmap = asmap; } CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) @@ -1024,7 +1023,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { @@ -1041,7 +1040,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that CAddrDB::Read creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2)); BOOST_CHECK(addrman2.size() == 3); @@ -1055,7 +1054,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -1071,7 +1070,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) // Test that CAddrDB::Read fails if peers.dat is corrupt CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); } diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 95aa53bff4..bc079451b4 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -28,17 +28,11 @@ class CAddrManDeterministic : public CAddrMan public: FuzzedDataProvider& m_fuzzed_data_provider; - explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider) - : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 0) + explicit CAddrManDeterministic(std::vector asmap, FuzzedDataProvider& fuzzed_data_provider) + : CAddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0) , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); - if (fuzzed_data_provider.ConsumeBool()) { - m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (!SanityCheckASMap(m_asmap)) { - m_asmap.clear(); - } - } } /** @@ -228,7 +222,14 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - auto addr_man_ptr = std::make_unique(fuzzed_data_provider); + std::vector asmap; + if (fuzzed_data_provider.ConsumeBool()) { + asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); + if (!SanityCheckASMap(asmap)) { + asmap.clear(); + } + } + auto addr_man_ptr = std::make_unique(asmap, fuzzed_data_provider); if (fuzzed_data_provider.ConsumeBool()) { const std::vector serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)}; CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION); @@ -237,7 +238,7 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) try { ds >> *addr_man_ptr; } catch (const std::ios_base::failure&) { - addr_man_ptr = std::make_unique(fuzzed_data_provider); + addr_man_ptr = std::make_unique(asmap, fuzzed_data_provider); } } CAddrManDeterministic& addr_man = *addr_man_ptr; @@ -306,9 +307,12 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrManDeterministic addr_man1{fuzzed_data_provider}; - CAddrManDeterministic addr_man2{fuzzed_data_provider}; - addr_man2.m_asmap = addr_man1.m_asmap; + std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); + if (!SanityCheckASMap(asmap)) { + asmap.clear(); + } + CAddrManDeterministic addr_man1{asmap, fuzzed_data_provider}; + CAddrManDeterministic addr_man2{asmap, fuzzed_data_provider}; CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION); diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 4116d5f343..01741103e4 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman, fuzzed_data_provider.ConsumeBool()}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 53400082ab..8178878c30 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -21,6 +21,6 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addr_man(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); CAddrDB::Read(addr_man, data_stream); } diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 49503e8dc6..f4235a973a 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan am(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index c9bb863a7b..ba6b3e32ea 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector(/* deterministic */ false, /* consistency_check_ratio */ 0); + m_node.addrman = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, From f9002cb5dbd573cd9ca200de21319fa296e26055 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 24 Aug 2021 11:40:21 +0100 Subject: [PATCH 4/6] [net] Rename the copyStats arg from m_asmap to asmap The m_ prefix indicates that a variable is a data member. Using it as a parameter name is misleading. Also update the name of the function from copyStats to CopyStats to comply with our style guide. --- src/net.cpp | 6 +++--- src/net.h | 2 +- src/test/fuzz/net.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 57b8844d6b..65544352ee 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -567,14 +567,14 @@ Network CNode::ConnectedThroughNetwork() const #undef X #define X(name) stats.name = name -void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) +void CNode::CopyStats(CNodeStats& stats, const std::vector& asmap) { stats.nodeid = this->GetId(); X(nServices); X(addr); X(addrBind); stats.m_network = ConnectedThroughNetwork(); - stats.m_mapped_as = addr.GetMappedAS(m_asmap); + stats.m_mapped_as = addr.GetMappedAS(asmap); if (m_tx_relay != nullptr) { LOCK(m_tx_relay->cs_filter); stats.fRelayTxes = m_tx_relay->fRelayTxes; @@ -2819,7 +2819,7 @@ void CConnman::GetNodeStats(std::vector& vstats) const vstats.reserve(vNodes.size()); for (CNode* pnode : vNodes) { vstats.emplace_back(); - pnode->copyStats(vstats.back(), addrman.m_asmap); + pnode->CopyStats(vstats.back(), addrman.m_asmap); } } diff --git a/src/net.h b/src/net.h index 316425e677..2d9c29a05e 100644 --- a/src/net.h +++ b/src/net.h @@ -651,7 +651,7 @@ public: void CloseSocketDisconnect(); - void copyStats(CNodeStats &stats, const std::vector &m_asmap); + void CopyStats(CNodeStats& stats, const std::vector& asmap); ServiceFlags GetLocalServices() const { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 20d8581312..e00b5b09bf 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -46,7 +46,7 @@ FUZZ_TARGET_INIT(net, initialize_net) return; } CNodeStats stats; - node.copyStats(stats, asmap); + node.CopyStats(stats, asmap); }, [&] { const CNode* add_ref_node = node.AddRef(); From 5840476714ffebb2599999c85a23b52ebcff6090 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 24 Aug 2021 11:47:17 +0100 Subject: [PATCH 5/6] [addrman] Make m_asmap private Add a GetAsmap() getter function that returns a reference to const. --- src/addrman.cpp | 4 ++-- src/addrman.h | 34 ++++++++++++++++++---------------- src/net.cpp | 10 +++++----- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 03818213fe..67473ab2aa 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -78,10 +78,10 @@ double CAddrInfo::GetChance(int64_t nNow) const } CAddrMan::CAddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio) - : m_asmap{std::move(asmap)} - , insecure_rand{deterministic} + : insecure_rand{deterministic} , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} , m_consistency_check_ratio{consistency_check_ratio} + , m_asmap{std::move(asmap)} { for (auto& bucket : vvNew) { for (auto& entry : bucket) { diff --git a/src/addrman.h b/src/addrman.h index f9c12ba3f9..3776e478ce 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -181,22 +181,6 @@ static const int64_t ADDRMAN_TEST_WINDOW = 40*60; // 40 minutes class CAddrMan { public: - // 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 m_asmap; - // Read asmap from provided binary file static std::vector DecodeAsmap(fs::path path); @@ -593,6 +577,8 @@ public: Check(); } + const std::vector& GetAsmap() const { return m_asmap; } + private: //! A mutex to protect the inner data structures. mutable Mutex cs; @@ -660,6 +646,22 @@ 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. + // 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 m_asmap; + //! Find an entry. CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/net.cpp b/src/net.cpp index 65544352ee..f4745f1f5d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1936,7 +1936,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) case ConnectionType::BLOCK_RELAY: case ConnectionType::ADDR_FETCH: case ConnectionType::FEELER: - setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); + setConnected.insert(pnode->addr.GetGroup(addrman.GetAsmap())); } // no default case, so the compiler can warn about missing cases } } @@ -2010,7 +2010,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) m_anchors.pop_back(); if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) || !HasAllDesirableServiceFlags(addr.nServices) || - setConnected.count(addr.GetGroup(addrman.m_asmap))) continue; + setConnected.count(addr.GetGroup(addrman.GetAsmap()))) continue; addrConnect = addr; LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToString()); break; @@ -2050,7 +2050,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } // Require outbound connections, other than feelers, to be to distinct network groups - if (!fFeeler && setConnected.count(addr.GetGroup(addrman.m_asmap))) { + if (!fFeeler && setConnected.count(addr.GetGroup(addrman.GetAsmap()))) { break; } @@ -2819,7 +2819,7 @@ void CConnman::GetNodeStats(std::vector& vstats) const vstats.reserve(vNodes.size()); for (CNode* pnode : vNodes) { vstats.emplace_back(); - pnode->CopyStats(vstats.back(), addrman.m_asmap); + pnode->CopyStats(vstats.back(), addrman.GetAsmap()); } } @@ -3082,7 +3082,7 @@ CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id) const uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& ad) const { - std::vector vchNetGroup(ad.GetGroup(addrman.m_asmap)); + std::vector vchNetGroup(ad.GetGroup(addrman.GetAsmap())); return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize(); } From 724c4975622bc22cedc3f3814dfc8e66cf8371f7 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 25 Aug 2021 13:30:17 +0100 Subject: [PATCH 6/6] [fuzz] Add ConsumeAsmap() function --- src/test/fuzz/addrman.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index bc079451b4..e95126a80f 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -218,17 +218,18 @@ public: } }; +[[nodiscard]] inline std::vector ConsumeAsmap(FuzzedDataProvider& fuzzed_data_provider) noexcept +{ + std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); + if (!SanityCheckASMap(asmap)) 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 asmap; - if (fuzzed_data_provider.ConsumeBool()) { - asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (!SanityCheckASMap(asmap)) { - asmap.clear(); - } - } + std::vector asmap = ConsumeAsmap(fuzzed_data_provider); auto addr_man_ptr = std::make_unique(asmap, fuzzed_data_provider); if (fuzzed_data_provider.ConsumeBool()) { const std::vector serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)}; @@ -307,10 +308,7 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - std::vector asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (!SanityCheckASMap(asmap)) { - asmap.clear(); - } + std::vector asmap = ConsumeAsmap(fuzzed_data_provider); CAddrManDeterministic addr_man1{asmap, fuzzed_data_provider}; CAddrManDeterministic addr_man2{asmap, fuzzed_data_provider};