Add fuzzing harness for CheckBlock(...) and other CBlock related functions.
Testing this PR
Run:
0$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
1$ make
2$ src/test/fuzz/block
3…
4# And to to quickly verify that the relevant code regions are triggered, that the
5# fuzzing throughput seems reasonable, etc.
6$ contrib/devtools/test_fuzzing_harnesses.sh '^block$'
test_fuzzing_harnesses.sh can be found in PR #17000.
24@@ -25,6 +25,7 @@ static bool read_stdin(std::vector<uint8_t>& data)
25 static void initialize()
26 {
27 const static auto verify_handle = MakeUnique<ECCVerifyHandle>();
28+ SelectParams(CBaseChainParams::REGTEST);
In commit 115bafe488ef0c53a88c2bc1e1b0508867c1daa8:
Not sure if we want to pick regtest for all tests. I’d argue to pick main.
Or maybe leave it to the individual tests to pick the params? This will result in a (slight) run time cost, but might make the code more clear?
It not always entirely clear which functions that indirectly depend on params being selected and the requirement can be quite far down in the call graph.
An example is from when I fuzzed EvalDescriptorStringOrObject:
When using this harness the fuzzer managed to create the input string "addr()" after roughly three million executions. That input triggered the following assertion failure:
0[#3170783](/bitcoin-bitcoin/3170783/) REDUCE cov: 3338 ft: 7992 corp: 325/63Kb exec/s: 1258 rss: 272Mb L: 61/4090 MS: 2 ChangeASCIIInt-EraseBytes-
1fuzzer: chainparams.cpp:385: const CChainParams &Params(): Assertion `globalChainParams' failed.
2...
3\"addr()\"
Turns out EvalDescriptorStringOrObject calls Parse which calls DecodeDestination which in turn depends on params being selected.
Since the requirement can run quite deep I think we should pick a network in the general initialize() so that harness writers don’t have to worry about crossing any code paths with params requirements that will generate faux crashes.
OTOH, I don’t feel strongly about the particular choice of network - I just choose regtest since that seemed like the least adventurous choice :)
What would be the pros/cons of going with main instead of going regtest (or test)?
I’d rather want to test against a (clean-as-possible) mainnet, than a testnet. However, some tests might want to use segwit or lower difficulty blocks, which is enabled only on regtest from genesis.
I think the choice really depends on each individual test.
I see that you run into a particular issue with the chainparams, but similarly a fuzzer might find a code path that requires connman or a mempool, or anything else. In that case we’d have to initialize the full (Basic)Testingsetup here. Which in turn is not only expensive and wasteful to do, but also hard to reset for each input. So I’d reiterate my previous point that this should probably be picked for each test case either:
SelectParamsBasicTestingSetupTestingSetup
Feedback addressed. Now allowing individual tests to pick the params.
Picking regtest for this override. regtest feels like the least risky option in case of fuzzer gone haywire. Think about the theoretical scenario of a fuzzer say writing to disk for some strange reason. Also nice with low difficulty :)
Please re-review.
0@@ -0,0 +1,58 @@
1+// Copyright (c) 2009-2019 The Bitcoin Core developers
33+ return;
34+ }
35+
36+ const Consensus::Params& consensus_params = Params().GetConsensus();
37+ CValidationState validation_state_pow_and_merkle;
38+ const bool valid_incl_pow_and_merkle = CheckBlock(block, validation_state_pow_and_merkle, consensus_params, true, true);
style-nit: Could use named arguments?
0/* bla */ true, /* foo */ false
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
ContextualCheckBlock(…).
When fuzzing BlockWitnessMerkleRoot(…) I noticed that leaves[0].SetNull() is done also in the case when leaves.empty() (due to block.vtx.empty()):
Makes sense to guard against this? :)
Update: Working around this by simply skipping the call to BlockWitnessMerkleRoot(…) in case of block.vtx.empty().
223@@ -224,11 +224,11 @@ test_test_bitcoin_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
224 endif
225
226 if ENABLE_FUZZ
227-test_fuzz_block_deserialize_SOURCES = $(FUZZ_SUITE) test/fuzz/deserialize.cpp
228-test_fuzz_block_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DBLOCK_DESERIALIZE=1
15+#include <version.h>
16+
17+#include <cassert>
18+#include <string>
19+
20+bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
39 const Consensus::Params& consensus_params = Params().GetConsensus();
40- CValidationState validation_state_pow_and_merkle;
41+
42+ BlockValidationState validation_state_contextual;
43+ const CBlockIndex* pindex_prev = nullptr;
44+ (void)ContextualCheckBlock(block, validation_state_contextual, consensus_params, pindex_prev);
in commit c5d08da5c38e4140f4b9465f3e8159bcd7874ab2:
On a second thought I am not sure if it makes sense to expose this internal function for the sole purpose to pass a nullptr to it, which makes it execute pretty much no code paths anyway. Also, passing a nullptr never happens in reality.
OK, reverted to previous version (see #17071 (review)).
Please re-review :)
pindex_prev is passed as a nullptr to ContextualCheckBlock, which then runs almost no code paths anyway. Not sure what the potential gains are of testing something that can never happen in reality.
ContextualCheckBlock. Please re-review :)
Labels
Tests