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.

mut-nit: Think it's slightly more efficient in terms of input if we don't mock time each time, just let the CallOneOf choose it as a distinct step

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!

6cca706a155246cf6ed6c3b166416b2f43cd0415

If we expect caller to never do this repeat of nodeid, can we just add an assertion near the top of PickTxForSend?

Assume(!GetSendStatusByNode(will_send_to_nodeid).has_value());

Would make the simulation here more complete, would let us remove this block of text aside from stating we're not repeating nodeids as required.

If we have to support it anyways, then obviously this wouldn't work.

inspired from my other now drafted PR, you can check the number of times things have been selected for broadcast, and smallest number of selections is the highest priority:

  --- a/src/test/fuzz/private_broadcast.cpp
  +++ b/src/test/fuzz/private_broadcast.cpp
  @@ -16,6 +16,9 @@
   #include <util/overflow.h>
   #include <util/time.h>

  +#include <algorithm>
  +#include <limits>
  +#include <unordered_map>
   #include <unordered_set>

   FUZZ_TARGET(private_broadcast)
  @@ -29,6 +32,11 @@
       // Random transaction that the test generated and passed to Add(). Trimmed when Remove() is called.
       std::unordered_set<CTransactionRef> transactions;

  +    // For each tracked transaction, the number of times it has been picked for sending, i.e. how many
  +    // send-statuses it should hold. Its key set always mirrors `transactions`. Lets us cross-check both
  +    // PickTxForSend()'s prioritization and the per-tx peer counts reported by GetBroadcastInfo().
  +    std::unordered_map<CTransactionRef, size_t> num_picked;
  +
       // Ids of nodes that were passed to PickTxForSend(). Trimmed when Remove() is called.
       std::unordered_set<NodeId> nodes_sent_to;

  @@ -59,6 +67,7 @@
                   if (pb.Add(tx)) {
                       Assert(!from_transactions);
                       transactions.emplace(tx);
  +                    num_picked.emplace(tx, 0);
                   }
               },
               [&] { // Remove()
  @@ -89,6 +98,7 @@
                   Assert(opt_num_confirmed.has_value());
                   Assert(opt_num_confirmed.value() == num_nodes_that_confirmed_tx);
                   Assert(!pb.Remove(tx).has_value());
  +                num_picked.erase(tx);
                   transactions.erase(transactions_it);
               },
               [&] { // PickTxForSend()
  @@ -100,6 +110,14 @@
                   // as confirmed. Later we would not be sure which transaction was removed by
                   // Remove() and would not be sure whether to leave the node id in
                   // nodes_that_confirmed_reception[].
  +                // num_picked is the primary key in Priority's comparison (fewest sends = highest
  +                // priority), so PickTxForSend() must return a transaction with the minimum send count
  +                // of any in the queue. Ties are broken by state we don't model, so only check this key.
  +                size_t min_picked{std::numeric_limits<size_t>::max()};
  +                for (const auto& [tx, n] : num_picked) {
  +                    min_picked = std::min(min_picked, n);
  +                }
  +
                   const NodeId will_send_to_nodeid{next_nodeid++};
                   const CService will_send_to_address{ConsumeService(fdp)};

  @@ -107,6 +125,10 @@

                   if (opt_tx.has_value()) {
                       Assert(transactions.contains(opt_tx.value()));
  +                    const auto picked_it{num_picked.find(opt_tx.value())};
  +                    Assert(picked_it != num_picked.end());
  +                    Assert(picked_it->second == min_picked); // picked the least-sent transaction
  +                    ++picked_it->second;                     // PickTxForSend() recorded exactly one send
                       const auto& [_, inserted]{nodes_sent_to.emplace(will_send_to_nodeid)};
                       Assert(inserted);
                   } else {
  @@ -171,6 +193,7 @@

                   for (const auto& info : all_broadcast_info) {
                       Assert(transactions.contains(info.tx));
  +                    Assert(info.peers.size() == num_picked.at(info.tx)); // exactly the sends we recorded
                   }
               });
           clock_ctx.set(ConsumeTime(fdp));

nit: Maybe, on top of checking that each returned tx is one we know about, we can additionally check time condition ? Maybe track time_added per tx and last_confirmed per node and assert the criterion directly, if it’s worth it.