From c4fe6085c86de50c24838fcecc688453e722ddc3 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 13:55:34 +0000 Subject: [PATCH 01/15] merge bitcoin#22226: add unittest core dump instructions --- src/test/README.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/test/README.md b/src/test/README.md index e46152b7e8..bcdaab2c4b 100644 --- a/src/test/README.md +++ b/src/test/README.md @@ -74,3 +74,29 @@ start debugging, just like you would with any other program: ```bash gdb src/test/test_dash ``` + +#### Segmentation faults + +If you hit a segmentation fault during a test run, you can diagnose where the fault +is happening by running `gdb ./src/test/test_dash` and then using the `bt` command +within gdb. + +Another tool that can be used to resolve segmentation faults is +[valgrind](https://valgrind.org/). + +If for whatever reason you want to produce a core dump file for this fault, you can do +that as well. By default, the boost test runner will intercept system errors and not +produce a core file. To bypass this, add `--catch_system_errors=no` to the +`test_dash` arguments and ensure that your ulimits are set properly (e.g. `ulimit -c +unlimited`). + +Running the tests and hitting a segmentation fault should now produce a file called `core` +(on Linux platforms, the file name will likely depend on the contents of +`/proc/sys/kernel/core_pattern`). + +You can then explore the core dump using +``` bash +gdb src/test/test_dash core + +(gbd) bt # produce a backtrace for where a segfault occurred +``` From c28b05c5ca1d2b85687fa5e481e32208e7d48adc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 18:17:32 +0000 Subject: [PATCH 02/15] merge bitcoin#22831: add addpeeraddress "tried", test addrman checks on restart with asmap We need to continue inheriting the existing set of arguments to prevent block invalidation due to missing arguments that allow for fast DIP3 activation (will manifest as `bad-qc-premature`) --- src/rpc/client.cpp | 1 + src/rpc/net.cpp | 14 +++++++++++--- test/functional/feature_asmap.py | 29 ++++++++++++++++++++++++++-- test/functional/rpc_net.py | 33 ++++++++++++++++++++++++-------- 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index be89fea835..8af1bcf441 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -225,6 +225,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "upgradetohd", 3, "rescan"}, { "getnodeaddresses", 0, "count"}, { "addpeeraddress", 1, "port"}, + { "addpeeraddress", 2, "tried"}, { "stop", 0, "wait" }, { "verifychainlock", 2, "blockHeight" }, { "verifyislock", 3, "maxHeight" }, diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 29b99c9e51..b8bdd38939 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -970,6 +970,7 @@ static RPCHelpMan addpeeraddress() { {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address of the peer"}, {"port", RPCArg::Type::NUM, RPCArg::Optional::NO, "The port of the peer"}, + {"tried", RPCArg::Type::BOOL, /* default */ "false", "If true, attempt to add the peer to the tried addresses table"}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -978,8 +979,8 @@ static RPCHelpMan addpeeraddress() }, }, RPCExamples{ - HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999") - + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999") + HelpExampleCli("addpeeraddress", "\"1.2.3.4\" 9999 true") + + HelpExampleRpc("addpeeraddress", "\"1.2.3.4\", 9999, true") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { @@ -991,6 +992,7 @@ static RPCHelpMan addpeeraddress() const std::string& addr_string{request.params[0].get_str()}; const uint16_t port = request.params[1].get_int(); + const bool tried{request.params[2].isTrue()}; UniValue obj(UniValue::VOBJ); CNetAddr net_addr; @@ -1001,7 +1003,13 @@ static RPCHelpMan addpeeraddress() address.nTime = GetAdjustedTime(); // The source address is set equal to the address. This is equivalent to the peer // announcing itself. - if (node.addrman->Add({address}, address)) success = true; + if (node.addrman->Add({address}, address)) { + success = true; + if (tried) { + // Attempt to move the address to the tried addresses table. + node.addrman->Good(address); + } + } } obj.pushKV("success", success); diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index 0eb998fab7..029d6555c6 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -14,9 +14,11 @@ Verify node behaviour and debug log when launching dashd in these cases: 4. `dashd -asmap/-asmap=` with no file specified, using the default asmap -5. `dashd -asmap` with no file specified and a missing default asmap file +5. `dashd -asmap` restart with an addrman containing new and tried entries -6. `dashd -asmap` with an empty (unparsable) default asmap file +6. `dashd -asmap` with no file specified and a missing default asmap file + +7. `dashd -asmap` with an empty (unparsable) default asmap file The tests are order-independent. @@ -37,6 +39,12 @@ def expected_messages(filename): class AsmapTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 + self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations. + + def fill_addrman(self, node_id): + """Add 2 tried addresses to the addrman, followed by 2 new addresses.""" + for addr, tried in [[0, True], [1, True], [2, False], [3, False]]: + self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333) def test_without_asmap_arg(self): self.log.info('Test dashd with no -asmap arg passed') @@ -72,6 +80,22 @@ class AsmapTest(BitcoinTestFramework): self.start_node(0, [arg]) os.remove(self.default_asmap) + def test_asmap_interaction_with_addrman_containing_entries(self): + self.log.info("Test dashd -asmap restart with addrman containing new and tried entries") + self.stop_node(0) + shutil.copyfile(self.asmap_raw, self.default_asmap) + self.start_node(0, ["-asmap", "-checkaddrman=1"]) + self.fill_addrman(node_id=0) + self.restart_node(0, ["-asmap", "-checkaddrman=1"]) + with self.node.assert_debug_log( + expected_msgs=[ + "Addrman checks started: new 2, tried 2, total 4", + "Addrman checks completed successfully", + ] + ): + self.node.getnodeaddresses() # getnodeaddresses re-runs the addrman checks + os.remove(self.default_asmap) + def test_default_asmap_with_missing_file(self): self.log.info('Test dashd -asmap with missing default map file') self.stop_node(0) @@ -97,6 +121,7 @@ class AsmapTest(BitcoinTestFramework): self.test_asmap_with_absolute_path() self.test_asmap_with_relative_path() self.test_default_asmap() + self.test_asmap_interaction_with_addrman_containing_entries() self.test_default_asmap_with_missing_file() self.test_empty_asmap() diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index aebd9e1672..f09e6031fb 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -257,7 +257,16 @@ class NetTest(DashTestFramework): assert_raises_rpc_error(-8, "Network not recognized: Foo", self.nodes[0].getnodeaddresses, 1, "Foo") def test_addpeeraddress(self): + """RPC addpeeraddress sets the source address equal to the destination address. + If an address with the same /16 as an existing new entry is passed, it will be + placed in the same new bucket and have a 1/64 chance of the bucket positions + colliding (depending on the value of nKey in the addrman), in which case the + new address won't be added. The probability of collision can be reduced to + 1/2^16 = 1/65536 by using an address from a different /16. We avoid this here + by first testing adding a tried table entry before testing adding a new table one. + """ self.log.info("Test addpeeraddress") + self.restart_node(1, self.extra_args[1] + ["-checkaddrman=1"]) node = self.nodes[1] self.log.debug("Test that addpeerinfo is a hidden RPC") @@ -269,17 +278,25 @@ class NetTest(DashTestFramework): assert_equal(node.addpeeraddress(address="", port=8333), {"success": False}) assert_equal(node.getnodeaddresses(count=0), []) - self.log.debug("Test that adding a valid address succeeds") - assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": True}) - addrs = node.getnodeaddresses(count=0) - assert_equal(len(addrs), 1) - assert_equal(addrs[0]["address"], "1.2.3.4") - assert_equal(addrs[0]["port"], 8333) + self.log.debug("Test that adding a valid address to the tried table succeeds") + assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True}) + with node.assert_debug_log(expected_msgs=["Addrman checks started: new 0, tried 1, total 1"]): + addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks + assert_equal(len(addrs), 1) + assert_equal(addrs[0]["address"], "1.2.3.4") + assert_equal(addrs[0]["port"], 8333) - self.log.debug("Test that adding the same address again when already present fails") - assert_equal(node.addpeeraddress(address="1.2.3.4", port=8333), {"success": False}) + self.log.debug("Test that adding an already-present tried address to the new and tried tables fails") + for value in [True, False]: + assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False}) assert_equal(len(node.getnodeaddresses(count=0)), 1) + self.log.debug("Test that adding a second address, this time to the new table, succeeds") + assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), {"success": True}) + with node.assert_debug_log(expected_msgs=["Addrman checks started: new 1, tried 1, total 2"]): + addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks + assert_equal(len(addrs), 2) + if __name__ == '__main__': NetTest().main() From ba4696718eb3913bdab814d789b98d6c7f6e1f10 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 17 Sep 2021 11:58:57 +0200 Subject: [PATCH 03/15] partial bitcoin#23025: update nanobench add `-min_time` includes: - eed99cf272426e5957bee35dc8e7d0798aec8ec0 - 153e6860e84df0a3d52e5a3b2fe9c37b5e0b029a --- src/bench/addrman.cpp | 37 +++++++++++++++---------------------- src/bench/nanobench.h | 43 ++++++++++++++++++++++++------------------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index 535dad15b3..b2c98df11b 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -101,40 +101,33 @@ static void AddrManGetAddr(benchmark::Bench& bench) }); } -static void AddrManGood(benchmark::Bench& bench) +static void AddrManAddThenGood(benchmark::Bench& bench) { - /* 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); - for (size_t i{0}; i < addrman_count; ++i) { - addrmans[i] = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); - FillAddrMan(*addrmans[i]); - } - 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) { - addrman.Good(g_addresses[source_i][addr_i]); - } + addrman.Good(g_addresses[source_i][addr_i]); } } }; - uint64_t i = 0; + CreateAddresses(); + bench.run([&] { - markSomeAsGood(*addrmans.at(i)); - ++i; + // To make the benchmark independent of the number of evaluations, we always prepare a new addrman. + // This is necessary because 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. + // + // This has some overhead (exactly the result of AddrManAdd benchmark), but that overhead is constant so improvements in + // AddrMan::Good() will still be noticeable. + AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddAddressesToAddrMan(addrman); + + markSomeAsGood(addrman); }); } BENCHMARK(AddrManAdd); BENCHMARK(AddrManSelect); BENCHMARK(AddrManGetAddr); -BENCHMARK(AddrManGood); +BENCHMARK(AddrManAddThenGood); diff --git a/src/bench/nanobench.h b/src/bench/nanobench.h index 030d6ebf6a..27df08fb69 100644 --- a/src/bench/nanobench.h +++ b/src/bench/nanobench.h @@ -33,7 +33,7 @@ // see https://semver.org/ #define ANKERL_NANOBENCH_VERSION_MAJOR 4 // incompatible API changes #define ANKERL_NANOBENCH_VERSION_MINOR 3 // backwards-compatible changes -#define ANKERL_NANOBENCH_VERSION_PATCH 4 // backwards-compatible bug fixes +#define ANKERL_NANOBENCH_VERSION_PATCH 6 // backwards-compatible bug fixes /////////////////////////////////////////////////////////////////////////////////////////////////// // public facing api - as minimal as possible @@ -88,13 +88,15 @@ } while (0) #endif -#if defined(__linux__) && defined(PERF_EVENT_IOC_ID) && defined(PERF_COUNT_HW_REF_CPU_CYCLES) && defined(PERF_FLAG_FD_CLOEXEC) && \ - !defined(ANKERL_NANOBENCH_DISABLE_PERF_COUNTERS) -// only enable perf counters on kernel 3.14 which seems to have all the necessary defines. The three PERF_... defines are not in -// kernel 2.6.32 (all others are). -# define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 1 -#else -# define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 0 +#define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 0 +#if defined(__linux__) && !defined(ANKERL_NANOBENCH_DISABLE_PERF_COUNTERS) +# include +# if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 14, 0) +// PERF_COUNT_HW_REF_CPU_CYCLES only available since kernel 3.3 +// PERF_FLAG_FD_CLOEXEC since kernel 3.14 +# undef ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS +# define ANKERL_NANOBENCH_PRIVATE_PERF_COUNTERS() 1 +# endif #endif #if defined(__clang__) @@ -2210,20 +2212,20 @@ struct IterationLogic::Impl { columns.emplace_back(10, 1, "err%", "%", rErrorMedian * 100.0); double rInsMedian = -1.0; - if (mResult.has(Result::Measure::instructions)) { + if (mBench.performanceCounters() && mResult.has(Result::Measure::instructions)) { rInsMedian = mResult.median(Result::Measure::instructions); columns.emplace_back(18, 2, "ins/" + mBench.unit(), "", rInsMedian / mBench.batch()); } double rCycMedian = -1.0; - if (mResult.has(Result::Measure::cpucycles)) { + if (mBench.performanceCounters() && mResult.has(Result::Measure::cpucycles)) { rCycMedian = mResult.median(Result::Measure::cpucycles); columns.emplace_back(18, 2, "cyc/" + mBench.unit(), "", rCycMedian / mBench.batch()); } if (rInsMedian > 0.0 && rCycMedian > 0.0) { columns.emplace_back(9, 3, "IPC", "", rCycMedian <= 0.0 ? 0.0 : rInsMedian / rCycMedian); } - if (mResult.has(Result::Measure::branchinstructions)) { + if (mBench.performanceCounters() && mResult.has(Result::Measure::branchinstructions)) { double rBraMedian = mResult.median(Result::Measure::branchinstructions); columns.emplace_back(17, 2, "bra/" + mBench.unit(), "", rBraMedian / mBench.batch()); if (mResult.has(Result::Measure::branchmisses)) { @@ -2402,6 +2404,14 @@ public: return (a + divisor / 2) / divisor; } + ANKERL_NANOBENCH_NO_SANITIZE("integer", "undefined") + static inline uint32_t mix(uint32_t x) noexcept { + x ^= x << 13; + x ^= x >> 17; + x ^= x << 5; + return x; + } + template ANKERL_NANOBENCH_NO_SANITIZE("integer", "undefined") void calibrate(Op&& op) { @@ -2441,15 +2451,10 @@ public: uint64_t const numIters = 100000U + (std::random_device{}() & 3); uint64_t n = numIters; uint32_t x = 1234567; - auto fn = [&]() { - x ^= x << 13; - x ^= x >> 17; - x ^= x << 5; - }; beginMeasure(); while (n-- > 0) { - fn(); + x = mix(x); } endMeasure(); detail::doNotOptimizeAway(x); @@ -2459,8 +2464,8 @@ public: beginMeasure(); while (n-- > 0) { // we now run *twice* so we can easily calculate the overhead - fn(); - fn(); + x = mix(x); + x = mix(x); } endMeasure(); detail::doNotOptimizeAway(x); From 8d22fe9945bf31679b0ec8587285ff2cc7033e91 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 24 Sep 2021 12:26:59 +0200 Subject: [PATCH 04/15] merge bitcoin#23084: avoid non-determinism in asmap-addrman test --- test/functional/feature_asmap.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index 029d6555c6..bec3ce60f2 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -42,8 +42,8 @@ class AsmapTest(BitcoinTestFramework): self.extra_args = [["-checkaddrman=1"]] # Do addrman checks on all operations. def fill_addrman(self, node_id): - """Add 2 tried addresses to the addrman, followed by 2 new addresses.""" - for addr, tried in [[0, True], [1, True], [2, False], [3, False]]: + """Add 1 tried address to the addrman, followed by 1 new address.""" + for addr, tried in [[0, True], [1, False]]: self.nodes[node_id].addpeeraddress(address=f"101.{addr}.0.0", tried=tried, port=8333) def test_without_asmap_arg(self): @@ -89,7 +89,7 @@ class AsmapTest(BitcoinTestFramework): self.restart_node(0, ["-asmap", "-checkaddrman=1"]) with self.node.assert_debug_log( expected_msgs=[ - "Addrman checks started: new 2, tried 2, total 4", + "Addrman checks started: new 1, tried 1, total 2", "Addrman checks completed successfully", ] ): From cdc8321c4d3931164c1fa33b20c860c40851217b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 3 Sep 2024 14:57:48 +0000 Subject: [PATCH 05/15] merge bitcoin#22872: improve checkaddrman logging with duration in milliseconds --- src/addrman.cpp | 12 +++++++----- src/addrman_impl.h | 6 ++++-- src/logging/timer.h | 23 +++++++++++++++++------ test/functional/feature_asmap.py | 4 ++-- test/functional/rpc_net.py | 4 ++-- 5 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index aa05b7a098..b0ff2c50a7 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -7,6 +7,8 @@ #include #include +#include +#include #include #include #include @@ -389,7 +391,7 @@ void AddrManImpl::Unserialize(Stream& s_) LogPrint(BCLog::ADDRMAN, "addrman lost %i new and %i tried addresses due to collisions or invalid addresses\n", nLostUnk, nLost); } - const int check_code{ForceCheckAddrman()}; + const int check_code{CheckAddrman()}; if (check_code != 0) { throw DbInconsistentError(strprintf( "Corrupt data. Consistency check failed with code %s", @@ -956,18 +958,19 @@ void AddrManImpl::Check() const if (m_consistency_check_ratio == 0) return; if (insecure_rand.randrange(m_consistency_check_ratio) >= 1) return; - const int err{ForceCheckAddrman()}; + const int err{CheckAddrman()}; if (err) { LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err); assert(false); } } -int AddrManImpl::ForceCheckAddrman() const +int AddrManImpl::CheckAddrman() const { AssertLockHeld(cs); - LogPrint(BCLog::ADDRMAN, "Addrman checks started: new %i, tried %i, total %u\n", nNew, nTried, vRandom.size()); + LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE( + strprintf("new %i, tried %i, total %u", nNew, nTried, vRandom.size()), BCLog::ADDRMAN); std::unordered_set setTried; std::unordered_map mapNew; @@ -1047,7 +1050,6 @@ int AddrManImpl::ForceCheckAddrman() const if (nKey.IsNull()) return -16; - LogPrint(BCLog::ADDRMAN, "Addrman checks completed successfully\n"); return 0; } diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 1e29b69da6..090fd654cf 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -6,6 +6,7 @@ #define BITCOIN_ADDRMAN_IMPL_H #include +#include #include #include #include @@ -269,12 +270,13 @@ private: std::pair SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); - //! Consistency check, taking into account m_consistency_check_ratio. Will std::abort if an inconsistency is detected. + //! 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); + int CheckAddrman() const EXCLUSIVE_LOCKS_REQUIRED(cs); }; #endif // BITCOIN_ADDRMAN_IMPL_H diff --git a/src/logging/timer.h b/src/logging/timer.h index 80835af5e2..168c292435 100644 --- a/src/logging/timer.h +++ b/src/logging/timer.h @@ -27,10 +27,12 @@ public: Timer( std::string prefix, std::string end_msg, - BCLog::LogFlags log_category = BCLog::LogFlags::ALL) : + BCLog::LogFlags log_category = BCLog::LogFlags::ALL, + bool msg_on_completion = true) : m_prefix(std::move(prefix)), m_title(std::move(end_msg)), - m_log_category(log_category) + m_log_category(log_category), + m_message_on_completion(msg_on_completion) { this->Log(strprintf("%s started", m_title)); m_start_t = GetTime(); @@ -38,7 +40,11 @@ public: ~Timer() { - this->Log(strprintf("%s completed", m_title)); + if (m_message_on_completion) { + this->Log(strprintf("%s completed", m_title)); + } else { + this->Log("completed"); + } } void Log(const std::string& msg) @@ -74,14 +80,17 @@ private: std::chrono::microseconds m_start_t{}; //! Log prefix; usually the name of the function this was created in. - const std::string m_prefix{}; + const std::string m_prefix; //! A descriptive message of what is being timed. - const std::string m_title{}; + const std::string m_title; //! Forwarded on to LogPrint if specified - has the effect of only //! outputting the timing log when a particular debug= category is specified. - const BCLog::LogFlags m_log_category{}; + const BCLog::LogFlags m_log_category; + + //! Whether to output the message again on completion. + const bool m_message_on_completion; }; } // namespace BCLog @@ -91,6 +100,8 @@ private: BCLog::Timer UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category) #define LOG_TIME_MILLIS_WITH_CATEGORY(end_msg, log_category) \ BCLog::Timer UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category) +#define LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(end_msg, log_category) \ + BCLog::Timer UNIQUE_NAME(logging_timer)(__func__, end_msg, log_category, /* msg_on_completion=*/false) #define LOG_TIME_SECONDS(end_msg) \ BCLog::Timer UNIQUE_NAME(logging_timer)(__func__, end_msg) diff --git a/test/functional/feature_asmap.py b/test/functional/feature_asmap.py index bec3ce60f2..e6c4a50e3a 100755 --- a/test/functional/feature_asmap.py +++ b/test/functional/feature_asmap.py @@ -89,8 +89,8 @@ class AsmapTest(BitcoinTestFramework): self.restart_node(0, ["-asmap", "-checkaddrman=1"]) with self.node.assert_debug_log( expected_msgs=[ - "Addrman checks started: new 1, tried 1, total 2", - "Addrman checks completed successfully", + "CheckAddrman: new 1, tried 1, total 2 started", + "CheckAddrman: completed", ] ): self.node.getnodeaddresses() # getnodeaddresses re-runs the addrman checks diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index f09e6031fb..16114a6d38 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -280,7 +280,7 @@ class NetTest(DashTestFramework): self.log.debug("Test that adding a valid address to the tried table succeeds") assert_equal(node.addpeeraddress(address="1.2.3.4", tried=True, port=8333), {"success": True}) - with node.assert_debug_log(expected_msgs=["Addrman checks started: new 0, tried 1, total 1"]): + with node.assert_debug_log(expected_msgs=["CheckAddrman: new 0, tried 1, total 1 started"]): addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks assert_equal(len(addrs), 1) assert_equal(addrs[0]["address"], "1.2.3.4") @@ -293,7 +293,7 @@ class NetTest(DashTestFramework): self.log.debug("Test that adding a second address, this time to the new table, succeeds") assert_equal(node.addpeeraddress(address="2.0.0.0", port=8333), {"success": True}) - with node.assert_debug_log(expected_msgs=["Addrman checks started: new 1, tried 1, total 2"]): + with node.assert_debug_log(expected_msgs=["CheckAddrman: new 1, tried 1, total 2 started"]): addrs = node.getnodeaddresses(count=0) # getnodeaddresses re-runs the addrman checks assert_equal(len(addrs), 2) From aba0ebd40017b5dbad59a43a1b1462b8201be31b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 07:28:59 +0000 Subject: [PATCH 06/15] merge bitcoin#23477: tidy up unit tests --- src/addrman.h | 4 +- src/test/addrman_tests.cpp | 138 +++++++++++++++---------------------- 2 files changed, 58 insertions(+), 84 deletions(-) diff --git a/src/addrman.h b/src/addrman.h index 2d5579eee8..70c1ec12f1 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -64,6 +64,7 @@ public: */ class AddrMan { +protected: const std::unique_ptr m_impl; public: @@ -149,9 +150,6 @@ public: AddrInfo GetAddressInfo(const CService& addr); const std::vector& GetAsmap() const; - - friend class AddrManTest; - friend class AddrManDeterministic; }; #endif // BITCOIN_ADDRMAN_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 05fb06ec0a..1caab46e11 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -20,81 +20,20 @@ using namespace std::literals; -class AddrManSerializationMock : public AddrMan -{ -public: - virtual void Serialize(CDataStream& s) const = 0; - - AddrManSerializationMock() - : AddrMan(/* asmap */ std::vector(), /* deterministic */ true, /* consistency_check_ratio */ 100) - {} -}; - -class AddrManUncorrupted : public AddrManSerializationMock -{ -public: - void Serialize(CDataStream& s) const override - { - AddrMan::Serialize(s); - } -}; - -class AddrManCorrupted : public AddrManSerializationMock -{ -public: - void Serialize(CDataStream& s) const override - { - // Produces corrupt output that claims addrman has 20 addrs when it only has one addr. - unsigned char nVersion = 1; - s << nVersion; - s << ((unsigned char)32); - s << uint256::ONE; - s << 10; // nNew - s << 10; // nTried - - int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30); - s << nUBuckets; - - CService serv; - BOOST_CHECK(Lookup("252.1.1.1", serv, 7777, false)); - CAddress addr = CAddress(serv, NODE_NONE); - CNetAddr resolved; - BOOST_CHECK(LookupHost("252.2.2.2", resolved, false)); - AddrInfo info = AddrInfo(addr, resolved); - s << info; - } -}; - -static CDataStream AddrmanToStream(const AddrManSerializationMock& _addrman) -{ - CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION); - ssPeersIn << Params().MessageStart(); - ssPeersIn << _addrman; - std::string str = ssPeersIn.str(); - std::vector vchData(str.begin(), str.end()); - return CDataStream(vchData, SER_DISK, CLIENT_VERSION); -} - class AddrManTest : public AddrMan { -private: - bool deterministic; - public: - explicit AddrManTest(bool makeDeterministic = true, - std::vector asmap = std::vector()) - : AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) - { - deterministic = makeDeterministic; - } + explicit AddrManTest(std::vector asmap = std::vector()) + : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100) + {} - AddrInfo* Find(const CService& addr, int* pnId = nullptr) + AddrInfo* Find(const CService& addr) { LOCK(m_impl->cs); - return m_impl->Find(addr, pnId); + return m_impl->Find(addr); } - AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId = nullptr) + AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId) { LOCK(m_impl->cs); return m_impl->Create(addr, addrSource, pnId); @@ -758,8 +697,8 @@ 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_asmap1 = std::make_unique(asmap1); + auto addrman_asmap1_dup = std::make_unique(asmap1); auto addrman_noasmap = std::make_unique(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); @@ -790,7 +729,7 @@ 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_asmap1 = std::make_unique(asmap1); addrman_noasmap = std::make_unique(); addrman_noasmap->Add({addr}, default_source); stream << *addrman_noasmap; @@ -802,7 +741,7 @@ 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_asmap1 = std::make_unique(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); @@ -1002,9 +941,20 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } +static CDataStream AddrmanToStream(const AddrMan& addrman) +{ + CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION); + ssPeersIn << Params().MessageStart(); + ssPeersIn << addrman; + std::string str = ssPeersIn.str(); + std::vector vchData(str.begin(), str.end()); + return CDataStream(vchData, SER_DISK, CLIENT_VERSION); +} + BOOST_AUTO_TEST_CASE(load_addrman) { - AddrManUncorrupted addrmanUncorrupted; + AddrMan addrman{/*asmap=*/ std::vector(), /*deterministic=*/ true, + /*consistency_check_ratio=*/ 100}; CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -1017,11 +967,11 @@ BOOST_AUTO_TEST_CASE(load_addrman) CService source; BOOST_CHECK(Lookup("252.5.1.1", source, 8333, false)); std::vector addresses{CAddress(addr1, NODE_NONE), CAddress(addr2, NODE_NONE), CAddress(addr3, NODE_NONE)}; - BOOST_CHECK(addrmanUncorrupted.Add(addresses, source)); - BOOST_CHECK(addrmanUncorrupted.size() == 3); + BOOST_CHECK(addrman.Add(addresses, source)); + BOOST_CHECK(addrman.size() == 3); // Test that the de-serialization does not throw an exception. - CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); + CDataStream ssPeers1 = AddrmanToStream(addrman); bool exceptionThrown = false; AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); @@ -1038,7 +988,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) BOOST_CHECK(exceptionThrown == false); // Test that ReadFromStream creates an addrman with the correct number of addrs. - CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); + CDataStream ssPeers2 = AddrmanToStream(addrman); AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); @@ -1046,13 +996,39 @@ BOOST_AUTO_TEST_CASE(load_addrman) BOOST_CHECK(addrman2.size() == 3); } +// Produce a corrupt peers.dat that claims 20 addrs when it only has one addr. +static CDataStream MakeCorruptPeersDat() +{ + CDataStream s(SER_DISK, CLIENT_VERSION); + s << ::Params().MessageStart(); + + unsigned char nVersion = 1; + s << nVersion; + s << ((unsigned char)32); + s << uint256::ONE; + s << 10; // nNew + s << 10; // nTried + + int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30); + s << nUBuckets; + + CService serv; + BOOST_CHECK(Lookup("252.1.1.1", serv, 7777, false)); + CAddress addr = CAddress(serv, NODE_NONE); + CNetAddr resolved; + BOOST_CHECK(LookupHost("252.2.2.2", resolved, false)); + AddrInfo info = AddrInfo(addr, resolved); + s << info; + + std::string str = s.str(); + std::vector vchData(str.begin(), str.end()); + return CDataStream(vchData, SER_DISK, CLIENT_VERSION); +} BOOST_AUTO_TEST_CASE(load_addrman_corrupted) { - AddrManCorrupted addrmanCorrupted; - - // Test that the de-serialization of corrupted addrman throws an exception. - CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); + // Test that the de-serialization of corrupted peers.dat throws an exception. + CDataStream ssPeers1 = MakeCorruptPeersDat(); bool exceptionThrown = false; AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); @@ -1068,7 +1044,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) BOOST_CHECK(exceptionThrown); // Test that ReadFromStream fails if peers.dat is corrupt - CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); + CDataStream ssPeers2 = MakeCorruptPeersDat(); AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); From 5b5dd39f4540f05f0397a33798754731c7ac5a0e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 12 Nov 2021 12:10:10 +0100 Subject: [PATCH 07/15] merge bitcoin#23492: tidy up addrman unit tests --- src/test/addrman_tests.cpp | 49 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 1caab46e11..ce28fd16ac 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -63,14 +63,14 @@ public: // Simulates connection failure so that we can test eviction of offline nodes void SimConnFail(const CService& addr) { - int64_t nLastSuccess = 1; - // Set last good connection in the deep past. - Good(addr, nLastSuccess); + int64_t nLastSuccess = 1; + // Set last good connection in the deep past. + Good(addr, nLastSuccess); - bool count_failure = false; - int64_t nLastTry = GetAdjustedTime()-61; - Attempt(addr, count_failure, nLastTry); - } + bool count_failure = false; + int64_t nLastTry = GetAdjustedTime() - 61; + Attempt(addr, count_failure, nLastTry); + } }; static CNetAddr ResolveIP(const std::string& ip) @@ -88,7 +88,8 @@ static CService ResolveService(const std::string& ip, uint16_t port = 0) } -static std::vector FromBytes(const unsigned char* source, int vector_size) { +static std::vector FromBytes(const unsigned char* source, int vector_size) +{ std::vector result(vector_size); for (int byte_i = 0; byte_i < vector_size / 8; ++byte_i) { unsigned char cur_byte = source[byte_i]; @@ -243,15 +244,15 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) BOOST_CHECK_EQUAL(addrman.size(), num_addrs); - while (num_addrs < 22) { // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1 + while (num_addrs < 22) { // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - //Test: No collision in new table yet. + // Test: No collision in new table yet. BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } - //Test: new table collision! + // Test: new table collision! CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs)); uint32_t collisions{1}; BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); @@ -272,16 +273,16 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) BOOST_CHECK_EQUAL(addrman.size(), num_addrs); - while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1 + while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(CAddress(addr, NODE_NONE)); - //Test: No collision in tried table yet. + // Test: No collision in tried table yet. BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } - //Test: tried table collision! + // 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)); @@ -690,7 +691,6 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) // Test: IP addresses in the different source /16 prefixes sometimes map to NO MORE // than 1 bucket. BOOST_CHECK(buckets.size() == 1); - } BOOST_AUTO_TEST_CASE(addrman_serialization) @@ -811,7 +811,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) // Add twenty two addresses. CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 23; i++) { - CService addr = ResolveService("250.1.1."+ToString(i)); + CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); @@ -822,13 +822,12 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) // Ensure Good handles duplicates well. for (unsigned int i = 1; i < 23; i++) { - CService addr = ResolveService("250.1.1."+ToString(i)); + CService addr = ResolveService("250.1.1." + ToString(i)); addrman.Good(addr); BOOST_CHECK(addrman.size() == 22); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } - } BOOST_AUTO_TEST_CASE(addrman_noevict) @@ -838,7 +837,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) // Add 35 addresses. CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 36; i++) { - CService addr = ResolveService("250.1.1."+ToString(i)); + CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); @@ -861,7 +860,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) // Lets create two collisions. for (unsigned int i = 37; i < 59; i++) { - CService addr = ResolveService("250.1.1."+ToString(i)); + CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); @@ -899,7 +898,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) // Add 35 addresses CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 36; i++) { - CService addr = ResolveService("250.1.1."+ToString(i)); + CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); addrman.Good(addr); @@ -946,9 +945,7 @@ static CDataStream AddrmanToStream(const AddrMan& addrman) CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION); ssPeersIn << Params().MessageStart(); ssPeersIn << addrman; - std::string str = ssPeersIn.str(); - std::vector vchData(str.begin(), str.end()); - return CDataStream(vchData, SER_DISK, CLIENT_VERSION); + return ssPeersIn; } BOOST_AUTO_TEST_CASE(load_addrman) @@ -1020,9 +1017,7 @@ static CDataStream MakeCorruptPeersDat() AddrInfo info = AddrInfo(addr, resolved); s << info; - std::string str = s.str(); - std::vector vchData(str.begin(), str.end()); - return CDataStream(vchData, SER_DISK, CLIENT_VERSION); + return s; } BOOST_AUTO_TEST_CASE(load_addrman_corrupted) From 8b2db6bce434c466d58a386ac0fcc4d8d15b13a0 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 07:37:12 +0000 Subject: [PATCH 08/15] merge bitcoin#23713: refactor addrman_tried_collisions test to directly check for collisions --- src/addrman.cpp | 24 ++++++++++++------------ src/addrman.h | 10 ++++++++-- src/addrman_impl.h | 4 ++-- src/test/addrman_tests.cpp | 26 +++++++++++++------------- 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index b0ff2c50a7..80b753f5fe 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -616,7 +616,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_ return fInsert; } -void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) +bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime) { AssertLockHeld(cs); @@ -627,8 +627,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT AddrInfo* pinfo = Find(addr, &nId); // if not found, bail out - if (!pinfo) - return; + if (!pinfo) return false; AddrInfo& info = *pinfo; @@ -640,13 +639,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT // currently-connected peers. // if it is already in the tried set, don't do anything else - if (info.fInTried) - return; + if (info.fInTried) return false; // if it is not in new, something bad happened - if (!Assume(info.nRefCount > 0)) { - return; - } + if (!Assume(info.nRefCount > 0)) return false; + // which tried bucket to move the entry to int tried_bucket = info.GetTriedBucket(nKey, m_asmap); @@ -665,6 +662,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT addr.ToString(), m_tried_collisions.size()); } + return false; } else { // move nId to the tried tables MakeTried(info, nId); @@ -672,6 +670,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT 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); } + return true; } } @@ -1068,12 +1067,13 @@ bool AddrManImpl::Add(const std::vector& vAddr, const CNetAddr& source return ret; } -void AddrManImpl::Good(const CService& addr, int64_t nTime) +bool AddrManImpl::Good(const CService& addr, int64_t nTime) { LOCK(cs); Check(); - Good_(addr, /* test_before_evict */ true, nTime); + auto ret = Good_(addr, /*test_before_evict=*/true, nTime); Check(); + return ret; } void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) @@ -1187,9 +1187,9 @@ bool AddrMan::Add(const std::vector& vAddr, const CNetAddr& source, in return m_impl->Add(vAddr, source, nTimePenalty); } -void AddrMan::Good(const CService& addr, int64_t nTime) +bool AddrMan::Good(const CService& addr, int64_t nTime) { - m_impl->Good(addr, nTime); + return m_impl->Good(addr, nTime); } void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime) diff --git a/src/addrman.h b/src/addrman.h index 70c1ec12f1..8b5a8af738 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -92,8 +92,14 @@ public: * @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". - void Good(const CService& addr, int64_t nTime = GetAdjustedTime()); + /** + * Mark an address record as accessible and attempt to move it to addrman's tried table. + * + * @param[in] addr Address record to attempt to move to tried table. + * @param[in] nTime The time that we were last connected to this peer. + * @return true if the address is successfully moved from the new table to the tried table. + */ + bool 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()); diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 090fd654cf..78a9efb57b 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -115,7 +115,7 @@ public: bool Add(const std::vector& vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void Good(const CService& addr, int64_t nTime) + bool Good(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(!cs); void Attempt(const CService& addr, bool fCountFailure, int64_t nTime) @@ -250,7 +250,7 @@ private: * @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 Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); bool Add_(const std::vector &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index ce28fd16ac..24d47bcb47 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -265,32 +265,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) BOOST_AUTO_TEST_CASE(addrman_tried_collisions) { - AddrManTest addrman; + auto addrman = std::make_unique(std::vector(), /*deterministic=*/true, /*consistency_check_ratio=*/100); CNetAddr source = ResolveIP("252.2.2.2"); uint32_t num_addrs{0}; - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); + BOOST_CHECK_EQUAL(addrman->size(), num_addrs); - while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1 + while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(CAddress(addr, NODE_NONE)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); + + // Test: Add to tried without collision + BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE))); - // Test: No collision in tried table yet. - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); } - // Test: tried table collision! + // Test: Unable to add to tried table due to 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_EQUAL(addrman.size(), num_addrs - collisions); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Good(CAddress(addr1, NODE_NONE))); + // Test: Add the next address to tried without collision CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE))); } BOOST_AUTO_TEST_CASE(addrman_find) From c14a54089fdbb73dab0bae9de4b2c3c86e27e4b3 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 14 Dec 2021 11:26:55 +0100 Subject: [PATCH 09/15] merge bitcoin#23780: update `addrman_tests.cpp` to use output from `AddrMan::Good()` --- src/test/addrman_tests.cpp | 73 ++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 24d47bcb47..a5018b3a27 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -191,7 +191,7 @@ BOOST_AUTO_TEST_CASE(addrman_select) 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(addrman.Good(CAddress(addr1, NODE_NONE))); BOOST_CHECK_EQUAL(addrman.size(), 1U); auto addr_ret2 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0"); @@ -217,11 +217,11 @@ BOOST_AUTO_TEST_CASE(addrman_select) CService addr7 = ResolveService("250.4.6.6", 8333); BOOST_CHECK(addrman.Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - addrman.Good(CAddress(addr5, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr5, NODE_NONE))); BOOST_CHECK(addrman.Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - addrman.Good(CAddress(addr6, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr6, NODE_NONE))); BOOST_CHECK(addrman.Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); - addrman.Good(CAddress(addr7, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr7, NODE_NONE))); // Test: 6 addrs + 1 addr from last test = 7. BOOST_CHECK_EQUAL(addrman.size(), 7U); @@ -813,19 +813,21 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) for (unsigned int i = 1; i < 23; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); - // No collisions yet. - BOOST_CHECK(addrman.size() == i); + // No collisions in tried. + BOOST_CHECK(addrman.Good(addr)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Ensure Good handles duplicates well. + // If an address is a duplicate, Good will return false but will not count it as a collision. for (unsigned int i = 1; i < 23; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - addrman.Good(addr); - BOOST_CHECK(addrman.size() == 22); + // Unable to add duplicate address to tried table. + BOOST_CHECK(!addrman.Good(addr)); + + // Verify duplicate address not marked as a collision. BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } } @@ -839,22 +841,20 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); // No collision yet. - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } - // Collision between 36 and 19. + // Collision in tried table between 36 and 19. CService addr36 = ResolveService("250.1.1.36"); BOOST_CHECK(addrman.Add({CAddress(addr36, NODE_NONE)}, source)); - addrman.Good(addr36); - - BOOST_CHECK(addrman.size() == 36); + BOOST_CHECK(!addrman.Good(addr36)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0"); // 36 should be discarded and 19 not evicted. + // This means we keep 19 in the tried table and + // 36 stays in the new table. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); @@ -862,26 +862,24 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) for (unsigned int i = 37; i < 59; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); - - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } - // Cause a collision. + // Cause a collision in the tried table. CService addr59 = ResolveService("250.1.1.59"); BOOST_CHECK(addrman.Add({CAddress(addr59, NODE_NONE)}, source)); - addrman.Good(addr59); - BOOST_CHECK(addrman.size() == 59); + BOOST_CHECK(!addrman.Good(addr59)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0"); - // Cause a second collision. + // Cause a second collision in the new table. BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source)); - addrman.Good(addr36); - BOOST_CHECK(addrman.size() == 59); + // 36 still cannot be moved from new to tried due to colliding with 19 + BOOST_CHECK(!addrman.Good(addr36)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0"); + + // Resolve all collisions. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } @@ -900,19 +898,16 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); // No collision yet. - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } // Collision between 36 and 19. CService addr = ResolveService("250.1.1.36"); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); + BOOST_CHECK(!addrman.Good(addr)); - BOOST_CHECK_EQUAL(addrman.size(), 36); auto info = addrman.SelectTriedCollision().first; BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0"); @@ -923,17 +918,17 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) addrman.ResolveCollisions(); 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); - + // If 36 was swapped for 19, then adding 36 to tried should fail because we + // are attempting to add a duplicate. + // We check this by verifying Good() returns false and also verifying that + // we have no collisions. + BOOST_CHECK(!addrman.Good(addr)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); - // If we insert 19 it should collide with 36 + // 19 should fail as a collision (not a duplicate) if we now attempt to move + // it to the tried table. CService addr19 = ResolveService("250.1.1.19"); - BOOST_CHECK(!addrman.Add({CAddress(addr19, NODE_NONE)}, source)); - addrman.Good(addr19); - + BOOST_CHECK(!addrman.Good(addr19)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0"); addrman.ResolveCollisions(); From df4356546446fa4b1704adb837bd4d421be5977d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 15:13:05 +0000 Subject: [PATCH 10/15] merge bitcoin#23826: Make AddrMan unit tests use public interface, extend coverage --- src/addrman.cpp | 37 +++ src/addrman.h | 34 +++ src/addrman_impl.h | 9 +- src/test/addrman_tests.cpp | 475 ++++++++++++++++--------------------- 4 files changed, 287 insertions(+), 268 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 80b753f5fe..2caedd3f12 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -949,6 +949,29 @@ std::pair AddrManImpl::SelectTriedCollision_() return {info_old, info_old.nLastTry}; } +std::optional AddrManImpl::FindAddressEntry_(const CAddress& addr) +{ + AssertLockHeld(cs); + + AddrInfo* addr_info = Find(addr); + + if (!addr_info) return std::nullopt; + + if(addr_info->fInTried) { + int bucket{addr_info->GetTriedBucket(nKey, m_asmap)}; + return AddressPosition(/*tried=*/true, + /*multiplicity=*/1, + /*bucket=*/bucket, + /*position=*/addr_info->GetBucketPosition(nKey, false, bucket)); + } else { + int bucket{addr_info->GetNewBucket(nKey, m_asmap)}; + return AddressPosition(/*tried=*/false, + /*multiplicity=*/addr_info->nRefCount, + /*bucket=*/bucket, + /*position=*/addr_info->GetBucketPosition(nKey, true, bucket)); + } +} + void AddrManImpl::Check() const { AssertLockHeld(cs); @@ -1135,6 +1158,15 @@ void AddrManImpl::SetServices(const CService& addr, ServiceFlags nServices) Check(); } +std::optional AddrManImpl::FindAddressEntry(const CAddress& addr) +{ + LOCK(cs); + Check(); + auto entry = FindAddressEntry_(addr); + Check(); + return entry; +} + AddrInfo AddrManImpl::GetAddressInfo(const CService& addr) { AddrInfo addrRet; @@ -1236,3 +1268,8 @@ const std::vector& AddrMan::GetAsmap() const { return m_impl->GetAsmap(); } + +std::optional AddrMan::FindAddressEntry(const CAddress& addr) +{ + return m_impl->FindAddressEntry(addr); +} diff --git a/src/addrman.h b/src/addrman.h index 8b5a8af738..10b81728bf 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -23,6 +23,31 @@ class AddrManImpl; /** Default for -checkaddrman */ static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{0}; +/** Test-only struct, capturing info about an address in AddrMan */ +struct AddressPosition { + // Whether the address is in the new or tried table + const bool tried; + + // Addresses in the tried table should always have a multiplicity of 1. + // Addresses in the new table can have multiplicity between 1 and + // ADDRMAN_NEW_BUCKETS_PER_ADDRESS + const int multiplicity; + + // If the address is in the new table, the bucket and position are + // populated based on the first source who sent the address. + // In certain edge cases, this may not be where the address is currently + // located. + const int bucket; + const int position; + + bool operator==(AddressPosition other) { + return std::tie(tried, multiplicity, bucket, position) == + std::tie(other.tried, other.multiplicity, other.bucket, other.position); + } + explicit AddressPosition(bool tried_in, int multiplicity_in, int bucket_in, int position_in) + : tried{tried_in}, multiplicity{multiplicity_in}, bucket{bucket_in}, position{position_in} {} +}; + class DbInconsistentError : public std::exception { using std::exception::exception; @@ -156,6 +181,15 @@ public: AddrInfo GetAddressInfo(const CService& addr); const std::vector& GetAsmap() const; + + /** Test-only function + * Find the address record in AddrMan and return information about its + * position. + * @param[in] addr The address record to look up. + * @return Information about the address record in AddrMan + * or nullopt if address is not found. + */ + std::optional FindAddressEntry(const CAddress& addr); }; #endif // BITCOIN_ADDRMAN_H diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 78a9efb57b..dab89b752d 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -137,11 +137,14 @@ public: void SetServices(const CService& addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(!cs); - AddrInfo GetAddressInfo(const CService& addr); + std::optional FindAddressEntry(const CAddress& addr) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + + AddrInfo GetAddressInfo(const CService& addr) + EXCLUSIVE_LOCKS_REQUIRED(!cs); const std::vector& GetAsmap() const; - friend class AddrManTest; friend class AddrManDeterministic; private: @@ -270,6 +273,8 @@ private: std::pair SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional FindAddressEntry_(const CAddress& addr) 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); diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index a5018b3a27..534a3b7152 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -20,59 +20,6 @@ using namespace std::literals; -class AddrManTest : public AddrMan -{ -public: - explicit AddrManTest(std::vector asmap = std::vector()) - : AddrMan(asmap, /*deterministic=*/true, /* consistency_check_ratio */ 100) - {} - - AddrInfo* Find(const CService& addr) - { - LOCK(m_impl->cs); - return m_impl->Find(addr); - } - - AddrInfo* Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId) - { - LOCK(m_impl->cs); - return m_impl->Create(addr, addrSource, pnId); - } - - void Delete(int nId) - { - LOCK(m_impl->cs); - m_impl->Delete(nId); - } - - // Used to test deserialization - std::pair GetBucketAndEntry(const CAddress& 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 == m_impl->vvNew[bucket][entry]) { - return std::pair(bucket, entry); - } - } - } - return std::pair(-1, -1); - } - - // Simulates connection failure so that we can test eviction of offline nodes - void SimConnFail(const CService& addr) - { - int64_t nLastSuccess = 1; - // Set last good connection in the deep past. - Good(addr, nLastSuccess); - - bool count_failure = false; - int64_t nLastTry = GetAdjustedTime() - 61; - Attempt(addr, count_failure, nLastTry); - } -}; - static CNetAddr ResolveIP(const std::string& ip) { CNetAddr addr; @@ -100,12 +47,17 @@ static std::vector FromBytes(const unsigned char* source, int vector_size) return result; } +/* Utility function to create a deterministic addrman, as used in most tests */ +static std::unique_ptr TestAddrMan(std::vector asmap = std::vector()) +{ + return std::make_unique(asmap, /*deterministic=*/true, /*consistency_check_ratio=*/100); +} BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(addrman_simple) { - auto addrman = std::make_unique(); + auto addrman = TestAddrMan(); CNetAddr source = ResolveIP("252.2.2.2"); @@ -139,7 +91,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 = TestAddrMan(); 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)); @@ -149,57 +101,57 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_AUTO_TEST_CASE(addrman_ports) { - AddrManTest addrman; + auto addrman = TestAddrMan(); CNetAddr source = ResolveIP("252.2.2.2"); - BOOST_CHECK_EQUAL(addrman.size(), 0U); + 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); + 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(), 2U); - auto addr_ret2 = addrman.Select().first; + BOOST_CHECK(addrman->Add({CAddress(addr1_port, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman->size(), 2U); + auto addr_ret2 = addrman->Select().first; 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; 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(), 2U); + addrman->Good(CAddress(addr1_port, NODE_NONE)); + BOOST_CHECK_EQUAL(addrman->size(), 2U); bool newOnly = true; - auto addr_ret3 = addrman.Select(newOnly).first; + auto addr_ret3 = addrman->Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); } BOOST_AUTO_TEST_CASE(addrman_select) { - AddrManTest addrman; + auto addrman = TestAddrMan(); CNetAddr source = ResolveIP("252.2.2.2"); // Test: Select from new with 1 addr in new. CService addr1 = ResolveService("250.1.1.1", 8333); - BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman->size(), 1U); bool newOnly = true; - auto addr_ret1 = addrman.Select(newOnly).first; + 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. - BOOST_CHECK(addrman.Good(CAddress(addr1, NODE_NONE))); - BOOST_CHECK_EQUAL(addrman.size(), 1U); - auto addr_ret2 = addrman.Select(newOnly).first; + BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); + BOOST_CHECK_EQUAL(addrman->size(), 1U); + auto addr_ret2 = addrman->Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0"); - auto addr_ret3 = addrman.Select().first; + auto addr_ret3 = addrman->Select().first; BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333"); - BOOST_CHECK_EQUAL(addrman.size(), 1U); + BOOST_CHECK_EQUAL(addrman->size(), 1U); // Add three addresses to new table. @@ -207,65 +159,97 @@ BOOST_AUTO_TEST_CASE(addrman_select) CService addr3 = ResolveService("250.3.2.2", 9999); CService addr4 = ResolveService("250.3.3.3", 9999); - BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman.Add({CAddress(addr3, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman.Add({CAddress(addr4, NODE_NONE)}, ResolveService("250.4.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr3, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Add({CAddress(addr4, NODE_NONE)}, ResolveService("250.4.1.1", 8333))); // Add three addresses to tried table. CService addr5 = ResolveService("250.4.4.4", 8333); CService addr6 = ResolveService("250.4.5.5", 7777); CService addr7 = ResolveService("250.4.6.6", 8333); - BOOST_CHECK(addrman.Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman.Good(CAddress(addr5, NODE_NONE))); - BOOST_CHECK(addrman.Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - BOOST_CHECK(addrman.Good(CAddress(addr6, NODE_NONE))); - BOOST_CHECK(addrman.Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); - BOOST_CHECK(addrman.Good(CAddress(addr7, NODE_NONE))); + BOOST_CHECK(addrman->Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Good(CAddress(addr5, NODE_NONE))); + BOOST_CHECK(addrman->Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); + BOOST_CHECK(addrman->Good(CAddress(addr6, NODE_NONE))); + BOOST_CHECK(addrman->Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); + BOOST_CHECK(addrman->Good(CAddress(addr7, NODE_NONE))); // Test: 6 addrs + 1 addr from last test = 7. - BOOST_CHECK_EQUAL(addrman.size(), 7U); + BOOST_CHECK_EQUAL(addrman->size(), 7U); // 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().first.GetPort()); + ports.insert(addrman->Select().first.GetPort()); } BOOST_CHECK_EQUAL(ports.size(), 3U); } BOOST_AUTO_TEST_CASE(addrman_new_collisions) { - AddrManTest addrman; + auto addrman = TestAddrMan(); CNetAddr source = ResolveIP("252.2.2.2"); uint32_t num_addrs{0}; - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); + BOOST_CHECK_EQUAL(addrman->size(), num_addrs); while (num_addrs < 22) { // Magic number! 250.1.1.1 - 250.1.1.22 do not collide with deterministic key = 1 CService addr = ResolveService("250.1.1." + ToString(++num_addrs)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); // Test: No collision in new table yet. - BOOST_CHECK_EQUAL(addrman.size(), num_addrs); + BOOST_CHECK_EQUAL(addrman->size(), num_addrs); } // Test: new 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_EQUAL(addrman.size(), num_addrs - collisions); + 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)); - BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source)); - BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions); + BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source)); + BOOST_CHECK_EQUAL(addrman->size(), num_addrs - collisions); +} + +BOOST_AUTO_TEST_CASE(addrman_new_multiplicity) +{ + auto addrman = TestAddrMan(); + CAddress addr{CAddress(ResolveService("253.3.3.3", 8333), NODE_NONE)}; + int64_t start_time{GetAdjustedTime()}; + addr.nTime = start_time; + + // test that multiplicity stays at 1 if nTime doesn't increase + for (unsigned int i = 1; i < 20; ++i) { + std::string addr_ip{ToString(i % 256) + "." + ToString(i >> 8 % 256) + ".1.1"}; + CNetAddr source{ResolveIP(addr_ip)}; + addrman->Add({addr}, source); + } + AddressPosition addr_pos = addrman->FindAddressEntry(addr).value(); + BOOST_CHECK_EQUAL(addr_pos.multiplicity, 1U); + BOOST_CHECK_EQUAL(addrman->size(), 1U); + + // if nTime increases, an addr can occur in up to 8 buckets + // The acceptance probability decreases exponentially with existing multiplicity - + // choose number of iterations such that it gets to 8 with deterministic addrman. + for (unsigned int i = 1; i < 400; ++i) { + std::string addr_ip{ToString(i % 256) + "." + ToString(i >> 8 % 256) + ".1.1"}; + CNetAddr source{ResolveIP(addr_ip)}; + addr.nTime = start_time + i; + addrman->Add({addr}, source); + } + AddressPosition addr_pos_multi = addrman->FindAddressEntry(addr).value(); + BOOST_CHECK_EQUAL(addr_pos_multi.multiplicity, 8U); + // multiplicity doesn't affect size + BOOST_CHECK_EQUAL(addrman->size(), 1U); } BOOST_AUTO_TEST_CASE(addrman_tried_collisions) { - auto addrman = std::make_unique(std::vector(), /*deterministic=*/true, /*consistency_check_ratio=*/100); + auto addrman = TestAddrMan(); CNetAddr source = ResolveIP("252.2.2.2"); @@ -293,87 +277,15 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE))); } -BOOST_AUTO_TEST_CASE(addrman_find) -{ - AddrManTest addrman; - - 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/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; Find discriminates by port number. - AddrInfo* info2 = addrman.Find(addr2); - BOOST_REQUIRE(info2); - 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); - BOOST_REQUIRE(info3); - BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333"); -} - -BOOST_AUTO_TEST_CASE(addrman_create) -{ - AddrManTest addrman; - - BOOST_CHECK_EQUAL(addrman.size(), 0U); - - CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); - CNetAddr source1 = ResolveIP("250.1.2.1"); - - int 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"); - - AddrInfo* info2 = addrman.Find(addr1); - BOOST_CHECK_EQUAL(info2->ToString(), "250.1.2.1:8333"); -} - - -BOOST_AUTO_TEST_CASE(addrman_delete) -{ - AddrManTest addrman; - - BOOST_CHECK_EQUAL(addrman.size(), 0U); - - CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); - CNetAddr source1 = ResolveIP("250.1.2.1"); - - int nId; - addrman.Create(addr1, source1, &nId); - - // Test: Delete should actually delete the addr. - BOOST_CHECK_EQUAL(addrman.size(), 1U); - addrman.Delete(nId); - BOOST_CHECK_EQUAL(addrman.size(), 0U); - AddrInfo* info2 = addrman.Find(addr1); - BOOST_CHECK(info2 == nullptr); -} BOOST_AUTO_TEST_CASE(addrman_getaddr) { - AddrManTest addrman; + auto addrman = TestAddrMan(); // Test: Sanity check, GetAddr should never return anything if addrman // is empty. - BOOST_CHECK_EQUAL(addrman.size(), 0U); - std::vector vAddr1 = addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt); + BOOST_CHECK_EQUAL(addrman->size(), 0U); + std::vector vAddr1 = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt); BOOST_CHECK_EQUAL(vAddr1.size(), 0U); CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE); @@ -390,18 +302,18 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr) CNetAddr source2 = ResolveIP("250.2.3.3"); // Test: Ensure GetAddr works with new addresses. - BOOST_CHECK(addrman.Add({addr1, addr3, addr5}, source1)); - BOOST_CHECK(addrman.Add({addr2, addr4}, source2)); + BOOST_CHECK(addrman->Add({addr1, addr3, addr5}, source1)); + BOOST_CHECK(addrman->Add({addr2, addr4}, source2)); - BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt).size(), 5U); + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 5U); // Net processing asks for 23% of addresses. 23% of 5 is 1 rounded down. - BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt).size(), 1U); + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/2500, /*max_pct=*/23, /*network=*/std::nullopt).size(), 1U); // Test: Ensure GetAddr works with new and tried addresses. - addrman.Good(CAddress(addr1, NODE_NONE)); - addrman.Good(CAddress(addr2, NODE_NONE)); - BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 0, /* max_pct */ 0, /* network */ std::nullopt).size(), 5U); - BOOST_CHECK_EQUAL(addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt).size(), 1U); + BOOST_CHECK(addrman->Good(CAddress(addr1, NODE_NONE))); + BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE))); + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 5U); + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/2500, /*max_pct=*/23, /*network=*/std::nullopt).size(), 1U); // Test: Ensure GetAddr still returns 23% when addrman has many addrs. for (unsigned int i = 1; i < (8 * 256); i++) { @@ -412,24 +324,22 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr) // Ensure that for all addrs in addrman, isTerrible == false. addr.nTime = GetAdjustedTime(); - addrman.Add({addr}, ResolveIP(strAddr)); + addrman->Add({addr}, ResolveIP(strAddr)); if (i % 8 == 0) - addrman.Good(addr); + addrman->Good(addr); } - std::vector vAddr = addrman.GetAddr(/* max_addresses */ 2500, /* max_pct */ 23, /* network */ std::nullopt); + std::vector vAddr = addrman->GetAddr(/*max_addresses=*/2500, /*max_pct=*/23, /*network=*/std::nullopt); - size_t percent23 = (addrman.size() * 23) / 100; + size_t percent23 = (addrman->size() * 23) / 100; BOOST_CHECK_EQUAL(vAddr.size(), percent23); BOOST_CHECK_EQUAL(vAddr.size(), 461U); // (Addrman.size() < number of addresses added) due to address collisions. - BOOST_CHECK_EQUAL(addrman.size(), 2006U); + BOOST_CHECK_EQUAL(addrman->size(), 2006U); } BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) { - AddrManTest addrman; - CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.1.1", 9999), NODE_NONE); @@ -483,8 +393,6 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) { - AddrManTest addrman; - CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE); @@ -561,8 +469,6 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket_legacy) // 101.8.0.0/16 AS8 BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) { - AddrManTest addrman; - CAddress addr1 = CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.1.1", 9999), NODE_NONE); @@ -616,8 +522,6 @@ BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket) BOOST_AUTO_TEST_CASE(caddrinfo_get_new_bucket) { - AddrManTest addrman; - CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE); CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE); @@ -697,72 +601,69 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(raw_tests::asmap, sizeof(raw_tests::asmap) * 8); - auto addrman_asmap1 = std::make_unique(asmap1); - auto addrman_asmap1_dup = std::make_unique(asmap1); - auto addrman_noasmap = std::make_unique(); + auto addrman_asmap1 = TestAddrMan(asmap1); + auto addrman_asmap1_dup = TestAddrMan(asmap1); + auto addrman_noasmap = TestAddrMan(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); CAddress addr = CAddress(ResolveService("250.1.1.1"), NODE_NONE); CNetAddr default_source; - addrman_asmap1->Add({addr}, default_source); stream << *addrman_asmap1; // serizalizing/deserializing addrman with the same asmap stream >> *addrman_asmap1_dup; - std::pair bucketAndEntry_asmap1 = addrman_asmap1->GetBucketAndEntry(addr); - std::pair bucketAndEntry_asmap1_dup = addrman_asmap1_dup->GetBucketAndEntry(addr); - BOOST_CHECK(bucketAndEntry_asmap1.second != -1); - BOOST_CHECK(bucketAndEntry_asmap1_dup.second != -1); + AddressPosition addr_pos1 = addrman_asmap1->FindAddressEntry(addr).value(); + AddressPosition addr_pos2 = addrman_asmap1_dup->FindAddressEntry(addr).value(); + BOOST_CHECK(addr_pos1.multiplicity != 0); + BOOST_CHECK(addr_pos2.multiplicity != 0); - BOOST_CHECK(bucketAndEntry_asmap1.first == bucketAndEntry_asmap1_dup.first); - BOOST_CHECK(bucketAndEntry_asmap1.second == bucketAndEntry_asmap1_dup.second); + BOOST_CHECK(addr_pos1 == addr_pos2); // deserializing asmaped peers.dat to non-asmaped addrman stream << *addrman_asmap1; stream >> *addrman_noasmap; - std::pair bucketAndEntry_noasmap = addrman_noasmap->GetBucketAndEntry(addr); - BOOST_CHECK(bucketAndEntry_noasmap.second != -1); - BOOST_CHECK(bucketAndEntry_asmap1.first != bucketAndEntry_noasmap.first); - BOOST_CHECK(bucketAndEntry_asmap1.second != bucketAndEntry_noasmap.second); + AddressPosition addr_pos3 = addrman_noasmap->FindAddressEntry(addr).value(); + BOOST_CHECK(addr_pos3.multiplicity != 0); + BOOST_CHECK(addr_pos1.bucket != addr_pos3.bucket); + BOOST_CHECK(addr_pos1.position != addr_pos3.position); // deserializing non-asmaped peers.dat to asmaped addrman - addrman_asmap1 = std::make_unique(asmap1); - addrman_noasmap = std::make_unique(); + addrman_asmap1 = TestAddrMan(asmap1); + addrman_noasmap = TestAddrMan(); addrman_noasmap->Add({addr}, default_source); stream << *addrman_noasmap; stream >> *addrman_asmap1; - std::pair bucketAndEntry_asmap1_deser = addrman_asmap1->GetBucketAndEntry(addr); - BOOST_CHECK(bucketAndEntry_asmap1_deser.second != -1); - BOOST_CHECK(bucketAndEntry_asmap1_deser.first != bucketAndEntry_noasmap.first); - BOOST_CHECK(bucketAndEntry_asmap1_deser.first == bucketAndEntry_asmap1_dup.first); - BOOST_CHECK(bucketAndEntry_asmap1_deser.second == bucketAndEntry_asmap1_dup.second); + + AddressPosition addr_pos4 = addrman_asmap1->FindAddressEntry(addr).value(); + BOOST_CHECK(addr_pos4.multiplicity != 0); + BOOST_CHECK(addr_pos4.bucket != addr_pos3.bucket); + BOOST_CHECK(addr_pos4 == addr_pos2); // used to map to different buckets, now maps to the same bucket. - addrman_asmap1 = std::make_unique(asmap1); - addrman_noasmap = std::make_unique(); + addrman_asmap1 = TestAddrMan(asmap1); + addrman_noasmap = TestAddrMan(); 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); - std::pair bucketAndEntry_noasmap_addr1 = addrman_noasmap->GetBucketAndEntry(addr1); - std::pair bucketAndEntry_noasmap_addr2 = addrman_noasmap->GetBucketAndEntry(addr2); - BOOST_CHECK(bucketAndEntry_noasmap_addr1.first != bucketAndEntry_noasmap_addr2.first); - BOOST_CHECK(bucketAndEntry_noasmap_addr1.second != bucketAndEntry_noasmap_addr2.second); + AddressPosition addr_pos5 = addrman_noasmap->FindAddressEntry(addr1).value(); + AddressPosition addr_pos6 = addrman_noasmap->FindAddressEntry(addr2).value(); + BOOST_CHECK(addr_pos5.bucket != addr_pos6.bucket); stream << *addrman_noasmap; stream >> *addrman_asmap1; - std::pair bucketAndEntry_asmap1_deser_addr1 = addrman_asmap1->GetBucketAndEntry(addr1); - std::pair bucketAndEntry_asmap1_deser_addr2 = addrman_asmap1->GetBucketAndEntry(addr2); - BOOST_CHECK(bucketAndEntry_asmap1_deser_addr1.first == bucketAndEntry_asmap1_deser_addr2.first); - BOOST_CHECK(bucketAndEntry_asmap1_deser_addr1.second != bucketAndEntry_asmap1_deser_addr2.second); + AddressPosition addr_pos7 = addrman_asmap1->FindAddressEntry(addr1).value(); + AddressPosition addr_pos8 = addrman_asmap1->FindAddressEntry(addr2).value(); + BOOST_CHECK(addr_pos7.bucket == addr_pos8.bucket); + BOOST_CHECK(addr_pos7.position != addr_pos8.position); } BOOST_AUTO_TEST_CASE(remove_invalid) { // Confirm that invalid addresses are ignored in unserialization. - auto addrman = std::make_unique(); + auto addrman = TestAddrMan(); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE}; @@ -794,29 +695,29 @@ 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 = TestAddrMan(); stream >> *addrman; BOOST_CHECK_EQUAL(addrman->size(), 2); } BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) { - AddrManTest addrman; + auto addrman = TestAddrMan(); - BOOST_CHECK(addrman.size() == 0); + BOOST_CHECK(addrman->size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); // Add twenty two addresses. CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 23; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); // No collisions in tried. - BOOST_CHECK(addrman.Good(addr)); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman->Good(addr)); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); } // Ensure Good handles duplicates well. @@ -825,114 +726,125 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) CService addr = ResolveService("250.1.1." + ToString(i)); // Unable to add duplicate address to tried table. - BOOST_CHECK(!addrman.Good(addr)); + BOOST_CHECK(!addrman->Good(addr)); // Verify duplicate address not marked as a collision. - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); } } BOOST_AUTO_TEST_CASE(addrman_noevict) { - AddrManTest addrman; + auto addrman = TestAddrMan(); // Add 35 addresses. CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); // No collision yet. - BOOST_CHECK(addrman.Good(addr)); + BOOST_CHECK(addrman->Good(addr)); } // Collision in tried table between 36 and 19. CService addr36 = ResolveService("250.1.1.36"); - BOOST_CHECK(addrman.Add({CAddress(addr36, NODE_NONE)}, source)); - BOOST_CHECK(!addrman.Good(addr36)); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0"); + BOOST_CHECK(addrman->Add({CAddress(addr36, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Good(addr36)); + BOOST_CHECK_EQUAL(addrman->SelectTriedCollision().first.ToString(), "250.1.1.19:0"); // 36 should be discarded and 19 not evicted. // This means we keep 19 in the tried table and // 36 stays in the new table. - addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + addrman->ResolveCollisions(); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); // Lets create two collisions. for (unsigned int i = 37; i < 59; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - BOOST_CHECK(addrman.Good(addr)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Good(addr)); } // Cause a collision in the tried table. CService addr59 = ResolveService("250.1.1.59"); - BOOST_CHECK(addrman.Add({CAddress(addr59, NODE_NONE)}, source)); - BOOST_CHECK(!addrman.Good(addr59)); + BOOST_CHECK(addrman->Add({CAddress(addr59, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Good(addr59)); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0"); + BOOST_CHECK_EQUAL(addrman->SelectTriedCollision().first.ToString(), "250.1.1.10:0"); // Cause a second collision in the new table. - BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Add({CAddress(addr36, NODE_NONE)}, source)); // 36 still cannot be moved from new to tried due to colliding with 19 - BOOST_CHECK(!addrman.Good(addr36)); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0"); + BOOST_CHECK(!addrman->Good(addr36)); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() != "[::]:0"); // Resolve all collisions. - addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + addrman->ResolveCollisions(); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); } BOOST_AUTO_TEST_CASE(addrman_evictionworks) { - AddrManTest addrman; + auto addrman = TestAddrMan(); - BOOST_CHECK(addrman.size() == 0); + BOOST_CHECK(addrman->size() == 0); // Empty addrman should return blank addrman info. - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); // Add 35 addresses CNetAddr source = ResolveIP("252.2.2.2"); for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); // No collision yet. - BOOST_CHECK(addrman.Good(addr)); + BOOST_CHECK(addrman->Good(addr)); } // Collision between 36 and 19. CService addr = ResolveService("250.1.1.36"); - BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - BOOST_CHECK(!addrman.Good(addr)); + BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source)); + BOOST_CHECK(!addrman->Good(addr)); - auto info = addrman.SelectTriedCollision().first; + 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. - addrman.SimConnFail(info); + // Update entry in tried by setting last good connection in the deep past. + BOOST_CHECK(!addrman->Good(info, /*nTime=*/1)); + addrman->Attempt(info, /*fCountFailure=*/false, /*nTime=*/GetAdjustedTime() - 61); // Should swap 36 for 19. - addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + addrman->ResolveCollisions(); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); + AddressPosition addr_pos{addrman->FindAddressEntry(CAddress(addr, NODE_NONE)).value()}; + BOOST_CHECK(addr_pos.tried); // If 36 was swapped for 19, then adding 36 to tried should fail because we // are attempting to add a duplicate. // We check this by verifying Good() returns false and also verifying that // we have no collisions. - BOOST_CHECK(!addrman.Good(addr)); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(!addrman->Good(addr)); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); // 19 should fail as a collision (not a duplicate) if we now attempt to move // it to the tried table. CService addr19 = ResolveService("250.1.1.19"); - BOOST_CHECK(!addrman.Good(addr19)); - BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0"); + BOOST_CHECK(!addrman->Good(addr19)); + BOOST_CHECK_EQUAL(addrman->SelectTriedCollision().first.ToString(), "250.1.1.36:0"); - addrman.ResolveCollisions(); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + // Eviction is also successful if too much time has passed since last try + SetMockTime(GetTime() + 4 * 60 *60); + addrman->ResolveCollisions(); + BOOST_CHECK(addrman->SelectTriedCollision().first.ToString() == "[::]:0"); + //Now 19 is in tried again, and 36 back to new + AddressPosition addr_pos19{addrman->FindAddressEntry(CAddress(addr19, NODE_NONE)).value()}; + BOOST_CHECK(addr_pos19.tried); + AddressPosition addr_pos36{addrman->FindAddressEntry(CAddress(addr, NODE_NONE)).value()}; + BOOST_CHECK(!addr_pos36.tried); } static CDataStream AddrmanToStream(const AddrMan& addrman) @@ -1041,4 +953,35 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure); } +BOOST_AUTO_TEST_CASE(addrman_update_address) +{ + // Tests updating nTime via Connected() and nServices via SetServices() + auto addrman = TestAddrMan(); + CNetAddr source{ResolveIP("252.2.2.2")}; + CAddress addr{CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE)}; + + int64_t start_time{GetAdjustedTime() - 10000}; + addr.nTime = start_time; + BOOST_CHECK(addrman->Add({addr}, source)); + BOOST_CHECK_EQUAL(addrman->size(), 1U); + + // Updating an addrman entry with a different port doesn't change it + CAddress addr_diff_port{CAddress(ResolveService("250.1.1.1", 8334), NODE_NONE)}; + addr_diff_port.nTime = start_time; + addrman->Connected(addr_diff_port); + addrman->SetServices(addr_diff_port, NODE_NETWORK_LIMITED); + std::vector vAddr1{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)}; + BOOST_CHECK_EQUAL(vAddr1.size(), 1U); + BOOST_CHECK_EQUAL(vAddr1.at(0).nTime, start_time); + BOOST_CHECK_EQUAL(vAddr1.at(0).nServices, NODE_NONE); + + // Updating an addrman entry with the correct port is successful + addrman->Connected(addr); + addrman->SetServices(addr, NODE_NETWORK_LIMITED); + std::vector vAddr2 = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt); + BOOST_CHECK_EQUAL(vAddr2.size(), 1U); + BOOST_CHECK(vAddr2.at(0).nTime >= start_time + 10000); + BOOST_CHECK_EQUAL(vAddr2.at(0).nServices, NODE_NETWORK_LIMITED); +} + BOOST_AUTO_TEST_SUITE_END() From b30f0fa4417ccb29314551ca74f660eb61206f25 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 15:16:42 +0000 Subject: [PATCH 11/15] test: remove `connman` local from `BasicTestingSetup` `NodeContext` member was moved from `TestingSetup` to `BasicTestingSetup` in bitcoin#18571 but `connman` (which was a remnant from when `NodeContext` was absent) wasn't removed. Let's resolve that. --- src/test/util/setup_common.cpp | 15 +++++++-------- src/test/util/setup_common.h | 1 - 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 0c3d69b35d..135c0c89fc 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -177,8 +177,11 @@ 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.chain = interfaces::MakeChain(m_node); + + m_node.addrman = std::make_unique(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. + // while g_wallet_init_interface is init here at very early stage // we can't get rid of unique_ptr from wallet/contex.h // TODO: remove unique_ptr from wallet/context.h after bitcoin/bitcoin#22219 @@ -186,7 +189,6 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve fCheckBlockIndex = true; m_node.evodb = std::make_unique(1 << 20, true, true); m_node.mnhf_manager = std::make_unique(*m_node.evodb); - connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); llmq::quorumSnapshotManager.reset(new llmq::CQuorumSnapshotManager(*m_node.evodb)); m_node.cpoolman = std::make_unique(*m_node.evodb); static bool noui_connected = false; @@ -200,11 +202,12 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve BasicTestingSetup::~BasicTestingSetup() { SetMockTime(0s); // Reset mocktime for following tests - connman.reset(); - llmq::quorumSnapshotManager.reset(); m_node.cpoolman.reset(); + llmq::quorumSnapshotManager.reset(); m_node.mnhf_manager.reset(); m_node.evodb.reset(); + m_node.connman.reset(); + m_node.addrman.reset(); LogInstance().DisconnectTestLogger(); fs::remove_all(m_path_root); @@ -227,8 +230,6 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve m_node.chainman = std::make_unique(); m_node.chainman->m_blockman.m_block_tree_db = std::make_unique(1 << 20, true); - m_node.connman = std::make_unique(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. - m_node.mn_metaman = std::make_unique(); m_node.netfulfilledman = std::make_unique(); m_node.sporkman = std::make_unique(); @@ -252,8 +253,6 @@ ChainTestingSetup::~ChainTestingSetup() m_node.sporkman.reset(); m_node.netfulfilledman.reset(); m_node.mn_metaman.reset(); - m_node.connman.reset(); - m_node.addrman.reset(); m_node.args = nullptr; m_node.mempool.reset(); m_node.scheduler.reset(); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 28af81f0f1..f819c4d8d9 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -88,7 +88,6 @@ struct BasicTestingSetup { explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector& extra_args = {}); ~BasicTestingSetup(); - std::unique_ptr connman; const fs::path m_path_root; ArgsManager m_args; }; From cdcaf2278c7fb091457849ba692bbb0fb432c5a9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 1 Sep 2024 18:17:00 +0000 Subject: [PATCH 12/15] merge bitcoin#23373: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change --- doc/fuzzing.md | 9 +++++ src/bench/addrman.cpp | 11 ++++-- src/bench/bench.cpp | 2 + src/qt/test/test_main.cpp | 3 ++ src/test/README.md | 28 ++++++++++---- src/test/addrman_tests.cpp | 70 ++++++++++++++++++---------------- src/test/fuzz/addrman.cpp | 18 +++++++-- src/test/fuzz/connman.cpp | 10 ++++- src/test/fuzz/deserialize.cpp | 12 +++++- src/test/fuzz/fuzz.cpp | 25 ++++++++++++ src/test/main.cpp | 15 ++++++++ src/test/util/setup_common.cpp | 17 ++++++--- src/test/util/setup_common.h | 4 ++ 13 files changed, 169 insertions(+), 55 deletions(-) diff --git a/doc/fuzzing.md b/doc/fuzzing.md index f7fac188ce..31d5de130c 100644 --- a/doc/fuzzing.md +++ b/doc/fuzzing.md @@ -64,6 +64,15 @@ block^@M-^?M-^?M-^?M-^?M-^?nM-^?M-^? In this case the fuzzer managed to create a `block` message which when passed to `ProcessMessage(...)` increased coverage. +It is possible to specify `dashd` arguments to the `fuzz` executable. +Depending on the test, they may be ignored or consumed and alter the behavior +of the test. Just make sure to use double-dash to distinguish them from the +fuzzer's own arguments: + +```sh +$ FUZZ=address_deserialize_v2 src/test/fuzz/fuzz -runs=1 fuzz_seed_corpus/address_deserialize_v2 --checkaddrman=5 --printtoconsole=1 +``` + ## Fuzzing corpora The project's collection of seed corpora is found in the [`bitcoin-core/qa-assets`](https://github.com/bitcoin-core/qa-assets) repo. diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index b2c98df11b..0a80ac5af1 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -14,6 +14,9 @@ static constexpr size_t NUM_SOURCES = 64; static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256; +static const std::vector EMPTY_ASMAP; +static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0}; + static std::vector g_sources; static std::vector> g_addresses; @@ -72,14 +75,14 @@ static void AddrManAdd(benchmark::Bench& bench) CreateAddresses(); bench.run([&] { - AddrMan addrman{/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0}; + AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; AddAddressesToAddrMan(addrman); }); } static void AddrManSelect(benchmark::Bench& bench) { - AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; FillAddrMan(addrman); @@ -91,7 +94,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; FillAddrMan(addrman); @@ -120,7 +123,7 @@ static void AddrManAddThenGood(benchmark::Bench& bench) // // This has some overhead (exactly the result of AddrManAdd benchmark), but that overhead is constant so improvements in // AddrMan::Good() will still be noticeable. - AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO}; AddAddressesToAddrMan(addrman); markSomeAsGood(addrman); diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp index 16d622ebc0..02ae5de7fd 100644 --- a/src/bench/bench.cpp +++ b/src/bench/bench.cpp @@ -16,6 +16,8 @@ const std::function G_TEST_LOG_FUN{}; +const std::function()> G_TEST_COMMAND_LINE_ARGUMENTS{}; + namespace { void GenerateTemplateResults(const std::vector& benchmarkResults, const fs::path& file, const char* tpl) diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index b9a91f0fb6..78d8331eb3 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #if defined(QT_STATICPLUGIN) #include @@ -40,6 +41,8 @@ Q_IMPORT_PLUGIN(QCocoaIntegrationPlugin); const std::function G_TEST_LOG_FUN{}; +const std::function()> G_TEST_COMMAND_LINE_ARGUMENTS{}; + // This is all you need to run all the tests int main(int argc, char* argv[]) { diff --git a/src/test/README.md b/src/test/README.md index bcdaab2c4b..cf76f12007 100644 --- a/src/test/README.md +++ b/src/test/README.md @@ -33,19 +33,31 @@ the `src/qt/test/test_main.cpp` file. ### Running individual tests -`test_dash` has some built-in command-line arguments; for -example, to run just the `getarg_tests` verbosely: +`test_dash` accepts the command line arguments from the boost framework. +For example, to run just the `getarg_tests` suite of tests: - test_dash --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT +```bash +test_dash --log_level=all --run_test=getarg_tests +``` `log_level` controls the verbosity of the test framework, which logs when a -test case is entered, for example. The `DEBUG_LOG_OUT` after the two dashes -redirects the debug log, which would normally go to a file in the test datadir +test case is entered, for example. `test_dash` also accepts the command +line arguments accepted by `dashd`. Use `--` to separate both types of +arguments: + +```bash +test_dash --log_level=all --run_test=getarg_tests -- -printtoconsole=1 +``` + +The `-printtoconsole=1` after the two dashes redirects the debug log, which +would normally go to a file in the test datadir (`BasicTestingSetup::m_path_root`), to the standard terminal output. ... or to run just the doubledash test: - test_dash --run_test=getarg_tests/doubledash +```bash +test_dash --run_test=getarg_tests/doubledash +``` Run `test_dash --help` for the full list. @@ -68,7 +80,7 @@ on failure. For running individual tests verbosely, refer to the section To write to logs from unit tests you need to use specific message methods provided by Boost. The simplest is `BOOST_TEST_MESSAGE`. -For debugging you can launch the `test_dash` executable with `gdb`or `lldb` and +For debugging you can launch the `test_dash` executable with `gdb` or `lldb` and start debugging, just like you would with any other program: ```bash @@ -95,7 +107,7 @@ Running the tests and hitting a segmentation fault should now produce a file cal `/proc/sys/kernel/core_pattern`). You can then explore the core dump using -``` bash +```bash gdb src/test/test_dash core (gbd) bt # produce a backtrace for where a segfault occurred diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 534a3b7152..973fa7ad04 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,14 @@ using namespace std::literals; +static const std::vector EMPTY_ASMAP; +static const bool DETERMINISTIC{true}; + +static int32_t GetCheckRatio(const NodeContext& node_ctx) +{ + return std::clamp(node_ctx.args->GetArg("-checkaddrman", 100), 0, 1000000); +} + static CNetAddr ResolveIP(const std::string& ip) { CNetAddr addr; @@ -47,17 +56,11 @@ static std::vector FromBytes(const unsigned char* source, int vector_size) return result; } -/* Utility function to create a deterministic addrman, as used in most tests */ -static std::unique_ptr TestAddrMan(std::vector asmap = std::vector()) -{ - return std::make_unique(asmap, /*deterministic=*/true, /*consistency_check_ratio=*/100); -} - BOOST_FIXTURE_TEST_SUITE(addrman_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(addrman_simple) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source = ResolveIP("252.2.2.2"); @@ -91,7 +94,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 = TestAddrMan(); + addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); 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)); @@ -101,7 +104,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_AUTO_TEST_CASE(addrman_ports) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source = ResolveIP("252.2.2.2"); @@ -129,7 +132,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports) BOOST_AUTO_TEST_CASE(addrman_select) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source = ResolveIP("252.2.2.2"); @@ -188,7 +191,7 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_AUTO_TEST_CASE(addrman_new_collisions) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source = ResolveIP("252.2.2.2"); @@ -217,7 +220,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions) BOOST_AUTO_TEST_CASE(addrman_new_multiplicity) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CAddress addr{CAddress(ResolveService("253.3.3.3", 8333), NODE_NONE)}; int64_t start_time{GetAdjustedTime()}; addr.nTime = start_time; @@ -249,7 +252,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_multiplicity) BOOST_AUTO_TEST_CASE(addrman_tried_collisions) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source = ResolveIP("252.2.2.2"); @@ -280,7 +283,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions) BOOST_AUTO_TEST_CASE(addrman_getaddr) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); // Test: Sanity check, GetAddr should never return anything if addrman // is empty. @@ -601,9 +604,11 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) { std::vector asmap1 = FromBytes(raw_tests::asmap, sizeof(raw_tests::asmap) * 8); - auto addrman_asmap1 = TestAddrMan(asmap1); - auto addrman_asmap1_dup = TestAddrMan(asmap1); - auto addrman_noasmap = TestAddrMan(); + const auto ratio = GetCheckRatio(m_node); + auto addrman_asmap1 = std::make_unique(asmap1, DETERMINISTIC, ratio); + auto addrman_asmap1_dup = std::make_unique(asmap1, DETERMINISTIC, ratio); + auto addrman_noasmap = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, ratio); + CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); CAddress addr = CAddress(ResolveService("250.1.1.1"), NODE_NONE); @@ -631,8 +636,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) BOOST_CHECK(addr_pos1.position != addr_pos3.position); // deserializing non-asmaped peers.dat to asmaped addrman - addrman_asmap1 = TestAddrMan(asmap1); - addrman_noasmap = TestAddrMan(); + addrman_asmap1 = std::make_unique(asmap1, DETERMINISTIC, ratio); + addrman_noasmap = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, ratio); addrman_noasmap->Add({addr}, default_source); stream << *addrman_noasmap; stream >> *addrman_asmap1; @@ -643,8 +648,8 @@ BOOST_AUTO_TEST_CASE(addrman_serialization) BOOST_CHECK(addr_pos4 == addr_pos2); // used to map to different buckets, now maps to the same bucket. - addrman_asmap1 = TestAddrMan(asmap1); - addrman_noasmap = TestAddrMan(); + addrman_asmap1 = std::make_unique(asmap1, DETERMINISTIC, ratio); + addrman_noasmap = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, ratio); 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); @@ -663,7 +668,7 @@ BOOST_AUTO_TEST_CASE(remove_invalid) { // Confirm that invalid addresses are ignored in unserialization. - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE}; @@ -695,14 +700,14 @@ 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 = TestAddrMan(); + addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); stream >> *addrman; BOOST_CHECK_EQUAL(addrman->size(), 2); } BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); BOOST_CHECK(addrman->size() == 0); @@ -735,7 +740,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) BOOST_AUTO_TEST_CASE(addrman_noevict) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); // Add 35 addresses. CNetAddr source = ResolveIP("252.2.2.2"); @@ -787,7 +792,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) BOOST_AUTO_TEST_CASE(addrman_evictionworks) { - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); BOOST_CHECK(addrman->size() == 0); @@ -857,8 +862,7 @@ static CDataStream AddrmanToStream(const AddrMan& addrman) BOOST_AUTO_TEST_CASE(load_addrman) { - AddrMan addrman{/*asmap=*/ std::vector(), /*deterministic=*/ true, - /*consistency_check_ratio=*/ 100}; + AddrMan addrman{EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)}; CService addr1, addr2, addr3; BOOST_CHECK(Lookup("250.7.1.1", addr1, 8333, false)); @@ -877,7 +881,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrman); bool exceptionThrown = false; - AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman1{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)}; BOOST_CHECK(addrman1.size() == 0); try { @@ -894,7 +898,7 @@ BOOST_AUTO_TEST_CASE(load_addrman) // Test that ReadFromStream creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrman); - AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman2{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)}; BOOST_CHECK(addrman2.size() == 0); ReadFromStream(addrman2, ssPeers2); BOOST_CHECK(addrman2.size() == 3); @@ -932,7 +936,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) // Test that the de-serialization of corrupted peers.dat throws an exception. CDataStream ssPeers1 = MakeCorruptPeersDat(); bool exceptionThrown = false; - AddrMan addrman1(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman1{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)}; BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -948,7 +952,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) // Test that ReadFromStream fails if peers.dat is corrupt CDataStream ssPeers2 = MakeCorruptPeersDat(); - AddrMan addrman2(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 100); + AddrMan addrman2{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)}; BOOST_CHECK(addrman2.size() == 0); BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure); } @@ -956,7 +960,7 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted) BOOST_AUTO_TEST_CASE(addrman_update_address) { // Tests updating nTime via Connected() and nServices via SetServices() - auto addrman = TestAddrMan(); + auto addrman = std::make_unique(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node)); CNetAddr source{ResolveIP("252.2.2.2")}; CAddress addr{CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE)}; diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index b6d75ba80d..796b723c77 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -11,8 +11,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -20,16 +22,26 @@ #include #include +namespace { +const BasicTestingSetup* g_setup; + +int32_t GetCheckRatio() +{ + return std::clamp(g_setup->m_node.args->GetArg("-checkaddrman", 0), 0, 1000000); +} +} // namespace + void initialize_addrman() { - SelectParams(CBaseChainParams::REGTEST); + static const auto testing_setup = MakeNoLogFileContext<>(CBaseChainParams::REGTEST); + g_setup = testing_setup.get(); } FUZZ_TARGET_INIT(data_stream_addr_man, initialize_addrman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - AddrMan addr_man(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addr_man{/*asmap=*/std::vector(), /*deterministic=*/false, GetCheckRatio()}; try { ReadFromStream(addr_man, data_stream); } catch (const std::exception&) { @@ -113,7 +125,7 @@ class AddrManDeterministic : public AddrMan { public: explicit AddrManDeterministic(std::vector asmap, FuzzedDataProvider& fuzzed_data_provider) - : AddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0) + : AddrMan{std::move(asmap), /*deterministic=*/true, GetCheckRatio()} { WITH_LOCK(m_impl->cs, m_impl->insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); } diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index aa2056c451..73e6e50cc1 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -11,21 +11,29 @@ #include #include #include +#include #include #include #include +namespace { +const BasicTestingSetup* g_setup; +} // namespace + void initialize_connman() { static const auto testing_setup = MakeNoLogFileContext<>(); + g_setup = testing_setup.get(); } FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - AddrMan addrman(/* asmap */ std::vector(), /* deterministic */ false, /* consistency_check_ratio */ 0); + AddrMan addrman(/*asmap=*/std::vector(), + /*deterministic=*/false, + g_setup->m_node.args->GetArg("-checkaddrman", 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 4bd4a55c83..1b184c67cc 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -21,7 +21,9 @@ #include #include