From bf222ab0cbf36398094f8139d900aedc94fac2eb 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 602a1182598..e441d10deac 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 b71dfb0e9ff..7a8a5a31505 100644
--- a/src/qt/test/wallettests.cpp
+++ b/src/qt/test/wallettests.cpp
@@ -10,6 +10,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>
@@ -35,6 +36,7 @@
 #include <QApplication>
 #include <QCheckBox>
 #include <QPushButton>
+#include <QRadioButton>
 #include <QTimer>
 #include <QVBoxLayout>
 #include <QTextEdit>
@@ -55,7 +57,7 @@ namespace
 //! Press "Yes" or "Cancel" buttons in modal send confirmation dialog.
 void ConfirmSend(QString* text = nullptr, bool cancel = false)
 {
-    QTimer::singleShot(0, [text, cancel]() {
+    QTimer::singleShot(10ms, [text, cancel]() {
         for (QWidget* widget : QApplication::topLevelWidgets()) {
             if (widget->inherits("SendConfirmationDialog")) {
                 SendConfirmationDialog* dialog = qobject_cast<SendConfirmationDialog*>(widget);
@@ -68,6 +70,35 @@ void ConfirmSend(QString* text = nullptr, bool cancel = false)
     });
 }
 
+//! 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)
 {
@@ -119,11 +150,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);
     } 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 c6f3f5b00c2..fc59ee85b72 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>
@@ -485,13 +487,21 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs)
 
 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 abf38ca79b6..1f127796207 100755
--- a/test/lint/lint-circular-dependencies.py
+++ b/test/lint/lint-circular-dependencies.py
@@ -20,6 +20,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",