private broadcast: wait synchronously for mempool acceptance #36566

Calling BroadcastTransaction with the NO_MEMPOOL_PRIVATE_BROADCAST method skips inserting the transaction into the mempool and returns immediately. This causes problems with software that expects the transaction to be in the mempool after the function returns. For example many third-party wallets or services call sendrawtransaction and expect the transaction to be in the mempool after it returns a successful response.

This PR adds a new TransactionAddedToMempoolSync method to CValidationInterface. This is then used in BroadcastTransaction when the method is NO_MEMPOOL_PRIVATE_BROADCAST to wait for the transaction to have been accepted back into the mempool before returning. It also adds a timeout (default 10 minutes) to BroadcastTransaction and exposes it as an option for sendrawtransaction. Setting the timeout to 0 disables the wait and returns immediately, preserving the current behavior.

Synchronously waiting for mempool acceptance is also a prerequisite for adding private broadcast support to the wallet. The TransactionAddedToMempoolSync method is added specifically for future wallet broadcasting, so a broadcast caller holding cs_wallet will not timeout waiting on another background event that is also trying to acquire cs_wallet.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

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)
• #36597 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36597.svg">} (wallet: return broadcast errors from CommitTransaction by pseudoramdom)
• #36482 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36482.svg">} (wallet: Only broadcast resubmitted transactions that were added to the mempool by achow101)
• #36429 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36429.svg">} ([RFC] Delayed transaction broadcast, minimal by optout21)
• #36384 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36384.svg">} (rpc: Use self.Arg<_>(name) helper over manual and duplicate parsing by maflcko)
• #36167 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36167.svg">} ([RFC] Enable -Wunused by fanquake)
• #36002 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36002.svg">} (txindex: allow running in pruned mode by andrewtoth)
• #35728 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35728.svg">} (rpc: Properly throw on internal I/O errors in GetTransaction by maflcko)
• #35511 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35511.svg">} (RFC: consensus: Make CAmount a class by hodlinator)

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.

For example many third-party wallets or services call sendrawtransaction and expect the transaction to be in the mempool after it returns a successful response.

Can we gather a list of these?

Given the Core wallet motivation in part I think I'd like to see end to end wallet integration before stepping through this change. Naively done, it could end up with some dangerous behavior for users, such as:

• sendtoaddress hangs while Tor connects and the transaction travels out and back. Waits up to 10 minutes(?)
• The user presses Ctrl-C. The transaction is already in the wallet and queued.
• The user runs getbalance or listtransactions to check what happened. Both block on cs_wallet, so the user can't see that the first send went through. I think they'd have to inspect the private broadcast queue instead to know?
• The user runs sendtoaddress again. It blocks on cs_wallet until the first call finishes. Then coin selection skips the now-spent inputs and builds a second transaction from other coins. Both get broadcast, and the user pays twice.

sendtoaddress was never idempotent to begin with, which is kind of scary on its own, I just think this broadens the scope of possible loss through unclear UX. Users suddenly "losing" funds by stuff not being in mempool and not being accounted for will at least cause the users to slow down :/