From 9773acd443dc948191e15c1c1ca3a5b47a4dd2b5 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 16 Oct 2026 00:52:43 -0500 Subject: [PATCH 1/4] fix(qt): surface a mempool rejection from WalletModel::sendCoins When the mempool refused a transaction on commit, WalletModel::sendCoins could not tell. The send dialog then announced the payment as sent and cleared the form, and the refused transaction stayed in the wallet as a pending debit that was never relayed; the wallet kept rebroadcasting it. CWallet::CommitTransaction and interfaces::Wallet::commitTransaction now return the broadcast error (#7763), and the GUI acts on it. sendCoins returns a SendCoinsReturn, and on a refusal it now abandons the transaction and returns the new TransactionCommitFailed code with the reason. The send dialog shows it as an error, emits no coinsSent and keeps the form, so the user can retry. If the refused transaction cannot be abandoned, the wallet's own state decides: in the mempool or a block, or InstantSend-locked (AbandonTransaction refuses those too), it went out and is reported as sent; otherwise (a spend of the same inputs confirmed in between, so it is conflicted) it is reported as not sent, with the mempool's reason. Behaviour change, deliberately: the GUI abandons a refused send whatever the reason, including transient ones such as a mempool minimum fee or a missing parent. BroadcastTransaction relays only what the mempool accepted, so a refused transaction has reached no peer and abandoning it has no network effect. Keeping it instead would let the wallet's periodic resend broadcast a payment after the user was told it failed and may have sent it again, paying twice. A transient refusal is therefore not retried automatically; the kept form makes the retry one click. Test (test_dash-qt WalletTests, existing TestGUI scenario): a send whose broadcast is refused (the wallet's fee ceiling lowered after the transaction is prepared) shows an error, emits no coinsSent, is not in the mempool, leaves the balance where it was, because the abandoned transaction's coins are spendable again, and keeps the amount in the form. A refused send retried with the identical transaction, which the wallet holds abandoned, is committed and reaches the mempool; committed again after it is mined, it is reported as sent, stays confirmed and the balance does not change. A transaction mined before the wallet processed the block is reported as sent. A transaction relayed through another path and seen by the wallet first is reported as sent and keeps its payment message. A send conflicting with an earlier one over the same coin reports the mempool's txn-mempool-conflict reason. WalletTests runs on cocoa, xcb and windows; it is skipped on macOS's minimal platform. Co-Authored-By: Claude Opus 5.5 --- src/qt/sendcoinsdialog.cpp | 15 ++++- src/qt/test/wallettests.cpp | 127 ++++++++++++++++++++++++++++++++++-- src/qt/walletmodel.cpp | 29 +++++++- src/qt/walletmodel.h | 6 +- 4 files changed, 164 insertions(+), 13 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 9be0ba7b22f7..b4a16cdd8145 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -646,8 +646,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) { @@ -841,7 +846,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: @@ -860,6 +865,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; diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 4fce62430de2..9a8f332de37a 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -20,9 +20,14 @@ #include #include #include +#include +#include #include +#include #include #include +#include +#include #include #include #include @@ -31,7 +36,9 @@ #include #include +#include #include +#include #include #include @@ -40,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -56,14 +64,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 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(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(); @@ -72,8 +82,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 before_confirm = {}) { QVBoxLayout* entries = sendCoinsDialog.findChild("entries"); SendCoinsEntry* entry = qobject_cast(entries->itemAt(0)->widget()); @@ -83,7 +95,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; @@ -190,6 +202,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(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("entries")}; + SendCoinsEntry* entry{qobject_cast(entries->itemAt(0)->widget())}; + QCOMPARE(entry->findChild("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 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 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 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); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 5e146b6106dc..f8c77be578eb 100644 --- a/src/qt/walletmodel.cpp +++ b/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 diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index c540b0a0a2e4..48c837d8ae15 100644 --- a/src/qt/walletmodel.h +++ b/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); From 856eb0491c4a998153b60bcd7db2478540ce8fd8 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 2 Nov 2026 20:38:33 +0000 Subject: [PATCH 2/4] refactor(wallet): keep the node's broadcast result in interfaces::Chain interfaces::Chain::broadcastTransaction collapsed node::BroadcastTransaction's TransactionError into a bool, so CWallet::CommitTransaction could not tell a transaction already in the chain from a refusal. It worked around that by treating a failure without a reason as "maybe in the chain" and looking the outputs up again with Chain::findCoins, which reads the mempool-backed view rather than the chain UTXO view BroadcastTransaction checks, under a separate lock. A refusal without a reason that was not in the chain, such as the -maxtxfee check, got a generic "Transaction could not be broadcast". Chain::broadcastTransaction now returns the TransactionError, as interfaces::Node::broadcastTransaction already does, and CWallet::SubmitTxMemoryPoolAndRelay passes it on as std::optional, nullopt when CanTxBeResent() keeps the transaction from the node. util::Result here carries only a message, so it cannot keep ALREADY_IN_CHAIN apart from a refusal. CommitTransaction reports OK and ALREADY_IN_CHAIN as committed, leaving the state of a transaction already in the chain for the block notification to set, and fills a missing reason from TransactionErrorString(), so a send over the fee ceiling now says so. ResendTransaction and ResubmitWalletTransactions count only OK as submitted, as before. Test: the TestGUI retry of a refused send checks that the refusal carries the MAX_FEE_EXCEEDED diagnostic; the existing check that a transaction mined before the wallet processed the block is reported as sent now exercises ALREADY_IN_CHAIN. Co-Authored-By: Claude Opus 5.5 --- src/interfaces/chain.h | 6 ++++-- src/node/interfaces.cpp | 8 ++----- src/qt/test/wallettests.cpp | 6 +++++- src/wallet/test/wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 37 +++++++++++++------------------- src/wallet/wallet.h | 7 ++++-- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index fe711c74d265..bf23fb88db6f 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -7,6 +7,7 @@ #include #include // For CTransactionRef +#include // For TransactionError #include // For util::SettingsValue #include @@ -238,8 +239,9 @@ class Chain //! Transaction is added to memory pool, if the transaction fee is below the //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true. - //! Return false if the transaction could not be added due to the fee or for another reason. - virtual bool broadcastTransaction(const CTransactionRef& tx, + //! Return the node's result: OK when it is in the mempool, ALREADY_IN_CHAIN when + //! its outputs are already in the chain, or why it could not be added. + virtual TransactionError broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 15787b5bf021..7f14eaf0a3a0 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1567,13 +1567,9 @@ class ChainImpl : public Chain auto it = m_node.mempool->GetIter(txid); return it && (*it)->GetCountWithDescendants() > 1; } - bool broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) override + TransactionError broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, bool relay, bilingual_str& err_string) override { - const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false); - // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures. - // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures - // that Chain clients do not need to know about. - return TransactionError::OK == err; + return BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false); } void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize, CAmount* ancestorfees) override { diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 9a8f332de37a..e273f61633c3 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -236,8 +237,11 @@ void TestGUI(interfaces::Node& node) 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); + const auto over_max_fee{walletModel.sendCoins(retried, /*fIsCoinJoin=*/false)}; wallet->m_default_max_tx_fee = max_fee; + QCOMPARE(over_max_fee.status, WalletModel::TransactionCommitFailed); + QCOMPARE(over_max_fee.reasonCommitFailed, + QString::fromStdString(TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original)); QCOMPARE(walletModel.wallet().getBalance(), balance); QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK); QVERIFY(node.context()->mempool->exists(retried.getWtx()->GetHash())); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 8f41cc393806..20db3c28a0c5 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -1008,7 +1008,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) auto block_tx = TestSimpleSpend(*m_coinbase_txns[0], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error) == TransactionError::OK); // Reload wallet and make sure new transactions are detected despite events @@ -1049,7 +1049,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup) block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); - BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error) == TransactionError::OK); SyncWithValidationInterfaceQueue(); }); wallet = TestLoadWallet(context); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8d151593bc46..1acf2f03b77d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1424,7 +1424,7 @@ bool CWallet::ResendTransaction(const uint256& hashTx) CWalletTx& wtx = it->second; bilingual_str unused_err_string; - return SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true); + return SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true) == TransactionError::OK; } void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) @@ -2200,11 +2200,11 @@ bool CWallet::CanTxBeResent(const CWalletTx& wtx) const !IsTxLockedByInstantSend(wtx); } -bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const +std::optional CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const { AssertLockHeld(cs_wallet); - if (!CanTxBeResent(wtx)) return false; + if (!CanTxBeResent(wtx)) return std::nullopt; // Submit transaction to mempool for relay WalletLogPrintf("Submitting wtx %s to mempool for relay\n", wtx.GetHash().ToString()); @@ -2217,8 +2217,8 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_stri // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect. // If transaction was previously in the mempool, it should be updated when // TransactionRemovedFromMempool fires. - bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); - if (ret) wtx.m_state = TxStateInMempool{}; + const TransactionError ret{chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string)}; + if (ret == TransactionError::OK) wtx.m_state = TxStateInMempool{}; return ret; } @@ -2302,7 +2302,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) // Now try submitting the transactions to the memory pool and (optionally) relay them. for (auto wtx : to_submit) { bilingual_str unused_err_string; - if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay)) ++submitted_tx_count; + if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay) == TransactionError::OK) ++submitted_tx_count; } } // cs_wallet @@ -2569,22 +2569,15 @@ util::Result CWallet::CommitTransaction(CTransactionRef tx, mapValue_t map } bilingual_str err_string; - if (!SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)) { - // A transaction already in the chain, e.g. mined through another path - // before this wallet processed its block, was sent. BroadcastTransaction - // refuses it without a reason, having found its outputs. - if (err_string.empty()) { - std::map outputs; - for (uint32_t i{0}; i < tx->vout.size(); ++i) outputs.emplace(COutPoint{tx->GetHash(), i}, Coin{}); - chain().findCoins(outputs); - if (std::any_of(outputs.begin(), outputs.end(), [](const auto& output) { return !output.second.IsSpent(); })) return {}; - } - WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string.original); - // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. - // Not every refusal carries a reason, e.g. the -maxtxfee check in BroadcastTransaction. - return util::Error{err_string.empty() ? _("Transaction could not be broadcast") : err_string}; - } - return {}; + const auto broadcast{SubmitTxMemoryPoolAndRelay(*wtx, err_string, true)}; + // A transaction already in the chain, e.g. mined through another path + // before this wallet processed its block, was sent. + if (broadcast == TransactionError::OK || broadcast == TransactionError::ALREADY_IN_CHAIN) return {}; + // Not every refusal carries the mempool's reason, e.g. the -maxtxfee check in BroadcastTransaction. + if (err_string.empty()) err_string = broadcast ? TransactionErrorString(*broadcast) : _("Transaction could not be broadcast"); + WalletLogPrintf("CommitTransaction(): Transaction cannot be broadcast immediately, %s\n", err_string.original); + // TODO: if we expect the failure to be long term or permanent, instead delete wtx from the wallet and return failure. + return util::Error{err_string}; } DBErrors CWallet::LoadWallet() diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 02af32e21391..45699678003b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -854,8 +854,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Will SubmitTxMemoryPoolAndRelay() consider wtx if supplied */ bool CanTxBeResent(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** Pass this transaction to node for mempool insertion and relay to peers if flag set to true */ - bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const + /** + * Pass this transaction to node for mempool insertion and relay to peers if flag set to true. + * @return the node's broadcast result, or nullopt if CanTxBeResent() kept wtx from the node. + */ + std::optional SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, bilingual_str& err_string, bool relay) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool DummySignTx(CMutableTransaction &txNew, const std::vector &txouts, const CCoinControl* coin_control = nullptr) const; From f068929cd8735b4c2f10cf68d80388e2c289e69e Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 2 Nov 2026 23:48:45 +0000 Subject: [PATCH 3/4] test(qt): cover a refused send in an avoid_reuse wallet With avoid_reuse set, AddToWallet marks a send's input addresses as used before the broadcast, and AbandonTransaction does not clear the mark. After sendCoins abandons a refused send, the balance therefore leaves out every coin at those addresses, and a send above what remains fails the GUI's balance check, although the coins are spendable again. Pin that in TestGUI, so a fix in AbandonTransaction (#8017) has to update it, and check that coin control can still spend the refused send's coin. Co-Authored-By: Claude Opus 5.5 --- src/qt/test/wallettests.cpp | 53 +++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index e273f61633c3..ce0076d8a3f5 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -57,7 +58,9 @@ using wallet::AddWallet; using wallet::CWallet; using wallet::CreateMockableWalletDatabase; +using wallet::GetBalance; using wallet::RemoveWallet; +using wallet::WALLET_FLAG_AVOID_REUSE; using wallet::WALLET_FLAG_DESCRIPTORS; using wallet::WalletContext; using wallet::WalletDescriptor; @@ -309,6 +312,56 @@ void TestGUI(interfaces::Node& node) QVERIFY2(refused.reasonCommitFailed.contains("txn-mempool-conflict"), qPrintable(refused.reasonCommitFailed)); } + // With avoid_reuse, a refused send's input address stays marked as used + // after the abandon, as for any abandoned transaction. The balance leaves + // out every coin at that address, so a send above what remains fails the + // balance check, although the coins are spendable again. + { + // The wallet follows the chain by now. Let it process the earlier sends' + // mempool notifications first, or they would mark their inputs as used. + SyncWithValidationInterfaceQueue(); + wallet->SetWalletFlag(WALLET_FLAG_AVOID_REUSE); + std::optional coin; + for (const auto& [dest, coins] : walletModel.wallet().listCoins()) { + for (const auto& [outpoint, out] : coins) { + if (!coin && out.depth_in_main_chain > 0 && + out.txout.scriptPubKey == GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())) { + coin = outpoint; + } + } + } + QVERIFY(coin); + wallet::CCoinControl one_coin; + one_coin.Select(*coin); + one_coin.m_allow_other_inputs = false; + const QList recipients{ + SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", COIN, ""}}; + WalletModelTransaction refused{recipients}; + QCOMPARE(walletModel.prepareTransaction(refused, one_coin).status, WalletModel::OK); + const CAmount max_fee{wallet->m_default_max_tx_fee}; + wallet->m_default_max_tx_fee = 1; + QCOMPARE(walletModel.sendCoins(refused, /*fIsCoinJoin=*/false).status, WalletModel::TransactionCommitFailed); + wallet->m_default_max_tx_fee = max_fee; + const CAmount unused{walletModel.wallet().getBalance()}; + const CAmount spendable{GetBalance(*wallet, /*min_depth=*/0, /*avoid_reuse=*/false).m_mine_trusted}; + QVERIFY(unused + COIN < spendable); + qApp->processEvents(); + walletModel.pollBalanceChanged(); + QCOMPARE(walletModel.getCachedBalance().balance, unused); + WalletModelTransaction over_unused{ + {SendCoinsRecipient{QString::fromStdString(EncodeDestination(PKHash())), "", unused + COIN, ""}}}; + QCOMPARE(walletModel.prepareTransaction(over_unused, wallet::CCoinControl{}).status, WalletModel::AmountExceedsBalance); + // Coin control can still pick the refused send's coin. + WalletModelTransaction retried{recipients}; + QCOMPARE(walletModel.prepareTransaction(retried, one_coin).status, WalletModel::OK); + QCOMPARE(walletModel.sendCoins(retried, /*fIsCoinJoin=*/false).status, WalletModel::OK); + QVERIFY(node.context()->mempool->exists(retried.getWtx()->GetHash())); + // Cached balances were computed under the flag. + wallet->UnsetWalletFlag(WALLET_FLAG_AVOID_REUSE); + wallet->MarkDirty(); + qApp->processEvents(); + } + // Check current balance on OverviewPage OverviewPage overviewPage; overviewPage.setClientModel(&clientModel); From d4e5b9b71c8f5cd4612d2454031c1744a845f6a8 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 3 Nov 2026 14:23:58 +0000 Subject: [PATCH 4/4] fix(qt): no "Sent transaction" notification for a refused send CommitTransaction adds a send to the wallet before it broadcasts it, so a send the mempool refuses still gets a new row in the transaction list, and WalletView raised the tray's "Sent transaction" notification for it next to the error dialog. WalletModel::sendCoins abandons such a send, and the table model receives the new row on a queued connection, so by then it is already abandoned. Skip the notification for a row that is abandoned when it arrives. The row stays in the history. If the status can't be read without blocking, the notification is shown as before. Co-Authored-By: Claude Opus 5.5 --- src/qt/test/wallettests.cpp | 9 +++++++++ src/qt/walletview.cpp | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index ce0076d8a3f5..bb65763fa3bc 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -189,6 +190,9 @@ void TestGUI(interfaces::Node& node) WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel); sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); + WalletView walletView(&walletModel, nullptr); + walletView.setClientModel(&clientModel); + QSignalSpy notified(&walletView, &WalletView::incomingTransaction); // Update walletModel cached balance which will trigger an update for the 'labelBalance' QLabel. walletModel.pollBalanceChanged(); @@ -203,6 +207,7 @@ void TestGUI(interfaces::Node& node) // Transaction table model updates on a QueuedConnection, so process events to ensure it's updated. qApp->processEvents(); QCOMPARE(transactionTableModel->rowCount({}), 107); + QCOMPARE(notified.count(), 2); QVERIFY(FindTx(*transactionTableModel, txid1).isValid()); QVERIFY(FindTx(*transactionTableModel, txid2).isValid()); @@ -224,6 +229,10 @@ void TestGUI(interfaces::Node& node) QCOMPARE(messages.first().at(2).toUInt(), static_cast(CClientUIInterface::MSG_ERROR)); QVERIFY(!node.context()->mempool->exists(refused)); QCOMPARE(walletModel.wallet().getBalance(), balance); + // Its new row raises no "Sent transaction" notification. + qApp->processEvents(); + QVERIFY(FindTx(*transactionTableModel, refused).isValid()); + QCOMPARE(notified.count(), 2); // The form is kept for a retry. QVBoxLayout* entries{sendCoinsDialog.findChild("entries")}; SendCoinsEntry* entry{qobject_cast(entries->itemAt(0)->widget())}; diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index bbc07ec0a3d0..792f584aee95 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -188,6 +188,10 @@ void WalletView::processNewTransaction(const QModelIndex& parent, int start, int return; QModelIndex index = ttm->index(start, 0, parent); + // WalletModel::sendCoins abandons a send whose broadcast was refused + // before its row arrives here. It was not sent. + if (ttm->data(index, TransactionTableModel::StatusRole).toInt() == TransactionStatus::Abandoned) return; + QSettings settings; if (!settings.value("fShowCoinJoinPopups").toBool()) { QVariant nType = ttm->data(index, TransactionTableModel::TypeRole);