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.
<img width="246" alt="bildschirmfoto 2017-02-06 um 15 31 34" src="https://cloud.githubusercontent.com/assets/178464/22650945/7b398aa6-ec81-11e6-95f1-75c19737e885.png"> <img width="532" alt="bildschirmfoto 2017-02-06 um 15 31 36" src="https://cloud.githubusercontent.com/assets/178464/22650948/7cfe142e-ec81-11e6-87b2-45143c2b7d78.png">
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))
wtx && SignalsOptInRBF(*wtx)
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);
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.
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.
Rebased on top of #9681.
Fixed the cs_main & cs_wallet lock problems mentioned by @ryanofsky.
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);
Doesn't look like lock is needed here (GetWalletTx acquires lock internally).
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))
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.
706 | + return false; 707 | +} 708 | + 709 | +bool WalletModel::bumpFee(uint256 hash) 710 | +{ 711 | + std::shared_ptr<CFeeBumper> feeBump;
Maybe prefer unique_ptr to shared_ptr since pointer isn't being shared.
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]) : "") +")");
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: 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.
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.
708 | + 709 | +bool WalletModel::bumpFee(uint256 hash) 710 | +{ 711 | + std::shared_ptr<CFeeBumper> feeBump; 712 | + { 713 | + 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.
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.
Adding milestone for 0.15
800 | + BumpFeeResult_INVALID_ADDRESS_OR_KEY, 801 | + BumpFeeResult_INVALID_REQUEST, 802 | + BumpFeeResult_INVALID_PARAMETER, 803 | + BumpFeeResult_WALLET_ERROR, 804 | + BumpFeeResult_MISC_ERROR, 805 | + };
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?
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)) {
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).
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();
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).
2782 | default:
2783 | throw JSONRPCError(RPC_MISC_ERROR, vErrors[0]);
2784 | break;
2785 | }
2786 | }
2787 | +
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) ?
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.
@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.
Rebased (since #9681 is now merged) and overhauled. This now also works with encrypted wallets.
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 |
Implemented @flack's idea. Now it looks like:
<img width="455" alt="bildschirmfoto 2017-04-26 um 09 59 11" src="https://cloud.githubusercontent.com/assets/178464/25424009/03e651a4-2a67-11e7-849f-017d5ef522c6.png">
<img width="332" alt="bildschirmfoto 2017-04-26 um 10 20 40" src="https://cloud.githubusercontent.com/assets/178464/25424798/0925c462-2a6a-11e7-978d-7b9ec24decb2.png">
Perhaps it is too much info for the confirmation form, but what about showing the current feerate and the new feerate as well?
@jonasschnelli that looks good!
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);
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?
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:
ACK 488ca9f9c78d7117d11e7f1f0be14d51065f7110
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?
Fixed @laanwj's point. Added a commit that ensures the table model row gets updated correctly after a bump.
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);
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).
ACK 1015ef40ea37a33faeded844e2da62cc6c66b0bf
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));
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.
765 | + return false; 766 | + } 767 | + // commit the bumped transaction 768 | + { 769 | + LOCK2(cs_main, wallet->cs_wallet); 770 | + 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.
As for not displaying bumps, I'd advocate for just hiding them wholesale in TransactionRecord::showTransaction (unless the original, non-bumped version got confirmed).
Added 3 commits to address @TheBlueMatt findings.
nConfirmTarget during Qt fee bumps (d9afe0192c9ad83594074661d55a4242f33de462)
Rebased.
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
typo "hasen't"
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)
... 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).
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?
utACK a387837. I agree with merging.
Changes since my last review were just rebase on updated master and 3 new commits.
Milestone
0.15.0