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
Approach ACK	vasild
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:

#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)

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

ACK d5d7c28e1b4b76daddad95a23cbf56209ca0b86e

Previous: 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.

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.

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

in src/private_broadcast.cpp:26 in 3c84f7fa7f outdated

23+        if (!state.received_from.has_value()) {
24+            return false;
25+        }
26+        state.received_from.reset();
27+        state.received_time.reset();
28+        state.send_statuses.clear();

optout21 commented at 10:24 am on March 23, 2026:

3c84f7f net: track received peer in private broadcast:

In PrivateBroadcast::Add, when the TX was already present, but has not been received back yet, it returns true. However, the documentation says for the return value: “The transaction was already present.”. I feel an inconsistency here: either the code should return false in this case, or the documentation be more precise.

andrewtoth commented at 1:38 pm on March 23, 2026:

I think the documentation should be updated. We need to return true in this case to maintain the same behavior. Previously, a received tx is removed, so calling Add again would return true and add the tx with a fresh state.

in src/net_processing.cpp:1872 in 3c84f7fa7f outdated

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

optout21 commented at 10:31 am on March 23, 2026:

3c84f7f net: track received peer in private broadcast:

This change has became unnecessary, it is no longer needed by later commit (was discussed previously in Resolved thread).

andrewtoth commented at 1:42 pm on March 23, 2026:

Removing this change causes a compilation error. It is needed when we add new fields to TxBroadcastInfo. The later commit that was discussed previously has been rolled into this one commit. In the previous version, updates to TxBroadcastInfo happened later and only TxState was updated, which is why the change wasn’t needed earlier.

0../src/net_processing.cpp: In member function ‘virtual std::vector<std::shared_ptr<const CTransaction> > {anonymous}::PeerManagerImpl::AbortPrivateBroadcast(const uint256&)’:
1../src/net_processing.cpp:1871:22: error: only 2 names provided for structured binding
2 1871 |     for (const auto& [tx, _] : snapshot) {
3      |                      ^~~~~~~
4../src/net_processing.cpp:1871:22: note: while ‘const PrivateBroadcast::TxBroadcastInfo’ decomposes into 4 elements

optout21 commented at 2:28 pm on March 23, 2026:

Got it. Sg. like (const auto& [tx, _a, _b, _c, _d] : snapshot) would be needed, so the changed version is better.

optout21 commented at 10:32 am on March 23, 2026: contributor

Minor comments provided.

andrewtoth force-pushed on Mar 23, 2026

andrewtoth commented at 10:36 pm on March 23, 2026: contributor

Thanks for your review @optout21. Took your suggestion #34707 (review). Only diff is changing docstring of PrivateBroadcast::Add().

git diff 673124c53013a91d0ca7a70de9c54d521bb3c308..d5d7c28e1b4b76daddad95a23cbf56209ca0b86e

in src/private_broadcast.cpp:166 in d5d7c28e1b

vasild commented at 11:15 am on March 26, 2026:

Just curious - why change the type of the DerivePriority()’s parameter? Do you think that maybe in the future it might be needed to use also other fields from TxSendStatus to derive the priority?

andrewtoth commented at 4:50 pm on April 4, 2026:

This commit was dropped, so no longer relevant.

Do you think that maybe in the future it might be needed to use also other fields from TxSendStatus to derive the priority?

Yes. If we want to periodically rebroadcast txs that have been received but are still acceptable to our utxo set (not mined or conflicted), we would need more information here. But this needs more discussion and we can update this parameter when we decide to do something like that.

in src/test/private_broadcast_tests.cpp:27 in d5d7c28e1b

23@@ -24,6 +24,17 @@ static CTransactionRef MakeDummyTx(uint32_t id, size_t num_witness)
24     return MakeTransactionRef(mtx);
25 }
26 
27+static auto FindTxInfo(const std::vector<PrivateBroadcast::TxBroadcastInfo>& infos, const CTransactionRef& tx)

vasild commented at 11:25 am on March 26, 2026:

Correct me if I am wrong, anonymous namespace and static (on global functions) achieve the same purpose - the symbols will stay within that file when compiled, so they cannot collide with symbols from other files. If that is the case, then better use just one - either static or a namespace.

andrewtoth commented at 4:50 pm on April 4, 2026:

Removed static.

in src/private_broadcast.h:148 in d5d7c28e1b

140@@ -124,6 +141,12 @@ class PrivateBroadcast
141         SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
142     };
143 
144+    struct TxSendStatus {
145+        std::vector<SendStatus> send_statuses;
146+        std::optional<CService> received_from;
147+        std::optional<NodeClock::time_point> received_time;
148+    };

vasild commented at 1:28 pm on March 26, 2026:

received_from and received_time must be either both set or both not set. To enforce this we can have one optional holding a struct with two members: from and when.

0    struct TxSendStatus {
1        std::vector<SendStatus> send_statuses;
2        struct Received {
3            CService from;
4            NodeClock::time_point when;
5        }
6        std::optional<Received> received;
7    };

andrewtoth commented at 4:50 pm on April 4, 2026:

Done.

in src/net_processing.cpp:4495 in d5d7c28e1b outdated

4491@@ -4403,10 +4492,10 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4492         const uint256& hash = peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256();
4493         AddKnownTx(peer, hash);
4494 
4495-        if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
4496+        if (const auto num_broadcasted{m_tx_for_private_broadcast.MarkReceived(ptx, CService{pfrom.addr})}) {

vasild commented at 4:39 pm on March 26, 2026:

Inbound Tor connections would probably look like 127.0.0.1 (the address of the Tor router). I am not sure if we should bother trying to make this more user friendly. If yes, then something like:

0if pfrom.m_inbound_onion is true
1    CService s;
2    s.SetSpecial("inboundconnectionfromtoraaaaaaaaaaaaaaaaaaaaaaaaaaan5wyd.onion");
3    use s
4else
5    use pfrom.addr

andrewtoth commented at 5:00 pm on April 4, 2026:

I decided not to take this. I’m not sure this is the best way to signal to a user that this is an inbound tor connection. They might mistake it for the actual address and be confused when it doesn’t exist. I think a cleaner approach might be to use a variant of {CService, std::string} and write "inbound tor" for the string? But I don’t think the code complexity would be worth it. I’m not entirely convinced using 127.0.0.1 will cause any issues.

in test/functional/p2p_private_broadcast.py:448 in d5d7c28e1b outdated

448-        self.log.info("Sending a transaction that is already in the mempool")
449+        info = [t for t in pbinfo["transactions"] if t["wtxid"] == txs[0]["wtxid"]]
450+        assert_equal(len(info), 1)
451+        assert "received_from" in info[0]
452+        assert "received_time" in info[0]
453         skip_destinations = len(self.destinations)

vasild commented at 4:48 pm on March 26, 2026:

This line is now not needed, can drop it.

in test/functional/p2p_private_broadcast.py:447 in d5d7c28e1b outdated

443+        self.log.info("Checking getprivatebroadcastinfo reports receive metadata after it is received back")
444         pbinfo = tx_originator.getprivatebroadcastinfo()
445-        pending = [t for t in pbinfo["transactions"] if t["txid"] == txs[0]["txid"] and t["wtxid"] == txs[0]["wtxid"]]
446-        assert_equal(len(pending), 0)
447-
448-        self.log.info("Sending a transaction that is already in the mempool")

vasild commented at 4:49 pm on March 26, 2026:

Why remove this test? It is passing, so maybe restore it?

 0--- i/test/functional/p2p_private_broadcast.py
 1+++ w/test/functional/p2p_private_broadcast.py
 2@@ -444,12 +444,17 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
 3         info = [t for t in pbinfo["transactions"] if t["wtxid"] == txs[0]["wtxid"]]
 4         assert_equal(len(info), 1)
 5         assert "received_from" in info[0]
 6         assert "received_time" in info[0]
 7         skip_destinations = len(self.destinations)
 8 
 9+        self.log.info("Sending a transaction that is already in the mempool")
10+        skip_destinations = len(self.destinations)
11+        tx_originator.sendrawtransaction(hexstring=txs[0]["hex"], maxfeerate=0)
12+        self.check_broadcasts("Broadcast of mempool transaction", txs[0], NUM_PRIVATE_BROADCAST_PER_TX, skip_destinations)
13+
14         self.log.info("Sending a transaction with a dependency in the mempool")
15         skip_destinations = len(self.destinations)
16         tx_originator.sendrawtransaction(hexstring=txs[1]["hex"], maxfeerate=0.1)
17         self.check_broadcasts("Dependency in mempool", txs[1], NUM_PRIVATE_BROADCAST_PER_TX, skip_destinations)
18 
19         self.log.info("Sending a transaction with a dependency not in the mempool (should be rejected)")

in src/test/private_broadcast_tests.cpp:148 in d5d7c28e1b

143+{
144+    PrivateBroadcast pb;
145+
146+    const auto missing_tx{MakeDummyTx(/*id=*/999, /*num_witness=*/0)};
147+    const auto tx{MakeDummyTx(/*id=*/42, /*num_witness=*/0)};
148+    in_addr ipv4Addr;

vasild commented at 5:02 pm on March 26, 2026:

The naming of this variable is usingCamelCase, should_be_snake.

in src/private_broadcast.cpp:147 in d5d7c28e1b

146-        entries.emplace_back(TxBroadcastInfo{.tx = tx, .peers = std::move(peers)});
147+        entries.emplace_back(TxBroadcastInfo{
148+            .tx = tx,
149+            .peers = std::move(peers),
150+            .received_from = state.received_from,
151+            .received_time = state.received_time,

vasild commented at 6:03 pm on March 26, 2026:

I have been thinking that the public structs TxBroadcastInfo and PeerSendInfo are the same as the private ones TxSendStatus and SendStatus. Before it wasn’t so striking, but now it becomes more obvious as more fields are added and they have to be added to both and copied here in GetBroadcastInfo()

We can have just one set of structs and return m_transactions from GetBroadcastInfo(). The callers of GetBroadcastInfo() don’t care whether it is a vector or unordered_map as long as they can iterate over it.

Here is a change that does that, on top of this PR. If you do not want to bloat this PR with it, I will submit as a followup:

  0diff --git i/src/net_processing.cpp w/src/net_processing.cpp
  1index ed3e5ebf98..db8a007a04 100644
  2--- i/src/net_processing.cpp
  3+++ w/src/net_processing.cpp
  4@@ -539,13 +539,13 @@ public:
  5     void CheckForStaleTipAndEvictPeers() override;
  6     util::Expected<void, std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
  7         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
  8     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
  9     std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
 10     PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 11-    std::vector<PrivateBroadcast::TxBroadcastInfo> GetPrivateBroadcastInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 12+    PrivateBroadcast::Transactions GetPrivateBroadcastInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 13     std::vector<CTransactionRef> AbortPrivateBroadcast(const uint256& id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 14     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 15     void InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 16     void InitiateTxBroadcastPrivate(const CTransactionRef& tx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 17     void SetBestBlock(int height, std::chrono::seconds time) override
 18     {
 19@@ -1854,25 +1854,24 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const
 20     return PeerManagerInfo{
 21         .median_outbound_time_offset = m_outbound_time_offsets.Median(),
 22         .ignores_incoming_txs = m_opts.ignore_incoming_txs,
 23     };
 24 }
 25 
 26-std::vector<PrivateBroadcast::TxBroadcastInfo> PeerManagerImpl::GetPrivateBroadcastInfo() const
 27+PrivateBroadcast::Transactions PeerManagerImpl::GetPrivateBroadcastInfo() const
 28 {
 29     return m_tx_for_private_broadcast.GetBroadcastInfo();
 30 }
 31 
 32 std::vector<CTransactionRef> PeerManagerImpl::AbortPrivateBroadcast(const uint256& id)
 33 {
 34     const auto snapshot{m_tx_for_private_broadcast.GetBroadcastInfo()};
 35     std::vector<CTransactionRef> removed_txs;
 36 
 37     size_t connections_cancelled{0};
 38-    for (const auto& tx_info : snapshot) {
 39-        const auto& tx{tx_info.tx};
 40+    for (const auto& [tx, status] : snapshot) {
 41         if (tx->GetHash().ToUint256() != id && tx->GetWitnessHash().ToUint256() != id) continue;
 42         if (const auto peer_acks{m_tx_for_private_broadcast.Remove(tx)}) {
 43             removed_txs.push_back(tx);
 44             if (NUM_PRIVATE_BROADCAST_PER_TX > *peer_acks) {
 45                 connections_cancelled += (NUM_PRIVATE_BROADCAST_PER_TX - *peer_acks);
 46             }
 47diff --git i/src/net_processing.h w/src/net_processing.h
 48index 36ae021f67..9472aa7735 100644
 49--- i/src/net_processing.h
 50+++ w/src/net_processing.h
 51@@ -118,13 +118,13 @@ public:
 52     virtual std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() = 0;
 53 
 54     /** Get peer manager info. */
 55     virtual PeerManagerInfo GetInfo() const = 0;
 56 
 57     /** Get info about transactions currently being privately broadcast. */
 58-    virtual std::vector<PrivateBroadcast::TxBroadcastInfo> GetPrivateBroadcastInfo() const = 0;
 59+    virtual PrivateBroadcast::Transactions GetPrivateBroadcastInfo() const = 0;
 60 
 61     /**
 62      * Abort private broadcast attempts for transactions currently being privately broadcast.
 63      *
 64      * [@param](/bitcoin-bitcoin/contributor/param/)[in] id A transaction identifier. It will be matched against both txid and wtxid for
 65      *               all transactions in the private broadcast queue.
 66diff --git i/src/private_broadcast.cpp w/src/private_broadcast.cpp
 67index 0c70fff7d8..e0349032be 100644
 68--- i/src/private_broadcast.cpp
 69+++ w/src/private_broadcast.cpp
 70@@ -124,37 +124,20 @@ std::vector<CTransactionRef> PrivateBroadcast::GetStale() const
 71             stale.push_back(tx);
 72         }
 73     }
 74     return stale;
 75 }
 76 
 77-std::vector<PrivateBroadcast::TxBroadcastInfo> PrivateBroadcast::GetBroadcastInfo() const
 78+PrivateBroadcast::Transactions PrivateBroadcast::GetBroadcastInfo() const
 79     EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
 80 {
 81     LOCK(m_mutex);
 82-    std::vector<TxBroadcastInfo> entries;
 83-    entries.reserve(m_transactions.size());
 84-
 85-    for (const auto& [tx, state] : m_transactions) {
 86-        std::vector<PeerSendInfo> peers;
 87-        peers.reserve(state.send_statuses.size());
 88-        for (const auto& status : state.send_statuses) {
 89-            peers.emplace_back(PeerSendInfo{.address = status.address, .sent = status.picked, .received = status.confirmed});
 90-        }
 91-        entries.emplace_back(TxBroadcastInfo{
 92-            .tx = tx,
 93-            .peers = std::move(peers),
 94-            .received_from = state.received_from,
 95-            .received_time = state.received_time,
 96-        });
 97-    }
 98-
 99-    return entries;
100+    return m_transactions;
101 }
102 
103-PrivateBroadcast::Priority PrivateBroadcast::DerivePriority(const TxSendStatus& tx_status)
104+PrivateBroadcast::Priority PrivateBroadcast::DerivePriority(const Status& tx_status)
105 {
106     Priority p;
107     p.num_picked = tx_status.send_statuses.size();
108     for (const auto& send_status : tx_status.send_statuses) {
109         p.last_picked = std::max(p.last_picked, send_status.picked);
110         if (send_status.confirmed.has_value()) {
111@@ -176,17 +159,17 @@ std::optional<PrivateBroadcast::TxAndSendStatusForNode> PrivateBroadcast::GetSen
112             }
113         }
114     }
115     return std::nullopt;
116 }
117 
118-std::vector<PrivateBroadcast::TransactionMap::iterator> PrivateBroadcast::GetPendingTransactions()
119+std::vector<PrivateBroadcast::Transactions::iterator> PrivateBroadcast::GetPendingTransactions()
120     EXCLUSIVE_LOCKS_REQUIRED(m_mutex)
121 {
122     AssertLockHeld(m_mutex);
123-    std::vector<TransactionMap::iterator> result;
124+    std::vector<Transactions::iterator> result;
125     for (auto it{m_transactions.begin()}; it != m_transactions.end(); ++it) {
126         if (!it->second.received_from.has_value()) {
127             result.push_back(it);
128         }
129     }
130     return result;
131diff --git i/src/private_broadcast.h w/src/private_broadcast.h
132index 5c70754df2..3c39ad298a 100644
133--- i/src/private_broadcast.h
134+++ w/src/private_broadcast.h
135@@ -28,25 +28,47 @@
136  * - Query whether a given recipient node has confirmed reception
137  * - Query whether any transactions that need sending are currently on the list
138  */
139 class PrivateBroadcast
140 {
141 public:
142-    struct PeerSendInfo {
143-        CService address;
144-        NodeClock::time_point sent;
145-        std::optional<NodeClock::time_point> received;
146+    /// Status of a transaction sent to a given node.
147+    struct SendStatus {
148+        const NodeId nodeid; /// Node to which the transaction will be sent (or was sent).
149+        const CService address; /// Address of the node.
150+        const NodeClock::time_point picked; ///< When was the transaction picked for sending to the node.
151+        std::optional<NodeClock::time_point> confirmed; ///< When was the transaction reception confirmed by the node (by PONG).
152+
153+        SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
154     };
155 
156-    struct TxBroadcastInfo {
157-        CTransactionRef tx;
158-        std::vector<PeerSendInfo> peers;
159-        std::optional<CService> received_from;
160-        std::optional<NodeClock::time_point> received_time;
161+    /// Status of a transaction, including all send attempts and a possible reception info.
162+    struct Status {
163+        std::vector<SendStatus> send_statuses; /// All send attempts.
164+        std::optional<CService> received_from; /// When we receive back the transaction from the network, this is the peer we got it from.
165+        std::optional<NodeClock::time_point> received_time; /// Time of receiving back the transaction.
166     };
167 
168+    // No need for salted hasher because we are going to store just a bunch of locally originating transactions.
169+
170+    struct CTransactionRefHash {
171+        size_t operator()(const CTransactionRef& tx) const
172+        {
173+            return static_cast<size_t>(tx->GetWitnessHash().ToUint256().GetUint64(0));
174+        }
175+    };
176+
177+    struct CTransactionRefComp {
178+        bool operator()(const CTransactionRef& a, const CTransactionRef& b) const
179+        {
180+            return a->GetWitnessHash() == b->GetWitnessHash(); // If wtxid equals, then txid also equals.
181+        }
182+    };
183+
184+    using Transactions = std::unordered_map<CTransactionRef, Status, CTransactionRefHash, CTransactionRefComp>;
185+
186     /**
187      * Add a transaction to the storage, or reset a transaction's state if the
188      * transaction has been marked received.
189      * [@param](/bitcoin-bitcoin/contributor/param/)[in] tx The transaction to add.
190      * [@retval](/bitcoin-bitcoin/contributor/retval/) true The transaction was added or reset.
191      * [@retval](/bitcoin-bitcoin/contributor/retval/) false The transaction was already present and not marked received.
192@@ -124,32 +146,16 @@ public:
193     std::vector<CTransactionRef> GetStale() const
194         EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
195 
196     /**
197      * Get stats about all transactions currently being privately broadcast.
198      */
199-    std::vector<TxBroadcastInfo> GetBroadcastInfo() const
200+    Transactions GetBroadcastInfo() const
201         EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
202 
203 private:
204-    /// Status of a transaction sent to a given node.
205-    struct SendStatus {
206-        const NodeId nodeid; /// Node to which the transaction will be sent (or was sent).
207-        const CService address; /// Address of the node.
208-        const NodeClock::time_point picked; ///< When was the transaction picked for sending to the node.
209-        std::optional<NodeClock::time_point> confirmed; ///< When was the transaction reception confirmed by the node (by PONG).
210-
211-        SendStatus(const NodeId& nodeid, const CService& address, const NodeClock::time_point& picked) : nodeid{nodeid}, address{address}, picked{picked} {}
212-    };
213-
214-    struct TxSendStatus {
215-        std::vector<SendStatus> send_statuses;
216-        std::optional<CService> received_from;
217-        std::optional<NodeClock::time_point> received_time;
218-    };
219-
220     /// Cumulative stats from all the send attempts for a transaction. Used to prioritize transactions.
221     struct Priority {
222         size_t num_picked{0}; ///< Number of times the transaction was picked for sending.
223         NodeClock::time_point last_picked{}; ///< The most recent time when the transaction was picked for sending.
224         size_t num_confirmed{0}; ///< Number of nodes that have confirmed reception of a transaction (by PONG).
225         NodeClock::time_point last_confirmed{}; ///< The most recent time when the transaction was confirmed.
226@@ -167,46 +173,28 @@ private:
227     /// A pair of a transaction and a sent status for a given node. Convenience return type of GetSendStatusByNode().
228     struct TxAndSendStatusForNode {
229         const CTransactionRef& tx;
230         SendStatus& send_status;
231     };
232 
233-    // No need for salted hasher because we are going to store just a bunch of locally originating transactions.
234-
235-    struct CTransactionRefHash {
236-        size_t operator()(const CTransactionRef& tx) const
237-        {
238-            return static_cast<size_t>(tx->GetWitnessHash().ToUint256().GetUint64(0));
239-        }
240-    };
241-
242-    struct CTransactionRefComp {
243-        bool operator()(const CTransactionRef& a, const CTransactionRef& b) const
244-        {
245-            return a->GetWitnessHash() == b->GetWitnessHash(); // If wtxid equals, then txid also equals.
246-        }
247-    };
248-
249-    using TransactionMap = std::unordered_map<CTransactionRef, TxSendStatus, CTransactionRefHash, CTransactionRefComp>;
250-
251     /**
252      * Derive the sending priority of a transaction.
253      * [@param](/bitcoin-bitcoin/contributor/param/)[in] status The send status of the transaction.
254      */
255-    static Priority DerivePriority(const TxSendStatus& status);
256+    static Priority DerivePriority(const Status& status);
257 
258     /**
259      * Find which transaction we sent to a given node (marked by PickTxForSend()).
260      * [@return](/bitcoin-bitcoin/contributor/return/) That transaction together with the send status or nullopt if we did not
261      * send any transaction to the given node.
262      */
263     std::optional<TxAndSendStatusForNode> GetSendStatusByNode(const NodeId& nodeid)
264         EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
265 
266-    std::vector<TransactionMap::iterator> GetPendingTransactions()
267+    std::vector<Transactions::iterator> GetPendingTransactions()
268         EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
269 
270     mutable Mutex m_mutex;
271-    TransactionMap m_transactions GUARDED_BY(m_mutex);
272+    Transactions m_transactions GUARDED_BY(m_mutex);
273 };
274 
275 #endif // BITCOIN_PRIVATE_BROADCAST_H
276diff --git i/src/rpc/mempool.cpp w/src/rpc/mempool.cpp
277index caa0c9e636..bdc61a3e8c 100644
278--- i/src/rpc/mempool.cpp
279+++ w/src/rpc/mempool.cpp
280@@ -174,34 +174,36 @@ static RPCHelpMan getprivatebroadcastinfo()
281             + HelpExampleRpc("getprivatebroadcastinfo", "")
282         },
283         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
284         {
285             const NodeContext& node{EnsureAnyNodeContext(request.context)};
286             const PeerManager& peerman{EnsurePeerman(node)};
287-            const auto txs{peerman.GetPrivateBroadcastInfo()};
288+            const auto pbinfo{peerman.GetPrivateBroadcastInfo()};
289 
290             UniValue transactions(UniValue::VARR);
291-            for (const auto& tx_info : txs) {
292+            for (const auto& [tx, status] : pbinfo) {
293                 UniValue o(UniValue::VOBJ);
294-                o.pushKV("txid", tx_info.tx->GetHash().ToString());
295-                o.pushKV("wtxid", tx_info.tx->GetWitnessHash().ToString());
296-                o.pushKV("hex", EncodeHexTx(*tx_info.tx));
297+                o.pushKV("txid", tx->GetHash().ToString());
298+                o.pushKV("wtxid", tx->GetWitnessHash().ToString());
299+                o.pushKV("hex", EncodeHexTx(*tx));
300                 UniValue peers(UniValue::VARR);
301-                for (const auto& peer : tx_info.peers) {
302+                for (const auto& peer : status.send_statuses) {
303                     UniValue p(UniValue::VOBJ);
304                     p.pushKV("address", peer.address.ToStringAddrPort());
305-                    p.pushKV("sent", TicksSinceEpoch<std::chrono::seconds>(peer.sent));
306-                    if (peer.received.has_value()) {
307-                        p.pushKV("received", TicksSinceEpoch<std::chrono::seconds>(*peer.received));
308+                    p.pushKV("sent", TicksSinceEpoch<std::chrono::seconds>(peer.picked));
309+                    if (peer.confirmed.has_value()) {
310+                        p.pushKV("received", TicksSinceEpoch<std::chrono::seconds>(*peer.confirmed));
311                     }
312                     peers.push_back(std::move(p));
313                 }
314                 o.pushKV("peers", std::move(peers));
315-                if (tx_info.received_from.has_value()) {
316-                    o.pushKV("received_from", tx_info.received_from->ToStringAddrPort());
317-                    o.pushKV("received_time", TicksSinceEpoch<std::chrono::seconds>(*tx_info.received_time));
318+                if (status.received_from.has_value()) {
319+                    o.pushKV("received_from", status.received_from->ToStringAddrPort());
320+                }
321+                if (status.received_time.has_value()) {
322+                    o.pushKV("received_time", TicksSinceEpoch<std::chrono::seconds>(status.received_time.value()));
323                 }
324                 transactions.push_back(std::move(o));
325             }
326 
327             UniValue ret(UniValue::VOBJ);
328             ret.pushKV("transactions", std::move(transactions));
329diff --git i/src/test/private_broadcast_tests.cpp w/src/test/private_broadcast_tests.cpp
330index 715cafb190..0cf58e5d0e 100644
331--- i/src/test/private_broadcast_tests.cpp
332+++ w/src/test/private_broadcast_tests.cpp
333@@ -21,19 +21,12 @@ static CTransactionRef MakeDummyTx(uint32_t id, size_t num_witness)
334         mtx.vin[0].scriptWitness = CScriptWitness{};
335         mtx.vin[0].scriptWitness.stack.resize(num_witness);
336     }
337     return MakeTransactionRef(mtx);
338 }
339 
340-static auto FindTxInfo(const std::vector<PrivateBroadcast::TxBroadcastInfo>& infos, const CTransactionRef& tx)
341-{
342-    const auto it{std::ranges::find(infos, tx->GetWitnessHash(), [](const auto& info) { return info.tx->GetWitnessHash(); })};
343-    BOOST_REQUIRE(it != infos.end());
344-    return it;
345-}
346-
347 } // namespace
348 
349 BOOST_FIXTURE_TEST_SUITE(private_broadcast_tests, BasicTestingSetup)
350 
351 BOOST_AUTO_TEST_CASE(basic)
352 {
353@@ -63,14 +56,14 @@ BOOST_AUTO_TEST_CASE(basic)
354     BOOST_REQUIRE(tx1->GetWitnessHash() != tx2->GetWitnessHash());
355 
356     BOOST_CHECK(pb.Add(tx2));
357     const auto check_peer_counts{[&](size_t tx1_peer_count, size_t tx2_peer_count) {
358         const auto infos{pb.GetBroadcastInfo()};
359         BOOST_CHECK_EQUAL(infos.size(), 2);
360-        BOOST_CHECK_EQUAL(FindTxInfo(infos, tx1)->peers.size(), tx1_peer_count);
361-        BOOST_CHECK_EQUAL(FindTxInfo(infos, tx2)->peers.size(), tx2_peer_count);
362+        BOOST_CHECK_EQUAL(infos.at(tx1).send_statuses.size(), tx1_peer_count);
363+        BOOST_CHECK_EQUAL(infos.at(tx2).send_statuses.size(), tx2_peer_count);
364     }};
365 
366     check_peer_counts(/*tx1_peer_count=*/0, /*tx2_peer_count=*/0);
367 
368     const auto tx_for_recipient1{pb.PickTxForSend(/*will_send_to_nodeid=*/recipient1, /*will_send_to_address=*/addr1).value()};
369     BOOST_CHECK(tx_for_recipient1 == tx1 || tx_for_recipient1 == tx2);
370@@ -105,24 +98,22 @@ BOOST_AUTO_TEST_CASE(basic)
371     BOOST_CHECK(pb.DidNodeConfirmReception(recipient1));
372     BOOST_CHECK(!pb.DidNodeConfirmReception(recipient2));
373 
374     const auto infos{pb.GetBroadcastInfo()};
375     BOOST_CHECK_EQUAL(infos.size(), 2);
376     {
377-        const auto info_it{FindTxInfo(infos, tx_for_recipient1)};
378-        const auto& peers{info_it->peers};
379+        const auto& peers{infos.at(tx_for_recipient1).send_statuses};
380         BOOST_CHECK_EQUAL(peers.size(), 1);
381         BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr1.ToStringAddrPort());
382-        BOOST_CHECK(peers[0].received.has_value());
383+        BOOST_CHECK(peers[0].confirmed.has_value());
384     }
385     {
386-        const auto info_it{FindTxInfo(infos, tx_for_recipient2)};
387-        const auto& peers{info_it->peers};
388+        const auto& peers{infos.at(tx_for_recipient2).send_statuses};
389         BOOST_CHECK_EQUAL(peers.size(), 1);
390         BOOST_CHECK_EQUAL(peers[0].address.ToStringAddrPort(), addr2.ToStringAddrPort());
391-        BOOST_CHECK(!peers[0].received.has_value());
392+        BOOST_CHECK(!peers[0].confirmed.has_value());
393     }
394 
395     BOOST_CHECK_EQUAL(pb.GetStale().size(), 1);
396     BOOST_CHECK_EQUAL(pb.GetStale()[0], tx_for_recipient2);
397 
398     SetMockTime(Now<NodeSeconds>() + 10h);
399@@ -164,36 +155,35 @@ BOOST_AUTO_TEST_CASE(mark_received)
400 
401     // MarkReceived succeeds and returns the number of confirmed sends.
402     BOOST_CHECK_EQUAL(pb.MarkReceived(tx, received_from).value(), 1);
403 
404     {
405         const auto infos{pb.GetBroadcastInfo()};
406-        const auto info{FindTxInfo(infos, tx)};
407-        BOOST_CHECK_EQUAL(info->received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
408-        BOOST_CHECK(info->received_time.has_value());
409+        const auto& info{infos.at(tx)};
410+        BOOST_CHECK_EQUAL(info.received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
411+        BOOST_CHECK(info.received_time.has_value());
412     }
413     BOOST_CHECK(!pb.HavePendingTransactions());
414     BOOST_CHECK(!pb.PickTxForSend(/*will_send_to_nodeid=*/recipient2, /*will_send_to_address=*/addr2).has_value());
415 
416     // Subsequent MarkReceived returns nullopt and does not overwrite.
417     in_addr ipv4Addr2;
418     ipv4Addr2.s_addr = 0xa0b0c099;
419     const CService received_from2{ipv4Addr2, 4444};
420     BOOST_CHECK(!pb.MarkReceived(tx, received_from2).has_value());
421 
422     {
423         const auto infos{pb.GetBroadcastInfo()};
424-        const auto info{FindTxInfo(infos, tx)};
425-        BOOST_CHECK_EQUAL(info->received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
426+        BOOST_CHECK_EQUAL(infos.at(tx).received_from->ToStringAddrPort(), received_from.ToStringAddrPort());
427         BOOST_CHECK(!pb.HavePendingTransactions());
428     }
429 
430     // Re-adding after received clears the received state and makes it pending again.
431     BOOST_CHECK(pb.Add(tx));
432     BOOST_CHECK(pb.HavePendingTransactions());
433     const auto infos{pb.GetBroadcastInfo()};
434-    const auto info{FindTxInfo(infos, tx)};
435-    BOOST_CHECK(!info->received_from.has_value());
436-    BOOST_CHECK(!info->received_time.has_value());
437+    const auto& info{infos.at(tx)};
438+    BOOST_CHECK(!info.received_from.has_value());
439+    BOOST_CHECK(!info.received_time.has_value());
440 }
441 
442 BOOST_AUTO_TEST_SUITE_END()

andrewtoth commented at 5:01 pm on April 4, 2026:

I had similar thoughts, but didn’t want to bloat the PR. I will leave this for your follow-up.

vasild commented at 6:04 pm on March 26, 2026: contributor

Approach ACK d5d7c28e1b4b76daddad95a23cbf56209ca0b86e

DrahtBot added the label Needs rebase on Apr 4, 2026

test: extract find_tx_info to static helper

Non-functional refactor.

3c73bd73e5

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.

b58ddf791b

test: add unit test covering MarkReceived f60a433037

andrewtoth force-pushed on Apr 4, 2026

DrahtBot removed the label Needs rebase on Apr 4, 2026

andrewtoth commented at 4:48 pm on April 4, 2026: contributor

Thank you for the detailed review @vasild. I’ve taken many of your suggestions.

Rebased due to #34873, which meant dropping the first commit.

git range-diff 902708e..d5d7c28 master..HEAD

net: keep finished private broadcast txs in memory #34707

Reviews

Conflicts