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

jonasschnelli commented at 8:57 am on October 21, 2016: contributor

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 added the label GUI on Oct 21, 2016

jonasschnelli added this to the milestone 0.14.0 on Oct 21, 2016

jonasschnelli force-pushed on Oct 21, 2016

in src/qt/sendcoinsdialog.cpp: in 63b9eb0989 outdated

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

jonasschnelli commented at 8:58 am on October 21, 2016:

This could have unexpected side effects and needs good testing.

laanwj commented at 9:27 am on October 21, 2016:

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.

laanwj commented at 9:34 am on October 21, 2016:

E.g. one clean way:

0CCoinControl ctrl;
1if (model->getOptionsModel()->getCoinControlFeatures()) {
2    ctrl = CoinControlDialog::coinControl;
3}
4if (ui->radioSmartFee->isChecked()) {
5    ctrl.nConfirmTarget = ui->sliderSmartFee->value();
6}
7prepareStatus = model->prepareTransaction(currentTransaction, ctrl);

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

jonasschnelli commented at 8:59 am on October 21, 2016: contributor

After this PR, slider looks like:

luke-jr commented at 8:59 am on October 21, 2016: member

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

?!

jonasschnelli commented at 9:00 am on October 21, 2016: contributor

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 commented at 9:18 am on October 21, 2016: member

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 commented at 9:20 am on October 21, 2016: member

(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 commented at 9:30 am on October 21, 2016: member

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 force-pushed on Oct 21, 2016

jonasschnelli commented at 12:28 pm on October 21, 2016: contributor

Updated:

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

jonasschnelli force-pushed on Oct 21, 2016

morcos commented at 12:56 pm on October 21, 2016: member

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 commented at 1:06 pm on October 21, 2016: contributor

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 commented at 1:07 pm on October 21, 2016: member

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.

MarcoFalke commented at 5:32 pm on October 21, 2016: member

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 commented at 1:55 am on October 22, 2016: none

(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 commented at 11:02 am on October 24, 2016: member

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.

CoinControl: add option for custom confirmation target 004168dcb7

[Qt] Hide nTxConfirmTarget behind WalletModel 6f0289967f

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

jonasschnelli force-pushed on Oct 28, 2016

jonasschnelli commented at 8:45 am on October 28, 2016: contributor

Rebased.

laanwj merged this on Oct 28, 2016

laanwj closed this on Oct 28, 2016

laanwj referenced this in commit d2143dc937 on Oct 28, 2016

laanwj referenced this in commit 0fdf810d26 on Oct 28, 2016

jtimon commented at 11:01 am on December 25, 2016: contributor

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.

codablock referenced this in commit a719b742f1 on Sep 19, 2017

codablock referenced this in commit 77b888a069 on Jan 13, 2018

andvgal referenced this in commit a5629489dc on Jan 6, 2019

CryptoCentric referenced this in commit 76ac1d750e on Feb 15, 2019

DrahtBot locked this on Sep 8, 2021

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