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:

#36607 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36607.svg"></sub> (private broadcast: Extra guard against unintended non-private broadcast of private transactions by optout21)
#36429 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36429.svg"></sub> ([RFC] Scheduled 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.

vasild commented at 5:09 PM on November 4, 2026:

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)

I like this. However there would be inconsistency between the rest of the statuses and received_by_us. For example it would look that we have sent it to nobody and we have received it. I imagine we could keep all statuses: original time added, all peers we have sent to and the received by us. Then we would replace the global m_max_send_attempts (1000) with a per-transaction one and whenever a transaction is re-added, bump its max from 1000 to 2000 and increment planned_sends with INITIAL_CONNECTION_COUNT (3) instead of resetting it to 3.

This way all the history from before and from after the re-add will be visible. Maybe an overkill?

frankomosh commented at 1:30 PM on November 5, 2026:

Maybe an overkill?

I tend to agree..., one cost to keep in mind: today, each tx keeps at most 1000 send records. In #35680 (review) I measured the worst-case getprivatebroadcastinfo output at about 1.15 GiB.

With a limit that grows on every re-add, a tx that is re-sent many times would keep growing with no cap. So if we go this way, maybe we still need a cap on the history

vasild commented at 4:44 PM on November 5, 2026:

Ok, then what to do with received_by_us in the case of re-add? I tend to lean towards resetting it as it is now in this PR because not resetting it would leave it inconsistent with the other fields.

optout21 commented at 2:30 PM on November 10, 2026:

After reconsidering, I'm also for keeping the current logic. My suggestion would add further complexity, and would not solve all inconsistencies. The root cause is that a submission can be repeated, but he hearing back will likely not. Resubmission is not a regular use case, but if it's done, the user has to know that the received info will only be valid since the resubmission. So for me it's fine like this.

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.

vasild commented at 1:32 PM on November 5, 2026:

Done

vasild commented at 1:32 PM on November 5, 2026:

Done

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

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

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

vasild commented at 1:32 PM on November 5, 2026:

Done

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

vasild commented at 1:31 PM on November 5, 2026:

Right, put it into a helper method. However NoMoreRetries() sounds too broad to me because we may do "no more retries" if the transaction was echoed back to us. Used PlannedSendsAtMax() as a name.

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

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>

95cc8addde

vasild force-pushed on Nov 5, 2026

vasild commented at 1:30 PM on November 5, 2026: contributor

295c961c1f156bdbbddfa0f961751d5aa371a62b...95cc8adddec6d686f7eeea9a8da3d851a1bbde54: rebase due to conflicts and address suggestions.

Still in discussion: #36477 (review)

DrahtBot removed the label Needs rebase on Nov 5, 2026