wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

kallewoof · 2020-03-06T05:12:36Z

This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413.

DrahtBot · 2020-03-06T06:43:32Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add Single Random Draw as an additional coin selection algorithm #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
Use effective values throughout coin selection #17331 (Use effective values throughout coin selection by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors · 2020-03-06T17:27:49Z

Can you add a test with createfundedpsbt's feeRate argument?

maflcko · 2020-03-06T19:04:44Z

src/wallet/wallet.cpp

+            // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
+            // provided one
+            if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) {
+                strFailReason = strprintf("Fee rate is too low for the current mempool. Increase it to %s or modify -maxmempool", nFeeRateNeeded.ToString());


The mempool fee is not considered when the user picks the fee rate. See also https://doxygen.bitcoincore.org/wallet_2fees_8cpp_source.html#l00028

How did you trigger this bug? It would be nice to include a test vector or steps to reproduce, otherwise this will just be dead code.

@MarcoFalke the error is triggered by setting a fee rate below 1 sat / byte in the updated sendtoaddress RPCs in #11413. See #11413 (comment) for discussion and some alternative ideas.

If it is not possible to hit this case with Bitcoin Core master, it probably makes sense to only add it when it can be hit. And also add a test to hit it.

I triggered it with no problem in bitcoin-qt (set fee to 0.00000001 btc/kb; note that the GUI has changed it back to the minimum in the screenshot after I clicked "Send")...

Without this patch, it goes to confirm screen, silently changing my fee rate:

The mempool fee is not considered when the user picks the fee rate. See also https://doxygen.bitcoincore.org/wallet_2fees_8cpp_source.html#l00028

How did you trigger this bug? It would be nice to include a test vector or steps to reproduce, otherwise this will just be dead code.

That code calls mempoolMinFee() though.

That code calls mempoolMinFee() though.

No? It doesn't.

The code is

// Obey mempool min fee when using smart fee estimation CFeeRate min_mempool_feerate = wallet.chain().mempoolMinFee();

Setting a fee manually in the gui is not "smart fee". What am I missing?

You're not missing anything, because you're absolutely right. Should've read the code more carefully...

I've fixed the message.

kallewoof · 2020-03-07T01:30:08Z

Can you add a test with createfundedpsbt's feeRate argument?

I tried and that takes me all the way to sendrawtransaction before it hits min relay fee not met, 1 < 141. The PSBT stuff doesn't use the same method, unfortunately.

Sjors · 2020-03-07T09:12:10Z

I was hoping this was only an RPC issue. The error message is not very GUI friendly, but this should be bellt-and-suspenders check. The GUI is supposed to prevent you from entering a too low amount.

Your screenshot is a bit confusing, because the UI automatically corrects the input form when the value is below the minimum relay fee. But that correction only happens when you click somewhere else, like another field or the send button.

You can also see the issue when you launch with -minrelayfee=0.001 and then try 0.00001 as the fee. It will switch the amount back to 0.001, but when you click on Send it will still try the low fee rate.

tACK 7ac14a5

On a related note: I don't like that the GUI changes the fee amount from under me. It should just color the field red and show an error message below the field. But that's a separate PR imo, this check is useful regardless.

fjahr · 2020-03-13T17:12:15Z

tACK 7ac14a5

meshcollider · 2020-04-17T10:54:17Z

Concept ACK

kallewoof · 2020-04-24T05:01:45Z

As @MarcoFalke pointed out, this actually does not rely on the current mempool limitations. I've updated the error message accordingly. It now reads

"Fee rate is lower than the minimum fee rate setting. Increase it to %s or modify -minrelaytxfee", nFeeRateNeeded.ToString()

src/wallet/wallet.cpp

fjahr · 2020-04-30T15:49:52Z

re-ACK b8711d0

Only changes were rebasing and improvement of the error message.

maflcko · 2020-05-01T13:31:10Z

I can not reproduce with the steps given here: #18275 (comment)

maflcko · 2020-05-01T13:43:23Z

Couldn't reproduce either with feeRate in fundrawtransaction. Still Concept ACK as a belt and suspenders, but this doesn't fix a bug I can reproduce.

kallewoof · 2020-05-02T05:52:16Z

I can not reproduce with the steps given here: #18275 (comment)

Are you trying to reproduce on this branch or on master? On master, this is what it ends up like (note that it silently resets my desired fee rate when I defocus and/or click send): https://youtu.be/I0JZpmJ-Gls

maflcko · 2020-05-02T20:51:29Z

I mean I couldn't get the newly introduced error to execute. Not in the GUI, and not in the RPC.

Just tried again with recompile. Maybe this is a mac-specific issue? Or I have something in my gui settings that prevents it.

Sjors · 2020-05-04T10:14:22Z

tACK b8711d0. Just a rebase and text improvement since my last review.

kallewoof · 2020-05-04T12:49:19Z

@MarcoFalke Sorry about that [previous now-deleted comment showing 'settxfee' error], I got the messages mixed up. You're right that it doesn't seem possible to trigger this error from RPC/CLI. To trigger in QT, you need to change the fee rate and then click send without defocusing the window. This is probably platform dependent, so it may not be reproducable on non-macOS.

Sjors · 2020-05-04T13:50:18Z

Just double checked: I can still produce this error in the GUI.

maflcko · 2020-05-04T15:12:14Z

code-review-only ACK b8711d0, seems this can only be hit on macos

meshcollider

utACK b8711d0

meshcollider · 2020-05-05T03:30:15Z

There is a silent merge conflict, strFailReason no longer exists on master (it has been replaced by a bilingual_str called error)

kallewoof · 2020-05-05T05:07:13Z

@meshcollider ~~Weird. Git happily automerged everything when I rebased on latest master. Pushed that rebase though.~~

Edit: Yeah, nevermind. Fixed.

…te differed This avoids cases where a user requests a fee rate below the minimum and is silently overruled by the wallet.

Sjors · 2020-05-05T17:53:11Z

utACK 44cc75f (rebased)

fjahr · 2020-05-05T20:54:27Z

re-ACK 44cc75f

…te differed This avoids cases where a user requests a fee rate below the minimum and is silently overruled by the wallet. Github-Pull: bitcoin#18275 Rebased-From: 44cc75f

Sjors · 2020-06-16T10:07:13Z

@meshcollider?

Due to bitcoin/bitcoin#18275, make sure that the minimum fee bump is at least matching the required fee level for relay. Otherwise the wallet Qt tests fail because they create a zero-fee transaction and bump its fee, and then run into this new check.

…but the needed fee rate differed 44cc75f wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm) Pull request description: This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for bitcoin#11413. ACKs for top commit: Sjors: utACK 44cc75f (rebased) fjahr: re-ACK 44cc75f Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483

…te differed Summary: This avoids cases where a user requests a fee rate below the minimum and is silently overruled by the wallet. This is a backport of [[bitcoin/bitcoin#18275 | core#18275]] Test Plan: Run bitcoin-qt and manually set a too low fee when trying to send a transaction. The following patch may be needed to let the interface set a fee lower than the minimum: ``` diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp index dc9c4bf699..99da1f837d 100644 --- a/src/qt/sendcoinsdialog.cpp +++ b/src/qt/sendcoinsdialog.cpp @@ -188,7 +188,7 @@ void SendCoinsDialog::setModel(WalletModel *_model) { connect(ui->customFee, &BitcoinAmountField::valueChanged, this, &SendCoinsDialog::coinControlUpdateLabels); Amount requiredFee = model->wallet().getRequiredFee(1000); - ui->customFee->SetMinValue(requiredFee); + ui->customFee->SetMinValue(Amount::satoshi()); if (ui->customFee->value() < requiredFee) { ui->customFee->setValue(requiredFee); } ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9936

kallewoof mentioned this pull request Mar 6, 2020

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413

Merged

fanquake added the Wallet label Mar 6, 2020

This was referenced Mar 6, 2020

Add Single Random Draw as an additional coin selection algorithm #17526

Merged

Use effective values throughout coin selection #17331

Merged

The ultimate send RPC #16378

Merged

maflcko reviewed Mar 6, 2020

View reviewed changes

kallewoof force-pushed the 2003-wallet-error-on-feechange branch 2 times, most recently from f6f525c to 7ac14a5 Compare March 7, 2020 01:37

kallewoof force-pushed the 2003-wallet-error-on-feechange branch from 7ac14a5 to f9b85c1 Compare April 24, 2020 05:00

maflcko reviewed Apr 24, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 25, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 2003-wallet-error-on-feechange branch from f9b85c1 to b8711d0 Compare April 25, 2020 14:19

meshcollider reviewed May 5, 2020

View reviewed changes

kallewoof force-pushed the 2003-wallet-error-on-feechange branch from b8711d0 to 1b71a59 Compare May 5, 2020 05:06

wallet: error if an explicit fee rate was given but the needed fee ra…

44cc75f

…te differed This avoids cases where a user requests a fee rate below the minimum and is silently overruled by the wallet.

kallewoof force-pushed the 2003-wallet-error-on-feechange branch from 1b71a59 to 44cc75f Compare May 5, 2020 05:09

maflcko merged commit 23b2a68 into bitcoin:master Jun 16, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

kallewoof commented Mar 6, 2020 •

edited

DrahtBot commented Mar 6, 2020 •

edited

Sjors commented Mar 6, 2020

maflcko Mar 6, 2020

Sjors Mar 6, 2020 •

edited

maflcko Mar 6, 2020

kallewoof Mar 7, 2020 •

edited

kallewoof Mar 7, 2020

maflcko Apr 17, 2020

kallewoof Apr 24, 2020

kallewoof commented Mar 7, 2020

Sjors commented Mar 7, 2020

fjahr commented Mar 13, 2020

meshcollider commented Apr 17, 2020

kallewoof commented Apr 24, 2020

fjahr commented Apr 30, 2020

maflcko commented May 1, 2020

maflcko commented May 1, 2020

kallewoof commented May 2, 2020 •

edited

maflcko commented May 2, 2020

Sjors commented May 4, 2020 •

edited

kallewoof commented May 4, 2020 •

edited

Sjors commented May 4, 2020

maflcko commented May 4, 2020

meshcollider left a comment

meshcollider commented May 5, 2020

kallewoof commented May 5, 2020 •

edited

Sjors commented May 5, 2020

fjahr commented May 5, 2020

Sjors commented Jun 16, 2020

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

wallet: error if an explicit fee rate was given but the needed fee rate differed #18275

Conversation

kallewoof commented Mar 6, 2020 • edited

DrahtBot commented Mar 6, 2020 • edited

Conflicts

Sjors commented Mar 6, 2020

maflcko Mar 6, 2020

Choose a reason for hiding this comment

Sjors Mar 6, 2020 • edited

Choose a reason for hiding this comment

maflcko Mar 6, 2020

Choose a reason for hiding this comment

kallewoof Mar 7, 2020 • edited

Choose a reason for hiding this comment

kallewoof Mar 7, 2020

Choose a reason for hiding this comment

maflcko Apr 17, 2020

Choose a reason for hiding this comment

kallewoof Apr 24, 2020

Choose a reason for hiding this comment

kallewoof commented Mar 7, 2020

Sjors commented Mar 7, 2020

fjahr commented Mar 13, 2020

meshcollider commented Apr 17, 2020

kallewoof commented Apr 24, 2020

fjahr commented Apr 30, 2020

maflcko commented May 1, 2020

maflcko commented May 1, 2020

kallewoof commented May 2, 2020 • edited

maflcko commented May 2, 2020

Sjors commented May 4, 2020 • edited

kallewoof commented May 4, 2020 • edited

Sjors commented May 4, 2020

maflcko commented May 4, 2020

meshcollider left a comment

Choose a reason for hiding this comment

meshcollider commented May 5, 2020

kallewoof commented May 5, 2020 • edited

Sjors commented May 5, 2020

fjahr commented May 5, 2020

Sjors commented Jun 16, 2020

kallewoof commented Mar 6, 2020 •

edited

DrahtBot commented Mar 6, 2020 •

edited

Sjors Mar 6, 2020 •

edited

kallewoof Mar 7, 2020 •

edited

kallewoof commented May 2, 2020 •

edited

Sjors commented May 4, 2020 •

edited

kallewoof commented May 4, 2020 •

edited

kallewoof commented May 5, 2020 •

edited