net: keep finished private broadcast txs in memory

andrewtoth commented at 1:46 am on March 2, 2026: contributor

Follow up from #34329.

Private broadcast transaction data is removed from memory when either 1) the tx is received back from another peer and inserted into our mempool or 2) the tx is no longer acceptable to our mempool.

We want to persist this data for a number of reasons. See #30471.

This PR changes the behavior of the private broadcast queue to keep received txs in memory. The peer’s address we received the tx from and the time is now also returned via getprivatebroadcastinfo RPC.

Keeping mempool rejected txs in memory is left for a follow-up.

This is designed to facilitate follow up work including:

rebroadcasting txs that are in our mempool but have not been confirmed after a while. This can obviate rebroadcasting wallet txs if private broadcast is enabled (see #34533 (comment)).
adding a delay when submitting private broadcast txs so they will be broadcast at a later time (see #34118).
persisting private broadcast state to disk for resuming after restart or crash (see #34322).
expiring txs after a while so they are removed from the queue without having to manually abort.

DrahtBot added the label P2P on Mar 2, 2026

DrahtBot commented at 1:47 am on March 2, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	optout21

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34873 (net: fix premature stale flagging of unpicked private broadcast txs by Mccalabrese)

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.

andrewtoth force-pushed on Mar 2, 2026

DrahtBot added the label CI failed on Mar 2, 2026

DrahtBot commented at 2:04 am on March 2, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/22558096885/job/65339168667 LLM reason (✨ experimental): private_broadcast_tests failed with an assertion error in netaddress.cpp (ToStringAddr), causing the test subprocess to abort.

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.

DrahtBot removed the label CI failed on Mar 2, 2026

in src/net_processing.cpp:4498 in fa857a980c

4493@@ -4491,10 +4494,10 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4494         const uint256& hash = peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256();
4495         AddKnownTx(peer, hash);
4496 
4497-        if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
4498-            LogDebug(BCLog::PRIVBROADCAST, "Received our privately broadcast transaction (txid=%s) from the "
4499+        if (const auto num_broadcasted{m_tx_for_private_broadcast.Received(ptx, CService{pfrom.addr})}) {
4500+        LogDebug(BCLog::PRIVBROADCAST, "Received our privately broadcast transaction (txid=%s) from the "

optout21 commented at 10:37 am on March 6, 2026:

nit: indenting is off

in src/private_broadcast.cpp:50 in fa857a980c

45+    const auto it{m_transactions.find(tx)};
46+    if (it == m_transactions.end()) return std::nullopt;
47+    if (it->second.received_from.has_value()) return std::nullopt;
48+    it->second.received_from.emplace(received_from);
49+    it->second.received_time.emplace(NodeClock::now());
50+    it->second.mempool_rejection_reason.emplace("missing-inputs");

optout21 commented at 10:40 am on March 6, 2026:

What’s the reason for using “missing-inputs” here?

andrewtoth commented at 1:27 am on March 11, 2026:

Removed this line.

in src/private_broadcast.cpp:62 in fa857a980c

57+{
58+    LOCK(m_mutex);
59+    const auto it{m_transactions.find(tx)};
60+    if (it == m_transactions.end()) return;
61+    it->second.mempool_rejection_reason = std::move(rejection_reason);
62+    if (!it->second.mempool_rejection_reason.has_value()) {

optout21 commented at 10:44 am on March 6, 2026:

I find the dependency between rejection_reason and received_from confusing here. If the TX can be accepted in the mempool, the received from info is cleared. Could you elaborate on the why?

Update: I found it described in the header comment, but I still don’t understand fully (may be my problem).

andrewtoth commented at 1:31 am on March 11, 2026:

I updated this logic and added a clarifying comment. It only clears the received from info now if there is a transition from mempool rejected -> mempool acceptable. This means the tx has been evicted from our mempool for some reason that does not cause a conflict with it. In this case, it is like we never received the tx from a peer. It would be confusing to have a state where we received it but it is not in our mempool and not conflicted or confirmed. So, we clear received state and try the broadcasting flow again.

Now we also pick txs if they have are acceptable to our mempool and have no received from state. So, it is necessary to clear this data to ever have this tx picked for private broadcast again.

andrewtoth commented at 1:45 pm on March 11, 2026:

I thought about this some more, and it seems it was as confusing as it was because I was trying to use a single piece of state to represent two different possible states. If mempool_rejection_reason was not nullopt it could mean either the tx is not in the mempool and is not acceptable, or is currently in the mempool.

I split this out into an enum with 3 states: IN_MEMPOOL, ACCEPTABLE, REJECTED.

Now only if we are REJECTED will we store a reason string.

This simplifies the logic since only ACCEPTABLE txs will be picked for broadcast and returned as pending.

in src/private_broadcast.h:161 in fa857a980c

154@@ -126,6 +155,9 @@ class PrivateBroadcast
155     };
156 
157     struct TxState {
158+        std::optional<std::string> mempool_rejection_reason;
159+        std::optional<CService> received_from;
160+        std::optional<NodeClock::time_point> received_time;
161         std::vector<SendStatus> sent_to;

optout21 commented at 10:55 am on March 6, 2026:

nit: Ordering according to the chronology of setting the fields would make more sense (i.e., sent_to first).

optout21 commented at 11:14 am on March 6, 2026: contributor

ConceptACK bd31f7391dd45b2582092762466e40ad2594b178

Another round of review, no concerns (but keeping the status for now). Prev: Pre-review, left some minor comments. A general conceptual point is raised in separate comment (main-thread).

optout21 commented at 11:20 am on March 6, 2026: contributor

A more important conceptual point I’d like to raise: this work is the precursor of features that are related to locally-originated transactions, and not private broadcast. I think that it is important to clarify this and use the right nomenclature at this early stage.

As I see:

there where wishes/proposals related to local-originated transaction handling, private broadcast being one of them
private broadcast for locally-originating txs was implemented (🔥), and it introduced a short-lived queue needed for private broadcast
other features are starting to be implemented on top of the private broadcast queue, but logically they are not strictly related to the private nature of the broadcast, but the locally-generated property of it.
a good test is to answer the question: would this feature make sense even if the broadcast was not private, but ‘regular’ broadcast? If the answer is yes or likely yes, than the feaure is not related to the private broadcast, and its naming should not be based on it.
it should be even possible to decouple private broadcast from other local-originating features (though it makes sense to use private bc always).
With the current direction of things I even propose that the new RPC’s names (getprivatebroadcastinfo) should not be based on the ‘private’ feature, but ’local tx’ . Also, if the mentioned features are being considered, the name of the queue could be reconsidered at this point.

In short, for the newly considered features I consider the “locally-originated TX” to be more relevant than the “private” nature of the broadcast, and suggest using it in namings.

(fyi @vasild)

in src/rpc/mempool.cpp:148 in afca84489c outdated

147@@ -148,7 +148,7 @@ static RPCHelpMan getprivatebroadcastinfo()
148         RPCResult{

vasild commented at 11:45 am on March 9, 2026:

In the commit message of fa857a980c2fd6a1f88cea8d0cff984824352609 net: track received peer and mempool status in private broadcast:

“… or not accepted to mempool in stale check”

s/accepted/acceptable/

andrewtoth commented at 1:33 am on March 11, 2026:

Oops forgot to update the commit message. Will do when I push again.

in src/private_broadcast.cpp:85 in afca84489c

89+        [](const auto& el) { return DerivePriority(el.second.sent_to); })};
90+
91+    if (it != pending.end()) {
92+        auto& [tx, state]{*it};
93+        state.sent_to.emplace_back(will_send_to_nodeid, will_send_to_address, NodeClock::now());
94+        return it->first;

vasild commented at 2:15 pm on March 9, 2026:

In commit fa857a980c2fd6a1f88cea8d0cff984824352609 net: track received peer and mempool status in private broadcast the return is unnecessary changed from

0return tx;

to

0return it->first;

I find the former more readable.

in src/net_processing.cpp:1874 in afca84489c outdated

1869@@ -1868,7 +1870,8 @@ std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint25
1870     std::vector<CTransactionRef> removed_txs;
1871 
1872     size_t connections_cancelled{0};
1873-    for (const auto& [tx, _] : snapshot) {
1874+    for (const auto& tx_info : snapshot) {
1875+        const auto& tx{tx_info.tx};

vasild commented at 2:30 pm on March 9, 2026:

Unnecessary change.

andrewtoth commented at 1:33 am on March 11, 2026:

It’s necessary in a later commit. I moved it to the one that requires this change.

in src/net_processing.cpp:1663 in afca84489c

1658                          "Reattempting broadcast of stale txid=%s wtxid=%s",
1659                          stale_tx->GetHash().ToString(), stale_tx->GetWitnessHash().ToString());
1660                 ++num_for_rebroadcast;
1661             } else {
1662+                auto reason{mempool_acceptable.m_state.ToString()};
1663                 LogDebug(BCLog::PRIVBROADCAST, "Giving up broadcast attempts for txid=%s wtxid=%s: %s",

vasild commented at 9:34 am on March 10, 2026:

“Giving up” brings the sense of finality. With this change we may start sending the tx again in the future. Maybe change to “Stopping”. Also, I think it would be more clear to put “not acceptable in our mempool: %s” in there.

0                LogDebug(BCLog::PRIVBROADCAST, "Stopping broadcast attempts for txid=%s wtxid=%s: not acceptable in our mempool: %s",

in src/private_broadcast.cpp:27 in afca84489c

24+    const bool was_finished{it->second.mempool_rejection_reason.has_value()};
25+    it->second.mempool_rejection_reason.reset();
26+    it->second.received_from.reset();
27+    it->second.received_time.reset();
28+    return was_finished;
29 }

vasild commented at 10:01 am on March 10, 2026:

There is this notion of “if acceptable in our mempool - send it, otherwise don’t send it”.

Here in this PR we reset the state from Add() for an already existent transaction to “acceptable in our mempool”. However, this will not result in “send it”. That is because PeerManagerImpl::ReattemptPrivateBroadcast() will reevaluate and reset mempool acceptance state before deciding whether a transaction is eligible for rebroadcast. So the effect of resetting the state to “acceptable” in Add() will be short lived and would have no effect.

Maybe keep this PR to just retaining the transactions after broadcast is concluded, without changing the functionality of Add()?

andrewtoth commented at 3:38 pm on March 10, 2026:

That is because PeerManagerImpl::ReattemptPrivateBroadcast() will reevaluate and reset mempool acceptance state before deciding whether a transaction is eligible for rebroadcast.

It may reevaluate and reset mempool acceptance, but it may also be sent first from one of the connections that are created. I think we can solve this issue by checking if a tx exists in the mempool and continuing before updating mempool state in ReattemptPrivateBroadcast.

andrewtoth commented at 1:37 am on March 11, 2026:

I removed this change to Add logic in this PR. But, it required also removing "Sending a transaction that is already in the mempool" from the functional test. Since resending a tx already in the queue now will not trigger a rebroadcast.

in src/private_broadcast.cpp:132 in afca84489c outdated

128+    return std::ranges::any_of(m_transactions, [](const auto& el) {
129+        return !el.second.mempool_rejection_reason.has_value();
130+    });
131 }
132 
133 std::vector<CTransactionRef> PrivateBroadcast::GetStale() const

vasild commented at 10:06 am on March 10, 2026:

GetStale() needs to be changed to omit the concluded (finished) transactions.

andrewtoth commented at 3:45 pm on March 10, 2026:

I was thinking more about this and I don’t see a way to have “finished” transactions.

If a tx is acceptable to our mempool, it is not yet finished because it has not been received. If a tx exists in our mempool, it is not yet finished because it has not been mined and may be evicted. If a tx is not acceptable to our mempool, it or a conflicting tx may have been mined. However, it may also be the case that its parent tx has been evicted from our mempool as well and not yet mined. Either way, checking the confirmed utxo set will also show this tx as unacceptable, when it will be acceptable if the parent enters the mempool again. This tx is not yet finished since it may be acceptable to our mempool again if the parent returns.

So, is there a heuristic we can use to determine if a tx is concluded (finished)? I don’t see one. We need to keep txs in the queue indefinitely. One way is to have an expiry on txs which you suggested in #34322, but I believe that is out of scope for this PR.

vasild commented at 5:13 pm on March 17, 2026:

A transaction is certainly finished if it is mined or if a conflicting transaction is mined, no?

Currently (on master without this PR) we consider a transaction finished once we receive it back from the network without even checking whether it will make it to our mempool. Then we call Remove() and forget about the transaction.

This is deliberate to avoid infinite broadcasting of unacceptable transactions. That leaves retries of transactions that made it back but after a while need to be broadcast again at the hands of the users. I think that is worth reviewing and better be done in a focused PR that does just that. That would slim the current PR to just keeping the finished transactions in memory, without changing the meaning of “finished”.

andrewtoth commented at 7:35 pm on March 20, 2026:

A transaction is certainly finished if it is mined or if a conflicting transaction is mined, no?

How to determine this is tricky. There are a lot of ways to potentially solve this problem with different trade-offs. Let’s leave this to a follow-up.

in src/private_broadcast.cpp:76 in afca84489c

72     LOCK(m_mutex);
73 
74+    auto pending{m_transactions | std::views::filter([](const auto& el) {
75+        return !el.second.mempool_rejection_reason.has_value();
76+    })};
77+

vasild commented at 10:08 am on March 10, 2026:

This is repeated also in PrivateBroadcast::HavePendingTransactions(). Better introduce a new method PrivateBroadcast::GetPendingTransactions() and use it here and in HavePendingTransactions().

optout21 commented at 10:17 am on March 12, 2026:

I second to the proposal for a separate HavePendingTransactions(). For simplicity, it can just call GetPendingTransactions() and test for emptiness, or do a similar search with early exit. I don’t think performance is relevant here (queues will be small generally), but it’s nice to express intent precisely.

andrewtoth commented at 12:09 pm on March 12, 2026:

I took this suggestion. There is now both HavePendingTransactions and GetPendingTransactions. I just left a thumbs up and resolved this comment though and didn’t leave a message.

optout21 commented at 1:30 pm on March 12, 2026:

Sorry, my mistake (I pattern-matched !GetPendingTransactions().empty() and missed that it’s exactly in HavePendingTransactions :D)

vasild commented at 10:14 am on March 10, 2026: contributor

Reviewed up to and including fa857a980c net: track received peer and mempool status in private broadcast. @optout21, good points. Consider also #34533 and #3828. In the light of that, a sensitive transaction may not have originated locally. So “local” may not be the best term to use for these.

andrewtoth force-pushed on Mar 11, 2026

DrahtBot added the label CI failed on Mar 11, 2026

andrewtoth commented at 1:50 am on March 11, 2026: contributor

Thank you for your reviews @optout21 and @vasild.

Add no longer clears received state, so it is a simpler change. However, it does not rebroadcast txs if they are already received and sent again with sendrawtransaction. Removed unit test and functional test that covered this.
Only clear received state if transitioning from mempool rejected -> mempool acceptable. This happens if the tx is evicted but is not conflicted. In this case it is no longer “received” and we want to start the broadcast flow again.
Check if a tx already exists in mempool in ReattemptPrivateBroadcast before checking mempool acceptance.

Also addressed several other refactor and style changes, and split up and reordered commits for easier review.

git range-diff 9cad97f6cdf1bd660732cd10e844a6a7e0771ea0..afca84489c26f920d842f34d00f8418349255a65 eed316189376d72fc959e8d2e45cafef279f77da..9f58d9a1691b10341784e13ae32549bb0244bbda

I consider the “locally-originated TX” to be more relevant than the “private” nature of the broadcast, and suggest using it in namings. @optout21 that’s an interesting point. I do think it is out of scope for this PR though. Could be raised independently.

andrewtoth force-pushed on Mar 11, 2026

andrewtoth commented at 1:47 pm on March 11, 2026: contributor

Updated test to fix a bad rebase. Also, addressed feedback to make rebroadcast logic more clear #34707 (review).

git diff 9f58d9a1691b10341784e13ae32549bb0244bbda..c9b19d76dd336b2342d6d766b408410e1b93107b

andrewtoth force-pushed on Mar 11, 2026

DrahtBot removed the label CI failed on Mar 11, 2026

andrewtoth force-pushed on Mar 11, 2026

andrewtoth force-pushed on Mar 12, 2026

in src/private_broadcast.h:78 in 56a655d946 outdated

73+     * Record that a transaction was received back from the network and
74+     * pause rebroadcast attempts until the mempool state is reset.
75+     * @param[in] tx Transaction received from the network.
76+     * @param[in] received_from Peer address we received the transaction from.
77+     * @retval !nullopt The number of times the transaction was sent and confirmed
78+     * by the recipient (if the transaction existed and was removed).

optout21 commented at 10:25 am on March 12, 2026:

Nit: “was sent and confirmed by the recipient” -> “was sent and confirmed by a recipient” Reading “the recipient” made me think that it’s always the same recipient.

andrewtoth commented at 7:39 pm on March 20, 2026:

Done, and updated the docstring for Removed as well.

andrewtoth commented at 2:32 pm on March 12, 2026: contributor

Changed the approach to use an enum for the mempool states we care about: IN_MEMPOOL, ACCEPTABLE, REJECTED.

This should make it easier to reason about what is happening in this PR. I also split up the commits so they are easier to review.

refactor: wrap sent_to in TxSendStatus struct

This is a non-functional change.
Wrapping the sent_to vector in a struct lets us add more fields to each tx.

902708ea7d

test: extract find_tx_info to static helper

Non-functional refactor.

9c56c5f3cd

andrewtoth force-pushed on Mar 20, 2026

andrewtoth renamed this:
~~net: keep finished private broadcast txs in memory, rebroadcast if evicted from mempool~~
net: keep finished private broadcast txs in memory
on Mar 20, 2026

andrewtoth commented at 7:33 pm on March 20, 2026: contributor

I updated this PR to only track transactions that are in the happy-path of received back from the network. Tracking txs that are removed due to mempool rejection and rebroadcasting is left for follow-ups.

This should make this much simpler to review.

I also aligned naming to what is done in #34873.

net: track received peer in private broadcast

Introduce MarkReceived method in PrivateBroadcast to record which peer
relayed the transaction back to us, and at what time. The transaction stays in the
queue instead of being removed, preserving broadcast metadata.
However, the transaction will not be selected for broadcasting again.

The received metadata is cleared when a transaction is added to the queue again,
allowing the transaction to be broadcast again.

3c84f7fa7f

test: add unit test covering MarkReceived 673124c530

andrewtoth force-pushed on Mar 20, 2026

DrahtBot added the label CI failed on Mar 20, 2026

DrahtBot removed the label CI failed on Mar 20, 2026

net: keep finished private broadcast txs in memory #34707

Reviews

Conflicts