[Qt] Add Smartfee to GUI

cozz commented at 5:22 am on November 3, 2014: contributor

By default a minimized label is shown. It updates automatically, if smartfee has been chosen. smartfeegui1

Smartfee not initialized -> pay minfee smartfeegui2

Smartfee initialized smartfeegui4

Fee removed from optionsdialog smartfeegui9

laanwj commented at 7:22 am on November 3, 2014: member

Whoa, great work

in src/wallet.cpp: in 359173171c outdated

25@@ -26,9 +26,11 @@ using namespace std;
26 CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE);
27 unsigned int nTxConfirmTarget = 1;
28 bool bSpendZeroConfChange = true;
29+bool fSendFreeTransactions = true;
30+bool fPayAtLeastCustomFee = true;
31 
32 /** Fees smaller than this (in satoshi) are considered zero fee (for transaction creation) */
33-CFeeRate CWallet::minTxFee = CFeeRate(10000);  // Override with -mintxfee
34+CFeeRate CWallet::minTxFee = CFeeRate(1000);  // Override with -mintxfee

laanwj commented at 7:22 am on November 3, 2014:

Changing this core default is probably accidental?

sipa commented at 7:35 am on November 3, 2014:

This is the default for the wallet only, not the relay fee minimum. (just pointing out; not saying it’s intentional or not).

cozz commented at 5:07 pm on November 3, 2014:

Sure, its intentional, we have reduced the minrelayfee from 10000 to 1000 satoshis in 0.9. But this did not make transactions cheaper for the people, because the wallet still paid 10000 (which is good, to ensure everybodys txs get relayed). After the 0.9 release there were even blogs about “How to make your bitcoin transactions cheaper”, they told people to start bitcoin with -mintxfee=0.00001000 (1000 satoshis). Back then this was a little dangerous, as your tx may not be relayed, but now its safe. This is just the second step of staging.

So, yes, we should definitely reduce the mintxfee to 1000 satoshis for the 0.10 release, See #3305.

Bitcoin transactions are super cheap these days as we also removed the rounding, a 192 bytes transaction only costs 192 satoshis. Its actually 1 satoshi per byte.

laanwj commented at 9:02 am on November 4, 2014:

OK - agreed, although it could be argued this needs to be in a separate change from the GUI changes, as it affects the RPC wallet as well

in src/qt/coincontroldialog.cpp: in 359173171c outdated

140+        sortView(settings.value("nCoinControlSortColumn").toInt(), ((Qt::SortOrder)settings.value("nCoinControlSortOrder").toInt()));
141 }
142 
143 CoinControlDialog::~CoinControlDialog()
144 {
145+    QSettings settings;

Diapolo commented at 9:06 am on November 3, 2014:

AFAIK we have a GUIUtil function for saving and restoring window positions. It may make sense to also add one for reading/writing generic values?

cozz commented at 0:07 am on November 5, 2014:

I am not completely objected to that idea, but on the other hand the values get written to disk in the QSettings destructor, so if we had a guiutil-function we would flush to disk on every call for no reason really.

And as for the writings we would replace 1 line

0settings.setValue(key, value);

with 1 line

0GUIUtil::setQSetting(key, value);

which is not that much of an improvement.

But if you have an idea, feel free to submit an improvement pull request afterwards.

in src/qt/coincontroldialog.cpp: in 359173171c outdated

130@@ -130,10 +131,22 @@ CoinControlDialog::CoinControlDialog(QWidget *parent) :
131 
132     // default view is sorted by amount desc
133     sortView(COLUMN_AMOUNT_INT64, Qt::DescendingOrder);
134+
135+    // restore list mode and sortorder as a convenience feature
136+    QSettings settings;
137+    if (settings.contains("nCoinControlMode") && settings.value("nCoinControlMode").toInt() == 1)

Diapolo commented at 9:07 am on November 3, 2014:

Could / should be a boolean as we only have 1 or 0 currently?

in src/qt/coincontroldialog.cpp: in 359173171c outdated

302@@ -290,19 +303,19 @@ void CoinControlDialog::clipboardAmount()
303 // copy label "Fee" to clipboard
304 void CoinControlDialog::clipboardFee()
305 {
306-    GUIUtil::setClipboard(ui->labelCoinControlFee->text().left(ui->labelCoinControlFee->text().indexOf(" ")));
307+    GUIUtil::setClipboard(ui->labelCoinControlFee->text().left(ui->labelCoinControlFee->text().indexOf(" ")).replace("~",""));

Diapolo commented at 9:08 am on November 3, 2014:

Coding nit: Can you add a space in here and for the other places where you use that :)?

in src/qt/coincontroldialog.cpp: in 359173171c outdated

436-        return ">=" + tr("medium");
437-    return tr("lowest");
438+    double dPriorityMedium = mempool.estimatePriority(nTxConfirmTarget);
439+
440+    if (dPriorityMedium <= 0)
441+        dPriorityMedium = COIN * 144 / 250; // not enough data, back to hard-coded

Diapolo commented at 9:09 am on November 3, 2014:

Do we have a constant for this somewhere or a function which returns that value?

in src/qt/optionsdialog.cpp: in 359173171c outdated

261-    {
262-        /* Update transactionFee with the current unit */
263-        ui->transactionFee->setDisplayUnit(model->getDisplayUnit());
264-    }
265-}
266+void OptionsDialog::updateDisplayUnit() {}

Diapolo commented at 9:12 am on November 3, 2014:

IMHO you could also remove that completely (the whole signal/slot stuff for this).

in src/qt/sendcoinsdialog.cpp: in 359173171c outdated

11 #include "coincontroldialog.h"
12 #include "guiutil.h"
13 #include "optionsmodel.h"
14 #include "sendcoinsentry.h"
15 #include "walletmodel.h"
16+#include "wallet.h"

Diapolo commented at 9:14 am on November 3, 2014:

Nit: wallet before walletmodel

in src/qt/sendcoinsdialog.cpp: in 359173171c outdated

595+    }
596+}
597+
598+void SendCoinsDialog::updateMinFeeLabel()
599+{
600+    ui->labelMinFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), CWallet::minTxFee.GetFeePerK()) + "/kB");

Diapolo commented at 9:16 am on November 3, 2014:

This should also have a NULL pointer check.

in src/qt/sendcoinsdialog.cpp: in 359173171c outdated

588+    if (ui->radioSmartFee->isChecked())
589+        ui->labelFeeMinimized->setText(ui->labelSmartFee->text());
590+    else if (ui->radioMinimumFee->isChecked())
591+        ui->labelFeeMinimized->setText(ui->labelMinFee->text());
592+    else {
593+        ui->labelFeeMinimized->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), ui->customFee->value()) +

Diapolo commented at 9:16 am on November 3, 2014:

Same here… NULL pointer check?

cozz commented at 0:07 am on November 5, 2014:

fixed

morcos commented at 8:44 pm on November 3, 2014: member

Agreed. This is a really nice change! I like how the slider starts at normal and you have to consciously select a lower confirm target. If we end up using #5159 or something similar, you might want to change the uninitialized error message. It’s possible to consistently not get an answer for a very low confirmation target like 1, because there is no fee that is reliably high enough and it seems kind of easy to not notice that’s whats happening and you’re accidentally putting a very low fee. Could the slider even snap back to the lowest confirm target which works?

in src/qt/coincontroldialog.cpp: in 359173171c outdated

433-    // Note: if mempool hasn't accumulated enough history (estimatePriority
434-    // returns -1) we're conservative and classify as "lowest"
435-    if (mempool.estimatePriority(nTxConfirmTarget) <= 0 && AllowFree(dPriority))
436-        return ">=" + tr("medium");
437-    return tr("lowest");
438+    double dPriorityMedium = mempool.estimatePriority(nTxConfirmTarget);

morcos commented at 8:49 pm on November 3, 2014:

Seems like it might be nice to move this call outside of the loops in updateLabels and updateView since its just the same call over and over again for each input.

in src/qt/sendcoinsdialog.cpp: in 359173171c outdated

86+    if (!settings.contains("nCustomRadio") && settings.contains("nTransactionFee") && settings.value("nTransactionFee").toLongLong() > 0) // compatibility
87+        settings.setValue("nCustomRadio", 1); // at least
88+    if (!settings.contains("nCustomRadio"))
89+        settings.setValue("nCustomRadio", 0); // per kilobyte
90+    if (!settings.contains("nTxConfirmTarget"))
91+        settings.setValue("nTxConfirmTarget", 0);

morcos commented at 8:51 pm on November 3, 2014:

This actually means the opposite of nTxConfirmTarget right? This is how far over the slider is set?

in src/qt/coincontroldialog.cpp: in 359173171c outdated

554-            if (dPriorityNeeded <= 0 && !AllowFree(dPriority)) // not enough mempool history: never send free
555-                dPriorityNeeded = std::numeric_limits<double>::max();
556+            if (fSendFreeTransactions)
557+            {
558+                double dPriorityNeeded = mempool.estimatePriority(nTxConfirmTarget);
559+                if (dPriorityNeeded <= 0 && !AllowFree(dPriority)) // not enough mempool history: never send free

morcos commented at 8:56 pm on November 3, 2014:

I’m not sure AllowFree is a good default here, that’s the minimum possible priority to be included in a block, but isn’t any indication that it’s ever going to happen.

cozz commented at 0:08 am on November 5, 2014:

Yes, I also thought that maybe we shouldnt allow free transactions by the hardcoded priority at all. On the other hand, smartfee needs some people to send free transactions in order to have a reference.

But this is the wrong pull request for this discussion, here coincontrol just needs to follow CWallet:CreateTransaction. If you want that to be changed, please create a separate issue for it, as this would require CWallet:CreateTransaction to be changed.

cozz force-pushed on Nov 5, 2014

cozz commented at 0:14 am on November 5, 2014: contributor

update: addressed inline comments @morcos Yes, if our estimation code has an answer for 25, but not for 1, it would be confusing to tell the user in order to get ‘fast’ confirmation, you need to pay a lower fee than for ’normal’. But that can be changed once we switch to the new estimation code. The current GUI is supposed to work with the current master.

laanwj added the label GUI on Nov 7, 2014

laanwj added this to the milestone 0.10.0 on Nov 7, 2014

laanwj added the label Priority Medium on Nov 7, 2014

laanwj commented at 3:38 pm on November 13, 2014: member

Looks good to me!

A question: “Send as free transaction if possible” - doesn’t this toggle apply to all three settings equally, Recommended Minimum and Custom? That feels more logical to me.

Some minor nits:

SmartFee → smart fee
at least → total at least (after Custom:)
Confirmation dialog should show the size of the transaction in kB, so that people can verify that the fee they selected is actually used

luke-jr commented at 3:50 pm on November 13, 2014: member

Grammatical corrections:

“1 block” should not be plural
“Send as free transaction if possible” should be “Send as gratis transaction if possible”; “free” means something else.

What is the difference between “Send as free transaction if possible” and “Minimum”? They sound like the same thing to me…

cozz commented at 4:36 pm on November 13, 2014: contributor

@laanwj Yes, I had that checkbox apply to all three, but then I realized that this is not possible with the current logic of CreateTransaction. Its not a problem, but it requires CreateTransaction to be redesigned again. If you want that, I can do this, I just did not, to avoid critical changes. In the current master, if you pay a voluntary fee, you never send free. I also vote for changing it, because its kind of obvious. @luke-jr I never heard gratis transaction before in bitcoinland. According to the dictionary, free can be used as synonym. I dont see a problem with it. “Minimum” means to pay the minimum fee, “free” means to pay nothing. But if its not clear to you, it may also not be clear to other people. I can add a tooltip to the free checkbox mentioning “gratis”.

luke-jr commented at 4:41 pm on November 13, 2014: member

@cozz “free transaction relay” has been used for nodes that don’t perform IsStandard filtering of transactions. It’s better to use unambiguous terms, even if you accept the newer redefinition of “free” to include “gratis”.

If gratis is possible, it would always be the minimum. So what is the difference?

gavinandresen commented at 4:44 pm on November 13, 2014: contributor

“gratis” is an obscure word. How about “zero-fee”.

cozz commented at 5:01 pm on November 13, 2014: contributor

So lets replace “free” with “zero-fee” then.

About the other thing, I can see that “minimum” may seem to imply “zero-fee”, but its not. If we move the free-checkbox below, to apply to all three, this should become clearer.

luke-jr commented at 5:21 pm on November 13, 2014: member

So what would “Minimum” do if you uncheck it? O.o

cozz commented at 5:23 pm on November 13, 2014: contributor

minimum is a radio button. you must select 1 of 3.

luke-jr commented at 5:31 pm on November 13, 2014: member

I mean selected Minimum and unchecked “zero-fee” - it doesn’t make sense… if zero-fee is possible, it’s always the minimum.

cozz commented at 5:59 pm on November 13, 2014: contributor

Ah. Yes, I understand what you mean. I just dont really know better. Renaming “Minimum” to “Standard” for example, would make it even more confusing. There just is a minimum relay fee, which is 1000 satoshis per kB. And I want to give the user the option to select that. The selected fee is independent of “free” or “not”. Thats what the checkbox is there for. Currently if you select “Minimum” you always pay a fee, never send free.

cozz commented at 6:45 pm on November 13, 2014: contributor

Normally the minimum fee should be the start of the slider, and there should also be an estimation of how long it takes to confirm, if you just pay the minfee. Then we could remove the “Minimum” radio button. But our smartfee is not that smart by design, so we just have to make the best of it.

We could also remove the “Minimum” radio button, and combine it with “Custom” somehow. Simplest would be to just explain in the tooltip about the minfee.

luke-jr commented at 7:15 pm on November 13, 2014: member

The slider is added by this PR. Why not just guarantee the “avoid rate limiting” value exists on it, and label it as such?

cozz commented at 5:37 pm on November 14, 2014: contributor

Yes, adding the minfee as part of the slider could be an idea. On the other hand this would make the slider rather complicated and not fool-proof anymore. One main purpose of smartfee is to just select a fee automatically, so that the transaction confirms, without that the user has to think or understand anything.

So what about making the minfee a checkbox as part of custom. So we would have one simple foolproof “recommended” section, and one complicated “custom” section, where people need to bother reading the tooltips in order to understand the gui. (I did not address the other nits, this is only about where to put the minfee) smartfeegui8

luke-jr commented at 5:41 pm on November 14, 2014: member

Hm, seeing it that way makes me think it should be a click-button to the right of the Custom options “Set for quick relay” or something, that changes the values?

cozz commented at 5:48 pm on November 14, 2014: contributor

If you click the checkbox, the upper controls would be disabled and preselected. But I have also thought about that button. I would name it “Set to minimum fee”.

luke-jr commented at 5:59 pm on November 14, 2014: member

Ah, okay, it makes sense to disable the upper part. I think relay should be mentioned, since that’s what it means.

cozz commented at 6:03 pm on November 14, 2014: contributor

Ok, so I’ll keep the checkbox. What you mean with relay? Its not the minimum relayfee. Its the minimum wallet fee.

luke-jr commented at 6:13 pm on November 14, 2014: member

That doesn’t make sense. What is a “minimum wallet fee”?

cozz commented at 6:14 pm on November 14, 2014: contributor

See #5209

luke-jr commented at 6:40 pm on November 14, 2014: member

In other words, the arbitrary value that floating fees is supposed to be replacing?

cozz commented at 7:10 pm on November 14, 2014: contributor

Yes, I could write another story now, about how bad I think smartfee is, but I already mentioned that in another pull request. This pull request is just about to bring to the GUI what we have. At least smartfee suggests “some” fee that we have seen confirms for sure. Which is an improvement. So people who have no idea what to pay in fee, are safe when they choose to pay the recommended fee, because the recommended fee is just what other people are paying (and confirmed).

morcos commented at 8:13 pm on November 14, 2014: member

Personally I think we’re not addressing the biggest issue. Given that you are selecting the fee rate you want to pay before your inputs are selected (unless you’re using coin control) you have very little control on the total fee you’re paying. I think before we could have a really effective GUI for paying fees we’d have to address #4082. There is a complicated interaction between the inputs you use, the speed you desire for confirmation, and the fee you’re paying. It makes a lot of sense to use your small inputs when you don’t care about speed (b/c you have to use a lot of inputs for the same value tx, but get to pay less per input used) and to use your big inputs when you do care about the speed of your tx (since each input used has a high fee per / kb cost). This is without even taking into account coin age to use your old coins to take advantage of priority when it saves you the most in fees. But I don’t know if optimizing all of this falls under the purview of the core wallet.

cozz commented at 8:55 pm on November 14, 2014: contributor

@morcos yes, I agree to what you say, there is a lot of room for improvement. But lets not make more of this pull request than what its supposed to do.

The point is that the current fee/priority selection, selects something thats confirms. And floats. Thats good. Even if way too high in some edge cases, its better than the hardcoded fee/priority values we had before. And people who know better than our smartfee, they can use coin control/custom fee.

Of course its bad, that sophisticated users may end up paying less fee than others in bitcoinland, but I guess there is not much we can do here in regards to the 0.10 release.

cozz force-pushed on Nov 15, 2014

cozz commented at 8:39 pm on November 15, 2014: contributor

update:

make minfee a checkbox as part of custom
move zero-fee checkbox below to apply independent of “recommended” or “custom”
nits

Added as separate commit for easier review. Should be squashed before merge. There were less changes to CreateTransaction required than I thought, but I introduced a “-sendfreetransactions” (default=false) command line parameter to have bitcoind behavior compatible.

smartfeegui10 smartfeegui11 smartfeegui12 smartfeegui13

luke-jr commented at 9:21 pm on November 15, 2014: member

When this says “to be confirmed within 25 blocks”, what metric is it using for confirmation? The usual 6 blocks? If it’s just 25 blocks to be mined, perhaps “to begin confirmation within 25 blocks” would be better.

It might be desirable to see the Smart-Fee estimate for mining/confirmation time even for Custom fee selection (move it outside the options?).

What does the new “Minimize” button do, since we still have the checkbox under Custom?

Does this under any conditions allow sending with a lower fee than the “minimum”? We still don’t have a mechanism for getting “unstuck” :/

morcos commented at 10:04 pm on November 15, 2014: member

I think having the “zero-fee” option apply to both is confusing, because I think it still uses the txConfirmTarget you set in the top section. I like the idea of having a top section which uses smart fee-logic (and has the zero-fee checkbox) and a bottom custom section (which either has the min-fee checkbox or makes it easy to set the min fee as the custom fee).

cozz commented at 10:43 pm on November 15, 2014: contributor

@luke-jr I can change it to “included in a block within 25 blocks”.

Our estimation API is supposed to take a target (like 11 blocks) and returns the fee, not the other way round. Also the estimations are not really good at the moment. As morcos said, even if you just pay the minfee, you get almost always included after 8 blocks. But our estimation code tells you 25 blocks, for a even higher fee. So I’ll leave that for the future, once our estimations are better.

Minimize button minimizes, see initial screenshots, the button is not new.

Its not possible to send a non-sense (like 1 satoshi): 0 < fee < minRelayFee @morcos yes, but txConfirmTarget is set to 25 internally for custom. I think thats fine. So you can choose to send a free transaction for both, recommended and custom.

cozz force-pushed on Nov 15, 2014

cozz commented at 11:02 pm on November 15, 2014: contributor

ok, changed to “Estimated to begin confirmation within x block(s).”

laanwj commented at 4:53 pm on November 18, 2014: member

ACK after squash ~~commithash 6bfa31feae0c731ab52777bafdd3cbda41522d76~~ ~~https://dev.visucore.com/bitcoin/acks/5200~~ (see below)

cozz force-pushed on Nov 18, 2014

cozz commented at 11:12 pm on November 18, 2014: contributor

squashed

cozz force-pushed on Nov 19, 2014

[Wallet] Add global boolean whether to send free transactions (default=true) 0ed9675be4

[Wallet] Add global boolean whether to pay at least the custom fee (default=true) ed3e5e468c

[Wallet] Prevent user from paying a non-sense fee e7876b2979

[Qt] Add Smartfee to GUI c1c9d5b415

in src/wallet.cpp: in 197342e2a2 outdated

34 /** 
35  * Fees smaller than this (in satoshi) are considered zero fee (for transaction creation) 
36  * Override with -mintxfee
37  */
38-CFeeRate CWallet::minTxFee = CFeeRate(10000);
39+CFeeRate CWallet::minTxFee = CFeeRate(1000);

laanwj commented at 9:29 am on November 19, 2014:

I thought you moved this change to #5209 so that it has its own discussion. You should remove it here.

cozz force-pushed on Nov 19, 2014

cozz commented at 3:08 pm on November 19, 2014: contributor

@laanwj ok, I just rebased and skipped that commit

laanwj merged this on Nov 19, 2014

laanwj closed this on Nov 19, 2014

laanwj referenced this in commit b7fe9cd04c on Nov 19, 2014

rebroad commented at 0:10 am on December 11, 2014: contributor

This doesn’t seem to work well with #5159 currently…

cozz commented at 0:53 am on December 11, 2014: contributor

The GUI slider just shows what estimatefee returns. You can test the estimatefee rpc-call to check.

MarcoFalke locked this on Sep 8, 2021

[Qt] Add Smartfee to GUI #5200