[Qt] simple fee bumper with user verification

jonasschnelli commented at 2:33 pm on February 6, 2017: contributor

~~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.

jonasschnelli added the label GUI on Feb 6, 2017

jonasschnelli force-pushed on Feb 6, 2017

in src/qt/walletmodel.cpp: in 8d7bbff188 outdated

693@@ -692,6 +694,34 @@ bool WalletModel::abandonTransaction(uint256 hash) const
694     return wallet->AbandonTransaction(hash);
695 }
696 
697+bool WalletModel::transactionSignalsRBF(uint256 hash) const
698+{
699+    LOCK2(cs_main, wallet->cs_wallet);
700+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
701+    if (wtx || SignalsOptInRBF(*wtx))

ryanofsky commented at 9:43 pm on February 6, 2017:

wtx && SignalsOptInRBF(*wtx)

in src/qt/walletmodel.cpp: in 8d7bbff188 outdated

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

ryanofsky commented at 9:52 pm on February 6, 2017:

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 commented at 8:12 am on February 7, 2017: contributor

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 force-pushed on Feb 14, 2017

jonasschnelli commented at 8:51 am on February 14, 2017: contributor

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

in src/qt/walletmodel.cpp: in f7132c3b14 outdated

696@@ -692,6 +697,65 @@ bool WalletModel::abandonTransaction(uint256 hash) const
697     return wallet->AbandonTransaction(hash);
698 }
699 
700+bool WalletModel::transactionSignalsRBF(uint256 hash) const
701+{
702+    LOCK2(cs_main, wallet->cs_wallet);

ryanofsky commented at 3:51 pm on February 24, 2017:

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

in src/qt/walletmodel.cpp: in f7132c3b14 outdated

696@@ -692,6 +697,65 @@ bool WalletModel::abandonTransaction(uint256 hash) const
697     return wallet->AbandonTransaction(hash);
698 }
699 
700+bool WalletModel::transactionSignalsRBF(uint256 hash) const
701+{
702+    LOCK2(cs_main, wallet->cs_wallet);
703+    const CWalletTx *wtx = wallet->GetWalletTx(hash);
704+    if (wtx && SignalsOptInRBF(*wtx))

ryanofsky commented at 3:54 pm on February 24, 2017:

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

TheBlueMatt commented at 6:45 pm on May 9, 2017:

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.

in src/qt/walletmodel.cpp: in f7132c3b14 outdated

706+    return false;
707+}
708+
709+bool WalletModel::bumpFee(uint256 hash)
710+{
711+    std::shared_ptr<CFeeBumper> feeBump;

ryanofsky commented at 3:57 pm on February 24, 2017:

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

in src/qt/walletmodel.cpp: in f7132c3b14 outdated

714+        feeBump = std::make_shared<CFeeBumper>(CFeeBumper(wallet, hash, 0, false, 0, true));
715+    }
716+    if (feeBump->getResult() != CFeeBumper::BumpFeeResult::OK)
717+    {
718+        QMessageBox::critical(0, tr("Fee bump error"), tr("Increasing transaction fee failed") + "<br />(" +
719+            (feeBump->getErrors().size() ? QString::fromStdString(feeBump->getErrors()[0]) : "") +")");

ryanofsky commented at 4:05 pm on February 24, 2017:

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.

ryanofsky commented at 11:08 pm on March 9, 2017:

I started writing unit tests for this change: https://github.com/ryanofsky/bitcoin/commit/9bfbdd88dd0f1e11f06403e81f39fd8df1d047c7

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.

ryanofsky commented at 3:30 pm on May 1, 2017:

Here’s an updated version of the tests I wrote previously: https://github.com/ryanofsky/bitcoin/commit/71c0a80fbfe95262be3b81c45474aceaccddfad3

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

in src/qt/walletmodel.cpp: in f7132c3b14 outdated

708+
709+bool WalletModel::bumpFee(uint256 hash)
710+{
711+    std::shared_ptr<CFeeBumper> feeBump;
712+    {
713+        LOCK2(cs_main, wallet->cs_wallet);

ryanofsky commented at 4:18 pm on February 24, 2017:

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.

ryanofsky commented at 4:22 pm on February 24, 2017: member

Tested ACK f7132c3b14cf714f71a2fe59241223fc30520814.

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.

laanwj added this to the milestone 0.15.0 on Mar 14, 2017

laanwj commented at 8:12 am on March 14, 2017: member

Adding milestone for 0.15

in src/wallet/wallet.h:805 in 641c99cb6f outdated

800+        BumpFeeResult_INVALID_ADDRESS_OR_KEY,
801+        BumpFeeResult_INVALID_REQUEST,
802+        BumpFeeResult_INVALID_PARAMETER,
803+        BumpFeeResult_WALLET_ERROR,
804+        BumpFeeResult_MISC_ERROR,
805+    };

jtimon commented at 7:23 pm on March 18, 2017:

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?

in src/wallet/wallet.cpp:2323 in f0c98b9bfa outdated

2319     return GetVirtualTransactionSize(txNew);
2320 }
2321 
2322 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)
2323 {
2324-    if (!pwalletMain->mapWallet.count(hash)) {

jtimon commented at 7:29 pm on March 18, 2017:

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

in src/wallet/wallet.cpp:2378 in f0c98b9bfa outdated

2404+        vErrors.push_back("Transaction contains inputs that cannot be signed");
2405+        return CWallet::BumpFeeResult_INVALID_ADDRESS_OR_KEY;
2406+    }
2407 
2408     // calculate the old fee and fee-rate
2409-    CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();

jtimon commented at 7:31 pm on March 18, 2017:

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

in src/wallet/rpcwallet.cpp:2867 in 65e2e0085a outdated

2782             default:
2783                 throw JSONRPCError(RPC_MISC_ERROR, vErrors[0]);
2784                 break;
2785         }
2786     }
2787+

jtimon commented at 7:38 pm on March 18, 2017:

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 commented at 7:39 pm on March 18, 2017: contributor

utACK individual commits: 8c6316038e8ebb1877d712f29171afc4bb76a159 641c99cb6f96afa166a293917f36f3cf8f28a08a (without fully verifying code movements, but look ok) f0c98b9bfa763fbef87f1d9d37853f28e9451c87 Fast review 65e2e0085a608d9f570bc911349a5953a6ca88ee 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 commented at 8:31 pm on March 18, 2017: member

@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” (f7132c3b14cf714f71a2fe59241223fc30520814). 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 force-pushed on Apr 10, 2017

jonasschnelli commented at 12:07 pm on April 10, 2017: contributor

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

jonasschnelli added this to the "Blockers" column in a project

flack commented at 8:47 pm on April 25, 2017: contributor

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 force-pushed on Apr 26, 2017

jonasschnelli commented at 8:21 am on April 26, 2017: contributor

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

jtimon commented at 7:15 pm on April 26, 2017: contributor

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

laanwj commented at 1:29 pm on May 1, 2017: member

@jonasschnelli that looks good!

in src/qt/sendcoinsdialog.h:110 in f1485f9979 outdated

106 {
107     Q_OBJECT
108 
109 public:
110-    SendConfirmationDialog(const QString &title, const QString &text, int secDelay = 0, QWidget *parent = 0);
111+    SendConfirmationDialog(const QString &title, const QString &text, int secDelay = SEND_CONFIRM_DELAY, QWidget *parent = 0);

ryanofsky commented at 1:47 pm on May 1, 2017:

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.

jonasschnelli commented at 7:53 am on May 4, 2017:

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

ryanofsky commented at 3:46 pm on May 4, 2017:

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.

TheBlueMatt commented at 6:39 pm on May 9, 2017:

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

laanwj commented at 2:06 pm on May 1, 2017: member

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: untitled2

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: untitled3

After mining the transaction, proving it works: untitled3

ryanofsky approved

ryanofsky commented at 3:43 pm on May 1, 2017: member

ACK 488ca9f9c78d7117d11e7f1f0be14d51065f7110

jonasschnelli commented at 7:34 am on May 4, 2017: contributor

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 commented at 8:27 am on May 4, 2017: contributor

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

in src/qt/transactionview.cpp:420 in 1015ef40ea outdated

414@@ -415,6 +415,9 @@ void TransactionView::bumpFee()
415 
416     // Bump tx fee over the walletModel
417     model->bumpFee(hash);
418+
419+    // Update the table
420+    model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);

ryanofsky commented at 3:53 pm on May 4, 2017:

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.

jonasschnelli commented at 7:57 pm on May 4, 2017:

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

ryanofsky commented at 3:55 pm on May 4, 2017: member

ACK 1015ef40ea37a33faeded844e2da62cc6c66b0bf

in src/qt/walletmodel.cpp:714 in 1015ef40ea outdated

709+bool WalletModel::bumpFee(uint256 hash)
710+{
711+    std::unique_ptr<CFeeBumper> feeBump;
712+    {
713+        LOCK2(cs_main, wallet->cs_wallet);
714+        feeBump.reset(new CFeeBumper(wallet, hash, 0, false, 0, true));

TheBlueMatt commented at 6:51 pm on May 9, 2017:

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?

jonasschnelli commented at 7:15 am on May 11, 2017:

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

in src/qt/walletmodel.cpp:771 in 1015ef40ea outdated

765+        return false;
766+    }
767+    // commit the bumped transaction
768+    {
769+        LOCK2(cs_main, wallet->cs_wallet);
770+        res = feeBump->commit(wallet);

TheBlueMatt commented at 7:05 pm on May 9, 2017:

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.

TheBlueMatt commented at 7:06 pm on May 9, 2017: member

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

jonasschnelli commented at 7:40 am on May 11, 2017: contributor

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)

[Qt] simple fee bumper with user verification fbf385cc83

Add cs_wallet lock assertion to SignTransaction() 2ec911f60d

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

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

Make sure we use nTxConfirmTarget during Qt fee bumps 6ed4368f12

Only update the transactionrecord if the fee bump has been commited 9b9ca538cd

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

jonasschnelli commented at 1:27 pm on May 11, 2017: contributor

Rebased.

jonasschnelli force-pushed on May 11, 2017

in src/wallet/feebumper.cpp:258 in a38783747b

254@@ -248,6 +255,11 @@ bool CFeeBumper::commit(CWallet *pWallet)
255     }
256     CWalletTx& oldWtx = pWallet->mapWallet[txid];
257 
258+    // make sure the transaction still has no descendants and hasen't been mined in the meantime

paveljanik commented at 8:50 am on May 17, 2017:

typo “hasen’t”

paveljanik approved

paveljanik commented at 8:56 am on May 17, 2017: contributor

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

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

paveljanik commented at 8:59 am on May 17, 2017: contributor

… 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 commented at 8:06 am on May 18, 2017: member

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 commented at 9:08 am on May 18, 2017: member

utACK a387837. I agree with merging.

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

jonasschnelli merged this on May 18, 2017

jonasschnelli closed this on May 18, 2017

jonasschnelli referenced this in commit 962cd3f058 on May 18, 2017

jonasschnelli removed this from the "Blockers" column in a project

PastaPastaPasta referenced this in commit 1665a01808 on Jun 10, 2019

DrahtBot locked this on Sep 8, 2021

[Qt] simple fee bumper with user verification #9697