fix(qt): surface a mempool rejection from WalletModel::sendCoins

src/interfaces/wallet.h

@@ -250,8 +250,9 @@ class Wallet
     //! Sign every wallet-owned input of a transaction and report whether signing is complete.
     virtual util::Result<WalletTxSignResult> signTransaction(const CMutableTransaction& tx) = 0;
 
-    //! Commit transaction.
-    virtual void commitTransaction(CTransactionRef tx,
+    //! Commit transaction to the wallet and broadcast it. Returns an error
+    //! when it could not be broadcast; it stays committed to the wallet.
+    virtual util::Result<void> commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,
         WalletOrderForm order_form) = 0;
 

src/qt/sendcoinsdialog.cpp

@@ -642,8 +642,13 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
         // failed, or more signatures are needed.
         if (broadcast) {
             // now send the prepared transaction
-            model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin());
-            Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash());
+            const auto send_status{model->sendCoins(*m_current_transaction, m_coin_control->IsUsingCoinJoin())};
+            if (send_status.status == WalletModel::OK) {
+                Q_EMIT coinsSent(m_current_transaction->getWtx()->GetHash());
+            } else {
+                processSendCoinsReturn(send_status);
+                send_failure = true;
+            }
         }
     }
     if (!send_failure) {
@@ -837,7 +842,7 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn
     msgParams.second = CClientUIInterface::MSG_WARNING;
 
     // This comment is specific to SendCoinsDialog usage of WalletModel::SendCoinsReturn.
-    // All status values are used only in WalletModel::prepareTransaction()
+    // All status values except TransactionCommitFailed are used only in WalletModel::prepareTransaction()
     switch(sendCoinsReturn.status)
     {
     case WalletModel::InvalidAddress:
@@ -856,6 +861,10 @@ void SendCoinsDialog::processSendCoinsReturn(const WalletModel::SendCoinsReturn
         msgParams.first = tr("Transaction creation failed!");
         msgParams.second = CClientUIInterface::MSG_ERROR;
         break;
+    case WalletModel::TransactionCommitFailed:
+        msgParams.first = tr("The transaction was not sent: %1").arg(sendCoinsReturn.reasonCommitFailed);
+        msgParams.second = CClientUIInterface::MSG_ERROR;
+        break;
     case WalletModel::AbsurdFee:
         msgParams.first = tr("A fee higher than %1 is considered an absurdly high fee.").arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), model->wallet().getDefaultMaxTxFee()));
         break;

src/qt/test/wallettests.cpp

@@ -19,17 +19,24 @@
 #include <qt/transactionview.h>
 #include <qt/walletmodel.h>
 #include <key_io.h>
+#include <node/context.h>
+#include <node/interface_ui.h>
 #include <test/util/setup_common.h>
+#include <txmempool.h>
 #include <util/system.h>
 #include <validation.h>
+#include <validationinterface.h>
+#include <wallet/coincontrol.h>
 #include <wallet/wallet.h>
 #include <qt/overviewpage.h>
 #include <qt/receivecoinsdialog.h>
 #include <qt/recentrequeststablemodel.h>
 #include <qt/receiverequestdialog.h>
 
 #include <chrono>
+#include <functional>
 #include <memory>
+#include <optional>
 
 #include <QAbstractButton>
 #include <QAction>
@@ -38,6 +45,7 @@
 #include <QDialogButtonBox>
 #include <QListView>
 #include <QPushButton>
+#include <QSignalSpy>
 #include <QTableView>
 #include <QTextEdit>
 #include <QTimer>
@@ -54,14 +62,16 @@ using wallet::WalletRescanReserver;
 
 namespace
 {
-//! Press "Yes" or "Cancel" buttons in modal send confirmation dialog.
-void ConfirmSend(QString* text = nullptr, bool cancel = false)
+//! Press "Yes" or "Cancel" buttons in modal send confirmation dialog,
+//! after running before_answer.
+void ConfirmSend(QString* text = nullptr, bool cancel = false, std::function<void()> before_answer = {})
 {
-    QTimer::singleShot(0, [text, cancel]() {
+    QTimer::singleShot(0, [text, cancel, before_answer]() {
         for (QWidget* widget : QApplication::topLevelWidgets()) {
             if (widget->inherits("SendConfirmationDialog")) {
                 SendConfirmationDialog* dialog = qobject_cast<SendConfirmationDialog*>(widget);
                 if (text) *text = dialog->text();
+                if (before_answer) before_answer();
                 QAbstractButton* button = dialog->button(cancel ? QMessageBox::Cancel : QMessageBox::Yes);
                 button->setEnabled(true);
                 button->click();
@@ -70,8 +80,10 @@ void ConfirmSend(QString* text = nullptr, bool cancel = false)
     });
 }
 
-//! Send coins to address and return txid.
-uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount)
+//! Send coins to address and return txid. before_confirm runs once the
+//! transaction is prepared, before the send is confirmed.
+uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount,
+                  std::function<void()> before_confirm = {})
 {
     QVBoxLayout* entries = sendCoinsDialog.findChild<QVBoxLayout*>("entries");
     SendCoinsEntry* entry = qobject_cast<SendCoinsEntry*>(entries->itemAt(0)->widget());
@@ -81,7 +93,7 @@ uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDe
     boost::signals2::scoped_connection c(wallet.NotifyTransactionChanged.connect([&txid](const uint256& hash, ChangeType status) {
         if (status == CT_NEW) txid = hash;
     }));
-    ConfirmSend();
+    ConfirmSend(/*text=*/nullptr, /*cancel=*/false, std::move(before_confirm));
     bool invoked = QMetaObject::invokeMethod(&sendCoinsDialog, "sendButtonClicked", Q_ARG(bool, false));
     assert(invoked);
     return txid;
@@ -188,6 +200,109 @@ void TestGUI(interfaces::Node& node)
     QVERIFY(FindTx(*transactionTableModel, txid1).isValid());
     QVERIFY(FindTx(*transactionTableModel, txid2).isValid());
 
+    // A send that cannot be broadcast is reported as an error instead of as
+    // sent, and is abandoned so its coins are spendable again. The fee ceiling
+    // is lowered after the transaction is prepared, so only the broadcast
+    // refuses it.
+    {
+        QSignalSpy sent(&sendCoinsDialog, &SendCoinsDialog::coinsSent);
+        QSignalSpy messages(&sendCoinsDialog, &SendCoinsDialog::message);
+        const CAmount balance{walletModel.wallet().getBalance()};
+        const CAmount max_fee{wallet->m_default_max_tx_fee};
+        const uint256 refused{SendCoins(*wallet, sendCoinsDialog, PKHash(), 2 * COIN,
+                                        [&wallet] { wallet->m_default_max_tx_fee = 1; })};
+        wallet->m_default_max_tx_fee = max_fee;
+        QVERIFY(!refused.IsNull());
+        QCOMPARE(sent.count(), 0);
+        QCOMPARE(messages.count(), 1);
+        QCOMPARE(messages.first().at(2).toUInt(), static_cast<unsigned int>(CClientUIInterface::MSG_ERROR));
+        QVERIFY(!node.context()->mempool->exists(refused));
+        QCOMPARE(walletModel.wallet().getBalance(), balance);
+        // The form is kept for a retry.
+        QVBoxLayout* entries{sendCoinsDialog.findChild<QVBoxLayout*>("entries")};
+        SendCoinsEntry* entry{qobject_cast<SendCoinsEntry*>(entries->itemAt(0)->widget())};
+        QCOMPARE(entry->findChild<BitcoinAmountField*>("payAmount")->value(), 2 * COIN);
+    }
+
+    // Retrying a refused send can rebuild the identical transaction, which the
+    // wallet holds abandoned: it is committed and sent like a new one.
+    {
+        const QList<SendCoinsRecipient> recipients{
+            SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 3 * COIN, ""}};
+        WalletModelTransaction retried{recipients};
+        QCOMPARE(walletModel.prepareTransaction(retried, wallet::CCoinControl{}).status, WalletModel::OK);
+        const CAmount balance{walletModel.wallet().getBalance()};
+        const CAmount max_fee{wallet->m_default_max_tx_fee};
+        wallet->m_default_max_tx_fee = 1;
+        QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::TransactionCommitFailed);
+        wallet->m_default_max_tx_fee = max_fee;
+        QCOMPARE(walletModel.wallet().getBalance(), balance);
+        QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK);
+        QVERIFY(node.context()->mempool->exists(retried.getWtx()->GetHash()));
+        QVERIFY(walletModel.wallet().getBalance() < balance);
+
+        // Mined through another path before the wallet processed the block
+        // (this wallet does not follow the chain yet), it was sent.
+        WalletModelTransaction mined{
+            {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 4 * COIN, ""}}};
+        QCOMPARE(walletModel.prepareTransaction(mined, wallet::CCoinControl{}).status, WalletModel::OK);
+        test.CreateAndProcessBlock({CMutableTransaction{*mined.getWtx()}}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
+        QCOMPARE(walletModel.sendCoins(mined, /*fIsCoinJoin=*/false).status, WalletModel::OK);
+
+        // Committed again once mined, it stays confirmed and its inputs spent.
+        // The wallet follows the chain for this, as a loaded wallet does.
+        wallet->m_chain_notifications_handler = node.context()->chain->handleNotifications(wallet);
+        test.CreateAndProcessBlock({CMutableTransaction{*retried.getWtx()}}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
+        SyncWithValidationInterfaceQueue();
+        const CAmount mined_balance{walletModel.wallet().getBalance()};
+        QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK);
+        interfaces::WalletTxStatus status{};
+        interfaces::WalletOrderForm order_form;
+        bool in_mempool{false};
+        int num_blocks{0};
+        walletModel.wallet().getWalletTxDetails(retried.getWtx()->GetHash(), status, order_form, in_mempool, num_blocks);
+        QCOMPARE(status.depth_in_main_chain, 1);
+        QCOMPARE(walletModel.wallet().getBalance(), mined_balance);
+
+        // Relayed through another path and seen by the wallet first, it still
+        // gets what the send records about it, such as the payment message.
+        WalletModelTransaction relayed{
+            {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", 1 * COIN, "relayed elsewhere"}}};
+        QCOMPARE(walletModel.prepareTransaction(relayed, wallet::CCoinControl{}).status, WalletModel::OK);
+        bilingual_str relay_error;
+        QCOMPARE(node.broadcastTransaction(relayed.getWtx(), /*max_tx_fee=*/0, relay_error), TransactionError::OK);
+        SyncWithValidationInterfaceQueue();
+        QCOMPARE(walletModel.sendCoins(relayed, /*fIsCoinJoin=*/false).status, WalletModel::OK);
+        walletModel.wallet().getWalletTxDetails(relayed.getWtx()->GetHash(), status, order_form, in_mempool, num_blocks);
+        QVERIFY(in_mempool);
+        QCOMPARE(order_form, (interfaces::WalletOrderForm{{"Message", "relayed elsewhere"}}));
+    }
+
+    // The mempool's reason reaches the caller: of two sends prepared over the
+    // same coin, the second is refused as a conflict of the first.
+    {
+        std::optional<COutPoint> coin;
+        for (const auto& [dest, coins] : walletModel.wallet().listCoins()) {
+            for (const auto& [outpoint, out] : coins) {
+                if (!coin && !out.is_spent && out.depth_in_main_chain > 0) coin = outpoint;
+            }
+        }
+        QVERIFY(coin);
+        wallet::CCoinControl same_coin;
+        same_coin.Select(*coin);
+        same_coin.m_allow_other_inputs = false;
+        const QList<SendCoinsRecipient> recipients{
+            SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", COIN, ""}};
+        WalletModelTransaction first{recipients};
+        WalletModelTransaction conflict{recipients};
+        QCOMPARE(walletModel.prepareTransaction(first, same_coin).status, WalletModel::OK);
+        QCOMPARE(walletModel.prepareTransaction(conflict, same_coin).status, WalletModel::OK);
+        QCOMPARE(walletModel.sendCoins(first, /*fIsCoinJoin=*/false).status, WalletModel::OK);
+        const auto refused{walletModel.sendCoins(conflict, /*fIsCoinJoin=*/false)};
+        QCOMPARE(refused.status, WalletModel::TransactionCommitFailed);
+        QVERIFY2(refused.reasonCommitFailed.contains("txn-mempool-conflict"), qPrintable(refused.reasonCommitFailed));
+    }
+
     // Check current balance on OverviewPage
     OverviewPage overviewPage;
     overviewPage.setClientModel(&clientModel);

src/qt/walletmodel.cpp

@@ -324,7 +324,20 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
     return SendCoinsReturn(OK);
 }
 
-void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin)
+namespace {
+bool WasSent(interfaces::Wallet& wallet, const uint256& txid)
+{
+    interfaces::WalletTxStatus status{};
+    interfaces::WalletOrderForm order_form;
+    bool in_mempool{false};
+    int num_blocks{0};
+    wallet.getWalletTxDetails(txid, status, order_form, in_mempool, num_blocks);
+    return status.depth_in_main_chain > 0 ||
+           (status.depth_in_main_chain == 0 && (in_mempool || status.is_islocked));
+}
+} // namespace
+
+WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin)
 {
     QByteArray transaction_array; /* store serialized transaction */
 
@@ -342,7 +355,18 @@ void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoi
         }
 
         auto& newTx = transaction.getWtx();
-        wallet().commitTransaction(newTx, /*value_map=*/std::move(mapValue), std::move(vOrderForm));
+        // A refused transaction was never relayed. Abandon it, so the wallet
+        // does not rebroadcast a payment the user is told failed and its inputs
+        // are free for a retry. It cannot be abandoned if a resend got it into
+        // the mempool or a block, or it was InstantSend-locked, meanwhile, and
+        // then it was sent, or if a spend of the same inputs has confirmed, and
+        // then it was not.
+        const auto res{wallet().commitTransaction(newTx, /*value_map=*/std::move(mapValue), std::move(vOrderForm))};
+        if (!res && (wallet().abandonTransaction(newTx->GetHash()) || !WasSent(wallet(), newTx->GetHash()))) {
+            const bilingual_str error{util::ErrorString(res)};
+            return SendCoinsReturn(TransactionCommitFailed,
+                                   QString::fromStdString(error.translated.empty() ? error.original : error.translated));
+        }
 
         CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
         ssTx << *newTx;
@@ -375,6 +399,7 @@ void WalletModel::sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoi
     }
 
     checkBalanceChanged(m_wallet->getBalances()); // update balance immediately, otherwise there could be a short noticeable delay until pollBalanceChanged hits
+    return SendCoinsReturn(OK);
 }
 
 OptionsModel* WalletModel::getOptionsModel() const

src/qt/walletmodel.h

@@ -64,6 +64,7 @@ class WalletModel : public QObject
         AmountWithFeeExceedsBalance,
         DuplicateAddress,
         TransactionCreationFailed, // Error returned when wallet is still locked
+        TransactionCommitFailed,
         AbsurdFee
     };
 
@@ -101,8 +102,9 @@ class WalletModel : public QObject
     // prepare transaction for getting txfee before sending coins
     SendCoinsReturn prepareTransaction(WalletModelTransaction &transaction, const wallet::CCoinControl& coinControl);
 
-    // Send coins to a list of recipients
-    void sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin);
+    // Send coins to a list of recipients; TransactionCommitFailed when the
+    // transaction could not be broadcast, in which case it is abandoned
+    SendCoinsReturn sendCoins(WalletModelTransaction& transaction, bool fIsCoinJoin);
 
     // Wallet encryption
     bool setWalletEncrypted(const SecureString& passphrase);

src/wallet/interfaces.cpp

@@ -533,12 +533,12 @@ class WalletImpl : public Wallet
             errors.push_back(std::move(error));
         return WalletTxSignResult{MakeTransactionRef(std::move(signed_tx)), complete, std::move(errors)};
     }
-    void commitTransaction(CTransactionRef tx,
+    util::Result<void> commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,
         WalletOrderForm order_form) override
     {
         LOCK(m_wallet->cs_wallet);
-        m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form));
+        return m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form));
     }
     bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); }
     bool transactionCanBeResent(const uint256& txid) override { return m_wallet->TransactionCanBeResent(txid); }