wallet: return broadcast errors from CommitTransaction #36597

Part of https://github.com/bitcoin-core/gui-qml/issues/1008.

The QML GUI shows "Transaction sent" even when the node rejected the transaction, because CWallet::CommitTransaction() only logs the rejection and returns void. This PR bubbles up the submission result through interfaces::Wallet::commitTransaction() so the GUI can report it.

Key changes

• Introduce TransactionBroadcastError, holding a TransactionError code and an untranslated message.
• Commit/Broadcast functions returns Expected<void, TransactionBroadcastError>.
  • Remove TransactionError::OK, a success value in an error enum.
• Add a SUBMISSION_SKIPPED code for when the wallet doesn't submit the transaction (broadcasting disabled, abandoned, coinbase, confirmed or conflicted). SubmitTxMemoryPoolAndRelay() previously returned false in these cases.

Note

• Existing callers (wallet RPCs, Qt widgets, fee bumper) ignore the new result, so their behavior and RPC responses don't change.
• commitBumpTransaction() similarly reports success for a rejected replacement. This will be addressed in a follow-up PR.
• gui-qml will need to update its interfaces::Wallet mock for the new commitTransaction() signature.

<!--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/36597
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36597.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36597#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36566 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36566.svg">} (private broadcast: wait synchronously for mempool acceptance by andrewtoth)
• #36520 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36520.svg">} (interfaces: Avoid node crashes from invalid Chain IPC calls by ryanofsky)
• #36481 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36481.svg">} (wallet, rpc, gui: Allow users to manually rebroadcast a wallet transaction by achow101)
• #36429 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36429.svg">} ([RFC] Delayed transaction broadcast, minimal by optout21)
• #36380 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36380.svg">} (wallet: extend -walletbroadcast to allow initial send only by vasild)
• #36070 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36070.svg">} (wallet: Add deriveHDKey interface by PraneethGunas)
• #36002 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36002.svg">} (txindex: allow running in pruned mode by andrewtoth)
• #35786 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35786.svg">} (wallet: drop spent parents redundant cache invalidation and notification by furszy)
• #35728 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35728.svg">} (rpc: Properly throw on internal I/O errors in GetTransaction by maflcko)
• #35302 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35302.svg">} (Silent Payments: Sending (take 2) by Eunovo)
• #33117 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33117.svg">} (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK

Concept ACK 4e3d1675597173075a7efd9b3905c928b2be8de5

With -walletbroadcast=0, commitTransaction now returns an error (SUBMISSION_SKIPPED), even though the user chose not to broadcast. Will the QML GUI treat that as "saved, not broadcast" rather than as a failure?

Concept ACK 4e3d167

With -walletbroadcast=0, commitTransaction now returns an error (SUBMISSION_SKIPPED), even though the user chose not to broadcast. Will the QML GUI treat that as "saved, not broadcast" rather than as a failure?

Yes, that's the intent. SUBMISSION_SKIPPED is a separate code precisely so a caller can differentiate between "the wallet didn't submit it" and "the node rejected it" and act accordingly.

Can you explain why this would be needed? If the wallet creates a consensus-invalid (permanently invalid) transaction, that seems like a severe bug that should be fixed instead. If the wallet creates a tx which permanently fails the mempool policy, that seems like an equally severe bug that should be fixed instead. If the wallet creates a tx that undershoots the fee, that seems irrelevant and can equally happen when the tx was accepted to the mempool, but then dropped? If the mempool min fee is 0.677 sat/vB, but the user explicitly asked for a tx at 0.1, it seems like a case that the GUI should not allow or display a warning while creating the tx. Some feedback directly after the tx was created seems misplaced? If this was needed, the feedback could be done async?

If the wallet creates a consensus-invalid (permanently invalid) transaction, that seems like a severe bug that should be fixed instead. If the wallet creates a tx which permanently fails the mempool policy, that seems like an equally severe bug that should be fixed instead.

The GUI can import a PSBT that spends the wallet's coins, sign it with the wallet and commit it. Its inputs and fee were chosen elsewhere and none of createTransaction()'s checks ran, so a rejection (fee above -maxtxfee, a non-standard output, a mempool conflict) isn't a wallet bug and can't be caught at creation. An early rejection with the reason at commitTransaction() time seems useful.

If the wallet creates a tx that undershoots the fee, that seems irrelevant and can equally happen when the tx was accepted to the mempool, but then dropped? If the mempool min fee is 0.677 sat/vB, but the user explicitly asked for a tx at 0.1, it seems like a case that the GUI should not allow or display a warning while creating the tx.

Partially agree. The GUI should catch this before the user confirms. Currently, it looks like GetMinimumFeeRate() only checks -mintxfee/-minrelaytxfee when the user has set the fee rate manually. Maybe GUI should also check for mempool minimum.

Some feedback directly after the tx was created seems misplaced? If this was needed, the feedback could be done async?

A synchronous error when submission fails right away seems more useful than returning void, which is what we do today. Async feedback sounds like a good idea. Something the GUI can subscribe to and be notified about (a later eviction). I see it as complementing the sync error rather than replacing it.

I ran some mutation testing on the changed files. One surviving mutant seems relevant here:

<details> <summary>Surviving mutant</summary>

wallet/interfaces.cpp: returning success on MEMPOOL_REJECTED survives. CommitTransactionErrors covers the rejection on CWallet, but the interface test commit_transaction (the path gui-qml#1008 uses) only covers SUBMISSION_SKIPPED and success.

-        return m_wallet->CommitTransaction(std::move(tx), /*replaces_txid=*/std::nullopt, /*comment=*/std::nullopt, /*comment_to=*/std::nullopt, messages);
+        auto result{m_wallet->CommitTransaction(std::move(tx), /*replaces_txid=*/std::nullopt, /*comment=*/std::nullopt, /*comment_to=*/std::nullopt, messages)};
+        // A transaction rejected by the mempool is still stored in the wallet and will be resubmitted later.
+        if (!result && result.error().code == node::TransactionError::MEMPOOL_REJECTED) return {};
+        return result;

</details>