Similar to all other tests (unit tests, bench tests, gui unit tests, …), the fuzz tests should also be built by default.
So, --enable-fuzz should be changed to --disable-fuzz.
See #19366 (comment)
build system background
For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.
@michaelfolkson This is not about running them, but building the fuzz test code - without actual fuzzing enabled. The advantage is that you’d notice when a change you’re making breaks the fuzz tests.
This is similar to why we build the benchmarks by default, even when you’re not interested in benchmarking: even if you don’t, the benchmarks shouldn’t break.
Actual fuzzing requires re-building with a whole different set of compilation options (and possibly a different compiler). Making that easier would be great too, but seems pretty hard. Yet, the code that implements the tests should be buildable easily by anyone who can build the rest of the code.
I’d like to take a stab at this to try and get a bit more familiar with the build system…
However at first glance it looks like compiling fuzz tests by default would require c++17 which might make this less desirable. For example, std::optional is c++17 only:
0$ git grep -c "std::optional"
1src/optional.h:1
2src/test/fuzz/addrdb.cpp:1
3src/test/fuzz/asmap_direct.cpp:1
4src/test/fuzz/block_header.cpp:2
5src/test/fuzz/blockfilter.cpp:1
6src/test/fuzz/bloom_filter.cpp:3
7src/test/fuzz/chain.cpp:1
8src/test/fuzz/coins_view.cpp:4
9src/test/fuzz/flatfile.cpp:2
10src/test/fuzz/merkleblock.cpp:1
11src/test/fuzz/policy_estimator.cpp:2
12src/test/fuzz/pow.cpp:2
13src/test/fuzz/primitives_transaction.cpp:3
14src/test/fuzz/protocol.cpp:2
15src/test/fuzz/rbf.cpp:2
16src/test/fuzz/rolling_bloom_filter.cpp:1
17src/test/fuzz/script.cpp:2
18src/test/fuzz/script_interpreter.cpp:2
19src/test/fuzz/script_sigcache.cpp:2
20src/test/fuzz/script_sign.cpp:7
21src/test/fuzz/util.h:1
It also increases my compile time by ~60% (from 440s to 730s), which might also not be desirable as the standard compile options.
Would this still be something that we would want to make the default based on the above?
Ah I’ve now found the discussion in #16684 …
So it seems like the question now is, is it more desirable to open a PR now and let it sit unmerged until c++17 becomes default, or just waiting?
@MarcoFalke it looks like the issue I am seeing is that the fuzz-test flag is fundamentally incompatible with the wallet being built. If I configure with --disable-wallet then I don’t see the linker errors (on my machine, travis still complains about other stuff sometimes).
I was then easily able to find an issue about this already: #16094
I’m not sure how to reconcile building the wallet and the fuzz tests in this case, do you have any ideas on this front?
@MarcoFalke I’ve confirmed that building (most of) the travis configs work without --disable-wallet or --disable-tests.
Re. lib_test: is this a file getting built by test/util/setup_common.cpp?
Yes everything in test/util is put into libtest_util.a
What is the link error you are seeing?
This is the link error. I just wasn’t sure where to look for lib_test to split it…
0$ ./configure --enable-wallet --enable-fuzz --enable-zmq --with-gui=qt5 --enable-glibc-back-compat --enable-reduce-exports --enable-c++17 --enable-debug CFLAGS="-g0 -O2 -funsigned-char" CXXFLAGS="-g0 -O2 -funsigned-char"
1 CXXLD test/fuzz/block_filter_deserialize
2/usr/bin/ld: libbitcoin_server.a(libbitcoin_server_a-init.o): in function `WalletInit::Construct(NodeContext&) const':
3init.cpp:(.text+0x1f49): undefined reference to `interfaces::MakeWalletClient(interfaces::Chain&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >)'
4collect2: error: ld returned 1 exit status
5make[2]: *** [Makefile:7913: test/fuzz/addrman_deserialize] Error 1
6make[2]: *** Waiting for unfinished jobs....
7/usr/bin/ld: libbitcoin_server.a(libbitcoin_server_a-init.o): in function `WalletInit::Construct(NodeContext&) const':
8init.cpp:(.text+0x1f49): undefined reference to `interfaces::MakeWalletClient(interfaces::Chain&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >)'
9collect2: error: ld returned 1 exit status
10make[2]: *** [Makefile:7949: test/fuzz/block] Error 1
11make[2]: Leaving directory '/home/will/src/bitcoin/src'
12make[1]: *** [Makefile:18288: all-recursive] Error 1
13make[1]: Leaving directory '/home/will/src/bitcoin/src'
14make: *** [Makefile:788: all-recursive] Error 1
LIBBITCOIN_WALLET should be empty when the wallet is disabled
How hard is it to revert the c++17isms in the fuzz tests? I would expect it’s only minimal (use Optional instead of std::optional etc)
That would be trivial :)
Context: Rationale to the C++17-switch for the fuzzers: #18901 (comment) + #18901#pullrequestreview-408557865.
@MarcoFalke taking another look at this now… Adding LIBBITCOIN_WALLET to FUZZ_SUITE_LD_COMMON if WALLET_ENABLED is set doesn’t seem to fix the link issue. The errors I’m seeing look like this: https://termbin.com/fm6d
Do you think this means that we’ll have to split test_bitcoin in two as you suggested earlier, or is there something else I might be missing here?
@willcl-ark Are you still working on this?
No @MarcoFalke I eventually gave up. I was trying to learn more about the build system by taking it on, but was a bit more than I could chew!
A suggestion I made a few days ago on IRC:
017:34:26< sipa> my biggest gripe with the fuzz builds is that it's impossible to even do any compilation of the fuzz
1 code without a separate configure step
217:34:46< sipa> after merging them all in one binary it'd be easy to permit building the fuzz binary without fuzzing
3 enabled, though
417:34:49< wumpus> making say the fuzz objects .fuzz.o would also prevent them accidentally being linked into a non-fuzz
5 binary (or the other wayr around)
617:35:44< wumpus> yes, that might help
717:41:41< sipa> it'd be great if we could have a .fuzz.o for everything
817:41:48< sipa> not sure how much work it is
917:43:44< wumpus> i trust cfields is right that this is hard to realize with auto* build system for some reason
1017:49:21< sipa> we could even make a "dummy" fuzz binary when building without fuzzing that can only run existing seeds
11 through the fuzz tests
1217:49:50< sipa> which would mean you can do the CI fuzz things without separate build
1317:52:44< aj> sipa: ooo! that's a good idea!
1417:54:51 * sipa suggests calling it phonyfuzz
1517:54:57< aj> phuzz
1617:55:15< aj> faux+fuzz=fauzz?
1717:55:23< fanquake> .PHONY
1818:06:45< wumpus> let's try to merge [#20560](/bitcoin-bitcoin/20560/) soon then
Hi,
I think I’ve got this working, though it was easy so I’m a little worried that I’ve done something wrong. I’ll work on a pull request in a bit.