p2p: Replace per-peer transaction rate-limiting with global rate limits #34628

ajtowns commented at 3:09 AM on February 20, 2026: contributor

Per-peer m_tx_inventory_to_send queues have CPU and memory costs that scale with both queue size and peer count. Under high transaction volume, this has previously caused severe issues (May 2023 disclosure) and still can cause measurable delays (Feb 2026 Runestone surge, with the msghand thread observed hitting 100% CPU and queue memory reaching ~95MB).

This PR replaces the per-peer rate limiting with a global queue using dual token buckets (limiting transaction by both count and serialized size). Transactions that arrive within the bucket capacity still relay nearly immediately, but excess transactions queue in a global backlog and drain as the token buckets refill.

Key parameters:

Count bucket: 14 tx/s, 420 capacity (30s buffer)
Size bucket: 20 kB/s (~12 MB/600s), 50 MB capacity
Outbound peers refill faster by a factor of 2.5

Per-peer queues are retained solely for privacy batching and are always fully emptied, removing the old INVENTORY_BROADCAST_MAX cap.

This reduces the memory and CPU burden during transaction spikes when the queuing logic is engaged from O(queue * peers) to O(queue), as the queued transactions no longer need to be retained per-peer or re-sorted per-peer.

Design discussion: https://gist.github.com/ajtowns/d61bea974a07190fa6c6c8eaef3638b9

DrahtBot added the label P2P on Feb 20, 2026

DrahtBot commented at 3:10 AM on February 20, 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/34628.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	0xB10C, polespinasa, instagibbs, naiyoma

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:

#35406 (private broadcast: limit outstanding txs to count of 1000 by instagibbs)
#35315 (refactor: Use NodeClock::time_point in more places by maflcko)
#35297 (p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll by maflcko)
#35267 (rpc: make getprivatebroadcastinfo fail if privatebroadcast is not enabled by polespinasa)
#35252 (net: send decoy transactions via private broadcast by andrewtoth)
#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)
#34824 (net: refactor: replace Peer::TxRelay RecursiveMutex instances with Mutex by w0xlt)
#34271 (net_processing: make m_tx_for_private_broadcast optional by vasild)
#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.

DrahtBot added the label CI failed on Feb 20, 2026

ajtowns force-pushed on Feb 20, 2026

ajtowns commented at 12:43 PM on February 20, 2026: contributor

CI failure is presumably either #34631 or #34387

DrahtBot removed the label CI failed on Feb 24, 2026

in src/util/tokenbucket.h:57 in 869a1ae012 outdated

  52 | +    }
  53 | +
  54 | +    /** Consume n tokens. Returns false if the balance dropped below m_max_debt. */
  55 | +    bool decrement(double n = 1.0)
  56 | +    {
  57 | +        m_value -= n;

chriszeng1010 commented at 5:33 PM on March 2, 2026:

Decrement can still go below m_max_debt before checking is complete.

ajtowns commented at 12:57 PM on March 4, 2026:

decrement() can always go below m_max_debt, it only reports when it has done so -- it leaves it up to the caller to not go further into debt.

DrahtBot added the label Needs rebase on Mar 11, 2026

ajtowns force-pushed on Mar 12, 2026

DrahtBot removed the label Needs rebase on Mar 12, 2026

0xB10C commented at 9:45 AM on March 12, 2026: contributor

Concept ACK!

I've been running this for a few days now and written down a few observations on a small mass-broadcast event that happend a few hours ago: https://bnoc.xyz/t/increased-b-msghand-thread-utilization-due-to-runestone-transactions-on-2026-02-17/81/11

The node with this patch was significantly less affected than the others running a recent master.

I haven't set up any monitoring for the newly added getnetworkinfo fields yet.

DrahtBot added the label CI failed on Mar 12, 2026

DrahtBot closed this on Mar 12, 2026

DrahtBot reopened this on Mar 12, 2026

ajtowns added this to the milestone 32.0 on Mar 12, 2026

DrahtBot removed the label CI failed on Mar 12, 2026

in src/txmempool.cpp:541 in 344de4b8dd outdated

 537 | @@ -538,6 +538,55 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 538 |          for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
 539 |          AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
 540 |      }
 541 | +

sipa commented at 6:22 PM on March 20, 2026:

In commit "txmempool: Add SortMiningScoreWithTopology"

This feels more like something for a fuzz or unit test. CTxMemPool::check is for internal consistency checks in the CTxMemPool representation, I feel.

ajtowns commented at 1:20 AM on March 21, 2026:

.... I might have spent too much time vibecoding and caught hallucinations? I could have sworn I was replacing existing code here. EDIT: Dropped this code.

in src/txmempool.cpp:605 in 344de4b8dd outdated

 601 | @@ -553,6 +602,27 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 602 |      assert(innerUsage == cachedInnerUsage);
 603 |  }
 604 |  
 605 | +std::vector<CTxMemPool::txiter> CTxMemPool::SortMiningScoreWithTopology(std::span<const Wtxid> wtxids, size_t n) const

sipa commented at 6:46 PM on March 20, 2026:

It looks like both eventual production call sites of this function (BumpInvVecForProcessing and PeerManagerImpl::SendMessages) do a deduplication pass on the results.

Would it make sense to do this on the fly inside this function? It can't use std::partial_sort anymore, but it can use std::make_heap and friends to implement partial sorting, with a dynamic end point until n distinct elements have been found? Something like

std::vector<CTxMemPool::txiter> CTxMemPool::SortMiningScoreWithTopology(std::span<const Wtxid> wtxids, size_t n) const
{
    auto cmp = [&](const auto& a, const auto& b) EXCLUSIVE_LOCKS_REQUIRED(cs) noexcept { return m_txgraph->CompareMainOrder(*a, *b) > 0; };

    std::vector<txiter> res;

    n = std::min(wtxids.size(), n);
    if (n > 0) {
        // Construct a heap with txiters for all wtxids that exist in the mempool.
        std::vector<txiter> heap;
        heap.reserve(wtxids.size());
        for (auto& wtxid : wtxids) {
            if (auto i{GetIter(wtxid)}; i.has_value()) {
                heap.push_back(i.value());
            }
        }
        std::ranges::make_heap(heap, cmp);

        // Pop transactions until n distinct ones in res have been found.
        res.reserve(heap.size());
        while (res.size() < n && !heap.empty()) {
            std::ranges::pop_heap(heap, cmp);
            if (res.empty() || heap.back() != res.back()) {
                res.push_back(heap.back());
            }
            heap.pop_back();
        }

        // Copy the remainder over, without sorting or deduplication.
        res.insert(res.end(), heap.begin(), heap.end());
    }

    return res;
}

With even more low-level code the duplicate vector can be avoided, I think. Tests don't pass with this, I haven't investigated why.

ajtowns commented at 1:14 AM on March 21, 2026:

Without having looked, is the comparison backwards?

My understanding is partial_sort has two benefits:

it only makes a heap out of the target size, so iterates through the source array once and then does log(k) work for each element, with better locality
when updating the k elements in the heap with a new element from the source, it does the sift-down algorithm which is more efficient than heap_push()/heap_pop(), but isn't exposed via the STL so would mean writing your own heap implementation

Deduping the fairly small output list as you pass through it, when duplicates are rare anyway, seemed fine to me?

sipa commented at 2:20 PM on March 21, 2026:

Without having looked, is the comparison backwards?

I don't think so. It's a max heap, but I want to pop the "lowest" elements off first, so I needed to swap the comparator I think.

My understanding is partial_sort has two benefits:

Interesting. So it has complexity O(n log m) (with n = number of elements, m = sorted prefix size), while the approach I have in mind is O(n + (m log n)) (O(n) to construct the heap of all elements, and then m operations of each O(log n) to extract the best m elements. Complexity-wise, my approach seems better, since n > m, except it has worse memory locality. This makes me wonder if I'm missing something, since the cppreference.cpp documentation seems to imply std::partial_sort is intended for low m values.

Deduping the fairly small output list as you pass through it, when duplicates are rare anyway, seemed fine to me?

Yeah, it probably doesn't matter that much. It just looked like the deduplication is something that CTxMemPool::SortMiningScoreWithTopology could do internally since both call sites need it anyway. And then it seemed possible to have the count be dynamic, but only when using a different approach than what std::partial_sort seems to enable.

ajtowns commented at 12:58 AM on March 22, 2026:

Complexity-wise, my approach seems better, since n > m, except it has worse memory locality.

Yeah. I think the ratio between the number of swaps each approach performs in the worst case is log2(m) : 2 -- if you give the input in exactly the wrong order, each element goes to the top of the heap with log(m) steps, whereas for the full heap, it adds up to 2. So for m~=100, that's 3.3x more swaps, but the swaps are contained to a set of 100 elements, which might get you more than a 3.3x speedup per-swap due to locality? (In the average case, for most elements you'll just compare to the top of heap element, find it's worse and do 0 swaps, and overall it reduces from O(n log(m) + m log(m)) to O(n + m log(m)) which is better than the O(n + m log(n)) from the full heap.

Oh, hmm; in the per-peer thread we're always taking everything, so I think there we should be explicitly using std::sort then and not any of this partial business anyway (pushed). That should ensure that the m sizes we're using in practice are always about 70 (-txsendrate times inbound broadcast interval).

in src/txmempool.cpp:483 in ea647debfd

 480 | @@ -481,10 +481,10 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 481 |          const CTransaction& tx = it->GetTx();
 482 |  
 483 |          // CompareMiningScoreWithTopology should agree with GetSortedScoreWithTopology()

sipa commented at 7:46 PM on March 20, 2026:

In commit "txmempool: Drop CompareMiningScoreWithTopology"

Comment is outdated now.

in src/util/tokenbucket.h:16 in 859e8eb020

  11 | +
  12 | +/** A token bucket rate limiter.
  13 | + *
  14 | + * Tokens are added at a steady rate (m_rate per second) up to a capacity
  15 | + * cap (m_cap). Tokens are removed by calling decrement(). The balance
  16 | + * may go negative down to m_max_debt; decrement() returns false when

sipa commented at 7:54 PM on March 20, 2026:

In commit "util/tokenbucket.h: Provide a generic TokenBucket class"

Is it useful to support debt? I believe it can be avoided by a transformation that raises both m_value and m_cap by -m_max_debt.

ajtowns commented at 4:04 AM on March 21, 2026:

The distinction is in InvToSendBucket::avail() which says "you can start doing stuff as long as the size_bucket's value is >=0", which would have to get incremented as well to be equivalent.

The main effect is that when the size bucket is under pressure, you get a chance to relay at least 50kB each iteration, rather than the avail test passing as soon as you can relay 1B, then the loop ending immediately after you relay the first tx.

I think it can be simplified a bit by moving the max_debt value to just being a parameter of decrement() though. Will update.

ajtowns force-pushed on Mar 21, 2026

ajtowns force-pushed on Mar 22, 2026

DrahtBot added the label CI failed on Mar 22, 2026

DrahtBot removed the label CI failed on Mar 22, 2026

in src/net_processing.cpp:6190 in ba3a81d036 outdated

6187 | @@ -6028,63 +6188,56 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6188 |                  // Determine transactions to relay
6189 |                  if (fSendTrickle) {
6190 |                      // Produce a vector with all candidates for sending

xyzconstant commented at 3:04 AM on April 15, 2026:

In commit: "net_processing: Change m_tx_inventory_to_send from set to vector" (2998d73f692059f149fa2d5a4108b172b21c8cac)

nit: Forgot to remove this comment as well?

ajtowns commented at 4:11 AM on April 15, 2026:

No? inv_tx is a vector with all candidates for sending in the new code.

xyzconstant commented at 4:33 AM on April 15, 2026:

Yeah you're right, but now filterrate definition sits in between

    // Produce a vector with all candidates for sending
    const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()};

    // Topologically and fee-rate sort the inventory we send for privacy and priority reasons.
    // sorted from lowest priority to highest, skipping low fee
    auto inv_tx = [&]() EXCLUSIVE_LOCKS_REQUIRED(tx_relay->m_tx_inventory_mutex) {

ajtowns commented at 4:32 AM on April 26, 2026:

Tweaked the comments a bit

DrahtBot added the label Needs rebase on Apr 23, 2026

ajtowns force-pushed on Apr 23, 2026

DrahtBot removed the label Needs rebase on Apr 23, 2026

in src/node/transaction.cpp:64 in cafa97202e outdated

  59 | @@ -60,15 +60,15 @@ TransactionError BroadcastTransaction(NodeContext& node,
  60 |              if (!existingCoin.IsSpent()) return TransactionError::ALREADY_IN_UTXO_SET;
  61 |          }
  62 |  
  63 | -        if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
  64 | +        mempool_tx = node.mempool->get(txid);
  65 | +        if (mempool_tx) {

polespinasa commented at 11:38 AM on April 24, 2026:

in cafa97202e95df278153369ccda9c1bb61880a5e I don't think we should have an if statement without code logic inside. Why not just if(!mempool_tx) and do what is inside the else code block

ajtowns commented at 3:05 AM on April 26, 2026:

The if block provides a place for the detailed comments on what happen in the case the tx is already in the mempool.

in src/net_processing.cpp:174 in 466dccc89c

 175 | -static constexpr unsigned int INVENTORY_BROADCAST_TARGET = INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL);
 176 | -/** Maximum number of inventory items to send per transmission. */
 177 | -static constexpr unsigned int INVENTORY_BROADCAST_MAX = 1000;
 178 | -static_assert(INVENTORY_BROADCAST_MAX >= INVENTORY_BROADCAST_TARGET, "INVENTORY_BROADCAST_MAX too low");
 179 | -static_assert(INVENTORY_BROADCAST_MAX <= node::MAX_PEER_TX_ANNOUNCEMENTS, "INVENTORY_BROADCAST_MAX too high");
 180 | +// static constexpr unsigned int INVENTORY_BROADCAST_TARGET = INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL);

polespinasa commented at 3:49 PM on April 24, 2026:

In 466dccc89c5748c7f4ccfb10b2079c2eb54589c5 you can delete this two lines that are commented and then deleted in 0491df8e7876b9b2b86e25190823f3ff632311c6.

ajtowns commented at 3:10 AM on April 26, 2026:

They're commented in the earlier commit then uncommented (not deleted) in the later commit. Commenting them makes it easy to see how they're (not) changed, while still allowing the in-between commits to compile without hitting "unused variable" warnings/errors.

in src/txmempool.cpp:541 in 798db790a8

 537 | @@ -538,6 +538,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 538 |          for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout);
 539 |          AddCoins(mempoolDuplicate, tx, std::numeric_limits<int>::max());
 540 |      }
 541 | +

polespinasa commented at 3:57 PM on April 24, 2026:

in 798db790a8a5b9c6b11b3e8599cb88f9d6015d87 nit: random empty line added here

in src/txmempool.cpp:567 in 798db790a8 outdated

 562 | +
 563 | +    n = std::min(wtxids.size(), n);
 564 | +    if (n > 0) {
 565 | +        res.reserve(wtxids.size());
 566 | +        for (auto& wtxid : wtxids) {
 567 | +            if (auto i{GetIter(wtxid)}; i.has_value()) {

polespinasa commented at 4:07 PM on April 24, 2026:

in 798db790a8a5b9c6b11b3e8599cb88f9d6015d87

nit: I think this can be simplified: if (auto i = GetIter(wtxid)) res.push_back(*i);

ajtowns commented at 3:13 AM on April 26, 2026:

The i.has_value() check is to catch if any of the txs have been removed from the mempool and to ensure that all the returned txiter's are valid. Having that check be implicit in the if is probably fine, but seems worse than writing it explicitly to me..

in src/txmempool.cpp:572 in 798db790a8 outdated

 567 | +            if (auto i{GetIter(wtxid)}; i.has_value()) {
 568 | +                res.push_back(i.value());
 569 | +            }
 570 | +        }
 571 | +
 572 | +        if (n >= res.size()) {

polespinasa commented at 4:47 PM on April 24, 2026:

in 798db790a8a5b9c6b11b3e8599cb88f9d6015d87

I think std::partial_sort gives the same result as std::sort if the mid iterator reaches the end. So changing the whole if ... else... block for:

std::partial_sort(res.rbegin(),
                  res.rbegin() + std::min(n, res.size()),
                  res.rend(),
                  cmp);

I think it should work.

ajtowns commented at 5:14 AM on April 25, 2026:

std::partial_sort is less efficient than std::sort when sorting the entire container, eg https://stackoverflow.com/questions/45455345/performance-of-stdpartial-sort-versus-stdsort-when-sorting-the-whole-ran

polespinasa commented at 4:51 PM on April 24, 2026: member

Concept ACK

Code reviewed till 798db790a8a5b9c6b11b3e8599cb88f9d6015d87 will continue soon :)

Left small comments and suggestions but nothing important, feel free to ignore.

DrahtBot added the label Needs rebase on Apr 24, 2026

ajtowns force-pushed on Apr 26, 2026

DrahtBot removed the label Needs rebase on Apr 26, 2026

ajtowns force-pushed on Apr 26, 2026

DrahtBot added the label CI failed on Apr 26, 2026

DrahtBot removed the label CI failed on Apr 26, 2026

ajtowns commented at 5:28 AM on April 26, 2026: contributor

Rebased past #35097, addressed review comments

in src/util/tokenbucket.h:43 in f632171cca

  38 | +    /** Refill tokens based on elapsed time since last call. No refill
  39 | +     *  occurs on the first call (establishes the time baseline). */
  40 | +    void increment(const time_point& now)
  41 | +    {
  42 | +        if (now > m_last_updated) {
  43 | +            if (m_value < m_cap && m_last_updated.time_since_epoch().count() > 0) {

polespinasa commented at 2:53 PM on April 27, 2026:

in f632171 I think this is a bit more clearer way to say "this is not the first call". As it is checking whether m_last_updated has been initialized or not.

if (m_value < m_cap && m_last_updated != time_point{}) {...}

ajtowns commented at 6:55 AM on April 28, 2026:

Yeah that's nicer. Introduce a MIN_TIME to compare against instead.

in src/net_processing.cpp:544 in 2a9ab8062b outdated

 539 | +        count_bucket.increment(now);
 540 | +    }
 541 | +
 542 | +    bool decrement(double size)
 543 | +    {
 544 | +        bool x = size_bucket.decrement(size, /*floor=*/-50e3);

polespinasa commented at 3:00 PM on April 27, 2026:

in 2a9ab8062bcadb5e8128572671bba762101efaa6 why negative floor?

ajtowns commented at 6:40 AM on April 28, 2026:

Because the comparison is m_value > floor not m_value > -floor

polespinasa commented at 11:13 AM on April 28, 2026:

Sorry, maybe the question was not clear. I mean why negative? Why can it go bellow 0 in the first place?

ajtowns commented at 3:23 PM on April 28, 2026:

So that if/when outgoing txs are bandwidth constrained (ie your bucket is often empty) you send ~50kB of tx data in each burst, rather than just the single highest priority tx in the backlog (since you'll immediately process the backlog when your size bucket goes above zero, even by just one byte).

polespinasa commented at 3:12 PM on April 27, 2026: member

code reviewed 54bbc5649c65395eadbff7f237359ad33a6862e5

Probably this needs a release note for -txsendrate and the new info in getnetworkinfo.

Left a small comment and a question

fanquake added the label Needs release note on Apr 27, 2026

ajtowns force-pushed on Apr 28, 2026

ajtowns commented at 6:57 AM on April 28, 2026: contributor

Added a release note. Are we meant to remove the "Needs release note" label when there's a release note included in the PR, or is it more like an "I need to breathe, and I am breathing" arrangement where the latter doesn't negate the former?

polespinasa commented at 7:14 AM on April 28, 2026: member

Added a release note. Are we meant to remove the "Needs release note" label when there's a release note included in the PR, or is it more like an "I need to breathe, and I am breathing" arrangement where the latter doesn't negate the former?

I think there's no policy on that, I've seen both cases in the past, #31278 got it and then removed it, #32138 got it and never removed the label even if the note was there.

IMHO is good to keep it as a reminder in case the release note is dropped by mistake at some point, so reviewers can realize that something is missing.

maflcko removed the label Needs release note on Apr 28, 2026

polespinasa commented at 11:18 AM on April 28, 2026: member

How can I help testing this? @0xB10C are you using a patch to measure it or just by enabling debug and net flag you catch inv_to_send

0xB10C commented at 3:59 PM on April 29, 2026: contributor

@0xB10C are you using a patch to measure it or just by enabling debug and net flag you catch inv_to_send

No, I've been running this PR. The measurements described on https://bnoc.xyz/t/increased-b-msghand-thread-utilization-due-to-runestone-transactions-on-2026-02-17/81/11 were done collecting data from a few different interfaces with peer-observer:

inv-to-send set sizes across multiple nodes via the getpeerinfo RPC
time spent in b-msghand thread via a prometheus process-exporter
the localhost ping-pong duration with a custom p2p client that measures the time it takes for the node to respond. This measures message backlog
size of the INVs the node sends us also with a custom P2P client on localhost that listens for INVs from the node.

Not sure if this helps much.

instagibbs commented at 8:11 AM on May 5, 2026: member

concept ACK, will review

in src/node/transaction.cpp:133 in f940743fac

 129 | @@ -130,7 +130,7 @@ TransactionError BroadcastTransaction(NodeContext& node,
 130 |      case TxBroadcast::MEMPOOL_NO_BROADCAST:
 131 |          break;
 132 |      case TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
 133 | -        node.peerman->InitiateTxBroadcastToAll(txid, wtxid);
 134 | +        node.peerman->InitiateTxBroadcastToAll(mempool_tx ? mempool_tx : tx);

instagibbs commented at 2:26 PM on May 12, 2026:

f940743fac03f27d8cf3c9f2d8a0dd5ba36209bd

Why not just tx unconfiditionally?

polespinasa commented at 7:29 PM on May 14, 2026:

I think is because we might have a tx in the mempool with same tx id but different witness. So we would be announcing a tx that we don't have in our mempool because it would conflict with our version of the tx.

instagibbs commented at 8:26 PM on May 14, 2026:

this is extremely non-obvious and should be documented if so

polespinasa commented at 8:34 PM on May 14, 2026:

It is :)

See my other comment: https://github.com/bitcoin/bitcoin/pull/34628/changes/BASE..f940743fac03f27d8cf3c9f2d8a0dd5ba36209bd#r3137421379

Maybe the comment could be moved removing the empty if?

instagibbs commented at 8:37 PM on May 14, 2026:

resolving, didnt notice the unmoved/unchanged comment in the diff

edit: github doesnt want to let me

in src/net_processing.cpp:173 in b47c81af3b

 168 | @@ -169,13 +169,9 @@ static constexpr auto INBOUND_INVENTORY_BROADCAST_INTERVAL{5s};
 169 |  static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL{2s};
 170 |  /** Maximum rate of inventory items to send per second.
 171 |   *  Limits the impact of low-fee transaction floods. */
 172 | -static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND{14};
 173 | +// static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND{14};
 174 |  /** Target number of tx inventory items to send per transmission. */

instagibbs commented at 2:36 PM on May 12, 2026:

b47c81af3b3e7ae59bc09a3f621ecdf8f3dc62da

unrelated to PR: appears to also be the target for blocks, not just tx?

ajtowns commented at 8:24 PM on May 14, 2026:

No: we do reserve that much space (INVENTORY_BROADCAST_TARGET) for the vector before announcing blocks by inv, but we'll just spam all the blocks we have queued (splitting into new messages as needed).

in src/txmempool.cpp:556 in 3a72379141

 552 | @@ -553,6 +553,31 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 553 |      assert(innerUsage == cachedInnerUsage);
 554 |  }
 555 |  
 556 | +std::vector<CTxMemPool::txiter> CTxMemPool::SortMiningScoreWithTopology(std::span<const Wtxid> wtxids, size_t n) const

instagibbs commented at 2:43 PM on May 12, 2026:

3a7237914178c9aa8018f86ae5e446f263e0c843

n is overly terse imo, and the help isn't clear to me either. n_best?

in src/net_processing.cpp:304 in 1ace17fc1b outdated

 300 | @@ -301,7 +301,7 @@ struct Peer {
 301 |           *  we retrieve the txid from the corresponding mempool transaction when
 302 |           *  constructing the `inv` message. We use the mempool to sort transactions
 303 |           *  in dependency order before relay, so this does not have to be sorted. */
 304 | -        std::set<Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
 305 | +        std::vector<Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);

instagibbs commented at 2:46 PM on May 12, 2026:

1ace17fc1bbf265bcb1100e8b025f279223c9da7

Still being called a set in the help

in src/net_processing.cpp:6049 in 1ace17fc1b outdated

6068 | +                    }();
6069 | +                    tx_relay->m_tx_inventory_to_send.clear();
6070 | +
6071 | +                    LOCK(tx_relay->m_bloom_filter_mutex);
6072 | +                    vInv.reserve(std::min<size_t>(MAX_INV_SZ, vInv.size() + inv_tx.size()));
6073 | +                    while (!inv_tx.empty()) {

instagibbs commented at 3:01 PM on May 12, 2026:

1ace17fc1bbf265bcb1100e8b025f279223c9da7

Feel like just reverse ranging it or similar, then not popping anything, is faster?

  for (auto it = inv_tx.rbegin(); it != inv_tx.rend(); ++it) {
      const auto& tx = *it;
      ...
  }

ajtowns commented at 8:23 PM on May 14, 2026:

"faster" ? Iterating over inv_tx a second time when destructing to decrement the CTxRefs would probably be slower I would have thought, but it seems likely to basically unmeasurable either way?

instagibbs commented at 8:25 PM on May 14, 2026:

ok, "less indirect"?

ajtowns commented at 6:30 AM on May 30, 2026:

I prefer the while (!empty) { x = back(); pop(); .... } approach here, so leaving as-is.

instagibbs commented at 6:08 PM on May 14, 2026: member

Some initial comments while I still work through the approach.

To be honest I'm finding it a little difficult to follow the lifetime of invs.

In this branch https://github.com/instagibbs/bitcoin/commit/3f87d24eea279fb6b68f5f9af9579cd8b8909db3 , I considered forcing all announcements through the backlog, and then draining this every tick if:

avail() is large enough to drain entire backlog (replacement for immediate path)
same as before in this PR, for when avail() batch gets "big enough" to cost a partial sort

I also am finding it difficult to understand the negative budgeting. InvToSendBucket::decrement return value is never checked and in my branch is deleted anyways. Does /*floor=*/-50e3 even do anything in the PR?

This change would mean in the immediate path we wouldn't be checking m_tx_inventory_known_filter and deduped later, fwiw.

Probably other issues with divergence in your attempt, but I can't make heads or tails right now.

ajtowns commented at 8:38 PM on May 14, 2026: contributor

I also am finding it difficult to understand the negative budgeting. InvToSendBucket::decrement return value is never checked and in my branch is deleted anyways. Does /*floor=*/-50e3 even do anything in the PR?

BumpInvVecForProcessing calls if (!inv_bucket.size_bucket.decrement(itervec[i]->GetTx().ComputeTotalSize())) which is where the -50e3 param should be having an effect (allowing a larger batch of txs when the size limit is in effect), but is missing.

naiyoma commented at 7:23 PM on May 29, 2026: contributor

Concept ACK.

I've attempted to test these changes using a Warnet scenario:

Setup:

4-node network,
38 additional silent listener peers attached to tank-0001 (they accept INVs but never reply, was trying to create a worst case for the per-peer sort path)
~10,000 transactions injected into tank-0000, then relayed to tank-0001
mempool size and test conditions consistent across both runs

I then compared the behavior before(on master) and after the changes in this PR.

The graphs below show per-peer inv_to_send queue sizes and ping times for each run, covering the period from initial connection. The first graph shows the queue size for each peer. The second graph shows the ping time for the nodes.(excluding silent peers)

On master, per-peer queues climbed to ~6,300 entries and honest-peer max pingtime crossed 200ms.

With this PR, per-peer queues stayed under ~440 and honest-peer max pingtime stayed under ~50ms.

measured b-msghand thread CPU on tank-0001 (same scenario, same network size, sampled per-second from /proc/<pid>/task/<tid>/stat):

On master, the thread crossed 50% CPU 13 times, with peaks near 90%.

With this PR, the same thread crossed 50% only twice.

ajtowns force-pushed on May 30, 2026

ajtowns commented at 6:34 AM on May 30, 2026: contributor

Reworked a bunch; now everything goes through the backlog which means there's just one code path, and simplifies a few things. It still immediately attempts to drain the backlog when receiving a new tx, so there's not much behaviour change (not doing this seems to require messing about with functional test assumptions). The size bucket decrement() floor now takes effect, and the count bucket now also uses a decrement() floor for the "try to do 70 transactions at once" logic.

in src/net_processing.cpp:6044 in 08528cd9bc

6043 | @@ -6046,13 +6044,9 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6044 |                      // A heap is used so that not all items need sorting if only a few are being sent.

instagibbs commented at 3:09 PM on June 1, 2026:

08528cd9bc1ac60538cc3fa9223c3ae7d9cfc286

This is probably nuked later, but this comment is stale as of this commit

in src/net_processing.cpp:2338 in 01b28af3ed

2349 | +        // save the remaining section as-is (probably mostly unsorted)
2350 | +        for (size_t j = 0; j < i; ++j) {
2351 | +            backlog.push_back(itervec[j]->GetTx().GetWitnessHash());
2352 | +        }
2353 | +        if (backlog.empty()) {
2354 | +            std::vector<Wtxid>{}.swap(backlog); // free memory associated with vec

instagibbs commented at 6:40 PM on June 1, 2026:

01b28af3ed7b794d214a3d1ab751605a2555d05f

I think this is a no-op? We're already swapping out backlog earlier in the function when we detect we are going to take all the entries, leaving an empty backlog, leading to another no-op swap?

ajtowns commented at 6:12 AM on June 2, 2026:

Good catch

instagibbs commented at 7:14 PM on June 1, 2026: member

Significantly clearer to me now, thanks.

I have another cleanup suggestion which makes it a bit easier for me to track what's happening: https://github.com/instagibbs/bitcoin/commit/5b26aa31ed42e2aba576f6837917c8d690e115bb

Like usual I may have missed a key detail.

ajtowns commented at 1:26 AM on June 2, 2026: contributor

I have another cleanup suggestion which makes it a bit easier for me to track what's happening: instagibbs@5b26aa3

I believe deduping first loses track of the distinction between the sorted elements and the unsorted ones, so if you sorted n+1 elements, then remove 5 duplicates, then relay the top n elements, you'll relay 4 elements with random fee rates.

net_processing: Remove per-peer rate-limiting

Per-peer rate limiting introduces storage and compute costs proportional
to the number of peers. This has caused severe bugs in the past, and
continues to be a risk in the event of periods of extremely high rates
of transaction submission. Avoid these problems by always completely
emptying the m_tx_inventory_to_send queue when processing it.

Note that this increases the potential size of INV messages we send
for normal tx relay from ~1000 (limited by INVENTORY_BROADCAST_MAX)
to potentially 50000 (limited by MAX_INV_SZ).

5dd3018db1

txmempool: Add ExtractBestByMiningScoreWithTopology

Add a method for (partially) sorting a batch of transactions (specified
as a vector of wtxids) per mempool order, designed for transaction relay.

a983ac1163

net_processing: Change m_tx_inventory_to_send from set to vector

Change the per-peer tx relay queue from std::set to std::vector. This
reduces the memory usage and improves locality, at the cost of not
automatically deduping entries. Also removed CompareInvMempoolOrder,
replacing it with the new SortMiningScoreWithTopology. The trickle send
code is reworked accordingly.

1f243296a1

txmempool: Drop CompareMiningScoreWithTopology

Now unused; replaced by SortMiningScoreWithTopology.

be6d3190e6

util/tokenbucket.h: Provide a generic TokenBucket class

This is a simple token bucket parameterized on clock type, used in the
following commit.

bc4aa952df

ajtowns force-pushed on Jun 2, 2026

ajtowns commented at 9:24 AM on June 2, 2026: contributor

Addressed feedback; txmempool func renamed to ExtractBestByMiningScoreWithTopology which now returns a fully sorted vector of txiters from best to worst, and updates the input vector with any remaining wtxids, minimising the work callers need to do.

net_processing: add a global delay queue for sending txs

Without the per-peer rate limiting, nodes can act as an amplifier for
transaction spam -- receiving many transactions from one node, but
relaying each of them to over 100 other nodes. Limit the impact of this
by providing a global rate limit.

This is implemented using dual token buckets, one that consumes a
token for every transaction, and one that consumes a token for every
serialized byte. This rate limits both per-tx resource usage (eg INV
messages) and overall relay bandwidth.

Main bucket parameters:
 * Count: 14tx/s rate, 420tx (30s) capacity
 * Size: 12MB/600s rate (4-6 blocks per target block interval), 50MB capacity

The size bucket is expected to be large enough to almost never have an
impact in normal usage, even during transaction storms, and is primarily
intended to mitigate attack-like scenarios.

Outbound connections get a separate pair of buckets, with rates boosted
by a 2.5x multiplier.

This avoids the excessive memory and CPU usage due to the 100x multiplier
from the queues being per-peer.

Note that this also reduces the size of INV messages we send for general
tx relay back to a more reasonable level of under 600 txs in 99.999%
of cases.

e7777f1c8b

init: add -txsendrate configuration parameter

Adds a debug-only configuration option to set the target
transaction/second rate for relay to inbound connections. This is mostly
intended to be set to artificially low values to aid in testing behaviour
when a backlog occurs, but is also available in case the default 14tx/s
target is somehow too low in practice.

696456f2f6

rpc: report -txsendrate and bucket info via getnetworkinfo

Add `tx_send_rate` and `inv_buckets` fields to getnetworkinfo. The
`inv_buckets` field has separate `inbound` and `outbound` entries,
reporting backlog count, count tokens, and size tokens. Useful for
monitoring relay behavior.

4752c3f73c

tests: basic functional test for tx rate limiting f5c4b02045

doc: Add release note for -txsendrate etc ba20da6c0f

net_processing: Drop unnecessary txid arg from InitiateTxBroadcastToAll 70646131f7

ajtowns force-pushed on Jun 2, 2026

DrahtBot added the label CI failed on Jun 2, 2026

DrahtBot removed the label CI failed on Jun 2, 2026

in src/txmempool.cpp:556 in a983ac1163

 552 | @@ -553,6 +553,61 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 553 |      assert(innerUsage == cachedInnerUsage);
 554 |  }
 555 |  
 556 | +std::vector<CTxMemPool::txiter> CTxMemPool::ExtractBestByMiningScoreWithTopology(std::vector<Wtxid>& wtxids, size_t n_to_sort) const

instagibbs commented at 12:56 PM on June 2, 2026:

a983ac1163e6638c4f9e0271982dbe9f8c864525

Take or leave suggestion: Could we return the new wtxids vector instead of editing in place? Could make things even clearer and still avoid performance hit.

in src/txmempool.cpp:582 in a983ac1163

 577 | +            bool extra = false;
 578 | +            if (n_to_sort + 1 >= res.size()) {
 579 | +                // use regular sort when taking everything
 580 | +                std::sort(begin, end, cmp);
 581 | +            } else {
 582 | +                // when doing the partial sort we include an element

instagibbs commented at 2:49 PM on June 2, 2026:

a983ac1163e6638c4f9e0271982dbe9f8c864525

Offline you said the invariant we're trying to achieve is "you're always eliminating N elements from the backlog and you're only sending elements that don't have a repeat in the backlog to peer queues".

One curious side-effect of this behavior is if we somehow have lots of high feerate, repeat duplicates (e.g., [A, A, A, A, ..., A]), we may emit nothing because of this cleanup/deferring behavior. Granted, this would be very odd / hard to achieve in practice, and we would quickly be clearing out the backlog every mempool inclusion and INVENTORY_BUCKET_CHECK_DELAY. (IIUC, the token bucket would not be draining for backlog clearing, only for things returned by ExtractBestByMiningScoreWithTopology)

The only sane alternative I think would be to drop this boundary checking entirely, relying on bloom filters to handle these repeats, with some loss in the bucket accounting exactness. I'd rather not special case this corner case either way.

As-is just including your explanation as the goal in the comment would help a ton.

in src/txmempool.cpp:572 in a983ac1163

 567 | +                res.push_back(i.value());
 568 | +            }
 569 | +        }
 570 | +        if (res.empty()) {
 571 | +            // nothing remaining in mempool
 572 | +            wtxids.clear();

instagibbs commented at 2:56 PM on June 2, 2026:

a983ac1163e6638c4f9e0271982dbe9f8c864525

unconditionally wiping wtxids above this seems to be fine and flow better to me

in src/net_processing.cpp:6033 in 1f243296a1

6052 | +                        auto itervec = m_mempool.ExtractBestByMiningScoreWithTopology(vec, vec.size());
6053 | +                        std::vector<CTransactionRef> res;
6054 | +                        res.reserve(itervec.size());
6055 | +                        for (auto txiter : itervec) {
6056 | +                            if (txiter->GetFee() < filterrate.GetFee(txiter->GetTxSize())) {
6057 | +                                continue;

instagibbs commented at 3:00 PM on June 2, 2026:

1f243296a1ea6b285ddcb079a5d5414addcb7473

reminding myself and other reviewers we do continue and not break because this is individual feerate, not chunk feerate

in src/txmempool.cpp:461 in be6d3190e6

 457 | @@ -458,7 +458,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
 458 |      assert(diagram.size() <= score_with_topo.size() + 1);
 459 |      assert(diagram.size() >= 1);
 460 |  
 461 | -    std::optional<Wtxid> last_wtxid = std::nullopt;
 462 | +    std::optional<txiter> last_iter = std::nullopt;

instagibbs commented at 3:06 PM on June 2, 2026:

be6d3190e631ebac2e7bd70750ea6f6246e0d6a6

old function name in commit message SortMiningScoreWithTopology

in src/net_processing.cpp:520 in e7777f1c8b

 515 | +     * Size bucket: Fills at 12MB every 600s, times mult so expected to be 6 times
 516 | +     *   the rate at which blocks can confirm transactions, but at least 3 times that in
 517 | +     *   the worst case. High limit to avoid triggering even with large spikes, but a
 518 | +     *   modest initial value to ensure that frequent node restarts don't raise the limit
 519 | +     *   too much.
 520 | +     * Count floor: In order to avoid resorting the global backlog too often, we ensure

instagibbs commented at 5:06 PM on June 2, 2026:

e7777f1c8bd929e4a6f6a90f61534cdb79274476

s/resorting/re-sorting/

in src/net_processing.cpp:2304 in e7777f1c8b

2300 | @@ -2239,27 +2301,101 @@ void PeerManagerImpl::SendPings()
2301 |      for(auto& it : m_peer_map) it.second->m_ping_queued = true;
2302 |  }
2303 |  
2304 | -void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wtxid)
2305 | +std::vector<Wtxid> InvToSendBucket::TakeForProcessing(CTxMemPool& mempool)

instagibbs commented at 5:12 PM on June 2, 2026:

e7777f1c8bd929e4a6f6a90f61534cdb79274476

I think this is logically equivalent and easier to read, since avail() is a superset of condition to do call ExtractBestByMiningScoreWithTopology, and we're calling it beforehand everywhere. Left the initial call to avoid taking the mempool lock but folding it in is also valid.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 05ff4b70a6..5e719c4229 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2291,86 +2291,90 @@ void PeerManagerImpl::BlockChecked(const std::shared_ptr<const CBlock>& block, c
     }
     if (it != mapBlockSource.end())
         mapBlockSource.erase(it);
 }
 
 //////////////////////////////////////////////////////////////////////////////
 //
 // Messages
 //
 
 bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash)
 {
     return m_chainman.m_blockman.LookupBlockIndex(block_hash) != nullptr;
 }
 
 void PeerManagerImpl::SendPings()
 {
     LOCK(m_peer_mutex);
     for(auto& it : m_peer_map) it.second->m_ping_queued = true;
 }
 
 std::vector<Wtxid> InvToSendBucket::TakeForProcessing(CTxMemPool& mempool)
 {
     AssertLockHeld(mempool.cs);
 
+    // We only kick off if we have count and size available and things waiting
+    Assume(avail());
+
     size_t n_to_take = static_cast<size_t>(std::max<double>(count_bucket.value() - count_floor, 0));
 
     std::vector<Wtxid> best;
 
-    if (n_to_take > 0 && !backlog.empty()) {
-        auto itervec = mempool.ExtractBestByMiningScoreWithTopology(backlog, n_to_take);
-        bool tokens_left = true;
-        for (auto txiter : itervec) {
-            auto& wtxid = txiter->GetTx().GetWitnessHash();
-            if (tokens_left) {
-                best.push_back(wtxid);
-                if (!decrement(txiter->GetTx().ComputeTotalSize())) {
-                    tokens_left = false;
-                }
-            } else {
-                backlog.push_back(wtxid);
+    auto itervec = mempool.ExtractBestByMiningScoreWithTopology(backlog, n_to_take);
+    bool tokens_left = true;
+    for (auto txiter : itervec) {
+        auto& wtxid = txiter->GetTx().GetWitnessHash();
+        if (tokens_left) {
+            best.push_back(wtxid);
+            // May go in "debt" through floor esp based on size
+            if (!decrement(txiter->GetTx().ComputeTotalSize())) {
+                tokens_left = false;
             }
+        } else {
+            backlog.push_back(wtxid);
         }
-        if (backlog.empty() && backlog.capacity() > MAX_INV_BACKLOG_RESERVE_CAPACITY) {
-            /* if backlog grew very large, free it */
-            std::vector<Wtxid>{}.swap(backlog);
-        }
+    }
+    if (backlog.empty() && backlog.capacity() > MAX_INV_BACKLOG_RESERVE_CAPACITY) {
+        /* if backlog grew very large, free it */
+        std::vector<Wtxid>{}.swap(backlog);
     }
     return best;
 }
 
 void PeerManagerImpl::ProcessInvBacklog(NodeClock::time_point now, bool backlog_bumped)
 {
     // Don't run the body of this function unless it's been a little
     // while since the last run, or we just added a new tx to the backlog.
     if (!backlog_bumped && now <= m_next_inv_bucket_check.load()) return;
     m_next_inv_bucket_check = now + INVENTORY_BUCKET_CHECK_DELAY;
 
     LOCK(m_inv_to_send_mutex);
     m_inbound_inv_bucket.increment(now);
     m_outbound_inv_bucket.increment(now);
+ 
+   // Early check to avoid taking mempool lock
     bool in_avail = m_inbound_inv_bucket.avail();
     bool out_avail = m_outbound_inv_bucket.avail();
     if (!in_avail && !out_avail) return;
 
     std::vector<Wtxid> for_inbound;
     std::vector<Wtxid> for_outbound;
 
     {
         LOCK(m_mempool.cs);
         if (in_avail) for_inbound = m_inbound_inv_bucket.TakeForProcessing(m_mempool);
         if (out_avail) for_outbound = m_outbound_inv_bucket.TakeForProcessing(m_mempool);
     }
 
     if (!for_inbound.empty() || !for_outbound.empty()) {
         bool any_inbound_connected = false;
         bool any_outbound_connected = false;
         LOCK(m_peer_mutex);
         for (auto& it : m_peer_map) {
             Peer& peer = *it.second;
             auto tx_relay = peer.GetTxRelay();
             if (!tx_relay) continue;
 
             LOCK(tx_relay->m_tx_inventory_mutex);
             // Only queue transactions for announcement once the version handshake
             // is completed. The time of arrival for these transactions is

instagibbs commented at 5:16 PM on June 2, 2026: member

getting easier and easier to understand each pass, thanks for taking all this feedback

Some tasteful debug logging would probably be good too, to at least see when we are building a backlog maybe? I'll also probably start running this soon.