net: always complete all initial private broadcast connections #36277

andrewtoth commented at 4:47 PM on September 16, 2026: contributor

Make sure we send out all three connections when initiating a private broadcast.

Use MarkResolved instead of Remove to mark a tx as having been received back or mempool-conflicted. Prevent additional stale retry connections by tracking disconnects and explicitly granting new connections with TryGrantRetry.

Intended as an alternative to #34707, addressing the theoretical timing attack described in #34707 (comment). Also fixes theoretical timing attacks described in #29415 (review). This is based on the idea described in #29415 (review):

always send 3 times regardless if the tx is received in our mempool or is otherwise invalid

DrahtBot added the label P2P on Sep 16, 2026

DrahtBot commented at 4:47 PM on September 16, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36277
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36277.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36277#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	vasild, mzumsande, sedited
Concept ACK	optout21, danielabrozzoni

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:

#36429 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36429.svg"> (Delayed transaction broadcast, minimal by optout21)
#35502 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35502.svg"> (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
#34707 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34707.svg"> (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 5:07 PM on September 16, 2026: contributor

Concept ACK, will review

in src/net_processing.cpp:1772 in e4204baff7

1767 | @@ -1767,7 +1768,8 @@ void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
1768 |                  LogDebug(BCLog::PRIVBROADCAST, "Giving up broadcast attempts for txid=%s wtxid=%s: %s",
1769 |                           stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString(),
1770 |                           mempool_acceptable.m_state.ToString());
1771 | -                m_tx_for_private_broadcast.Remove(stale_tx);
1772 | +                // Mark received so it will get cleaned up.
1773 | +                m_tx_for_private_broadcast.MarkReceived(stale_tx);

vasild commented at 3:26 PM on September 18, 2026:

Why replace Remove() with MarkReceived() here? The transaction has not been actually received.

andrewtoth commented at 2:40 PM on September 19, 2026:

If ReattemptPrivateBroadcast gets scheduled before we complete all connections but the transaction now conflicts with our mempool, it will be removed and won't achieve the goal of this patch.

The transaction has not been actually received.

Right but this is an easy way to make sure no more connections are scheduled for this transaction.

andrewtoth commented at 8:04 PM on September 20, 2026:

I renamed this method to MarkResolved, to handle both cases.

in src/net_processing.cpp:1856 in e4204baff7

1856 | -        m_connman.m_private_broadcast.NumToOpenAdd(1);
1857 | +    if (node.IsPrivateBroadcastConn()) {
1858 | +        m_tx_for_private_broadcast.NodeDisconnected(nodeid);
1859 | +        // Retry a connection if we didn't complete the handshake.
1860 | +        if (!node.fSuccessfullyConnected && m_tx_for_private_broadcast.HavePendingTransactions()) {
1861 | +            m_connman.m_private_broadcast.NumToOpenAdd(1);

vasild commented at 3:31 PM on September 18, 2026:

Maybe elaborate this comment to something like:

// We consider a transaction has been sent to a peer if we sent them an INV.
// Normally they should request the transaction with GETDATA, but this may
// not happen if they are already aware of the transaction.
// If we didn't send them an INV, then schedule a new connection to compensate
// for this send failure. Since whether we sent them INV or not is not readily
// available here we use fSuccessfullyConnected instead because INV is sent right
// after a successful connection.

vasild commented at 4:19 AM on September 20, 2026:

After some more thinking on this - we do not need the NumToOpenAdd(1) call here at all. It is to compensate for a failed send. IIRC the origin of this was when the "retry stale" logic was running after 10 minutes. But it was changed to run every 2-3 minutes. I think that in the unlikely event that the send fails after opening the connection, the other 2 connections will suffice for the broadcast (just 1 suffices). In the even more unlikely event that they don't either, then the "retry stale" logic will pick it up in 2-3 minutes. Right?

andrewtoth commented at 4:33 PM on September 20, 2026:

This was discussed in #29415 (review). If we connect but fail the handshake (which is quite common) we do not reach PickTxForSend, so we don't increase send_statuses.size(). If we also implement your suggestion #36277 (review), then we would never open a new connection and could leave a pending transaction stranded.

I think this would also just make the broadcasting less reliable, since many of the first 3 initial txs would just fail handshakes.

andrewtoth commented at 8:04 PM on September 20, 2026:

Took the comment.

in src/private_broadcast.cpp:22 in e4204baff7

  13 | @@ -14,12 +14,21 @@ PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
  14 |      EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
  15 |  {
  16 |      LOCK(m_mutex);
  17 | +    // Cleanup finished transactions
  18 | +    std::erase_if(m_transactions, [this](const auto& entry) {
  19 | +        const auto& state{entry.second};
  20 | +        return state.received && !IsPending(state) &&
  21 | +               std::ranges::all_of(state.send_statuses, [](const auto& status) { return status.disconnected; });
  22 | +    });

vasild commented at 3:59 PM on September 18, 2026:

What about removing transactions from the list only when the size of the list reaches m_max_transactions? Then maybe send_status.disconnected and PrivateBroadcast::NodeDisconnected() will not be needed?

andrewtoth commented at 8:05 PM on September 20, 2026:

Done. Removed NodeDisconnected. This simplifies it :thumbsup:.

in src/net_processing.cpp:4734 in e4204baff7

4728 | @@ -4726,15 +4729,10 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4729 |          const uint256& hash = peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256();
4730 |          AddKnownTx(peer, hash);
4731 |  
4732 | -        if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
4733 | +        if (m_tx_for_private_broadcast.MarkReceived(ptx)) {
4734 |              LogDebug(BCLog::PRIVBROADCAST, "Received our privately broadcast transaction (txid=%s) from the "
4735 | -                                           "network from %s; stopping private broadcast attempts",
4736 | +                                           "network from %s; stopping private broadcast retries",

vasild commented at 4:08 PM on September 18, 2026:

After receiving the transaction (with this PR) we may send a few times more, so drop the last part:

-                                           "network from %s; stopping private broadcast attempts",
+                                           "network from %s",

in src/private_broadcast.h:111 in e4204baff7

 102 | @@ -100,6 +103,30 @@ class PrivateBroadcast
 103 |      std::optional<size_t> Remove(const CTransactionRef& tx)
 104 |          EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 105 |  
 106 | +    /**
 107 | +     * Mark a transaction as being received back from the network.
 108 | +     * @param[in] tx Transaction received from the network.
 109 | +     * @return Whether the transaction was found in the storage.
 110 | +     */
 111 | +    bool MarkReceived(const CTransactionRef& tx)

vasild commented at 4:10 PM on September 18, 2026:

Add comments for the new functions before class PrivateBroadcast.

in src/net_processing.cpp:1851 in e4204baff7 outdated

1848 | @@ -1847,11 +1849,12 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
1849 |          LOCK(m_headers_presync_mutex);
1850 |          m_headers_presync_stats.erase(nodeid);
1851 |      }
1852 | -    if (node.IsPrivateBroadcastConn() &&
1853 | -        !m_tx_for_private_broadcast.DidNodeConfirmReception(nodeid) &&

vasild commented at 4:30 PM on September 18, 2026:

DidNodeConfirmReception() is now unused, can be removed.

andrewtoth commented at 8:05 PM on September 20, 2026:

Added a second commit that removes it.

in src/private_broadcast.h:263 in e4204baff7

 256 | @@ -228,6 +257,10 @@ class PrivateBroadcast
 257 |      struct TxSendStatus {
 258 |          NodeClock::time_point time_added{NodeClock::now()};
 259 |          std::vector<SendStatus> send_statuses;
 260 | +        /// Total number of sends granted, including initial count.
 261 | +        size_t send_limit{INITIAL_BROADCAST_COUNT};
 262 | +        /// Whether the transaction was received back from the network.
 263 | +        bool received{false};

vasild commented at 4:38 PM on September 18, 2026:

Naming nit: I would find it more understandable if received is called received_by_us. Because nearby send_statuses contains information whether the transaction has been received by the peers we send it to.

andrewtoth commented at 8:05 PM on September 20, 2026:

Renamed to resolved.

in src/private_broadcast.h:261 in e4204baff7

 256 | @@ -228,6 +257,10 @@ class PrivateBroadcast
 257 |      struct TxSendStatus {
 258 |          NodeClock::time_point time_added{NodeClock::now()};
 259 |          std::vector<SendStatus> send_statuses;
 260 | +        /// Total number of sends granted, including initial count.
 261 | +        size_t send_limit{INITIAL_BROADCAST_COUNT};

vasild commented at 4:42 PM on September 18, 2026:

Naming nit: it is not a "limit" because we will always aim to send it this many times and will not settle for less. Maybe planned_sends?

andrewtoth commented at 8:06 PM on September 20, 2026:

Renamed to planned_sends.

in src/net_processing.cpp:205 in e4204baff7

 201 | @@ -202,7 +202,7 @@ static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1};
 202 |   *  is exempt from this limit). */
 203 |  static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND};
 204 |  /** For private broadcast, send a transaction to this many peers. */
 205 | -static constexpr size_t NUM_PRIVATE_BROADCAST_PER_TX{3};
 206 | +static constexpr size_t NUM_PRIVATE_BROADCAST_PER_TX{PrivateBroadcast::INITIAL_BROADCAST_COUNT};

vasild commented at 4:44 PM on September 18, 2026:

I do not see a value in NUM_PRIVATE_BROADCAST_PER_TX now. Consider using PrivateBroadcast::INITIAL_BROADCAST_COUNT all over the place and dropping NUM_PRIVATE_BROADCAST_PER_TX.

Also the name NUM_PRIVATE_BROADCAST_PER_TX is/was a bit misleading - we could send more times than that. Should be NUM_INITIAL_PRIVATE_BROADCAST_PER_TX.

in src/net_processing.cpp:1762 in e4204baff7 outdated

1758 | @@ -1759,6 +1759,7 @@ void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
1759 |              LOCK(cs_main);
1760 |              auto mempool_acceptable = m_chainman.ProcessTransaction(stale_tx, /*test_accept=*/true);
1761 |              if (mempool_acceptable.m_result_type == MempoolAcceptResult::ResultType::VALID) {
1762 | +                if (!m_tx_for_private_broadcast.TryGrantRetry(stale_tx)) continue;

vasild commented at 5:13 PM on September 18, 2026:

This increments the send_limit counter for a transaction that came from GetStale() and is acceptable in the mempool. However such a transaction might still have unused "allowance". For example: broadcast 1 times, to be sent 2 more times (send_limit is 3), not received back. Such transaction will have 2 more sends and does not need send_limit to be incremented from 3 to 4.

andrewtoth commented at 8:10 PM on September 20, 2026:

Right, we would grant an extra connection if ReattemptPrivateBroadcast was called before all 3 initial connections were made. Fixed by also checking that the tx is no longer pending.

vasild commented at 5:19 PM on September 18, 2026: contributor

Approach ACK e4204baff71dfef93597ce5c92ee8111b5b380b0

The code looks solid. It introduces a rigid number of times a transaction will be broadcast, regardless of whether it is received back or not. It starts at 3 and may be incremented if the transaction is stale in order to induce a new broadcast.

It also tracks when the connections are closed so that it does not remove a transaction from the list in the middle of a connection.

I did not review the tests yet. Will review them for a full ACK.

Thanks!

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Approach ACK e4204baff71dfef93597ce5c92ee8111b5b380b0

The code looks solid. It introduces a rigid number of times a transaction will be broadcast, regardless of whether it is received back or not. It starts at 3 and may be incremented if the transaction is stale in order to induce a new broadcast.

It also tracks when the connections are closed so that it does not remove a transaction from the list in the middle of a connection.

I did not review the tests yet. Will review them for a full ACK.

Thanks!
-----BEGIN PGP SIGNATURE-----

iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmqtcfUbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/mJwf/jA0KM2pZZ4MF7NrK6Vh
BVpc344gxh1913MWk9U4ArzFtKhgU9oreZ99UiZAcOZRaYrwvYwN5nP/v8MKneO9
2S6WoilOcWdTo4GuKbtqFlasXc8L/VQNjlczWY2kBJrsCsDu4POH5EHmY9dIZbnA
8Y1ZvX+RUNQnIYY11KBjVWg6+cwocYRdlOdkz8fij4d4rI5Bpek6oAWJY8Cb6a8v
lqjEFfkp9bx9oGkuktJlNubIABXO/e/jyYB1+1+dA4y09LS0R4qfXE0Z7PSjBoQB
AJpYkVwFasGB3H5NuxjffKfLXFKdbxv7M+vl8zxmIiIiDcWic/7o6EHCYUrp7NxA
fdZQvoWqwbteWHAWNLbSavprq2ftqVOOzyPhfEyXu7uklpU8M3HzRNFm+ocB7XNb
VZULG69A+p7wfSbBBQScNm8a7RSnF05zp8v8ONsgNIKS6C0Cdi4HqWXEFt3PKQFg
bcqT4ze0pz3VxkdTnEW2G25kJnnCyfYw3ILt606N8TfU1PAXOrG24aahQnU5EsVD
b4qNTi32YWIOoHcjpLOYojPJ0DC3LSuJbK9V9/fvFo1V+9OtKH0uo7yz4mDZsaN2
3IrWYQbWXlwY8v3SmoRdeNKaQO46cZheliL5ni5KFLpLbfwgGbXIMUGA18GbB2cg
6qTfo5lwJ0rVRxNNi4xfuX0no6S4fXZiM4rx9wXAbF4UJNfoW2z+VCkNbqqDzk2/
qqOKiY6lvNHN7R33lDWj9WKJnh9p2CZTpNynd8s0cz0OVVAuP2Q/L7Ci+wXty+eG
NVeD/HSqmSGb+jwepQJXfJ4TMMyOKt/c0vphzpID17BZnsH48iSZ8axX4w/LR8ef
VLigikN2oxnqT4BFQPNh0u5GET8lOS5bkUdpMCFyfGb7uCUbLnd93qW75LYJcRVp
SodDVfYpgVM/3JVK/eXfVvfQtNW1iZYQwxlnn7bfrIsSOtz8hwaOnZ2cQJDW7VUD
Riv9xqnU7aEEMKs8QPkvnhXV1tgP9RLhP7ECDc+VoqF77wjvu7SVkstDxkeCngcO
E2A0GOcXbuLzE0Vp7irlHMjGwJWNp+D1dHpXsf1Lk5Z/48At/z+GAdVcQijsWMbH
3TiUWg0VYdyVhmEMnTwqBaI/MN9cUmC+Gh8l/5Id3UwL8Wsedc88/rtM3dph/TSc
0k/+07QRhLtJ2ELOh9emua2CSAua2iPcMmH0uHws0TB8MjayCmU/kDzE+FpAOAxN
oiXeda3qrX8zGp3qzwJtuadkjG1oMtGzHJGEPAQ8HV3E6uPk6wZloHp7Y2GIV0g/
H32YRC73S3zTF4W273Q/2jihs2ealLqFa/SZt0vwMi/oPfz6Zi8IVOtgNz8bka+w
o0M=
=9DC2
-----END PGP SIGNATURE-----

vasild's public key is on openpgp.org

</details>

andrewtoth force-pushed on Sep 20, 2026

DrahtBot added the label CI failed on Sep 20, 2026

DrahtBot removed the label CI failed on Sep 20, 2026

andrewtoth commented at 8:29 PM on September 20, 2026: contributor

Thanks @vasild for your detailed review. I have taken most of your suggestions.

Renamed MarkReceived -> MarkResolved.
Cleanup finished txs at capacity, so we can remove NodeDisconnected and related disconnected state.
Removed DidNodeConfirmReception.
Other various renames and cleanups.

https://github.com/bitcoin/bitcoin/compare/e4204baff71dfef93597ce5c92ee8111b5b380b0..dad979bf010adeed16d9eebe9fb822f5c519b90c

in src/private_broadcast.h:35 in dad979bf01

  31 |  class PrivateBroadcast
  32 |  {
  33 |  public:
  34 |  
  35 | +    /// Number of connections to make for initial broadcast.
  36 | +    static constexpr size_t INITIAL_BROADCAST_COUNT{3};

vasild commented at 7:19 AM on September 21, 2026:

super naming nit: since this variable is in the context of the PrivateBroadcast class, it does not need to contain the word "broadcast". There is some redundancy and unnecessary verbosity:

PrivateBroadcast::INITIAL_BROADCAST_COUNT could be PrivateBroadcast::INITIAL_COUNT

feel free to ignore.

in src/test/fuzz/private_broadcast.cpp:48 in dad979bf01

  47 | @@ -48,6 +48,7 @@ FUZZ_TARGET(private_broadcast)
  48 |      // Random transaction that the test generated and passed to Add(). Trimmed when Remove() is called.

vasild commented at 8:00 AM on September 21, 2026:

    // Random transactions that the test generated and passed to Add(). Trimmed when Remove() is called.

in src/test/fuzz/private_broadcast.cpp:51 in dad979bf01 outdated

  47 | @@ -48,6 +48,7 @@ FUZZ_TARGET(private_broadcast)
  48 |      // Random transaction that the test generated and passed to Add(). Trimmed when Remove() is called.
  49 |      // The values are the number of times a transaction was picked for sending.
  50 |      std::unordered_map<CTransactionRef, size_t, CTransactionRefHash, CTransactionRefComp> transactions;
  51 | +    std::unordered_map<CTransactionRef, size_t, CTransactionRefHash, CTransactionRefComp> planned_sends;

vasild commented at 8:01 AM on September 21, 2026:

// The values are the number of times a transaction is planned to be sent.
    std::unordered_map<CTransactionRef, size_t, CTransactionRefHash, CTransactionRefComp> planned_sends;

in src/private_broadcast.cpp:18 in dad979bf01 outdated

  14 | @@ -15,15 +15,24 @@ PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
  15 |  {
  16 |      LOCK(m_mutex);
  17 |      if (const auto it{m_transactions.find(tx)}; it != m_transactions.end()) {
  18 | -        if (IsPending(it->second)) return AddResult::AlreadyPresent;
  19 | +        if (it->second.send_statuses.size() < m_max_send_attempts) return AddResult::AlreadyPresent;

vasild commented at 9:16 AM on September 21, 2026:

This is fine, but I am just thinking - here we allow readd/reset only if the transaction has been sent 1000 times. I do not see this actually ever happening in realistic environments. Is it not more likely that somebody may want to readd/reset (and thus send again) a transaction which has reached its planned_sends and has been marked as resolved?

What about changing this condition to:

        if (!it->second.resolved || IsPending(it->second)) return AddResult::AlreadyPresent;

mzumsande commented at 8:01 PM on September 21, 2026:

in any case, the comment in line 20 should be made precise, because it is unclear what "exhausted" means, because it changed bahavior: Now the retry is basically dead code, since it will take a long time until 1000 resend attempts are reached - I think it might as well be removed. Before this change, it could be reached within minutes.

vasild commented at 12:26 PM on September 22, 2026:

master:

IsPending():
    return status.send_statuses.size() < m_max_send_attempts;

Add():
    if (IsPending(it->second)) return AddResult::AlreadyPresent;

PR:

Add():
    if (it->second.send_statuses.size() < m_max_send_attempts) return AddResult::AlreadyPresent;

andrewtoth commented at 2:40 AM on September 23, 2026:

I've reverted to clear resolved/exhausted entries on Add, so we reset as before. This makes this no longer relevant.

in test/functional/p2p_private_broadcast.py:324 in dad979bf01

 320 | @@ -321,10 +321,10 @@ def set_tx_returner_and_other():
 321 |          pending = [t for t in pbinfo["transactions"] if t["txid"] == txs[0]["txid"] and t["wtxid"] == txs[0]["wtxid"]]
 322 |          assert_equal(len(pending), 0)
 323 |  
 324 | -        self.log.info("Sending a transaction that is already in the mempool")

vasild commented at 9:54 AM on September 21, 2026:

This test had some value. It was removed because txs[0] can no longer be used for it. Can we create another, unrelated transaction, put it in the tx_originator's mempool somehow (maybe get another peer to INV it to tx_originator edit: or directly send TX message) and keep this test?

andrewtoth commented at 2:41 AM on September 23, 2026:

I reverted the behavior back to using NodeDisconnected, so this is also no longer changed.

in test/functional/p2p_private_broadcast.py:327 in dad979bf01

 326 | -        tx_originator.sendrawtransaction(hexstring=txs[0]["hex"], maxfeerate=0)
 327 | -        self.check_broadcasts("Broadcast of mempool transaction", txs[0], NUM_PRIVATE_BROADCAST_PER_TX, skip_destinations)
 328 | +        self.log.info("Resending the received transaction must not restart private broadcast")
 329 | +        with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
 330 | +            tx_originator.sendrawtransaction(hexstring=txs[0]["hex"], maxfeerate=0)
 331 | +        assert_equal(tx_originator.getprivatebroadcastinfo()["transactions"], [])

vasild commented at 9:59 AM on September 21, 2026:

This is a weird situation for the user:

Trying to resend a transaction is refused with "Ignoring unnecessary request to schedule an already scheduled transaction"
getprivatebroadcastinfo RPC does not contain the transaction
abortprivatebroadcast RPC errors with "Transaction not in private broadcast queue"
new send attempts will not be made by the system

I think would be resolved by the suggestion in #36277 (review) because that would allow a forced resend (first point above will change).

andrewtoth commented at 2:41 AM on September 23, 2026:

Reverted to using NodeDisconnected, so this is no longer relevant.

in test/functional/p2p_private_broadcast_completion.py:70 in dad979bf01 outdated

  65 | +        for i, peer in enumerate(peers):
  66 | +            P2PInterface.on_version(peer, peer.last_message["version"])
  67 | +            peer.wait_for_disconnect()
  68 | +            assert_equal(peer.last_message["tx"].tx.txid_hex, tx["txid"])
  69 | +            if i == 0:
  70 | +                self.log.info("Return the transaction while the other two peers still wait on handshake")

vasild commented at 10:24 AM on September 21, 2026:

This test establishes the 3 connections and then returns the transaction and then checks that the transaction gets delivered to the existent connections.

Would be nice if we can test that new connections are established/opened as well, after receiving the transaction. Could be achieved by:

shut down the proxy
sendrawtransaction
will be some "connection refused" errors trying to connect to the proxy
receive back the transaction
make the proxy work
observe 3 proper connections come to it that deliver the transaction

Maybe not worth it if it is going to overcomplicate this test.

andrewtoth commented at 2:41 AM on September 23, 2026:

Added the test.

vasild approved

vasild commented at 10:40 AM on September 21, 2026: contributor

ACK dad979bf010adeed16d9eebe9fb822f5c519b90c modulo weird situation for the user

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

vasild's public key is on openpgp.org

</details>

in src/private_broadcast.cpp:30 in d5324e53dc

  27 |      }
  28 |  
  29 | -    if (m_transactions.size() >= m_max_transactions) return AddResult::QueueFull;
  30 | +    if (m_transactions.size() >= m_max_transactions) {
  31 | +        // Clean up resolved transactions when space is needed.
  32 | +        std::erase_if(m_transactions, [this](const auto& entry) {

mzumsande commented at 8:11 PM on September 21, 2026:

This would lead to a potential unexpected increase in resource usage - before, we'd only keep transactions while they were in flight, and the shared pointer would be released, typically when they were mined. Now, we'll keep them forever (well, until the next restart or until the maximum limit is reached) without a real necessity, which doesn't seem ideal, if someone would send a lot of large txns with private broadcast.

andrewtoth commented at 11:50 PM on September 21, 2026:

Indeed, but the tradeoff here is that we do not need to have a third new method NodeDisconnected and track the disconnected state for each SendStatus. I'm not sure which way reviewers would prefer.

vasild commented at 12:39 PM on September 22, 2026:

Deleting the transactions eagerly is not a blocker for me. My preference, however, is to have a simpler interface (the public section of the PrivateBroadcast class) and simpler implementation (src/private_broadcast.cpp), which means to not track when a peer is disconnected. The queue is limited to 10k transactions. A full queue with 500bytes-sized transactions would occupy 5MB.

Some historical data on transactions' sizes: https://bitcoinvisuals.com/chain-tx-size

andrewtoth commented at 2:43 AM on September 23, 2026:

I reverted to deleting the entries at the start of Add. This re-adds the NodeDisconnected method and disconnected state, but it resolves this concern. It was brought up as an objection to #34707 as well, so hopefully it makes this change less controversial.

in src/net_processing.cpp:2003 in d5324e53dc

1999 | @@ -1994,8 +2000,8 @@ std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint25
2000 |          if (tx->GetHash().ToUint256() != id && tx->GetWitnessHash().ToUint256() != id) continue;
2001 |          if (const auto peer_acks{m_tx_for_private_broadcast.Remove(tx)}) {
2002 |              removed_txs.push_back(tx);
2003 | -            if (NUM_PRIVATE_BROADCAST_PER_TX > *peer_acks) {
2004 | -                connections_cancelled += (NUM_PRIVATE_BROADCAST_PER_TX - *peer_acks);
2005 | +            if (PrivateBroadcast::INITIAL_BROADCAST_COUNT > *peer_acks) {

mzumsande commented at 8:24 PM on September 21, 2026:

Should Remove() return planned_sends - send_statuses.size()? Seems like it was wrong/brittle already before, but now a peer that completed the handshake (fDisconnect=true) but doesn't answer the pong would not get a peer_ack, but also wouldn't get a try back, so if we then call Remove() it could steal slots from other, unrelated transactions.

andrewtoth commented at 2:43 AM on September 23, 2026:

Done.

mzumsande commented at 8:50 PM on September 21, 2026: contributor

Concept ACK

(haven't looked at the test changes yet)

andrewtoth force-pushed on Sep 23, 2026

andrewtoth commented at 2:49 AM on September 23, 2026: contributor

@mzumsande @vasild thank you for your reviews.

Reverted to using NodeDisconnected and storing the disconnected state. This makes cleanup at the beginning of Add possible, and gets rid of the concern in #36277 (review).
Fixed the accounting in Remove to return the difference between planned sends and current sends.
Extended the functional test to also receive the tx when the proxy is down and then ensure all 3 connections are still completed.
Various cleanups.

in src/private_broadcast.cpp:48 in e2a6665e87

  43 | @@ -35,12 +44,41 @@ std::optional<size_t> PrivateBroadcast::Remove(const CTransactionRef& tx)
  44 |      LOCK(m_mutex);
  45 |      const auto handle{m_transactions.extract(tx)};
  46 |      if (handle) {
  47 | -        const auto p{DerivePriority(handle.mapped().send_statuses)};
  48 | -        return p.num_confirmed;
  49 | +        const auto& state{handle.mapped()};
  50 | +        return std::min(state.planned_sends, m_max_send_attempts) - state.send_statuses.size();

vasild commented at 1:57 PM on September 23, 2026:

Logically, planned_sends or m_max_send_attempts should always be bigger than send_statuses.size(). However they are updated in different places in the code and the > relationship depends on some higher level logic and not trivial to verify. It is correct now. But because we do not know how this code will be changed in the future, it would be more robust to guard against an underflow here:

const size_t planned{std::min(state.planned_sends, m_max_send_attempts);
const size_t actual{state.send_statuses.size()};
return planned > actual ? planned - actual : 0;

optout21 commented at 9:14 AM on September 28, 2026:

8912547 net: always complete all initial private broadcast connections:

Second to that, as this form is also underflow-risk-free, while the current for can technically underflow.

andrewtoth commented at 3:26 PM on October 1, 2026:

Done.

vasild approved

vasild commented at 2:05 PM on September 23, 2026: contributor

ACK e2a6665e8719ecc5ccd9ed03d3880c5543a15797

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

vasild's public key is on openpgp.org

</details>

DrahtBot requested review from mzumsande on Sep 23, 2026

instagibbs commented at 2:16 PM on September 23, 2026: member

is this intended for backport?

andrewtoth commented at 2:24 PM on September 23, 2026: contributor

is this intended for backport? @instagibbs ideally this can get merged and backported before release.

sedited added the label Needs Backport (32.x) on Sep 23, 2026

instagibbs commented at 3:21 PM on September 23, 2026: member

This PR should be merged and backported before a release or not at all imo. It's a pretty large behavioral delta which would compete for review time against a more complete fix for the issues a little further out.

Caveat: I will not be reviewing it further, just my two cents

andrewtoth commented at 3:45 PM on September 23, 2026: contributor

This PR should be merged and backported before a release or not at all imo. It's a pretty large behavioral delta which would compete for review time against a more complete fix for the issues a little further out.

I somewhat agree, but it's hard to say for sure until we have a consensus on what is meant by "a more complete fix". It is a fairly large change that is already past the release cutoff. However, we agreed on language that we are providing our users "best-effort" privacy. Our best-effort would be to try and backport this change.

vasild commented at 10:43 AM on September 24, 2026: contributor

IMO this should be included in 32.0 and in 31.x

sedited added this to the milestone 32.0 on Sep 24, 2026

in src/private_broadcast.cpp:21 in 8912547752 outdated

  13 | @@ -14,12 +14,21 @@ PrivateBroadcast::AddResult PrivateBroadcast::Add(const CTransactionRef& tx)
  14 |      EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
  15 |  {
  16 |      LOCK(m_mutex);
  17 | +    // Cleanup finished transactions
  18 | +    std::erase_if(m_transactions, [this](const auto& entry) {
  19 | +        const auto& state{entry.second};
  20 | +        return state.resolved && !IsPending(state) &&
  21 | +               std::ranges::all_of(state.send_statuses, [](const auto& status) { return status.disconnected; });

optout21 commented at 9:10 AM on September 28, 2026:

8912547 net: always complete all initial private broadcast connections:

Nit: The expression could be moved to a separate method, e.g. ReadyForCleanup(). Can be non-public.

andrewtoth commented at 3:27 PM on October 1, 2026:

Since this is would be the single callsite, I preferred not to move this into a helper. Hope that's ok.

in src/private_broadcast.h:37 in 8912547752

  32 | @@ -30,6 +33,9 @@ class PrivateBroadcast
  33 |  {
  34 |  public:
  35 |  
  36 | +    /// Number of connections to make for initial broadcast.
  37 | +    static constexpr size_t INITIAL_COUNT{3};

optout21 commented at 9:17 AM on September 28, 2026:

8912547 net: always complete all initial private broadcast connections:

Nit: The name could be a bit more telling, e.g. INITIAL_CONNECTION_COUNT

andrewtoth commented at 3:26 PM on October 1, 2026:

Done.

optout21 commented at 9:21 AM on September 28, 2026: contributor

Concept ACK

More streamlined change than #34707 (but it also achieves less). Left few nits.

in src/private_broadcast.cpp:187 in 8912547752 outdated

 183 | @@ -145,7 +184,8 @@ std::vector<PrivateBroadcast::TxBroadcastInfo> PrivateBroadcast::GetBroadcastInf
 184 |  
 185 |  bool PrivateBroadcast::IsPending(const TxSendStatus& status) const
 186 |  {
 187 | -    return status.send_statuses.size() < m_max_send_attempts;
 188 | +    const size_t limit{std::min(status.planned_sends, m_max_send_attempts)};

mzumsande commented at 3:26 PM on September 28, 2026:

nit: maybe add a comment here that it deliberately ignores resolved, so that the initially scheduled connections are always completed.

vasild commented at 9:05 AM on October 1, 2026:

So that somebody does not "fix" it in the future :-D

andrewtoth commented at 3:27 PM on October 1, 2026:

Added the comment.

in src/test/fuzz/private_broadcast.cpp:211 in 8912547752 outdated

 207 | @@ -207,6 +208,14 @@ FUZZ_TARGET(private_broadcast)
 208 |                      Assert(!pb.HavePendingTransactions());
 209 |                  }
 210 |              },
 211 | +            [&] { // TryGrantRetry()

mzumsande commented at 3:35 PM on September 28, 2026:

would be nice if the fuzz test would also call MarkResolved() and NodeDisconnected(), so it covers the actual way private broadcast works in the node. This would also require changing some assertion, as fas as I am concerned it could also be done in a follow-up.

andrewtoth commented at 3:29 PM on October 1, 2026:

I added the two other methods to the fuzz test, which also required adding some more state to track. Some of the new state containers could be combined into structs, but that would be a bigger change so I would prefer to leave that cleanup to a follow-up.

mzumsande commented at 5:21 PM on September 28, 2026: contributor

Code Review ACK e2a6665e8719ecc5ccd9ed03d3880c5543a15797

DrahtBot requested review from optout21 on Sep 28, 2026

net: always complete all initial private broadcast connections

Make sure we send out all three connections when intiating a private broadcast.

Use MarkResolved instead of Remove to mark a tx as having been received back or mempool-conflicted.
Prevent additional stale retry connections by tracking disconnects and explicitly granting new connections with TryGrantRetry.

dc95a705d4

net: remove PrivateBroadcast::DidNodeConfirmReception

This method is no longer called in production.

00c1b52ee5

andrewtoth force-pushed on Oct 1, 2026

andrewtoth commented at 3:34 PM on October 1, 2026: contributor

Thanks @mzumsande @optout21 @vasild for your reviews. I have taken most of your suggestions.

Guard against integer underflow in Remove #36277 (review)
Rename INITIAL_COUNT -> INITIAL_CONNECTION_COUNT #36277 (review)
Add comment that ignoring resolved is deliberate in IsPending #36277 (review)
Added MarkResolved and NodeDisconnected to fuzz test #36277 (review)

I ran the updated fuzz test overnight on 8 cores for ~10.5 hours, so ~84 hours of CPU time at a combined ~30k exec/s. No issues found.

git diff e2a6665e8719ecc5ccd9ed03d3880c5543a15797..00c1b52ee56b9a2f067befc56c171eef27efda41

vasild approved

vasild commented at 2:15 PM on October 5, 2026: contributor

ACK 00c1b52ee56b9a2f067befc56c171eef27efda41

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

vasild's public key is on openpgp.org

</details>

DrahtBot requested review from mzumsande on Oct 5, 2026

mzumsande commented at 2:25 PM on October 5, 2026: contributor

Code Review ACK 00c1b52ee56b9a2f067befc56c171eef27efda41

danielabrozzoni commented at 5:52 AM on October 6, 2026: member

I haven't reviewed the code yet, I plan to do this in the next few days, but please don't hold up merge waiting for my review, I'm ok reviewing post merge, especially since the release is coming up :)

Concept ACK

As far as I understand, currently on master there are two possible leaks, and this PR is solving them both:

inv leak

We have one transaction in the private broadcast queue, we open a connection to a peer. If the connection handshake finishes after we receive the transaction back from the network, we will disconnect without sending an INV.

A sybil attacker can use this to confirm a suspicion that a transaction is ours: it can delay the VERACK, feed us the tx over another connection, complete the handshake and observe that we don't INV over the first connection.

getdata leak

The flow of messages sent over a private broadcast connection is the following:

handshake
us -- inv --> receiver
us < -- getdata -- receiver
us -- tx --> receiver
ping-pong to confirm tx reception

In private broadcast connections we only reply to getdata regarding the specific broadcast transaction that we INVed. In master we currently remove transactions after they are received back from the network, so if a peer sends getdata after we receive the transaction back from the network, we won't reply to it and instead disconnect.

A sybil attacker can use this to confirm a suspicion that a transaction is ours: it can delay the GETDATA, feed us the tx over another connection, send the GETDATA in the first connection and observe that we don't reply to it, and disconnect instead.

sedited approved

sedited commented at 7:42 AM on October 6, 2026: contributor

utACK 00c1b52ee56b9a2f067befc56c171eef27efda41

DrahtBot requested review from danielabrozzoni on Oct 6, 2026

sedited merged this on Oct 6, 2026

sedited closed this on Oct 6, 2026

in src/private_broadcast.h:122 in dc95a705d4

 119 | +
 120 | +    /**
 121 | +     * Mark a connection finished.
 122 | +     * @param[in] nodeid Node whose connection has ended.
 123 | +     */
 124 | +    void NodeDisconnected(NodeId nodeid)

optout21 commented at 8:31 AM on October 6, 2026:

dc95a70 net: always complete all initial private broadcast connections:

Nit: This could be pass-by-reference (mainly for consistency).

optout21 commented at 8:51 AM on October 6, 2026: contributor

Post-Merge Code Review ACK 00c1b52ee56b9a2f067befc56c171eef27efda41 I've got rug-pulled, PR got merged while I was performing my review :) (motto: nothing is final :P )

I share two concerns:

What happens when connections can't be opened due to some reason (e.g. Tor is down). The submitted TX will stay in the queue, but not show up in getprivatebroadcastinfo, and can't be aborted via abortprivatebroadcast. Maybe it would be more transparent if it considered in both of these RPCs.
What happens when a transaction gets resolved for some reason (e.g. evicted from mempool or mempool check fails with a temporary fee issue), and the transaction get resubmitted? I think no retry will happen and no error is returned.

fanquake removed the label Needs Backport (32.x) on Oct 6, 2026

fanquake commented at 2:03 PM on October 6, 2026: member

Backported to 32.x in #36427.