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

instagibbs commented at 3:03 PM on May 28, 2026: member

Add a belt-and-suspenders feature, limit the amount of memory possible when unlucky or simply misconfigured. The worst case limit is roughly 400kB * 1000 = 400MB, regardless of usage pattern.

Before this change, sheer volume of broadcasts, mismatches in standardness rules, or simply fee mismatches may result in unbounded growth of memory usage. As the feature may be expande in the future, explicit bounds helps reasoning going forward.

Tests are a little repetitive of each other, but I like having them at both layers.

DrahtBot commented at 3:03 PM on May 28, 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/35406.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	stickies-v
Stale ACK	optout21

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:

#35252 (net: send decoy transactions via private broadcast by andrewtoth)
#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)
#34707 (net: keep finished private broadcast txs in memory by andrewtoth)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

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.

instagibbs force-pushed on May 28, 2026

fanquake commented at 3:14 PM on May 28, 2026: member

Is this for backport?

instagibbs commented at 3:20 PM on May 28, 2026: member

@fanquake worthy of backport for safety and user expectations going forward imo

fanquake added the label Needs Backport (31.x) on May 28, 2026

fanquake requested review from vasild on May 28, 2026

fanquake requested review from andrewtoth on May 28, 2026

instagibbs force-pushed on May 28, 2026

DrahtBot added the label CI failed on May 28, 2026

instagibbs force-pushed on May 28, 2026

in src/private_broadcast.cpp:24 in 65ad93bc12

  21 | +    m_transactions.try_emplace(tx, std::prev(m_insertion_order.end()));
  22 | +
  23 | +    if (m_transactions.size() > MAX_TRANSACTIONS) {
  24 | +        // Evict the oldest entry (FIFO). The front of m_insertion_order is the
  25 | +        // earliest-added tx; the just-inserted tx sits at the back.
  26 | +        const CTransactionRef oldest{m_insertion_order.front()};

andrewtoth commented at 7:28 PM on May 28, 2026:

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

instagibbs commented at 7:36 PM on May 28, 2026:

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

andrewtoth commented at 7:39 PM on May 28, 2026:

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.

instagibbs commented at 8:05 PM on May 28, 2026:

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

instagibbs force-pushed on May 28, 2026

in src/test/private_broadcast_tests.cpp:164 in de1059f51b

 156 | @@ -157,4 +157,59 @@ BOOST_AUTO_TEST_CASE(stale_unpicked_tx)
 157 |      BOOST_CHECK_EQUAL(stale_state[0], tx);
 158 |  }
 159 |  
 160 | +BOOST_AUTO_TEST_CASE(fifo_eviction_at_cap)
 161 | +{
 162 | +    PrivateBroadcast pb;
 163 | +    constexpr size_t kCap{PrivateBroadcast::MAX_TRANSACTIONS};
 164 | +    constexpr size_t kOver{5};

andrewtoth commented at 9:30 PM on May 28, 2026:

style nit: don't use hungarian notation.

instagibbs commented at 1:29 PM on May 29, 2026:

done

in src/rpc/mempool.cpp:60 in de1059f51b outdated

  59 | @@ -60,6 +60,9 @@ static RPCMethod sendrawtransaction()
  60 |          "dedicated, short-lived connections to Tor or I2P peers or IPv4/IPv6 peers\n"

andrewtoth commented at 9:31 PM on May 28, 2026:

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.

optout21 commented at 7:39 AM on May 29, 2026:

The same applies to the PR description as well.

instagibbs commented at 1:29 PM on May 29, 2026:

done

DrahtBot removed the label CI failed on May 28, 2026

in test/functional/p2p_private_broadcast_cap.py:22 in de1059f51b outdated

  17 | +
  18 | +class PrivateBroadcastCapTest(BitcoinTestFramework):
  19 | +    def set_test_params(self):
  20 | +        self.num_nodes = 1
  21 | +        # -privatebroadcast is incompatible with the framework's default
  22 | +        # -connect=0; allow autoconnect (no actual peers will succeed though).

andrewtoth commented at 9:33 PM on May 28, 2026:

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

instagibbs commented at 1:29 PM on May 29, 2026:

leaving as is

in src/private_broadcast.cpp:19 in de1059f51b outdated

  13 | @@ -12,8 +14,25 @@ bool PrivateBroadcast::Add(const CTransactionRef& tx)
  14 |      EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
  15 |  {
  16 |      LOCK(m_mutex);
  17 | -    const bool inserted{m_transactions.try_emplace(tx).second};
  18 | -    return inserted;
  19 | +    if (!m_transactions.try_emplace(tx).second) return false;
  20 | +
  21 | +    if (m_transactions.size() > MAX_TRANSACTIONS) {

optout21 commented at 7:45 AM on May 29, 2026:

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.

instagibbs commented at 1:29 PM on May 29, 2026:

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.

optout21 commented at 8:36 AM on May 29, 2026: contributor

ACK 401459243159e34fed93dc3ae1ec19c1c19075fc

Reviewed code, executed tests locally, LGTM.

This PR adds a safety-net max limit for the private broadcast queue. While it can be argued that over-use is not very likely and not very catastrophic (usage through protected RPC interface, modest expected regular usage, reduced memory usage), it is good practice to have safety limits in place. It makes the risk explicit (however small), and introduces a "just in case" limit. The limit of 1000 transaction is mostly arbitrary number, and not configurable. This is perfectly acceptable, having in mind the low-probability, low-harm nature of the queue being full.

private broadcast: limit outstanding txs to count of 1000

Add a belt-and-suspenders feature, limit the amount of memory
possible when unlucky or simply misconfigured. The worst case limit is
roughly 400kB * 1000 = 400MB, regardless of usage pattern.

Before this change, sheer volume of broadcasts, mismatches in
standardness rules, or simply fee mismatches may result in unbounded
growth of memory usage. As the feature may be expande in the future,
explicit bounds helps reasoning going forward.

53cec31ec2

private broadcast: add release note for limited cap 9aeb4f8810

instagibbs force-pushed on May 29, 2026

in src/private_broadcast.cpp:26 in 9aeb4f8810

  23 | +        // expanded in scope.
  24 | +        const auto oldest{std::ranges::min_element(
  25 | +                m_transactions, {},
  26 | +                [](const auto& el) { return el.second.time_added; })};
  27 | +        const CTransactionRef& tx_to_evict{oldest->first};
  28 | +        LogDebug(BCLog::PRIVBROADCAST,

stickies-v commented at 2:12 PM on May 29, 2026:

I think this should be LogInfo at least?

stickies-v commented at 2:19 PM on May 29, 2026: contributor

Concept ACK for bounding resource usage.

Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx? When it fails, user can use abortprivatebroadcast to drop what they want, rather than us doing it silently.

andrewtoth commented at 2:25 PM on May 29, 2026: contributor

Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx?

Might be easiest way, just do a length check in Add and return false if max or higher.

instagibbs commented at 2:35 PM on May 29, 2026: member

Approach-wise, did you consider letting sendrawtransaction error instead of dropping the oldest tx? When it fails, user can use abortprivatebroadcast to drop what they want, rather than us doing it silently.

In the misconfigured scenario, I think either strategy works just fine, we just don't want to crash and nothing is propagating for some reason.

In the high volume scenario, where we were simply wrong about how users intend to use it (now or going forward), they will have to know about and able to handle this explicitly (I have low confidence that people are doing sensible things with our weird error reporting fwiw).

One pro of explicit handling is that the caller will never be surprised that something ended up being dropped.

I'm slightly predisposed to handling it implicitly and allowing naive rebroadcasting to "fix" things, but I can see it both ways.

andrewtoth commented at 3:07 PM on May 29, 2026: contributor

I think hitting this means something is misconfigured, so explicitly erroring will alert the user to the issue. I would be in favor of @stickies-v's approach.

fanquake added the label Private Broadcast on May 29, 2026

stickies-v commented at 3:20 PM on May 29, 2026: contributor

they will have to know about and able to handle this explicitly

Isn't that true regardless? When we silently drop, they'll need to (well, should) track / manage submitted txs and implement logic on how/when to retry the tx. Regardless, from an interface design pov, I think explicit is better than implicit.

Not familiar with wallet, so just my 2c, won't push back on it further.

instagibbs commented at 3:22 PM on May 29, 2026: member

so explicitly erroring will alert the user to the issue.

Just noting again to be clear: Outside of manual use I expect the error to simply be ignore/logged by many integrations, along with "already in mempool" kinda messages.

I suppose with good docs, the error-ignoring user can simply check the broadcast set, evict whatever the like, and it will self-heal next attempt at broadcasting.

I think hitting this means something is misconfigured

We shouldn't ignore usage patterns that plausibly exist. Question is what is best when our assumptions are violated.

Isn't that true regardless? When we silently drop, they'll need to (well, should) track / manage submitted txs and implement logic on how/when to retry the tx.

The caller knows a tx isn't being confirmed by looking at blocks and calls sendrawtransaction again. No need to even look at error messages in normal operation with or without private broadcast.

Regardless, from an interface design pov, I think explicit is better than implicit.

Agreed. I think with good docs it can be made idiot proof. Let me get that implemented.

vasild commented at 4:58 PM on May 29, 2026: contributor

A few thoughts:

Just 1000? The OP says 400kB * 1000 = 400MB, but where did that 400kB come from? A typical transaction is a few hundred bytes. Plus some storage overhead, I think we can reasonably assume one transaction takes 500-1000 bytes of RAM in the private broadcast queue. So, in 400MB one can store about 400'000 transactions, conservatively.
I think it would be better to return an immediate error from the sendrawtransaction RPC because then the user (or the user program driving this):

knows which transaction couldn't be sent - the one it got error about
is more likely to have the transaction at hand so it can retry after a short wait for the queue to empty otherwise "some of the transactions you scheduled some time ago was dropped but you do not know which one". That is harder to handle.

I guess Bitcoin Core runs on a variety of hardware. Maybe some machines have 100x more RAM than others. It is not reasonable to assume that one number will fit all. So consider making this manually configurable via config option (eh :/) or set it internally based on the amount of RAM available.

instagibbs commented at 5:26 PM on May 29, 2026: member

Just 1000? The OP says 400kB * 1000 = 400MB, but where did that 400kB come from?

it's the currently maximum sized transaction we will accept into our mempool (or private broadcast set). I think the simplest alternative is to have the serialized sized accounting instead of pure tx accounting, so the average case would be much higher and unlikely to be hit. Just increases the required testing a bit, and users rectifying it would have to abandon a variable amount of stuck broadcasts to make room for new ones.

I think I'd rather bump this number a few X and keep the accounting / abandon flow as simple as possible.

I guess Bitcoin Core runs on a variety of hardware. Maybe some machines have 100x more RAM than others. It is not reasonable to assume that one number will fit all. So consider making this manually configurable via config option (eh :/) or set it internally based on the amount of RAM available.

eh same to me, would rather we keep things as simple as possible unless there is real demand for something other than a anti-footgun