fuzz: Improve fuzzing stability for minisketch harness #29064

pull dergoegge wants to merge 1 commits into bitcoin:master from dergoegge:2023-12-fuzz-stability-minisketch changing 1 files +21 −5

dergoegge commented at 6:00 PM on December 12, 2023: member
The minisketch harness has low stability due to:
- Rng internal to minisketch
- Benchmarkning for the best minisketch impl
Fix this by seeding the rng and letting the fuzzer choose the impl.

Also see #29018.
DrahtBot commented at 6:01 PM on December 12, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Dec 12, 2023

Type	Reviewers
ACK	maflcko

in src/test/fuzz/minisketch.cpp:18 in aee13a6710 outdated

  11 | @@ -12,14 +12,21 @@
  12 |  #include <map>
  13 |  #include <numeric>
  14 |  
  15 | -using node::MakeMinisketch32;
  16 | +namespace {
  17 | +Minisketch MakeFuzzMinisketch32(size_t capacity)
  18 | +{
  19 | +    return Minisketch(32, 0, capacity);

maflcko commented at 6:15 PM on December 12, 2023:

Why hardcode this to 0? Wouldn't it be better to let the fuzz engine pick one of MaxImplementation()?

in src/test/fuzz/minisketch.cpp:27 in aee13a6710 outdated

  25 |      FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
  26 |      const auto capacity{fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 200)};
  27 | -    Minisketch sketch_a{Assert(MakeMinisketch32(capacity))};
  28 | -    Minisketch sketch_b{Assert(MakeMinisketch32(capacity))};
  29 | +    Minisketch sketch_a{Assert(MakeFuzzMinisketch32(capacity))};
  30 | +    Minisketch sketch_b{Assert(MakeFuzzMinisketch32(capacity))};

maflcko commented at 6:15 PM on December 12, 2023:

When touching this, maybe move the Assert into the helper?

in src/test/fuzz/minisketch.cpp:66 in aee13a6710 outdated

  63 | +
  64 |      sketch_ar.Deserialize(sketch_a.Serialize());
  65 |      sketch_br.Deserialize(sketch_b.Serialize());
  66 |  
  67 |      Minisketch sketch_diff{std::move(fuzzed_data_provider.ConsumeBool() ? sketch_a : sketch_ar)};
  68 | +    sketch_diff.SetSeed(fuzzed_data_provider.ConsumeIntegral<uint64_t>());

maflcko commented at 6:15 PM on December 12, 2023:

Is this needed? Both should have been seeded already, no?

maflcko approved
maflcko commented at 6:16 PM on December 12, 2023: member

lgtm
dergoegge marked this as a draft on Dec 13, 2023
dergoegge force-pushed on Dec 14, 2023
maflcko commented at 10:20 AM on December 14, 2023: member

lgtm. Could squash the simple test fix into one commit?

dergoegge commented at 10:22 AM on December 14, 2023: member

Fuzzer now crashes on:

$ echo "HwoKCQoKCgoKCgoKCgkKCgoKvQoLCgoKCgkKCgoKvQoLCgoKCgoJCgoKCgpBCgoKCgpBDg==" | base64 --decode > minisketch.crash
$ FUZZ=minisketch ./src/test/fuzz/fuzz minisketch.crash
test/fuzz/minisketch.cpp:73 minisketch_fuzz_target: Assertion `dec.size() == num_diff' failed.

I think this stems from the fact that the minisketch implementation now might differ between the sketches in the test.

DrahtBot added the label CI failed on Dec 14, 2023
sipa commented at 3:05 PM on December 14, 2023: member

@dergoegge I believe that what's going on is that the new fuzz test invokes Merge on sketches with different implementation values. The library detects this and returns 0 from minisketch_merge, but the C++ wrapper Minisketch::Merge ignores the library result.

sipa commented at 3:18 PM on December 14, 2023: member

This fixes it I think:

--- a/src/test/fuzz/minisketch.cpp
+++ b/src/test/fuzz/minisketch.cpp
@@ -65,7 +65,9 @@ FUZZ_TARGET(minisketch)
     sketch_br.Deserialize(sketch_b.Serialize());
 
     Minisketch sketch_diff{std::move(fuzzed_data_provider.ConsumeBool() ? sketch_a : sketch_ar)};
-    sketch_diff.Merge(fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br);
+    auto& sketch_merge = fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br;
+    if (sketch_diff.GetImplementation() != sketch_merge.GetImplementation()) return;
+    sketch_diff.Merge(sketch_merge);
 
     if (capacity >= num_diff) {
         const auto max_elements{fuzzed_data_provider.ConsumeIntegralInRange<size_t>(num_diff, capacity)};

That may not be the fix you actually want to use, as it's probably better to decode fuzzer data in such a way that the merge is guaranteed to be between consistent implementations, rather than bailing out when it isn't. But it at least demonstrates the problem.

[fuzz] Improve fuzzing stability for minisketch harness

* Seed minisketch rng
* Use fuzzer chosen minisketch impl instead of benchmarking for the best
  impl

b2fc7a2eda

dergoegge force-pushed on Dec 14, 2023
maflcko commented at 8:14 PM on December 14, 2023: member

review ACK b2fc7a2eda103724ac8cbeaf99df3ce6f5b7d974
dergoegge marked this as ready for review on Dec 14, 2023
DrahtBot removed the label CI failed on Dec 14, 2023
fanquake merged this on Dec 18, 2023
fanquake closed this on Dec 18, 2023
bitcoin locked this on Dec 17, 2024

Contributors

Labels

Linked (view graph)

#29018 fuzz: Fix stability, determinism issues

fuzz: Improve fuzzing stability for minisketch harness #29064

Code Coverage

Reviews