test: add fuzz test for private broadcast #35129

Please don't use the legacy SetMockTime, which modifies the global and leaks the value between test cases. Probably fine here, but for consistency, I'd avoid it and use NodeClockContext.

Also, it seems odd to hold the value constant, this will limit the possible coverage?

It seems unnecessary to mock the time, removed SetMockTime() altogether.

It seems unnecessary to mock the time, removed SetMockTime() altogether.

Huh? Running the fuzz target immediately prints:

The current fuzz target accessed system time.

This is acceptable, but requires the fuzz target to use 
a NodeClockContext, SteadyClockContext or call 
SetMockTime() at the 
beginning of processing the 
fuzz input.

Without setting mock time, time-dependent behavior can lead 
to non-reproducible bugs or inefficient fuzzing.

Also, the private broadcast module certainly uses time, so I don't understand why it would be unnecessary.

For reference, NodeConfirmedReception, GetStale and PickTxForSend call NodeClock::now().

Alright, 3rd attempt: added NodeClockContext clock_ctx{ConsumeTime(fdp)}; and then on every iteration change it with clock_ctx.set(ConsumeTime(fdp));. That would allow the time to go backwards, but should be ok to test as it might happen in the real world due to clock adjustment. Shouldn't cause misbehavior from the PrivateBroadcast class.

Please use the LIMITED_WHILE pattern with ConsumeBool(). Otherwise, if something is added later, it will be silently dead code. Of course, it is fine right now, but just taking the risk in the future seems not worth it.

Changed to LIMITED_WHILE(fdp.ConsumeBool(), 10000).

I am just curious why that is preferred over LIMITED_WHILE(fdp.remaining_bytes() > 0, 10000). To me the logic should be - as long as there is data, keep going. That is better described by the latter. The ConsumeBool() variant seems to create extra unnecessary work for the fuzzer to figure out that trues should be planted in specific places in the data to avoid premature exit.

No need to consume input if we are already going in this path.

        if (next_nodeid == 0 || fdp.ConsumeBool()) {

Done.

No need to consume input if we are already going in this path. Using ConsumeIntegralInRange<uint8_t>(0, 10) != 0 is an anti-pattern in fuzz targets. The fuzzer will explore all paths regardless. It doesn't respect the hand-crafted probability distribution, which just wastes time trying to find inputs that reach both sides. ConsumeBool() gives the fuzzer a clean branch to explore both. The comments // New transaction most of the time. and // A duplicate occasionally. should also be removed.

                if (transactions.empty() || fdp.ConsumeBool()) {

Done.

feel free to ignore but you might assert that a tx from transactions is not added, assuming you're using PickIterator here to exercise adding a tx that has been already added, e.g.:

@@ -47,12 +47,15 @@ FUZZ_TARGET(private_broadcast)
             fdp,
             [&] { // Add()
                 CTransactionRef tx;
+                bool from_transactions{false};
                 if (transactions.empty() || fdp.ConsumeBool()) {
                     tx = MakeTransactionRef(ConsumeTransaction(fdp, std::nullopt));
                 } else {
                     tx = *PickIterator(fdp, transactions);
+                    from_transactions = true;
                 }
                 if (pb.Add(tx)) {
+                    Assert(!from_transactions);
                     transactions.emplace(tx);
                 }
             },

Done.

I think GetStale() testing will be more effective if you put one more lambda that would advance mock time (assuming you must mock time in this harness btw). Otherwise, it looks Assert(stale.size() <= transactions.size()) is trivially true.

Now the test changes the mock time. Not in one lambda but on every iteration, I guess it is fine either way. It could also make it go back, in addition to advancing it forward.

Instead of just checking whether Remove has or not value, you could for each tx, count how many of its sent-to nodes are in nodes_that_confirmed_reception, and assert *pb.Remove(tx) == expected_confirmed.

Done, thanks!