net: keep finished private broadcast txs in memory, rebroadcast if evicted from mempool by andrewtoth · Pull Request #34707 · bitcoin/bitcoin

andrewtoth · 2026-03-02T01:46:35Z

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 and mempool-rejected txs in memory. The peer's address we received the tx from and the time, or the mempool rejection reason is now also returned via getprivatebroadcastinfo RPC.

We store the txs mempool state as either IN_MEMPOOL, ACCEPTABLE, or REJECTED. Only txs that are currently not in the mempool but are acceptable will be picked for broadcast and returned as pending transactions. This maintains the current behavior of stopping broadcasting when these conditions are met.

When reattempting to broadcast stale private broadcast txs, every tx whether pending or not is checked for mempool acceptance. This has the added benefit of rebroadcasting previously received txs that have since been evicted from the mempool.

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 wallet: resubmit transactions with private broadcast if enabled #34533 (comment)).
adding a delay when submitting private broadcast txs so they will be broadcast at a later time (see [feature] Scheduled Transaction Broadcast #34118).
persisting private broadcast state to disk for resuming after restart or crash (see node: Persist private broadcast transactions over node restarts #34322).
expiring txs after a while so they are removed from the queue without having to manually abort.

DrahtBot · 2026-03-02T01:47:14Z

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  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34271 (net_processing: make m_tx_for_private_broadcast optional by vasild)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

receive -> received [The log message "Checking getprivatebroadcastinfo reports receive metadata after it is received back" uses "receive" where the past participle "received" is needed for grammatical clarity.]

^{2026-03-12 00:12:34}

DrahtBot · 2026-03-02T02:04:19Z

🚧 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.}

Hints

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.

src/net_processing.cpp

optout21

ConceptACK afca844

Pre-review, left some minor comments.
I general conceptual point is raised in separate comment (main-thread).

src/private_broadcast.cpp

optout21 · 2026-03-06T10:44:47Z

src/private_broadcast.cpp

+    const auto it{m_transactions.find(tx)};
+    if (it == m_transactions.end()) return;
+    it->second.mempool_rejection_reason = std::move(rejection_reason);
+    if (!it->second.mempool_rejection_reason.has_value()) {


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).

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.

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.

src/private_broadcast.h

optout21 · 2026-03-06T11:20:10Z

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)

vasild

Reviewed up to and including fa857a9 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.

src/rpc/mempool.cpp

src/private_broadcast.cpp

src/net_processing.cpp

src/private_broadcast.cpp

vasild · 2026-03-10T10:06:20Z

src/private_broadcast.cpp

+    });
 }

 std::vector<CTransactionRef> PrivateBroadcast::GetStale() const


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

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.

src/private_broadcast.cpp

andrewtoth · 2026-03-11T01:50:16Z

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 · 2026-03-11T13:47:38Z

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

git diff 9f58d9a1691b10341784e13ae32549bb0244bbda..c9b19d76dd336b2342d6d766b408410e1b93107b

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

Introduce MempoolState enum (IN_MEMPOOL, ACCEPTABLE, REJECTED) and UpdateMempoolState method in PrivateBroadcast. Add GetPendingTransactions to filter by ACCEPTABLE state. Only pick pending txs for sending. Don't remove txs when not acceptable to mempool in stale check; instead track their rejection state.

Introduce Received method in PrivateBroadcast to record which peer relayed the transaction back to us. The transaction stays in the queue instead of being removed, preserving broadcast metadata.

Non-functional refactor.

DrahtBot added the P2P label Mar 2, 2026

andrewtoth force-pushed the private_broadcast_store branch from a88810a to afca844 Compare March 2, 2026 02:03

DrahtBot added the CI failed label Mar 2, 2026

This was referenced Mar 2, 2026

Feature Request: Broadcast Pool #30471

Open

node: Persist private broadcast transactions over node restarts #34322

Draft

DrahtBot removed the CI failed label Mar 2, 2026

This was referenced Mar 2, 2026

net/log: standardize peer+addr log formatting via LogPeer #34389

Merged

net_processing: make m_tx_for_private_broadcast optional #34271

Open

vasild mentioned this pull request Mar 4, 2026

Private broadcast tracking #34476

Open

13 tasks

andrewtoth mentioned this pull request Mar 4, 2026

wallet: resubmit transactions with private broadcast if enabled #34533

Draft

optout21 reviewed Mar 6, 2026

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

optout21 reviewed Mar 6, 2026

View reviewed changes

vasild reviewed Mar 10, 2026

View reviewed changes

andrewtoth force-pushed the private_broadcast_store branch 2 times, most recently from 2c3c80d to 9f58d9a Compare March 11, 2026 01:40

DrahtBot added the CI failed label Mar 11, 2026

andrewtoth force-pushed the private_broadcast_store branch from 9f58d9a to c9b19d7 Compare March 11, 2026 13:39

andrewtoth force-pushed the private_broadcast_store branch from c9b19d7 to 6ecbf18 Compare March 11, 2026 15:12

DrahtBot removed the CI failed label Mar 11, 2026

andrewtoth added 2 commits March 11, 2026 16:58

refactor: wrap sent_to in TxState struct

91f7d44

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

andrewtoth force-pushed the private_broadcast_store branch from 6ecbf18 to 911d4eb Compare March 11, 2026 20:58

andrewtoth added 3 commits March 11, 2026 20:11

net: track received peer in private broadcast

56a655d

Introduce Received method in PrivateBroadcast to record which peer relayed the transaction back to us. The transaction stays in the queue instead of being removed, preserving broadcast metadata.

net: return received data and mempool rejection reason in pb info

c57c834

test: extract find_tx_info to static helper

84094e9

Non-functional refactor.

andrewtoth added 4 commits March 11, 2026 20:11

tests: add unit test for Received method

3644db0

tests: add unit test for UpdateMempoolState method

ce26dbb

rpc,net: return received data or mempool rejection info

ea6d6ae

test: cover new privatebroadcastinfo fields in functional test

bd31f73

andrewtoth force-pushed the private_broadcast_store branch from 911d4eb to bd31f73 Compare March 12, 2026 00:12

Conversation

andrewtoth commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

DrahtBot commented Mar 2, 2026

Uh oh!

Uh oh!

optout21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

optout21 Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewtoth Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

andrewtoth Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

optout21 commented Mar 6, 2026

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

andrewtoth Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewtoth commented Mar 11, 2026

Uh oh!

andrewtoth commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andrewtoth commented Mar 2, 2026 •

edited

Loading

DrahtBot commented Mar 2, 2026 •

edited

Loading

optout21 Mar 6, 2026 •

edited

Loading

andrewtoth Mar 11, 2026 •

edited

Loading

andrewtoth Mar 10, 2026 •

edited

Loading