[Qt] overhaul smart-fee slider, adjust default confirmation target #8989

jonasschnelli · 2016-10-21T08:57:03Z

Inverts the smartfee-slider, left = fast, right = normal
Use CWallets default confirmation (currently 2 blocks)
Show the estimated time for confirmation
Decouple the global nTxConfirmTarget from the GUI (used a per-tx value!)
Always throw CoinControl into CreateTransaction

jonasschnelli · 2016-10-21T08:58:19Z

src/qt/sendcoinsdialog.cpp

-        prepareStatus = model->prepareTransaction(currentTransaction);
+
+    // always throw in the CoinControl instance, it is also used for internal tx-creation parameters (ex. confirmation target)
+    prepareStatus = model->prepareTransaction(currentTransaction, CoinControlDialog::coinControl);


This could have unexpected side effects and needs good testing.

jonasschnelli · 2016-10-21T08:59:26Z

After this PR, slider looks like:

luke-jr · 2016-10-21T08:59:26Z

Inverts the smartfee-slider, left = fast, right = normal

?!

jonasschnelli · 2016-10-21T09:00:34Z

Inverts the smartfee-slider, left = fast, right = normal

?!

Before this PR, left side was "normal" confirmation speed (of a target of 25 blocks!), right side = fast confirmation.
This PR flips it (left side = fast confirmation, right side = slow confirmation).

luke-jr · 2016-10-21T09:18:29Z

Yes, I'm saying it doesn't make sense to flip it, and you included no rationale.. not only does it make more sense for right=(faster|more fees) at least in LTR languages, changing existing behaviour in such a complete reversal is going to create even additional confusion among users.

luke-jr · 2016-10-21T09:20:18Z

(personally I think it should be replaced with a dropdown for "eventually" (lowest reasonable fee), "within a week" (targeting 1000 blocks), "tomorrow" (targeting 144 blocks), "today" (target 66 blocks?), and "ASAP" (target next block))

laanwj · 2016-10-21T09:27:35Z

src/qt/sendcoinsdialog.cpp

-        prepareStatus = model->prepareTransaction(currentTransaction);
+
+    // always throw in the CoinControl instance, it is also used for internal tx-creation parameters (ex. confirmation target)
+    prepareStatus = model->prepareTransaction(currentTransaction, CoinControlDialog::coinControl);


I don't think we should do it this way. CoinControlDialog::coinControl should not be used if getCoinControlFeatures is disabled. It may have invalid state.
What about creating a temporary CoinControl object and just changing the confirmation setting in the case of non-coin-control? This will ensure it's at least starting from defaults.

E.g. one clean way:

CCoinControl ctrl; if (model->getOptionsModel()->getCoinControlFeatures()) { ctrl = CoinControlDialog::coinControl; } if (ui->radioSmartFee->isChecked()) { ctrl.nConfirmTarget = ui->sliderSmartFee->value(); } prepareStatus = model->prepareTransaction(currentTransaction, ctrl);

(not sure if the ui->radioSmartFee->isChecked() needs to be there)

laanwj · 2016-10-21T09:30:32Z

Reversing left and right on the slider out of the blue is probably not a good idea - people will be used to the way it currently is and it will suddenly do something else, resulting in possible invalid sends. Let's replace it with a different widget altogether if it needs to be changed (e.g. for example what @luke-jr says).

jonasschnelli · 2016-10-21T12:28:41Z

Updated:

Reverted the left/right reversing
Followed @laanwj's advice of using a custom CCoinControl instance

morcos · 2016-10-21T12:56:33Z

As discussed on IRC, I'm opposed to adjusting the default confirmation target to 2. I think with the way current fee estimation works, the default case should not be "I really want to be sure I'm included in one of the next 2 blocks" because it will lead to you paying too high a fee for what most people are trying to accomplish. I don't see much value in changing a default that any can easily change by moving a slider, however if we want to make command line and GUI line up, then I suggest we choose 6 for both. Alternatively, we wait until we overhaul fee estimation to make changes to the GUI at which time we do something closer to what Luke is suggesting.

jonasschnelli · 2016-10-21T13:06:42Z

IMO there is no reason to have different confirmation target (especially not 2 vs 25) between the GUI and the RPC interface. The only reason I could imaging why we should have different default values, would be, if we assume we have different user-types between the RPC-wallet interface and the GUI.

I think there should be a discussion about the default confirmation target,... but probably in a different PR.

This change should result in having the same default-value between RPC and the GUI, using the RPC value as the dominating default-value. This is probably what users would expect if they use both interfaces or if they switch from GUI to RPC or vice versa.

At the moment, if one does not expand the fee tab and just send a transaction, he could be surprised by the pretty-uncommon confirmation-target of 25 blocks.

laanwj · 2016-10-21T13:07:55Z

then I suggest we choose 6 for both

Yes there seemed to be some agreement around 6 in yesterday's meeting. I think it would be good to change both to that.

Though I think changing that default is separate from what is done here - this just unifies the UI default to the overall default.

Either way it needs to be clearly documented in the release notes that these defaults are changed.

maflcko · 2016-10-21T17:32:33Z

Completely agree with what @laanwj said in the last comment. I think setting the default for both to 6 is a good compromise and a short mention in the release notes as well as the reworked GUI will hopefully let users make more educated/suitable choices.

da2ce7 · 2016-10-22T01:55:59Z

(Possibly Scope Creep)
The transaction fee area should show the following additional information: Transaction Weight; Transaction Fee Rate; and Total Fee to be Paid.

Also, is the unit: BTC/kB the correct unit for the fee when counting fees by weight?

Users who are creating heavy transactions may wish to prioritise them lower to pay less fee.

laanwj · 2016-10-24T11:02:57Z

Yes, that is definitely scope creep. I think we should aim to merge this, then change the default to 6. Other changes can be done later.

jonasschnelli · 2016-10-28T08:45:16Z

Rebased.

…ion target cfe77ef [Qt] overhaul smart-fee slider, adjust default confirmation target (Jonas Schnelli) 6f02899 [Qt] Hide nTxConfirmTarget behind WalletModel (Jonas Schnelli) 004168d CoinControl: add option for custom confirmation target (Jonas Schnelli)

Recent discussion (in IRC meetings, and e.g. bitcoin#8989) has shown a preference for the default confirm target for smartfees to be 6 instead of 2, to avoid overpaying fees for questionable gain. 6 is also a compromise between the GUI's pre-bitcoin#8989 value of 25 and the bitcoind `-txconfirmtarget` default of 2. These were unified in bitcoin#8989, but this has made the (overly expensive) default of 2 as GUI default.

jtimon · 2016-12-25T11:01:24Z

Mhmm, I'm confused, it seems it is discussed here that the slider shouldn't be inverted, but it ended being inverted anyway, no?
I completely missed the rationale for the inversion.

…nfirmation target cfe77ef [Qt] overhaul smart-fee slider, adjust default confirmation target (Jonas Schnelli) 6f02899 [Qt] Hide nTxConfirmTarget behind WalletModel (Jonas Schnelli) 004168d CoinControl: add option for custom confirmation target (Jonas Schnelli)

jonasschnelli added the GUI label Oct 21, 2016

jonasschnelli added this to the 0.14.0 milestone Oct 21, 2016

jonasschnelli force-pushed the 2016/10/qt_slider branch from c328672 to 63b9eb0 Compare October 21, 2016 08:57

jonasschnelli commented Oct 21, 2016

View reviewed changes

laanwj reviewed Oct 21, 2016

View reviewed changes

jonasschnelli force-pushed the 2016/10/qt_slider branch 3 times, most recently from 147f98c to 008a3ec Compare October 21, 2016 12:27

jonasschnelli force-pushed the 2016/10/qt_slider branch from 008a3ec to 312e6cb Compare October 21, 2016 12:46

laanwj mentioned this pull request Oct 24, 2016

Bitcoin-qt's default send speed is "normal" #8615

Closed

jonasschnelli added 3 commits October 28, 2016 10:44

CoinControl: add option for custom confirmation target

004168d

[Qt] Hide nTxConfirmTarget behind WalletModel

6f02899

[Qt] overhaul smart-fee slider, adjust default confirmation target

cfe77ef

jonasschnelli force-pushed the 2016/10/qt_slider branch from 312e6cb to cfe77ef Compare October 28, 2016 08:45

laanwj merged commit cfe77ef into bitcoin:master Oct 28, 2016

laanwj mentioned this pull request Oct 28, 2016

wallet: Change default confirm target from 2 to 6 #9036

Merged

jonasschnelli mentioned this pull request Oct 28, 2016

TODO for release notes 0.14.0 #8455

Closed

18 tasks

This was referenced Oct 30, 2016

0.12: Transaction fee slider - User feedback: "normal" fee means confirmation time 4h ? Slider does not default to value of txconfirmtarget ? #7754

Closed

GUI fee estimator may be inaccurate #8979

Closed

jonasschnelli mentioned this pull request Dec 2, 2016

Disable fee estimates for 1 block target #9239

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[Qt] overhaul smart-fee slider, adjust default confirmation target #8989

[Qt] overhaul smart-fee slider, adjust default confirmation target #8989

Uh oh!

Conversation

jonasschnelli commented Oct 21, 2016

Uh oh!

jonasschnelli Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 21, 2016

Uh oh!

luke-jr commented Oct 21, 2016

Uh oh!

jonasschnelli commented Oct 21, 2016

Uh oh!

luke-jr commented Oct 21, 2016

Uh oh!

luke-jr commented Oct 21, 2016

Uh oh!

laanwj Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 21, 2016

Uh oh!

jonasschnelli commented Oct 21, 2016

Uh oh!

morcos commented Oct 21, 2016

Uh oh!

jonasschnelli commented Oct 21, 2016

Uh oh!

laanwj commented Oct 21, 2016

Uh oh!

maflcko commented Oct 21, 2016

Uh oh!

da2ce7 commented Oct 22, 2016

Uh oh!

laanwj commented Oct 24, 2016

Uh oh!

jonasschnelli commented Oct 28, 2016

Uh oh!

jtimon commented Dec 25, 2016

Uh oh!

Uh oh!

laanwj Oct 21, 2016 •

edited

Loading

laanwj Oct 21, 2016 •

edited

Loading