Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.
Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.
Concept ACK: very nice fuzzing work @vasild! :)
Tested ACK b0f06d04c1a57b3b440ebccf8482fcee124b6337
Thanks for improving our fuzzing harnesses @vasild! :)
Concept ACK
Tested b0f06d0 with FUZZ=addrman src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/addrman
INFO: Seed: 66461418
INFO: Loaded 1 modules (442033 inline 8-bit counters): 442033 [0x562796653cc0, 0x5627966bfb71),
INFO: Loaded 1 PC tables (442033 PCs): 442033 [0x5627966bfb78,0x562796d7e688),
INFO: 3980 files found in qa-assets/fuzz_seed_corpus/addrman
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 3980 min: 1b max: 1048576b total: 46509329b rss: 109Mb
[#512](/github-metadata-backup-bitcoin-bitcoin/512/) pulse cov: 4448 ft: 10018 corp: 209/12622b exec/s: 256 rss: 132Mb
[#1024](/github-metadata-backup-bitcoin-bitcoin/1024/) pulse cov: 4989 ft: 16253 corp: 342/26Kb exec/s: 128 rss: 154Mb
...
Works as expected.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
b0f06d04c1...0de22a097e: rebase due to conflicts
ACK 0de22a097e89af200d1c6125ba7e5c71fa71e53e
re-ACK 0de22a097e89af200d1c6125ba7e5c71fa71e53e
52 | + const uint8_t net = insecure_rand.randrange(6) + 1; // [1..6] 53 | + 54 | + CDataStream s(SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT); 55 | + 56 | + s << net; 57 | + s << insecure_rand.randbytes(net_len_map.at(net));
Wouldn't it be better to read the address from the fuzz input? Fuzz input isn't uniform and thus can easier reach edge-cases like 000000, which would be non-trivial or next to impossible with a uniform distribution over a large enough range.
Also, how is this different from ConsumeNetAddr?
The problem with consuming addresses directly from the fuzz input is that it is limited in size. This test needs 1000s of addresses. What happened in practice was that some random addresses were consumed followed by 1000s of all-zero addresses due to the fuzzing input being exhausted.
That is the main difference from ConsumeNetAddr().
Maybe add a few fuzzed addresses and fill up the rest with synthetic ones?
Right. The fuzzed provider is not used for anything after consuming the pile of addresses, so it is ok to fully exhaust it. Once it is exhausted, start generating ones from insecure_rand.
0de22a097e...fe9401b75d: rebase due to conflicts and address review suggestion
45 | + * Generate a random address. Always returns a valid address. 46 | + */ 47 | + CNetAddr RandAddr() 48 | + { 49 | + CNetAddr addr; 50 | + if (m_fuzzed_data_provider.remaining_bytes() > 0) {
Maybe allow the fuzz engine to flip-flop between the two branches, if possible?
if (m_fuzzed_data_provider.ConsumeBool()) {
Even better!
Kept the remaining_bytes() check for clarity (it will still work without it because ConsumeBool() would always return false once fuzz input is exhausted but that would be somewhat unclear/obscure).
Concept ACK
fe9401b75d...2fe1efa94c: address review suggestion
Move the addrman init code from the test case to a newly added
`CAddrManDeterministic` constructor. This way it can be reused by other
tests.
2fe1efa94c...1f00d22e12: rebase due to conflicts
6d5e55e755...87651795d8: fix locking in the test's RandAddr(), optimize addrman comparison (3-4x speedup!) and reduce the size of addrmans to avoid tests running so long as to cause a timeout.
optimize addrman comparison (3-4x speedup!) and reduce the size of addrmans to avoid tests running so long as to cause a timeout.
Nice!
cr ACK 87651795d8622d354f8e3c481eb868d9433b841c
91 | + // Add some of the addresses directly to the "tried" table. 92 | + 93 | + // 0, 1, 2, 3 corresponding to 0%, 100%, 50%, 33% 94 | + const size_t n = m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 3); 95 | + 96 | + const size_t num_sources = m_fuzzed_data_provider.ConsumeIntegralInRange<size_t>(10, 50);
Is there a reason to exclude num_sources == 1?
No.
ACK 87651795d8622d354f8e3c481eb868d9433b841c rebased to current master, reviewed, fuzz build, ran FUZZ=addrman_serdeser src/test/fuzz/fuzz
102 | + const size_t num_addresses = insecure_rand.randrange(500) + 1; // [1..500] 103 | + 104 | + for (size_t j = 0; j < num_addresses; ++j) { 105 | + const auto addr = CAddress{CService{RandAddr(), 8333}, NODE_NETWORK}; 106 | + const auto time_penalty = insecure_rand.randrange(100000001); 107 | +#if 1
What is this for?
Change it to #if 0 to enable the equivalent #else branch which is much shorter and clear, but also slower. The #else snippet also shows what the faster snippet is trying to achieve by "manually" fiddling with addrman internals.