From c9a645f8144640d8a8e3e15eef95de68953f89bf Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 1 Jun 2024 15:23:37 +0000 Subject: [PATCH 01/12] merge bitcoin#22879: Fix format string in deserialize error --- src/addrman.cpp | 6 +-- test/functional/feature_addrman.py | 87 ++++++++++++++++++++++++++++++ test/functional/feature_anchors.py | 3 -- test/functional/test_runner.py | 1 + 4 files changed, 91 insertions(+), 6 deletions(-) create mode 100755 test/functional/feature_addrman.py diff --git a/src/addrman.cpp b/src/addrman.cpp index 75aebb065c..b298ad6f28 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -249,9 +249,9 @@ void CAddrMan::Unserialize(Stream& s_) const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE; if (lowest_compatible > FILE_FORMAT) { throw std::ios_base::failure(strprintf( - "Unsupported format of addrman database: %u. It is compatible with formats >=%u, " - "but the maximum supported by this version of %s is %u.", - format, lowest_compatible, PACKAGE_NAME, static_cast(FILE_FORMAT))); + "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})); } s >> nKey; diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py new file mode 100755 index 0000000000..8ccff340f0 --- /dev/null +++ b/test/functional/feature_addrman.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +# 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. +"""Test addrman functionality""" + +import os +import struct + +from test_framework.messages import ser_uint256, hash256 +from test_framework.p2p import MAGIC_BYTES +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + + +def serialize_addrman(*, format=1, lowest_compatible=3): + new = [] + tried = [] + INCOMPATIBILITY_BASE = 32 + r = MAGIC_BYTES["regtest"] + r += struct.pack("B", format) + r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible) + r += ser_uint256(1) + r += struct.pack("i", len(new)) + r += struct.pack("i", len(tried)) + ADDRMAN_NEW_BUCKET_COUNT = 1 << 10 + r += struct.pack("i", ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30)) + for _ in range(ADDRMAN_NEW_BUCKET_COUNT): + r += struct.pack("i", 0) + checksum = hash256(r) + r += checksum + return r + + +def write_addrman(peers_dat, **kwargs): + with open(peers_dat, "wb") as f: + f.write(serialize_addrman(**kwargs)) + + +class AddrmanTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + + def run_test(self): + peers_dat = os.path.join(self.nodes[0].datadir, self.chain, "peers.dat") + + self.log.info("Check that mocked addrman is valid") + self.stop_node(0) + write_addrman(peers_dat) + with self.nodes[0].assert_debug_log(["Loaded 0 addresses from peers.dat"]): + 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.stop_node(0) + write_addrman(peers_dat, lowest_compatible=111) + with self.nodes[0].assert_debug_log([ + f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.', + "Recreating peers.dat", + ]): + self.start_node(0) + assert_equal(self.nodes[0].getnodeaddresses(), []) + + self.log.info("Check that corrupt addrman cannot be read") + self.stop_node(0) + with open(peers_dat, "wb") as f: + f.write(serialize_addrman()[:-1]) + with self.nodes[0].assert_debug_log([ + "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file", + "Recreating peers.dat", + ]): + self.start_node(0) + assert_equal(self.nodes[0].getnodeaddresses(), []) + + self.log.info("Check that missing addrman is recreated") + self.stop_node(0) + os.remove(peers_dat) + with self.nodes[0].assert_debug_log([ + f"Missing or invalid file {peers_dat}", + "Recreating peers.dat", + ]): + self.start_node(0) + assert_equal(self.nodes[0].getnodeaddresses(), []) + + +if __name__ == "__main__": + AddrmanTest().main() diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py index 24bb02bc90..1c04ba5ff4 100755 --- a/test/functional/feature_anchors.py +++ b/test/functional/feature_anchors.py @@ -24,9 +24,6 @@ class AnchorsTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 - def setup_network(self): - self.setup_nodes() - def run_test(self): node_anchors_path = os.path.join( self.nodes[0].datadir, "regtest", "anchors.dat" diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 85d9c9a8f9..2ba8755a55 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -315,6 +315,7 @@ BASE_SCRIPTS = [ 'p2p_add_connections.py', 'p2p_blockfilters.py', 'p2p_message_capture.py', + 'feature_addrman.py', 'feature_asmap.py', 'feature_includeconf.py', 'mempool_unbroadcast.py', From 8aefa9b93ab65413368d04cbc75a9a64dc767a91 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 1 Jun 2024 12:26:14 +0000 Subject: [PATCH 02/12] merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted --- src/addrdb.cpp | 86 ++++++++++++++++++------------ src/addrdb.h | 8 ++- src/init.cpp | 14 +---- src/test/addrman_tests.cpp | 4 +- src/test/fuzz/data_stream.cpp | 5 +- test/functional/feature_addrman.py | 34 +++++++----- 6 files changed, 86 insertions(+), 65 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 8d38bef80d..48b1f7f008 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -17,10 +17,17 @@ #include #include #include +#include #include namespace { + +class DbNotFoundError : public std::exception +{ + using std::exception::exception; +}; + template bool SerializeDB(Stream& stream, const Data& data) { @@ -78,47 +85,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data } template -bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) +void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) { - try { - CHashVerifier verifier(&stream); - // de-serialize file header (network specific magic number) and .. - unsigned char pchMsgTmp[4]; - verifier >> pchMsgTmp; - // ... verify the network matches ours - if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) - return error("%s: Invalid network magic number", __func__); + CHashVerifier verifier(&stream); + // de-serialize file header (network specific magic number) and .. + unsigned char pchMsgTmp[4]; + verifier >> pchMsgTmp; + // ... verify the network matches ours + if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) { + throw std::runtime_error{"Invalid network magic number"}; + } - // de-serialize data - verifier >> data; + // de-serialize data + verifier >> data; - // verify checksum - if (fCheckSum) { - uint256 hashTmp; - stream >> hashTmp; - if (hashTmp != verifier.GetHash()) { - return error("%s: Checksum mismatch, data corrupted", __func__); - } + // verify checksum + if (fCheckSum) { + uint256 hashTmp; + stream >> hashTmp; + if (hashTmp != verifier.GetHash()) { + throw std::runtime_error{"Checksum mismatch, data corrupted"}; } } - catch (const std::exception& e) { - return error("%s: Deserialize or I/O error - %s", __func__, e.what()); - } - - return true; } template -bool DeserializeFileDB(const fs::path& path, Data& data, int version) +void DeserializeFileDB(const fs::path& path, Data& data, int version) { // open input file, and associate with CAutoFile FILE* file = fsbridge::fopen(path, "rb"); CAutoFile filein(file, SER_DISK, version); if (filein.IsNull()) { - LogPrintf("Missing or invalid file %s\n", path.string()); - return false; + throw DbNotFoundError{}; } - return DeserializeDB(filein, data); + DeserializeDB(filein, data); } } // namespace @@ -177,15 +177,32 @@ bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr) return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION); } -bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr) +void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers) { - const auto pathAddr = GetDataDir() / "peers.dat"; - return DeserializeFileDB(pathAddr, addr, CLIENT_VERSION); + DeserializeDB(ssPeers, addr, false); } -bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers) +std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman) { - return DeserializeDB(ssPeers, addr, false); + auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); + addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + + int64_t nStart = GetTimeMillis(); + const auto path_addr{GetDataDir() / "peers.dat"}; + try { + DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION); + 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(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + LogPrintf("Creating peers.dat because the file was not found (%s)\n", 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."), + e.what(), PACKAGE_BUGREPORT, path_addr); + } + return std::nullopt; } void DumpAnchors(const fs::path& anchors_db_path, const std::vector& anchors) @@ -197,9 +214,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector& a std::vector ReadAnchors(const fs::path& anchors_db_path) { std::vector anchors; - if (DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT)) { + try { + DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT); LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename()); - } else { + } catch (const std::exception&) { anchors.clear(); } diff --git a/src/addrdb.h b/src/addrdb.h index c31c126ee3..33cc1f9204 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -10,17 +10,18 @@ #include // For banmap_t #include +#include #include class ArgsManager; class CAddrMan; class CAddress; class CDataStream; +struct bilingual_str; bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr); -bool ReadPeerAddresses(const ArgsManager& args, CAddrMan& addr); /** Only used by tests. */ -bool ReadFromStream(CAddrMan& addr, CDataStream& ssPeers); +void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers); /** Access to the banlist database (banlist.json) */ class CBanDB @@ -46,6 +47,9 @@ public: bool Read(banmap_t& banSet); }; +/** Returns an error string on failure */ +std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman); + /** * Dump the anchor IP address database (anchors.dat) * diff --git a/src/init.cpp b/src/init.cpp index 1045f67413..04442f2db2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1693,19 +1693,9 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc 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(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); - - // Load addresses from peers.dat uiInterface.InitMessage(_("Loading P2P addresses…").translated); - int64_t nStart = GetTimeMillis(); - if (ReadPeerAddresses(args, *node.addrman)) { - 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(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); - LogPrintf("Recreating peers.dat\n"); - DumpPeerAddresses(args, *node.addrman); + if (const auto error{LoadAddrman(asmap, args, node.addrman)}) { + return InitError(*error); } } diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 7986981ec2..90d96c6cf0 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -1042,7 +1042,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); - BOOST_CHECK(ReadFromStream(addrman2, ssPeers2)); + ReadFromStream(addrman2, ssPeers2); BOOST_CHECK(addrman2.size() == 3); } @@ -1072,7 +1072,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); - BOOST_CHECK(!ReadFromStream(addrman2, ssPeers2)); + BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index bda4b8ada9..f5adb45ec3 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -23,5 +23,8 @@ 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(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); - ReadFromStream(addr_man, data_stream); + try { + ReadFromStream(addr_man, data_stream); + } catch (const std::exception&) { + } } diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 8ccff340f0..ee421c89b5 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -10,6 +10,7 @@ import struct from test_framework.messages import ser_uint256, hash256 from test_framework.p2p import MAGIC_BYTES from test_framework.test_framework import BitcoinTestFramework +from test_framework.test_node import ErrorMatch from test_framework.util import assert_equal @@ -43,6 +44,12 @@ class AddrmanTest(BitcoinTestFramework): def run_test(self): peers_dat = os.path.join(self.nodes[0].datadir, self.chain, "peers.dat") + init_error = lambda reason: ( + f"Error: Invalid or corrupt peers.dat \\({reason}\\). If you believe this " + f"is a bug, please report it to {self.config['environment']['PACKAGE_BUGREPORT']}. " + f'As a workaround, you can move the file \\("{peers_dat}"\\) out of the way \\(rename, ' + "move, or delete\\) to have a new one created on the next start." + ) self.log.info("Check that mocked addrman is valid") self.stop_node(0) @@ -54,30 +61,29 @@ class AddrmanTest(BitcoinTestFramework): self.log.info("Check that addrman from future cannot be read") self.stop_node(0) write_addrman(peers_dat, lowest_compatible=111) - with self.nodes[0].assert_debug_log([ - f'ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 1. It is compatible with formats >=111, but the maximum supported by this version of {self.config["environment"]["PACKAGE_NAME"]} is 3.', - "Recreating peers.dat", - ]): - self.start_node(0) - assert_equal(self.nodes[0].getnodeaddresses(), []) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error( + "Unsupported format of addrman database: 1. It is compatible with " + "formats >=111, but the maximum supported by this version of " + f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)" + ), + match=ErrorMatch.FULL_REGEX, + ) self.log.info("Check that corrupt addrman cannot be read") self.stop_node(0) with open(peers_dat, "wb") as f: f.write(serialize_addrman()[:-1]) - with self.nodes[0].assert_debug_log([ - "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file", - "Recreating peers.dat", - ]): - self.start_node(0) - assert_equal(self.nodes[0].getnodeaddresses(), []) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("CAutoFile::read: end of file.*"), + match=ErrorMatch.FULL_REGEX, + ) self.log.info("Check that missing addrman is recreated") self.stop_node(0) os.remove(peers_dat) with self.nodes[0].assert_debug_log([ - f"Missing or invalid file {peers_dat}", - "Recreating peers.dat", + f'Creating peers.dat because the file was not found ("{peers_dat}")', ]): self.start_node(0) assert_equal(self.nodes[0].getnodeaddresses(), []) From 2420ac9e5391569bbaa288e0a7c705950ee8815d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 1 Jun 2024 18:59:24 +0000 Subject: [PATCH 03/12] merge bitcoin#23041: Add addrman deserialization error tests --- test/functional/feature_addrman.py | 52 ++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index ee421c89b5..daa0f1e4e4 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -14,22 +14,30 @@ from test_framework.test_node import ErrorMatch from test_framework.util import assert_equal -def serialize_addrman(*, format=1, lowest_compatible=3): +def serialize_addrman( + *, + format=1, + lowest_compatible=3, + net_magic="regtest", + len_new=None, + len_tried=None, + mock_checksum=None, +): new = [] tried = [] INCOMPATIBILITY_BASE = 32 - r = MAGIC_BYTES["regtest"] + r = MAGIC_BYTES[net_magic] r += struct.pack("B", format) r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible) r += ser_uint256(1) - r += struct.pack("i", len(new)) - r += struct.pack("i", len(tried)) + r += struct.pack("i", len_new or len(new)) + r += struct.pack("i", len_tried or len(tried)) ADDRMAN_NEW_BUCKET_COUNT = 1 << 10 r += struct.pack("i", ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30)) for _ in range(ADDRMAN_NEW_BUCKET_COUNT): r += struct.pack("i", 0) checksum = hash256(r) - r += checksum + r += mock_checksum or checksum return r @@ -70,7 +78,7 @@ class AddrmanTest(BitcoinTestFramework): match=ErrorMatch.FULL_REGEX, ) - self.log.info("Check that corrupt addrman cannot be read") + self.log.info("Check that corrupt addrman cannot be read (EOF)") self.stop_node(0) with open(peers_dat, "wb") as f: f.write(serialize_addrman()[:-1]) @@ -79,6 +87,38 @@ class AddrmanTest(BitcoinTestFramework): match=ErrorMatch.FULL_REGEX, ) + self.log.info("Check that corrupt addrman cannot be read (magic)") + self.stop_node(0) + write_addrman(peers_dat, net_magic="devnet") + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("Invalid network magic number"), + match=ErrorMatch.FULL_REGEX, + ) + + self.log.info("Check that corrupt addrman cannot be read (checksum)") + self.stop_node(0) + write_addrman(peers_dat, mock_checksum=b"ab" * 32) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("Checksum mismatch, data corrupted"), + match=ErrorMatch.FULL_REGEX, + ) + + self.log.info("Check that corrupt addrman cannot be read (len_tried)") + self.stop_node(0) + write_addrman(peers_dat, len_tried=-1) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("Corrupt CAddrMan serialization: nTried=-1, should be in \\[0, 16384\\]:.*"), + match=ErrorMatch.FULL_REGEX, + ) + + self.log.info("Check that corrupt addrman cannot be read (len_new)") + self.stop_node(0) + write_addrman(peers_dat, len_new=-1) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("Corrupt CAddrMan serialization: nNew=-1, should be in \\[0, 65536\\]:.*"), + match=ErrorMatch.FULL_REGEX, + ) + self.log.info("Check that missing addrman is recreated") self.stop_node(0) os.remove(peers_dat) From 236cf36d881c31f93e2118275723c506a4d91e5b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 18 Aug 2021 09:07:18 +0200 Subject: [PATCH 04/12] merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize --- src/Makefile.test.include | 1 - src/addrman.cpp | 27 ++++++++++++++++++++++----- src/addrman.h | 18 +++++------------- src/test/fuzz/addrman.cpp | 11 +++++++++++ src/test/fuzz/data_stream.cpp | 30 ------------------------------ test/functional/feature_addrman.py | 11 ++++++++++- 6 files changed, 48 insertions(+), 50 deletions(-) delete mode 100644 src/test/fuzz/data_stream.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 3f71e6112f..82cf080233 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -263,7 +263,6 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/crypto_hkdf_hmac_sha256_l32.cpp \ test/fuzz/crypto_poly1305.cpp \ test/fuzz/cuckoocache.cpp \ - test/fuzz/data_stream.cpp \ test/fuzz/decode_tx.cpp \ test/fuzz/descriptor_parse.cpp \ test/fuzz/deserialize.cpp \ diff --git a/src/addrman.cpp b/src/addrman.cpp index b298ad6f28..7e231a43e4 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -389,7 +389,12 @@ void CAddrMan::Unserialize(Stream& s_) LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost); } - Check(); + const int check_code{ForceCheckAddrman()}; + if (check_code != 0) { + throw std::ios_base::failure(strprintf( + "Corrupt data. Consistency check failed with code %s", + check_code)); + } } // explicit instantiation @@ -764,14 +769,26 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const } } -int CAddrMan::Check_() const +void CAddrMan::Check() const { AssertLockHeld(cs); - LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size()); // Run consistency checks 1 in m_consistency_check_ratio times if enabled - if (m_consistency_check_ratio == 0) return 0; - if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return 0; + if (m_consistency_check_ratio == 0) return; + if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return; + + const int err{ForceCheckAddrman()}; + if (err) { + LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); + assert(false); + } +} + +int CAddrMan::ForceCheckAddrman() const +{ + AssertLockHeld(cs); + + LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size()); std::unordered_set setTried; std::unordered_map mapNew; diff --git a/src/addrman.h b/src/addrman.h index 695e69daec..595274bebe 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -406,20 +406,12 @@ private: //! Return a random to-be-evicted tried table address. CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); - //! Consistency check - void Check() const EXCLUSIVE_LOCKS_REQUIRED(cs) - { - AssertLockHeld(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); - const int err = Check_(); - if (err) { - LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); - assert(false); - } - } - - //! Perform consistency check. Returns an error code or zero. - int 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); /** * Return all or many randomly selected addresses, optionally by network. diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 30c022d36e..f772397e9d 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -23,6 +23,17 @@ void initialize_addrman() SelectParams(CBaseChainParams::REGTEST); } +FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); + CAddrMan addr_man(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + try { + ReadFromStream(addr_man, data_stream); + } catch (const std::exception&) { + } +} + class CAddrManDeterministic : public CAddrMan { public: diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp deleted file mode 100644 index f5adb45ec3..0000000000 --- a/src/test/fuzz/data_stream.cpp +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) 2020 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 -#include -#include -#include -#include -#include -#include - -#include -#include - -void initialize_data_stream_addr_man() -{ - static const auto testing_setup = MakeNoLogFileContext<>(); -} - -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(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); - try { - ReadFromStream(addr_man, data_stream); - } catch (const std::exception&) { - } -} diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index daa0f1e4e4..9638cbfc36 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -19,6 +19,7 @@ def serialize_addrman( format=1, lowest_compatible=3, net_magic="regtest", + bucket_key=1, len_new=None, len_tried=None, mock_checksum=None, @@ -29,7 +30,7 @@ def serialize_addrman( r = MAGIC_BYTES[net_magic] r += struct.pack("B", format) r += struct.pack("B", INCOMPATIBILITY_BASE + lowest_compatible) - r += ser_uint256(1) + r += ser_uint256(bucket_key) r += struct.pack("i", len_new or len(new)) r += struct.pack("i", len_tried or len(tried)) ADDRMAN_NEW_BUCKET_COUNT = 1 << 10 @@ -119,6 +120,14 @@ class AddrmanTest(BitcoinTestFramework): match=ErrorMatch.FULL_REGEX, ) + self.log.info("Check that corrupt addrman cannot be read (failed check)") + self.stop_node(0) + write_addrman(peers_dat, bucket_key=0) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error("Corrupt data. Consistency check failed with code -16: .*"), + match=ErrorMatch.FULL_REGEX, + ) + self.log.info("Check that missing addrman is recreated") self.stop_node(0) os.remove(peers_dat) From b6ec8ab6df8836f50f429d1057bed54cc82d6b0b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 3 Jun 2024 14:16:57 +0000 Subject: [PATCH 05/12] merge bitcoin#22950: Pimpl AddrMan to abstract implementation details --- src/Makefile.am | 1 + src/addrdb.cpp | 10 +- src/addrdb.h | 8 +- src/addrman.cpp | 652 +++++++++++++++++++---------- src/addrman.h | 397 ++---------------- src/addrman_impl.h | 278 ++++++++++++ src/bench/addrman.cpp | 22 +- src/net.cpp | 15 +- src/net.h | 4 +- src/net_processing.cpp | 10 +- src/net_processing.h | 4 +- src/netaddress.cpp | 2 +- src/netaddress.h | 2 +- src/node/context.h | 4 +- src/test/addrman_tests.cpp | 211 +++++----- src/test/fuzz/addrman.cpp | 75 ++-- src/test/fuzz/connman.cpp | 2 +- src/test/fuzz/deserialize.cpp | 5 +- src/test/fuzz/net.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- test/functional/feature_addrman.py | 4 +- 21 files changed, 936 insertions(+), 774 deletions(-) create mode 100644 src/addrman_impl.h diff --git a/src/Makefile.am b/src/Makefile.am index 859cc0ace7..c89f3591cb 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -133,6 +133,7 @@ BITCOIN_CORE_H = \ addressindex.h \ spentindex.h \ addrman.h \ + addrman_impl.h \ attributes.h \ banman.h \ base58.h \ diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 48b1f7f008..940f03193c 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -171,21 +171,21 @@ bool CBanDB::Read(banmap_t& banSet) return true; } -bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr) +bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr) { const auto pathAddr = GetDataDir() / "peers.dat"; return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION); } -void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers) +void ReadFromStream(AddrMan& addr, CDataStream& ssPeers) { DeserializeDB(ssPeers, addr, false); } -std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman) +std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman) { auto check_addrman = std::clamp(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); - addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); int64_t nStart = GetTimeMillis(); const auto path_addr{GetDataDir() / "peers.dat"}; @@ -194,7 +194,7 @@ std::optional LoadAddrman(const std::vector& 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(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + addrman = std::make_unique(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr); DumpPeerAddresses(args, *addrman); } catch (const std::exception& e) { diff --git a/src/addrdb.h b/src/addrdb.h index 33cc1f9204..19be4b5bb4 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -14,14 +14,14 @@ #include class ArgsManager; -class CAddrMan; +class AddrMan; class CAddress; class CDataStream; struct bilingual_str; -bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr); +bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr); /** Only used by tests. */ -void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers); +void ReadFromStream(AddrMan& addr, CDataStream& ssPeers); /** Access to the banlist database (banlist.json) */ class CBanDB @@ -48,7 +48,7 @@ public: }; /** Returns an error string on failure */ -std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman); +std::optional LoadAddrman(const std::vector& asmap, const ArgsManager& args, std::unique_ptr& addrman); /** * Dump the anchor IP address database (anchors.dat) diff --git a/src/addrman.cpp b/src/addrman.cpp index 7e231a43e4..b9e38ae522 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -4,25 +4,27 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include -#include #include -#include #include +#include +#include #include #include +#include +#include +#include #include #include #include -#include -#include /** Over how many buckets entries with tried addresses from a single group (/16 for IPv4) are spread */ static constexpr uint32_t ADDRMAN_TRIED_BUCKETS_PER_GROUP{8}; /** Over how many buckets entries with new addresses originating from a single group are spread */ static constexpr uint32_t ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP{64}; -/** Maximum number of times an address can be added to the new table */ +/** Maximum number of times an address can occur in the new table */ static constexpr int32_t ADDRMAN_NEW_BUCKETS_PER_ADDRESS{8}; /** How old addresses can maximally be */ static constexpr int64_t ADDRMAN_HORIZON_DAYS{30}; @@ -39,7 +41,7 @@ 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 CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector &asmap) const +int AddrInfo::GetTriedBucket(const uint256& nKey, const std::vector& asmap) 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(); @@ -49,7 +51,7 @@ int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector &asma return tried_bucket; } -int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std::vector &asmap) const +int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std::vector& asmap) const { std::vector vchSourceGroupKey = src.GetGroup(asmap); uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetCheapHash(); @@ -60,13 +62,13 @@ int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std: return new_bucket; } -int CAddrInfo::GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const +int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int nBucket) const { uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? uint8_t{'N'} : uint8_t{'K'}) << nBucket << GetKey()).GetCheapHash(); return hash1 % ADDRMAN_BUCKET_SIZE; } -bool CAddrInfo::IsTerrible(int64_t nNow) const +bool AddrInfo::IsTerrible(int64_t nNow) const { if (nNow - nLastTry <= 60) { // never remove things tried in the last minute return false; @@ -88,7 +90,7 @@ bool CAddrInfo::IsTerrible(int64_t nNow) const return false; } -double CAddrInfo::GetChance(int64_t nNow) const +double AddrInfo::GetChance(int64_t nNow) const { double fChance = 1.0; int64_t nSinceLastTry = std::max(nNow - nLastTry, 0); @@ -103,12 +105,12 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } -CAddrMan::CAddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts) +AddrManImpl::AddrManImpl(std::vector&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports) : insecure_rand{deterministic} , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} , m_consistency_check_ratio{consistency_check_ratio} , m_asmap{std::move(asmap)} - , discriminatePorts{_discriminatePorts} + , m_discriminate_ports{discriminate_ports} { for (auto& bucket : vvNew) { for (auto& entry : bucket) { @@ -122,8 +124,13 @@ CAddrMan::CAddrMan(std::vector asmap, bool deterministic, int32_t consiste } } +AddrManImpl::~AddrManImpl() +{ + nKey.SetNull(); +} + template -void CAddrMan::Serialize(Stream& s_) const +void AddrManImpl::Serialize(Stream& s_) const { LOCK(cs); @@ -186,7 +193,7 @@ void CAddrMan::Serialize(Stream& s_) const int nIds = 0; for (const auto& entry : mapInfo) { mapUnkIds[entry.first] = nIds; - const CAddrInfo &info = entry.second; + const AddrInfo& info = entry.second; if (info.nRefCount) { assert(nIds != nNew); // this means nNew was wrong, oh ow s << info; @@ -195,7 +202,7 @@ void CAddrMan::Serialize(Stream& s_) const } nIds = 0; for (const auto& entry : mapInfo) { - const CAddrInfo &info = entry.second; + const AddrInfo& info = entry.second; if (info.fInTried) { assert(nIds != nTried); // this means nTried was wrong, oh ow s << info; @@ -226,7 +233,7 @@ void CAddrMan::Serialize(Stream& s_) const } template -void CAddrMan::Unserialize(Stream& s_) +void AddrManImpl::Unserialize(Stream& s_) { LOCK(cs); @@ -265,21 +272,21 @@ void CAddrMan::Unserialize(Stream& s_) if (nNew > ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nNew < 0) { throw std::ios_base::failure( - strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %d]", + strprintf("Corrupt AddrMan serialization: nNew=%d, should be in [0, %d]", nNew, ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE)); } if (nTried > ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nTried < 0) { throw std::ios_base::failure( - strprintf("Corrupt CAddrMan serialization: nTried=%d, should be in [0, %d]", + strprintf("Corrupt AddrMan serialization: nTried=%d, should be in [0, %d]", nTried, ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE)); } // Deserialize entries from the new table. for (int n = 0; n < nNew; n++) { - CAddrInfo &info = mapInfo[n]; + AddrInfo& info = mapInfo[n]; s >> info; mapAddr[info] = n; info.nRandomPos = vRandom.size(); @@ -290,7 +297,7 @@ void CAddrMan::Unserialize(Stream& s_) // Deserialize entries from the tried table. int nLost = 0; for (int n = 0; n < nTried; n++) { - CAddrInfo info; + AddrInfo info; s >> info; int nKBucket = info.GetTriedBucket(nKey, m_asmap); int nKBucketPos = info.GetBucketPosition(nKey, false, nKBucket); @@ -347,7 +354,7 @@ void CAddrMan::Unserialize(Stream& s_) for (auto bucket_entry : bucket_entries) { int bucket{bucket_entry.first}; const int entry_index{bucket_entry.second}; - CAddrInfo& info = mapInfo[entry_index]; + AddrInfo& info = mapInfo[entry_index]; // Don't store the entry in the new bucket if it's not a valid address for our addrman if (!info.IsValid()) continue; @@ -397,19 +404,10 @@ void CAddrMan::Unserialize(Stream& s_) } } -// explicit instantiation -template void CAddrMan::Serialize(CHashWriter& s) const; -template void CAddrMan::Serialize(CAutoFile& s) const; -template void CAddrMan::Serialize(CDataStream& s) const; -template void CAddrMan::Unserialize(CAutoFile& s); -template void CAddrMan::Unserialize(CHashVerifier& s); -template void CAddrMan::Unserialize(CDataStream& s); -template void CAddrMan::Unserialize(CHashVerifier& s); - -CAddrInfo* CAddrMan::Find(const CService& addr, int* pnId) +AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId) { CService addr2 = addr; - if (!discriminatePorts) { + if (!m_discriminate_ports) { addr2.SetPort(0); } @@ -424,16 +422,16 @@ CAddrInfo* CAddrMan::Find(const CService& addr, int* pnId) return nullptr; } -CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId) +AddrInfo* AddrManImpl::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId) { CService addr2 = addr; - if (!discriminatePorts) { + if (!m_discriminate_ports) { addr2.SetPort(0); } AssertLockHeld(cs); int nId = nIdCount++; - mapInfo[nId] = CAddrInfo(addr, addrSource); + mapInfo[nId] = AddrInfo(addr, addrSource); mapAddr[addr2] = nId; mapInfo[nId].nRandomPos = vRandom.size(); vRandom.push_back(nId); @@ -442,7 +440,7 @@ CAddrInfo* CAddrMan::Create(const CAddress& addr, const CNetAddr& addrSource, in return &mapInfo[nId]; } -void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const +void AddrManImpl::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const { AssertLockHeld(cs); @@ -466,17 +464,17 @@ void CAddrMan::SwapRandom(unsigned int nRndPos1, unsigned int nRndPos2) const vRandom[nRndPos2] = nId1; } -void CAddrMan::Delete(int nId) +void AddrManImpl::Delete(int nId) { AssertLockHeld(cs); assert(mapInfo.count(nId) != 0); - CAddrInfo& info = mapInfo[nId]; + AddrInfo& info = mapInfo[nId]; assert(!info.fInTried); assert(info.nRefCount == 0); CService addr = info; - if (!discriminatePorts) { + if (!m_discriminate_ports) { addr.SetPort(0); } @@ -487,14 +485,14 @@ void CAddrMan::Delete(int nId) nNew--; } -void CAddrMan::ClearNew(int nUBucket, int nUBucketPos) +void AddrManImpl::ClearNew(int nUBucket, int nUBucketPos) { AssertLockHeld(cs); // if there is an entry in the specified bucket, delete it. if (vvNew[nUBucket][nUBucketPos] != -1) { int nIdDelete = vvNew[nUBucket][nUBucketPos]; - CAddrInfo& infoDelete = mapInfo[nIdDelete]; + AddrInfo& infoDelete = mapInfo[nIdDelete]; assert(infoDelete.nRefCount > 0); infoDelete.nRefCount--; vvNew[nUBucket][nUBucketPos] = -1; @@ -504,7 +502,7 @@ void CAddrMan::ClearNew(int nUBucket, int nUBucketPos) } } -void CAddrMan::MakeTried(CAddrInfo& info, int nId) +void AddrManImpl::MakeTried(AddrInfo& info, int nId) { AssertLockHeld(cs); @@ -532,7 +530,7 @@ void CAddrMan::MakeTried(CAddrInfo& info, int nId) // find an item to evict int nIdEvict = vvTried[nKBucket][nKBucketPos]; assert(mapInfo.count(nIdEvict) == 1); - CAddrInfo& infoOld = mapInfo[nIdEvict]; + AddrInfo& infoOld = mapInfo[nIdEvict]; // Remove the to-be-evicted item from the tried set. infoOld.fInTried = false; @@ -557,7 +555,7 @@ void CAddrMan::MakeTried(CAddrInfo& info, int nId) info.fInTried = true; } -void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime) +void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) { AssertLockHeld(cs); @@ -565,13 +563,13 @@ void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime nLastGood = nTime; - CAddrInfo* pinfo = Find(addr, &nId); + AddrInfo* pinfo = Find(addr, &nId); // if not found, bail out if (!pinfo) return; - CAddrInfo& info = *pinfo; + AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) if (info != addr) @@ -617,7 +615,7 @@ void CAddrMan::Good_(const CService& addr, bool test_before_evict, int64_t nTime } } -bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) +bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) { AssertLockHeld(cs); @@ -626,7 +624,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP bool fNew = false; int nId; - CAddrInfo* pinfo = Find(addr, &nId); + AddrInfo* pinfo = Find(addr, &nId); // Do not set a penalty for a source's self-announcement if (addr == source) { @@ -675,7 +673,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP if (vvNew[nUBucket][nUBucketPos] != nId) { bool fInsert = vvNew[nUBucket][nUBucketPos] == -1; if (!fInsert) { - CAddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; + AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) { // Overwrite the existing new table entry. fInsert = true; @@ -694,17 +692,17 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP return fNew; } -void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) +void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) { AssertLockHeld(cs); - CAddrInfo* pinfo = Find(addr); + AddrInfo* pinfo = Find(addr); // if not found, bail out if (!pinfo) return; - CAddrInfo& info = *pinfo; + AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) if (info != addr) @@ -718,15 +716,13 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) } } -CAddrInfo CAddrMan::Select_(bool newOnly) const +std::pair AddrManImpl::Select_(bool newOnly) const { AssertLockHeld(cs); - if (vRandom.empty()) - return CAddrInfo(); + if (vRandom.empty()) return {}; - if (newOnly && nNew == 0) - return CAddrInfo(); + if (newOnly && nNew == 0) return {}; // Use a 50% chance for choosing between tried and new table entries. if (!newOnly && @@ -743,9 +739,10 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const int nId = vvTried[nKBucket][nKBucketPos]; const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); - const CAddrInfo& info{it_found->second}; - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) - return info; + const AddrInfo& info{it_found->second}; + if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + return {info, info.nLastTry}; + } fChanceFactor *= 1.2; } } else { @@ -761,15 +758,204 @@ CAddrInfo CAddrMan::Select_(bool newOnly) const int nId = vvNew[nUBucket][nUBucketPos]; const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); - const CAddrInfo& info{it_found->second}; - if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) - return info; + const AddrInfo& info{it_found->second}; + if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + return {info, info.nLastTry}; + } fChanceFactor *= 1.2; } } } -void CAddrMan::Check() const +std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const +{ + AssertLockHeld(cs); + + size_t nNodes = vRandom.size(); + if (max_pct != 0) { + nNodes = max_pct * nNodes / 100; + } + if (max_addresses != 0) { + nNodes = std::min(nNodes, max_addresses); + } + + // gather a list of random nodes, skipping those of low quality + const int64_t now{GetAdjustedTime()}; + std::vector addresses; + for (unsigned int n = 0; n < vRandom.size(); n++) { + if (addresses.size() >= nNodes) + break; + + int nRndPos = insecure_rand.randrange(vRandom.size() - n) + n; + SwapRandom(n, nRndPos); + const auto it{mapInfo.find(vRandom[n])}; + assert(it != mapInfo.end()); + + const AddrInfo& ai{it->second}; + + // Filter by network (optional) + if (network != std::nullopt && ai.GetNetClass() != network) continue; + + // Filter for quality + if (ai.IsTerrible(now)) continue; + + addresses.push_back(ai); + } + + return addresses; +} + +void AddrManImpl::Connected_(const CService& addr, int64_t nTime) +{ + AssertLockHeld(cs); + + AddrInfo* pinfo = Find(addr); + + // if not found, bail out + if (!pinfo) + return; + + AddrInfo& info = *pinfo; + + // check whether we are talking about the exact same CService (including same port) + if (info != addr) + return; + + // update info + int64_t nUpdateInterval = 20 * 60; + if (nTime - info.nTime > nUpdateInterval) + info.nTime = nTime; +} + +void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices) +{ + AssertLockHeld(cs); + + AddrInfo* pinfo = Find(addr); + + // if not found, bail out + if (!pinfo) + return; + + AddrInfo& info = *pinfo; + + // check whether we are talking about the exact same CService (including same port) + if (info != addr) + return; + + // update info + info.nServices = nServices; +} + +AddrInfo AddrManImpl::GetAddressInfo_(const CService& addr) +{ + AddrInfo* pinfo = Find(addr); + + // if not found, bail out + if (!pinfo) + return AddrInfo(); + + AddrInfo& info = *pinfo; + + // check whether we are talking about the exact same CService (including same port) + if (info != addr) + return AddrInfo(); + + return *pinfo; +} + +void AddrManImpl::ResolveCollisions_() +{ + AssertLockHeld(cs); + + for (std::set::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) { + int id_new = *it; + + bool erase_collision = false; + + // If id_new not found in mapInfo remove it from m_tried_collisions + if (mapInfo.count(id_new) != 1) { + erase_collision = true; + } else { + 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_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; + } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty + + // Get the to-be-evicted address that is being tested + int id_old = vvTried[tried_bucket][tried_bucket_pos]; + AddrInfo& info_old = mapInfo[id_old]; + + const auto current_time{GetAdjustedTime()}; + + // Has successfully connected in last X hours + if (current_time - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { + erase_collision = true; + } else if (current_time - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { // attempted to connect and failed in last X hours + + // Give address at least 60 seconds to successfully connect + if (current_time - info_old.nLastTry > 60) { + LogPrint(BCLog::ADDRMAN, "Replacing %s with %s in tried table\n", info_old.ToString(), info_new.ToString()); + + // Replaces an existing address already in the tried table with the new address + Good_(info_new, false, current_time); + erase_collision = true; + } + } else if (current_time - info_new.nLastSuccess > ADDRMAN_TEST_WINDOW) { + // If the collision hasn't resolved in some reasonable amount of time, + // just evict the old entry -- we must not be able to + // connect to it for some reason. + LogPrint(BCLog::ADDRMAN, "Unable to test; replacing %s with %s in tried table anyway\n", info_old.ToString(), info_new.ToString()); + Good_(info_new, false, current_time); + erase_collision = true; + } + } else { // Collision is not actually a collision anymore + Good_(info_new, false, GetAdjustedTime()); + erase_collision = true; + } + } + + if (erase_collision) { + m_tried_collisions.erase(it++); + } else { + it++; + } + } +} + +std::pair AddrManImpl::SelectTriedCollision_() +{ + AssertLockHeld(cs); + + if (m_tried_collisions.size() == 0) return {}; + + std::set::iterator it = m_tried_collisions.begin(); + + // Selects a random element from m_tried_collisions + std::advance(it, insecure_rand.randrange(m_tried_collisions.size())); + int id_new = *it; + + // If id_new not found in mapInfo remove it from m_tried_collisions + if (mapInfo.count(id_new) != 1) { + m_tried_collisions.erase(it); + return {}; + } + + 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_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket); + + const AddrInfo& info_old = mapInfo[vvTried[tried_bucket][tried_bucket_pos]]; + return {info_old, info_old.nLastTry}; +} + +void AddrManImpl::Check() const { AssertLockHeld(cs); @@ -784,7 +970,7 @@ void CAddrMan::Check() const } } -int CAddrMan::ForceCheckAddrman() const +int AddrManImpl::ForceCheckAddrman() const { AssertLockHeld(cs); @@ -798,7 +984,7 @@ int CAddrMan::ForceCheckAddrman() const for (const auto& entry : mapInfo) { int n = entry.first; - const CAddrInfo& info = entry.second; + const AddrInfo& info = entry.second; if (info.fInTried) { if (!info.nLastSuccess) return -1; @@ -872,188 +1058,192 @@ int CAddrMan::ForceCheckAddrman() const return 0; } -void CAddrMan::GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional network) const +size_t AddrManImpl::size() const { - AssertLockHeld(cs); + LOCK(cs); // TODO: Cache this in an atomic to avoid this overhead + return vRandom.size(); +} - size_t nNodes = vRandom.size(); - if (max_pct != 0) { - nNodes = max_pct * nNodes / 100; +bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) +{ + LOCK(cs); + int nAdd = 0; + Check(); + for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) + nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0; + Check(); + if (nAdd) { + LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew); } - if (max_addresses != 0) { - nNodes = std::min(nNodes, max_addresses); - } - - // gather a list of random nodes, skipping those of low quality - const int64_t now{GetAdjustedTime()}; - for (unsigned int n = 0; n < vRandom.size(); n++) { - if (vAddr.size() >= nNodes) - break; - - int nRndPos = insecure_rand.randrange(vRandom.size() - n) + n; - SwapRandom(n, nRndPos); - const auto it{mapInfo.find(vRandom[n])}; - assert(it != mapInfo.end()); - - const CAddrInfo& ai{it->second}; - - // Filter by network (optional) - if (network != std::nullopt && ai.GetNetClass() != network) continue; - - // Filter for quality - if (ai.IsTerrible(now)) continue; - - vAddr.push_back(ai); + return nAdd > 0; +} + +void AddrManImpl::Good(const CService& addr, int64_t nTime) +{ + LOCK(cs); + Check(); + Good_(addr, /* test_before_evict */ true, nTime); + Check(); +} + +void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) +{ + LOCK(cs); + Check(); + Attempt_(addr, fCountFailure, nTime); + Check(); +} + +void AddrManImpl::ResolveCollisions() +{ + LOCK(cs); + Check(); + ResolveCollisions_(); + Check(); +} + +std::pair AddrManImpl::SelectTriedCollision() +{ + LOCK(cs); + Check(); + const auto ret = SelectTriedCollision_(); + Check(); + return ret; +} + +std::pair AddrManImpl::Select(bool newOnly) const +{ + LOCK(cs); + Check(); + const auto addrRet = Select_(newOnly); + Check(); + return addrRet; +} + +std::vector AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const +{ + LOCK(cs); + Check(); + const auto addresses = GetAddr_(max_addresses, max_pct, network); + Check(); + return addresses; +} + +void AddrManImpl::Connected(const CService& addr, int64_t nTime) +{ + LOCK(cs); + Check(); + Connected_(addr, nTime); + Check(); +} + +void AddrManImpl::SetServices(const CService& addr, ServiceFlags nServices) +{ + LOCK(cs); + Check(); + SetServices_(addr, nServices); + Check(); +} + +AddrInfo AddrManImpl::GetAddressInfo(const CService& addr) +{ + AddrInfo addrRet; + { + LOCK(cs); + Check(); + addrRet = GetAddressInfo_(addr); + Check(); } + return addrRet; } -void CAddrMan::Connected_(const CService& addr, int64_t nTime) +const std::vector& AddrManImpl::GetAsmap() const { - AssertLockHeld(cs); - - CAddrInfo* pinfo = Find(addr); - - // if not found, bail out - if (!pinfo) - return; - - CAddrInfo& info = *pinfo; - - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - - // update info - int64_t nUpdateInterval = 20 * 60; - if (nTime - info.nTime > nUpdateInterval) - info.nTime = nTime; + return m_asmap; } -void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices) +AddrMan::AddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports) + : m_impl(std::make_unique(std::move(asmap), deterministic, consistency_check_ratio, discriminate_ports)) {} + +AddrMan::~AddrMan() = default; + +template +void AddrMan::Serialize(Stream& s_) const { - AssertLockHeld(cs); - - CAddrInfo* pinfo = Find(addr); - - // if not found, bail out - if (!pinfo) - return; - - CAddrInfo& info = *pinfo; - - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return; - - // update info - info.nServices = nServices; + m_impl->Serialize(s_); } -CAddrInfo CAddrMan::GetAddressInfo_(const CService& addr) +template +void AddrMan::Unserialize(Stream& s_) { - CAddrInfo* pinfo = Find(addr); - - // if not found, bail out - if (!pinfo) - return CAddrInfo(); - - CAddrInfo& info = *pinfo; - - // check whether we are talking about the exact same CService (including same port) - if (info != addr) - return CAddrInfo(); - - return *pinfo; + m_impl->Unserialize(s_); } -void CAddrMan::ResolveCollisions_() +// explicit instantiation +template void AddrMan::Serialize(CHashWriter& s) const; +template void AddrMan::Serialize(CAutoFile& s) const; +template void AddrMan::Serialize(CDataStream& s) const; +template void AddrMan::Unserialize(CAutoFile& s); +template void AddrMan::Unserialize(CHashVerifier& s); +template void AddrMan::Unserialize(CDataStream& s); +template void AddrMan::Unserialize(CHashVerifier& s); + +size_t AddrMan::size() const { - AssertLockHeld(cs); - - for (std::set::iterator it = m_tried_collisions.begin(); it != m_tried_collisions.end();) { - int id_new = *it; - - bool erase_collision = false; - - // If id_new not found in mapInfo remove it from m_tried_collisions - if (mapInfo.count(id_new) != 1) { - erase_collision = true; - } else { - CAddrInfo& 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_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; - } else if (vvTried[tried_bucket][tried_bucket_pos] != -1) { // The position in the tried bucket is not empty - - // Get the to-be-evicted address that is being tested - int id_old = vvTried[tried_bucket][tried_bucket_pos]; - CAddrInfo& info_old = mapInfo[id_old]; - - const auto current_time{GetAdjustedTime()}; - - // Has successfully connected in last X hours - if (current_time - info_old.nLastSuccess < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { - erase_collision = true; - } else if (current_time - info_old.nLastTry < ADDRMAN_REPLACEMENT_HOURS*(60*60)) { // attempted to connect and failed in last X hours - - // Give address at least 60 seconds to successfully connect - if (current_time - info_old.nLastTry > 60) { - LogPrint(BCLog::ADDRMAN, "Replacing %s with %s in tried table\n", info_old.ToString(), info_new.ToString()); - - // Replaces an existing address already in the tried table with the new address - Good_(info_new, false, current_time); - erase_collision = true; - } - } else if (current_time - info_new.nLastSuccess > ADDRMAN_TEST_WINDOW) { - // If the collision hasn't resolved in some reasonable amount of time, - // just evict the old entry -- we must not be able to - // connect to it for some reason. - LogPrint(BCLog::ADDRMAN, "Unable to test; replacing %s with %s in tried table anyway\n", info_old.ToString(), info_new.ToString()); - Good_(info_new, false, current_time); - erase_collision = true; - } - } else { // Collision is not actually a collision anymore - Good_(info_new, false, GetAdjustedTime()); - erase_collision = true; - } - } - - if (erase_collision) { - m_tried_collisions.erase(it++); - } else { - it++; - } - } + return m_impl->size(); } -CAddrInfo CAddrMan::SelectTriedCollision_() +bool AddrMan::Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) { - AssertLockHeld(cs); - - if (m_tried_collisions.size() == 0) return CAddrInfo(); - - std::set::iterator it = m_tried_collisions.begin(); - - // Selects a random element from m_tried_collisions - std::advance(it, insecure_rand.randrange(m_tried_collisions.size())); - int id_new = *it; - - // If id_new not found in mapInfo remove it from m_tried_collisions - if (mapInfo.count(id_new) != 1) { - m_tried_collisions.erase(it); - return CAddrInfo(); - } - - const CAddrInfo& newInfo = mapInfo[id_new]; - - // which tried bucket to move the entry to - int tried_bucket = newInfo.GetTriedBucket(nKey, m_asmap); - int tried_bucket_pos = newInfo.GetBucketPosition(nKey, false, tried_bucket); - - int id_old = vvTried[tried_bucket][tried_bucket_pos]; - - return mapInfo[id_old]; + return m_impl->Add(vAddr, source, nTimePenalty); +} + +void AddrMan::Good(const CService& addr, int64_t nTime) +{ + m_impl->Good(addr, nTime); +} + +void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) +{ + m_impl->Attempt(addr, fCountFailure, nTime); +} + +void AddrMan::ResolveCollisions() +{ + m_impl->ResolveCollisions(); +} + +std::pair AddrMan::SelectTriedCollision() +{ + return m_impl->SelectTriedCollision(); +} + +std::pair AddrMan::Select(bool newOnly) const +{ + return m_impl->Select(newOnly); +} + +std::vector AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const +{ + return m_impl->GetAddr(max_addresses, max_pct, network); +} + +void AddrMan::Connected(const CService& addr, int64_t nTime) +{ + m_impl->Connected(addr, nTime); +} + +void AddrMan::SetServices(const CService& addr, ServiceFlags nServices) +{ + m_impl->SetServices(addr, nServices); +} + +AddrInfo AddrMan::GetAddressInfo(const CService& addr) +{ + return m_impl->GetAddressInfo(addr); +} + +const std::vector& AddrMan::GetAsmap() const +{ + return m_impl->GetAsmap(); } diff --git a/src/addrman.h b/src/addrman.h index 595274bebe..49ea77cdff 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -6,94 +6,23 @@ #ifndef BITCOIN_ADDRMAN_H #define BITCOIN_ADDRMAN_H -#include -#include #include #include -#include +#include #include #include +#include #include -#include -#include +#include #include +class AddrInfo; +class AddrManImpl; + /** Default for -checkaddrman */ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; -/** - * Extended statistics about a CAddress - */ -class CAddrInfo : public CAddress -{ -public: - //! last try whatsoever by us (memory only) - int64_t nLastTry{0}; - - //! last counted attempt (memory only) - int64_t nLastCountAttempt{0}; - -private: - //! where knowledge about this address first came from - CNetAddr source; - - //! last successful connection by us - int64_t nLastSuccess{0}; - - //! connection attempts since last successful attempt - int nAttempts{0}; - - //! reference count in new sets (memory only) - int nRefCount{0}; - - //! in tried set? (memory only) - bool fInTried{false}; - - //! position in vRandom - mutable int nRandomPos{-1}; - - friend class CAddrMan; - friend class CAddrManDeterministic; - -public: - - SERIALIZE_METHODS(CAddrInfo, obj) - { - READWRITEAS(CAddress, obj); - READWRITE(obj.source, obj.nLastSuccess, obj.nAttempts); - } - - CAddrInfo(const CAddress &addrIn, const CNetAddr &addrSource) : CAddress(addrIn), source(addrSource) - { - } - - CAddrInfo() : CAddress(), source() - { - } - - //! Calculate in which "tried" bucket this entry belongs - int GetTriedBucket(const uint256 &nKey, const std::vector &asmap) const; - - //! Calculate in which "new" bucket this entry belongs, given a certain source - int GetNewBucket(const uint256 &nKey, const CNetAddr& src, const std::vector &asmap) const; - - //! Calculate in which "new" bucket this entry belongs, using its default source - int GetNewBucket(const uint256 &nKey, const std::vector &asmap) const - { - return GetNewBucket(nKey, source, asmap); - } - - //! Calculate in which position of a bucket to store this entry. - int GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const; - - //! Determine whether the statistics about this entry are bad enough so that it can just be deleted - bool IsTerrible(int64_t nNow = GetAdjustedTime()) const; - - //! Calculate the relative chance this entry should be given when selecting nodes to connect to - double GetChance(int64_t nNow = GetAdjustedTime()) const; -}; - /** Stochastic address manager * * Design goals: @@ -123,115 +52,53 @@ public: * * Several indexes are kept for high performance. Setting m_consistency_check_ratio with the -checkaddrman * configuration option will introduce (expensive) consistency checks for the entire data structure. */ - -/** Total number of buckets for tried addresses */ -static constexpr int32_t ADDRMAN_TRIED_BUCKET_COUNT_LOG2{8}; -static constexpr int ADDRMAN_TRIED_BUCKET_COUNT{1 << ADDRMAN_TRIED_BUCKET_COUNT_LOG2}; - -/** Total number of buckets for new addresses */ -static constexpr int32_t ADDRMAN_NEW_BUCKET_COUNT_LOG2{10}; -static constexpr int ADDRMAN_NEW_BUCKET_COUNT{1 << ADDRMAN_NEW_BUCKET_COUNT_LOG2}; - -/** Maximum allowed number of entries in buckets for new and tried addresses */ -static constexpr int32_t ADDRMAN_BUCKET_SIZE_LOG2{6}; -static constexpr int ADDRMAN_BUCKET_SIZE{1 << ADDRMAN_BUCKET_SIZE_LOG2}; - -/** - * Stochastical (IP) address manager - */ -class CAddrMan +class AddrMan { + const std::unique_ptr m_impl; + public: - template - void Serialize(Stream& s_) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + explicit AddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports = false); + + ~AddrMan(); template - void Unserialize(Stream& s_) EXCLUSIVE_LOCKS_REQUIRED(!cs); + void Serialize(Stream& s_) const; - explicit CAddrMan(std::vector asmap, bool deterministic, int32_t consistency_check_ratio, bool _discriminatePorts = false); - - ~CAddrMan() - { - nKey.SetNull(); - } + template + void Unserialize(Stream& s_); //! Return the number of (unique) addresses in all tables. - size_t size() const - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); // TODO: Cache this in an atomic to avoid this overhead - return vRandom.size(); - } + size_t size() const; //! Add addresses to addrman's new table. - bool Add(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty = 0) - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - int nAdd = 0; - Check(); - for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) - nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0; - Check(); - if (nAdd) { - LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew); - } - return nAdd > 0; - } + bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0); - //! Mark an entry as accessible. - void Good(const CService &addr, int64_t nTime = GetAdjustedTime()) - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - Good_(addr, /* test_before_evict */ true, nTime); - Check(); - } + //! Mark an entry as accessible, possibly moving it from "new" to "tried". + void 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()) - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - Attempt_(addr, fCountFailure, nTime); - Check(); - } + void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime()); //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions. - void ResolveCollisions() - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - ResolveCollisions_(); - Check(); - } + void ResolveCollisions(); - //! Randomly select an address in tried that another address is attempting to evict. - CAddrInfo SelectTriedCollision() - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - const CAddrInfo ret = SelectTriedCollision_(); - Check(); - return ret; - } + /** + * Randomly select an address in the tried table that another address is + * attempting to evict. + * + * @return CAddress The record for the selected tried peer. + * int64_t The last time we attempted to connect to that peer. + */ + std::pair SelectTriedCollision(); /** * Choose an address to connect to. + * + * @param[in] newOnly Whether to only select addresses from the new table. + * @return CAddress The record for the selected peer. + * int64_t The last time we attempted to connect to that peer. */ - CAddrInfo Select(bool newOnly = false) const - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - const CAddrInfo addrRet = Select_(newOnly); - Check(); - return addrRet; - } + std::pair Select(bool newOnly = false) const; /** * Return all or many randomly selected addresses, optionally by network. @@ -239,189 +106,10 @@ public: * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). * @param[in] network Select only addresses of this network (nullopt = all). - */ - std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - std::vector vAddr; - GetAddr_(vAddr, max_addresses, max_pct, network); - Check(); - return vAddr; - } - - //! Outer function for Connected_() - void Connected(const CService &addr, int64_t nTime = GetAdjustedTime()) - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - Connected_(addr, nTime); - Check(); - } - - void SetServices(const CService &addr, ServiceFlags nServices) - EXCLUSIVE_LOCKS_REQUIRED(!cs) - { - LOCK(cs); - Check(); - SetServices_(addr, nServices); - Check(); - } - - const std::vector& GetAsmap() const { return m_asmap; } - - CAddrInfo GetAddressInfo(const CService& addr) - { - CAddrInfo addrRet; - { - LOCK(cs); - Check(); - addrRet = GetAddressInfo_(addr); - Check(); - } - return addrRet; - } - -private: - //! A mutex to protect the inner data structures. - mutable Mutex cs; - - //! Source of random numbers for randomization in inner loops - mutable FastRandomContext insecure_rand GUARDED_BY(cs); - - //! secret key to randomize bucket select with - uint256 nKey; - - //! Serialization versions. - enum Format : uint8_t { - V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 - V1_DETERMINISTIC = 1, //!< for pre-asmap files - V2_ASMAP = 2, //!< for files including asmap version - V3_BIP155 = 3, //!< same as V2_ASMAP plus addresses are in BIP155 format - }; - - //! The maximum format this software knows it can unserialize. Also, we always serialize - //! in this format. - //! The format (first byte in the serialized stream) can be higher than this and - //! still this software may be able to unserialize the file - if the second byte - //! (see `lowest_compatible` in `Unserialize()`) is less or equal to this. - static constexpr Format FILE_FORMAT = Format::V3_BIP155; - - //! The initial value of a field that is incremented every time an incompatible format - //! change is made (such that old software versions would not be able to parse and - //! understand the new file format). This is 32 because we overtook the "key size" - //! field which was 32 historically. - //! @note Don't increment this. Increment `lowest_compatible` in `Serialize()` instead. - static constexpr uint8_t INCOMPATIBILITY_BASE = 32; - - //! last used nId - int nIdCount GUARDED_BY(cs){0}; - - //! table with information about all nIds - std::unordered_map mapInfo GUARDED_BY(cs); - - //! find an nId based on its network address - std::unordered_map mapAddr GUARDED_BY(cs); - - //! randomly-ordered vector of all nIds - //! This is mutable because it is unobservable outside the class, so any - //! changes to it (even in const methods) are also unobservable. - mutable std::vector vRandom GUARDED_BY(cs); - - // number of "tried" entries - int nTried GUARDED_BY(cs){0}; - - //! list of "tried" buckets - int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); - - //! number of (unique) "new" entries - int nNew GUARDED_BY(cs){0}; - - //! list of "new" buckets - int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); - - //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse. - int64_t nLastGood GUARDED_BY(cs){1}; - - //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions. - std::set m_tried_collisions; - - /** 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; - - // discriminate entries based on port. Should be false on mainnet/testnet and can be true on devnet/regtest - bool discriminatePorts GUARDED_BY(cs); - - //! Find an entry. - CAddrInfo* Find(const CService& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom. - CAddrInfo* Create(const CAddress &addr, const CNetAddr &addrSource, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Swap two elements in vRandom. - void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) const EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Move an entry from the "new" table(s) to the "tried" table - void MakeTried(CAddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Delete an entry. It must not be in tried, and have refcount 0. - void Delete(int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Clear a position in a "new" table. This is the only place where entries are actually deleted. - void ClearNew(int nUBucket, int nUBucketPos) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Mark an entry "good", possibly moving it from "new" to "tried". - void Good_(const CService &addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Add an entry to the "new" table. - bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Mark an entry as attempted to connect. - void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Select an address to connect to, if newOnly is set to true, only the new table is selected from. - CAddrInfo Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions. - void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs); - - //! Return a random to-be-evicted tried table address. - CAddrInfo SelectTriedCollision_() 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); - - /** - * Return all or many randomly selected addresses, optionally by network. * - * @param[out] vAddr Vector of randomly selected addresses from vRandom. - * @param[in] max_addresses Maximum number of addresses to return (0 = all). - * @param[in] max_pct Maximum percentage of addresses to return (0 = all). - * @param[in] network Select only addresses of this network (nullopt = all). + * @return A vector of randomly selected addresses from vRandom. */ - void GetAddr_(std::vector& vAddr, size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const; /** We have successfully connected to this peer. Calling this function * updates the CAddress's nTime, which is used in our IsTerrible() @@ -434,17 +122,18 @@ private: * @param[in] addr The address of the peer we were connected to * @param[in] nTime The time that we were last connected to this peer */ - void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); + void Connected(const CService& addr, int64_t nTime = GetAdjustedTime()); //! Update an entry's service bits. - void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); + void SetServices(const CService& addr, ServiceFlags nServices); - //! Get address info for address - CAddrInfo GetAddressInfo_(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(cs); + //! 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& GetAsmap() const; - friend class CAddrManTest; - friend class CAddrManDeterministic; + friend class AddrManTest; + friend class AddrManDeterministic; }; #endif // BITCOIN_ADDRMAN_H diff --git a/src/addrman_impl.h b/src/addrman_impl.h new file mode 100644 index 0000000000..4b1d23d416 --- /dev/null +++ b/src/addrman_impl.h @@ -0,0 +1,278 @@ +// 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_ADDRMAN_IMPL_H +#define BITCOIN_ADDRMAN_IMPL_H + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +/** Total number of buckets for tried addresses */ +static constexpr int32_t ADDRMAN_TRIED_BUCKET_COUNT_LOG2{8}; +static constexpr int ADDRMAN_TRIED_BUCKET_COUNT{1 << ADDRMAN_TRIED_BUCKET_COUNT_LOG2}; +/** Total number of buckets for new addresses */ +static constexpr int32_t ADDRMAN_NEW_BUCKET_COUNT_LOG2{10}; +static constexpr int ADDRMAN_NEW_BUCKET_COUNT{1 << ADDRMAN_NEW_BUCKET_COUNT_LOG2}; +/** Maximum allowed number of entries in buckets for new and tried addresses */ +static constexpr int32_t ADDRMAN_BUCKET_SIZE_LOG2{6}; +static constexpr int ADDRMAN_BUCKET_SIZE{1 << ADDRMAN_BUCKET_SIZE_LOG2}; + +/** + * Extended statistics about a CAddress + */ +class AddrInfo : public CAddress +{ +public: + //! last try whatsoever by us (memory only) + int64_t nLastTry{0}; + + //! last counted attempt (memory only) + int64_t nLastCountAttempt{0}; + + //! where knowledge about this address first came from + CNetAddr source; + + //! last successful connection by us + int64_t nLastSuccess{0}; + + //! connection attempts since last successful attempt + int nAttempts{0}; + + //! reference count in new sets (memory only) + int nRefCount{0}; + + //! in tried set? (memory only) + bool fInTried{false}; + + //! position in vRandom + mutable int nRandomPos{-1}; + + SERIALIZE_METHODS(AddrInfo, obj) + { + READWRITEAS(CAddress, obj); + READWRITE(obj.source, obj.nLastSuccess, obj.nAttempts); + } + + AddrInfo(const CAddress &addrIn, const CNetAddr &addrSource) : CAddress(addrIn), source(addrSource) + { + } + + AddrInfo() : CAddress(), source() + { + } + + //! Calculate in which "tried" bucket this entry belongs + int GetTriedBucket(const uint256 &nKey, const std::vector &asmap) const; + + //! Calculate in which "new" bucket this entry belongs, given a certain source + int GetNewBucket(const uint256 &nKey, const CNetAddr& src, const std::vector &asmap) const; + + //! Calculate in which "new" bucket this entry belongs, using its default source + int GetNewBucket(const uint256 &nKey, const std::vector &asmap) const + { + return GetNewBucket(nKey, source, asmap); + } + + //! Calculate in which position of a bucket to store this entry. + int GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const; + + //! Determine whether the statistics about this entry are bad enough so that it can just be deleted + bool IsTerrible(int64_t nNow = GetAdjustedTime()) const; + + //! Calculate the relative chance this entry should be given when selecting nodes to connect to + double GetChance(int64_t nNow = GetAdjustedTime()) const; +}; + +class AddrManImpl +{ +public: + AddrManImpl(std::vector&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports); + + ~AddrManImpl(); + + template + void Serialize(Stream& s_) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + + template + void Unserialize(Stream& s_) EXCLUSIVE_LOCKS_REQUIRED(!cs); + + size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + + bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + void Good(const CService& addr, int64_t nTime) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + void Attempt(const CService& addr, bool fCountFailure, int64_t nTime) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!cs); + + std::pair SelectTriedCollision() EXCLUSIVE_LOCKS_REQUIRED(!cs); + + std::pair Select(bool newOnly) const + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + std::vector GetAddr(size_t max_addresses, size_t max_pct, std::optional network) const + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + void Connected(const CService& addr, int64_t nTime) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + void SetServices(const CService& addr, ServiceFlags nServices) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + AddrInfo GetAddressInfo(const CService& addr); + + const std::vector& GetAsmap() const; + + friend class AddrManTest; + friend class AddrManDeterministic; + +private: + //! A mutex to protect the inner data structures. + mutable Mutex cs; + + //! Source of random numbers for randomization in inner loops + mutable FastRandomContext insecure_rand GUARDED_BY(cs); + + //! secret key to randomize bucket select with + uint256 nKey; + + //! Serialization versions. + enum Format : uint8_t { + V0_HISTORICAL = 0, //!< historic format, before commit e6b343d88 + V1_DETERMINISTIC = 1, //!< for pre-asmap files + V2_ASMAP = 2, //!< for files including asmap version + V3_BIP155 = 3, //!< same as V2_ASMAP plus addresses are in BIP155 format + }; + + //! The maximum format this software knows it can unserialize. Also, we always serialize + //! in this format. + //! The format (first byte in the serialized stream) can be higher than this and + //! still this software may be able to unserialize the file - if the second byte + //! (see `lowest_compatible` in `Unserialize()`) is less or equal to this. + static constexpr Format FILE_FORMAT = Format::V3_BIP155; + + //! The initial value of a field that is incremented every time an incompatible format + //! change is made (such that old software versions would not be able to parse and + //! understand the new file format). This is 32 because we overtook the "key size" + //! field which was 32 historically. + //! @note Don't increment this. Increment `lowest_compatible` in `Serialize()` instead. + static constexpr uint8_t INCOMPATIBILITY_BASE = 32; + + //! last used nId + int nIdCount GUARDED_BY(cs){0}; + + //! table with information about all nIds + std::unordered_map mapInfo GUARDED_BY(cs); + + //! find an nId based on its network address + std::unordered_map mapAddr GUARDED_BY(cs); + + //! randomly-ordered vector of all nIds + //! This is mutable because it is unobservable outside the class, so any + //! changes to it (even in const methods) are also unobservable. + mutable std::vector vRandom GUARDED_BY(cs); + + // number of "tried" entries + int nTried GUARDED_BY(cs){0}; + + //! list of "tried" buckets + int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); + + //! number of (unique) "new" entries + int nNew GUARDED_BY(cs){0}; + + //! list of "new" buckets + int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs); + + //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse. + int64_t nLastGood GUARDED_BY(cs){1}; + + //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions. + std::set m_tried_collisions; + + /** 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; + + //! Discriminate entries based on port. + bool m_discriminate_ports GUARDED_BY(cs); + + //! Find an entry. + AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); + + //! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom. + AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); + + //! Swap two elements in vRandom. + void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) const EXCLUSIVE_LOCKS_REQUIRED(cs); + + //! Delete an entry. It must not be in tried, and have refcount 0. + void Delete(int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); + + //! Clear a position in a "new" table. This is the only place where entries are actually deleted. + void ClearNew(int nUBucket, int nUBucketPos) EXCLUSIVE_LOCKS_REQUIRED(cs); + + //! Move an entry from the "new" table(s) to the "tried" table + void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); + + void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); + + bool Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); + + void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); + + std::pair Select_(bool newOnly) const EXCLUSIVE_LOCKS_REQUIRED(cs); + + std::vector GetAddr_(size_t max_addresses, size_t max_pct, std::optional network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + + void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); + + void SetServices_(const CService& addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs); + + AddrInfo GetAddressInfo_(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(cs); + + void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs); + + std::pair SelectTriedCollision_() 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); +}; + +#endif // BITCOIN_ADDRMAN_IMPL_H diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index c1fd9fe4a5..535dad15b3 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -51,14 +51,14 @@ static void CreateAddresses() } } -static void AddAddressesToAddrMan(CAddrMan& addrman) +static void AddAddressesToAddrMan(AddrMan& addrman) { for (size_t source_i = 0; source_i < NUM_SOURCES; ++source_i) { addrman.Add(g_addresses[source_i], g_sources[source_i]); } } -static void FillAddrMan(CAddrMan& addrman) +static void FillAddrMan(AddrMan& addrman) { CreateAddresses(); @@ -72,26 +72,26 @@ static void AddrManAdd(benchmark::Bench& bench) CreateAddresses(); bench.run([&] { - CAddrMan addrman{/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0}; + AddrMan addrman{/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0}; AddAddressesToAddrMan(addrman); }); } static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); bench.run([&] { const auto& address = addrman.Select(); - assert(address.GetPort() > 0); + assert(address.first.GetPort() > 0); }); } static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -103,21 +103,21 @@ static void AddrManGetAddr(benchmark::Bench& bench) static void AddrManGood(benchmark::Bench& bench) { - /* Create many CAddrMan objects - one to be modified at each loop iteration. - * This is necessary because the CAddrMan::Good() method modifies the + /* 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> addrmans(addrman_count); + std::vector> addrmans(addrman_count); for (size_t i{0}; i < addrman_count; ++i) { - addrmans[i] = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + addrmans[i] = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(*addrmans[i]); } - auto markSomeAsGood = [](CAddrMan& addrman) { + 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) { diff --git a/src/net.cpp b/src/net.cpp index d4cecd4758..e82d132060 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2626,17 +2626,18 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe if (nTries > 100) break; - CAddrInfo addr; + CAddress addr; + int64_t addr_last_try{0}; if (fFeeler) { // First, try to get a tried table collision address. This returns // an empty (invalid) address if there are no collisions to try. - addr = addrman.SelectTriedCollision(); + std::tie(addr, addr_last_try) = addrman.SelectTriedCollision(); if (!addr.IsValid()) { // No tried table collisions. Select a new table address // for our feeler. - addr = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true); } else if (AlreadyConnectedToAddress(addr)) { // If test-before-evict logic would have us connect to a // peer that we're already connected to, just mark that @@ -2645,11 +2646,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe // a currently-connected peer. addrman.Good(addr); // Select a new table address for our feeler instead. - addr = addrman.Select(true); + std::tie(addr, addr_last_try) = addrman.Select(true); } } else { // Not a feeler - addr = addrman.Select(); + std::tie(addr, addr_last_try) = addrman.Select(); } auto dmn = mnList.GetMNByService(addr); @@ -2678,7 +2679,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect, CDe continue; // only consider very recently tried nodes after 30 failed attempts - if (nANow - addr.nLastTry < 600 && nTries < 30) + if (nANow - addr_last_try < 600 && nTries < 30) continue; // for non-feelers, require all the services we'll want, @@ -3308,7 +3309,7 @@ void CConnman::SetNetworkActive(bool active, CMasternodeSync* const mn_sync) uiInterface.NotifyNetworkActiveChanged(fNetworkActive); } -CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, CAddrMan& addrman_in, bool network_active) : +CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In, AddrMan& addrman_in, bool network_active) : addrman(addrman_in), nSeed0(nSeed0In), nSeed1(nSeed1In) { SetTryNewOutboundPeer(false); diff --git a/src/net.h b/src/net.h index 259a3ddf49..d50bde2105 100644 --- a/src/net.h +++ b/src/net.h @@ -892,7 +892,7 @@ public: m_onion_binds = connOptions.onion_binds; } - CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman, bool network_active = true); + CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true); ~CConnman(); bool Start(CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CScheduler& scheduler, const Options& options) @@ -1403,7 +1403,7 @@ private: std::vector vhListenSocket; std::atomic fNetworkActive{true}; bool fAddressesInitialized{false}; - CAddrMan& addrman; + AddrMan& addrman; std::deque m_addr_fetches GUARDED_BY(m_addr_fetches_mutex); Mutex m_addr_fetches_mutex; std::vector m_added_nodes GUARDED_BY(m_added_nodes_mutex); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f41f00aaa3..b9d14457da 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -363,7 +363,7 @@ using PeerRef = std::shared_ptr; class PeerManagerImpl final : public PeerManager { public: - PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, BanMan* banman, + PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CGovernanceManager& govman, CSporkManager& sporkman, @@ -497,7 +497,7 @@ private: const CChainParams& m_chainparams; CConnman& m_connman; - CAddrMan& m_addrman; + AddrMan& m_addrman; /** Pointer to this node's banman. May be nullptr - check existence before dereferencing. */ BanMan* const m_banman; ChainstateManager& m_chainman; @@ -1836,7 +1836,7 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT); } -std::unique_ptr PeerManager::make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, BanMan* banman, +std::unique_ptr PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CGovernanceManager& govman, CSporkManager& sporkman, @@ -1848,7 +1848,7 @@ std::unique_ptr PeerManager::make(const CChainParams& chainparams, return std::make_unique(chainparams, connman, addrman, banman, scheduler, chainman, pool, mn_metaman, mn_sync, govman, sporkman, mn_activeman, dmnman, cj_ctx, llmq_ctx, ignore_incoming_txs); } -PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, BanMan* banman, +PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CGovernanceManager& govman, CSporkManager& sporkman, @@ -3440,7 +3440,7 @@ void PeerManagerImpl::ProcessMessage( // table is also potentially detrimental because new-table entries // are subject to eviction in the event of addrman collisions. We // mitigate the information-leak by never calling - // CAddrMan::Connected() on block-relay-only peers; see + // AddrMan::Connected() on block-relay-only peers; see // FinalizeNode(). // // This moves an address from New to Tried table in Addrman, diff --git a/src/net_processing.h b/src/net_processing.h index 47bccd83bf..cc7c846b68 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -14,7 +14,7 @@ #include class CActiveMasternodeManager; -class CAddrMan; +class AddrMan; class CTxMemPool; class CDeterministicMNManager; class CMasternodeMetaMan; @@ -56,7 +56,7 @@ struct CNodeStateStats { class PeerManager : public CValidationInterface, public NetEventsInterface { public: - static std::unique_ptr make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, + static std::unique_ptr make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, CScheduler &scheduler, ChainstateManager& chainman, CTxMemPool& pool, CMasternodeMetaMan& mn_metaman, CMasternodeSync& mn_sync, CGovernanceManager& govman, CSporkManager& sporkman, diff --git a/src/netaddress.cpp b/src/netaddress.cpp index b0cc8b052b..5c0d1b907d 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -166,7 +166,7 @@ void CNetAddr::SetLegacyIPv6(Span ipv6) } /** - * Create an "internal" address that represents a name or FQDN. CAddrMan uses + * Create an "internal" address that represents a name or FQDN. AddrMan uses * these fake addresses to keep track of which DNS seeds were used. * @returns Whether or not the operation was successful. * @see NET_INTERNAL, INTERNAL_IN_IPV6_PREFIX, CNetAddr::IsInternal(), CNetAddr::IsRFC4193() diff --git a/src/netaddress.h b/src/netaddress.h index 9db4184a41..69c90facb1 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -63,7 +63,7 @@ enum Network { NET_CJDNS, /// A set of addresses that represent the hash of a string or FQDN. We use - /// them in CAddrMan to keep track of which DNS seeds were used. + /// them in AddrMan to keep track of which DNS seeds were used. NET_INTERNAL, /// Dummy value to indicate the number of NET_* constants. diff --git a/src/node/context.h b/src/node/context.h index 88d8128842..ed435f31fc 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -13,7 +13,7 @@ class ArgsManager; class BanMan; class CActiveMasternodeManager; -class CAddrMan; +class AddrMan; class CBlockPolicyEstimator; class CConnman; class CCreditPoolManager; @@ -53,7 +53,7 @@ class Loader; //! any member functions. It should just be a collection of references that can //! be used without pulling in unwanted dependencies or functionality. struct NodeContext { - std::unique_ptr addrman; + std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; std::unique_ptr fee_estimator; diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 90d96c6cf0..ca34bbc3b4 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -19,26 +20,26 @@ using namespace std::literals; -class CAddrManSerializationMock : public CAddrMan +class AddrManSerializationMock : public AddrMan { public: virtual void Serialize(CDataStream& s) const = 0; - CAddrManSerializationMock() - : CAddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) + AddrManSerializationMock() + : AddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) {} }; -class CAddrManUncorrupted : public CAddrManSerializationMock +class AddrManUncorrupted : public AddrManSerializationMock { public: void Serialize(CDataStream& s) const override { - CAddrMan::Serialize(s); + AddrMan::Serialize(s); } }; -class CAddrManCorrupted : public CAddrManSerializationMock +class AddrManCorrupted : public AddrManSerializationMock { public: void Serialize(CDataStream& s) const override @@ -59,12 +60,12 @@ public: CAddress addr = CAddress(serv, NODE_NONE); CNetAddr resolved; BOOST_CHECK(LookupHost("252.2.2.2", resolved, false)); - CAddrInfo info = CAddrInfo(addr, resolved); + AddrInfo info = AddrInfo(addr, resolved); s << info; } }; -static CDataStream AddrmanToStream(const CAddrManSerializationMock& _addrman) +static CDataStream AddrmanToStream(const AddrManSerializationMock& _addrman) { CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION); ssPeersIn << Params().MessageStart(); @@ -74,45 +75,45 @@ static CDataStream AddrmanToStream(const CAddrManSerializationMock& _addrman) return CDataStream(vchData, SER_DISK, CLIENT_VERSION); } -class CAddrManTest : public CAddrMan +class AddrManTest : public AddrMan { private: bool deterministic; public: - explicit CAddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) - : CAddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) + explicit AddrManTest(bool makeDeterministic = true, + std::vector asmap = std::vector()) + : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) { deterministic = makeDeterministic; } - CAddrInfo* Find(const CService& addr, int* pnId = nullptr) + AddrInfo* Find(const CService& addr, int* pnId = nullptr) { - LOCK(cs); - return CAddrMan::Find(addr, pnId); + LOCK(m_impl->cs); + return m_impl->Find(addr, pnId); } - CAddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) + AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) { - LOCK(cs); - return CAddrMan::Create(addr, addrSource, pnId); + LOCK(m_impl->cs); + return m_impl->Create(addr, addrSource, pnId); } void Delete(int nId) { - LOCK(cs); - CAddrMan::Delete(nId); + LOCK(m_impl->cs); + m_impl->Delete(nId); } // Used to test deserialization std::pair GetBucketAndEntry(const CAddress& addr) { - LOCK(cs); - int nId = mapAddr[addr]; + LOCK(m_impl->cs); + int nId = m_impl->mapAddr[addr]; for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) { for (int entry = 0; entry < ADDRMAN_BUCKET_SIZE; ++entry) { - if (nId == vvNew[bucket][entry]) { + if (nId == m_impl->vvNew[bucket][entry]) { return std::pair(bucket, entry); } } @@ -164,20 +165,20 @@ BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(addrman_simple) { - auto addrman = std::make_unique(); + auto addrman = std::make_unique(); CNetAddr source = ResolveIP("252.2.2.2"); // Test: Does Addrman respond correctly when empty. BOOST_CHECK_EQUAL(addrman->size(), 0U); - CAddrInfo addr_null = addrman->Select(); + auto addr_null = addrman->Select().first; BOOST_CHECK_EQUAL(addr_null.ToString(), "[::]:0"); // Test: Does Addrman::Add work as expected. CService addr1 = ResolveService("250.1.1.1", 8333); BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman->size(), 1U); - CAddrInfo addr_ret1 = addrman->Select(); + auto addr_ret1 = addrman->Select().first; BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: Does IP address deduplication work correctly. @@ -198,7 +199,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_CHECK(addrman->size() >= 1); // Test: reset addrman and test AddrMan::Add multiple addresses works as expected - addrman = std::make_unique(); + addrman = std::make_unique(); std::vector vAddr; vAddr.push_back(CAddress(ResolveService("250.1.1.3", 8333), NODE_NONE)); vAddr.push_back(CAddress(ResolveService("250.1.1.4", 8333), NODE_NONE)); @@ -208,7 +209,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_AUTO_TEST_CASE(addrman_ports) { - CAddrManTest addrman; + AddrManTest addrman; CNetAddr source = ResolveIP("252.2.2.2"); @@ -222,7 +223,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports) CService addr1_port = ResolveService("250.1.1.1", 8334); BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman.size(), 1U); - CAddrInfo addr_ret2 = addrman.Select(); + auto addr_ret2 = addrman.Select().first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333"); // Test: Add same IP but diff port to tried table, it doesn't get added. @@ -230,14 +231,14 @@ BOOST_AUTO_TEST_CASE(addrman_ports) addrman.Good(CAddress(addr1_port, NODE_NONE)); BOOST_CHECK_EQUAL(addrman.size(), 1U); bool newOnly = true; - CAddrInfo addr_ret3 = addrman.Select(newOnly); + auto addr_ret3 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); } BOOST_AUTO_TEST_CASE(addrman_select) { - CAddrManTest addrman; + AddrManTest addrman; CNetAddr source = ResolveIP("252.2.2.2"); @@ -247,16 +248,16 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK_EQUAL(addrman.size(), 1U); bool newOnly = true; - CAddrInfo addr_ret1 = addrman.Select(newOnly); + auto addr_ret1 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: move addr to tried, select from new expected nothing returned. addrman.Good(CAddress(addr1, NODE_NONE)); BOOST_CHECK_EQUAL(addrman.size(), 1U); - CAddrInfo addr_ret2 = addrman.Select(newOnly); + auto addr_ret2 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0"); - CAddrInfo addr_ret3 = addrman.Select(); + auto addr_ret3 = addrman.Select().first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); BOOST_CHECK_EQUAL(addrman.size(), 1U); @@ -289,14 +290,14 @@ BOOST_AUTO_TEST_CASE(addrman_select) // Test: Select pulls from new and tried regardless of port number. std::set ports; for (int i = 0; i < 20; ++i) { - ports.insert(addrman.Select().GetPort()); + ports.insert(addrman.Select().first.GetPort()); } BOOST_CHECK_EQUAL(ports.size(), 3U); } BOOST_AUTO_TEST_CASE(addrman_new_collisions) { - CAddrManTest addrman; + AddrManTest addrman; CNetAddr source = ResolveIP("252.2.2.2"); @@ -325,7 +326,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) BOOST_AUTO_TEST_CASE(addrman_tried_collisions) { - CAddrManTest addrman; + AddrManTest addrman; CNetAddr source = ResolveIP("252.2.2.2"); @@ -355,7 +356,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) BOOST_AUTO_TEST_CASE(addrman_find) { - CAddrManTest addrman; + AddrManTest addrman; BOOST_CHECK_EQUAL(addrman.size(), 0U); @@ -371,24 +372,24 @@ BOOST_AUTO_TEST_CASE(addrman_find) BOOST_CHECK(addrman.Add({addr3}, source1)); // Test: ensure Find returns an IP matching what we searched on. - CAddrInfo* info1 = addrman.Find(addr1); + AddrInfo* info1 = addrman.Find(addr1); BOOST_REQUIRE(info1); BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333"); // Test 18; Find does not discriminate by port number. - CAddrInfo* info2 = addrman.Find(addr2); + AddrInfo* info2 = addrman.Find(addr2); BOOST_REQUIRE(info2); BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString()); // Test: Find returns another IP matching what we searched on. - CAddrInfo* info3 = addrman.Find(addr3); + AddrInfo* info3 = addrman.Find(addr3); BOOST_REQUIRE(info3); BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333"); } BOOST_AUTO_TEST_CASE(addrman_create) { - CAddrManTest addrman; + AddrManTest addrman; BOOST_CHECK_EQUAL(addrman.size(), 0U); @@ -396,19 +397,19 @@ BOOST_AUTO_TEST_CASE(addrman_create) CNetAddr source1 = ResolveIP("250.1.2.1"); int nId; - CAddrInfo* pinfo = addrman.Create(addr1, source1, &nId); + AddrInfo* pinfo = addrman.Create(addr1, source1, &nId); // Test: The result should be the same as the input addr. BOOST_CHECK_EQUAL(pinfo->ToString(), "250.1.2.1:8333"); - CAddrInfo* info2 = addrman.Find(addr1); + AddrInfo* info2 = addrman.Find(addr1); BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:8333"); } BOOST_AUTO_TEST_CASE(addrman_delete) { - CAddrManTest addrman; + AddrManTest addrman; BOOST_CHECK_EQUAL(addrman.size(), 0U); @@ -422,13 +423,13 @@ BOOST_AUTO_TEST_CASE(addrman_delete) BOOST_CHECK_EQUAL(addrman.size(), 1U); addrman.Delete(nId); BOOST_CHECK_EQUAL(addrman.size(), 0U); - CAddrInfo* info2 = addrman.Find(addr1); + AddrInfo* info2 = addrman.Find(addr1); BOOST_CHECK(info2 == nullptr); } BOOST_AUTO_TEST_CASE(addrman_getaddr) { - CAddrManTest addrman; + AddrManTest addrman; // Test: Sanity check, GetAddr should never return anything if addrman // is empty. @@ -488,7 +489,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr) BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) { - CAddrManTest addrman; + AddrManTest addrman; CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.1.1", 9999), NODE_NONE); @@ -496,7 +497,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) CNetAddr source1 = ResolveIP("250.1.1.1"); - CAddrInfo info1 = CAddrInfo(addr1, source1); + AddrInfo info1 = AddrInfo(addr1, source1); uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); @@ -511,14 +512,14 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) // Test: Two addresses with same IP but different ports can map to // different buckets because they have different keys. - CAddrInfo info2 = CAddrInfo(addr2, source1); + AddrInfo info2 = AddrInfo(addr2, source1); BOOST_CHECK(info1.GetKey() != info2.GetKey()); BOOST_CHECK(info1.GetTriedBucket(nKey1, asmap) != info2.GetTriedBucket(nKey1, asmap)); std::set buckets; for (int i = 0; i < 255; i++) { - CAddrInfo infoi = CAddrInfo( + AddrInfo infoi = AddrInfo( CAddress(ResolveService("250.1.1." + ToString(i)), NODE_NONE), ResolveIP("250.1.1." + ToString(i))); int bucket = infoi.GetTriedBucket(nKey1, asmap); @@ -530,7 +531,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) buckets.clear(); for (int j = 0; j < 255; j++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("250." + ToString(j) + ".1.1"), NODE_NONE), ResolveIP("250." + ToString(j) + ".1.1")); int bucket = infoj.GetTriedBucket(nKey1, asmap); @@ -543,14 +544,14 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) { - CAddrManTest addrman; + AddrManTest addrman; CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE); CNetAddr source1 = ResolveIP("250.1.2.1"); - CAddrInfo info1 = CAddrInfo(addr1, source1); + AddrInfo info1 = AddrInfo(addr1, source1); uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); @@ -566,13 +567,13 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) BOOST_CHECK(info1.GetNewBucket(nKey1, asmap) != info1.GetNewBucket(nKey2, asmap)); // Test: Ports should not affect bucket placement in the addr - CAddrInfo info2 = CAddrInfo(addr2, source1); + AddrInfo info2 = AddrInfo(addr2, source1); BOOST_CHECK(info1.GetKey() != info2.GetKey()); BOOST_CHECK_EQUAL(info1.GetNewBucket(nKey1, asmap), info2.GetNewBucket(nKey1, asmap)); std::set buckets; for (int i = 0; i < 255; i++) { - CAddrInfo infoi = CAddrInfo( + AddrInfo infoi = AddrInfo( CAddress(ResolveService("250.1.1." + ToString(i)), NODE_NONE), ResolveIP("250.1.1." + ToString(i))); int bucket = infoi.GetNewBucket(nKey1, asmap); @@ -584,7 +585,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) buckets.clear(); for (int j = 0; j < 4 * 255; j++) { - CAddrInfo infoj = CAddrInfo(CAddress( + AddrInfo infoj = AddrInfo(CAddress( ResolveService( ToString(250 + (j / 255)) + "." + ToString(j % 256) + ".1.1"), NODE_NONE), ResolveIP("251.4.1.1")); @@ -597,7 +598,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) buckets.clear(); for (int p = 0; p < 255; p++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("250.1.1.1"), NODE_NONE), ResolveIP("250." + ToString(p) + ".1.1")); int bucket = infoj.GetNewBucket(nKey1, asmap); @@ -621,7 +622,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) // 101.8.0.0/16 AS8 BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) { - CAddrManTest addrman; + AddrManTest addrman; CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.1.1", 9999), NODE_NONE); @@ -629,7 +630,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) CNetAddr source1 = ResolveIP("250.1.1.1"); - CAddrInfo info1 = CAddrInfo(addr1, source1); + AddrInfo info1 = AddrInfo(addr1, source1); uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); @@ -644,14 +645,14 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) // Test: Two addresses with same IP but different ports can map to // different buckets because they have different keys. - CAddrInfo info2 = CAddrInfo(addr2, source1); + AddrInfo info2 = AddrInfo(addr2, source1); BOOST_CHECK(info1.GetKey() != info2.GetKey()); BOOST_CHECK(info1.GetTriedBucket(nKey1, asmap) != info2.GetTriedBucket(nKey1, asmap)); std::set buckets; for (int j = 0; j < 255; j++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("101." + ToString(j) + ".1.1"), NODE_NONE), ResolveIP("101." + ToString(j) + ".1.1")); int bucket = infoj.GetTriedBucket(nKey1, asmap); @@ -663,7 +664,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) buckets.clear(); for (int j = 0; j < 255; j++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("250." + ToString(j) + ".1.1"), NODE_NONE), ResolveIP("250." + ToString(j) + ".1.1")); int bucket = infoj.GetTriedBucket(nKey1, asmap); @@ -676,14 +677,14 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) { - CAddrManTest addrman; + AddrManTest addrman; CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE); CNetAddr source1 = ResolveIP("250.1.2.1"); - CAddrInfo info1 = CAddrInfo(addr1, source1); + AddrInfo info1 = AddrInfo(addr1, source1); uint256 nKey1 = (uint256)(CHashWriter(SER_GETHASH, 0) << 1).GetHash(); uint256 nKey2 = (uint256)(CHashWriter(SER_GETHASH, 0) << 2).GetHash(); @@ -699,13 +700,13 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) BOOST_CHECK(info1.GetNewBucket(nKey1, asmap) != info1.GetNewBucket(nKey2, asmap)); // Test: Ports should not affect bucket placement in the addr - CAddrInfo info2 = CAddrInfo(addr2, source1); + AddrInfo info2 = AddrInfo(addr2, source1); BOOST_CHECK(info1.GetKey() != info2.GetKey()); BOOST_CHECK_EQUAL(info1.GetNewBucket(nKey1, asmap), info2.GetNewBucket(nKey1, asmap)); std::set buckets; for (int i = 0; i < 255; i++) { - CAddrInfo infoi = CAddrInfo( + AddrInfo infoi = AddrInfo( CAddress(ResolveService("250.1.1." + ToString(i)), NODE_NONE), ResolveIP("250.1.1." + ToString(i))); int bucket = infoi.GetNewBucket(nKey1, asmap); @@ -717,7 +718,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) buckets.clear(); for (int j = 0; j < 4 * 255; j++) { - CAddrInfo infoj = CAddrInfo(CAddress( + AddrInfo infoj = AddrInfo(CAddress( ResolveService( ToString(250 + (j / 255)) + "." + ToString(j % 256) + ".1.1"), NODE_NONE), ResolveIP("251.4.1.1")); @@ -730,7 +731,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) buckets.clear(); for (int p = 0; p < 255; p++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("250.1.1.1"), NODE_NONE), ResolveIP("101." + ToString(p) + ".1.1")); int bucket = infoj.GetNewBucket(nKey1, asmap); @@ -742,7 +743,7 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) buckets.clear(); for (int p = 0; p < 255; p++) { - CAddrInfo infoj = CAddrInfo( + AddrInfo infoj = AddrInfo( CAddress(ResolveService("250.1.1.1"), NODE_NONE), ResolveIP("250." + ToString(p) + ".1.1")); int bucket = infoj.GetNewBucket(nKey1, asmap); @@ -758,9 +759,9 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(raw_tests::asmap, sizeof(raw_tests::asmap) * 8); - auto addrman_asmap1 = std::make_unique(true, asmap1); - auto addrman_asmap1_dup = std::make_unique(true, asmap1); - auto addrman_noasmap = std::make_unique(); + auto addrman_asmap1 = std::make_unique(true, asmap1); + auto addrman_asmap1_dup = std::make_unique(true, asmap1); + auto addrman_noasmap = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); CAddress addr = CAddress(ResolveService("250.1.1.1"), NODE_NONE); @@ -790,8 +791,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) BOOST_CHECK(bucketAndEntry_asmap1.second != bucketAndEntry_noasmap.second); // deserializing non-asmaped peers.dat to asmaped addrman - addrman_asmap1 = std::make_unique(true, asmap1); - addrman_noasmap = std::make_unique(); + addrman_asmap1 = std::make_unique(true, asmap1); + addrman_noasmap = std::make_unique(); addrman_noasmap->Add({addr}, default_source); stream << *addrman_noasmap; stream >> *addrman_asmap1; @@ -802,8 +803,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) BOOST_CHECK(bucketAndEntry_asmap1_deser.second == bucketAndEntry_asmap1_dup.second); // used to map to different buckets, now maps to the same bucket. - addrman_asmap1 = std::make_unique(true, asmap1); - addrman_noasmap = std::make_unique(); + addrman_asmap1 = std::make_unique(true, asmap1); + addrman_noasmap = std::make_unique(); CAddress addr1 = CAddress(ResolveService("250.1.1.1"), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.2.1.1"), NODE_NONE); addrman_noasmap->Add({addr, addr2}, default_source); @@ -823,7 +824,7 @@ BOOST_AUTO_TEST_CASE(remove_invalid) { // Confirm that invalid addresses are ignored in unserialization. - auto addrman = std::make_unique(); + auto addrman = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE}; @@ -855,19 +856,19 @@ BOOST_AUTO_TEST_CASE(remove_invalid) BOOST_REQUIRE(pos + sizeof(tried2_raw_replacement) <= stream.size()); memcpy(stream.data() + pos, tried2_raw_replacement, sizeof(tried2_raw_replacement)); - addrman = std::make_unique(); + addrman = std::make_unique(); stream >> *addrman; BOOST_CHECK_EQUAL(addrman->size(), 2); } BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) { - CAddrManTest addrman; + AddrManTest addrman; BOOST_CHECK(addrman.size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Add twenty two addresses. CNetAddr source = ResolveIP("252.2.2.2"); @@ -878,7 +879,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) // No collisions yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Ensure Good handles duplicates well. @@ -887,14 +888,14 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) addrman.Good(addr); BOOST_CHECK(addrman.size() == 22); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } } BOOST_AUTO_TEST_CASE(addrman_noevict) { - CAddrManTest addrman; + AddrManTest addrman; // Add 35 addresses. CNetAddr source = ResolveIP("252.2.2.2"); @@ -905,7 +906,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) // No collision yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Collision between 36 and 19. @@ -914,11 +915,11 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr36); BOOST_CHECK(addrman.size() == 36); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.19:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0"); // 36 should be discarded and 19 not evicted. addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Lets create two collisions. for (unsigned int i = 37; i < 59; i++) { @@ -927,7 +928,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr); BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Cause a collision. @@ -936,26 +937,26 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) addrman.Good(addr59); BOOST_CHECK(addrman.size() == 59); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.10:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0"); // Cause a second collision. BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source)); addrman.Good(addr36); BOOST_CHECK(addrman.size() == 59); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() != "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0"); addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } BOOST_AUTO_TEST_CASE(addrman_evictionworks) { - CAddrManTest addrman; + AddrManTest addrman; BOOST_CHECK(addrman.size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // Add 35 addresses CNetAddr source = ResolveIP("252.2.2.2"); @@ -966,7 +967,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // No collision yet. BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Collision between 36 and 19. @@ -975,7 +976,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) addrman.Good(addr); BOOST_CHECK_EQUAL(addrman.size(), 36); - CAddrInfo info = addrman.SelectTriedCollision(); + auto info = addrman.SelectTriedCollision().first; BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0"); // Ensure test of address fails, so that it is evicted. @@ -983,28 +984,28 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // Should swap 36 for 19. addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // If 36 was swapped for 19, then this should cause no collisions. BOOST_CHECK(!addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); // If we insert 19 it should collide with 36 CService addr19 = ResolveService("250.1.1.19"); BOOST_CHECK(!addrman.Add({CAddress(addr19, NODE_NONE)}, source)); addrman.Good(addr19); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().ToString(), "250.1.1.36:0"); + BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0"); addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); + BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } BOOST_AUTO_TEST_CASE(load_addrman) { - CAddrManUncorrupted addrmanUncorrupted; + AddrManUncorrupted addrmanUncorrupted; CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -1023,7 +1024,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { @@ -1040,7 +1041,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) // Test that ReadFromStream creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); ReadFromStream(addrman2, ssPeers2); BOOST_CHECK(addrman2.size() == 3); @@ -1049,12 +1050,12 @@ BOOST_AUTO_TEST_CASE(load_addrman) BOOST_AUTO_TEST_CASE(load_addrman_corrupted) { - CAddrManCorrupted addrmanCorrupted; + AddrManCorrupted addrmanCorrupted; // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -1070,7 +1071,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) // Test that ReadFromStream fails if peers.dat is corrupt CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure); } diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index f772397e9d..18a1078cd4 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -27,29 +28,29 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addr_man(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); try { ReadFromStream(addr_man, data_stream); } catch (const std::exception&) { } } -class CAddrManDeterministic : public CAddrMan +class AddrManDeterministic : public AddrMan { public: FuzzedDataProvider& m_fuzzed_data_provider; - explicit CAddrManDeterministic(std::vector asmap, FuzzedDataProvider& fuzzed_data_provider) - : CAddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0) + explicit AddrManDeterministic(std::vector asmap, FuzzedDataProvider& fuzzed_data_provider) + : AddrMan(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)}); + WITH_LOCK(m_impl->cs, m_impl->insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); } /** * Generate a random address. Always returns a valid address. */ - CNetAddr RandAddr() EXCLUSIVE_LOCKS_REQUIRED(cs) + CNetAddr RandAddr() EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs) { CNetAddr addr; if (m_fuzzed_data_provider.remaining_bytes() > 1 && m_fuzzed_data_provider.ConsumeBool()) { @@ -61,7 +62,7 @@ public: {4, ADDR_TORV3_SIZE}, {5, ADDR_I2P_SIZE}, {6, ADDR_CJDNS_SIZE}}; - uint8_t net = insecure_rand.randrange(5) + 1; // [1..5] + uint8_t net = m_impl->insecure_rand.randrange(5) + 1; // [1..5] if (net == 3) { net = 6; } @@ -69,7 +70,7 @@ public: CDataStream s(SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT); s << net; - s << insecure_rand.randbytes(net_len_map.at(net)); + s << m_impl->insecure_rand.randbytes(net_len_map.at(net)); s >> addr; } @@ -89,7 +90,7 @@ public: */ void Fill() { - LOCK(cs); + LOCK(m_impl->cs); // Add some of the addresses directly to the "tried" table. @@ -102,20 +103,20 @@ public: // the latter is exhausted it just returns 0. for (size_t i = 0; i < num_sources; ++i) { const auto source = RandAddr(); - const size_t num_addresses = insecure_rand.randrange(500) + 1; // [1..500] + const size_t num_addresses = m_impl->insecure_rand.randrange(500) + 1; // [1..500] for (size_t j = 0; j < num_addresses; ++j) { const auto addr = CAddress{CService{RandAddr(), 8333}, NODE_NETWORK}; - const auto time_penalty = insecure_rand.randrange(100000001); - Add_(addr, source, time_penalty); + const auto time_penalty = m_impl->insecure_rand.randrange(100000001); + m_impl->Add_(addr, source, time_penalty); - if (n > 0 && mapInfo.size() % n == 0) { - Good_(addr, false, GetTime()); + if (n > 0 && m_impl->mapInfo.size() % n == 0) { + m_impl->Good_(addr, false, GetTime()); } // Add 10% of the addresses from more than one source. - if (insecure_rand.randrange(10) == 0 && prev_source.IsValid()) { - Add_(addr, prev_source, time_penalty); + if (m_impl->insecure_rand.randrange(10) == 0 && prev_source.IsValid()) { + m_impl->Add_({addr}, prev_source, time_penalty); } } prev_source = source; @@ -129,46 +130,46 @@ public: * - vvNew entries refer to the same addresses * - vvTried entries refer to the same addresses */ - bool operator==(const CAddrManDeterministic& other) const + bool operator==(const AddrManDeterministic& other) const { - LOCK2(cs, other.cs); + LOCK2(m_impl->cs, other.m_impl->cs); - if (mapInfo.size() != other.mapInfo.size() || nNew != other.nNew || - nTried != other.nTried) { + if (m_impl->mapInfo.size() != other.m_impl->mapInfo.size() || m_impl->nNew != other.m_impl->nNew || + m_impl->nTried != other.m_impl->nTried) { return false; } // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`. // Keys may be different. - using CAddrInfoHasher = std::function; - using CAddrInfoEq = std::function; + using AddrInfoHasher = std::function; + using AddrInfoEq = std::function; CNetAddrHash netaddr_hasher; - CAddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const CAddrInfo& a) { + AddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const AddrInfo& a) { return netaddr_hasher(static_cast(a)) ^ netaddr_hasher(a.source) ^ a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried; }; - CAddrInfoEq addrinfo_eq = [](const CAddrInfo& lhs, const CAddrInfo& rhs) { + AddrInfoEq addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) { return static_cast(lhs) == static_cast(rhs) && lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess && lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount && lhs.fInTried == rhs.fInTried; }; - using Addresses = std::unordered_set; + using Addresses = std::unordered_set; - const size_t num_addresses{mapInfo.size()}; + const size_t num_addresses{m_impl->mapInfo.size()}; Addresses addresses{num_addresses, addrinfo_hasher, addrinfo_eq}; - for (const auto& [id, addr] : mapInfo) { + for (const auto& [id, addr] : m_impl->mapInfo) { addresses.insert(addr); } Addresses other_addresses{num_addresses, addrinfo_hasher, addrinfo_eq}; - for (const auto& [id, addr] : other.mapInfo) { + for (const auto& [id, addr] : other.m_impl->mapInfo) { other_addresses.insert(addr); } @@ -176,14 +177,14 @@ public: return false; } - auto IdsReferToSameAddress = [&](int id, int other_id) EXCLUSIVE_LOCKS_REQUIRED(cs, other.cs) { + auto IdsReferToSameAddress = [&](int id, int other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) { if (id == -1 && other_id == -1) { return true; } if ((id == -1 && other_id != -1) || (id != -1 && other_id == -1)) { return false; } - return mapInfo.at(id) == other.mapInfo.at(other_id); + return m_impl->mapInfo.at(id) == other.m_impl->mapInfo.at(other_id); }; // Check that `vvNew` contains the same addresses as `other.vvNew`. Notice - `vvNew[i][j]` @@ -191,7 +192,7 @@ public: // themselves may differ between `vvNew` and `other.vvNew`. for (size_t i = 0; i < ADDRMAN_NEW_BUCKET_COUNT; ++i) { for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) { - if (!IdsReferToSameAddress(vvNew[i][j], other.vvNew[i][j])) { + if (!IdsReferToSameAddress(m_impl->vvNew[i][j], other.m_impl->vvNew[i][j])) { return false; } } @@ -200,7 +201,7 @@ public: // Same for `vvTried`. for (size_t i = 0; i < ADDRMAN_TRIED_BUCKET_COUNT; ++i) { for (size_t j = 0; j < ADDRMAN_BUCKET_SIZE; ++j) { - if (!IdsReferToSameAddress(vvTried[i][j], other.vvTried[i][j])) { + if (!IdsReferToSameAddress(m_impl->vvTried[i][j], other.m_impl->vvTried[i][j])) { return false; } } @@ -222,7 +223,7 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); std::vector asmap = ConsumeAsmap(fuzzed_data_provider); - auto addr_man_ptr = std::make_unique(asmap, 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)}; CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION); @@ -231,10 +232,10 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) try { ds >> *addr_man_ptr; } catch (const std::ios_base::failure&) { - addr_man_ptr = std::make_unique(asmap, fuzzed_data_provider); + addr_man_ptr = std::make_unique(asmap, fuzzed_data_provider); } } - CAddrManDeterministic& addr_man = *addr_man_ptr; + AddrManDeterministic& addr_man = *addr_man_ptr; while (fuzzed_data_provider.ConsumeBool()) { CallOneOf( fuzzed_data_provider, @@ -304,8 +305,8 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) SetMockTime(ConsumeTime(fuzzed_data_provider)); std::vector asmap = ConsumeAsmap(fuzzed_data_provider); - CAddrManDeterministic addr_man1{asmap, fuzzed_data_provider}; - CAddrManDeterministic addr_man2{asmap, fuzzed_data_provider}; + AddrManDeterministic addr_man1{asmap, fuzzed_data_provider}; + AddrManDeterministic 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 7e2ecad471..aa2056c451 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(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index 5040e9d570..4bd4a55c83 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -103,7 +104,7 @@ FUZZ_TARGET_DESERIALIZE(block_filter_deserialize, { }) */ FUZZ_TARGET_DESERIALIZE(addr_info_deserialize, { - CAddrInfo addr_info; + AddrInfo addr_info; DeserializeFromFuzzingInput(buffer, addr_info); }) FUZZ_TARGET_DESERIALIZE(block_file_info_deserialize, { @@ -189,7 +190,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan am(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 454770cdcd..f91660f9c1 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -37,7 +37,7 @@ FUZZ_TARGET_INIT(net, initialize_net) CallOneOf( fuzzed_data_provider, [&] { - CAddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); CConnman connman{fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral(), addrman}; node.CloseSocketDisconnect(&connman); }, diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index dc3a893287..857134f452 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -180,7 +180,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve SetupNetworking(); InitSignatureCache(); InitScriptExecutionCache(); - m_node.addrman = std::make_unique(/* asmap */ 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.chain = interfaces::MakeChain(m_node); // while g_wallet_init_interface is init here at very early stage // we can't get rid of unique_ptr from wallet/contex.h diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 9638cbfc36..9fe2da53a5 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -108,7 +108,7 @@ class AddrmanTest(BitcoinTestFramework): self.stop_node(0) write_addrman(peers_dat, len_tried=-1) self.nodes[0].assert_start_raises_init_error( - expected_msg=init_error("Corrupt CAddrMan serialization: nTried=-1, should be in \\[0, 16384\\]:.*"), + expected_msg=init_error("Corrupt AddrMan serialization: nTried=-1, should be in \\[0, 16384\\]:.*"), match=ErrorMatch.FULL_REGEX, ) @@ -116,7 +116,7 @@ class AddrmanTest(BitcoinTestFramework): self.stop_node(0) write_addrman(peers_dat, len_new=-1) self.nodes[0].assert_start_raises_init_error( - expected_msg=init_error("Corrupt CAddrMan serialization: nNew=-1, should be in \\[0, 65536\\]:.*"), + expected_msg=init_error("Corrupt AddrMan serialization: nNew=-1, should be in \\[0, 65536\\]:.*"), match=ErrorMatch.FULL_REGEX, ) From 3910c6802840633515c374b356764295fb933fe3 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 21 Sep 2021 09:35:40 +0100 Subject: [PATCH 06/12] merge bitcoin#23053: Use public methods in addrman fuzz tests --- src/test/fuzz/addrman.cpp | 155 ++++++++++++++++++-------------------- 1 file changed, 75 insertions(+), 80 deletions(-) diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 18a1078cd4..44b0d1e118 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -35,94 +36,88 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman) } } +/** + * Generate a random address. Always returns a valid address. + */ +CNetAddr RandAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext& fast_random_context) +{ + CNetAddr addr; + if (fuzzed_data_provider.remaining_bytes() > 1 && fuzzed_data_provider.ConsumeBool()) { + addr = ConsumeNetAddr(fuzzed_data_provider); + } else { + // The networks [1..6] correspond to CNetAddr::BIP155Network (private). + static const std::map net_len_map = {{1, ADDR_IPV4_SIZE}, + {2, ADDR_IPV6_SIZE}, + {4, ADDR_TORV3_SIZE}, + {5, ADDR_I2P_SIZE}, + {6, ADDR_CJDNS_SIZE}}; + uint8_t net = fast_random_context.randrange(5) + 1; // [1..5] + if (net == 3) { + net = 6; + } + + CDataStream s(SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT); + + s << net; + s << fast_random_context.randbytes(net_len_map.at(net)); + + s >> addr; + } + + // Return a dummy IPv4 5.5.5.5 if we generated an invalid address. + if (!addr.IsValid()) { + in_addr v4_addr = {}; + v4_addr.s_addr = 0x05050505; + addr = CNetAddr{v4_addr}; + } + + return addr; +} + +/** Fill addrman with lots of addresses from lots of sources. */ +void FillAddrman(AddrMan& addrman, FuzzedDataProvider& fuzzed_data_provider) +{ + // Add a fraction of the addresses to the "tried" table. + // 0, 1, 2, 3 corresponding to 0%, 100%, 50%, 33% + const size_t n = fuzzed_data_provider.ConsumeIntegralInRange(0, 3); + + const size_t num_sources = fuzzed_data_provider.ConsumeIntegralInRange(1, 50); + CNetAddr prev_source; + // Generate a FastRandomContext seed to use inside the loops instead of + // fuzzed_data_provider. When fuzzed_data_provider is exhausted it + // just returns 0. + FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)}; + for (size_t i = 0; i < num_sources; ++i) { + const auto source = RandAddr(fuzzed_data_provider, fast_random_context); + const size_t num_addresses = fast_random_context.randrange(500) + 1; // [1..500] + + for (size_t j = 0; j < num_addresses; ++j) { + const auto addr = CAddress{CService{RandAddr(fuzzed_data_provider, fast_random_context), 8333}, NODE_NETWORK}; + const auto time_penalty = fast_random_context.randrange(100000001); + addrman.Add({addr}, source, time_penalty); + + if (n > 0 && addrman.size() % n == 0) { + addrman.Good(addr, GetTime()); + } + + // Add 10% of the addresses from more than one source. + if (fast_random_context.randrange(10) == 0 && prev_source.IsValid()) { + addrman.Add({addr}, prev_source, time_penalty); + } + } + prev_source = source; + } +} + class AddrManDeterministic : public AddrMan { public: - FuzzedDataProvider& m_fuzzed_data_provider; - explicit AddrManDeterministic(std::vector asmap, FuzzedDataProvider& fuzzed_data_provider) : AddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0) - , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(m_impl->cs, m_impl->insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); } - /** - * Generate a random address. Always returns a valid address. - */ - CNetAddr RandAddr() EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs) - { - CNetAddr addr; - if (m_fuzzed_data_provider.remaining_bytes() > 1 && m_fuzzed_data_provider.ConsumeBool()) { - addr = ConsumeNetAddr(m_fuzzed_data_provider); - } else { - // The networks [1..6] correspond to CNetAddr::BIP155Network (private). - static const std::map net_len_map = {{1, ADDR_IPV4_SIZE}, - {2, ADDR_IPV6_SIZE}, - {4, ADDR_TORV3_SIZE}, - {5, ADDR_I2P_SIZE}, - {6, ADDR_CJDNS_SIZE}}; - uint8_t net = m_impl->insecure_rand.randrange(5) + 1; // [1..5] - if (net == 3) { - net = 6; - } - - CDataStream s(SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT); - - s << net; - s << m_impl->insecure_rand.randbytes(net_len_map.at(net)); - - s >> addr; - } - - // Return a dummy IPv4 5.5.5.5 if we generated an invalid address. - if (!addr.IsValid()) { - in_addr v4_addr = {}; - v4_addr.s_addr = 0x05050505; - addr = CNetAddr{v4_addr}; - } - - return addr; - } - - /** - * Fill this addrman with lots of addresses from lots of sources. - */ - void Fill() - { - LOCK(m_impl->cs); - - // Add some of the addresses directly to the "tried" table. - - // 0, 1, 2, 3 corresponding to 0%, 100%, 50%, 33% - const size_t n = m_fuzzed_data_provider.ConsumeIntegralInRange(0, 3); - - const size_t num_sources = m_fuzzed_data_provider.ConsumeIntegralInRange(1, 50); - CNetAddr prev_source; - // Use insecure_rand inside the loops instead of m_fuzzed_data_provider because when - // the latter is exhausted it just returns 0. - for (size_t i = 0; i < num_sources; ++i) { - const auto source = RandAddr(); - const size_t num_addresses = m_impl->insecure_rand.randrange(500) + 1; // [1..500] - - for (size_t j = 0; j < num_addresses; ++j) { - const auto addr = CAddress{CService{RandAddr(), 8333}, NODE_NETWORK}; - const auto time_penalty = m_impl->insecure_rand.randrange(100000001); - m_impl->Add_(addr, source, time_penalty); - - if (n > 0 && m_impl->mapInfo.size() % n == 0) { - m_impl->Good_(addr, false, GetTime()); - } - - // Add 10% of the addresses from more than one source. - if (m_impl->insecure_rand.randrange(10) == 0 && prev_source.IsValid()) { - m_impl->Add_({addr}, prev_source, time_penalty); - } - } - prev_source = source; - } - } - /** * Compare with another AddrMan. * This compares: @@ -310,7 +305,7 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION); - addr_man1.Fill(); + FillAddrman(addr_man1, fuzzed_data_provider); data_stream << addr_man1; data_stream >> addr_man2; assert(addr_man1 == addr_man2); From 19b0145379d9cbe74472705e7bffa4e1660226a2 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 1 Jun 2024 19:32:22 +0000 Subject: [PATCH 07/12] merge bitcoin#22839: improve addrman logging --- src/addrman.cpp | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index b9e38ae522..35834f966e 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -45,10 +45,7 @@ int AddrInfo::GetTriedBucket(const uint256& nKey, const std::vector& asmap { 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(); - int tried_bucket = hash2 % ADDRMAN_TRIED_BUCKET_COUNT; - uint32_t mapped_as = GetMappedAS(asmap); - LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket); - return tried_bucket; + return hash2 % ADDRMAN_TRIED_BUCKET_COUNT; } int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std::vector& asmap) const @@ -56,10 +53,7 @@ int AddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std:: std::vector vchSourceGroupKey = src.GetGroup(asmap); uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetCheapHash(); uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << vchSourceGroupKey << (hash1 % ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP)).GetCheapHash(); - int new_bucket = hash2 % ADDRMAN_NEW_BUCKET_COUNT; - uint32_t mapped_as = GetMappedAS(asmap); - LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket); - return new_bucket; + return hash2 % ADDRMAN_NEW_BUCKET_COUNT; } int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int nBucket) const @@ -496,6 +490,7 @@ void AddrManImpl::ClearNew(int nUBucket, int nUBucketPos) assert(infoDelete.nRefCount > 0); infoDelete.nRefCount--; vvNew[nUBucket][nUBucketPos] = -1; + LogPrint(BCLog::ADDRMAN, "Removed %s from new[%i][%i]\n", infoDelete.ToString(), nUBucket, nUBucketPos); if (infoDelete.nRefCount == 0) { Delete(nIdDelete); } @@ -547,6 +542,8 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId) infoOld.nRefCount = 1; vvNew[nUBucket][nUBucketPos] = nIdEvict; nNew++; + LogPrint(BCLog::ADDRMAN, "Moved %s from tried[%i][%i] to new[%i][%i] to make space\n", + infoOld.ToString(), nKBucket, nKBucketPos, nUBucket, nUBucketPos); } assert(vvTried[nKBucket][nKBucketPos] == -1); @@ -597,21 +594,24 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT // Will moving this address into tried evict another entry? if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) { - // Output the entry we'd be colliding with, for debugging purposes - auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]); - if (fLogIPs) { - LogPrint(BCLog::ADDRMAN, "Collision inserting element into tried table (%s), moving %s to m_tried_collisions=%d\n", - colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", - addr.ToString(), m_tried_collisions.size()); - } if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) { m_tried_collisions.insert(nId); } + // Output the entry we'd be colliding with, for debugging purposes + auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]); + if (fLogIPs) { + LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n", + colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "", + addr.ToString(), + m_tried_collisions.size()); + } } else { - if (fLogIPs) LogPrint(BCLog::ADDRMAN, "Moving %s to tried\n", addr.ToString()); - // 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); + } } } @@ -683,6 +683,8 @@ bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTi ClearNew(nUBucket, nUBucketPos); 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); } else { if (pinfo->nRefCount == 0) { Delete(nId); @@ -741,6 +743,7 @@ std::pair AddrManImpl::Select_(bool newOnly) const assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + LogPrint(BCLog::ADDRMAN, "Selected %s from tried\n", info.ToString()); return {info, info.nLastTry}; } fChanceFactor *= 1.2; @@ -760,6 +763,7 @@ std::pair AddrManImpl::Select_(bool newOnly) const assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { + LogPrint(BCLog::ADDRMAN, "Selected %s from new\n", info.ToString()); return {info, info.nLastTry}; } fChanceFactor *= 1.2; @@ -801,7 +805,7 @@ std::vector AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct addresses.push_back(ai); } - + LogPrint(BCLog::ADDRMAN, "GetAddr returned %d random addresses\n", addresses.size()); return addresses; } From d56702aa0ced53d2f8b8844a31535193d60ff554 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 29 Sep 2021 16:22:44 -0400 Subject: [PATCH 08/12] merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first --- src/addrman.cpp | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 35834f966e..4c03fc7359 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -732,40 +732,56 @@ std::pair AddrManImpl::Select_(bool newOnly) const // use a tried node double fChanceFactor = 1.0; while (1) { + // Pick a tried bucket, and an initial position in that bucket. int nKBucket = insecure_rand.randrange(ADDRMAN_TRIED_BUCKET_COUNT); int nKBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); - while (vvTried[nKBucket][nKBucketPos] == -1) { - nKBucket = (nKBucket + insecure_rand.randbits(ADDRMAN_TRIED_BUCKET_COUNT_LOG2)) % ADDRMAN_TRIED_BUCKET_COUNT; - nKBucketPos = (nKBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE; + // Iterate over the positions of that bucket, starting at the initial one, + // and looping around. + int i; + for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { + if (vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break; } - int nId = vvTried[nKBucket][nKBucketPos]; + // If the bucket is entirely empty, start over with a (likely) different one. + if (i == ADDRMAN_BUCKET_SIZE) continue; + // Find the entry to return. + int nId = vvTried[nKBucket][(nKBucketPos + i) % ADDRMAN_BUCKET_SIZE]; const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; + // With probability GetChance() * fChanceFactor, return the entry. if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { LogPrint(BCLog::ADDRMAN, "Selected %s from tried\n", info.ToString()); return {info, info.nLastTry}; } + // Otherwise start over with a (likely) different bucket, and increased chance factor. fChanceFactor *= 1.2; } } else { // use a new node double fChanceFactor = 1.0; while (1) { + // Pick a new bucket, and an initial position in that bucket. int nUBucket = insecure_rand.randrange(ADDRMAN_NEW_BUCKET_COUNT); int nUBucketPos = insecure_rand.randrange(ADDRMAN_BUCKET_SIZE); - while (vvNew[nUBucket][nUBucketPos] == -1) { - nUBucket = (nUBucket + insecure_rand.randbits(ADDRMAN_NEW_BUCKET_COUNT_LOG2)) % ADDRMAN_NEW_BUCKET_COUNT; - nUBucketPos = (nUBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE; + // Iterate over the positions of that bucket, starting at the initial one, + // and looping around. + int i; + for (i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { + if (vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE] != -1) break; } - int nId = vvNew[nUBucket][nUBucketPos]; + // If the bucket is entirely empty, start over with a (likely) different one. + if (i == ADDRMAN_BUCKET_SIZE) continue; + // Find the entry to return. + int nId = vvNew[nUBucket][(nUBucketPos + i) % ADDRMAN_BUCKET_SIZE]; const auto it_found{mapInfo.find(nId)}; assert(it_found != mapInfo.end()); const AddrInfo& info{it_found->second}; + // With probability GetChance() * fChanceFactor, return the entry. if (insecure_rand.randbits(30) < fChanceFactor * info.GetChance() * (1 << 30)) { LogPrint(BCLog::ADDRMAN, "Selected %s from new\n", info.ToString()); return {info, info.nLastTry}; } + // Otherwise start over with a (likely) different bucket, and increased chance factor. fChanceFactor *= 1.2; } } From 1a050d6cb48200a71261d2dcacfd7bb88bbe6b43 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 3 Jun 2024 14:17:24 +0000 Subject: [PATCH 09/12] merge bitcoin#23306: Make AddrMan support multiple ports per IP --- src/addrman.cpp | 10 +++++----- src/addrman_impl.h | 4 ++-- src/netaddress.h | 35 ++++++++++++++++++----------------- src/test/addrman_tests.cpp | 27 ++++++++++++++------------- src/test/fuzz/addrman.cpp | 33 +++++++++++++++++++-------------- 5 files changed, 58 insertions(+), 51 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 4c03fc7359..55a77c8ebe 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -569,7 +569,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) - if (info != addr) + if (!m_discriminate_ports && info != addr) return; // update info @@ -707,7 +707,7 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) - if (info != addr) + if (!m_discriminate_ports && info != addr) return; // update info @@ -838,7 +838,7 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime) AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) - if (info != addr) + if (!m_discriminate_ports && info != addr) return; // update info @@ -860,7 +860,7 @@ void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices) AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) - if (info != addr) + if (!m_discriminate_ports && info != addr) return; // update info @@ -878,7 +878,7 @@ AddrInfo AddrManImpl::GetAddressInfo_(const CService& addr) AddrInfo& info = *pinfo; // check whether we are talking about the exact same CService (including same port) - if (info != addr) + if (!m_discriminate_ports && info != addr) return AddrInfo(); return *pinfo; diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 4b1d23d416..22d900c48e 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -181,8 +181,8 @@ private: //! table with information about all nIds std::unordered_map mapInfo GUARDED_BY(cs); - //! find an nId based on its network address - std::unordered_map mapAddr GUARDED_BY(cs); + //! find an nId based on its network address and port. + std::unordered_map mapAddr GUARDED_BY(cs); //! randomly-ordered vector of all nIds //! This is mutable because it is unobservable outside the class, so any diff --git a/src/netaddress.h b/src/netaddress.h index 69c90facb1..cbb10a2483 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -261,7 +261,6 @@ public: } } - friend class CNetAddrHash; friend class CSubNet; private: @@ -482,22 +481,6 @@ public: } }; -class CNetAddrHash -{ -public: - size_t operator()(const CNetAddr& a) const noexcept - { - CSipHasher hasher(m_salt_k0, m_salt_k1); - hasher.Write(a.m_net); - hasher.Write(a.m_addr.data(), a.m_addr.size()); - return static_cast(hasher.Finalize()); - } - -private: - const uint64_t m_salt_k0 = GetRand(std::numeric_limits::max()); - const uint64_t m_salt_k1 = GetRand(std::numeric_limits::max()); -}; - class CSubNet { protected: @@ -582,7 +565,25 @@ public: READWRITE(Using>(obj.port)); } + friend class CServiceHash; friend CService MaybeFlipIPv6toCJDNS(const CService& service); }; +class CServiceHash +{ +public: + size_t operator()(const CService& a) const noexcept + { + CSipHasher hasher(m_salt_k0, m_salt_k1); + hasher.Write(a.m_net); + hasher.Write(a.port); + hasher.Write(a.m_addr.data(), a.m_addr.size()); + return static_cast(hasher.Finalize()); + } + +private: + const uint64_t m_salt_k0 = GetRand(std::numeric_limits::max()); + const uint64_t m_salt_k1 = GetRand(std::numeric_limits::max()); +}; + #endif // BITCOIN_NETADDRESS_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index ca34bbc3b4..e3ff732f46 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; AddrManSerializationMock() - : AddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) + : AddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100, /* discriminate_ports */ true) {} }; @@ -82,8 +82,9 @@ private: public: explicit AddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) - : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) + std::vector asmap = std::vector(), + bool discriminatePorts = true) + : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100, discriminatePorts) { deterministic = makeDeterministic; } @@ -221,15 +222,15 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_CHECK_EQUAL(addrman.size(), 1U); CService addr1_port = ResolveService("250.1.1.1", 8334); - BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK(addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman.size(), 2U); auto addr_ret2 = addrman.Select().first; - BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333"); + BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333" || addr_ret2.ToString() == "250.1.1.1:8334"); - // Test: Add same IP but diff port to tried table, it doesn't get added. - // Perhaps this is not ideal behavior but it is the current behavior. + // Test: Add same IP but diff port to tried table; this converts the entry with + // the specified port to tried, but not the other. addrman.Good(CAddress(addr1_port, NODE_NONE)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK_EQUAL(addrman.size(), 2U); bool newOnly = true; auto addr_ret3 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); @@ -368,18 +369,18 @@ BOOST_AUTO_TEST_CASE(addrman_find) CNetAddr source2 = ResolveIP("250.1.2.2"); BOOST_CHECK(addrman.Add({addr1}, source1)); - BOOST_CHECK(!addrman.Add({addr2}, source2)); + BOOST_CHECK(addrman.Add({addr2}, source2)); BOOST_CHECK(addrman.Add({addr3}, source1)); - // Test: ensure Find returns an IP matching what we searched on. + // Test: ensure Find returns an IP/port matching what we searched on. AddrInfo* info1 = addrman.Find(addr1); BOOST_REQUIRE(info1); BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333"); - // Test 18; Find does not discriminate by port number. + // Test; Find discriminates by port number. AddrInfo* info2 = addrman.Find(addr2); BOOST_REQUIRE(info2); - BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString()); + BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:9999"); // Test: Find returns another IP matching what we searched on. AddrInfo* info3 = addrman.Find(addr3); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 44b0d1e118..b6d75ba80d 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -137,24 +137,29 @@ public: // Check that all values in `mapInfo` are equal to all values in `other.mapInfo`. // Keys may be different. - using AddrInfoHasher = std::function; - using AddrInfoEq = std::function; - - CNetAddrHash netaddr_hasher; - - AddrInfoHasher addrinfo_hasher = [&netaddr_hasher](const AddrInfo& a) { - return netaddr_hasher(static_cast(a)) ^ netaddr_hasher(a.source) ^ - a.nLastSuccess ^ a.nAttempts ^ a.nRefCount ^ a.fInTried; + auto addrinfo_hasher = [](const AddrInfo& a) { + CSipHasher hasher(0, 0); + auto addr_key = a.GetKey(); + auto source_key = a.source.GetAddrBytes(); + hasher.Write(a.nLastSuccess); + hasher.Write(a.nAttempts); + hasher.Write(a.nRefCount); + hasher.Write(a.fInTried); + hasher.Write(a.GetNetwork()); + hasher.Write(a.source.GetNetwork()); + hasher.Write(addr_key.size()); + hasher.Write(source_key.size()); + hasher.Write(addr_key.data(), addr_key.size()); + hasher.Write(source_key.data(), source_key.size()); + return (size_t)hasher.Finalize(); }; - AddrInfoEq addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) { - return static_cast(lhs) == static_cast(rhs) && - lhs.source == rhs.source && lhs.nLastSuccess == rhs.nLastSuccess && - lhs.nAttempts == rhs.nAttempts && lhs.nRefCount == rhs.nRefCount && - lhs.fInTried == rhs.fInTried; + auto addrinfo_eq = [](const AddrInfo& lhs, const AddrInfo& rhs) { + return std::tie(static_cast(lhs), lhs.source, lhs.nLastSuccess, lhs.nAttempts, lhs.nRefCount, lhs.fInTried) == + std::tie(static_cast(rhs), rhs.source, rhs.nLastSuccess, rhs.nAttempts, rhs.nRefCount, rhs.fInTried); }; - using Addresses = std::unordered_set; + using Addresses = std::unordered_set; const size_t num_addresses{m_impl->mapInfo.size()}; From 7a97aabfe00a3ae11b43dbe68ef6aadd04cb6574 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 3 Jun 2024 12:50:04 +0000 Subject: [PATCH 10/12] test: restore pre-bitcoin#23306 tests to validate port distinguishment Dash has supported the storage of address-port pairs and multi-port support before upstream for ease of development (on select networks, most notably, not mainnet). Bitcoin has introduced support for the above mentioned features and has modified tests to validate new functionality. As Dash already has this functionality and *selectively* allows it, we need to test both cases, the new upstream case with distinguishment enabled and Dash's case of distinguishment disabled (the former upstream case). --- src/test/addrman_tests.cpp | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index e3ff732f46..3b3d3ae311 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -236,6 +236,33 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); } +BOOST_AUTO_TEST_CASE(addrman_ports_nondiscriminate) +{ + AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector(), /* discriminate */ false); + + CNetAddr source = ResolveIP("252.2.2.2"); + + BOOST_CHECK_EQUAL(addrman.size(), 0U); + + // Test 7; Addr with same IP but diff port does not replace existing addr. + CService addr1 = ResolveService("250.1.1.1", 8333); + BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman.size(), 1U); + + CService addr1_port = ResolveService("250.1.1.1", 8334); + BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman.size(), 1U); + auto addr_ret2 = addrman.Select().first; + BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333"); + + // Test: Add same IP but diff port to tried table, it doesn't get added. + // Perhaps this is not ideal behavior but it is the current behavior. + addrman.Good(CAddress(addr1_port, NODE_NONE)); + BOOST_CHECK_EQUAL(addrman.size(), 1U); + bool newOnly = true; + auto addr_ret3 = addrman.Select(newOnly).first; + BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); +} BOOST_AUTO_TEST_CASE(addrman_select) { @@ -388,6 +415,39 @@ BOOST_AUTO_TEST_CASE(addrman_find) BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333"); } +BOOST_AUTO_TEST_CASE(addrman_find_nondiscriminate) +{ + AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector(), /* discriminate */ false); + + BOOST_CHECK_EQUAL(addrman.size(), 0U); + + CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); + CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE); + CAddress addr3 = CAddress(ResolveService("251.255.2.1", 8333), NODE_NONE); + + CNetAddr source1 = ResolveIP("250.1.2.1"); + CNetAddr source2 = ResolveIP("250.1.2.2"); + + BOOST_CHECK(addrman.Add({addr1}, source1)); + BOOST_CHECK(!addrman.Add({addr2}, source2)); + BOOST_CHECK(addrman.Add({addr3}, source1)); + + // Test: ensure Find returns an IP matching what we searched on. + AddrInfo* info1 = addrman.Find(addr1); + BOOST_REQUIRE(info1); + BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333"); + + // Test 18; Find does not discriminate by port number. + AddrInfo* info2 = addrman.Find(addr2); + BOOST_REQUIRE(info2); + BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString()); + + // Test: Find returns another IP matching what we searched on. + AddrInfo* info3 = addrman.Find(addr3); + BOOST_REQUIRE(info3); + BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333"); +} + BOOST_AUTO_TEST_CASE(addrman_create) { AddrManTest addrman; From d1a4b14b48bc3272224e0c22770b5e082335e6cc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 1 Jun 2024 19:33:25 +0000 Subject: [PATCH 11/12] merge bitcoin#23354: Introduce new V4 format addrman --- src/addrman.cpp | 2 +- src/addrman_impl.h | 3 ++- test/functional/feature_addrman.py | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 55a77c8ebe..285407b3f0 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -174,7 +174,7 @@ void AddrManImpl::Serialize(Stream& s_) const // Increment `lowest_compatible` iff a newly introduced format is incompatible with // the previous one. - static constexpr uint8_t lowest_compatible = Format::V3_BIP155; + static constexpr uint8_t lowest_compatible = Format::V4_MULTIPORT; s << static_cast(INCOMPATIBILITY_BASE + lowest_compatible); s << nKey; diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 22d900c48e..2e4093eeb6 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -159,6 +159,7 @@ private: V1_DETERMINISTIC = 1, //!< for pre-asmap files V2_ASMAP = 2, //!< for files including asmap version V3_BIP155 = 3, //!< same as V2_ASMAP plus addresses are in BIP155 format + V4_MULTIPORT = 4, //!< adds support for multiple ports per IP }; //! The maximum format this software knows it can unserialize. Also, we always serialize @@ -166,7 +167,7 @@ private: //! The format (first byte in the serialized stream) can be higher than this and //! still this software may be able to unserialize the file - if the second byte //! (see `lowest_compatible` in `Unserialize()`) is less or equal to this. - static constexpr Format FILE_FORMAT = Format::V3_BIP155; + static constexpr Format FILE_FORMAT = Format::V4_MULTIPORT; //! The initial value of a field that is incremented every time an incompatible format //! change is made (such that old software versions would not be able to parse and diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 9fe2da53a5..1dc1bfae51 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -17,7 +17,7 @@ from test_framework.util import assert_equal def serialize_addrman( *, format=1, - lowest_compatible=3, + lowest_compatible=4, net_magic="regtest", bucket_key=1, len_new=None, @@ -74,7 +74,7 @@ class AddrmanTest(BitcoinTestFramework): expected_msg=init_error( "Unsupported format of addrman database: 1. It is compatible with " "formats >=111, but the maximum supported by this version of " - f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)" + f"{self.config['environment']['PACKAGE_NAME']} is 4.: (.+)" ), match=ErrorMatch.FULL_REGEX, ) From a93fec6f2d48559ffbd4b06a97df6894eaf34d31 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 28 Oct 2021 12:46:26 +0100 Subject: [PATCH 12/12] merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging --- src/addrman.cpp | 167 ++++++++++++++-------------- src/addrman.h | 10 +- src/addrman_impl.h | 6 +- src/test/addrman_tests.cpp | 2 +- test/functional/p2p_addr_relay.py | 1 - test/functional/p2p_addrv2_relay.py | 3 - 6 files changed, 101 insertions(+), 88 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 285407b3f0..5664b0559a 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -552,6 +552,83 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId) info.fInTried = true; } +bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) +{ + AssertLockHeld(cs); + + if (!addr.IsRoutable()) + return false; + + int nId; + AddrInfo* pinfo = Find(addr, &nId); + + // Do not set a penalty for a source's self-announcement + if (addr == source) { + nTimePenalty = 0; + } + + if (pinfo) { + // periodically update nTime + bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60); + int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); + if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) { + pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty); + } + + // add services + pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices); + + // do not update if no new information is present + if (addr.nTime <= pinfo->nTime) { + return false; + } + + // do not update if the entry was already in the "tried" table + if (pinfo->fInTried) + return false; + + // do not update if the max reference count is reached + if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS) + return false; + + // stochastic test: previous nRefCount == N: 2^N times harder to increase it + int nFactor = 1; + for (int n = 0; n < pinfo->nRefCount; n++) + nFactor *= 2; + if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0)) + return false; + } else { + pinfo = Create(addr, source, &nId); + pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); + nNew++; + } + + int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap); + int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket); + bool fInsert = vvNew[nUBucket][nUBucketPos] == -1; + if (vvNew[nUBucket][nUBucketPos] != nId) { + if (!fInsert) { + AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; + if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) { + // Overwrite the existing new table entry. + fInsert = true; + } + } + if (fInsert) { + ClearNew(nUBucket, nUBucketPos); + 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); + } else { + if (pinfo->nRefCount == 0) { + Delete(nId); + } + } + } + return fInsert; +} + void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) { AssertLockHeld(cs); @@ -615,83 +692,16 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT } } -bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) +bool AddrManImpl::Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) { - AssertLockHeld(cs); - - if (!addr.IsRoutable()) - return false; - - bool fNew = false; - int nId; - AddrInfo* pinfo = Find(addr, &nId); - - // Do not set a penalty for a source's self-announcement - if (addr == source) { - nTimePenalty = 0; + int added{0}; + for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) { + added += AddSingle(*it, source, nTimePenalty) ? 1 : 0; } - - if (pinfo) { - // periodically update nTime - bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60); - int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); - if (pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty) { - pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty); - } - - // add services - pinfo->nServices = ServiceFlags(pinfo->nServices | addr.nServices); - - // do not update if no new information is present - if (addr.nTime <= pinfo->nTime) { - return false; - } - - // do not update if the entry was already in the "tried" table - if (pinfo->fInTried) - return false; - - // do not update if the max reference count is reached - if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS) - return false; - - // stochastic test: previous nRefCount == N: 2^N times harder to increase it - int nFactor = 1; - for (int n = 0; n < pinfo->nRefCount; n++) - nFactor *= 2; - if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0)) - return false; - } else { - pinfo = Create(addr, source, &nId); - pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty); - nNew++; - fNew = true; + if (added > 0) { + LogPrint(BCLog::ADDRMAN, "Added %i addresses (of %i) from %s: %i tried, %i new\n", added, vAddr.size(), source.ToString(), nTried, nNew); } - - int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap); - int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket); - if (vvNew[nUBucket][nUBucketPos] != nId) { - bool fInsert = vvNew[nUBucket][nUBucketPos] == -1; - if (!fInsert) { - AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]]; - if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) { - // Overwrite the existing new table entry. - fInsert = true; - } - } - if (fInsert) { - ClearNew(nUBucket, nUBucketPos); - 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); - } else { - if (pinfo->nRefCount == 0) { - Delete(nId); - } - } - } - return fNew; + return added > 0; } void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) @@ -1087,15 +1097,10 @@ size_t AddrManImpl::size() const bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) { LOCK(cs); - int nAdd = 0; Check(); - for (std::vector::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) - nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0; + auto ret = Add_(vAddr, source, nTimePenalty); Check(); - if (nAdd) { - LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew); - } - return nAdd > 0; + return ret; } void AddrManImpl::Good(const CService& addr, int64_t nTime) diff --git a/src/addrman.h b/src/addrman.h index 49ea77cdff..3515d98cae 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -70,7 +70,15 @@ public: //! Return the number of (unique) addresses in all tables. size_t size() const; - //! Add addresses to addrman's new table. + /** + * Attempt to add one or more addresses to addrman's new table. + * + * @param[in] vAddr Address records to attempt to add. + * @param[in] source The address of the node that sent us these addr records. + * @param[in] nTimePenalty A "time penalty" to apply to the address record's nTime. If a peer + * sends us an address record with nTime=n, then we'll add it to our + * addrman with nTime=(n - nTimePenalty). + * @return true if at least one address is successfully added. */ bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0); //! Mark an entry as accessible, possibly moving it from "new" to "tried". diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 2e4093eeb6..d139c1f554 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -248,9 +248,13 @@ private: //! Move an entry from the "new" table(s) to the "tried" table void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Attempt to add a single address to addrman's new table. + * @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 Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); + bool Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 3b3d3ae311..b65c36b92b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -374,7 +374,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) //Test: tried table collision! CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); uint32_t collisions{1}; - BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK(!addrman.Add({CAddress(addr1, NODE_NONE)}, source)); BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index bd8d08b78c..9437ee9621 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -148,7 +148,6 @@ class AddrTest(BitcoinTestFramework): msg = self.setup_addr_msg(num_ipv4_addrs) with self.nodes[0].assert_debug_log( [ - 'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs), 'received: addr (301 bytes) peer=1', ] ): diff --git a/test/functional/p2p_addrv2_relay.py b/test/functional/p2p_addrv2_relay.py index 0e5530259e..49984f4df3 100755 --- a/test/functional/p2p_addrv2_relay.py +++ b/test/functional/p2p_addrv2_relay.py @@ -74,9 +74,6 @@ class AddrTest(BitcoinTestFramework): addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) msg.addrs = ADDRS with self.nodes[0].assert_debug_log([ - # The I2P address is not added to node's own addrman because it has no - # I2P reachability (thus 10 - 1 = 9). - 'Added 9 addresses from 127.0.0.1: 0 tried', 'received: addrv2 (159 bytes) peer=1', ]): addr_source.send_and_ping(msg)