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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild
Concept ACK	nervana21, instagibbs

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

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?

frankomosh commented at 2:43 PM on April 22, 2026:

As for src/test/fuzz/p2p_handshake.cpp, have confirmed that SendMessages coverage is identical before and after applying the fabf8d1 pattern fix, so thats fine. Happy to do a followup though to remove the the dangling reference_wrapper<AddrMan> risk, which I believe is still necessary.

maflcko commented at 3:06 PM on April 22, 2026:

Nice. Feel free to create a new pull request right away. I'd be happy to review/ack it.

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

vasild commented at 4:43 PM on April 23, 2026:

Still the lines in the commit message are very long.

frankomosh commented at 8:46 AM on May 21, 2026:

done. thanks

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.

andrewtoth commented at 4:46 PM on April 27, 2026:

I don't think "random" is the right way to think about consuming fuzzer input. Fuzzers are coverage guided. They will discover paths to take and then mutate the input that takes that path to fully exercise every path they can. If we give the harness a path to exercise private broadcast connections, it will find the path and take it.

But I'm not sure if the seeding of m_tx_for_private_broadcast can be solved by corpus growth alone

Wouldn't adding the following to process_messages exercise everything in this harness and more? This would be preferable, no?

    // Seed with up to 3 transactions to test multiple pending broadcasts.
    const int num_txs{fuzzed_data_provider.ConsumeIntegralInRange(0, 3)};
    for (int i = 0; i < num_txs; ++i) {
        node.peerman->InitiateTxBroadcastPrivate(
            MakeTransactionRef(ConsumeTransaction(fuzzed_data_provider, /*prevout_txids=*/std::nullopt)));
    }

maflcko commented at 8:41 AM on April 28, 2026:

Wouldn't adding the following to process_messages exercise everything in this harness and more? This would be preferable, no?

I think it is missing the mocktime setting? Otherwise, this is what I had in mind. When it comes to "preferable", I'd say either is fine. A dedicated fuzz target has a smaller overall input space to reach full coverage, so it may be easier to handle. Though, a more general fuzz target may trigger a bug that hasn't been envisioned while writing the more focussed one.

So I think anything is fine here, or we may even go as far and do both? :sweat_smile:

frankomosh commented at 9:35 AM on April 28, 2026:

or we may even go as far and do both? 😅

That might also be a good approach

frankomosh force-pushed on Apr 20, 2026

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

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

Filled in #35129.

The current PR goes via peerman / connman while #35129 calls directly the PrivateBroadcast methods. I think both complement each other.

maflcko commented at 1:38 PM on April 21, 2026: member

Right, I think both are valuable. Though, my preference would still be to avoid the https://en.wikipedia.org/wiki/Streetlight_effect of only looking in the happy path by supplying only the valid message types, a single peer of a single type, and a single transaction.

The beauty of a fuzz target is that it gives the fuzz engine the freedom to explore the space on its own, so I think it would be valuable to let the fuzz engine explore the space where "the light is off" (aka an invalid msg type, a second peer (maybe of a different type), or a second/third/duplicate transaction, etc).

If you are worried about the fuzz engine not making progress, I'd doubt that without any benchmarks, but there is always the option to just slap an if (ConsumeBool()) to branch between a path that picks a well-formed valid outcome/msg and one that picks a possibly invalid/nonsensical outcome/msg.

fanquake added the label Private Broadcast on Apr 21, 2026

frankomosh force-pushed on Apr 22, 2026

frankomosh commented at 2:12 PM on April 22, 2026: contributor

Updated the harness based on review feedback. Now seeds 1–3 transactions instead of one, to exercise duplicate and multiple broadcast handling. Also, now optionally adds peers of random connection types alongside the PRIVATE_BROADCAST peer. The fuzzer would explore both guided message types and arbitrary ones. Lastly, Asserts outbound INV is well formed after handshake (brunoerg's suggestion)

DrahtBot added the label CI failed on Apr 22, 2026

DrahtBot removed the label CI failed on Apr 23, 2026

in src/test/fuzz/p2p_private_broadcast.cpp:119 in 6d0d18fca6 outdated

 114 | +
 115 | +    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
 116 | +    {
 117 | +        // Pick any random peer to test interleaved message handling.
 118 | +        CNode& p2p_node = *PickValue(fuzzed_data_provider, peers);
 119 | +        if (p2p_node.fDisconnect) continue;

vasild commented at 4:44 PM on April 23, 2026:

Does it make sense to break; from the loop if all peers have fDisconnect == true?

frankomosh commented at 3:30 PM on April 27, 2026:

I think break would terminate prematurely in this case (am using multiple peers unlike before where I had just one peer). Also, I believe the loop terminates naturally because of ConsumeBool() returning false, and capped at 100 iterations.

vasild approved

vasild commented at 4:44 PM on April 23, 2026: contributor

ACK 6d0d18fca6cbb2fb846d987d9793f7adfc0d1712

sedited requested review from brunoerg on May 10, 2026

in src/test/fuzz/p2p_private_broadcast.cpp:97 in 6d0d18fca6 outdated

  92 | +        /*nLocalHostNonceIn=*/fuzzed_data_provider.ConsumeIntegral<uint64_t>(),
  93 | +        /*addrBindIn=*/ConsumeAddress(fuzzed_data_provider),
  94 | +        /*addrNameIn=*/fuzzed_data_provider.ConsumeRandomLengthString(64),
  95 | +        /*conn_type_in=*/ConnectionType::PRIVATE_BROADCAST,
  96 | +        /*inbound_onion=*/false,
  97 | +        /*network_key=*/fuzzed_data_provider.ConsumeIntegral<uint64_t>());

brunoerg commented at 12:56 PM on May 19, 2026:

6d0d18fca6cbb2fb846d987d9793f7adfc0d1712: I don't think we need to consume from the input for some fields here (e.g. m_network_key), they could be a constant since their values will not be used in practice.

frankomosh commented at 7:58 AM on May 20, 2026:

I think these fields still influence node identity and could impact connection establishment in edge cases. For example, nLocalHostNonceIn is checked against the peer's VERSION nonce for self-connection detection, which determines if the peer is immediately dropped.

Also, I agree with maflcko's earlier review feedback about avoiding the "streetlight effect" (https://github.com/bitcoin/bitcoin/pull/35090#issuecomment-4288969545). My guess is that fixing these to constants could perhaps narrow the state space and blind the fuzzer to edge cases, without providing a clear corresponding benefit to exercising the private broadcast paths.

brunoerg commented at 9:38 PM on May 21, 2026:

I think these fields still influence node identity and could impact connection establishment in edge cases. For example, nLocalHostNonceIn is checked against the peer's VERSION nonce for self-connection detection, which determines if the peer is immediately dropped.

Specifically in the context of private broadcast? Which is what you really want to exercise here.

frankomosh commented at 8:24 AM on May 22, 2026:

I have grepped deeper and found that the VERSION handler and the self-connection check is gated on IsInboundConn(), so guess this means it never fires for outbound PRIVATE_BROADCAST peers regardless of nLocalHostNonceIn. The other fields (nKeyedNetGroup, addrBind, addrName, network_key) also don't appear in this path. Changed in new commit. Thanks.

in src/test/fuzz/p2p_private_broadcast.cpp:77 in 6d0d18fca6 outdated

  72 | +    connman.SetAddrman(*node.addrman);
  73 | +
  74 | +    // Seed with 1-3 transactions to test multiple pending broadcasts.
  75 | +    const int num_txs{fuzzed_data_provider.ConsumeIntegralInRange(1, 3)};
  76 | +    for (int i = 0; i < num_txs; ++i) {
  77 | +        node.peerman->InitiateTxBroadcastPrivate(

Crypt-iQ commented at 2:17 PM on May 20, 2026:

Any value to call this in the loop?

frankomosh commented at 8:27 AM on May 21, 2026:

can you share a coverage report? I think the harness could be improved to also create valid transactions.

Is this(https://github.com/bitcoin/bitcoin/pull/35090#issuecomment-4258396013) helpful ?

Any value to call this in the loop?

The loop produces distinct transactions per call (fresh bytes from fuzzed_data_provider each iteration)?..Initially I had just a single tx, but after review insights here I decided to seed multiple transactions(I am no sure if there is any way to add the multiple txs without the loop...)

Crypt-iQ commented at 10:06 AM on May 21, 2026:

I meant if there's value in calling it in the LIMITED_WHILE loop below. Looking at the coverage, there is missing coverage in ReattemptPrivateBroadcast, AbortPrivateBroadcast, private broadcast branches in NetMsgType::TX processing.

frankomosh commented at 2:25 PM on May 21, 2026:

I meant if there's value in calling it in the LIMITED_WHILE loop below.

Ok. trying that now. Edit: Yes, I see value in doing that. Updated in new commit. Thanks.

Looking at the coverage, there is missing coverage in ReattemptPrivateBroadcast, AbortPrivateBroadcast, private broadcast branches in NetMsgType::TX processing.

Seems to be a structural gap. Have not checked to see the kind of refactors that would cover these, though.

Edit: I have worked on a refactor that helps reach NetMsgType::TX . However for ReattemptPrivateBroadcast and AbortPrivateBroadcast, they might be out of scope here?

in src/test/fuzz/p2p_private_broadcast.cpp:101 in 6d0d18fca6 outdated

  96 | +        /*inbound_onion=*/false,
  97 | +        /*network_key=*/fuzzed_data_provider.ConsumeIntegral<uint64_t>());
  98 | +
  99 | +    peers.push_back(pb_node);
 100 | +    connman.AddTestNode(*pb_node);
 101 | +    node.peerman->InitializeNode(

Crypt-iQ commented at 2:17 PM on May 20, 2026:

formatting seems off compared to other tests, here and elsewhere

frankomosh commented at 8:33 AM on May 21, 2026:

Thanks. will fix this

in src/test/fuzz/p2p_private_broadcast.cpp:121 in 6d0d18fca6 outdated

 116 | +    {
 117 | +        // Pick any random peer to test interleaved message handling.
 118 | +        CNode& p2p_node = *PickValue(fuzzed_data_provider, peers);
 119 | +        if (p2p_node.fDisconnect) continue;
 120 | +
 121 | +        clock_ctx += ConsumeDuration<std::chrono::seconds>(fuzzed_data_provider, 0s, 600s);

Crypt-iQ commented at 2:18 PM on May 20, 2026:

I think this would be more readable if this and choosing m_type was in a CallOneOf

frankomosh commented at 8:33 AM on May 21, 2026:

Tried CallOneOf, measured coverage and realised complete regression. I'm not sure if it works well in this kind of harness.

Crypt-iQ commented at 10:08 AM on May 21, 2026:

Using CallOneOf shouldn't make a difference for coverage afaict? Plus I personally find it a bit easier to review and evaluate since the cases are separated nicely.

frankomosh commented at 1:36 PM on May 21, 2026:

Using CallOneOf shouldn't make a difference for coverage afaict? Plus I personally find it a bit easier to review and evaluate since the cases are separated nicely.

Indeed, you are right. I fuzzed a few more hours on nosan and found that my earlier 'regression' was just a corpus size issue.

Crypt-iQ commented at 2:21 PM on May 20, 2026: contributor

can you share a coverage report? I think the harness could be improved to also create valid transactions.

frankomosh force-pushed on May 21, 2026

frankomosh force-pushed on May 22, 2026

frankomosh commented at 8:40 AM on May 22, 2026: contributor

Updated based on review feedback. Simplified unused CNode fields to constants. Applied CallOneOf() to improve readability. And also add coverage to NetMsgType::TX

frankomosh force-pushed on May 22, 2026

DrahtBot added the label CI failed on May 22, 2026

DrahtBot commented at 8:54 AM on May 22, 2026: contributor

🚧 At least one of the CI tasks failed. Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/26276848345/job/77343990183 LLM reason (✨ experimental): CI failed due to a build error in the fuzz target: -Werror=array-bounds from an out-of-bounds __builtin_memcpy triggered by 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>

maflcko commented at 9:09 AM on May 22, 2026: member

If the GCC warning is a false positive, you can suppress it by expanding this:

ci/test/00_setup_env_arm.sh:  -DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=maybe-uninitialized' \

in src/test/fuzz/p2p_private_broadcast.cpp:151 in 3ed2adee0e

 146 | +                    const auto& tx{PickValue(fuzzed_data_provider, seeded_txs)};
 147 | +                    net_msg.emplace();
 148 | +                    net_msg->m_type = NetMsgType::GETDATA;
 149 | +                    CInv inv{MSG_TX, tx->GetHash().ToUint256()};
 150 | +                    DataStream ds{};
 151 | +                    ds << std::vector<CInv>{inv};

vasild commented at 12:05 PM on May 22, 2026:

https://github.com/bitcoin/bitcoin/actions/runs/26278223054/job/77348533404?pr=35090#step:10:4782

...
    inlined from ‘p2p_private_broadcast_fuzz_target(FuzzBufferType)::<lambda()>’ at /home/runner/work/_temp/src/test/fuzz/p2p_private_broadcast.cpp:151:48:
/usr/arm-linux-gnueabihf/include/c++/14/bits/stl_construct.h:119:7: error: ‘void* __builtin_memcpy(void*, const void*, unsigned int)’ offset [5, 36] is out of the bounds [0, 5] [-Werror=array-bounds=]
  119 |       ::new((void*)__p) _Tp(std::forward<_Args>(__args)...);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Hmm!?

frankomosh commented at 12:59 PM on May 22, 2026:

Thinking of trying out NetMsg::Make(https://github.com/bitcoin/bitcoin/blob/master/src/netmessagemaker.h) to replace DataStream, as a workaround

frankomosh commented at 6:53 AM on May 24, 2026:

Used NetMsg::Make which I think works fine for this specific use case (Constructing outbound GETDATA). It serializes through VectorWriter directly into std::vector<unsigned char>, bypassing the std::byte which was troublesome before

vasild approved

vasild commented at 12:06 PM on May 22, 2026: contributor

ACK 3ed2adee0ee2865c4c9123bc65f9167e6b413b0e modulo squashing the 3 commits into one and the compilation warning.

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

vasild commented at 12:10 PM on May 22, 2026: contributor

If the GCC warning is a false positive, you can suppress it by expanding this:
ci/test/00_setup_env_arm.sh:  -DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=maybe-uninitialized' \

~~Did you mean array-bounds instead of maybe-uninitialized?~~ edit: maybe-uninitialized is in the existent code, and you meant to extend it, sorry for the noise

Btw that would unerror all warnings of this class, including future real ones. If that is undesirable, then one can use #pragma inside the source code to surround the offending snippet.

maflcko commented at 12:14 PM on May 22, 2026: member

Btw that would unerror all warnings of this class, including future real ones.

I don't think I've ever seen a real one. Those GCC array-bound and uninitialized warnings are so noisy, that I wonder if they are a net negative overall (ref the sqlite "static analyser" meme: https://sqlite.org/testing.html#static_analysis).

fuzz: add p2p_private_broadcast harness

Add a fuzz harness targeting ConnectionType::PRIVATE_BROADCAST.
Seeds m_tx_for_private_broadcast via InitiateTxBroadcastPrivate
so PushPrivateBroadcastTx reaches the send-INV and other paths.
Guarantees one PRIVATE_BROADCAST peer per iteration, optionally
adds peers of other types, uses CallOneOf() branching between
guided and arbitrary message types, and verifies the outbound
INV is well-formed after the handshake completes.

81b4868e3f

frankomosh force-pushed on May 22, 2026

DrahtBot removed the label CI failed on May 22, 2026

vasild approved

vasild commented at 6:47 AM on May 25, 2026: contributor

ACK 81b4868e3f172239c475b12dd9703f684ff52c36

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

fanquake requested review from marcofleon on May 26, 2026

in src/test/fuzz/p2p_private_broadcast.cpp:42 in 81b4868e3f

  37 | +// Used as the guided path in the CallOneOf() below.
  38 | +constexpr std::array INBOUND_MSG_TYPES{
  39 | +    NetMsgType::VERSION,
  40 | +    NetMsgType::VERACK,
  41 | +    NetMsgType::GETDATA,
  42 | +    NetMsgType::PONG,

nervana21 commented at 7:27 PM on May 27, 2026:

nit: The PR summary states that there are 5 types, but there are only 4 types

in src/test/fuzz/p2p_private_broadcast.cpp:115 in 81b4868e3f

 110 | +    for (int i = 0; i < extra_peers; ++i) {
 111 | +        peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, node_id++).release());
 112 | +        connman.AddTestNode(*peers.back());
 113 | +        node.peerman->InitializeNode(
 114 | +            *peers.back(),
 115 | +            static_cast<ServiceFlags>(fuzzed_data_provider.ConsumeIntegral<uint64_t>()));

nervana21 commented at 7:28 PM on May 27, 2026:

Would it make sense to bias towards known service flags like the below? Or are we intentionally keeping this search space wide open?

            ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS));

frankomosh commented at 6:18 AM on May 28, 2026:

Would it make sense to bias towards known service flags like the below?

I doubt if it helps much in this harness. Also, p2p_handshake.cpp (from which most initial formatting for this harness was copied) uses the same pattern https://github.com/bitcoin/bitcoin/blob/eeeeb2a0b902ed69b5cd5523833d3ab5d963c81f/src/test/fuzz/p2p_handshake.cpp#L72

nervana21 commented at 2:24 PM on May 28, 2026:

Okay I ran some tests and verified that you're correct! This example clears up some randomness misunderstandings that I had. Thanks :)

nervana21 commented at 7:29 PM on May 27, 2026: contributor

Concept ACK

instagibbs commented at 4:02 PM on May 28, 2026: member

concept ACK, just noticed we don't have one in master

brunoerg commented at 6:11 PM on May 28, 2026: contributor

can you share a coverage report? I think the harness could be improved to also create valid transactions.

I've generated one but from just few hours of execution: https://brunoerg.xyz/bitcoin-core-coverage/35090/

in src/test/fuzz/p2p_private_broadcast.cpp:52 in 81b4868e3f

  47 | +{
  48 | +    SeedRandomStateForTest(SeedRand::ZEROS);
  49 | +    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
  50 | +
  51 | +    auto& node{g_setup->m_node};
  52 | +    auto& connman{static_cast<ConnmanTestMsg&>(*node.connman)};

marcofleon commented at 10:14 PM on May 28, 2026:

I think m_num_to_open will keep incrementing if we don't reset connman. Similar to the process_message(s) fuzz tests, we should add connman.Reset() here so that state doesn't persist between iterations.

marcofleon commented at 11:43 PM on May 28, 2026: contributor

Are there any more invariants we can assert on in a p2p test like this? I don't know the private broadcast code well enough to have something pop out to me right away, but I can think on it some more.

Looking at the harness coverage, the inv assertions are barely being hit. It depends on how much we want to steer this harness down the happy path, but as is, the private broadcast handshake doesn't complete most of the time. Also, it looks like vSendMsg is often empty by the time we get to the assertions. I think that's happening in SocketSendData here.

Claude (opus 4.8 🚀) suggested a fix similar to what net_tests.cpp does here. So we would set connman.SetCaptureMessages(true) and then override the global CaptureMessage hook to record outbound messages. CConnman::PushMessage calls that hook before SocketSendData, so we'd be able to check invariants on every successful handshake. That should work afaict.

in src/test/fuzz/p2p_private_broadcast.cpp:106 in 81b4868e3f

 101 | +    peers.push_back(pb_node);
 102 | +    connman.AddTestNode(*pb_node);
 103 | +    // We advertise our services as NODE_NONE to private broadcast peers (see PushNodeVersion()).
 104 | +    node.peerman->InitializeNode(
 105 | +        *pb_node,
 106 | +        NODE_NONE);

nervana21 commented at 12:14 PM on May 29, 2026:

I tested the coverage of the harness without and with the handshake patch suggested below and got the following diff:

net_processing.cpp:3570 (PushPrivateBroadcastTx)

-  3570|      0|    MakeAndPushMessage(node, NetMsgType::INV, ...
+  3570|    377|    MakeAndPushMessage(node, NetMsgType::INV, ...

Without this handshake (modeled after p2p_headers_presync), the harness never reaches the INV send path. InitializeNode does not set nVersion, so inbound VERACK is ignored before PushPrivateBroadcastTx runs.

    // Complete handshake so PushPrivateBroadcastTx runs.
    connman.Handshake(
        /*node=*/*pb_node,
        /*successfully_connected=*/true,
        /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
        /*local_services=*/NODE_NONE,
        /*version=*/PROTOCOL_VERSION,
        /*relay_txs=*/true);