[fuzz] Improve transport deserialization fuzz test coverage

dhruv commented at 4:18 am on May 24, 2021: member

This PR has 3 commits that increase the fuzz test coverage:

Before commit 1:

0[#306853](/bitcoin-bitcoin/306853/) REDUCE cov: 798 ft: 5820 corp: 150/375Kb lim: 68333 exec/s: 1382 rss: 461Mb L: 254/63171 MS: 1 EraseBytes-
1[#1453105](/bitcoin-bitcoin/1453105/) REDUCE cov: 798 ft: 5820 corp: 150/369Kb lim: 79613 exec/s: 1467 rss: 461Mb L: 6027/60873 MS: 1 EraseBytes-

After commit 1 (adds serialization to de-serialization test):

0[#303389](/bitcoin-bitcoin/303389/) NEW cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
1[#1428759](/bitcoin-bitcoin/1428759/) REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-

After commit 2 (provides an occasional checksum assist to the fuzzer inputs):

0[#304820](/bitcoin-bitcoin/304820/) NEW cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
1[#1416181](/bitcoin-bitcoin/1416181/) REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-

After commit 3 (provides an occasional magic bytes assist to the fuzzer inputs):

0[#302684](/bitcoin-bitcoin/302684/) NEW cov: 1454 ft: 3936 corp: 84/7056b lim: 2424 exec/s: 4146 rss: 477Mb L: 65/1108 MS: 3 CopyPart-CrossOver-CMP- DE: "\x0e\x00\x00\x00"-
1[#1383925](/bitcoin-bitcoin/1383925/) REDUCE cov: 1454 ft: 4828 corp: 102/14573b lim: 4096 exec/s: 3954 rss: 534Mb L: 116/4050 MS: 2 EraseBytes-ChangeByte-

If reviewers only accept the first commit, the seeds are not invalidated and new seeds are at: https://github.com/bitcoin-core/qa-assets/pull/61. In this case, we can also revert the test name change.

If reviewers accept all three commits, the existing seeds are invalidated.

fanquake added the label Tests on May 24, 2021

practicalswift commented at 8:04 pm on May 24, 2021: contributor

Concept ACK

Note that the harness rename will unnecessarily invalidate all existing seeds. Non-blocking suggestion: could skip the rename? :)

dhruv commented at 8:26 pm on May 24, 2021: member

Note that the harness rename will unnecessarily invalidate all existing seeds. Non-blocking suggestion: could skip the rename? :)

I renamed the directory for seeds in the first commit here. Will seeds still be invalidated? I’m happy to skip the rename if that is not sufficient.

Separately, I added some instrumentation and saw 67% of the seeds are not getting more coverage because of an invalid dbl-sha256 checksum - the fuzzer could use a little help. Another 30% because of an invalid message type. I am preparing a patch that uses ConsumeBool() to inject a valid checksum half the time. I’ll report back if I see meaningfully increased coverage. Do you think that would be a good change, @practicalswift ?

MarcoFalke commented at 7:50 am on May 25, 2021: member

Agree that the rename is better skipped

[fuzz] Add serialization to deserialization test

Before commit:
306853	REDUCE cov: 798 ft: 5820 corp: 150/375Kb lim: 68333 exec/s: 1382 rss: 461Mb L: 254/63171 MS: 1 EraseBytes-
1453105	REDUCE cov: 798 ft: 5820 corp: 150/369Kb lim: 79613 exec/s: 1467 rss: 461Mb L: 6027/60873 MS: 1 EraseBytes-

After commit:
303389	NEW    cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
1428759	REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-

654472a461

[fuzz] Occasional valid checksum for transport serialization fuzz test

Before commit:
Unable to deserialize: 0%
Wrong message start  : ~1.27%
Header too large     : ~0.5%
Wrong checksum       : ~67.99%
Invalid message type : ~30.1%

303389	NEW    cov: 1202 ft: 8382 corp: 157/382Kb lim: 68189 exec/s: 1451 rss: 447Mb L: 1386/65459 MS: 1 CopyPart-
1428759	REDUCE cov: 1202 ft: 8512 corp: 169/389Kb lim: 78749 exec/s: 1528 rss: 463Mb L: 1627/60488 MS: 1 EraseBytes-

After commit(new seeds; old seeds invalidated):
Unable to deserialize: 0%
Wrong message start  : ~45.62%
Header too large     : ~14.5%
Wrong checksum       : ~38.13%
Invalid message type : ~1.78%

304820	NEW    cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
1416181	REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-

35571d8d9e

[fuzz] Occasional valid magic bytes for transport serialization test

Before commit:
Unable to deserialize : 0%
Wrong message start   : ~45.62%
Header too large      : ~14.5%
Wrong checksum        : ~38.13%
Invalid message type  : ~1.78%

304820	NEW    cov: 1440 ft: 4452 corp: 92/12551b lim: 2237 exec/s: 3386 rss: 486Mb L: 47/1111 MS: 1 ChangeByte-
1416181	REDUCE cov: 1442 ft: 5681 corp: 125/59Kb lim: 4096 exec/s: 3522 rss: 535Mb L: 2164/4049 MS: 1 EraseBytes-

After commit:
Unable to deserialize : 0%
Wrong message start   : ~39.6%
Header too large      : ~30.85%
Wrong checksum        : ~25.54%
Invalid message type  : ~4.01%

302684	NEW    cov: 1454 ft: 3936 corp: 84/7056b lim: 2424 exec/s: 4146 rss: 477Mb L: 65/1108 MS: 3 CopyPart-CrossOver-CMP- DE: "\x0e\x00\x00\x00"-
1383925	REDUCE cov: 1454 ft: 4828 corp: 102/14573b lim: 4096 exec/s: 3954 rss: 534Mb L: 116/4050 MS: 2 EraseBytes-ChangeByte-

e337145577

dhruv force-pushed on May 25, 2021

dhruv renamed this:
~~[fuzz] Add serialization to transport deserialization test~~
[fuzz] Improve transport deserialization fuzz test coverage
on May 25, 2021

dhruv commented at 3:32 pm on May 25, 2021: member

Thank you @MarcoFalke, @practicalswift. I ended up adding two more commits which meaningfully increased coverage by on occasion, assisting the fuzzer inputs with valid magic bytes and checksums (see updated PR description). These will invalidate the seeds. Please take another look when you can. If the feedback is to skip the last two commits, I will revert the name change.

Ready for further review.

practicalswift commented at 9:28 pm on May 25, 2021: contributor

Tested ACK e33714557747dd479f123425aa2dd08d272ef377

Coverage is increased significantly.

Thanks for improving the fuzzing harnesses!

in src/Makefile.test.include:260 in e337145577

256@@ -257,7 +257,7 @@ test_fuzz_fuzz_SOURCES = \
257  test/fuzz/netaddress.cpp \
258  test/fuzz/netbase_dns_lookup.cpp \
259  test/fuzz/node_eviction.cpp \
260- test/fuzz/p2p_transport_deserializer.cpp \
261+ test/fuzz/p2p_transport_serialization.cpp \

MarcoFalke commented at 6:38 am on May 26, 2021:

still moved?

dhruv commented at 2:51 pm on May 26, 2021:

The new commits invalidate the seeds anyway so I thought may as well have a more appropriate name. Am I missing a reason to skip changing the name?

laanwj merged this on May 27, 2021

laanwj closed this on May 27, 2021

sidhujag referenced this in commit 2041fface3 on May 27, 2021

dhruv added this to the "Done" column in a project

gwillen referenced this in commit 92436f454e on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

[fuzz] Improve transport deserialization fuzz test coverage #22029