private broadcast: limit outstanding txs to count of 1000 #35406

I feel like this would be a lot simpler by just using the time_added field, instead of adding a parallel data structure? It would be an O(n) scan, but it's capped at MAX_TRANSACTIONs.

        const CTransactionRef oldest{std::ranges::min_element(
            m_transactions, {},
            [](const auto& el) { return el.second.time_added; })};

Fair, if we're landing on 1000 cap or less I'll simplify.

Even 10k should be fine. It's not performance sensitive. This is guarding an edge case. I don't think anyone would hit this limit in practice.

done (right, obviously overkill for something that only happens once per rpc submission! not a recurring task)

style nit: don't use hungarian notation.

done

In commit: de1059f51bfbebc5f59423ee8e23608093381c92

The commit message:

As the feature may be expande in the future, explicity bounds helps reasoning going forward.

Should be:

As the feature may be expanded in the future, explicit bounds helps reasoning going forward.

The same applies to the PR description as well.

done

We can use the -maxconnections=0 workaround for this if needed (not sure it's needed).

leaving as is

de1059f private broadcast: limit outstanding txs to count of 1000:

The implicit assumption here is that size exceeds max by exactly one. This is true, for as long as adding only happens only through Add, and never simultaneously. Changing the if to while would be more robust -- it's not really needed, but it's worth considering.

We have the trailing Assume() that would catch this in debug builds, I think it's easiest to think about this way? Leaving as is.

I think this should be LogInfo at least?

Now that the caller is getting an error, going to leave it as debug

b466ec5 private broadcast: limit outstanding txs to count of 1000:

Nit: The return value of try_emplace is not checked here, though I don't see why it could fail. Maybe it could be assert-ed.

It's building a pair with the key being default value. I don't think an assert gives us much given the check just above. Will leave as is unless others agree.

up space when stuck via `abortprivatebroadcast`. (#35406)

done

        "aborted. Use getprivatebroadcastinfo to inspect the queue and abortprivatebroadcast to abort.\n"

done

It seems a bit awkward to compress the PrivateBroadcast::AddResult result into a bool to then later decompress it into a TransactionError again. Have you considered returning a TransactionError from InitiateTxBroadcastPrivate instead? Seems more upgradable too.

I also think [[nodiscard]] would be good here.

<details> <summary>git diff on e96c343137</summary>

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 8b6bdba75c..bdc4292166 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -540,7 +540,7 @@ public:
     std::vector<CTransactionRef> AbortPrivateBroadcast(const uint256& id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-    bool InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    node::TransactionError InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void SetBestBlock(int height, std::chrono::seconds time) override
     {
         m_best_height = height;
@@ -2264,20 +2264,20 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt
     }
 }
 
-bool PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx)
+node::TransactionError PeerManagerImpl::InitiateTxBroadcastPrivate(const CTransactionRef& tx)
 {
     const auto txstr{strprintf("txid=%s, wtxid=%s", tx->GetHash().ToString(), tx->GetWitnessHash().ToString())};
     switch (m_tx_for_private_broadcast.Add(tx)) {
     case PrivateBroadcast::AddResult::Added:
         LogDebug(BCLog::PRIVBROADCAST, "Requesting %d new connections due to %s", NUM_PRIVATE_BROADCAST_PER_TX, txstr);
         m_connman.m_private_broadcast.NumToOpenAdd(NUM_PRIVATE_BROADCAST_PER_TX);
-        return true;
+        return node::TransactionError::OK;
     case PrivateBroadcast::AddResult::AlreadyPresent:
         LogDebug(BCLog::PRIVBROADCAST, "Ignoring unnecessary request to schedule an already scheduled transaction: %s", txstr);
-        return true;
+        return node::TransactionError::OK;
     case PrivateBroadcast::AddResult::QueueFull:
         LogDebug(BCLog::PRIVBROADCAST, "Rejecting private broadcast, queue full (cap=%u): %s", PrivateBroadcast::MAX_TRANSACTIONS, txstr);
-        return false;
+        return node::TransactionError::PRIVATE_BROADCAST_FULL;
     } // no default case, so the compiler can warn about missing cases
     assert(false);
 }
diff --git a/src/net_processing.h b/src/net_processing.h
index 651e669ca2..b251e868c1 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -9,6 +9,7 @@
 #include <consensus/amount.h>
 #include <net.h>
 #include <node/txorphanage.h>
+#include <node/types.h>
 #include <private_broadcast.h>
 #include <protocol.h>
 #include <uint256.h>
@@ -146,10 +147,10 @@ public:
     /**
      * Initiate a private transaction broadcast. This is done
      * asynchronously via short-lived connections to peers on privacy networks.
-     * [@retval](/bitcoin-bitcoin/contributor/retval/) true The transaction is scheduled for private broadcast (or was already scheduled).
-     * [@retval](/bitcoin-bitcoin/contributor/retval/) false Rejected because the private broadcast queue is full.
+     * [@retval](/bitcoin-bitcoin/contributor/retval/) node::TransactionError::OK The transaction is scheduled for private broadcast (or was already scheduled).
+     * [@retval](/bitcoin-bitcoin/contributor/retval/) node::TransactionError::PRIVATE_BROADCAST_FULL Rejected because the private broadcast queue is full.
      */
-    virtual bool InitiateTxBroadcastPrivate(const CTransactionRef& tx) = 0;
+    [[nodiscard]] virtual node::TransactionError InitiateTxBroadcastPrivate(const CTransactionRef& tx) = 0;
 
     /** Send ping message to all peers */
     virtual void SendPings() = 0;
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 0d462103d4..e7877c6985 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -133,10 +133,7 @@ TransactionError BroadcastTransaction(NodeContext& node,
         node.peerman->InitiateTxBroadcastToAll(txid, wtxid);
         break;
     case TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
-        if (!node.peerman->InitiateTxBroadcastPrivate(tx)) {
-            return TransactionError::PRIVATE_BROADCAST_FULL;
-        }
-        break;
+        return node.peerman->InitiateTxBroadcastPrivate(tx);
     }
 
     return TransactionError::OK;

</details>

taken thanks

nit: this documentation style is now discouraged by developer-notes, instead it is recommended to:

<details> <summary>git diff on e96c343137</summary>

diff --git a/src/private_broadcast.h b/src/private_broadcast.h
index 1968d496fe..27af749cf0 100644
--- a/src/private_broadcast.h
+++ b/src/private_broadcast.h
@@ -56,9 +56,12 @@ public:
 
     /// Outcome of Add().
     enum class AddResult {
-        Added,          //!< The transaction was newly added.
-        AlreadyPresent, //!< The transaction was already present; no change.
-        QueueFull,      //!< Rejected: the queue is already at MAX_TRANSACTIONS.
+        //! The transaction was newly added.
+        Added,
+        //! The transaction was already present; no change.
+        AlreadyPresent,
+        //! Rejected: the queue is already at MAX_TRANSACTIONS.
+        QueueFull,
     };
 
     /**

</details>

done

nit: I don't think we need this?

<details> <summary> iwyu output</summary>

link

2026-06-04T16:25:41.6566108Z /home/runner/work/_temp/src/private_broadcast.cpp should add these lines:
2026-06-04T16:25:41.6566591Z #include <compare>    // for operator<, strong_ordering
2026-06-04T16:25:41.6568246Z #include <memory>     // for shared_ptr
2026-06-04T16:25:41.6568572Z #include <utility>    // for get, move
2026-06-04T16:25:41.6568793Z 
2026-06-04T16:25:41.6569029Z /home/runner/work/_temp/src/private_broadcast.cpp should remove these lines:
2026-06-04T16:25:41.6569486Z - #include <logging.h>  // lines 7-7
2026-06-04T16:25:41.6569789Z - #include <util/check.h>  // lines 8-8
2026-06-04T16:25:41.6569994Z 
2026-06-04T16:25:41.6570223Z The full include-list for /home/runner/work/_temp/src/private_broadcast.cpp:
2026-06-04T16:25:41.6570741Z #include <private_broadcast.h>
2026-06-04T16:25:41.6571101Z #include <algorithm>  // for __max_element_fn, max, max_element
2026-06-04T16:25:41.6571514Z #include <compare>    // for operator<, strong_ordering
2026-06-04T16:25:41.6571872Z #include <memory>     // for shared_ptr
2026-06-04T16:25:41.6572174Z #include <utility>    // for get, move

</details>

removed

I think this would benefit from [[nodiscard]] since we're returning error codes.

done

I think this can be simplified by just checking the response is unchanged?

<details> <summary>git diff on e96c343137</summary>

diff --git a/test/functional/p2p_private_broadcast_cap.py b/test/functional/p2p_private_broadcast_cap.py
index 57b9cf5390..909116aba1 100755
--- a/test/functional/p2p_private_broadcast_cap.py
+++ b/test/functional/p2p_private_broadcast_cap.py
@@ -74,15 +74,7 @@ class PrivateBroadcastCapTest(BitcoinTestFramework):
             assert_raises_rpc_error(-25, "Private broadcast queue is full",
                                     node.sendrawtransaction, child["hex"])
 
-        pbinfo = node.getprivatebroadcastinfo()
-        assert_equal(len(pbinfo["transactions"]), MAX_TRANSACTIONS)
-        present_wtxids = {t["wtxid"] for t in pbinfo["transactions"]}
-        for i, child in enumerate(children[:MAX_TRANSACTIONS]):
-            assert child["wtxid"] in present_wtxids, \
-                f"tx index {i} (wtxid={child['wtxid']}) should still be present"
-        for child in children[MAX_TRANSACTIONS:]:
-            assert child["wtxid"] not in present_wtxids, \
-                f"rejected tx (wtxid={child['wtxid']}) must not be in the queue"
+        assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])
 
         # Re-submitting an already-queued transaction is a no-op, not an error,
         # even when the queue is full.

</details>

done

nit: would benefit from [[nodiscard]]

done

I don't think documenting this in init has a lot of value, there is nothing the user can do here, it shouldn't generally occur, and the only way this can manifest is through the sendrawtransaction RPC where you've added proper documentation?

As the feature expands, various places will be effected yet this limit will still be enforced. Removed since I weakly hold this position.

Not too difficult to switch the limit from "number of transactions" to "number of bytes" and also base the limit on the actual amount of RAM on the system.

    const size_t max_mem{GetTotalRam().value_or(16_GiB) / 100}; // Use max 1% of the RAM.
    size_t used_mem{0};
    for (const auto& [t, _] : m_transactions) {
        used_mem += t->ComputeTotalSize();
        if (used_mem > max_mem) {
            return AddResult::QueueFull;
        }
    }

I do not want to do dynamic cap. Switching to aggregate serialized size cap alone would bring the effective capacity up ~100x to ~500x based on most usage patterns. The downside is the aborting UX becomes a bit more complicated if it's ever hit (and I need to write more comprehensive tests to test boundary stuff, which is fine)

edit: I'd want to do some more analysis and testing to see how things work when you have >>1k oustanding broadcasts probably first. I'll think along these lines for a bit.

made a longer comment in the main thread #35406 (comment)

I'm declining to take on such a change at this time.

These can all be BOOST_CHECK_EQUAL.

Can do the same here as above for consistency

        assert_equal(pbinfo["transactions"], node.getprivatebroadcastinfo()["transactions"])