refactor: replace manual promise with SyncWithValidationInterfaceQueue #33962

ANtutov commented at 4:12 PM on November 27, 2025: contributor

BroadcastTransaction() now waits for validation callbacks using the built-in validation_signals>SyncWithValidationInterfaceQueue() instead of creating a local std::promise and scheduling a lambda. This removes an unnecessary allocation and uses the canonical API.

DrahtBot added the label Refactoring on Nov 27, 2025

DrahtBot commented at 4:12 PM on November 27, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33962.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, maflcko, sedited
Stale ACK	furszy

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

maflcko commented at 4:29 PM on November 27, 2025: member

lgtm ACK d09e9306cab86c0917cc0ae198ef1993d764d106

in src/node/transaction.cpp:115 in d09e9306ca

 111 | @@ -115,9 +112,6 @@ TransactionError BroadcastTransaction(NodeContext& node,
 112 |                  // with a transaction to/from their wallet, immediately call some
 113 |                  // wallet RPC, and get a stale result because callbacks have not
 114 |                  // yet been processed.
 115 | -                node.validation_signals->CallFunctionInValidationInterfaceQueue([&promise] {
 116 | -                    promise.set_value();
 117 | -                });
 118 |                  callback_set = true;

rkrux commented at 10:39 AM on November 28, 2025:

Nit: The callback_set variable seems unnecessary now.

diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 05450299e5..0b6213f7fd 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -45,7 +45,6 @@ TransactionError BroadcastTransaction(NodeContext& node,
 
     Txid txid = tx->GetHash();
     Wtxid wtxid = tx->GetWitnessHash();
-    bool callback_set = false;
 
     {
         LOCK(cs_main);
@@ -102,22 +101,19 @@ TransactionError BroadcastTransaction(NodeContext& node,
                 }
                 break;
             }
-
-            if (wait_callback && node.validation_signals) {
-                // For transactions broadcast from outside the wallet, make sure
-                // that the wallet has been notified of the transaction before
-                // continuing.
-                //
-                // This prevents a race where a user might call sendrawtransaction
-                // with a transaction to/from their wallet, immediately call some
-                // wallet RPC, and get a stale result because callbacks have not
-                // yet been processed.
-                callback_set = true;
-            }
         }
     } // cs_main
 
-    if (callback_set) {
+    if (wait_callback && node.validation_signals) {
+        // For transactions broadcast from outside the wallet, make sure
+        // that the wallet has been notified of the transaction before
+        // continuing.
+        //
+        // This prevents a race where a user might call sendrawtransaction
+        // with a transaction to/from their wallet, immediately call some
+        // wallet RPC, and get a stale result because callbacks have not
+        // yet been processed.
+
         // Wait until Validation Interface clients have been notified of the
         // transaction entering the mempool.
         node.validation_signals->SyncWithValidationInterfaceQueue();

maflcko commented at 1:48 PM on December 1, 2025:

Nit: The callback_set variable seems unnecessary now.

Why would it be unnecessary now? Sure, it is harmless to call SyncWithValidationInterfaceQueue when the tx is in the mempool, but this will need to be mentioned

maflcko commented at 5:13 PM on December 3, 2025:

@ANtutov are you still working on this?

rkrux commented at 6:14 PM on December 3, 2025:

Sorry for the late reply. I don't fully understand this comment though: #33962 (review)

I suggested to remove this variable because the if condition below could be managed by the wait_callback and node.validation_signals properties directly and specially setting the callback_set variable in an if block above that doesn't do anything else now seemed unnecessary to me.

maflcko commented at 7:24 PM on December 3, 2025:

in an if block above that doesn't do anything else now seemed unnecessary to me.

What the if-else block does is explained right after the if:

        if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
            // There's already a transaction in the mempool with this txid.

rkrux commented at 7:36 PM on December 3, 2025:

Ah, there's another if above on line 66: https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/node/transaction.cpp#L66

I had missed that, good catch. I see now what you mean.

Setting callback_set in the else block seems reasonable. This suggestion can be reverted: #33962 (review).

ANtutov commented at 7:36 AM on December 4, 2025:

@ANtutov are you still working on this?

I'm sorry for the delay, had some problems with laptop, gonna correct

maflcko commented at 4:35 PM on December 4, 2025:

Please just push the prior exact commit hash.

rkrux commented at 9:49 AM on December 10, 2025:

This one to be precise: https://github.com/bitcoin/bitcoin/commit/d09e9306cab86c0917cc0ae198ef1993d764d106

ANtutov commented at 11:49 AM on December 10, 2025:

This one to be precise: d09e930

I'm grateful to you for this

rkrux commented at 10:40 AM on November 28, 2025: contributor

ACK d09e9306cab86c0917cc0ae198ef1993d764d106

Looks fine, this function doc highlights the similarity:

https://github.com/bitcoin/bitcoin/blob/808f1d972be35f4c66bdc30ab0f4064dab0c43c0/src/validationinterface.h#L208-L217

fanquake commented at 4:22 PM on November 28, 2025: member

You'll need to squash your commits: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits.

ANtutov force-pushed on Nov 28, 2025

DrahtBot added the label CI failed on Nov 28, 2025

DrahtBot commented at 4:30 PM on November 28, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19769125602/job/56649201966</sub> <sub>LLM reason (✨ experimental): CI failed due to trailing whitespace detected by lint in src/node/transaction.cpp.</sub>

<details><summary>Hints</summary>

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.

</details>

ANtutov force-pushed on Nov 28, 2025

DrahtBot removed the label CI failed on Nov 28, 2025

furszy commented at 9:26 PM on November 28, 2025: member

ACK d56718838308c474cd73930ee276f1be7334dd68

DrahtBot requested review from maflcko on Nov 28, 2025

DrahtBot requested review from rkrux on Nov 28, 2025

rkrux approved

rkrux commented at 8:14 AM on November 29, 2025: contributor

Edit: removed a-c-k in light of #33962 (review)

DrahtBot requested review from rkrux on Dec 3, 2025

ANtutov force-pushed on Dec 4, 2025

refactor: replace manual promise with SyncWithValidationInterfaceQueue e71c4df168

ANtutov force-pushed on Dec 10, 2025

rkrux approved

rkrux commented at 12:24 PM on December 10, 2025: contributor

lgtm ACK e71c4df1685131f5ab48aac6ccb07ac944e91e9f

Sorry for causing some confusion earlier.

DrahtBot requested review from furszy on Dec 10, 2025

ANtutov commented at 5:35 PM on December 10, 2025: contributor

lgtm ACK e71c4df

Sorry for causing some confusion earlier.

I'm grateful to you cause i was confused a bit

maflcko commented at 4:00 PM on January 27, 2026: member

review ACK e71c4df1685131f5ab48aac6ccb07ac944e91e9f 🌃

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e71c4df1685131f5ab48aac6ccb07ac944e91e9f 🌃
z/gawPZ53kHhS2adeoH9PHTf57rS1gUjXRRrobZqpAYTp6Bm5Vc0V3NOCJU2XeDLblm5+Xt7sHwTXsIKpfNFBg==

</details>

sedited approved

sedited commented at 4:49 PM on January 27, 2026: contributor

ACK e71c4df1685131f5ab48aac6ccb07ac944e91e9f

sedited merged this on Jan 27, 2026

sedited closed this on Jan 27, 2026