private broadcast: bound rebroadcast attempts to 1,000 #35680

instagibbs commented at 8:12 PM on July 7, 2026: member

Since rebroacasts introduce additional state, bound the state growth by capping the number of rebroadcasts. With ~72 bytes per record, 10k transactions rebroadcasting for ~42 hours will result about 703 MiB allocated with overhead.

DrahtBot commented at 8:12 PM on July 7, 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/35680.

Reviews

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

Type	Reviewers
ACK	andrewtoth, frankomosh, sedited
Concept ACK	pablomartin4btc
Approach ACK	vasild
Stale ACK	optout21

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

Conflicts

No conflicts as of last run.

fanquake added the label Private Broadcast on Jul 7, 2026

instagibbs force-pushed on Jul 7, 2026

DrahtBot added the label CI failed on Jul 7, 2026

DrahtBot removed the label CI failed on Jul 7, 2026

fanquake commented at 8:56 AM on July 8, 2026: member

cc @vasild @andrewtoth

in src/rpc/mempool.cpp:165 in 85d109a3ef outdated

 159 | @@ -160,7 +160,9 @@ static RPCMethod getprivatebroadcastinfo()
 160 |                                  {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
 161 |                                  {RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded transaction data"},
 162 |                                  {RPCResult::Type::NUM_TIME, "time_added", "The time this transaction was added to the private broadcast queue (seconds since epoch)"},
 163 | -                                {RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction",
 164 | +                                {RPCResult::Type::NUM, "num_sent", "Total number of peers this transaction was sent to so far"},
 165 | +                                {RPCResult::Type::NUM, "num_acknowledged", "Total number of peers that acknowledged reception of this transaction"},
 166 | +                                {RPCResult::Type::ARR, "peers", "Per-peer send and acknowledgment information for this transaction, for currently connected peers only",

andrewtoth commented at 1:46 PM on July 8, 2026:

If we make this change, is it worth reporting this? It will only have a value for max 3 minutes, and the received field will only ever have a value for a tiny amount of time that will require racing between receiving the PONG and disconnecting.

If we don't think we should store this data for stats a la #34707, we should remove these fields IMO. If we think they're valuable, we should instead cap this to last ~3 peers instead and just truncate the vector as we make more connections.

instagibbs commented at 5:26 PM on July 8, 2026:

A limited buffer of disconnected peer stats, in addition to all the live ones, could make sense, if that's what you're suggesting

instagibbs commented at 3:28 PM on August 3, 2026:

Some thoughts:

You'll definitely see "stuck" in the rpc responses as-is in this PR, and for successful ones the aggregates are returned
I feel like bolting on a buffer now is something that will be revisited again in #34707 and hopefully less ad hoc then here.

I'm leaning on the side of "as is" so we can do it more holistically later.

andrewtoth commented at 3:52 PM on August 3, 2026:

A "stuck" tx will generally be stuck because it's struggling to connect. Once connected, the peer is alive for 3 minutes max. So, if we don't want a buffer of peers that have since disconnected, it would make sense to remove this field. I think it might be confusing for consumers otherwise, since they would write software to expect values there but generally there will be nothing returned.

I still think having these be reported in #34707 would have some value. Maybe not the specific address of the peer, but definitely which network we connected to and the timestamps. I'm not sure though how much complexity that adds to this change here. But, if we were to add that in #34707, then removing this field would have some wasted churn.

instagibbs commented at 4:02 PM on August 3, 2026:

The "peers" result is already being changed in this PR IIRC, so I think it's probably fine to break it more, then revisit holistically in #34707 ?

pablomartin4btc commented at 5:07 PM on August 3, 2026:

... we should instead cap this to last ~3 peers instead and just truncate the vector as we make more connections... " @andrewtoth, is this why you suggested ~3 — mapping to NUM_PRIVATE_BROADCAST_PER_TX?

If I got it correctly... if we do add the buffer in #34707, the struct would also need a status field to distinguish live/timed-out/confirmed, otherwise the buffer entries would be ambiguous (?).

andrewtoth commented at 3:30 AM on August 7, 2026:

@pablomartin4btc I guess I suggested 3 because most txs will be received back in under 3. I don't think it matters anymore though since we are going in a different direction.

Yes, in #34707 we will add a received_by field that will indicate the tx was received back and is no longer broadcasting.

maflcko removed the label Private Broadcast on Jul 10, 2026

DrahtBot added the label Private Broadcast on Jul 10, 2026

optout21 commented at 12:17 PM on July 14, 2026: contributor

Concept ACK Made a light preliminary review (85d109a3ef8c42cb19075cd2b45a4daff072a711), LGTM. I note that unfortunately this PR has a significant overlap and strong conflict with #35016. I'm not sure what to recommend, since at this moment is hard to predict how they will land.

instagibbs commented at 1:12 PM on July 14, 2026: member

@optout21 wasn't aware of it, will review

optout21 commented at 1:25 PM on July 14, 2026: contributor

wasn't aware of it, will review

👍 #35016 is no-behavior-change refactor, relatively non-contentious, however the PR moves a bit slowly. If it would be possible to change this PR to be closer to the approach there, that would be beneficial, but I'm not sure how the per-node map added here combines with the changes there.

instagibbs commented at 2:25 PM on July 14, 2026: member

From a glance, I'm not sure how that would work, they seem to be opposite tactics entirely? If the refactor was merged first, I'd have to undo most of it to reintroduce the wrapper types. Willing to be wrong, but I think we should pick a way forward and commit on strategy first.

optout21 commented at 2:40 PM on July 14, 2026: contributor

CC: @vasild @andrewtoth

DrahtBot added the label Needs rebase on Jul 14, 2026

andrewtoth commented at 6:11 PM on August 1, 2026: contributor

Concept ACK

fanquake commented at 2:21 PM on August 3, 2026: member

Waiting for #35090 to be merged

This has been merged.

instagibbs force-pushed on Aug 3, 2026

instagibbs commented at 3:27 PM on August 3, 2026: member

rebased and added another unit test covering more tie-breaking behavior that is missing since I'm touching how that's derived in this PR

DrahtBot removed the label Needs rebase on Aug 3, 2026

in test/functional/p2p_private_broadcast.py:233 in 879ee85f0b

 232 | -        assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), broadcasts_to_expect)
 233 | +        assert_greater_than_or_equal(pending[0]["num_sent"], NUM_PRIVATE_BROADCAST_PER_TX)
 234 | +        assert_greater_than_or_equal(pending[0]["num_acknowledged"], broadcasts_to_expect)
 235 | +        # The peers list only contains currently connected peers; the ones
 236 | +        # observed above have already disconnected.
 237 | +        assert all("address" in p and "sent" in p for p in pending[0]["peers"])

pablomartin4btc commented at 5:39 PM on August 3, 2026:

nit: the assert all("address" in p and "sent" in p for p in pending[0]["peers"]) wouldn't be true when peers is empty? — which per the comment above is the expected state at this point. If so assert_equal(pending[0]["peers"], []) would make the expectation explicit.

pablomartin4btc commented at 5:50 PM on August 3, 2026: member

Concept ACK

I see the old design had an unbounded growth (each failed send appended permanently to the tx record), new design caps it cleanly at O(queued txs + live connections) at the cost of losing per-peer history after disconnect (the audit trail of failed attempts is available in the debug log — not structured or queryable, but it's there; anyone debugging "why isn't my tx propagating?" can use -debug=privbroadcast and get the full picture). The tradeoff I think is justified because the historical data perhaps had limited value and the memory concern is real even if rare.

This also changes getprivatebroadcastinfo output: peers now only contains currently-connected peers (almost always empty in practice - unless tx is stuck due to connectivity?) and two new cumulative fields num_sent/num_acknowledged carry the historical signal instead.

Note: it seems the unbounded growth wasn't flagged during #34329 review — this appears to be a fresh catch, likely surfaced while checking #35090 fuzz harness (?).

instagibbs commented at 5:57 PM on August 3, 2026: member

Note: it seems the unbounded growth wasn't flagged during #34329 review — this appears to be a fresh catch, likely surfaced while checking #35090 fuzz harness (?).

It was discovered while designing #35406 fwiw

vasild commented at 6:03 PM on August 3, 2026: contributor

#35406 (merged) already limits the number of transactions in the queue.

I have been using #34707 for a while now and I find the displayed information in getprivatebroadcastinfo very useful. This PR would cripple that by dropping some of the information.

The stats per-peer-to-whom-we-sent-the-tx are just a few dozens of bytes. I don't see that ever becoming a problem of using too much memory. Like, are we going to send one transaction to millions of peers? We only have 70k potential peers in addrman. If we send a single transaction to all of them (!?) then, remembering the address, port and two timestamps would be about 2MB of memory.

This PR adds further code and complexity.

Given the above, Concept NACK.

instagibbs commented at 6:32 PM on August 3, 2026: member

I have been using #34707 for a while now and I find the displayed information in getprivatebroadcastinfo very useful. This PR would cripple that by dropping some of the information.

Ideally we could agree on what the drafted PR could look like in a world of bounded memory usage. I've been going back and forth with @andrewtoth on keeping some bounded history in a buffer inside this very PR to keep observability reasonable: #35680 (review) vs looking forward to #34707 as a more complete answer. I'd accept either direction. I don't think we should leave memory usage unbounded to unblock a draft PR.

The stats per-peer-to-whom-we-sent-the-tx are just a few dozens of bytes. I don't see that ever becoming a problem of using too much memory. Like, are we going to send one transaction to millions of peers?

I think it's bad engineering to choose unbounded memory usage even if you even I find usage unlikely, unless there's a compelling reason not to. I see no need for it.

This PR adds further code and complexity.

The PR as-id results in less code, unless we are counting tests. Bounded buffer may add a few lines. We can argue about complexity but I think this is quite tame for the payoff of reasoning about invariants.

andrewtoth commented at 4:18 AM on August 4, 2026: contributor

If we send a single transaction to all of them (!?) then, remembering the address, port and two timestamps would be about 2MB of memory.

I think it's bad engineering to choose unbounded memory usage even if you even I find usage unlikely

Would an alternative, much simpler approach of just stopping broadcast if we reach 1000 attempted peers be acceptable? If we are successfully connecting every ~2.5 minutes that's about 42 hours of broadcasting non-stop and not getting the tx back. I think that bounds the memory usage (~80kb per tx => ~800MB worst case if all 10k txs fail with 1000 tries), and also bounds the infinite retry loop.

instagibbs commented at 11:09 AM on August 4, 2026: member

@andrewtoth not sure why I didn't think of this one. It can be targeting X hours before giving up, at which point the user will have to intervene anyways. We don't get the other efficiencies but those were just a side-benefit. I'll work on this.

instagibbs force-pushed on Aug 4, 2026

instagibbs renamed this:
~~private broadcast: bound memory use of broadcast attempts~~
private broadcast: bound rebroadcast attempts to 1,000
on Aug 4, 2026

in doc/release-notes-35680.md:4 in 431f9199cb

   0 | @@ -0,0 +1,13 @@
   1 | +P2P and network changes
   2 | +-----------------------
   3 | +
   4 | +- Memory usage of private broadcast (`-privatebroadcast`) is now bounded by

andrewtoth commented at 2:03 AM on August 5, 2026:

I'm not sure "Memory usage" here is what users reading the release notes will care about? Maybe lead with "Private broadcast transactions are now limited to 1,000 attempts, after which they will stop broadcasting. They can be retried by calling sendrawtransaction with the transaction again."?

vasild commented at 7:34 AM on August 7, 2026:

"Private broadcast transactions are now limited to 1,000 attempts..." sounds to me like 1000 cumulative across all transactions. Maybe "Each transaction is now limited to 1000 send attempts..."

instagibbs commented at 11:31 AM on August 7, 2026:

lmk what you think

in src/rpc/mempool.cpp:199 in 431f9199cb

 195 | @@ -193,6 +196,10 @@ static RPCMethod getprivatebroadcastinfo()
 196 |                  o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
 197 |                  o.pushKV("hex", EncodeHexTx(*tx_info.tx));
 198 |                  o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
 199 | +                const size_t attempts_remaining{tx_info.peers.size() < PrivateBroadcast::MAX_SEND_ATTEMPTS

andrewtoth commented at 2:09 AM on August 5, 2026:

Maybe simpler 1 line

                const size_t attempts_remaining{PrivateBroadcast::MAX_SEND_ATTEMPTS - std::min(tx_info.peers.size(), PrivateBroadcast::MAX_SEND_ATTEMPTS)};

instagibbs commented at 11:31 AM on August 7, 2026:

taken

in src/private_broadcast.cpp:50 in 431f9199cb

  45 | @@ -44,14 +46,22 @@ std::optional<CTransactionRef> PrivateBroadcast::PickTxForSend(const NodeId& wil
  46 |          return std::nullopt;
  47 |      }
  48 |  
  49 | +    auto with_attempts_remaining{m_transactions | std::views::filter([](const auto& entry) {
  50 | +        return entry.second.send_statuses.size() < MAX_SEND_ATTEMPTS;

andrewtoth commented at 3:13 AM on August 7, 2026:

Can we extract this predicate to a bool IsPending(const TxBroadcastStatus& status) const method and use it wherever we check for send attempts being exceeded? This will make rebasing #34707 much simpler.

vasild commented at 7:41 AM on August 7, 2026:

bool IsPending(const TxBroadcastStatus& status) const
{
    return status.send_statuses.size() < MAX_SEND_ATTEMPTS;
}

auto with_attempts_remaining{m_transactions | std::views::filter(IsPending)};

bool PrivateBroadcast::HavePendingTransactions()
{                 
    LOCK(m_mutex);
    return std::ranges::any_of(m_transactions, IsPending);
}

or

auto GetPendingTransactions() const
{
    return m_transactions | std::views::filter([](const auto& entry) {
        return entry.second.send_statuses.size() < MAX_SEND_ATTEMPTS;
    };
}

auto with_attempts_remaining{GetPendingTransactions()};

bool PrivateBroadcast::HavePendingTransactions()
{                 
    LOCK(m_mutex);
    return !GetPendingTransactions.empty();
}

?

PS: the former can be reused in the fuzz test as well

instagibbs commented at 11:31 AM on August 7, 2026:

made a IsPending helper and used in various spots

in src/private_broadcast.cpp:19 in 431f9199cb

andrewtoth commented at 3:18 AM on August 7, 2026:

Can we check here if the tx is pending, and if so reset time_added and clear send_statuses before returning Added? I think it makes sense to reset if a caller adds again, instead of forcing a user to have to abort the tx first. This will also make it easier to rebase #34707.

vasild commented at 7:41 AM on August 7, 2026:

Agree.

instagibbs commented at 11:31 AM on August 7, 2026:

done

in src/private_broadcast.h:47 in 431f9199cb outdated

  41 | @@ -42,6 +42,10 @@ class PrivateBroadcast
  42 |      /// Additions that would exceed this are rejected (see Add()).
  43 |      static constexpr size_t MAX_TRANSACTIONS{10'000};
  44 |  
  45 | +    /// Maximum number of send attempts for a transaction. Once this limit is
  46 | +    /// reached, the transaction remains tracked but is not sent again.
  47 | +    static constexpr size_t MAX_SEND_ATTEMPTS{1'000};

andrewtoth commented at 3:20 AM on August 7, 2026:

Can we make this injectable so we can hit this more easily in fuzz and unit tests?

instagibbs commented at 11:31 AM on August 7, 2026:

done

in src/test/fuzz/private_broadcast.cpp:145 in 431f9199cb

 140 | @@ -136,7 +141,9 @@ FUZZ_TARGET(private_broadcast)
 141 |                      const auto& [_, inserted]{nodes_sent_to.emplace(will_send_to_nodeid)};
 142 |                      Assert(inserted);
 143 |                  } else {
 144 | -                    Assert(transactions.empty());
 145 | +                    Assert(std::ranges::none_of(transactions, [](const auto& entry) {
 146 | +                        return entry.second < PrivateBroadcast::MAX_SEND_ATTEMPTS;

andrewtoth commented at 3:21 AM on August 7, 2026:

We can pull this predicate out into an is_pending lambda in the fuzz test as well, and use it in all the asserts.

instagibbs commented at 11:31 AM on August 7, 2026:

done

andrewtoth changes_requested

andrewtoth commented at 3:27 AM on August 7, 2026: contributor

Thanks @instagibbs this looks better and won't conflict with the approach in #34707.

I think we should reset the tx if it uses all its attempts when adding again, instead of requiring it be aborted first.

I left a few other comments that will make it even simpler to rebase #34707. @vasild wdyt of this new approach?

in src/test/fuzz/private_broadcast.cpp:188 in 431f9199cb

 188 | -                    Assert(transactions.empty());
 189 | -                }
 190 | +                const bool expected{std::ranges::any_of(transactions, [](const auto& entry) {
 191 | +                    return entry.second < PrivateBroadcast::MAX_SEND_ATTEMPTS;
 192 | +                })};
 193 | +                Assert(pb.HavePendingTransactions() == expected);

vasild commented at 7:52 AM on August 7, 2026:

If this assert fails, then it will not be immediately obvious from the message which one was true and which one was false. The following would be slightly more easier to analyze:

if (std::ranges::any_of(transactions, [](const auto& entry) { return entry.second < PrivateBroadcast::MAX_SEND_ATTEMPTS; }) {
    Assert(pb.HavePendingTransactions());
} else {
    Assert(!pb.HavePendingTransactions());
}

instagibbs commented at 11:31 AM on August 7, 2026:

taken

in src/test/fuzz/private_broadcast.cpp:198 in 431f9199cb

 199 |  
 200 |                  for (const auto& stale_tx : stale) {
 201 | -                    Assert(transactions.contains(stale_tx));
 202 | +                    const auto it{transactions.find(stale_tx)};
 203 | +                    Assert(it != transactions.end());
 204 | +                    Assert(it->second < PrivateBroadcast::MAX_SEND_ATTEMPTS);

vasild commented at 7:54 AM on August 7, 2026:

                    Assert(it->second <= PrivateBroadcast::MAX_SEND_ATTEMPTS);

instagibbs commented at 11:31 AM on August 7, 2026:

I don't believe that's correct, replaced with IsPending check which is equivalent and clearer

in src/test/private_broadcast_tests.cpp:185 in 431f9199cb

 180 | +    in_addr ipv4_addr;
 181 | +    ipv4_addr.s_addr = 0xa0b0c001;
 182 | +    const CService address{ipv4_addr, 1111};
 183 | +
 184 | +    for (size_t attempt{0}; attempt < PrivateBroadcast::MAX_SEND_ATTEMPTS; ++attempt) {
 185 | +        BOOST_REQUIRE_EQUAL(pb.PickTxForSend(/*will_send_to_nodeid=*/static_cast<NodeId>(attempt), address).value(), tx);

vasild commented at 7:57 AM on August 7, 2026:

Can add:

BOOST_CHECK(pb.HavePendingTransactions());

before the PickTxForSend() call.

instagibbs commented at 11:31 AM on August 7, 2026:

done

in src/test/private_broadcast_tests.cpp:203 in 431f9199cb

 198 | +
 199 | +    // An exhausted transaction does not prevent another transaction from being sent.
 200 | +    const auto next_tx{MakeDummyTx(/*id=*/2, /*num_witness=*/0)};
 201 | +    BOOST_REQUIRE_EQUAL(pb.Add(next_tx), PrivateBroadcast::AddResult::Added);
 202 | +    BOOST_CHECK(pb.HavePendingTransactions());
 203 | +    BOOST_REQUIRE_EQUAL(pb.PickTxForSend(/*will_send_to_nodeid=*/static_cast<NodeId>(PrivateBroadcast::MAX_SEND_ATTEMPTS + 1), address).value(), next_tx);

vasild commented at 8:02 AM on August 7, 2026:

This is a bit fragile as it silently requires that previously we have called PickTxForSend() exactly PrivateBroadcast::MAX_SEND_ATTEMPTS times. It would be more robust to use:

NodeId node_id{0};

... PickTxForSend(node_id++, ...); ...

instagibbs commented at 11:31 AM on August 7, 2026:

done

vasild commented at 8:04 AM on August 7, 2026: contributor

Approach ACK 431f9199cb5b145e920350aa854fbf5ef2ae8bf1

optout21 commented at 10:03 AM on August 7, 2026: contributor

ACK 431f9199cb5b145e920350aa854fbf5ef2ae8bf1 LGTM after code review & local tests. Much simpler than previous approach.

DrahtBot requested review from pablomartin4btc on Aug 7, 2026

DrahtBot requested review from vasild on Aug 7, 2026

DrahtBot requested review from andrewtoth on Aug 7, 2026

fanquake added this to the milestone 32.0 on Aug 7, 2026

instagibbs force-pushed on Aug 7, 2026

instagibbs commented at 11:33 AM on August 7, 2026: member

all feedback should be addressed, major change is allowing re-submission to reset the state to allow further attempts

DrahtBot added the label CI failed on Aug 7, 2026

DrahtBot commented at 12:54 PM on August 7, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/31174405280/job/92853099001</sub> <sub>LLM reason (✨ experimental): CI failed due to a fuzz test crash in private_broadcast (assertion confirmed failed in src/test/fuzz/private_broadcast.cpp:198).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

instagibbs force-pushed on Aug 7, 2026

instagibbs commented at 1:31 PM on August 7, 2026: member

Fuzz harness bug only due to new behavior not matching. If wtxid matches, it's the same tx and should re-set exhausted item.

DrahtBot removed the label CI failed on Aug 7, 2026

in src/private_broadcast.cpp:54 in 5ac9c635d7

  50 | @@ -44,12 +51,13 @@ std::optional<CTransactionRef> PrivateBroadcast::PickTxForSend(const NodeId& wil
  51 |          return std::nullopt;
  52 |      }
  53 |  
  54 | +    auto with_attempts_remaining{m_transactions | std::views::filter([this](const auto& entry) { return IsPending(entry.second); })};

andrewtoth commented at 4:59 PM on August 7, 2026:

If pushing again, we should call this pending_transactions instead.

    auto pending_transactions{m_transactions | std::views::filter([this](const auto& entry) { return IsPending(entry.second); })};

instagibbs commented at 4:53 PM on August 8, 2026:

will do if I touch

in src/test/fuzz/private_broadcast.cpp:153 in 5ac9c635d7

 149 | @@ -126,17 +150,18 @@ FUZZ_TARGET(private_broadcast)
 150 |                      // (fewest sends = highest priority), so PickTxForSend() must return a transaction
 151 |                      // with the minimum send count of any in the queue. Ties are broken by state we
 152 |                      // don't model, so only check this key.
 153 | +                    auto with_attempts_remaining{transactions | std::views::filter(is_pending)};

andrewtoth commented at 9:01 PM on August 7, 2026:

Same, if retouching

                    auto pending_transactions{transactions | std::views::filter(is_pending)};

instagibbs commented at 4:53 PM on August 8, 2026:

will do if I touch

andrewtoth commented at 9:05 PM on August 7, 2026: contributor

ACK 5ac9c635d70e0ae2f2234531820712267fc531d0

in src/private_broadcast.cpp:115 in 5ac9c635d7 outdated

 111 | @@ -104,6 +112,7 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
 112 |      const auto now{NodeClock::now()};
 113 |      std::vector<CTransactionRef> stale;
 114 |      for (const auto& [tx, state] : m_transactions) {
 115 | +        if (!IsPending(state)) continue;

frankomosh commented at 8:31 AM on August 14, 2026:

I don’t think exhausted transactions ever get cleaned up? If that's so then over a long enough uptime, especially for something like an exchange or wallet service broadcasting a lot, some transactions will genuinely never confirm (double-spent, fee too low, whatever). If enough of those pile up and new, legitimate broadcasts start getting rejected as "queue full," even though most of what's filling it up is doing nothing.

To be precise: not forever, since this is all in-memory and a restart clears it. But I doubt production nodes restart casually, and losing the whole pending queue doesn’t look like a great fix either. So in practice: the queue might be slowly clogs over the node's uptime, and the only way out is manually calling abortprivatebroadcast (repeatedly) or restarting.

Wrote the test below to try and simulate this situation.

@@ -299,4 +299,39 @@
     BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap);
 }

+BOOST_AUTO_TEST_CASE(stale_pending_tx_found_alongside_exhausted)
+{
+    // GetStale() must find every genuinely stale, pending transaction even
+    // when an exhausted transaction is also present and may be iterated
+    // first (unordered_map iteration order is unspecified). Using many
+    // pending-stale transactions against a single exhausted one, and
+    // requiring ALL of them to be found.
+    FakeNodeClock clock{};
+
+    constexpr size_t cap{1};
+    PrivateBroadcast pb{PrivateBroadcast::MAX_TRANSACTIONS, cap};
+
+    std::vector<CTransactionRef> pending_txs;
+    for (uint32_t i{0}; i < 20; ++i) {
+        auto tx{MakeDummyTx(i, /*num_witness=*/0)};
+        BOOST_REQUIRE_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::Added);
+        pending_txs.push_back(tx);
+    }
+
+    const auto exhausted_tx{MakeDummyTx(/*id=*/100, /*num_witness=*/0)};
+    BOOST_REQUIRE_EQUAL(pb.Add(exhausted_tx), PrivateBroadcast::AddResult::Added);
+    BOOST_REQUIRE(pb.PickTxForSend(/*will_send_to_nodeid=*/0, CService{}).has_value());
+
+    clock += PrivateBroadcast::INITIAL_STALE_DURATION + 1min;
+
+    const auto stale{pb.GetStale()};
+    BOOST_TEST_MESSAGE("Stale candidates found: " << stale.size() << " (expected " << pending_txs.size() << ")");
+    for (const auto& tx : pending_txs) {
+        BOOST_CHECK_MESSAGE(std::ranges::find(stale, tx) != stale.end(),
+                            "pending tx missing from stale results");
+    }
+    BOOST_CHECK_EQUAL(stale.size(), pending_txs.size());
+}
+
 BOOST_AUTO_TEST_SUITE_END()

</details>

andrewtoth commented at 4:33 PM on August 14, 2026:

some transactions will genuinely never confirm (double-spent, fee too low, whatever)

The private broadcast queue removes txs when they are returned to us from the network. It doesn't monitor confirmations. So as long as these txs are returned from the network before 1000 attempts to broadcast the tx, the tx will be removed. Further, before exhaustion every tx is checked before an attempt is made whether it is still acceptable to our mempool. So, a double-spent tx or a tx with a fee that is now too low for our mempool will also be removed from the queue.

instagibbs commented at 8:46 PM on August 14, 2026:

Like before, transactions only get cleaned up on success or abandonment.

but I doubt production nodes restart casually

ime that's not true, they are well-suited to restart on any turbulence like crashes

frankomosh commented at 9:54 AM on August 17, 2026:

The private broadcast queue removes txs when they are returned to us from the network. It doesn't monitor confirmations. So as long as these txs are returned from the network before 1000 attempts to broadcast the tx, the tx will be removed. Further, before exhaustion every tx is checked before an attempt is made whether it is still acceptable to our mempool. So, a double-spent tx or a tx with a fee that is now too low for our mempool will also be removed from the queue.

Didn't realise it got cleaned up that reliably. Good to know, and thanks for taking me through that.

in src/private_broadcast.cpp:22 in 5ac9c635d7 outdated

  19 | +    if (const auto it{m_transactions.find(tx)}; it != m_transactions.end()) {
  20 | +        if (IsPending(it->second)) return AddResult::AlreadyPresent;
  21 | +
  22 | +        // An exhausted transaction can be explicitly retried by adding it again.
  23 | +        it->second.time_added = NodeClock::now();
  24 | +        it->second.send_statuses.clear();

frankomosh commented at 9:02 AM on August 14, 2026:

This send_statuses.clear wipes all sent records, including one for a connection that might still be open and has not disconnected yet. If that peer's confirmation lands right after the reset, it could be silently lost and therefore nothing left to update it against. FinalizeNode() (untouched by this PR) checks DidNodeConfirmReception(), finds nothing, and opens an extra connection it did not need. The full effect therefore could be one wasted connection. Fuzz harness private_broadcast.cpp models this on purpose, so I take it as an intentional design choice?,

@@ -299,4 +299,38 @@
     BOOST_CHECK_EQUAL(pb.GetBroadcastInfo().size(), num_cap);
 }

+BOOST_AUTO_TEST_CASE(reset_silently_drops_live_confirmation)
+{
+    // A transaction can have multiple concurrent in-flight sends (up to
+    // NUM_PRIVATE_BROADCAST_PER_TX in production). If it becomes exhausted
+    // and is reset via Add() while one of those connections is still
+    // genuinely open, the reset's send_statuses.clear() wipes that live
+    // connection's record too.
+    constexpr size_t cap{2};
+    PrivateBroadcast pb{PrivateBroadcast::MAX_TRANSACTIONS, cap};
+    const auto tx{MakeDummyTx(/*id=*/1, /*num_witness=*/0)};
+    BOOST_REQUIRE_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::Added);
+
+    const NodeId node0{0};
+    const NodeId node1{1};
+    BOOST_REQUIRE_EQUAL(pb.PickTxForSend(node0, CService{}).value(), tx);
+    BOOST_REQUIRE_EQUAL(pb.PickTxForSend(node1, CService{}).value(), tx);
+
+    // Tx is now exhausted; both connections are still genuinely open.
+    BOOST_CHECK(!pb.HavePendingTransactions());
+    BOOST_CHECK(pb.GetTxForNode(node0).has_value());
+
+    // Caller resubmits (e.g. via sendrawtransaction), resetting the tx
+    // while node0's connection is still live and unconfirmed.
+    BOOST_REQUIRE_EQUAL(pb.Add(tx), PrivateBroadcast::AddResult::Added);
+
+    // node0's real, still-open connection is now untracked.
+    BOOST_CHECK(!pb.GetTxForNode(node0).has_value());
+
+    // If node0 now sends back a genuine confirmation, it is silently lost.
+    pb.NodeConfirmedReception(node0);
+    BOOST_CHECK(!pb.DidNodeConfirmReception(node0));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

andrewtoth commented at 4:37 PM on August 14, 2026:

This is a very narrow edge case. On the 1000th attempt that is still in flight (for 3 minutes max), the user resends the tx before the peer sends the pong back. I don't think it's worth hardening against, considering worst case it's a single extra connection that is made in vain.

instagibbs commented at 8:47 PM on August 14, 2026:

I don't think it's worth hardening, unless there is a clear correctness/privacy issue

in src/rpc/mempool.cpp:200 in 5ac9c635d7

 196 | @@ -193,6 +197,8 @@ static RPCMethod getprivatebroadcastinfo()
 197 |                  o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
 198 |                  o.pushKV("hex", EncodeHexTx(*tx_info.tx));
 199 |                  o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
 200 | +                const size_t attempts_remaining{PrivateBroadcast::MAX_SEND_ATTEMPTS - std::min(tx_info.peers.size(), PrivateBroadcast::MAX_SEND_ATTEMPTS)};

frankomosh commented at 9:14 AM on August 14, 2026:

This reads the static MAX_SEND_ATTEMPTS constant and not the real per-instance cap the constructor takes (m_max_send_attempts, added specifically so tests can use a smaller value). Completely harmless today since production always runs with the default and the two happen to match. But the moment this cap becomes configurable, then getprivatebroadcastinfo would report the wrong number of attempts left, which could mislead an operator into thinking a stuck tx is still being retried.

I tested this directly with a custom cap of 5: after exhausting a tx, this line reports 995 remaining instead of the real 0.

Perhaps a suggestion could be to compute this inside GetBroadcastInfo(), where the real cap already lives, and expose it as a field on TxBroadcastInfo ?.. Tried this and confirmed it reports correctly (0, not 995) for the same case:

<details> <summary>a suggestion</summary>

--- a/src/private_broadcast.h
+++ b/src/private_broadcast.h
@@ -64,6 +64,7 @@
     struct TxBroadcastInfo {
         CTransactionRef tx;
         NodeClock::time_point time_added;
+        size_t attempts_remaining;
         std::vector<PeerSendInfo> peers;
     };

--- a/src/private_broadcast.cpp
+++ b/src/private_broadcast.cpp
@@ -136,7 +136,8 @@
         for (const auto& status : state.send_statuses) {
             peers.emplace_back(PeerSendInfo{.address = status.address, .sent = status.picked, .received = status.confirmed});
         }
-        entries.emplace_back(TxBroadcastInfo{.tx = tx, .time_added = state.time_added, .peers = std::move(peers)});
+        const size_t attempts_remaining{m_max_send_attempts - std::min(state.send_statuses.size(), m_max_send_attempts)};
+        entries.emplace_back(TxBroadcastInfo{.tx = tx, .time_added = state.time_added, .attempts_remaining = attempts_remaining, .peers = std::move(peers)});
     }

     return entries;

--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -197,8 +197,7 @@
                 o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
                 o.pushKV("hex", EncodeHexTx(*tx_info.tx));
                 o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
-                const size_t attempts_remaining{PrivateBroadcast::MAX_SEND_ATTEMPTS - std::min(tx_info.peers.size(), PrivateBroadcast::MAX_SEND_ATTEMPTS)};
-                o.pushKV("attempts_remaining", attempts_remaining);
+                o.pushKV("attempts_remaining", tx_info.attempts_remaining);
                 UniValue peers(UniValue::VARR);
                 for (const auto& peer : tx_info.peers) {
                     UniValue p(UniValue::VOBJ);

instagibbs commented at 9:09 PM on August 14, 2026:

taken

in src/rpc/mempool.cpp:203 in 5ac9c635d7 outdated

 196 | @@ -193,6 +197,8 @@ static RPCMethod getprivatebroadcastinfo()
 197 |                  o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
 198 |                  o.pushKV("hex", EncodeHexTx(*tx_info.tx));
 199 |                  o.pushKV("time_added", TicksSinceEpoch<std::chrono::seconds>(tx_info.time_added));
 200 | +                const size_t attempts_remaining{PrivateBroadcast::MAX_SEND_ATTEMPTS - std::min(tx_info.peers.size(), PrivateBroadcast::MAX_SEND_ATTEMPTS)};
 201 | +                o.pushKV("attempts_remaining", attempts_remaining);
 202 |                  UniValue peers(UniValue::VARR);
 203 |                  for (const auto& peer : tx_info.peers) {

frankomosh commented at 9:49 AM on August 14, 2026:

Side note. Worst case works out to about 1.15 GiB of JSON in a single getprivatebroadcastinfo call; measured this now that the 1,000 cap makes it possible to put a number on it. That's roughly the same size as the ~703 MiB already quoted for memory in the PR description. Isn't 1.15 GiB a lot to return from one RPC call?

instagibbs commented at 8:44 PM on August 14, 2026:

It's a lot, but it was literally unbounded before. Maybe revisit in #34707 ?

frankomosh commented at 9:44 AM on August 17, 2026:

It's a lot, but it was literally unbounded before. Maybe revisit in #34707 ?

Thanks. Will do!

frankomosh commented at 10:08 AM on August 14, 2026: contributor

Tested ACK 5ac9c635d70e0ae2f2234531820712267fc531d0. The approach is similar to what I first had in mind when the issue surfaced in #35406.

Most of my inline comments are nits/inquiries, I don’t think they are blocking to this PR.

private broadcast: bound broadcast attempts per tx to 1k

Rather than rebroadcasting forever, bound attempts at
private broadcast, report remaining attempts over RPC
results, and allow exhausted transactions to be
retried when submitted.

fe7d475d45

instagibbs force-pushed on Aug 14, 2026

instagibbs commented at 9:10 PM on August 14, 2026: member

All comments addressed, touched up some older comments by @andrewtoth

andrewtoth commented at 6:42 PM on August 15, 2026: contributor

ACK fe7d475d450b9aabd549627c6e61024ae45e7a8c

DrahtBot requested review from frankomosh on Aug 15, 2026

frankomosh commented at 9:45 AM on August 17, 2026: contributor

ReACK fe7d475d450b9aabd549627c6e61024ae45e7a8c

sedited approved

sedited commented at 12:49 PM on August 18, 2026: contributor

ACK fe7d475d450b9aabd549627c6e61024ae45e7a8c

sedited merged this on Aug 18, 2026

sedited closed this on Aug 18, 2026