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

vasild commented at 12:18 pm on October 27, 2021: member

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 #20233 (comment) for further motivation for this.

DrahtBot added the label GUI on Oct 27, 2021

DrahtBot commented at 2:33 pm on October 27, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
#22910 ([RFC] Encapsulate asmap in NetGroupManager by jnewbery)
#21878 (Make all networking code mockable by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/test/fuzz/fuzz.cpp:26 in 9b6098f41f outdated

18@@ -19,6 +19,28 @@
19 
20 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
21 
22+/**
23+ * A copy of the command line arguments that start with `--`.
24+ * First `LLVMFuzzerInitialize()` is called, which saves the arguments to `g_args`.
25+ * Later, depending on the fuzz test, `G_TEST_COMMAND_LINE_ARGUMENTS()` may be
26+ * called to lookup those arguments and fill `gArgs` from them.

jonatack commented at 4:05 pm on November 3, 2021:

5d774a98 (only if you retouch) s/lookup/look up|fetch/ (“lookup” is a noun or adjective, the verb form is “look up”)

vasild commented at 3:33 pm on November 4, 2021:

Done.

jonatack commented at 8:16 pm on November 3, 2021: member

Initial lightly tested code review ACK, was distracted by addrman unit test failures I was seeing that turned out to be already on master (and spurious, not sure yet what happened); will come back to finish reviewing here.

in src/test/main.cpp:32 in c79eb4ad9f outdated

23@@ -24,3 +24,17 @@ const std::function<void(const std::string&)> G_TEST_LOG_FUN = [](const std::str
24     if (!should_log) return;
25     std::cout << s;
26 };
27+
28+/**
29+ * Retrieve the command line arguments from boost.
30+ * Allows usage like:
31+ * `test_bitcoin --run_test="net_tests/cnode_listen_port" -- -checkaddrman=1 -printtoconsole=1`

mzumsande commented at 11:24 pm on November 3, 2021:

There is an additional parameter -cool in the commit message of c79eb4ad9f0adf733a9da59d87dc3a0d10a90738, is this on purpose? Just noting that providing a non-recognized parameter like that causes an assertion for each unit test. I’m not sure if this is a problem or not.

vasild commented at 10:23 am on November 4, 2021:

Well, the -cool is on purpose, like, not by accident, but may as well be removed. I wanted to use it to show that any other option is also ok there, like e.g -putanyoptionyouwanthere. Will remove it.

vasild commented at 3:33 pm on November 4, 2021:

Removed.

vasild commented at 3:37 pm on November 4, 2021:

providing a non-recognized parameter like that causes an assertion for each unit test

Right, that was not very user (or developer) friendly. For example, the command:

0./src/test/test_bitcoin --run_test="net_tests/cnode_listen_port" -- -foo

resulted in:

0Assertion failed: (success), function BasicTestingSetup, file test/util/setup_common.cpp, line 104.
1unknown location(0): fatal error: in "net_tests/cnode_listen_port": signal: SIGABRT (application abort requested)

I have changed it to print the error message (Invalid parameter -foo):

0unknown location(0): fatal error: in "net_tests/cnode_listen_port": std::runtime_error: Invalid parameter -foo

mzumsande commented at 11:37 pm on November 3, 2021: member

Concept ACK, I think it would be useful to have the option of providing arguments to unit and fuzz tests.

Is all the code processing -- DEBUG_LOG_OUT still needed when we might do as well -- -printttoconsole=1 ?

vasild commented at 10:29 am on November 4, 2021: member

Is all the code processing -- DEBUG_LOG_OUT still needed when we might do as well -- -printttoconsole=1 ?

Not needed anymore. I did not remove it because I do not want to break existing functionality - I do not know how many scripts out there use it, or in CI, or how many devs are just used to it. Given that it is just for testing/development purposes, not used in real/production nodes I think it should be ok to remove it in a followup PR some time after this is merged.

vasild force-pushed on Nov 4, 2021

vasild commented at 3:33 pm on November 4, 2021: member

9b6098f41f...0c541a6dba: take suggestions and print the error from unit tests if parsing of the arguments fails.

in src/test/util/setup_common.cpp:117 in 0c541a6dba outdated

100@@ -97,9 +101,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
101     {
102         SetupServerArgs(*m_node.args);
103         std::string error;
104-        const bool success{m_node.args->ParseParameters(arguments.size(), arguments.data(), error)};
105-        assert(success);
106-        assert(error.empty());
107+        if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
108+            throw std::runtime_error{error};

MarcoFalke commented at 3:41 pm on November 4, 2021:

Is it now impossible to use DEBUG_LOG_OUT? If yes, might as well remove it.

vasild commented at 3:51 pm on November 4, 2021:

No, it is not broken by this PR. For example, both commands below produce the same output:

0test_bitcoin --run_test="net_tests/cnode_listen_port" -- DEBUG_LOG_OUT
1test_bitcoin --run_test="net_tests/cnode_listen_port" -- -printtoconsole=1

in src/test/util/setup_common.cpp:199 in 0c541a6dba outdated

195@@ -192,7 +196,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
196         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
197     }
198 
199-    m_node.addrman = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
200+    m_node.addrman = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(),

MarcoFalke commented at 4:09 pm on November 4, 2021:

0    m_node.addrman = std::make_unique<AddrMan>(/*asmap=*/ std::vector<bool>(),

nit: Could use the clang-tidy understandable syntax when touching this?

vasild commented at 1:48 pm on November 5, 2021:

Done.

in src/test/util/setup_common.cpp:201 in 0c541a6dba outdated

195@@ -192,7 +196,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
196         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
197     }
198 
199-    m_node.addrman = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
200+    m_node.addrman = std::make_unique<AddrMan>(/* asmap */ std::vector<bool>(),
201+                                               /* deterministic */ false,
202+                                               gArgs.GetIntArg("-checkaddrman", 0));

MarcoFalke commented at 4:10 pm on November 4, 2021:

Seems odd to push the args into m_node.args, but then read them from gArgs. Would be clearer to use the same in both places.

vasild commented at 1:52 pm on November 5, 2021:

Changed the PR to avoid using gArgs in all places.

The global was convenient in the addrman unit tests which have 4 different AddrMan-derived classes. I think now is better because all of them receive the same arguments as the base class AddrMan. This increased the size of commit test: addrman unit tests: override-able check ratio, I think it is justified.

in src/test/fuzz/connman.cpp:15 in 0c541a6dba outdated

11@@ -12,6 +12,7 @@
12 #include <test/fuzz/util.h>
13 #include <test/util/setup_common.h>
14 #include <util/translation.h>
15+#include <util/system.h>

jonatack commented at 4:12 pm on November 4, 2021:

9b6098f nit, sort

0 #include <test/util/setup_common.h>
1-#include <util/translation.h>
2 #include <util/system.h>
3+#include <util/translation.h>

vasild commented at 1:54 pm on November 5, 2021:

Sorted.

in src/test/fuzz/fuzz.cpp:32 in 0c541a6dba outdated

27+ */
28+static std::vector<const char*> g_args;
29+
30+static void set_args(int argc, char** argv) {
31+    for (int i = 1; i < argc; ++i) {
32+        // Only take into account arguments that start with `--`. The others are for the fuzzer:

MarcoFalke commented at 4:13 pm on November 4, 2021:

0        // Only take into account arguments that start with `--`. The others are for the fuzz engine:

(nit)

vasild commented at 1:52 pm on November 5, 2021:

Done.

in src/test/fuzz/deserialize.cpp:198 in 0c541a6dba outdated

192@@ -189,7 +193,9 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, {
193     BlockMerkleRoot(block, &mutated);
194 })
195 FUZZ_TARGET_DESERIALIZE(addrman_deserialize, {
196-    AddrMan am(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0);
197+    AddrMan am(/* asmap */ std::vector<bool>(),
198+               /* deterministic */ false,
199+               gArgs.GetIntArg("-checkaddrman", 0));

MarcoFalke commented at 4:16 pm on November 4, 2021:

It might be clearer to use testing_setup.m_node.args, since this is where they were put.

vasild commented at 1:53 pm on November 5, 2021:

Done, it was necessary to define the testing_setup variable as a global, outside of the initialize_...() function so that it can be accessed by this code.

in src/test/util/setup_common.h:30 in 0c541a6dba outdated

23@@ -24,6 +24,9 @@
24 /** This is connected to the logger. Can be used to redirect logs to any other log */
25 extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;
26 
27+/** Retrieve the command line arguments. */
28+extern const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS;

jonatack commented at 4:16 pm on November 4, 2021:

c79eb4ad headers where std::functional is added

0+++ src/test/util/setup_common.h
1+#include <functional>
2 #include <type_traits>
3 #include <vector>

0+++ src/test/main.cpp 
1 #include <test/util/setup_common.h>
2 
3+#include <functional>
4 #include <iostream>

…and so on.

vasild commented at 2:10 pm on November 5, 2021:

Added #include <functional> in all places where std::function is used (relevant to this PR).

in src/test/fuzz/fuzz.cpp:26 in 0c541a6dba outdated

18@@ -19,6 +19,28 @@
19 
20 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
21 
22+/**
23+ * A copy of the command line arguments that start with `--`.
24+ * First `LLVMFuzzerInitialize()` is called, which saves the arguments to `g_args`.
25+ * Later, depending on the fuzz test, `G_TEST_COMMAND_LINE_ARGUMENTS()` may be
26+ * called to fetch those arguments and fill `gArgs` from them.

MarcoFalke commented at 4:17 pm on November 4, 2021:

The fetching is done implicitly, so mention that instead of mentioning gArgs?

vasild commented at 10:27 am on November 5, 2021:

Reworded to

0 * Later, depending on the fuzz test, `G_TEST_COMMAND_LINE_ARGUMENTS()` may be
1 * called by `BasicTestingSetup` constructor to fetch those arguments and store
2 * them in `BasicTestingSetup::m_node::args`.

Is it better?

MarcoFalke commented at 10:29 am on November 5, 2021:

It is called, not may be called? Sounds better either way.

vasild commented at 11:12 am on November 5, 2021:

It is called if the fuzz test creates a BasicTestingSetup from its initialize_...() function, not all of them do, right?

MarcoFalke commented at 11:40 am on November 5, 2021:

If there is no BasicTestingSetup, you can’t fetch them either ;)

But as I said, both new versions are equally fine.

vasild commented at 2:21 pm on November 5, 2021:

Ok :)

in src/test/addrman_tests.cpp:1055 in 0c541a6dba outdated

1049@@ -1042,7 +1050,9 @@ BOOST_AUTO_TEST_CASE(load_addrman)
1050     // Test that ReadFromStream creates an addrman with the correct number of addrs.
1051     CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
1052 
1053-    AddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
1054+    AddrMan addrman2(/* asmap */ std::vector<bool>(),
1055+                     /* deterministic */ false,
1056+                     gArgs.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECK_RATIO));

MarcoFalke commented at 4:17 pm on November 4, 2021:

Same (obviously everywhere)

vasild commented at 1:54 pm on November 5, 2021:

Removed all usage of gArgs.

in src/test/main.cpp:35 in 0c541a6dba outdated

29+ * Retrieve the command line arguments from boost.
30+ * Allows usage like:
31+ * `test_bitcoin --run_test="net_tests/cnode_listen_port" -- -checkaddrman=1 -printtoconsole=1`
32+ * which would return `["-checkaddrman=1", "-printtoconsole=1"]`.
33+ */
34+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS = []() {

jonatack commented at 4:35 pm on November 4, 2021:

c79eb4ad9f0adf733a9da59d87dc3a0d10a90738 Does it makes sense for G_TEST_COMMAND_LINE_ARGUMENTS to be named as a constant (as it should update at runtime, yes?)

vasild commented at 10:02 am on November 5, 2021:

Good question. G_TEST_COMMAND_LINE_ARGUMENTS is a constant (its value is the function itself, we don’t change it to another function). So, strictly speaking this is a const variable. But we use it as a function - G_TEST_COMMAND_LINE_ARGUMENTS(). I used that name for consistency with the already existent G_TEST_LOG_FUN which is defined a few lines earlier.

Semantically it is a function and personally I think it would be most logical to name it like a function TestCommandLineArguments(). Like I did with CreateSock:

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/netbase.h#L190

Maybe the guidelines should be updated to clarify how to name things that are both variables and functions.

MarcoFalke commented at 10:26 am on November 5, 2021:

Upper case might be correct, see #18568. Though, maybe the G_ should be removed, as upper case implies G_?

vasild commented at 1:56 pm on November 5, 2021:

Anyway, for this PR I think consistency with the neighboring G_TEST_LOG_FUN is the most important. Marking this as resolved, feel free to comment further / unresolve it.

jonatack commented at 4:40 pm on November 4, 2021: member

Did a bit more testing. It seems it could be very helpful for this patch to print the parsed arguments and values as feedback to the user, to be sure they are correct. A few minor comments follow.

vasild force-pushed on Nov 5, 2021

vasild commented at 2:31 pm on November 5, 2021: member

0c541a6dba...17fac018c2: address suggestions, biggest changes due to avoidance of gArgs.

in src/bench/addrman.cpp:81 in 17fac018c2 outdated

77@@ -74,14 +78,14 @@ static void AddrManAdd(benchmark::Bench& bench)
78     CreateAddresses();
79 
80     bench.run([&] {
81-        AddrMan addrman{/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0};
82+        AddrMan addrman{EMPTY_ASMAP, !DETERMINISTIC, ADDRMAN_CONSISTENCY_CHECK_RATIO};

MarcoFalke commented at 2:49 pm on November 5, 2021:

why the !?

vasild commented at 3:47 pm on November 5, 2021:

Because false was passed in the original code, denoting “not deterministic”.

MarcoFalke commented at 3:53 pm on November 5, 2021:

It appears a bit confusing to me to set DETERMINISTIC=true and get non-determinism

vasild commented at 4:10 pm on November 5, 2021:

I think it is ok, but if it is confusing somebody then it is not good. Should I go back to /*deterministic=*/false?

MarcoFalke commented at 4:14 pm on November 5, 2021:

no opinion on that, but if you want to keep DETERMINISTIC, it might be better set to false

vasild commented at 5:00 pm on November 5, 2021:

Reverted to /*deterministic=*/false.

in src/test/fuzz/addrman.cpp:29 in 17fac018c2 outdated

25+static std::unique_ptr<const BasicTestingSetup> g_testing_setup;
26+
27 void initialize_addrman()
28 {
29-    SelectParams(CBaseChainParams::REGTEST);
30+    if (!g_testing_setup) {

MarcoFalke commented at 2:53 pm on November 5, 2021:

Why is this check needed?

vasild commented at 3:59 pm on November 5, 2021:

initialize_addrman() will be called from data_stream_addr_man, addrman and addrman_serdeser tests. Without this check it will create and destroy the BasicTestingSetup object before/after each test. I do not know if this is a problem. The code I mimic (and which presumably works) is using static variable inside the initialize_...() function:

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/test/fuzz/banman.cpp#L30-L32

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/test/fuzz/coins_view.cpp#L40-L42

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/test/fuzz/load_external_block_file.cpp#L20-L22

https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/test/fuzz/net.cpp#L24-L26

These static variables will be initialized only once, even if the initialize...() function is called multiple times. So I mimic that with the if check.

MarcoFalke commented at 4:13 pm on November 5, 2021:

It is only possible to run one fuzz target at a time and it is not possible to switch the fuzz target.

The reasons to use the initialize function is because globals that are initialized before main are bad. For fuzzing that means even globals that are never used can poison the coverage data or crash the fuzz target. Also, using the init function allows the fuzz engine to properly set up its own accounting before and exactly know when the fuzz targets own initialization code happens.

vasild commented at 4:19 pm on November 5, 2021:

Why are those initialize...() functions using static variables?

MarcoFalke commented at 4:31 pm on November 5, 2021:

static inside a function will only call the constructor when called the first time.

They use static because the constructed object needs to live after the function returned.

vasild commented at 5:03 pm on November 5, 2021:

They use static because the constructed object needs to live after the function returned

Oh, yes, of course. I completely missed that aspect. Was only focusing on the fact that the static variables will be initialized only once even if the function is called multiple times.

Changed now to do like the other tests, e.g. src/test/fuzz/coins_view.cpp.

in src/test/fuzz/connman.cpp:24 in 17fac018c2 outdated

20+static std::unique_ptr<const BasicTestingSetup> g_testing_setup;
21+
22 void initialize_connman()
23 {
24-    static const auto testing_setup = MakeNoLogFileContext<>();
25+    if (!g_testing_setup) {

MarcoFalke commented at 2:54 pm on November 5, 2021:

same

vasild commented at 5:04 pm on November 5, 2021:

Removed.

vasild force-pushed on Nov 5, 2021

vasild commented at 5:06 pm on November 5, 2021: member

17fac018c2...b374709725: address suggestions

DrahtBot added the label Needs rebase on Nov 12, 2021

vasild force-pushed on Nov 12, 2021

vasild commented at 11:14 am on November 12, 2021: member

b374709725...51b241bf2a: rebase due to conflicts

MarcoFalke removed the label GUI on Nov 12, 2021

DrahtBot removed the label Needs rebase on Nov 12, 2021

DrahtBot added the label GUI on Nov 12, 2021

DrahtBot added the label Needs rebase on Dec 1, 2021

vasild force-pushed on Dec 3, 2021

vasild commented at 12:07 pm on December 3, 2021: member

51b241bf2a...9a7725986a: rebase due to conflicts

DrahtBot removed the label Needs rebase on Dec 3, 2021

in src/test/fuzz/fuzz.cpp:45 in d0fd37b762 outdated

19 #include <vector>
20 
21 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
22 
23+const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
24+

josibake commented at 9:35 am on December 9, 2021:

https://github.com/bitcoin/bitcoin/pull/23373/commits/d0fd37b762babd44da9347650a880a6701247afd are these lines supposed to be here? the commit message mentions only unit tests so i thought it was odd to see changes to fuzz.cpp and then i noticed these lines are removed in the next commit

vasild commented at 4:17 pm on December 9, 2021:

Yes because every commit has to be self-sufficient - transform the tree from one valid state to another valid state (where it compiles, tests pass, there are no bugs, etc). If G_TEST_COMMAND_LINE_ARGUMENTS is not defined in src/test/fuzz/fuzz.cpp in the first commit, then that commit (without the subsequent ones) will not compile due to undefined reference. The next commit just changes the value of the variable.

josibake commented at 5:08 pm on December 9, 2021:

huh, interesting. might be worth squashing the commits and changing the commit message to indicate you are changing unit and fuzz?

then again, if no one else has mentioned it, probably not a big deal

vasild commented at 1:52 pm on December 10, 2021:

I would keep it as is - the first commit introduces command line parsing from unit tests. As a necessity, it adds stubs in src/bench/bench.cpp, src/qt/test/test_main.cpp and src/test/fuzz/fuzz.cpp because without those stubs the compilation would fail. The subsequent commits are not necessary for the first one - it can be merged alone, should reviewers reject e.g. the command line parsing from fuzz tests.

josibake commented at 9:53 am on December 9, 2021: member

Concept ACK

Big fan of parameters over hardcoding. Also seems like it would be nice to consistently use -printconsole=1 vs having to remember -- DEBUG_LOG_OUT.

did a light code review and it looks good, tho I’d suggest removing the changes to src/test/fuzz/fuzz.cpp in https://github.com/bitcoin/bitcoin/pull/23373/commits/d0fd37b762babd44da9347650a880a6701247afd. I’ll take a closer look at the code and do some testing in the meanwhile

josibake commented at 6:32 pm on December 10, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/23373/commits/9a7725986ab82ca0318dd709d9a94bb4c2bd14ee

compiled, ran the unit tests. ran both the unit tests and fuzzer with -checkaddrman and tried passing various other parameters (-printtoconsole -logtimestamps=0, for example). works as expected. also passed invalid parameters to verify i got an error message.

maybe this is better for a follow-up pr (which id be happy to do), but this feature should definitely be documented in src/test/README.md and doc/fuzzing.md

vasild force-pushed on Dec 13, 2021

vasild commented at 2:08 pm on December 13, 2021: member

9a7725986a...4e77962f2b: add some documentation, as suggested above. Thanks, @josibake!

Invalidates ACK from @josibake

josibake commented at 5:35 pm on December 13, 2021: member

re-ACK https://github.com/bitcoin/bitcoin/pull/23373/commits/4e77962f2b0680933ff35d27a40274ef686a7ef9

verified the documentation changes with git range-diff 9a77259...4e77962 .

+1 for removing the reference to DEBUG_LOG_OUT, as this sets us up nicely to remove that code in the future in favor of -printtoconsole=1

DrahtBot added the label Needs rebase on Dec 15, 2021

vasild force-pushed on Dec 15, 2021

vasild commented at 11:14 am on December 15, 2021: member

4e77962f2b...9a089d9eb5: rebase due to conflicts

Invalidates ACK from @josibake

DrahtBot removed the label Needs rebase on Dec 15, 2021

josibake commented at 1:44 pm on December 15, 2021: member

reACK https://github.com/bitcoin/bitcoin/pull/23373/commits/9a089d9eb576bc15f010f4db1216dd4489100a06

in src/test/util/setup_common.cpp:117 in 3207186750 outdated

103@@ -100,9 +104,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
104     {
105         SetupServerArgs(*m_node.args);
106         std::string error;
107-        const bool success{m_node.args->ParseParameters(arguments.size(), arguments.data(), error)};
108-        assert(success);
109-        assert(error.empty());
110+        if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
111+            throw std::runtime_error{error};

mzumsande commented at 1:35 am on December 16, 2021:

For me, test_bitcoin --run_test=addrman_tests -- -foo will only report an invalid parameter for the first subtest - subsequent tests of the suite will assert again in ArgsManager::AddArg.

I think the reason for this is that if this throws an unhandled exception, the destructor of BasicTestingSetup won’t be called and gArgs won’t be cleared.

vasild commented at 1:40 pm on December 17, 2021:

Right, I added a call to ClearArgs() to undo the SetupServerArgs() call.

This code is a bit fishy because it initializes a global variable (gArgs) from the constructor of BasicTestingSetup and clears it from the destructor…

vasild force-pushed on Dec 17, 2021

vasild commented at 1:41 pm on December 17, 2021: member

9a089d9eb5...ce94229bad: address #23373 (review)

Invalidates ACK from @josibake

in src/test/README.md:44 in cf715ebd9e outdated

44+```
45 
46 `log_level` controls the verbosity of the test framework, which logs when a
47-test case is entered, for example. The `DEBUG_LOG_OUT` after the two dashes
48-redirects the debug log, which would normally go to a file in the test datadir
49+test case is entered, for example. `test_bitcoin` also accepts the command

mzumsande commented at 9:13 pm on December 19, 2021:

nit (feel free to ignore): I think the added explanations for bitcoind command args would best be a separate paragraph at the end of this section.

mzumsande commented at 10:57 pm on December 19, 2021: member

The code changes look good to me, and I verified that for both unit and fuzz tests withh libfuzzer (using arguments such as -printtoconsole or -checkaddrman) work and that unrecognized arguments lead to sensible errors.

Before I ACK, one last concern: Will this play nice with other fuzzers that may interpret -- arguments for themselves? I never got anything other than libfuzzer running, but fuzzing.md mentions a line

FUZZ=bech32 dotnet Eclipser/build/Eclipser.dll fuzz -p src/test/fuzz/fuzz -t 36000 -o outputs --src stdin Could that --src interpreted as an invalid bitcoind command arg after this?

vasild force-pushed on Dec 23, 2021

vasild commented at 9:53 am on December 23, 2021: member

ce94229bad...72544a0053: only parse fuzz arguments if using libFuzzer - other fuzzers may not support:

Flags starting with ‘–’ will be ignored and will be passed verbatim to subprocesses.

and so they may be confused by bitcoind args (e.g. --checkaddrman=5) and also bitcoind may be confused by fuzzer’s own arguments (e.g. --src). In other words, only libFuzzer is good for the distinction -foo for the fuzzer and --foo for bitcoind. Thus this PR changes nothing if a fuzzer engine other than libFuzzer is used.

Thanks, @mzumsande, for spotting this!

mzumsande commented at 5:57 pm on December 23, 2021: member

ACK 72544a0053d551a732b6679b3293a3e767c5ef82

I reviewed the code and did some testing with the new functionality for unit and fuzz tests as described in #23373#pullrequestreview-835903425

MarcoFalke removed the label GUI on Dec 27, 2021

josibake commented at 11:59 am on December 27, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/23373/commits/72544a0053d551a732b6679b3293a3e767c5ef82

verified the change using git range-diff and also ran the fuzzer to verify. I think it makes sense to limit just to libFuzz for now, but it would be nice to confirm if this is an issue for other fuzzers or not

DrahtBot added the label GUI on Dec 27, 2021

MarcoFalke removed the label GUI on Dec 27, 2021

MarcoFalke added the label Tests on Dec 27, 2021

MarcoFalke renamed this:
~~Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change~~
test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change
on Dec 27, 2021

in doc/fuzzing.md:74 in 72544a0053 outdated

70@@ -71,6 +71,14 @@ block^@M-^?M-^?M-^?M-^?M-^?nM-^?M-^?
71 
72 In this case the fuzzer managed to create a `block` message which when passed to `ProcessMessage(...)` increased coverage.
73 
74+It is possible to specify `bitcoind` arguments to the `fuzz` executable.

MarcoFalke commented at 8:40 am on January 4, 2022:

This should only be possible if that specific FUZZ test starts a Bitcoin Core node

vasild commented at 5:13 pm on January 4, 2022:

Right, added some more words to make it clearer (hopefully). Feel free to suggest how to explain it better, if still unclear.

in src/test/README.md:39 in 72544a0053 outdated

32@@ -33,19 +33,31 @@ the `src/qt/test/test_main.cpp` file.
33 
34 ### Running individual tests
35 
36-`test_bitcoin` has some built-in command-line arguments; for
37-example, to run just the `getarg_tests` verbosely:
38+`test_bitcoin` accepts the command line arguments from the boost framework.
39+For example, to run just the `getarg_tests` suite of tests:
40 
41-    test_bitcoin --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT

MarcoFalke commented at 8:43 am on January 4, 2022:

I don’t think --printtoconsole=1 is a replacement for -- DEBUG_LOG_OUT. DEBUG_LOG_OUT would work even if the unit test didn’t start a Bitcoin Core node internally, no?

vasild commented at 4:50 pm on January 4, 2022:

Right. However, currently all unit tests create such a node: BOOST_FIXTURE_TEST_SUITE(..., X) where X is BasicTestingSetup or something that has inherited from it. So I think DEBUG_LOG_OUT can be completely phased out, following this PR.

MarcoFalke commented at 4:58 pm on January 4, 2022:

It is possible to create “pure” unit tests without any fixture, IIRC

vasild commented at 10:10 am on January 5, 2022:

Ok, are you suggesting to restore the mention of DEBUG_LOG_OUT in src/test/README.md or to not phase out DEBUG_LOG_OUT after this PR?

MarcoFalke commented at 10:15 am on January 5, 2022:

I guess it is fine to remove DEBUG_LOG_OUT. I just wanted to mention that not all test spin up a node.

DrahtBot added the label Needs rebase on Jan 4, 2022

vasild force-pushed on Jan 4, 2022

vasild commented at 5:12 pm on January 4, 2022: member

72544a0053...e25dce2e91: rebase due to conflicts, elaborate the added doc snippet

Invalidates ACK from @mzumsande, @josibake

DrahtBot removed the label Needs rebase on Jan 4, 2022

in src/test/addrman_tests.cpp:53 in accecbf7e1 outdated

56@@ -49,17 +57,11 @@ static std::vector<bool> FromBytes(const unsigned char* source, int vector_size)
57     return result;
58 }
59 
60-/* Utility function to create a deterministic addrman, as used in most tests */
61-static std::unique_ptr<AddrMan> TestAddrMan(std::vector<bool> asmap = std::vector<bool>())

mzumsande commented at 8:41 am on January 5, 2022:

This utility function was suggested to me by @jnewbery in #23826 (review) (Github link doesn’t really work for me but I just unresolved that conversation). Since it is removed again - do you disagree with that? Otherwise, you could have just adjusted that function to use DETERMINISTIC and GetCheckRatio(m_node).

vasild commented at 10:04 am on January 5, 2022:

This line appears 17 times in the test. It’s also very noisy and distracting from the purpose of the test:

0auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /*deterministic=*/true, /*consistency_check_ratio=*/100);

I agree. In master the function TestAddrMan() makes sense IMO.

However, with this PR, we also have to pass the ratio to it (or m_node so that it can call GetCheckRatio()) and then in some places addrman is constructed with deterministic=false and for this TestAddrMan() is not used which is not consistent/uniform way of creating addrman objects across the file. So, to get it all consistent and still use TestAddrMan() it would have to be extended to take a boolean argument (deterministic) and the ratio or NodeContext object, so calls would look like:

0auto addrman = TestAddrMan(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));
1// or
2auto addrman = TestAddrMan(EMPTY_ASMAP, DETERMINISTIC, m_node);

which I think is an unnecessary indirection and better construct the object directly:

0auto addrman = std::make_unique<AddrMan>(EMPTY_ASMAP, DETERMINISTIC, GetCheckRatio(m_node));

Anyway, I would restore TestAddrMan() if you prefer it over direct construction. Just suggest exactly what arguments should it take and whether it should be used on places where addrman objects are created on-stack, without unique_ptr.

mzumsande commented at 10:34 am on January 5, 2022:

Ah, I hadn’t realized that we would have to pass m_node each time now. I agree with you that the explicit construction makes more sense in this case - it was mostly for better readability, and the more arguments we’d have to pass each time, the less sense it makes. Also, making the calls shorter with the constants EMPTY_ASMAP, DETERMINISTIC instead of the long named arguments each time is an alternative way to make the line more readable.

So, I think it makes sense the way it is.

vasild commented at 10:42 am on January 5, 2022:

Yeah, my thought exactly! You said it with less words :)

mzumsande commented at 3:07 pm on January 5, 2022: member

ACK e25dce2e9111a6174767e8d83662805573bb43e0 after rebase.

DrahtBot added the label Needs rebase on Jan 11, 2022

MarcoFalke commented at 10:48 am on January 11, 2022: member

ACK e25dce2 after rebase.

Oh, I didn’t realize this meant re-ACK. I interpreted it as I’ll ack after rebase, so I didn’t merge this.

test: parse the command line arguments in unit tests

Retrieve the command line arguments from boost and pass them to
`BasicTestingSetup` so that we gain extra flexibility of passing any
config options on the test command line, e.g.:

```
test_bitcoin -- -printtoconsole=1 -checkaddrman=5
```

92a0f7e58d

fuzz: parse the command line arguments in fuzz tests

Retrieve the command line arguments from the fuzzer and save them for
later retrieval by `BasicTestingSetup` so that we gain extra flexibility
of passing any config options on the test command line, e.g.:

```
FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5
```

A fuzz test should call `MakeNoLogFileContext<>()` in its initialize
function in order to invoke the constructor of `BasicTestingSetup`,
which sets `gArgs`.

6f7c7567c5

bench: put addrman check ratio in a variable

So that it is easy to modify through the file `bench/addrman.cpp`.

6dff6214be

test: addrman unit tests: override-able check ratio

In addrman unit tests, make it possible to override the check ratio from
the command line, without recompiling:

```
test_bitcoin --run_test="addrman_tests/*" -- -checkaddrman=1
```

Also, make the arguments of the constructor of `AddrManTest` the
same as the arguments of `AddrMan`.

81e4d54d3a

test: non-addrman unit tests: override-able check ratio

Make it possible to override from the command line (without recompiling)
the addrman check ratio in the common `TestingSetup::m_node::addrman`
(used by all unit tests) instead of hardcoding it to 0:

```
test_bitcoin --run_test="transaction_tests/tx_valid" -- -checkaddrman=1
```

46b0fe7829

fuzz: addrman fuzz tests: override-able check ratio

Make it possible to override from the command line (without recompiling)
the addrman check ratio in addrman fuzz tests instead of hardcoding it
to 0:

```
FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5
```

3bd83e273d

fuzz: non-addrman fuzz tests: override-able check ratio

Make it possible to override from the command line (without recompiling)
the addrman check ratio in non-addrman fuzz tests (connman and
deserialize) instead of hardcoding it to 0:

```
FUZZ=connman ./src/test/fuzz/fuzz --checkaddrman=5
```

7f122a4188

vasild commented at 11:17 am on January 11, 2022: member

e25dce2e91...7f122a4188: rebase due to conflicts

Invalidates ACK from @mzumsande

Previously invalidated ACK from @josibake

vasild force-pushed on Jan 11, 2022

DrahtBot removed the label Needs rebase on Jan 11, 2022

mzumsande commented at 0:07 am on January 13, 2022: member

re-ACK 7f122a4188af7130be9251611e41136a17c814f1

josibake commented at 5:45 pm on January 13, 2022: member

reACK https://github.com/bitcoin/bitcoin/pull/23373/commits/7f122a4188af7130be9251611e41136a17c814f1

MarcoFalke merged this on Jan 17, 2022

MarcoFalke closed this on Jan 17, 2022

vasild deleted the branch on Jan 17, 2022

sidhujag referenced this in commit 52b430a3f9 on Jan 18, 2022

DrahtBot locked this on Jan 17, 2023

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

Conflicts