Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 4 commits into from
Nov 3, 2026
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9773acd
fix(qt): surface a mempool rejection from WalletModel::sendCoins
PastaPastaPasta Oct 16, 2026
856eb04
refactor(wallet): keep the node's broadcast result in interfaces::Chain
PastaPastaPasta Nov 2, 2026
f068929
test(qt): cover a refused send in an avoid_reuse wallet
PastaPastaPasta Nov 2, 2026
d4e5b9b
fix(qt): no "Sent transaction" notification for a refused send
PastaPastaPasta Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/interfaces/chain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <blockfilter.h>
#include <primitives/transaction.h> // For CTransactionRef
#include <util/error.h> // For TransactionError
#include <util/settings.h> // For util::SettingsValue
#include <util/result.h>

Expand Down Expand Up @@ -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;
Expand Down
8 changes: 2 additions & 6 deletions src/node/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
15 changes: 12 additions & 3 deletions src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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:
Expand All @@ -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;
Expand Down
131 changes: 125 additions & 6 deletions src/qt/test/wallettests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,15 @@
#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/error.h>
#include <util/system.h>
#include <validation.h>
#include <validationinterface.h>
#include <wallet/coincontrol.h>
#include <wallet/test/util.h>
#include <wallet/wallet.h>
#include <qt/overviewpage.h>
Expand All @@ -31,7 +37,9 @@
#include <qt/receiverequestdialog.h>

#include <chrono>
#include <functional>
#include <memory>
#include <optional>

#include <QAbstractButton>
#include <QAction>
Expand All @@ -40,6 +48,7 @@
#include <QDialogButtonBox>
#include <QListView>
#include <QPushButton>
#include <QSignalSpy>
#include <QTableView>
#include <QTextEdit>
#include <QTimer>
Expand All @@ -56,14 +65,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();
Expand All @@ -72,8 +83,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());
Expand All @@ -83,7 +96,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;
Expand Down Expand Up @@ -190,6 +203,112 @@ 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;
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()));
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);
Expand Down
29 changes: 27 additions & 2 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand All @@ -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))};

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reactivate identical abandoned transactions before committing

When a rejected send is retried and transaction creation produces the same txid (for example, a coin-controlled subtract-fee/no-change payment at the same tip), the first pass has left that wallet entry abandoned. This call invokes CommitTransaction(), whose AddToWallet(..., TxStateInactive{}) sees the existing abandoned TxStateInactive as the same variant and hits its fatal serialized-index assertion (-1 versus 0); without assertions it remains abandoned and cannot be relayed. Thus the retained-form retry can crash dash-qt or repeatedly fail; reactivate the abandoned state before recommitting and cover an actual identical retry.

AGENTS.md reference: AGENTS.md:L40-L44

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Oct 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in c8e27b5. If the wallet already holds the transaction as abandoned, CWallet::CommitTransaction now un-abandons it before re-adding it, and the existing reconcile step marks its inputs spent again. A new TestGUI block sends one prepared transaction twice, refused and then accepted; without the wallet change it hits the AddToWallet assertion. (Develop already crashed the same way after a manual "Abandon transaction" followed by an identical resend.)

🤖 Posted autonomously by Claude on behalf of pasta.

if (!res && (wallet().abandonTransaction(newTx->GetHash()) || !WasSent(wallet(), newTx->GetHash()))) {

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore rejected inputs for avoid-reuse wallets

When WALLET_FLAG_AVOID_REUSE is enabled, a failed send has already passed through AddToWallet(), which persists every input address as previously spent at src/wallet/wallet.cpp:1061-1070. This abandonment restores the UTXO but never clears that marker, and GetBalance()/AvailableCoins() then exclude its IsSpentKey outputs (src/wallet/receive.cpp:175-181, src/wallet/spend.cpp:303-305). A rejection before relay can therefore remove the input—and all other coins on that address—from the GUI balance, so the retained form may fail for insufficient funds instead of being retryable; avoid or roll back the marker for this never-relayed path and cover the flag in the regression test.

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Oct 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as is. Abandoning a refused send leaves the AVOID_REUSE "used" mark on its input addresses, but that is how abandoning already works in develop and upstream Bitcoin Core: abandontransaction keeps the mark too, because AddToWallet sets it and nothing rolls it back. avoid_reuse is opt-in, and clearing used-key state on abandon is a wallet-wide change that belongs in its own upstream-aligned PR, not in this GUI fix.

🤖 Posted autonomously by Claude on behalf of pasta.

Comment on lines +364 to +365

@thepastaclaw thepastaclaw Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Restore avoid-reuse accounting for locally rejected sends

AddToWallet() marks every wallet-owned input destination as previously spent through SetSpentKeyState() before broadcasting. The new rejected-send path abandons the transaction and reconciles its outpoints, but it never reverses those address-used markers. With WALLET_FLAG_AVOID_REUSE enabled, GetBalance() and coin selection continue excluding outputs at those destinations, including other spendable outputs at the same address, even though this transaction never entered the mempool. Consequently the retained form can still show a reduced balance or fail to retry for insufficient funds. The rejection path needs to recompute or roll back the marker when the transaction is known to have been locally refused, or explicitly preserve this behavior with coverage for avoid-reuse wallets.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)

@PastaPastaPasta PastaPastaPasta Oct 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as is, as in the earlier round. AbandonTransaction has never cleared the avoid-reuse markers that AddToWallet sets (SetSpentKeyState is only ever called with used = true). The same is true of an abandoned send in upstream Bitcoin Core and of abandontransaction over RPC, so this PR keeps the existing behaviour rather than changing what "used" means for an abandoned transaction. If we want that, it belongs in its own change to AbandonTransaction, covering every caller.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upstream behavior explains why AbandonTransaction() itself does not clear the marker, but this change adds a new automatic-abandon path and explicitly promises that a refused send's inputs are free for retry. AddToWallet() marks destinations through SetSpentKeyState(), while ReconcileWalletUTXOs() only restores outpoints; it does not clear previously_spent, so with avoid-reuse enabled another spendable output at the same address remains excluded after this GUI failure. That interaction is introduced by this PR's refusal handling, rather than being only a generic follow-up to manual abandonment.

@PastaPastaPasta PastaPastaPasta Oct 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that this PR's automatic abandon now reaches the marker, and I checked what that costs. With avoid_reuse set, the refused send's input addresses stay marked as used. The GUI's cached balance comes from GetBalance(avoid_reuse=true), so it leaves those coins out. prepareTransaction checks the amount against that balance, so a retry of nearly the whole balance can fail with AmountExceedsBalance. Coin selection itself still uses them: the GUI's CCoinControl leaves m_avoid_address_reuse false, so allow_used_addresses is true (src/wallet/spend.cpp:216).

I'm still not changing it here:

  • Not a regression. On develop a refused send stays a pending debit with its input spent, so the same coins are unavailable and the user is also told it was sent. With avoid_reuse the balance after this PR is the same as develop's or higher. Without the flag it is fully restored.
  • Narrow reach. dash-qt can't create or toggle an avoid_reuse wallet; that takes createwallet avoid_reuse=true or setwalletflag over RPC. The coins also still show up there, under getbalances' used bucket.
  • A correct rollback isn't local to this path. The "previously spent" mark is one bit per address with no count (SetSpentKeyState). An earlier relayed spend may already have set it for the same address, so clearing it for this transaction's inputs would be wrong. CommitTransaction can't drop it at the refusal either. RPC callers keep the refused transaction and rebroadcast it, and in the GUI the abandon can still fail because the transaction got out. A fix means recording which addresses this transaction newly marked, or recomputing them when a transaction is abandoned. That belongs in AbandonTransaction for every caller, including the abandontransaction RPC, where upstream has the same behaviour.

I'd rather fix that in its own change to AbandonTransaction than add a GUI-only version here.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right; I overstated this as a new coin-selection regression. The GUI's default CCoinControl permits used addresses, while the cached-balance precheck can still prevent an automatic retry after those addresses are marked used. That limitation also exists before this change, and clearing the address-wide bit could incorrectly erase an earlier spend's history. I withdraw the request to change avoid-reuse accounting in this PR.

Withdrawing this finding.

@PastaPastaPasta PastaPastaPasta Nov 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered and tracked in f068929 (test(qt): cover a refused send in an avoid_reuse wallet). The behaviour itself is unchanged.

Two points about the fixture, for whoever picks up #8017:

  • Queue drain: once TestGUI attaches chain notifications, the earlier sends' TransactionAddedToMempool callbacks may still be queued. Under the flag they would mark their inputs as used on their own, so the block calls SyncWithValidationInterfaceQueue() before it sets the flag.
  • Cached balances: turning the flag off doesn't invalidate cached balances that were computed with it on. The block calls MarkDirty() afterwards so the later OverviewPage check stays meaningful.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Nov 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred (re-reviewed at f068929c): Your new TestGUI block covers the reduced avoid-reuse balance, the cached-balance rejection, and a successful coin-controlled retry; it also drains earlier notifications and invalidates caches after disabling the flag. With #8017 tracking the shared AbandonTransaction accounting fix, keeping the existing address-wide marker semantics here is a reasonable deferral.

const bilingual_str error{util::ErrorString(res)};
return SendCoinsReturn(TransactionCommitFailed,
QString::fromStdString(error.translated.empty() ? error.original : error.translated));
}
Comment on lines +364 to +369

@thepastaclaw thepastaclaw Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Withdrawn at 29e1f07a; see the replies below.

🔴 Blocking: Recheck transaction status before accepting a successful abandon

When abandonTransaction() returns true, the short-circuit prevents WasSent() from checking the transaction at all. A successful abandon only proves that the wallet entry was inactive when the wallet lock was acquired; a block or mempool notification can be queued after that point and processed later. In that case the transaction can become confirmed or enter the mempool even though sendCoins has already returned TransactionCommitFailed and retained the retry form. The final decision must recheck current transaction status after abandonment, and must report success when the transaction was accepted or confirmed rather than unconditionally trusting the earlier successful abandon.

source: gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)

@PastaPastaPasta PastaPastaPasta Oct 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining; I don't see a way to reach this.

A transaction reaches this branch only when the node's own mempool has just refused it, so this node never relayed it, and a freshly built send exists nowhere else. For it to then enter a mempool or be confirmed, someone would have to hold a transaction we never sent. The one in-chain case, ALREADY_IN_CHAIN, is already detected through findCoins in CommitTransaction before we ever get here.

If it did happen anyway, the queued blockConnected replaces the abandoned state with the confirmed one, as AddToWallet does for any transaction, so the wallet ends up correct. The only effect would be a misleading error for that one attempt.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing premise is that this transaction cannot exist anywhere else. A local mempool refusal only says this submission was rejected; another node or submission path can still provide the same transaction, and a block containing it can arrive before the queued wallet notification is processed. In that interleaving abandonTransaction() sees the inactive state and returns true, the short-circuit skips WasSent(), and the GUI reports failure even though the later notification marks the transaction confirmed; the promised status-based resolution still requires a recheck after abandonment.

@PastaPastaPasta PastaPastaPasta Oct 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granted that it can't be ruled out in principle, but the requested recheck can't detect it. In the interleaving you describe, the block notification is still queued when abandonTransaction() returns true, so a WasSent() call straight afterwards reads the same wallet state: the transaction is abandoned, neither in the mempool nor confirmed, and it reports "not sent" just the same. Dropping the short-circuit would change no outcome.

To catch it, sendCoins would have to wait for the validation queue to drain on the GUI thread before deciding. That blocks the UI to cover a transaction appearing from elsewhere moments after our own mempool refused it. If it does happen, the queued blockConnected marks the transaction confirmed, the wallet ends up correct and the payment shows in the history, so the cost is one misleading error message. I'd rather keep the current code.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that an immediate WasSent() call cannot resolve the queued-notification interleaving: AbandonTransaction() checks the wallet state under cs_wallet, and the notification may be applied only after both calls return. I was treating the post-abandon check as a synchronization point when it is not; detecting that case would require waiting for pending chain notifications, which is a different behavior and not justified here.

Withdrawing this finding.

@thepastaclaw thepastaclaw Oct 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Withdrawn (re-reviewed at 29e1f07a): You are right that an immediate WasSent call reads the same cached wallet state while the notification remains queued, so removing the short-circuit would not fix the proposed interleaving. I withdraw the request because I have not established a reachable regression that warrants validation-queue synchronization on the GUI thread.


CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << *newTx;
Expand Down Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions src/qt/walletmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class WalletModel : public QObject
AmountWithFeeExceedsBalance,
DuplicateAddress,
TransactionCreationFailed, // Error returned when wallet is still locked
TransactionCommitFailed,
AbsurdFee
};

Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/test/wallet_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading