fuzz: add p2p_private_broadcast harness #35090

frankomosh commented at 8:01 AM on April 16, 2026: contributor

Add a fuzz harness for ConnectionType::PRIVATE_BROADCAST, a privacy-preserving transaction relay mechanism whose p2p code paths had no meaningful fuzz coverage.

Current process_message touches it but is insufficient in exercising it. It creates PRIVATE_BROADCAST nodes via ConsumeNode(), but some structural problems prevent it from covering the relevant logic:

m_tx_for_private_broadcast is never seeded, PushPrivateBroadcastTx always takes the immediate disconnect path (7 accidental hits, all on lines 3559–3562). Lines 3564–3570 (the actual INV send) had 0 hits.
ALL_NET_MESSAGE_TYPES is used as the message pool. CConnman::PushMessage silently drops anything outside the five-type allowlist for private broadcast connections, wasting most iterations.
Connection types are picked randomly, hence private broadcast coverage is accidental.

To solve the issues above;

this harness explicitly constructs nodes with ConnectionType::PRIVATE_BROADCAST
seeds m_tx_for_private_broadcast via InitiateTxBroadcastPrivate before the peer connects, so PushPrivateBroadcastTx reaches the transaction send path
constrains the message pool to the five types permitted by CConnman::PushMessage on private broadcast connections (VERSION, VERACK, GETDATA, PONG)
passes {NODE_NONE} to InitializeNode, matching what PushNodeVersion advertises for private broadcast peers.

DrahtBot added the label Fuzzing on Apr 16, 2026

DrahtBot commented at 8:02 AM on April 16, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	vasild

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

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

ConsumeDuration<std::chrono::seconds>(fuzzed_data_provider, 0s, 600s) in src/test/fuzz/p2p_private_broadcast.cpp

2026-04-20 13:32:45

fanquake commented at 8:08 AM on April 16, 2026: member

cc @vasild

frankomosh commented at 8:09 AM on April 16, 2026: contributor

Also measured some coverage reports with build_cov (clang, -fprofile-instr-generate -fcoverage-mapping). Before and After.

Some of the key coverage highlight diffs are as follows:

<details> <summary><code>PushPrivateBroadcastTx</code>, send-INV path (lines 3564–3570)</summary>

-  3564|      0|    const CTransactionRef& tx{*opt_tx};
-  3566|      0|    LogDebug(BCLog::PRIVBROADCAST, "P2P handshake completed, ...
-  3570|      0|    MakeAndPushMessage(node, NetMsgType::INV, ...
+  3564|     19|    const CTransactionRef& tx{*opt_tx};
+  3566|     19|    LogDebug(BCLog::PRIVBROADCAST, "P2P handshake completed, ...
+  3570|     19|    MakeAndPushMessage(node, NetMsgType::INV, ...

</details>

<details> <summary><code>InitiateTxBroadcastPrivate</code>, first coverage</summary>

-   |  Branch (2270:9): [True: 0, False: 0]
-  2271|      0|        m_connman.m_private_broadcast.NumToOpenAdd(NUM_PRIVATE_BROADCAST_PER_TX);
+   |  Branch (2270:9): [True: 1.01k, False: 0]
+  2271|  1.01k|        m_connman.m_private_broadcast.NumToOpenAdd(NUM_PRIVATE_BROADCAST_PER_TX);

</details>

<details> <summary><code>FinalizeNode</code>, private broadcast retry branch (line 1747)</summary>

-   |  Branch (1745:9): [True: 0, False: 13]
-  1747|      0|        m_connman.m_private_broadcast.NumToOpenAdd(1);
+   |  Branch (1745:9): [True: 1.02k, False: 0]
+  1747|  1.02k|        m_connman.m_private_broadcast.NumToOpenAdd(1);

</details>

<details> <summary><code>SendMessages</code>, private broadcast timeout branch (lines 5743–5748)</summary>

-  5743|      0|        if (node.m_connected + PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME < current_time) {
-  5746|      0|            node.fDisconnect = true;
-  5748|      0|        return true;
+  5743|      7|        if (node.m_connected + PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME < current_time) {
+  5746|      7|            node.fDisconnect = true;
+  5748|      7|        return true;

</details>

<details> <summary>VERACK handler, private broadcast path</summary>

-  3854|  2.06k|        if (pfrom.IsPrivateBroadcastConn()) {
-   |  Branch (3854:13): [True: 7, False: 2.05k]
-  3861|      7|            PushPrivateBroadcastTx(pfrom);
+  3854|  2.08k|        if (pfrom.IsPrivateBroadcastConn()) {
+   |  Branch (3854:13): [True: 26, False: 2.05k]
+  3861|     26|            PushPrivateBroadcastTx(pfrom);

</details>

in src/test/fuzz/p2p_private_broadcast.cpp:51 in f1eae996ba outdated

  46 | +
  47 | +FUZZ_TARGET(p2p_private_broadcast, .init = ::initialize)
  48 | +{
  49 | +    SeedRandomStateForTest(SeedRand::ZEROS);
  50 | +    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
  51 | +

maflcko commented at 8:19 AM on April 16, 2026:

nit use: auto& node{g_setup->m_node}; here?

frankomosh commented at 8:41 AM on April 16, 2026:

Thanks

in src/test/fuzz/p2p_private_broadcast.cpp:60 in f1eae996ba

  55 | +    NodeClockContext clock_ctx{1610000000s};
  56 | +    chainman.ResetIbd();
  57 | +
  58 | +    node::Warnings warnings{};
  59 | +    auto netgroupman{NetGroupManager::NoAsmap()};
  60 | +    AddrMan addrman{netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0};

maflcko commented at 8:21 AM on April 16, 2026:

Is this copied from an older code before commit fabf8d1c5bdb6a944762792cdc762caa6c1a760b? Because it seems you are re-introducing the issue (or at least the risk of the issue) that was fixed and worked around in that pull request?

frankomosh commented at 8:40 AM on April 16, 2026:

Yes you are right. Seems I was working from an older branch. I'll update this to reflect the changes

maflcko commented at 8:45 AM on April 16, 2026:

If it was written by an LLM, it could also be that it had the old code in memory?

frankomosh commented at 8:47 AM on April 16, 2026:

It's not written by llm. Or does anything seems to suggest otherwise ?

maflcko commented at 8:54 AM on April 16, 2026:

No indication. I just wondered how this could have happened, because the commit f1eae996ba69badd8a81b4a60da63b9527f78634 is from today and based on a recent main commit. It is fine, I was just curious :sweat_smile:

maflcko commented at 10:54 AM on April 17, 2026:

Ah, you copied from src/test/fuzz/p2p_handshake.cpp? Maybe commit https://github.com/bitcoin/bitcoin/commit/fabf8d1c5bdb6a944762792cdc762caa6c1a760b should be applied to that file as well?

frankomosh commented at 4:26 AM on April 18, 2026:

Seemingly so, I can see that it also calls peerman→SendMessages(), so would probably also need the fabf8d1 fix. Would it be appropriate to add it here as a second commit or just open a separate PR? Vasild has proposed deduplicating some features here #35090 (review), Do you think that would help?

DrahtBot added the label CI failed on Apr 16, 2026

frankomosh force-pushed on Apr 16, 2026

frankomosh commented at 10:20 AM on April 16, 2026: contributor

Replaced local AddrMan, NetGroupManager, and node::Warnings with the node context members, following the pattern change by @maflcko from https://github.com/bitcoin/bitcoin/commit/fabf8d1c5bdb6a944762792cdc762caa6c1a760b, that I had earlier missed. edit. also added the reset block

vasild commented at 11:43 AM on April 16, 2026: contributor

Concept ACK would be nice to have fuzz tests for private broadcast.

I had a fuzz tests wip commit at some point, will try to dig it out and compare with this.

Added to #34476

Thanks!

in src/test/fuzz/p2p_private_broadcast.cpp:101 in 694d42ad8c

  96 | +        *pb_node,
  97 | +        ServiceFlags{NODE_NONE}); // Private broadcast peers advertise NODE_NONE (see PushNodeVersion).
  98 | +
  99 | +    CNode& p2p_node = *pb_node.release();
 100 | +
 101 | +    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)

andrewtoth commented at 12:50 PM on April 16, 2026:

Break line below can be added here.

    LIMITED_WHILE(!p2p_node.fDisconnect && fuzzed_data_provider.ConsumeBool(), 100)

frankomosh commented at 2:00 PM on April 16, 2026:

Thanks for taking a look. Did you mean LIMITED_WHILE(!p2p_node.fDisconnect && fuzzed_data_provider.ConsumeBool(), 100) , in your suggestion ?

in src/test/fuzz/p2p_private_broadcast.cpp:116 in 694d42ad8c outdated

 111 | +
 112 | +        connman.FlushSendBuffer(p2p_node);
 113 | +        (void)connman.ReceiveMsgFrom(p2p_node, std::move(net_msg));
 114 | +
 115 | +        bool more_work{true};
 116 | +        while (more_work) {

andrewtoth commented at 12:52 PM on April 16, 2026:

How long does this loop expect to go on?

        LIMITED_WHILE(more_work && fuzzed_data_provider.ConsumeBool(), 100) {

frankomosh commented at 2:05 PM on April 16, 2026:

I think the plain while (more_work) loop is the established pattern in other similar harnesses (process_message and p2p_handshake).

in src/test/fuzz/p2p_private_broadcast.cpp:126 in 694d42ad8c

 121 | +            }
 122 | +            node.peerman->SendMessages(p2p_node);
 123 | +        }
 124 | +    }
 125 | +    node.connman->StopNodes();
 126 | +}

andrewtoth commented at 12:53 PM on April 16, 2026:

nit: newline at end of file

frankomosh commented at 2:02 PM on April 16, 2026:

will change. thanks

in src/test/fuzz/CMakeLists.txt:77 in 694d42ad8c outdated

  76 | @@ -77,6 +77,7 @@ add_executable(fuzz
  77 |    overflow.cpp

vasild commented at 1:15 PM on April 16, 2026:

In the commit message: would be nice to follow the 50-72 rule.

s/Cconnman::PushMessage/CConnman::PushMessage/

frankomosh commented at 2:51 PM on April 16, 2026:

great. will update that

in src/test/fuzz/p2p_private_broadcast.cpp:97 in 694d42ad8c

  92 | +        /*network_key=*/fuzzed_data_provider.ConsumeIntegral<uint64_t>());
  93 | +
  94 | +    connman.AddTestNode(*pb_node);
  95 | +    node.peerman->InitializeNode(
  96 | +        *pb_node,
  97 | +        ServiceFlags{NODE_NONE}); // Private broadcast peers advertise NODE_NONE (see PushNodeVersion).

vasild commented at 1:32 PM on April 16, 2026:

        ServiceFlags{NODE_NONE}); // We advertise our services as NODE_NONE to private broadcast peers (see PushNodeVersion()).

otherwise it looks like that they advertise to us.

vasild commented at 1:33 PM on April 16, 2026:

There is no need for the cast, NODE_NONE is already of type ServiceFlags:

        NODE_NONE); // Private broadcast peers advertise NODE_NONE (see PushNodeVersion).

in src/test/fuzz/p2p_private_broadcast.cpp:99 in 694d42ad8c outdated

  94 | +    connman.AddTestNode(*pb_node);
  95 | +    node.peerman->InitializeNode(
  96 | +        *pb_node,
  97 | +        ServiceFlags{NODE_NONE}); // Private broadcast peers advertise NODE_NONE (see PushNodeVersion).
  98 | +
  99 | +    CNode& p2p_node = *pb_node.release();

vasild commented at 1:35 PM on April 16, 2026:

This will be a memory leak at the end of the test?

frankomosh commented at 2:42 PM on April 16, 2026:

I dont think so. release() transfers ownership to connman.m_nodes via AddTestNode() (src/test/util/net.h:65), and connman.StopNodes() deletes every node in that list at the end of the iteration (src/net.cpp:3661).

vasild commented at 10:01 AM on April 17, 2026:

Ok, I was looking for where it is freed after the release(). But it is added to connman before. So it is good. I would find it more natural to do the release first:

CNode& p2p_node = *pb_node.release();
connman.AddTestNode(p2p_node);
node.peerman->InitializeNode(p2p_node, ...);

and then the question why use unique_ptr in the first place if we are going to release() is right after. The production uses bare new, maybe the test can do the same:

CNode* pb_node = new CNode(...);
connman.AddTestNode(*pb_node);
node.peerman->InitializeNode(*pb_node, ...);

in src/test/fuzz/p2p_private_broadcast.cpp:37 in 694d42ad8c

  32 | +    g_setup = testing_setup.get();
  33 | +}
  34 | +
  35 | +// Outbound message types permitted on PRIVATE_BROADCAST connections.
  36 | +// CConnman::PushMessage silently drops all others.
  37 | +constexpr std::array PRIVATE_BROADCAST_MSG_TYPES{

vasild commented at 1:39 PM on April 16, 2026:

Would be good to indicate that these are "out" or "outbound" message types. Won't be long before we extend this to do something with "inbound" message types, which are different (e.g. GETDATA is allowed).

Given that this is inside a file called p2p_private_broadcast.cpp, may omit the PRIVATE_BROADCAST from the name of the variable if you wish.

frankomosh commented at 2:41 PM on April 16, 2026:

will update. thanks

vasild commented at 10:06 AM on April 17, 2026:

Wait! These are the messages that the tests simulates we have received from the peer. Those are inbound messages. INV, TX, and PING are ignored. The interesting ones are: VERSION, VERACK, GETDATA, PONG.

frankomosh commented at 7:47 AM on April 19, 2026:

Yes, looking more keenly, I see that {VERSION, VERACK, GETDATA, PONG} is indeed the right model for what's tested here. And there’s actually some additional coverage(GETDATA handler (line 4144, 0 → 23 hits) and the post-handshake message filter (line 4016, 0 → 27 hits)). The earlier array was mistakenly based on the CConnman::PushMessage outbound allowlist. Thanks for catching this.

in src/test/fuzz/p2p_private_broadcast.cpp:46 in 694d42ad8c outdated

  41 | +    NetMsgType::TX,
  42 | +    NetMsgType::PING,
  43 | +};
  44 | +} // namespace
  45 | +
  46 | +FUZZ_TARGET(p2p_private_broadcast, .init = ::initialize)

vasild commented at 1:54 PM on April 16, 2026:

This test is very very similar to p2p_handshake from src/test/fuzz/p2p_handshake.cpp. Can it be deduplicated somehow?

frankomosh commented at 2:48 PM on April 16, 2026:

Yes, the overlap is in the setup scaffolding (peerman/addrman reset, connman casts, g_msgproc_mutex lock, StopNodes()), and the same scaffolding exists across the other p2p harnesses. Agreed it's worth deduplicating, I’ll try it separately and if it comes out good it could be a follow-up?, or open a different PR altogether for it

vasild commented at 10:09 AM on April 17, 2026:

Also the bool more_work{true}; while (more_work) { is duplicated. Already in more than one fuzz test. I agree here we can add one more dup and then separately try to dedup.

DrahtBot removed the label CI failed on Apr 16, 2026

DrahtBot added the label CI failed on Apr 16, 2026

DrahtBot commented at 7:22 PM on April 16, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/24504783325/job/71708064887 LLM reason (✨ experimental): CI failed because the lint check “trailing_newline” reported a missing trailing newline in src/test/fuzz/p2p_private_broadcast.cpp.

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

frankomosh force-pushed on Apr 17, 2026

frankomosh force-pushed on Apr 19, 2026

DrahtBot removed the label CI failed on Apr 19, 2026

in src/test/fuzz/p2p_private_broadcast.cpp:98 in ef8aad4833

  93 | +    connman.AddTestNode(*pb_node);
  94 | +    node.peerman->InitializeNode(
  95 | +        *pb_node,
  96 | +        NODE_NONE); // We advertise our services as NODE_NONE to private broadcast peers (see PushNodeVersion()).
  97 | +
  98 | +    CNode& p2p_node = *pb_node;

vasild commented at 9:52 AM on April 20, 2026:

nit: this alias seems unnecessary. Maybe use pb_node-> below instead of p2p_node. and *pb_node instead of p2p_node. Or move this 2 statements earlier and use p2p_node in the calls to AddTestNode() and InitializeNode() which now use *pb_node.

vasild approved

vasild commented at 9:53 AM on April 20, 2026: contributor

ACK ef8aad4833630cf90aaf9d2d2897375a2e6e70fb

in src/test/fuzz/p2p_private_broadcast.cpp:103 in ef8aad4833

  98 | +    CNode& p2p_node = *pb_node;
  99 | +
 100 | +    LIMITED_WHILE(!p2p_node.fDisconnect && fuzzed_data_provider.ConsumeBool(), 100)
 101 | +    {
 102 | +        clock_ctx += std::chrono::seconds{
 103 | +            fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, 600)};

maflcko commented at 10:02 AM on April 20, 2026:

nit: You can use ConsumeDuration with 10min, but just a style-nit.

in src/test/fuzz/p2p_private_broadcast.cpp:37 in ef8aad4833 outdated

  32 | +    g_setup = testing_setup.get();
  33 | +}
  34 | +
  35 | +// Inbound message types a private broadcast peer sends to our node.
  36 | +// Post-handshake, our node ignores all others (see ProcessMessage()).
  37 | +constexpr std::array INBOUND_MSG_TYPES{

maflcko commented at 10:05 AM on April 20, 2026:

Is there a reason to limit those? Shouldn't it be the goal of the fuzz test to check that the node does not crash and ignores them when unneeded messages are sent?

Looking at the code below the goal seems to be to just check against crashes/coverage, and not specifically about logic checks?

vasild commented at 10:58 AM on April 20, 2026:

I might be mistaken, my understanding is that it would be very unlikely for the fuzzer to guess interesting (valid) message types. It would be good to feed also some random garbage into it and normally I would do some random and some of those predefined (interesting) ones, however it seems that test/fuzz/process_messages.cpp is already doing the random garbage stuff?

maflcko commented at 11:06 AM on April 20, 2026:

I might be mistaken, my understanding is that it would be very unlikely for the fuzzer to guess interesting (valid) message types.

I am confident this is not true, at least with modern fuzz engines, which have the full message array (list of strings) in memory and will inject it into the fuzz byte stream. Even without this array, it shouldn't take more than a few minutes for a modern fuzz engine to figure it out. Moreover, with an initial set, using the pre-existing qa-assets files, it is likely that there is already close to full coverage in this test without even starting a fuzz engine.

however it seems that test/fuzz/process_messages.cpp is already doing the random garbage stuff?

I don't think this is true. If the private broadcast stuff was already covered by another fuzz target, there would be no need for this pull request and it could be closed.

frankomosh commented at 1:06 PM on April 20, 2026:

Looking at the code below the goal seems to be to just check against crashes/coverage, and not specifically about logic checks?

Shouldn't we be relying on the code’s own invariant checks?,

maflcko commented at 1:10 PM on April 20, 2026:

To give some more context, the code changes should follow what the goal here is of this pull request.

If the goal is to just add code coverage, to catch crashes, it may be better to modify the other fuzz targets to just have an optional call to InitiateTxBroadcastPrivate, because the other fuzz targets also create other message types already and also create more than one peer, etc ... (Avoiding duplicate code was brought up before, IIRC)

If the goal is to add more fine grained checks, it may be better to have a separate fuzz target, which exercises the inner logic with more strict checks.

maflcko commented at 1:12 PM on April 20, 2026:

Shouldn't we be relying on the code’s own invariant checks?,

Yes, this is a possible answer and actually true for most fuzz targets. However, it doesn't have to be, because fuzz tests are free to check any advanced state or outcome they want to.

vasild commented at 2:03 PM on April 20, 2026:

So the only difference between this and test/fuzz/process_messages.cpp is that this has an extra call to InitiateTxBroadcastPrivate()?

frankomosh commented at 2:11 PM on April 20, 2026:

So the only difference between this and test/fuzz/process_messages.cpp is that this has an extra call to InitiateTxBroadcastPrivate()?

There are a few reasons why I did not want to bundle it in test/fuzz/process_messages.cpp .

Here, every iteration uses ConnectionType::PRIVATE_BROADCAST (not random), m_tx_for_private_broadcast is seeded before the handshake so PushPrivateBroadcastTx reaches the send-INV path rather than immediately disconnecting,

well, and of course the message type constraining to the four, with private broadcast-specific handling. I think that without the seeding in particular, process_messages only ever hits the disconnect branch of PushPrivateBroadcastTx

frankomosh commented at 2:15 PM on April 20, 2026:

and of course I thought forcing it inside there might reduce the effectiveness of those particular harnesses (process_messages or p2p_handshake)

frankomosh commented at 2:23 PM on April 20, 2026:

I know that maflcko is right that the randomness issue alone could, in theory, be solved by running the fuzzer long enough and that it will eventually pick PRIVATE_BROADCAST. But I'm not sure if the seeding of m_tx_for_private_broadcast can be solved by corpus growth alone, without a structural change to the harness setup

brunoerg commented at 6:16 PM on April 20, 2026:

To give some more context, the code changes should follow what the goal here is of this pull request.

If the goal is to just add code coverage, to catch crashes, it may be better to modify the other fuzz targets to just have an optional call to InitiateTxBroadcastPrivate, because the other fuzz targets also create other message types already and also create more than one peer, etc ... (Avoiding duplicate code was brought up before, IIRC)

If the goal is to add more fine grained checks, it may be better to have a separate fuzz target, which exercises the inner logic with more strict checks.

+1. I think you can add some verifications such as INVs in vSendMsg are well-formed and contains a single msg tx, then you could check that its hash is the same of the created tx.

fuzz: add p2p_private_broadcast harness

No existing harness deliberately covers ConnectionType::PRIVATE_BROADCAST. process_message never seeds m_tx_for_private_broadcast, so the actual transaction-send path in PushPrivateBroadcastTx had
0 hits, and related branches in FinalizeNode and SendMessages were similarly unreached.

This harness seeds m_tx_for_private_broadcast via InitiateTxBroadcastPrivate before the peer connects, explicitly uses ConnectionType::PRIVATE_BROADCAST, and constrains the outbound message pool to the five types CConnman::PushMessage permits on these connections.

6abb711964

frankomosh force-pushed on Apr 20, 2026