From a12f0a28503ee33800adb6836b2fb114ccfb8953 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:22:58 -0500 Subject: [PATCH 1/4] interfaces: Expose CreateRateBumpTransaction's orig_change_pos --- src/interfaces/wallet.h | 3 ++- src/qt/walletmodel.cpp | 2 +- src/wallet/interfaces.cpp | 5 +++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 7955820b295..dc746fe76e2 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -164,7 +164,8 @@ class Wallet std::vector& errors, CAmount& old_fee, CAmount& new_fee, - CMutableTransaction& mtx) = 0; + CMutableTransaction& mtx, + std::optional reduce_output) = 0; //! Sign bump transaction. virtual bool signBumpTransaction(CMutableTransaction& mtx) = 0; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 97f2f4268ca..a75cf7789ab 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -469,7 +469,7 @@ bool WalletModel::bumpFee(Txid hash, Txid& new_hash) CAmount old_fee; CAmount new_fee; CMutableTransaction mtx; - if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx)) { + if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx, /*reduce_output=*/std::nullopt)) { QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Increasing transaction fee failed") + "
(" + (errors.size() ? QString::fromStdString(errors[0].translated) : "") +")"); return false; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index bed9be0a5a3..9b5f43f48c5 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -286,10 +286,11 @@ class WalletImpl : public Wallet std::vector& errors, CAmount& old_fee, CAmount& new_fee, - CMutableTransaction& mtx) override + CMutableTransaction& mtx, + std::optional reduce_output) override { std::vector outputs; // just an empty list of new recipients for now - return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx, /* require_mine= */ true, outputs) == feebumper::Result::OK; + return feebumper::CreateRateBumpTransaction(*m_wallet.get(), txid, coin_control, errors, old_fee, new_fee, mtx, /* require_mine= */ true, outputs, reduce_output) == feebumper::Result::OK; } bool signBumpTransaction(CMutableTransaction& mtx) override { return feebumper::SignTransaction(*m_wallet.get(), mtx); } bool commitBumpTransaction(const Txid& txid, From f51420d7a0178828851ddcddbca64a5615c71768 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:23:45 -0500 Subject: [PATCH 2/4] interfaces: Add isChange to wallet interface --- src/interfaces/wallet.h | 3 +++ src/wallet/interfaces.cpp | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index dc746fe76e2..d45f18fd075 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -125,6 +125,9 @@ class Wallet //! Save or remove receive request. virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; + //! Whether the given output is a change + virtual bool isChange(const CTxOut& txout) const = 0; + //! Display address on external signer virtual util::Result displayAddress(const CTxDestination& dest) = 0; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 9b5f43f48c5..b2faedd4194 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -233,6 +233,11 @@ class WalletImpl : public Wallet return value.empty() ? m_wallet->EraseAddressReceiveRequest(batch, dest, id) : m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } + bool isChange(const CTxOut& txout) const override + { + LOCK(m_wallet->cs_wallet); + return OutputIsChange(*m_wallet, txout); + } util::Result displayAddress(const CTxDestination& dest) override { LOCK(m_wallet->cs_wallet); From 66cf07de3b4eb6a03e01bcdb2703cb081236c493 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 27 Jul 2026 14:44:19 -0700 Subject: [PATCH 3/4] gui, test: Ensure buttons are actually pressed before returning ConfirmSend and ConfirmMessage assumed that the widgets with the buttons to click would be ready by the time the functions fired. This may not necessarily be the case, so instead they should check for the right widgets in a loop and only exit when a button has been clicked. --- src/qt/test/util.cpp | 6 +++++- src/qt/test/wallettests.cpp | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/qt/test/util.cpp b/src/qt/test/util.cpp index 30f7839fc0a..4da1476b711 100644 --- a/src/qt/test/util.cpp +++ b/src/qt/test/util.cpp @@ -17,11 +17,15 @@ void ConfirmMessage(QString* text, std::chrono::milliseconds msec) { QTimer::singleShot(msec, [text]() { for (QWidget* widget : QApplication::topLevelWidgets()) { - if (widget->inherits("QMessageBox")) { + if (widget->inherits("QMessageBox") && widget->isVisible()) { QMessageBox* messageBox = qobject_cast(widget); if (text) *text = messageBox->text(); messageBox->defaultButton()->click(); + return; } } + // Dialog not found yet. Retry with a short delay so we fire in the next + // event loop rather than spinning in the current one. + QTimer::singleShot(std::chrono::milliseconds{10}, [text]() { ConfirmMessage(text, std::chrono::milliseconds{0}); }); }); } diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index af620b67382..716133a9a3f 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -59,18 +59,23 @@ using wallet::WalletRescanReserver; namespace { //! Press "Yes" or "Cancel" buttons in modal send confirmation dialog. +//! If the dialog is not yet visible, reschedule to catch it in the next event loop iteration. void ConfirmSend(QString* text = nullptr, QMessageBox::StandardButton confirm_type = QMessageBox::Yes) { QTimer::singleShot(0, [text, confirm_type]() { for (QWidget* widget : QApplication::topLevelWidgets()) { - if (widget->inherits("SendConfirmationDialog")) { + if (widget->inherits("SendConfirmationDialog") && widget->isVisible()) { SendConfirmationDialog* dialog = qobject_cast(widget); if (text) *text = dialog->text(); QAbstractButton* button = dialog->button(confirm_type); button->setEnabled(true); button->click(); + return; } } + // Dialog not found yet. Retry with a short delay so we fire in the next + // event loop rather than spinning in the current one. + QTimer::singleShot(10, [text, confirm_type]() { ConfirmSend(text, confirm_type); }); }); } From 822b3b1579c8911c09629b4be4e10f24b762f503 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:24:24 -0500 Subject: [PATCH 4/4] qt: Add a dialog to select the change output when bumping fee In order to correctly choose the change output when doing fee bumping in the GUI, we need to ask the user which output is change. We can make a guess using our ScriptIsChange heuristic, however the user may have chosen to have a custom change address or have otherwise labeled their change address which makes our change detection fail. By asking the user when fee bumping, we can avoid adding additional change outputs that are unnecessary. --- src/qt/CMakeLists.txt | 2 + src/qt/bumpfeechoosechangedialog.cpp | 75 +++++++++++++++ src/qt/bumpfeechoosechangedialog.h | 36 +++++++ src/qt/forms/bumpfeechoosechangedialog.ui | 109 ++++++++++++++++++++++ src/qt/test/wallettests.cpp | 37 ++++++++ src/qt/walletmodel.cpp | 12 ++- test/lint/lint-circular-dependencies.py | 1 + 7 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 src/qt/bumpfeechoosechangedialog.cpp create mode 100644 src/qt/bumpfeechoosechangedialog.h create mode 100644 src/qt/forms/bumpfeechoosechangedialog.ui diff --git a/src/qt/CMakeLists.txt b/src/qt/CMakeLists.txt index 3a6ee2d1289..fc7e913c510 100644 --- a/src/qt/CMakeLists.txt +++ b/src/qt/CMakeLists.txt @@ -79,6 +79,8 @@ add_library(bitcoinqt STATIC EXCLUDE_FROM_ALL bitcoingui.h bitcoinunits.cpp bitcoinunits.h + bumpfeechoosechangedialog.cpp + bumpfeechoosechangedialog.h clientmodel.cpp clientmodel.h csvmodelwriter.cpp diff --git a/src/qt/bumpfeechoosechangedialog.cpp b/src/qt/bumpfeechoosechangedialog.cpp new file mode 100644 index 00000000000..5f08388d2c3 --- /dev/null +++ b/src/qt/bumpfeechoosechangedialog.cpp @@ -0,0 +1,75 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#if defined(HAVE_CONFIG_H) +#include +#endif + +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +BumpfeeChooseChangeDialog::BumpfeeChooseChangeDialog(WalletModel *model, QWidget *parent, const Txid& txid) : + QDialog(parent, GUIUtil::dialog_flags), + ui(new Ui::BumpfeeChooseChangeDialog), + model(model) +{ + ui->setupUi(this); + + bool found_change = false; + CTransactionRef tx = model->wallet().getTx(txid); + for (size_t i = 0; i < tx->vout.size(); ++i) { + const CTxOut& txout = tx->vout.at(i); + QString address_info = tr("No address decoded"); + CTxDestination dest; + if (ExtractDestination(txout.scriptPubKey, dest)) { + std::string address = EncodeDestination(dest); + std::string label; + if (model->wallet().getAddress(dest, &label, nullptr) && !label.empty()) { + address_info = QString::fromStdString(label) + QString(" (") + QString::fromStdString(address) + QString(")"); + } else { + address_info = QString::fromStdString(address); + } + } + QString output_info = tr("%1: %2 to %3").arg(i).arg(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), txout.nValue)).arg(address_info); + + QRadioButton *radio_button = new QRadioButton(output_info, nullptr); + radio_button->setObjectName(QString::number(i) + QString("_radioButton")); + ui->verticalLayout->addWidget(radio_button); + + if (!found_change && model->wallet().isChange(txout)) { + radio_button->setChecked(true); + ui->none_radioButton->setChecked(false); + found_change = true; + } + } + GUIUtil::handleCloseWindowShortcut(this); +} + +std::optional BumpfeeChooseChangeDialog::GetSelectedOutput() +{ + for (int i = 0; i < ui->verticalLayout->count(); ++i) { + QRadioButton* child = dynamic_cast(ui->verticalLayout->itemAt(i)->widget()); + if (child->isChecked()) { + if (i == 0) { + // "None" option selected + return std::nullopt; + } + // Return the output index, offset by one for the "None" option at index 0 + return static_cast(i - 1); + } + } + return std::nullopt; +} diff --git a/src/qt/bumpfeechoosechangedialog.h b/src/qt/bumpfeechoosechangedialog.h new file mode 100644 index 00000000000..7183d91fd7d --- /dev/null +++ b/src/qt/bumpfeechoosechangedialog.h @@ -0,0 +1,36 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_QT_BUMPFEECHOOSECHANGEDIALOG_H +#define BITCOIN_QT_BUMPFEECHOOSECHANGEDIALOG_H + +#include +#include + +#include + +class WalletModel; +class uint256; + +namespace Ui { + class BumpfeeChooseChangeDialog; +} + +/** Dialog for choosing the change output when bumping fee + */ +class BumpfeeChooseChangeDialog : public QDialog +{ + Q_OBJECT + +public: + + explicit BumpfeeChooseChangeDialog(WalletModel *model, QWidget *parent, const Txid& txid); + std::optional GetSelectedOutput(); + +private: + Ui::BumpfeeChooseChangeDialog *ui; + WalletModel *model; +}; + +#endif // BITCOIN_QT_BUMPFEECHOOSECHANGEDIALOG_H diff --git a/src/qt/forms/bumpfeechoosechangedialog.ui b/src/qt/forms/bumpfeechoosechangedialog.ui new file mode 100644 index 00000000000..c77da831f14 --- /dev/null +++ b/src/qt/forms/bumpfeechoosechangedialog.ui @@ -0,0 +1,109 @@ + + + BumpfeeChooseChangeDialog + + + + 0 + 0 + 737 + 422 + + + + Choose Change Output + + + true + + + + + + <html><head/><body><p>Choose which output is change.</p><p>The additional transaction fee will be deducted from this output. If it is insufficient, new inputs may be added and the resulting change sent to the address of the selected output.</p></body></html> + + + Qt::RichText + + + true + + + + + + + true + + + + + 0 + 0 + 711 + 288 + + + + + + + None + + + true + + + + + + + + + + + Qt::Horizontal + + + QDialogButtonBox::Cancel|QDialogButtonBox::Ok + + + + + + + + + buttonBox + accepted() + BumpfeeChooseChangeDialog + accept() + + + 210 + 395 + + + 200 + 210 + + + + + buttonBox + rejected() + BumpfeeChooseChangeDialog + reject() + + + 210 + 395 + + + 200 + 210 + + + + + diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 716133a9a3f..d206e64a0de 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -40,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -79,6 +81,40 @@ void ConfirmSend(QString* text = nullptr, QMessageBox::StandardButton confirm_ty }); } +//! In the BumpfeeChooseChangeDialog, choose the radio button at the index, or use the default. Then Press "Yes" or "Cancel". +//! If the dialog is not yet visible, reschedule to catch it in the next event loop iteration. +void ChooseBumpfeeOutput(std::optional index = std::nullopt, bool cancel = false) +{ + QTimer::singleShot(0, [index, cancel]() { + for (QWidget* widget : QApplication::topLevelWidgets()) { + if (widget->inherits("BumpfeeChooseChangeDialog") && widget->isVisible()) { + BumpfeeChooseChangeDialog* dialog = qobject_cast(widget); + + if (index.has_value()) { + QString button_name; + if (index.value() == 0) { + button_name = QString("none_radioButton"); + } else { + button_name = QString::number(index.value() - 1) + QString("_radioButton"); + } + QRadioButton* button = dialog->findChild(button_name); + button->setEnabled(true); + button->click(); + } + + QDialogButtonBox* button_box = dialog->findChild(QString("buttonBox")); + QAbstractButton* button = button_box->button(cancel ? QDialogButtonBox::Cancel : QDialogButtonBox::Ok); + button->setEnabled(true); + button->click(); + return; + } + } + // Dialog not found yet. Retry with a short delay so we fire in the next + // event loop rather than spinning in the current one. + QTimer::singleShot(10, [index, cancel]() { ChooseBumpfeeOutput(index, cancel); }); + }); +} + //! Send coins to address and return txid. Txid SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount, QMessageBox::StandardButton confirm_type = QMessageBox::Yes) @@ -127,6 +163,7 @@ void BumpFee(TransactionView& view, const Txid& txid, bool expectDisabled, std:: QCOMPARE(action->isEnabled(), !expectDisabled); action->setEnabled(true); + ChooseBumpfeeOutput(); QString text; if (expectError.empty()) { ConfirmSend(&text, cancel ? QMessageBox::Cancel : QMessageBox::Yes); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index a75cf7789ab..d781e3f4f8f 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include +#include #include #include #include @@ -464,12 +466,20 @@ WalletModel::UnlockContext::~UnlockContext() bool WalletModel::bumpFee(Txid hash, Txid& new_hash) { + // Ask the user which is the change output + auto choose_change_dialog = new BumpfeeChooseChangeDialog(this, nullptr, hash); + const auto choose_change_retval = choose_change_dialog->exec(); + if (choose_change_retval != QDialog::Accepted) { + return false; + } + std::optional change_pos = choose_change_dialog->GetSelectedOutput(); + CCoinControl coin_control; std::vector errors; CAmount old_fee; CAmount new_fee; CMutableTransaction mtx; - if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx, /*reduce_output=*/std::nullopt)) { + if (!m_wallet->createBumpTransaction(hash, coin_control, errors, old_fee, new_fee, mtx, change_pos)) { QMessageBox::critical(nullptr, tr("Fee bump error"), tr("Increasing transaction fee failed") + "
(" + (errors.size() ? QString::fromStdString(errors[0].translated) : "") +")"); return false; diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index 8114b6dd5ea..7166b3b87c7 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -19,6 +19,7 @@ "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel", "qt/sendcoinsdialog -> qt/walletmodel -> qt/sendcoinsdialog", "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel", + "qt/bumpfeechoosechangedialog -> qt/walletmodel -> qt/bumpfeechoosechangedialog", "wallet/wallet -> wallet/walletdb -> wallet/wallet", "kernel/coinstats -> validation -> kernel/coinstats", "versionbits -> versionbits_impl -> versionbits",