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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33962.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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;
Nit: The callback_set variable seems unnecessary now.
0diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
1index 05450299e5..0b6213f7fd 100644
2--- a/src/node/transaction.cpp
3+++ b/src/node/transaction.cpp
4@@ -45,7 +45,6 @@ TransactionError BroadcastTransaction(NodeContext& node,
5
6 Txid txid = tx->GetHash();
7 Wtxid wtxid = tx->GetWitnessHash();
8- bool callback_set = false;
9
10 {
11 LOCK(cs_main);
12@@ -102,22 +101,19 @@ TransactionError BroadcastTransaction(NodeContext& node,
13 }
14 break;
15 }
16-
17- if (wait_callback && node.validation_signals) {
18- // For transactions broadcast from outside the wallet, make sure
19- // that the wallet has been notified of the transaction before
20- // continuing.
21- //
22- // This prevents a race where a user might call sendrawtransaction
23- // with a transaction to/from their wallet, immediately call some
24- // wallet RPC, and get a stale result because callbacks have not
25- // yet been processed.
26- callback_set = true;
27- }
28 }
29 } // cs_main
30
31- if (callback_set) {
32+ if (wait_callback && node.validation_signals) {
33+ // For transactions broadcast from outside the wallet, make sure
34+ // that the wallet has been notified of the transaction before
35+ // continuing.
36+ //
37+ // This prevents a race where a user might call sendrawtransaction
38+ // with a transaction to/from their wallet, immediately call some
39+ // wallet RPC, and get a stale result because callbacks have not
40+ // yet been processed.
41+
42 // Wait until Validation Interface clients have been notified of the
43 // transaction entering the mempool.
44 node.validation_signals->SyncWithValidationInterfaceQueue();
Nit: The
callback_setvariable 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
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.
in an
ifblock above that doesn’t do anything else now seemed unnecessary to me.
What the if-else block does is explained right after the if:
0 if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
1 // There's already a transaction in the mempool with this txid.
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).
ACK d09e9306cab86c0917cc0ae198ef1993d764d106
Looks fine, this function doc highlights the similarity:
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19769125602/job/56649201966
LLM reason (✨ experimental): CI failed due to trailing whitespace detected by lint in src/node/transaction.cpp.
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.
lgtm ACK e71c4df1685131f5ab48aac6ccb07ac944e91e9f
Sorry for causing some confusion earlier.