Skip to content

Commit

Permalink
qt: Add a dialog to select the change output when bumping fee
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
achow101 committed Sep 7, 2023
1 parent 1e5dac8 commit b3653bc
Show file tree
Hide file tree
Showing 8 changed files with 272 additions and 3 deletions.
2 changes: 2 additions & 0 deletions build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down Expand Up @@ -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" />
Expand Down
4 changes: 4 additions & 0 deletions src/Makefile.qt.include
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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 \
Expand Down Expand Up @@ -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 \
Expand Down Expand Up @@ -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 \
Expand Down
75 changes: 75 additions & 0 deletions src/qt/bumpfeechoosechangedialog.cpp
Original file line number Diff line number Diff line change
@@ -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;
}
36 changes: 36 additions & 0 deletions src/qt/bumpfeechoosechangedialog.h
Original file line number Diff line number Diff line change
@@ -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
109 changes: 109 additions & 0 deletions src/qt/forms/bumpfeechoosechangedialog.ui
Original file line number Diff line number Diff line change
@@ -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>
36 changes: 34 additions & 2 deletions src/qt/test/wallettests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -40,6 +41,7 @@
#include <QClipboard>
#include <QObject>
#include <QPushButton>
#include <QRadioButton>
#include <QTimer>
#include <QVBoxLayout>
#include <QTextEdit>
Expand All @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 11 additions & 1 deletion src/qt/walletmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -32,6 +33,7 @@
#include <functional>

#include <QDebug>
#include <QDialog>
#include <QMessageBox>
#include <QSet>
#include <QTimer>
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions test/lint/lint-circular-dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
"kernel/mempool_persist -> validation -> kernel/mempool_persist",
Expand Down

0 comments on commit b3653bc

Please sign in to comment.