test: add fuzz test for private broadcast #35129

vasild commented at 12:57 PM on April 21, 2026: contributor

Add a fuzz test that exercises the public methods of the PrivateBroadcast class from src/private_broadcast.h and checks for correctness.

DrahtBot added the label Tests on Apr 21, 2026

DrahtBot commented at 12:57 PM on April 21, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35129.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs
Concept ACK	frankomosh
Stale ACK	nervana21

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34707 (net: keep finished private broadcast txs in memory by andrewtoth)

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.

vasild commented at 1:02 PM on April 21, 2026: contributor

The test was originally provided by @kevkevinpal in #29415 (comment)

There is a complementary fuzz test in #35090 which exercises the peerman / connman call paths while this one here calls directly the PrivateBroadcast methods.

in src/test/fuzz/private_broadcast.cpp:24 in d31c86f255

  19 | +
  20 | +FUZZ_TARGET(private_broadcast)
  21 | +{
  22 | +    SeedRandomStateForTest(SeedRand::ZEROS);
  23 | +    FuzzedDataProvider fdp(buffer.data(), buffer.size());
  24 | +    SetMockTime(ConsumeTime(fdp));

maflcko commented at 1:17 PM on April 21, 2026:

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?

vasild commented at 8:58 AM on April 22, 2026:

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

maflcko commented at 11:41 AM on April 22, 2026:

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.

brunoerg commented at 1:07 PM on April 22, 2026:

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

vasild commented at 2:00 PM on April 22, 2026:

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.

in src/test/fuzz/private_broadcast.cpp:46 in d31c86f255

  41 | +            return next_nodeid++;
  42 | +        }
  43 | +        return fdp.ConsumeIntegralInRange<NodeId>(0, next_nodeid - 1);
  44 | +    };
  45 | +
  46 | +    while (fdp.remaining_bytes() > 0) {

maflcko commented at 1:19 PM on April 21, 2026:

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.

vasild commented at 9:04 AM on April 22, 2026:

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.

maflcko commented at 1:49 PM on June 16, 2026:

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.

It is about the risk that something is added later on. The while body is more than 100 lines long, and if the fdp was used after that body, it would actually not return data from the fuzz engine, but constants.

I don't think it is that much extra work for the fuzz engine to place some odd bytes. Without a benchmark, it could even help the fuzz engine (depending on the engine and the fuzz target code)

DrahtBot added the label CI failed on Apr 21, 2026

DrahtBot commented at 1:45 PM on April 21, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/24723618894/job/72318638206</sub> <sub>LLM reason (✨ experimental): CI failed because clang-tidy reported a warnings-as-errors performance-unnecessary-copy-initialization in test/fuzz/private_broadcast.cpp (copy-constructing tx from a const reference).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

fanquake added the label Private Broadcast on Apr 21, 2026

in src/test/fuzz/private_broadcast.cpp:40 in d31c86f255

  35 | +    std::unordered_set<NodeId> nodes_that_confirmed_reception;
  36 | +
  37 | +    NodeId next_nodeid{0}; // Generate unique node ids.
  38 | +
  39 | +    const auto ExistentOrNewNodeId = [&next_nodeid, &fdp](){
  40 | +        if (fdp.ConsumeBool() || next_nodeid == 0) {

andrewtoth commented at 6:40 PM on April 21, 2026:

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

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

vasild commented at 9:04 AM on April 22, 2026:

Done.

in src/test/fuzz/private_broadcast.cpp:51 in d31c86f255

  46 | +    while (fdp.remaining_bytes() > 0) {
  47 | +        CallOneOf(
  48 | +            fdp,
  49 | +            [&] { // Add()
  50 | +                CTransactionRef tx;
  51 | +                if (fdp.ConsumeIntegralInRange<uint8_t>(0, 10) != 0 || transactions.empty()) {

andrewtoth commented at 6:48 PM on April 21, 2026:

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()) {

vasild commented at 9:04 AM on April 22, 2026:

Done.

vasild force-pushed on Apr 22, 2026

vasild commented at 8:57 AM on April 22, 2026: contributor

d31c86f255e12c94402c21b7e98f64eb6de93365...0f07f37c0e11b02f4ca2bfb421c95ce2ba922781: address suggestions and pet tidy

in src/test/fuzz/private_broadcast.cpp:55 in 0f07f37c0e outdated

  50 | +                if (transactions.empty() || fdp.ConsumeBool()) {
  51 | +                    tx = MakeTransactionRef(ConsumeTransaction(fdp, std::nullopt));
  52 | +                } else {
  53 | +                    tx = *PickIterator(fdp, transactions);
  54 | +                }
  55 | +                if (pb.Add(tx)) {

brunoerg commented at 1:19 PM on April 22, 2026:

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);
                 }
             },

vasild commented at 2:27 PM on April 22, 2026:

Done.

in src/test/fuzz/private_broadcast.cpp:146 in 0f07f37c0e outdated

 141 | +                    Assert(!transactions.empty());
 142 | +                } else {
 143 | +                    Assert(transactions.empty());
 144 | +                }
 145 | +            },
 146 | +            [&] { // GetStale()

brunoerg commented at 1:22 PM on April 22, 2026:

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.

vasild commented at 2:32 PM on April 22, 2026:

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.

instagibbs commented at 3:56 PM on May 29, 2026:

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

vasild commented at 1:24 PM on June 17, 2026:

... don't mock time each time, just let the CallOneOf choose it ...

Done, thanks!

in src/test/fuzz/private_broadcast.cpp:78 in 0f07f37c0e

  73 | +                    } else {
  74 | +                        ++it;
  75 | +                    }
  76 | +                }
  77 | +
  78 | +                Assert(pb.Remove(tx).has_value());

brunoerg commented at 1:25 PM on April 22, 2026:

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.

vasild commented at 2:33 PM on April 22, 2026:

Done, thanks!

vasild force-pushed on Apr 22, 2026

vasild commented at 2:26 PM on April 22, 2026: contributor

0f07f37c0e11b02f4ca2bfb421c95ce2ba922781...6cca706a155246cf6ed6c3b166416b2f43cd0415: address suggestions

DrahtBot removed the label CI failed on Apr 23, 2026

in src/test/fuzz/private_broadcast.cpp:95 in 6cca706a15

  90 | +                Assert(opt_num_confirmed.value() == num_nodes_that_confirmed_tx);
  91 | +                Assert(!pb.Remove(tx).has_value());
  92 | +                transactions.erase(transactions_it);
  93 | +            },
  94 | +            [&] { // PickTxForSend()
  95 | +                // Only give pristine node ids to PickTxForSend(). If PickTxForSend() is called

instagibbs commented at 3:54 PM on May 29, 2026:

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.

vasild commented at 1:31 PM on June 17, 2026:

Done.

in src/test/fuzz/private_broadcast.cpp:36 in 6cca706a15 outdated

  31 | +
  32 | +    // Ids of nodes that were passed to PickTxForSend(). Trimmed when Remove() is called.
  33 | +    std::unordered_set<NodeId> nodes_sent_to;
  34 | +
  35 | +    // A subset of `nodes_sent_to`, node ids passed to NodeConfirmedReception(). Trimmed when Remove() is called.
  36 | +    std::unordered_set<NodeId> nodes_that_confirmed_reception;

instagibbs commented at 4:01 PM on May 29, 2026:

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));

vasild commented at 1:35 PM on June 17, 2026:

Taken. Tweaked the patch above a little bit - transactions and num_picked become redundant, so use just one map instead of two. Thanks!

instagibbs commented at 4:43 PM on May 29, 2026: member

concept ACK

coverage looks good

nervana21 commented at 3:10 PM on June 1, 2026: contributor

Concept ACK

in src/test/fuzz/private_broadcast.cpp:164 in 6cca706a15 outdated

 159 | +                const auto stale{pb.GetStale()};
 160 | +
 161 | +                Assert(stale.size() <= transactions.size());
 162 | +
 163 | +                for (const auto& stale_tx : stale) {
 164 | +                    Assert(transactions.contains(stale_tx));

frankomosh commented at 11:51 AM on June 4, 2026:

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.

vasild commented at 1:41 PM on June 17, 2026:

That would require a call to GetBroadcastInfo() and search into its result. I am leaving it as it is for now.

frankomosh commented at 5:19 AM on June 24, 2026:

Ok. what I had in mind didn't actually need GetBroadcastInfo(), just tracking time_added/last_confirmed directly in the harness's shadow state. Worked out a diff to confirm it's doable, but it's ~20 extra lines of code that might add a lot of review burden and might not be that useful for now anyways.

frankomosh commented at 11:51 AM on June 4, 2026: contributor

Concept ACK

nervana21 commented at 1:34 PM on June 16, 2026: contributor

tACK 6cca706a155246cf6ed6c3b166416b2f43cd0415

DrahtBot requested review from frankomosh on Jun 16, 2026

DrahtBot requested review from instagibbs on Jun 16, 2026

private broadcast: enforce sending to unique node ids

Sending more than one transaction to a given node would be a privacy
leak and thus enforce that this is not done. `GetSendStatusByNode()`
assumes unique node ids.

Note that sending more than one transaction to a given address is fine,
if that is done via separate connections, in which case the node ids
would be different.

08b7c61fc7

test: add fuzz test for private broadcast

Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

2ee4fafa3f

vasild force-pushed on Jun 17, 2026

vasild commented at 1:41 PM on June 17, 2026: contributor

6cca706a155246cf6ed6c3b166416b2f43cd0415...2ee4fafa3f4887e1f8cfc1a65a980f9c055549ca: address suggestions

instagibbs commented at 2:06 PM on June 17, 2026: member

ACK 2ee4fafa3f4887e1f8cfc1a65a980f9c055549ca

Added the new assertion which avoids/detects peerid reuse, and took suggestions. Will run for a bit.

fanquake removed review request from frankomosh on Jun 17, 2026

fanquake requested review from marcofleon on Jun 17, 2026

DrahtBot requested review from frankomosh on Jun 17, 2026

in src/test/fuzz/private_broadcast.cpp:111 in 2ee4fafa3f

 106 | +                Assert(!pb.Remove(tx).has_value());
 107 | +                transactions.erase(transactions_it);
 108 | +            },
 109 | +            [&] { // PickTxForSend()
 110 | +                // Only give pristine node ids to PickTxForSend() as required.
 111 | +                const NodeId will_send_to_nodeid{next_nodeid++};

nervana21 commented at 10:53 AM on June 22, 2026:

2ee4fafa3f4887e1f8cfc1a65a980f9c055549ca: test: add fuzz test for private broadcast

Now that nodeid reuse is handled, should we update this portion of the fuzzing logic to test that path?

vasild commented at 12:13 PM on June 22, 2026:

How? That is a requirement on the caller.

DrahtBot requested review from nervana21 on Jun 22, 2026

in src/private_broadcast.cpp:39 in 2ee4fafa3f

  32 | @@ -33,6 +33,11 @@ std::optional<CTransactionRef> PrivateBroadcast::PickTxForSend(const NodeId& wil
  33 |  {
  34 |      LOCK(m_mutex);
  35 |  
  36 | +    if (GetSendStatusByNode(will_send_to_nodeid).has_value()) { // nodeid reuse, shouldn't send >1 tx to a given node
  37 | +        Assume(false);
  38 | +        return std::nullopt;
  39 | +    }

nervana21 commented at 10:54 AM on June 22, 2026:

08b7c61fc70393a94a40c6be8280b18fc14a4af7: private broadcast: enforce sending to unique node ids

When there is nodeid reuse and PickTxForSend returns nullopt, the caller may interpret the return value as "there are no transactions in private broadcast queue".

Would something like this be more consistent?

    Assert(!GetSendStatusByNode(will_send_to_nodeid).has_value()); // nodeid reuse, shouldn't send >1 tx to a given node

vasild commented at 12:23 PM on June 22, 2026:

Unsure. That argument more or less can be applied on any Assume(), asking for it to be upgraded to Assert(). Would it be better if non-debug (aka production) crashes? The idea behind the existence of Assume() is that some errors are not so critical as to stop the entire process in non-debug build and at the same time will likely be hit during testing with debug builds in which case they will crash.

All good in principle, but the threshold when to use Assume vs Assert() is sometimes blurry. doc/developer-notes.md mentions:

Assume should be used to document assumptions when program execution can safely continue even if the assumption is violated. In debug builds it behaves like Assert/assert to notify developers and testers about nonfatal errors.

In this case the program can continue. I would pick Assert() if the program cannot continue, e.g.:

int Divide(int x, int y)
{
    Assert(y != 0); // cannot continue in any way if asked to divide by 0.
    return x / y;
}

instagibbs commented at 1:29 PM on June 22, 2026:

current change makes sense to me: Abort broadcast attempt but don't crash, unless debug build

DrahtBot requested review from nervana21 on Jun 22, 2026