Merge bitcoin/bitcoin#23373: test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change

7f122a4 fuzz: non-addrman fuzz tests: override-able check ratio (Vasil Dimov)
3bd83e2 fuzz: addrman fuzz tests: override-able check ratio (Vasil Dimov)
46b0fe7 test: non-addrman unit tests: override-able check ratio (Vasil Dimov)
81e4d54 test: addrman unit tests: override-able check ratio (Vasil Dimov)
6dff621 bench: put addrman check ratio in a variable (Vasil Dimov)
6f7c756 fuzz: parse the command line arguments in fuzz tests (Vasil Dimov)
92a0f7e test: parse the command line arguments in unit tests (Vasil Dimov)

Pull request description:

  Previously command line arguments passed to unit and fuzz tests would be ignored by the tests themselves. They would be used by the boost test framework (e.g. `--run_test="addrman_tests/*"`) or by the fuzzer (e.g. `-runs=1`). However both provide ways to pass down the extra arguments to the test itself. Use that, parse the arguments and make them available to the tests via `gArgs`.

  This makes the tests more flexible as they can be run with any bitcoind config option specified on the command line.

  When creating `AddrMan` objects in tests, use `-checkaddrman=` (if provided) instead of hardcoding the check ratio in many different places. See bitcoin/bitcoin#20233 (comment) for further motivation for this.

ACKs for top commit:
  mzumsande:
    re-ACK 7f122a4
  josibake:
    reACK bitcoin/bitcoin@7f122a4

Tree-SHA512: 3a05e61e4d70a0569bb67594bcce3aad8fdef63cdcc54e2823a3bc9f18679571985004412b6c332a210f67849bab32d8467b4115fbff8f5fac9834982e60dcf3

Author: MarcoFalke

doc/fuzzing.md

@@ -71,6 +71,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 `bitcoind` 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.

src/bench/addrman.cpp

@@ -16,6 +16,9 @@
 static constexpr size_t NUM_SOURCES = 64;
 static constexpr size_t NUM_ADDRESSES_PER_SOURCE = 256;
 
+static const std::vector<bool> EMPTY_ASMAP;
+static constexpr uint32_t ADDRMAN_CONSISTENCY_CHECK_RATIO{0};
+
 static std::vector<CAddress> g_sources;
 static std::vector<std::vector<CAddress>> g_addresses;
 
@@ -74,14 +77,14 @@ static void AddrManAdd(benchmark::Bench& bench)
     CreateAddresses();
 
     bench.run([&] {
-        AddrMan addrman{/* asmap */ std::vector<bool>(), /* 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<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
 
     FillAddrMan(addrman);
 
@@ -93,7 +96,7 @@ static void AddrManSelect(benchmark::Bench& bench)
 
 static void AddrManGetAddr(benchmark::Bench& bench)
 {
-    AddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+    AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
 
     FillAddrMan(addrman);
 
@@ -122,7 +125,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<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
+        AddrMan addrman{EMPTY_ASMAP, /*deterministic=*/false, ADDRMAN_CONSISTENCY_CHECK_RATIO};
         AddAddressesToAddrMan(addrman);
 
         markSomeAsGood(addrman);

src/bench/bench.cpp

@@ -19,6 +19,8 @@ using namespace std::chrono_literals;
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
+
 namespace {
 
 void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const std::string& filename, const char* tpl)

src/qt/test/test_main.cpp

@@ -22,6 +22,7 @@
 #include <QApplication>
 #include <QObject>
 #include <QTest>
+#include <functional>
 
 #if defined(QT_STATICPLUGIN)
 #include <QtPlugin>
@@ -43,6 +44,8 @@ using node::NodeContext;
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
+
 // This is all you need to run all the tests
 int main(int argc, char* argv[])
 {

src/test/README.md

@@ -33,19 +33,31 @@ the `src/qt/test/test_main.cpp` file.
 
 ### Running individual tests
 
-`test_bitcoin` has some built-in command-line arguments; for
-example, to run just the `getarg_tests` verbosely:
+`test_bitcoin` accepts the command line arguments from the boost framework.
+For example, to run just the `getarg_tests` suite of tests:
 
-    test_bitcoin --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT
+```bash
+test_bitcoin --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_bitcoin` also accepts the command
+line arguments accepted by `bitcoind`. Use `--` to separate both types of
+arguments:
+
+```bash
+test_bitcoin --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_bitcoin --run_test=getarg_tests/doubledash
+```bash
+test_bitcoin --run_test=getarg_tests/doubledash
+```
 
 Run `test_bitcoin --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_bitcoin` executable with `gdb`or `lldb` and
+For debugging you can launch the `test_bitcoin` 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_bitcoin core
 
 (gbd) bt  # produce a backtrace for where a segfault occurred

src/test/addrman_tests.cpp

@@ -21,6 +21,15 @@
 #include <string>
 
 using namespace std::literals;
+using node::NodeContext;
+
+static const std::vector<bool> EMPTY_ASMAP;
+static const bool DETERMINISTIC{true};
+
+static int32_t GetCheckRatio(const NodeContext& node_ctx)
+{
+    return std::clamp<int32_t>(node_ctx.args->GetIntArg("-checkaddrman", 100), 0, 1000000);
+}
 
 static CNetAddr ResolveIP(const std::string& ip)
 {
@@ -49,17 +58,11 @@ static std::vector<bool> 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<AddrMan> TestAddrMan(std::vector<bool> asmap = std::vector<bool>())
-{
-    return std::make_unique<AddrMan>(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<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
@@ -93,7 +96,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<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
     std::vector<CAddress> 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));
@@ -103,7 +106,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
 
 BOOST_AUTO_TEST_CASE(addrman_ports)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
@@ -132,7 +135,7 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
 
 BOOST_AUTO_TEST_CASE(addrman_select)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
@@ -191,7 +194,7 @@ BOOST_AUTO_TEST_CASE(addrman_select)
 
 BOOST_AUTO_TEST_CASE(addrman_new_collisions)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
@@ -220,7 +223,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
 
 BOOST_AUTO_TEST_CASE(addrman_new_multiplicity)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(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;
@@ -252,7 +255,7 @@ BOOST_AUTO_TEST_CASE(addrman_new_multiplicity)
 
 BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     CNetAddr source = ResolveIP("252.2.2.2");
 
@@ -283,7 +286,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
 
 BOOST_AUTO_TEST_CASE(addrman_getaddr)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     // Test: Sanity check, GetAddr should never return anything if addrman
     //  is empty.
@@ -604,9 +607,11 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
 {
     std::vector<bool> asmap1 = FromBytes(asmap_raw, sizeof(asmap_raw) * 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<AddrMan>(asmap1, DETERMINISTIC, ratio);
+    auto addrman_asmap1_dup = std::make_unique<AddrMan>(asmap1, DETERMINISTIC, ratio);
+    auto addrman_noasmap = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, ratio);
+
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
 
     CAddress addr = CAddress(ResolveService("250.1.1.1"), NODE_NONE);
@@ -634,8 +639,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<AddrMan>(asmap1, DETERMINISTIC, ratio);
+    addrman_noasmap = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, ratio);
     addrman_noasmap->Add({addr}, default_source);
     stream << *addrman_noasmap;
     stream >> *addrman_asmap1;
@@ -646,8 +651,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<AddrMan>(asmap1, DETERMINISTIC, ratio);
+    addrman_noasmap = std::make_unique<AddrMan>(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);
@@ -666,7 +671,7 @@ BOOST_AUTO_TEST_CASE(remove_invalid)
 {
     // Confirm that invalid addresses are ignored in unserialization.
 
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
     CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
 
     const CAddress new1{ResolveService("5.5.5.5"), NODE_NONE};
@@ -698,14 +703,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<AddrMan>(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<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     BOOST_CHECK(addrman->size() == 0);
 
@@ -738,7 +743,7 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision)
 
 BOOST_AUTO_TEST_CASE(addrman_noevict)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     // Add 35 addresses.
     CNetAddr source = ResolveIP("252.2.2.2");
@@ -790,7 +795,7 @@ BOOST_AUTO_TEST_CASE(addrman_noevict)
 
 BOOST_AUTO_TEST_CASE(addrman_evictionworks)
 {
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
 
     BOOST_CHECK(addrman->size() == 0);
 
@@ -860,8 +865,7 @@ static CDataStream AddrmanToStream(const AddrMan& addrman)
 
 BOOST_AUTO_TEST_CASE(load_addrman)
 {
-    AddrMan addrman{/*asmap=*/ std::vector<bool>(), /*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));
@@ -880,7 +884,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<bool>(), /*deterministic=*/false, /*consistency_check_ratio=*/100);
+    AddrMan addrman1{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)};
 
     BOOST_CHECK(addrman1.size() == 0);
     try {
@@ -897,7 +901,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<bool>(), /*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);
@@ -935,7 +939,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<bool>(), /*deterministic=*/false, /*consistency_check_ratio=*/100);
+    AddrMan addrman1{EMPTY_ASMAP, !DETERMINISTIC, GetCheckRatio(m_node)};
     BOOST_CHECK(addrman1.size() == 0);
     try {
         unsigned char pchMsgTmp[4];
@@ -951,15 +955,15 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
     // Test that ReadFromStream fails if peers.dat is corrupt
     CDataStream ssPeers2 = MakeCorruptPeersDat();
 
-    AddrMan addrman2(/*asmap=*/std::vector<bool>(), /*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);
 }
 
 BOOST_AUTO_TEST_CASE(addrman_update_address)
 {
     // Tests updating nTime via Connected() and nServices via SetServices()
-    auto addrman = TestAddrMan();
+    auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
     CNetAddr source{ResolveIP("252.2.2.2")};
     CAddress addr{CAddress(ResolveService("250.1.1.1", 8333), NODE_NONE)};