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.
bitcoin-core · Oct 4, 2024 · 674187c · 674187c
1 parent e1cce62
commit 674187c
Show file tree

Hide file tree

Showing 7 changed files with 268 additions and 3 deletions.
diff --git 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
@@ -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 <config/bitcoin-config.h>
+#endif
+
+#include <qt/bumpfeechoosechangedialog.h>
+#include <qt/forms/ui_bumpfeechoosechangedialog.h>
+
+#include <addresstype.h>
+#include <key_io.h>
+#include <qt/bitcoinunits.h>
+#include <qt/guiutil.h>
+#include <qt/optionsmodel.h>
+#include <qt/walletmodel.h>
+
+#include <QHBoxLayout>
+#include <QLabel>
+#include <QRadioButton>
+#include <QVBoxLayout>
+
+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<uint32_t> BumpfeeChooseChangeDialog::GetSelectedOutput()
+{
+    for (int i = 0; i < ui->verticalLayout->count(); ++i) {
+        QRadioButton* child = dynamic_cast<QRadioButton*>(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<uint32_t>(i - 1);
+        }
+    }
+    return std::nullopt;
+}
diff --git a/src/qt/bumpfeechoosechangedialog.h 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 <QDialog>
+#include <optional>
+
+#include <uint256.h>
+
+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<uint32_t> 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
@@ -0,0 +1,109 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ui version="4.0">
+ <class>BumpfeeChooseChangeDialog</class>
+ <widget class="QDialog" name="BumpfeeChooseChangeDialog">
+  <property name="geometry">
+   <rect>
+    <x>0</x>
+    <y>0</y>
+    <width>737</width>
+    <height>422</height>
+   </rect>
+  </property>
+  <property name="windowTitle">
+   <string>Choose Change Output</string>
+  </property>
+  <property name="sizeGripEnabled">
+   <bool>true</bool>
+  </property>
+  <layout class="QVBoxLayout" name="verticalLayout_2">
+   <item>
+    <widget class="QLabel" name="label">
+     <property name="text">
+      <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Choose which output is change.&lt;/p&gt;&lt;p&gt;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.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
+     </property>
+     <property name="textFormat">
+      <enum>Qt::RichText</enum>
+     </property>
+     <property name="wordWrap">
+      <bool>true</bool>
+     </property>
+    </widget>
+   </item>
+   <item>
+    <widget class="QScrollArea" name="scrollArea">
+     <property name="widgetResizable">
+      <bool>true</bool>
+     </property>
+     <widget class="QWidget" name="scrollAreaWidgetContents">
+      <property name="geometry">
+       <rect>
+        <x>0</x>
+        <y>0</y>
+        <width>711</width>
+        <height>288</height>
+       </rect>
+      </property>
+      <layout class="QVBoxLayout" name="verticalLayout">
+       <item>
+        <widget class="QRadioButton" name="none_radioButton">
+         <property name="text">
+          <string>None</string>
+         </property>
+         <property name="checked">
+          <bool>true</bool>
+         </property>
+        </widget>
+       </item>
+      </layout>
+     </widget>
+    </widget>
+   </item>
+   <item>
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="orientation">
+      <enum>Qt::Horizontal</enum>
+     </property>
+     <property name="standardButtons">
+      <set>QDialogButtonBox::Cancel|QDialogButtonBox::Ok</set>
+     </property>
+    </widget>
+   </item>
+  </layout>
+ </widget>
+ <resources/>
+ <connections>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>accepted()</signal>
+   <receiver>BumpfeeChooseChangeDialog</receiver>
+   <slot>accept()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>210</x>
+     <y>395</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>200</x>
+     <y>210</y>
+    </hint>
+   </hints>
+  </connection>
+  <connection>
+   <sender>buttonBox</sender>
+   <signal>rejected()</signal>
+   <receiver>BumpfeeChooseChangeDialog</receiver>
+   <slot>reject()</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>210</x>
+     <y>395</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>200</x>
+     <y>210</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
+</ui>
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
@@ -11,6 +11,7 @@
 #include <key_io.h>
 #include <qt/bitcoinamountfield.h>
 #include <qt/bitcoinunits.h>
+#include <qt/bumpfeechoosechangedialog.h>
 #include <qt/clientmodel.h>
 #include <qt/optionsmodel.h>
 #include <qt/overviewpage.h>
@@ -40,6 +41,7 @@
 #include <QClipboard>
 #include <QObject>
 #include <QPushButton>
+#include <QRadioButton>
 #include <QTimer>
 #include <QVBoxLayout>
 #include <QTextEdit>
@@ -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<SendConfirmationDialog*>(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<uint32_t> index = std::nullopt, bool cancel = false)
+{
+    QTimer::singleShot(0, [index, cancel]() {
+        for (QWidget* widget : QApplication::topLevelWidgets()) {
+            if (widget->inherits("BumpfeeChooseChangeDialog")) {
+                BumpfeeChooseChangeDialog* dialog = qobject_cast<BumpfeeChooseChangeDialog*>(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<QRadioButton*>(button_name);
+                    button->setEnabled(true);
+                    button->click();
+                }
+
+                QDialogButtonBox* button_box = dialog->findChild<QDialogButtonBox*>(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
@@ -5,6 +5,7 @@
 #include <qt/walletmodel.h>
 
 #include <qt/addresstablemodel.h>
+#include <qt/bumpfeechoosechangedialog.h>
 #include <qt/clientmodel.h>
 #include <qt/guiconstants.h>
 #include <qt/guiutil.h>
@@ -29,6 +30,7 @@
 #include <functional>
 
 #include <QDebug>
+#include <QDialog>
 #include <QMessageBox>
 #include <QSet>
 #include <QTimer>
@@ -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<uint32_t> change_pos = choose_change_dialog->GetSelectedOutput();
+
     CCoinControl coin_control;
     coin_control.m_signal_bip125_rbf = true;
     std::vector<bilingual_str> 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") + "<br />(" +
             (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
@@ -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",