From fbf385cc830df2aec7cdcbab0c2b09b46569e8c1 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Fri, 3 Feb 2017 22:04:39 +0100 Subject: [Qt] simple fee bumper with user verification --- src/qt/sendcoinsdialog.cpp | 2 -- src/qt/sendcoinsdialog.h | 3 +- src/qt/transactionview.cpp | 22 ++++++++++++++- src/qt/transactionview.h | 2 ++ src/qt/walletmodel.cpp | 70 ++++++++++++++++++++++++++++++++++++++++++++++ src/qt/walletmodel.h | 3 ++ 6 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index 098cda6d32..8dcb0fd016 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -31,8 +31,6 @@ #include #include -#define SEND_CONFIRM_DELAY 3 - SendCoinsDialog::SendCoinsDialog(const PlatformStyle *_platformStyle, QWidget *parent) : QDialog(parent), ui(new Ui::SendCoinsDialog), diff --git a/src/qt/sendcoinsdialog.h b/src/qt/sendcoinsdialog.h index a402edc961..a932f129be 100644 --- a/src/qt/sendcoinsdialog.h +++ b/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: diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 30f4db9450..af48f13c24 100644 --- a/src/qt/transactionview.cpp +++ b/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,21 @@ 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 + model->bumpFee(hash); +} + void TransactionView::copyAddress() { GUIUtil::copyEntryData(transactionView, 0, TransactionTableModel::AddressRole); diff --git a/src/qt/transactionview.h b/src/qt/transactionview.h index 595701cdd9..52e57cae4c 100644 --- a/src/qt/transactionview.h +++ b/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&); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index a2a9271904..bcf8d5cab3 100644 --- a/src/qt/walletmodel.cpp +++ b/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 #include +#include #include #include @@ -693,6 +698,71 @@ 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 feeBump; + { + LOCK2(cs_main, wallet->cs_wallet); + feeBump.reset(new CFeeBumper(wallet, hash, 0, false, 0, true)); + } + if (feeBump->getResult() != BumpFeeResult::OK) + { + QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "
(" + + (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 from %1 to %2").arg( + BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), feeBump->getOldFee()), + BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), feeBump->getNewFee())); + 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") + "
(" + + QString::fromStdString(feeBump->getErrors()[0])+")"); + return false; + } + return true; +} + bool WalletModel::isWalletEnabled() { return !GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 78e45dc369..df5acaf684 100644 --- a/src/qt/walletmodel.h +++ b/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; -- cgit v1.2.3 From 2ec911f60dc89133ffcf91a6deccc2c7ddb434e6 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 10 Apr 2017 14:05:49 +0200 Subject: Add cs_wallet lock assertion to SignTransaction() --- src/wallet/wallet.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ea329d6ebe..c4600446a8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2277,6 +2277,8 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm bool CWallet::SignTransaction(CMutableTransaction &tx) { + AssertLockHeld(cs_wallet); // mapWallet + // sign the new tx CTransaction txNewConst(tx); int nIn = 0; -- cgit v1.2.3 From 2678d3dc63d5a47fe55a5f96de77174051536784 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Wed, 26 Apr 2017 09:55:54 +0200 Subject: Show old-fee, increase a new-fee in Qt fee bumper confirmation dialog --- src/qt/walletmodel.cpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index bcf8d5cab3..7c0e22b89b 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -722,9 +722,24 @@ bool WalletModel::bumpFee(uint256 hash) } // allow a user based fee verification - QString questionString = tr("Do you want to increase the fee from %1 to %2").arg( - BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), feeBump->getOldFee()), - BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), feeBump->getNewFee())); + QString questionString = tr("Do you want to increase the fee?"); + questionString.append("
"); + CAmount oldFee = feeBump->getOldFee(); + CAmount newFee = feeBump->getNewFee(); + questionString.append(""); + questionString.append("
"); + questionString.append(tr("Current fee:")); + questionString.append(""); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), oldFee)); + questionString.append("
"); + questionString.append(tr("Increase:")); + questionString.append(""); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee - oldFee)); + questionString.append("
"); + questionString.append(tr("New fee:")); + questionString.append(""); + questionString.append(BitcoinUnits::formatHtmlWithUnit(getOptionsModel()->getDisplayUnit(), newFee)); + questionString.append("
"); SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), questionString); confirmationDialog.exec(); QMessageBox::StandardButton retval = (QMessageBox::StandardButton)confirmationDialog.result(); -- cgit v1.2.3 From be08fc39d0343de931762eea9deb2b9115624bb8 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 4 May 2017 10:24:14 +0200 Subject: Make sure we always update the table row after a bumpfee call --- src/qt/transactionview.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index af48f13c24..4616685d35 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -415,6 +415,9 @@ void TransactionView::bumpFee() // Bump tx fee over the walletModel model->bumpFee(hash); + + // Update the table + model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false); } void TransactionView::copyAddress() -- cgit v1.2.3 From 6ed4368f121572b4860d42d0d48007716c640608 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 11 May 2017 09:33:05 +0200 Subject: Make sure we use nTxConfirmTarget during Qt fee bumps --- src/qt/walletmodel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 7c0e22b89b..37af8abb38 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -712,7 +712,7 @@ bool WalletModel::bumpFee(uint256 hash) std::unique_ptr feeBump; { LOCK2(cs_main, wallet->cs_wallet); - feeBump.reset(new CFeeBumper(wallet, hash, 0, false, 0, true)); + feeBump.reset(new CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true)); } if (feeBump->getResult() != BumpFeeResult::OK) { -- cgit v1.2.3 From 9b9ca538cdf95c3a16408229772b69b0c956fbc0 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 11 May 2017 09:33:40 +0200 Subject: Only update the transactionrecord if the fee bump has been commited --- src/qt/transactionview.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 4616685d35..9008c81634 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -414,10 +414,10 @@ void TransactionView::bumpFee() hash.SetHex(hashQStr.toStdString()); // Bump tx fee over the walletModel - model->bumpFee(hash); - - // Update the table - model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false); + if (model->bumpFee(hash)) { + // Update the table + model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false); + } } void TransactionView::copyAddress() -- cgit v1.2.3 From a38783747b1c9a9209f70204639e4efcea4193c6 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 11 May 2017 09:34:39 +0200 Subject: Make sure we re-check the conditions of a feebump during commit --- src/wallet/feebumper.cpp | 50 ++++++++++++++++++++++++++++++------------------ src/wallet/feebumper.h | 3 +++ 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index b3cb6a718c..c10a9eccd9 100644 --- a/src/wallet/feebumper.cpp +++ b/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)) { - 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; + if (!preconditionChecks(pWallet, wtx)) { 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); diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 681f55e4e5..f40d05da28 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -8,6 +8,7 @@ #include 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; -- cgit v1.2.3