[Qt] simple fee bumper with user verification #9697

jonasschnelli · 2017-02-06T14:33:23Z

~~Based on #9681.~~
Add "Increase transaction fee" action to the transaction table context menu.
The user can verify the new (and old) fee before sign & commit.

ryanofsky · 2017-02-06T21:43:36Z

src/qt/walletmodel.cpp

+{
+    LOCK2(cs_main, wallet->cs_wallet);
+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
+    if (wtx || SignalsOptInRBF(*wtx))


wtx && SignalsOptInRBF(*wtx)

ryanofsky · 2017-02-06T21:52:28Z

src/qt/walletmodel.cpp

+    std::vector<std::string> vErrors;
+
+    // do a simple feebump with default confirmation target
+    CWallet::BumpFeeResult res = wallet->BumpFee(hash, 0, false, 0, true, nOldFee, nNewFee, wtxRef, vErrors, verificationCallback);


I'm surprised it's ok to be holding onto the cs_main and cs_wallet locks while waiting for user input from a confirmation dialog, since it would seem to grind the whole bitcoin node to a halt. But maybe there are other precedents for doing this.

If you wanted to avoid it, though, I think you easily could by splitting bumpfee into two functions: one which creates the new transaction, and the other which commits this.

jonasschnelli · 2017-02-07T08:12:17Z

Thanks for the review @ryanofsky. Yes. The current situation with holding cs_main/cs_wallet during the QMessageBox must be avoided. Using LEAVE_CRITICAL_SECTION and ENTER_CRITICAL_SECTION would be another alternative, but I'd like your idea about splitting bumpFee into a part where we create the tx and a part where we sign and commit.
The signing part could potentially be shared with CreateTransaction in future.

jonasschnelli · 2017-02-14T08:51:01Z

Rebased on top of #9681.
Fixed the cs_main & cs_wallet lock problems mentioned by @ryanofsky.

ryanofsky

Tested ACK f7132c3.

Confirmed fee bumping, error handling, and RBF detection all work correctly. The interface is very clean and convenient, and it seems like it could be logically extended to give users control of the new fee in the future.

ryanofsky · 2017-02-24T15:51:56Z

src/qt/walletmodel.cpp

@@ -692,6 +697,65 @@ bool WalletModel::abandonTransaction(uint256 hash) const
    return wallet->AbandonTransaction(hash);
 }

+bool WalletModel::transactionSignalsRBF(uint256 hash) const
+{
+    LOCK2(cs_main, wallet->cs_wallet);


Doesn't look like lock is needed here (GetWalletTx acquires lock internally).

ryanofsky · 2017-02-24T15:54:03Z

src/qt/walletmodel.cpp

+{
+    LOCK2(cs_main, wallet->cs_wallet);
+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
+    if (wtx && SignalsOptInRBF(*wtx))


I'd slightly prefer SignalsOptInRBF(wtx->tx), because the implicit CWalletTx -> CTransaction conversion seems kind of hacky to me.

Maybe also get rid of if statement and just return wtx && SignalsOptInRBF(wtx->tx);

Should probably check that it is also unconfirmed, no? And, if you do keep the cs_main lock, maybe the in-mempool deps check that the CFeeBumper does.

ryanofsky · 2017-02-24T15:57:33Z

src/qt/walletmodel.cpp

+
+bool WalletModel::bumpFee(uint256 hash)
+{
+    std::shared_ptr<CFeeBumper> feeBump;


Maybe prefer unique_ptr to shared_ptr since pointer isn't being shared.

ryanofsky · 2017-02-24T16:05:29Z

src/qt/walletmodel.cpp

+    if (feeBump->getResult() != CFeeBumper::BumpFeeResult::OK)
+    {
+        QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
+            (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");


Are we able to write unit tests to exercise our qt interface code? I don't know very much about qt, but googling around it seems to be possible: http://doc.qt.io/qt-5/qttestlib-tutorial3-example.html

Asking because the logic here isn't totally trivial, and this seems like something we would want automated testing for to ensure it doesn't break in the future, especially if there will be more refactorings.

I started writing unit tests for this change: ryanofsky@9bfbdd8

They are mostly complete, but have a few things I want to fix up. They also required changes to the testing framework which I'm going to split off and make a separate PR.

Here's an updated version of the tests I wrote previously: ryanofsky@71c0a80

Feel free to cherry-pick or I can submit as a separate PR.

ryanofsky · 2017-02-24T16:18:03Z

src/qt/walletmodel.cpp

+{
+    std::shared_ptr<CFeeBumper> feeBump;
+    {
+        LOCK2(cs_main, wallet->cs_wallet);


Seems like it would be nice for this code if CFeeBumper constructor, wallet SignTransaction method, and CFeeBumper commit could acquire the locks they need themselves. Not asking for a change, since I don't want to hold up #9697, but curious if there was something that got in the way of doing this.

laanwj · 2017-03-14T08:12:19Z

Adding milestone for 0.15

jtimon · 2017-03-18T19:23:50Z

src/wallet/wallet.h

+        BumpFeeResult_INVALID_PARAMETER,
+        BumpFeeResult_WALLET_ERROR,
+        BumpFeeResult_MISC_ERROR,
+    };


The function doesn't seem to return anything and the enum is not used anywhere it seems.
Perhaps the function could start as void in this code movement and change it afterwards?

jtimon · 2017-03-18T19:29:05Z

src/wallet/wallet.cpp

    }
    return GetVirtualTransactionSize(txNew);
 }

 enum CWallet::BumpFeeResult CWallet::BumpFee(uint256 txid, int newConfirmTarget, bool specifiedConfirmTarget, CAmount totalFee, bool newTxReplaceable, CAmount& nOldFee, CAmount& nNewFee, std::shared_ptr<CWalletTx>& wtxNew, std::vector<std::string>& vErrors)
 {
-    if (!pwalletMain->mapWallet.count(hash)) {


Did the previous commit compile without hash?
I though we wanted every commit to compile and pass tests (for git bisect but also for general history cleanness).

jtimon · 2017-03-18T19:31:37Z

src/wallet/wallet.cpp


    // calculate the old fee and fee-rate
-    CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();


This could be done when making nOldFee a parameter of this function too (ie the function doesn't need to start in his final form).

jtimon · 2017-03-18T19:38:09Z

src/wallet/rpcwallet.cpp

            default:
                throw JSONRPCError(RPC_MISC_ERROR, vErrors[0]);
                break;
        }
    }
+


Seems weird and unnecessary to move this to wallet and then move it back here. Perhaps SignTransaction/BumpFeePrepare/BumpFeeCommit should be created before BumpFee (ie BumpFee never being created to just remove it in the same PR) ?

jtimon · 2017-03-18T19:39:53Z

utACK individual commits: 8c63160 641c99c (without fully verifying code movements, but look ok) f0c98b9
Fast review 65e2e00
It seems to me that the history in this PR is unnecessarily complicated and not very clean (and I believe it doesn't compile commit by commit, but I haven't checked).
Needs rebase.

ryanofsky · 2017-03-18T20:31:55Z

@jtimon, if you are interested in reviewing this PR, the only commit worth looking at is the final one: "[Qt] simple fee bumper with user verification" (f7132c3). The preceding commits which you reviewed were just pulled in from an old version of #9681, and they will disappear after #9681 is merged and this is rebased.

If you want to review the latest version of the feebumper code in #9681, that would be very helpful. #9681 is fully up to date and its commit history has been simplified.

jonasschnelli · 2017-04-10T12:07:16Z

Rebased (since #9681 is now merged) and overhauled.
This now also works with encrypted wallets.

flack · 2017-04-25T20:47:08Z

It's kind of hard to see in the confirmation dialog by how much the fee will actually increase. Could you work that somehow into your sentence, or maybe add a table like this:

Current fee	0.00010595
increase	0.00000960
New fee	0.00011555

jonasschnelli · 2017-04-26T08:21:36Z

Implemented @flack's idea.
Now it looks like:

jtimon · 2017-04-26T19:15:32Z

Perhaps it is too much info for the confirmation form, but what about showing the current feerate and the new feerate as well?

laanwj · 2017-05-01T13:29:36Z

@jonasschnelli that looks good!

laanwj · 2017-05-01T14:06:10Z

I tested this out a bit on testnet; seems to work! one nit. I bumped a single transaction a few times, and it showed it as being sent multiple times:

This could be scary to users, as it looks like the transaction is sent successfully multiple times, and will possibly be mined multiple times.

After restart it was better:

After mining the transaction, proving it works:

ryanofsky

ACK 488ca9f

ryanofsky · 2017-05-01T13:47:37Z

src/qt/sendcoinsdialog.h


 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);


In commit "[Qt] simple fee bumper with user verification"

Maybe get rid of the SEND_CONFIRM_DELAY usage in sendcoinsdialog.cpp now that this is the default. This would make it easier to see that changing the default doesn't actually affect current behavior.

@ryanofsky: because setting QWidget *parent = 0 is required, that would either result in swapping QWidget *parent = 0 with int secDelay = SEND_CONFIRM_DELAY or passing SEND_CONFIRM_DELAY in sendcoinsdialog.cpp (as it is by now), right?
The current situation seems to be okay to me.
Or do I misunderstand something?

Or do I misunderstand something?

No you're right. I didn't see sendcoinsdialog is also passing a this argument, so there is no way to use the default delay argument.

Why not just get rid of the argument entirely? I dont see any callers of the constructor using a non-default value?

ryanofsky · 2017-05-01T15:30:04Z

src/qt/walletmodel.cpp

+    if (feeBump->getResult() != CFeeBumper::BumpFeeResult::OK)
+    {
+        QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
+            (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");


Here's an updated version of the tests I wrote previously: ryanofsky@71c0a80

Feel free to cherry-pick or I can submit as a separate PR.

jonasschnelli · 2017-05-04T07:34:14Z

What would be the best solution regarding bump conflicts?

At the moment RPCs listtransaction does also list all previous tx in a bump (you can see the walletconflicts there).

Completely hiding the conflicted transactions (only own bump conflicts) would be a solution though I would provide to just "flagging" (color or opacity) the conflicts in mempool.
I once had a PR for that: #7826
(EDIT: Maybe this comments makes sense: #7826 (comment))

Any ideas?

jonasschnelli · 2017-05-04T08:27:10Z

Fixed @laanwj's point.
Added a commit that ensures the table model row gets updated correctly after a bump.

ryanofsky

ACK 1015ef40ea37a33faeded844e2da62cc6c66b0bf

ryanofsky · 2017-05-04T15:46:47Z

src/qt/sendcoinsdialog.h


 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);


Or do I misunderstand something?

No you're right. I didn't see sendcoinsdialog is also passing a this argument, so there is no way to use the default delay argument.

ryanofsky · 2017-05-04T15:53:37Z

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);


In commit "Make sure we always update the table row after a bumpfee call"

I was surprised that passing false for the updateTransaction showTransaction parameter doesn't hide the transaction, though it's good that it doesn't. Do you think this parameter should be removed (separately, not part of this change)? I'm not seeing what it does other than set a temporary state that gets immediately overwritten.

Heh. Yes. This is really surprising and yes, I think we should remove it (in a clean follow up PR).

TheBlueMatt

As for not displaying bumps, I'd advocate for just hiding them wholesale in TransactionRecord::showTransaction (unless the original, non-bumped version got confirmed).

TheBlueMatt · 2017-05-09T18:39:50Z

src/qt/sendcoinsdialog.h


 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);


Why not just get rid of the argument entirely? I dont see any callers of the constructor using a non-default value?

TheBlueMatt · 2017-05-09T18:45:01Z

src/qt/walletmodel.cpp

+{
+    LOCK2(cs_main, wallet->cs_wallet);
+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
+    if (wtx && SignalsOptInRBF(*wtx))


Should probably check that it is also unconfirmed, no? And, if you do keep the cs_main lock, maybe the in-mempool deps check that the CFeeBumper does.

TheBlueMatt · 2017-05-09T18:51:27Z

src/qt/walletmodel.cpp

+    std::unique_ptr<CFeeBumper> feeBump;
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        feeBump.reset(new CFeeBumper(wallet, hash, 0, false, 0, true));


I'm confused...looks like this will either use the payTxFee per-wallet global or fall back to fallbackFee (as we call into GetMinimumFee with nConfirmTarget set to 0, which will fail the estimateSmartFee). This will likely normally result in the user always just bumping by walletIncrementalRelayFee, which seems like a strange choice. Was this intentional, or are you just planning on following up with a tweak to that in the future?

Oh. Right. Here it misses nTxConfirmTarget. Will fix.

TheBlueMatt · 2017-05-09T19:05:06Z

src/qt/walletmodel.cpp

+    // commit the bumped transaction
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        res = feeBump->commit(wallet);


I believe this is the first use of CFeeBumper without holding cs_wallet through the object's entire lifetime, so more thourough review of that is merited. I read through it and /think/ its OK, but it would probably be good to duplicate a few of the checks in the CFeeBumper constructor in commit, specifically the HasWalletSpend/mempool descendant count, wtx.mapValue.count("replaced_by_txid"), and GetDepthInMainChain() != 0 ones.

jonasschnelli · 2017-05-11T07:40:20Z

Added 3 commits to address @TheBlueMatt findings.

Make sure to pass nConfirmTarget during Qt fee bumps (d9afe0192c9ad83594074661d55a4242f33de462)
Re-check the conditions during feebump "commit". Because the Qt dialog may stay open for a couple of seconds (or even minutes), we need to re-check the descendants and if the tx has been mined (717495875b995c63e3e4166245e88e8def7f1e47). This still has room to optimise. We may want to timeout a bump...
Don't hide rows when the bump has been cancelled (c9c48ea36fab3c0c36665289cbb4038491aa1354)

jonasschnelli · 2017-05-11T13:27:15Z

Rebased.

paveljanik · 2017-05-17T08:50:10Z

src/wallet/feebumper.cpp

@@ -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


typo "hasen't"

paveljanik · 2017-05-17T08:56:31Z

Minor nit: can we gray "Increase transaction fee" menu entry when on transaction that was already fee-bumped?

It has no reason to allow bumping the fee when it already was fee bumped and clicking on the menu entry will show

Increasing transaction fee failed
(Cannot bump transaction xyz which was already bumped by transaction abc)

paveljanik · 2017-05-17T08:59:59Z

... and now when my tx has been mined, there is no reason to show this menu entry at all (because clicking on it will tell me it is mined already).

laanwj · 2017-05-18T08:06:08Z

utACK a387837

I think this is ready for merging. Sure, it's not perfect yet, but it's usable and can be improved later.
It has a reasonable number of ACKs/utACKs and nits have been fixed a few times.

Anyone disagree?

ryanofsky · 2017-05-18T09:08:09Z

utACK a387837. I agree with merging.

Changes since my last review were just rebase on updated master and 3 new commits.

a387837 Make sure we re-check the conditions of a feebump during commit (Jonas Schnelli) 9b9ca53 Only update the transactionrecord if the fee bump has been commited (Jonas Schnelli) 6ed4368 Make sure we use nTxConfirmTarget during Qt fee bumps (Jonas Schnelli) be08fc3 Make sure we always update the table row after a bumpfee call (Jonas Schnelli) 2678d3d Show old-fee, increase a new-fee in Qt fee bumper confirmation dialog (Jonas Schnelli) 2ec911f Add cs_wallet lock assertion to SignTransaction() (Jonas Schnelli) fbf385c [Qt] simple fee bumper with user verification (Jonas Schnelli) Tree-SHA512: a3ce626201abf64cee496dd1d83870de51ba633de40c48eb0219c3eba5085c038af34c284512130d2544de20c1bff9fea1b78f92e3574c21dd4e96c11b8e7d76

jonasschnelli added the GUI label Feb 6, 2017

jonasschnelli force-pushed the 2017/02/qt_bumpfee branch from a62aac3 to 8d7bbff Compare February 6, 2017 14:35

ryanofsky mentioned this pull request Feb 6, 2017

Refactor Bumpfee, move core functionality to CWallet #9681

Merged

ryanofsky reviewed Feb 6, 2017

View reviewed changes

jonasschnelli force-pushed the 2017/02/qt_bumpfee branch 4 times, most recently from 885027c to f7132c3 Compare February 14, 2017 08:49

ryanofsky reviewed Feb 24, 2017

View reviewed changes

This was referenced Mar 8, 2017

[Qt] Add checkbox in the GUI to opt-in to RBF when creating a transaction #9592

Merged

Add basic Qt wallet test #9974

Merged

laanwj added this to the 0.15.0 milestone Mar 14, 2017

jtimon reviewed Mar 18, 2017

View reviewed changes

jonasschnelli force-pushed the 2017/02/qt_bumpfee branch from f7132c3 to f1485f9 Compare April 10, 2017 12:04

jonasschnelli mentioned this pull request Apr 20, 2017

Wallets: add fee handling info to Choose Your Wallet page bitcoin-dot-org/Bitcoin.org#1568

Merged

jonasschnelli force-pushed the 2017/02/qt_bumpfee branch from 3a24406 to 488ca9f Compare April 26, 2017 07:58

ryanofsky approved these changes May 1, 2017

View reviewed changes

ryanofsky reviewed May 4, 2017

View reviewed changes

TheBlueMatt reviewed May 9, 2017

View reviewed changes

jonasschnelli added 7 commits May 11, 2017 15:27

[Qt] simple fee bumper with user verification

fbf385c

Add cs_wallet lock assertion to SignTransaction()

2ec911f

Show old-fee, increase a new-fee in Qt fee bumper confirmation dialog

2678d3d

Make sure we always update the table row after a bumpfee call

be08fc3

Make sure we use nTxConfirmTarget during Qt fee bumps

6ed4368

Only update the transactionrecord if the fee bump has been commited

9b9ca53

Make sure we re-check the conditions of a feebump during commit

a387837

jonasschnelli force-pushed the 2017/02/qt_bumpfee branch from 7174958 to a387837 Compare May 11, 2017 13:27

paveljanik approved these changes May 17, 2017

View reviewed changes

jonasschnelli merged commit a387837 into bitcoin:master May 18, 2017

jnewbery mentioned this pull request Jul 31, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

Sjors mentioned this pull request Oct 25, 2017

[Qt] Improved copy for RBF checkbox and tooltip #11556

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		// calculate the old fee and fee-rate
		CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();

[Qt] simple fee bumper with user verification #9697

[Qt] simple fee bumper with user verification #9697

Uh oh!

Conversation

jonasschnelli commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 7, 2017

Uh oh!

jonasschnelli commented Feb 14, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 18, 2017

Uh oh!

ryanofsky commented Mar 18, 2017

Uh oh!

jonasschnelli commented Apr 10, 2017

Uh oh!

flack commented Apr 25, 2017

Uh oh!

jonasschnelli commented Apr 26, 2017

Uh oh!

jtimon commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 1, 2017

Uh oh!

laanwj commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Feb 6, 2017 •

edited

Loading

jtimon Mar 18, 2017 •

edited

Loading

jtimon commented Apr 26, 2017 •

edited

Loading

laanwj commented May 1, 2017 •

edited

Loading

jonasschnelli commented May 4, 2017 •

edited

Loading