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.
Tested ACK b0f06d04c1a57b3b440ebccf8482fcee124b6337
Thanks for improving our fuzzing harnesses @vasild! :)
Tested b0f06d0 with FUZZ=addrman src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/addrman
0INFO: Seed: 66461418
1INFO: Loaded 1 modules (442033 inline 8-bit counters): 442033 [0x562796653cc0, 0x5627966bfb71),
2INFO: Loaded 1 PC tables (442033 PCs): 442033 [0x5627966bfb78,0x562796d7e688),
3INFO: 3980 files found in qa-assets/fuzz_seed_corpus/addrman
4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
5INFO: seed corpus: files: 3980 min: 1b max: 1048576b total: 46509329b rss: 109Mb
6[#512](/bitcoin-bitcoin/512/) pulse cov: 4448 ft: 10018 corp: 209/12622b exec/s: 256 rss: 132Mb
7[#1024](/bitcoin-bitcoin/1024/) pulse cov: 4989 ft: 16253 corp: 342/26Kb exec/s: 128 rss: 154Mb
8...
Works as expected.
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.
b0f06d04c1...0de22a097e: rebase due to conflicts
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().
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?
0 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).
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!
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);
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
#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.