private broadcast: show finished transactions in getprivatebroadcastinfo #36477

vasild commented at 3:16 PM on October 8, 2026: contributor

For finished transactions, this allows the inspection of when and to whom the transaction was sent as well as when/if it was echoed from the network.

DrahtBot added the label Private Broadcast on Oct 8, 2026

DrahtBot commented at 3:16 PM on October 8, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36477
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36477.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36477#author-ralph

Reviews

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

Type	Reviewers
Concept ACK	optout21

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36628 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36628.svg"> (net: reimplement private broadcast as jobs isolated from the node by instagibbs)
#36608 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36608.svg"> (Reimplement private broadcast as node state-less jobs + package relay support by instagibbs)
#36607 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36607.svg"> (private broadcast: Extra guard against unintended non-private broadcast of private transactions by optout21)
#36429 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36429.svg"> ([RFC] Delayed transaction broadcast, minimal by optout21)

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.

vasild force-pushed on Oct 8, 2026

DrahtBot added the label CI failed on Oct 8, 2026

DrahtBot removed the label CI failed on Oct 8, 2026

in src/private_broadcast.cpp:27 in bce73a17fd outdated

  29 |  
  30 |      if (const auto it{m_transactions.find(tx)}; it != m_transactions.end()) {
  31 | -        if (it->second.send_statuses.size() < m_max_send_attempts) return AddResult::AlreadyPresent;
  32 | +        auto& state{it->second};
  33 | +
  34 | +        if (!IsFinished(state)) { // We are still trying

frankomosh commented at 6:42 PM on October 19, 2026:

A tx that used all its send attempts, but never came back from the network, can no longer be sent again. Now Add() can only restarts a tx that is resolved. But GetStale() skips such a tx, so nothing marks it resolved.

Are we getting rid of the 'retry' feature..?

vasild commented at 1:19 PM on October 22, 2026:

No, that was not intentional. Fixed. Please double check again, this has become non-trivial.

optout21 commented at 2:40 PM on October 22, 2026:

Related, what happens when a previously-finished transaction is requested to be private-broadcast again?

Add() sees it finished, and clears received_by_us and resolved
It is sent out again (to 3 peers)
It never gets re-added to our mempool, as it already there. MarkResolved is not called again.
The transaction may be taken as failed
Final state reported in RPC: attempts_remaining 0 and no received_by_us

Is this intended this way?

vasild commented at 2:50 PM on October 23, 2026:

It never gets re-added to our mempool, as it already there. MarkResolved is not called again.

Note that MarkResolved() is called after we receive the transaction but before we try to put it in the mempool:

https://github.com/bitcoin/bitcoin/blob/df0dc1e2d2059213e49a9f115fd55a9f0119d26c/src/net_processing.cpp#L4734

https://github.com/bitcoin/bitcoin/blob/df0dc1e2d2059213e49a9f115fd55a9f0119d26c/src/net_processing.cpp#L4770

So, it will be marked as resolved if we receive it, even if it wouldn't enter the mempool because it is already there.

But... would we receive it (a second time)? After the initial receive we would send it to all peers. So all of them know we have it. They wouldn't INV it to us anymore and even if they do we wouldn't request it (with GETDATA) because we have it in our mempool already.

So, yes, there is no reliable way to know if the second broadcast succeeded. This is the same in master right now. There may be new peers in the meantime that are unaware that we have the transaction and they will INV it to us (if it even starts propagating a second time through the network). Something to consider, separately from this PR. I do not think this PR changes the behavior.

optout21 commented at 4:35 PM on October 24, 2026:

True, upon a re-request there new new relay will happen. Other nodes and this node will have the TX already in their mempool. As a result, "received_by_us" will never be set, and it will be not possible to decide/observe the success of the second broadcast.

My point was, that if the TX is already in our mempool, and for some strange reason it is relayed back to us after the re-request, our node will not request GETDATA (because TX is in the mempool), the handler will not be called, and the "receive" flags will remain empty.

The behavior on master is the same, except that the "received_by_us" flag is a new addition.

I can see these cases:

Already in the mempool, and no re-relay happens (the likely case): The "received_by_us" stays empty
Already in the mempool, but re-relay happens: The "received_by_us" stays empty -- AlreadyHaveTx is true, so no GETDATA is sent, no TX arrives, and the TX handler (MarkResolved) is never called.
Not in the mempool and re-relay happens: "received_by_us" is set.

Case 2. could be argued about, but I think it's fine. The root of the issue is that upon re-request there will be no new relay.

One idea that we could consider though: upon a re-request don't clean the "received" flag if the TX is already in the mempool (persist that info that we have received it previously; and do perform the new sendouts).

frankomosh commented at 6:57 PM on October 27, 2026:

No, that was not intentional. Fixed. Please double check again, this has become non-trivial.

Looks good now.

Also noticed a good side effect: a tx that used all its attempts can now be removed when the queue is full. Before, it blocked the queue.

in src/private_broadcast.cpp:80 in bce73a17fd outdated

  77 |      if (it == m_transactions.end()) return false;
  78 | -    it->second.resolved = true;
  79 | +    auto& state{it->second};
  80 | +    state.resolved = true;
  81 | +    if (received_from.has_value()) {
  82 | +        state.received_by_us.emplace(received_from.value(), NodeClock::now());

frankomosh commented at 6:44 PM on October 19, 2026:

If another peer sends us the same tx later, this line replaces the saved peer and time. So received_by_us may not show the first peer that sent it back.

Should we keep the first one?

vasild commented at 1:23 PM on October 22, 2026:

Right, changed it to only set received_by_us if it is not set.

frankomosh commented at 7:03 PM on October 27, 2026:

Checked on 295c961.., looks good.

Maybe later we could add a test for it. For now, removing && !state.received_by_us.has_value()(mutant), and all tests still unnecesarily pass (unit, fuzz and functional).

frankomosh commented at 6:48 PM on October 19, 2026: contributor

I guess this is a continuation of #34707. Could you share what was added or left out compared to it?

vasild force-pushed on Oct 22, 2026

vasild commented at 1:32 PM on October 22, 2026: contributor

bce73a17fd2754f5fad9248bdcd13b3bad0ae023...0aa81936cf2d51f55eeaca26fd036ad8d5fdc09f: rebase and address suggestions from @frankomosh and https://review.fish.foo/traces/36477-bce73a17fd2754f5fad9248bdcd13b3bad0ae023-991-2.html

PS

I guess this is a continuation of #34707. Could you share what was added or left out compared to it?

I am unsure. Better review this one as a fresh new unrelated one.

optout21 commented at 2:17 PM on October 22, 2026: contributor

Concept ACK Keeping info about completed transaction is useful for the user. This info may be useful for the feature implementation as well (e.g. see #36607 for a proposal how to refuse non-private broadcast of private transactions).

in src/rpc/mempool.cpp:184 in 0aa81936cf

 179 | @@ -180,21 +180,22 @@ static RPCMethod getprivatebroadcastinfo()
 180 |      return RPCMethod{
 181 |          "getprivatebroadcastinfo",
 182 |          "Returns information about transactions tracked for private broadcast.\n"
 183 | -        "Transactions that have reached the send-attempt limit remain in the result with attempts_remaining=0.\n"
 184 | +        "Transactions that have reached the send-attempt limit, or that have been echoed from\n"
 185 | +        "the network remain in the result with attempts_remaining=0.\n"

optout21 commented at 2:31 PM on October 22, 2026:

0aa8193 private broadcast: show finished transactions in getprivatebroadcastinfo:

Minor inaccuracy: echoed transaction may still have attempts_remaining > 0 initially. Maybe the text should list the stored categories: transactions being actively sent, sent but not echoed back, and echoed back.

vasild commented at 3:02 PM on October 23, 2026:

Right. This text is to explain that some transactions will remain (because all others would be removed). Now I think it is not necessary because all will remain, so no need to explain that some particular ones will remain. Dropped the sentence.

in src/rpc/mempool.cpp:269 in 0aa81936cf

optout21 commented at 2:33 PM on October 22, 2026:

0aa8193 private broadcast: show finished transactions in getprivatebroadcastinfo:

This should also be updated.

vasild commented at 3:04 PM on October 23, 2026:

Shortened to just:

"abortprivatebroadcast",
"Abort private broadcast of a transaction and remove it from the queue.\n"
"This method is only available when running with -privatebroadcast enabled.\n",

good?

optout21 commented at 11:59 AM on October 28, 2026:

295c961 private broadcast: show finished transactions in getprivatebroadcastinfo:

Minor: Comment doc should be updated.

optout21 commented at 12:02 PM on October 28, 2026:

295c961 private broadcast: show finished transactions in getprivatebroadcastinfo:

Nit: The "Trimmed by Remove() and Add() cleanup" part is not fully correct now.

optout21 commented at 2:41 PM on October 22, 2026: contributor

Performed a review pass (0aa81936cf2d51f55eeaca26fd036ad8d5fdc09f), looks good generally; left some comments.

private broadcast: show finished transactions in getprivatebroadcastinfo

For finished transactions, this allows the inspection of when and to
whom the transaction was sent as well as when/if it was echoed from
the network.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

295c961c1f

vasild force-pushed on Oct 23, 2026

vasild commented at 3:00 PM on October 23, 2026: contributor

0aa81936cf2d51f55eeaca26fd036ad8d5fdc09f...295c961c1f156bdbbddfa0f961751d5aa371a62b: address suggestions

in src/private_broadcast.h:79 in 295c961c1f

  74 | +    };
  75 | +
  76 |      struct TxBroadcastInfo {
  77 |          CTransactionRef tx;
  78 |          NodeClock::time_point time_added;
  79 |          /// Number of additional send attempts allowed for this transaction (0 if exhausted).

optout21 commented at 12:00 PM on October 28, 2026:

295c961 private broadcast: show finished transactions in getprivatebroadcastinfo:

Nit: "(0 if exhausted or completed)"

in src/private_broadcast.cpp:20 in 295c961c1f

  21 | -               std::ranges::all_of(state.send_statuses, [](const auto& status) { return status.disconnected; });
  22 | -    });
  23 | +
  24 | +    const auto IsFinished{[this](const TxSendStatus& state) {
  25 | +        return !IsPending(state) && // All planned sends are completed.
  26 | +               (state.resolved || state.planned_sends >= m_max_send_attempts) && // No more planned sends will be added.

optout21 commented at 12:07 PM on October 28, 2026:

295c961 private broadcast: show finished transactions in getprivatebroadcastinfo:

Nit: the expression state.planned_sends >= m_max_send_attempts appears 4 times, maybe it could be put into a helper method (e.g. NoMoreRetries()).

optout21 commented at 12:11 PM on October 28, 2026: contributor

Reviewed again, left some nits. I'm happy to revisit soon! Friendly reminder one previous question of mine has not been fully addressed yet.

DrahtBot added the label Needs rebase on Oct 29, 2026

DrahtBot commented at 4:46 PM on October 29, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.