Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Qt] simple fee bumper with user verification #9697

Merged
merged 7 commits into from May 18, 2017

Conversation

Projects
None yet
7 participants
@jonasschnelli
Copy link
Member

jonasschnelli commented Feb 6, 2017

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.

bildschirmfoto 2017-02-06 um 15 31 34

bildschirmfoto 2017-02-06 um 15 31 36

@jonasschnelli jonasschnelli added the GUI label Feb 6, 2017

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/02/qt_bumpfee branch Feb 6, 2017

src/qt/walletmodel.cpp Outdated
{
LOCK2(cs_main, wallet->cs_wallet);
const CWalletTx *wtx = wallet->GetWalletTx(hash);
if (wtx || SignalsOptInRBF(*wtx))

This comment has been minimized.

@ryanofsky

ryanofsky Feb 6, 2017

Contributor

wtx && SignalsOptInRBF(*wtx)

src/qt/walletmodel.cpp Outdated
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);

This comment has been minimized.

@ryanofsky

ryanofsky Feb 6, 2017

Contributor

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

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Feb 7, 2017

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 jonasschnelli force-pushed the jonasschnelli:2017/02/qt_bumpfee branch 4 times, most recently to f7132c3 Feb 14, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Feb 14, 2017

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

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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.

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 24, 2017

Contributor

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

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 24, 2017

Contributor

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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.


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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 24, 2017

Contributor

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

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]) : "") +")");

This comment has been minimized.

@ryanofsky

ryanofsky Feb 24, 2017

Contributor

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.

This comment has been minimized.

@ryanofsky

ryanofsky Mar 9, 2017

Contributor

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.

This comment has been minimized.

@ryanofsky

ryanofsky May 1, 2017

Contributor

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.

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 24, 2017

Contributor

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

This comment has been minimized.

Copy link
Member

laanwj commented Mar 14, 2017

Adding milestone for 0.15

BumpFeeResult_INVALID_PARAMETER,
BumpFeeResult_WALLET_ERROR,
BumpFeeResult_MISC_ERROR,
};

This comment has been minimized.

@jtimon

jtimon Mar 18, 2017

Member

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?

}
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)) {

This comment has been minimized.

@jtimon

jtimon Mar 18, 2017

Member

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


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

This comment has been minimized.

@jtimon

jtimon Mar 18, 2017

Member

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

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

This comment has been minimized.

@jtimon

jtimon Mar 18, 2017

Member

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

This comment has been minimized.

Copy link
Member

jtimon commented Mar 18, 2017

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Mar 18, 2017

@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 jonasschnelli force-pushed the jonasschnelli:2017/02/qt_bumpfee branch from f7132c3 to f1485f9 Apr 10, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Apr 10, 2017

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

@flack

This comment has been minimized.

Copy link
Contributor

flack commented Apr 25, 2017

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 jonasschnelli force-pushed the jonasschnelli:2017/02/qt_bumpfee branch to 488ca9f Apr 26, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Apr 26, 2017

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

bildschirmfoto 2017-04-26 um 09 59 11

bildschirmfoto 2017-04-26 um 10 20 40

@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Apr 26, 2017

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 1, 2017

@jonasschnelli that looks good!

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 1, 2017

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
Copy link
Contributor

ryanofsky left a comment

ACK 488ca9f


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

This comment has been minimized.

@ryanofsky

ryanofsky May 1, 2017

Contributor

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 4, 2017

Author Member

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

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Contributor

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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

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]) : "") +")");

This comment has been minimized.

@ryanofsky

ryanofsky May 1, 2017

Contributor

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

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented May 4, 2017

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

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented May 4, 2017

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

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

ACK 1015ef40ea37a33faeded844e2da62cc6c66b0bf


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

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Contributor

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.

src/qt/transactionview.cpp Outdated
@@ -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);

This comment has been minimized.

@ryanofsky

ryanofsky May 4, 2017

Contributor

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 4, 2017

Author Member

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

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

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


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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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.

src/qt/walletmodel.cpp Outdated
std::unique_ptr<CFeeBumper> feeBump;
{
LOCK2(cs_main, wallet->cs_wallet);
feeBump.reset(new CFeeBumper(wallet, hash, 0, false, 0, true));

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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?

This comment has been minimized.

@jonasschnelli

jonasschnelli May 11, 2017

Author Member

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

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt May 9, 2017

Contributor

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

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented May 11, 2017

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 added some commits Feb 3, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented May 11, 2017

Rebased.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2017/02/qt_bumpfee branch to a387837 May 11, 2017

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

This comment has been minimized.

@paveljanik

paveljanik May 17, 2017

Contributor

typo "hasen't"

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented May 17, 2017

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

This comment has been minimized.

Copy link
Contributor

paveljanik commented May 17, 2017

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

This comment has been minimized.

Copy link
Member

laanwj commented May 18, 2017

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented May 18, 2017

utACK a387837. I agree with merging.

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request May 18, 2017

Merge #9697: [Qt] simple fee bumper with user verification
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 jonasschnelli removed this from Blockers in High-priority for review May 18, 2017

@jnewbery jnewbery referenced this pull request Jul 31, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.