From 15ebc44020a5ef39ffaccd6c087dcb0e6b2c3bd2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:22:58 -0500 Subject: [PATCH 1/3] 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 df1ced48a71..c7195462e8b 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -173,7 +173,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 0a01c0a45b1..468f2302a2e 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -487,7 +487,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& 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 21e8a0b3bd2..58b4da39171 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -314,10 +314,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 uint256& txid, From e1cce622f745db6d2c2cc16ad23c207689d0d8e5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:23:45 -0500 Subject: [PATCH 2/3] 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 c7195462e8b..d93ff7be64d 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -131,6 +131,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 58b4da39171..ca6507daacf 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -249,6 +249,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 674187caf5bd5715486226285d94622b1a7b3736 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 18:24:24 -0500 Subject: [PATCH 3/3] 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 | 36 ++++++- src/qt/walletmodel.cpp | 12 ++- test/lint/lint-circular-dependencies.py | 1 + 7 files changed, 268 insertions(+), 3 deletions(-) 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 7ec2b74cc8f..6f2197ebd84 100644 --- a/src/qt/CMakeLists.txt +++ b/src/qt/CMakeLists.txt @@ -63,6 +63,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..df00689254e --- /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 uint256& 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, 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..c22c1eb104b --- /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 uint256& 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 98dfe12f084..ed08ef6f6d2 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 @@ -61,7 +63,7 @@ namespace //! Press "Yes" or "Cancel" buttons in modal send confirmation dialog. void ConfirmSend(QString* text = nullptr, QMessageBox::StandardButton confirm_type = QMessageBox::Yes) { - QTimer::singleShot(0, [text, confirm_type]() { + QTimer::singleShot(10ms, [text, confirm_type]() { for (QWidget* widget : QApplication::topLevelWidgets()) { if (widget->inherits("SendConfirmationDialog")) { SendConfirmationDialog* dialog = qobject_cast(widget); @@ -74,6 +76,35 @@ 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". +void ChooseBumpfeeOutput(std::optional index = std::nullopt, bool cancel = false) +{ + QTimer::singleShot(0, [index, cancel]() { + for (QWidget* widget : QApplication::topLevelWidgets()) { + if (widget->inherits("BumpfeeChooseChangeDialog")) { + 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(); + } + } + }); +} + //! Send coins to address and return txid. uint256 SendCoins(CWallet& wallet, SendCoinsDialog& sendCoinsDialog, const CTxDestination& address, CAmount amount, bool rbf, QMessageBox::StandardButton confirm_type = QMessageBox::Yes) @@ -126,11 +157,12 @@ void BumpFee(TransactionView& view, const uint256& txid, bool expectDisabled, st QCOMPARE(action->isEnabled(), !expectDisabled); action->setEnabled(true); + ChooseBumpfeeOutput(); QString text; if (expectError.empty()) { ConfirmSend(&text, cancel ? QMessageBox::Cancel : QMessageBox::Yes); } else { - ConfirmMessage(&text, 0ms); + ConfirmMessage(&text, 10ms); } action->trigger(); QVERIFY(text.indexOf(QString::fromStdString(expectError)) != -1); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 468f2302a2e..b306550e0f6 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -29,6 +30,7 @@ #include #include +#include #include #include #include @@ -481,13 +483,21 @@ WalletModel::UnlockContext::~UnlockContext() bool WalletModel::bumpFee(uint256 hash, uint256& 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; coin_control.m_signal_bip125_rbf = true; 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 6f9a633807a..b525b40f58b 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",