Merge #9697: [Qt] simple fee bumper with user verification

a38783747 Make sure we re-check the conditions of a feebump during commit (Jonas Schnelli)
9b9ca538c Only update the transactionrecord if the fee bump has been commited (Jonas Schnelli)
6ed4368f1 Make sure we use nTxConfirmTarget during Qt fee bumps (Jonas Schnelli)
be08fc39d Make sure we always update the table row after a bumpfee call (Jonas Schnelli)
2678d3dc6 Show old-fee, increase a new-fee in Qt fee bumper confirmation dialog (Jonas Schnelli)
2ec911f60 Add cs_wallet lock assertion to SignTransaction() (Jonas Schnelli)
fbf385cc8 [Qt] simple fee bumper with user verification (Jonas Schnelli)

Tree-SHA512: a3ce626201abf64cee496dd1d83870de51ba633de40c48eb0219c3eba5085c038af34c284512130d2544de20c1bff9fea1b78f92e3574c21dd4e96c11b8e7d76

src/qt/sendcoinsdialog.cpp

@@ -31,8 +31,6 @@
 #include <QTextDocument>
 #include <QTimer>
 
-#define SEND_CONFIRM_DELAY   3
-
 SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *parent) :
     QDialog(parent),
     ui(new Ui::SendCoinsDialog),

src/qt/sendcoinsdialog.h

@@ -100,13 +100,14 @@ Q_SIGNALS:
 };
 
 
+#define SEND_CONFIRM_DELAY   3
 
 class SendConfirmationDialog : public QMessageBox
 {
     Q_OBJECT
 
 public:
-    SendConfirmationDialog(const QString &title, const QString &text, int secDelay = 0, QWidget *parent = 0);
+    SendConfirmationDialog(const QString &title, const QString &text, int secDelay = SEND_CONFIRM_DELAY, QWidget *parent = 0);
     int exec();
 
 private Q_SLOTS:

src/qt/transactionview.cpp

@@ -11,6 +11,7 @@
 #include "guiutil.h"
 #include "optionsmodel.h"
 #include "platformstyle.h"
+#include "sendcoinsdialog.h"
 #include "transactiondescdialog.h"
 #include "transactionfilterproxy.h"
 #include "transactionrecord.h"
@@ -37,7 +38,7 @@
 
 TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *parent) :
     QWidget(parent), model(0), transactionProxyModel(0),
-    transactionView(0), abandonAction(0), columnResizingFixer(0)
+    transactionView(0), abandonAction(0), bumpFeeAction(0), columnResizingFixer(0)
 {
     // Build filter row
     setContentsMargins(0,0,0,0);
@@ -138,6 +139,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 
     // Actions
     abandonAction = new QAction(tr("Abandon transaction"), this);
+    bumpFeeAction = new QAction(tr("Increase transaction fee"), this);
     QAction *copyAddressAction = new QAction(tr("Copy address"), this);
     QAction *copyLabelAction = new QAction(tr("Copy label"), this);
     QAction *copyAmountAction = new QAction(tr("Copy amount"), this);
@@ -156,6 +158,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
     contextMenu->addAction(copyTxPlainText);
     contextMenu->addAction(showDetailsAction);
     contextMenu->addSeparator();
+    contextMenu->addAction(bumpFeeAction);
     contextMenu->addAction(abandonAction);
     contextMenu->addAction(editLabelAction);
 
@@ -173,6 +176,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
     connect(view, SIGNAL(doubleClicked(QModelIndex)), this, SIGNAL(doubleClicked(QModelIndex)));
     connect(view, SIGNAL(customContextMenuRequested(QPoint)), this, SLOT(contextualMenu(QPoint)));
 
+    connect(bumpFeeAction, SIGNAL(triggered()), this, SLOT(bumpFee()));
     connect(abandonAction, SIGNAL(triggered()), this, SLOT(abandonTx()));
     connect(copyAddressAction, SIGNAL(triggered()), this, SLOT(copyAddress()));
     connect(copyLabelAction, SIGNAL(triggered()), this, SLOT(copyLabel()));
@@ -372,6 +376,7 @@ void TransactionView::contextualMenu(const QPoint &point)
     uint256 hash;
     hash.SetHex(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
     abandonAction->setEnabled(model->transactionCanBeAbandoned(hash));
+    bumpFeeAction->setEnabled(model->transactionSignalsRBF(hash));
 
     if(index.isValid())
     {
@@ -397,6 +402,24 @@ void TransactionView::abandonTx()
     model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);
 }
 
+void TransactionView::bumpFee()
+{
+    if(!transactionView || !transactionView->selectionModel())
+        return;
+    QModelIndexList selection = transactionView->selectionModel()->selectedRows(0);
+
+    // get the hash from the TxHashRole (QVariant / QString)
+    uint256 hash;
+    QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
+    hash.SetHex(hashQStr.toStdString());
+
+    // Bump tx fee over the walletModel
+    if (model->bumpFee(hash)) {
+        // Update the table
+        model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);
+    }
+}
+
 void TransactionView::copyAddress()
 {
     GUIUtil::copyEntryData(transactionView, 0, TransactionTableModel::AddressRole);

src/qt/transactionview.h

@@ -76,6 +76,7 @@ private:
     QDateTimeEdit *dateFrom;
     QDateTimeEdit *dateTo;
     QAction *abandonAction;
+    QAction *bumpFeeAction;
 
     QWidget *createDateRangeWidget();
 
@@ -99,6 +100,7 @@ private Q_SLOTS:
     void openThirdPartyTxUrl(QString url);
     void updateWatchOnlyColumn(bool fHaveWatchOnly);
     void abandonTx();
+    void bumpFee();
 
 Q_SIGNALS:
     void doubleClicked(const QModelIndex&);

src/qt/walletmodel.cpp

@@ -8,8 +8,10 @@
 #include "consensus/validation.h"
 #include "guiconstants.h"
 #include "guiutil.h"
+#include "optionsmodel.h"
 #include "paymentserver.h"
 #include "recentrequeststablemodel.h"
+#include "sendcoinsdialog.h"
 #include "transactiontablemodel.h"
 
 #include "base58.h"
@@ -17,15 +19,18 @@
 #include "keystore.h"
 #include "validation.h"
 #include "net.h" // for g_connman
+#include "policy/rbf.h"
 #include "sync.h"
 #include "ui_interface.h"
 #include "util.h" // for GetBoolArg
+#include "wallet/feebumper.h"
 #include "wallet/wallet.h"
 #include "wallet/walletdb.h" // for BackupWallet
 
 #include <stdint.h>
 
 #include <QDebug>
+#include <QMessageBox>
 #include <QSet>
 #include <QTimer>
 
@@ -693,6 +698,86 @@ bool WalletModel::abandonTransaction(uint256 hash) const
     return wallet->AbandonTransaction(hash);
 }
 
+bool WalletModel::transactionSignalsRBF(uint256 hash) const
+{
+    LOCK2(cs_main, wallet->cs_wallet);
+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
+    if (wtx && SignalsOptInRBF(*wtx))
+        return true;
+    return false;
+}
+
+bool WalletModel::bumpFee(uint256 hash)
+{
+    std::unique_ptr<CFeeBumper> feeBump;
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        feeBump.reset(new CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true));
+    }
+    if (feeBump->getResult() != BumpFeeResult::OK)
+    {
+        QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
+            (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");
+         return false;
+    }
+
+    // allow a user based fee verification
+    QString questionString = tr("Do you want to increase the fee?");
+    questionString.append("<br />");
+    CAmount oldFee = feeBump->getOldFee();
+    CAmount newFee = feeBump->getNewFee();
+    questionString.append("<table style=\"text-align: left;\">");
+    questionString.append("<tr><td>");
+    questionString.append(tr("Current fee:"));
+    questionString.append("</td><td>");
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), oldFee));
+    questionString.append("</td></tr><tr><td>");
+    questionString.append(tr("Increase:"));
+    questionString.append("</td><td>");
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee - oldFee));
+    questionString.append("</td></tr><tr><td>");
+    questionString.append(tr("New fee:"));
+    questionString.append("</td><td>");
+    questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee));
+    questionString.append("</td></tr></table>");
+    SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), questionString);
+    confirmationDialog.exec();
+    QMessageBox::StandardButton retval = (QMessageBox::StandardButton)confirmationDialog.result();
+
+    // cancel sign&broadcast if users doesn't want to bump the fee
+    if (retval != QMessageBox::Yes) {
+        return false;
+    }
+
+    WalletModel::UnlockContext ctx(requestUnlock());
+    if(!ctx.isValid())
+    {
+        return false;
+    }
+
+    // sign bumped transaction
+    bool res = false;
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        res = feeBump->signTransaction(wallet);
+    }
+    if (!res) {
+        QMessageBox::critical(0, tr("Fee bump error"), tr("Can't sign transaction."));
+        return false;
+    }
+    // commit the bumped transaction
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        res = feeBump->commit(wallet);
+    }
+    if(!res) {
+        QMessageBox::critical(0, tr("Fee bump error"), tr("Could not commit transaction") + "<br />(" +
+            QString::fromStdString(feeBump->getErrors()[0])+")");
+         return false;
+    }
+    return true;
+}
+
 bool WalletModel::isWalletEnabled()
 {
    return !GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET);

src/qt/walletmodel.h

@@ -207,6 +207,9 @@ public:
     bool transactionCanBeAbandoned(uint256 hash) const;
     bool abandonTransaction(uint256 hash) const;
 
+    bool transactionSignalsRBF(uint256 hash) const;
+    bool bumpFee(uint256 hash);
+
     static bool isWalletEnabled();
 
     bool hdEnabled() const;

src/wallet/feebumper.cpp

@@ -41,6 +41,31 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal
     return GetVirtualTransactionSize(txNew);
 }
 
+bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx) {
+    if (pWallet->HasWalletSpend(wtx.GetHash())) {
+        vErrors.push_back("Transaction has descendants in the wallet");
+        currentResult = BumpFeeResult::INVALID_PARAMETER;
+        return false;
+    }
+
+    {
+        LOCK(mempool.cs);
+        auto it_mp = mempool.mapTx.find(wtx.GetHash());
+        if (it_mp != mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1) {
+            vErrors.push_back("Transaction has descendants in the mempool");
+            currentResult = BumpFeeResult::INVALID_PARAMETER;
+            return false;
+        }
+    }
+
+    if (wtx.GetDepthInMainChain() != 0) {
+        vErrors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
+        currentResult = BumpFeeResult::WALLET_ERROR;
+        return false;
+    }
+    return true;
+}
+
 CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable)
     :
     txid(std::move(txidIn)),
@@ -58,25 +83,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf
     auto it = pWallet->mapWallet.find(txid);
     const CWalletTx& wtx = it->second;
 
-    if (pWallet->HasWalletSpend(txid)) {
+    if (!preconditionChecks(pWallet, wtx)) {
-        vErrors.push_back("Transaction has descendants in the wallet");
-        currentResult = BumpFeeResult::INVALID_PARAMETER;
-        return;
-    }
-
-    {
-        LOCK(mempool.cs);
-        auto it_mp = mempool.mapTx.find(txid);
-        if (it_mp != mempool.mapTx.end() && it_mp->GetCountWithDescendants() > 1) {
-            vErrors.push_back("Transaction has descendants in the mempool");
-            currentResult = BumpFeeResult::INVALID_PARAMETER;
-            return;
-        }
-    }
-
-    if (wtx.GetDepthInMainChain() != 0) {
-        vErrors.push_back("Transaction has been mined, or is conflicted with a mined transaction");
-        currentResult = BumpFeeResult::WALLET_ERROR;
         return;
     }
 
@@ -248,6 +255,11 @@ bool CFeeBumper::commit(CWallet *pWallet)
     }
     CWalletTx& oldWtx = pWallet->mapWallet[txid];
 
+    // make sure the transaction still has no descendants and hasen't been mined in the meantime
+    if (!preconditionChecks(pWallet, oldWtx)) {
+        return false;
+    }
+
     CWalletTx wtxBumped(pWallet, MakeTransactionRef(std::move(mtx)));
     // commit/broadcast the tx
     CReserveKey reservekey(pWallet);

src/wallet/feebumper.h

@@ -8,6 +8,7 @@
 #include <primitives/transaction.h>
 
 class CWallet;
+class CWalletTx;
 class uint256;
 
 enum class BumpFeeResult
@@ -44,6 +45,8 @@ public:
     bool commit(CWallet *pWalletNonConst);
 
 private:
+    bool preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx);
+
     const uint256 txid;
     uint256 bumpedTxid;
     CMutableTransaction mtx;

src/wallet/wallet.cpp

@@ -2311,6 +2311,8 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
 
 bool CWallet::SignTransaction(CMutableTransaction &tx)
 {
+    AssertLockHeld(cs_wallet); // mapWallet
+
     // sign the new tx
     CTransaction txNewConst(tx);
     int nIn = 0;