From 0b238718a07978f36e5fe414227a2676928f75d1 Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Mon, 7 Nov 2022 18:24:24 -0500
Subject: [PATCH] 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.
---
 .../libbitcoin_qt/libbitcoin_qt.vcxproj       |   2 +
 src/Makefile.qt.include                       |   4 +
 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 +
 8 files changed, 272 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/build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj b/build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj
index 9f9dc9d5fa0..c9c53ecd993 100644
--- a/build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj
+++ b/build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj
@@ -17,6 +17,7 @@
     <ClCompile Include="..\..\src\qt\bitcoingui.cpp" />
     <ClCompile Include="..\..\src\qt\bitcoinstrings.cpp" />
     <ClCompile Include="..\..\src\qt\bitcoinunits.cpp" />
+    <ClCompile Include="..\..\src\qt\bumpfeechoosechangedialog.cpp" />
     <ClCompile Include="..\..\src\qt\clientmodel.cpp" />
     <ClCompile Include="..\..\src\qt\coincontroldialog.cpp" />
     <ClCompile Include="..\..\src\qt\coincontroltreewidget.cpp" />
@@ -73,6 +74,7 @@
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_bitcoinamountfield.cpp" />
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_bitcoingui.cpp" />
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_bitcoinunits.cpp" />
+    <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_bumpfeechoosechangedialog.cpp" />
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_clientmodel.cpp" />
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_coincontroldialog.cpp" />
     <ClCompile Include="$(GeneratedFilesOutDir)\moc\moc_coincontroltreewidget.cpp" />
diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 7852d1a2fa9..96667137ade 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -16,6 +16,7 @@ include Makefile.qt_locale.include
 QT_FORMS_UI = \
   qt/forms/addressbookpage.ui \
   qt/forms/askpassphrasedialog.ui \
+  qt/forms/bumpfeechoosechangedialog.ui \
   qt/forms/coincontroldialog.ui \
   qt/forms/createwalletdialog.ui \
   qt/forms/editaddressdialog.ui \
@@ -38,6 +39,7 @@ QT_MOC_CPP = \
   qt/moc_addressbookpage.cpp \
   qt/moc_addresstablemodel.cpp \
   qt/moc_askpassphrasedialog.cpp \
+  qt/moc_bumpfeechoosechangedialog.cpp \
   qt/moc_createwalletdialog.cpp \
   qt/moc_bantablemodel.cpp \
   qt/moc_bitcoin.cpp \
@@ -109,6 +111,7 @@ BITCOIN_QT_H = \
   qt/addressbookpage.h \
   qt/addresstablemodel.h \
   qt/askpassphrasedialog.h \
+  qt/bumpfeechoosechangedialog.h \
   qt/bantablemodel.h \
   qt/bitcoin.h \
   qt/bitcoinaddressvalidator.h \
@@ -252,6 +255,7 @@ BITCOIN_QT_WALLET_CPP = \
   qt/addressbookpage.cpp \
   qt/addresstablemodel.cpp \
   qt/askpassphrasedialog.cpp \
+  qt/bumpfeechoosechangedialog.cpp \
   qt/coincontroldialog.cpp \
   qt/coincontroltreewidget.cpp \
   qt/createwalletdialog.cpp \
diff --git a/src/qt/bumpfeechoosechangedialog.cpp b/src/qt/bumpfeechoosechangedialog.cpp
new file mode 100644
index 00000000000..2432aa4e94d
--- /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 <config/bitcoin-config.h>
+#endif
+
+#include <qt/bumpfeechoosechangedialog.h>
+#include <qt/forms/ui_bumpfeechoosechangedialog.h>
+
+#include <key_io.h>
+#include <script/standard.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 = QString::number(i) + QString(": ") + BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), txout.nValue) + tr(" to ") + 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
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 <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
new file mode 100644
index 00000000000..c77da831f14
--- /dev/null
+++ 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
index 5f789e400ef..0509b35a110 100644
--- 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>
@@ -39,6 +40,7 @@
 #include <QClipboard>
 #include <QObject>
 #include <QPushButton>
+#include <QRadioButton>
 #include <QTimer>
 #include <QVBoxLayout>
 #include <QTextEdit>
@@ -60,7 +62,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);
@@ -73,6 +75,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)
@@ -125,11 +156,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 ee3327530c4..1ae1272be5c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -9,6 +9,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>
@@ -32,6 +33,7 @@
 #include <functional>
 
 #include <QDebug>
+#include <QDialog>
 #include <QMessageBox>
 #include <QSet>
 #include <QTimer>
@@ -478,13 +480,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)) {
+    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
index 307b4dca5ab..90f9a66d25b 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/fees -> wallet/wallet -> wallet/fees",
     "wallet/wallet -> wallet/walletdb -> wallet/wallet",
     "kernel/coinstats -> validation -> kernel/coinstats",