Disable fee estimates for 1 block target #9239

morcos · 2016-11-29T18:14:26Z

Bitcoin Core fee estimation works by returning a fee rate such that the overwhelming majority (95%) of recent transactions with that fee rate were included in a block within the target number of blocks.

With a confirmation target of 1 it is often not possible to find a feerate that is high enough to meet that criteria and when it is possible it is sometimes an exorbitantly high fee rate. Given the way Bitcoin Core's fee estimation works, the lowest target it is reasonable to estimate a feerate for is 2 blocks. This PR avoids returning occasional absurdly high fee rates for the 1 block target by never giving an answer for that target. This is not different from the common case in practice.

estimatefee is modified to return -1 with a target of 1 always.
estimatesmartfee is modified to start searching with a target of 2 if given a target of 1.

The second commit changes the GUI so targets of 1 can't be selected with the smartfee slider. Selecting a target of 1 via config option will be equivalent to selecting a target of 2 as all internal fee estimation is done using estimatesmartfee.

sipa · 2016-11-29T19:28:40Z

Concept ACK

TheBlueMatt · 2016-11-29T19:50:46Z

Concept ACK. Maybe we want to disable for 2 as well?

jonasschnelli · 2016-11-30T10:33:30Z

src/qt/sendcoinsdialog.cpp

@@ -175,7 +175,7 @@ void SendCoinsDialog::setModel(WalletModel *_model)
        // set the smartfee-sliders default value (wallets default conf.target or last stored value)
        QSettings settings;
        if (settings.value("nSmartFeeSliderPosition").toInt() == 0)
-            ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 1);


Don't we need to explicitly deal with the state where the nSmartFeeSliderPosition (persisted position) is set to 1?

Wait, I mean where nSmartFeeSliderPosition is 24 (=conf target 1).

I don't think so. We already weren't bounds checking what was passed in as nTxConfirmTarget and it turns out that QT sliders make values fit between the min and max, so it just works...

luke-jr · 2016-11-30T10:37:21Z

It seems what is exorbitant is a matter of opinion, and it might make sense to allow users to choose it. But I don't care strongly about this. Another possibility is to only have it in the GUI when coin control is enabled, but that may not be worth the effort.

jonasschnelli · 2016-11-30T10:39:32Z

Tested ACK e878689
Also tested with a persistent slider position on confirmation target 1.

morcos · 2016-11-30T13:50:07Z

@luke-jr Yeah it was hard to explain concisely in the PR description. It's not necessarily that the answer is a high fee rate, its that it seems to be the case that when it is a high fee rate, (e.g. 1000 sat/byte), that a much lower fee rate (say 100 sat/byte) is almost just as good an answer, maybe being included in a block 93% of the time instead of 95%.

I think any dumb algorithm is always going to have edge cases that human inspection will indicate aren't the best answer, and we can't go chase them all down, and just add more rules to try to deal with them. However in this case, from the very inception of the algorithm we knew that setting a very high confirmation target (95%) for the first block would be problematic if for no other reason that you would see some amount of transactions that miners hadn't even seen yet when they created the block just due to timing differences. This was the reason the original target was 85%, hopefully low enough that we'd get decent answer for 1 confirmation. Then we changed the target to 95% to make the algorithm more responsive to changes in market conditions, and I should have thought more at the time how the persistent flakiness of answers at a block target of 1 would be confusing to users.

I do think its possible a future improvement may give better answers for a target of 1, but I'd rather get this small change out there now.

gmaxwell · 2016-11-30T15:23:13Z

It seems what is exorbitant is a matter of opinion,

Not really, I've observed returning values that were 5x higher than the highest feerate transaction in the mempool in the brief window before and long after. That is objectively too high. A somewhat different approach is probably needed for estimatefee 1-- including ignoring transactions which arrived too soon before the block, clamping it based on mempool observations, etc.

utACK.

jtimon · 2016-12-01T20:52:09Z

src/policy/fees.cpp

@@ -423,6 +424,10 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoun
    if (confTarget <= 0 || (unsigned int)confTarget > feeStats.GetMaxConfirms())
        return CFeeRate(0);

+    // It's not possible to get reasonable estimates for confTarget of 1
+    if (confTarget == 1)


maybe an assert or an exception would be better here?
EDIT: or return CFeeRate(0) like in CBlockPolicyEstimator::estimateFee

@jtimon I wanted to leave the external behavior essentially unchanged. For estimatefee, it often returned -1, so making it always do that would not be unexpected. It would be very unexpected for estimatesmartfee to not return a reasonable answer. Note that because of this the GUI changes are actually optional, but I think provide for a more clear experience.

I don't understand.
Who is currently calling estimateSmartFee with confTarget = 1 ?
And how is 0 less "reasonable" than the answer for another value?
Why do we want the GUI changes to be optimal?

Before this PR anybody could have been using the RPC call estimatesmartfee and not expecting it to return -1 (which is what CFeeRate(0) gets translated into via RPC). estimatesmartfee already gave you the answer for another value when it couldn't give you one for the target you asked for, it would have been exceedingly unusually for it to give you a -1 except right at startup. So for any outside users of estimatesmartfee, this is the expected result. You should be able to ask for a target of 1, and it'll give you the best answer it can.

EDIT: Also anyone that has a config file or a command line option setting nTxConfirmTarget = 1 will be calling the internal function estimateSmartFee and their behavior would become unnecessarily broken if it returns -1.

We don't want to GUI changes to be optional, but it points out why the other changes are the correct ones, they don't break the existing API.

jtimon · 2016-12-01T20:56:58Z

src/qt/sendcoinsdialog.cpp

@@ -175,7 +175,7 @@ void SendCoinsDialog::setModel(WalletModel *_model)
        // set the smartfee-sliders default value (wallets default conf.target or last stored value)
        QSettings settings;
        if (settings.value("nSmartFeeSliderPosition").toInt() == 0)
-            ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 1);
+            ui->sliderSmartFee->setValue(ui->sliderSmartFee->maximum() - model->getDefaultConfirmTarget() + 2);


A constant would be nice

#define MIN_CONFIRM_TARGET 2

?

I agree this is slightly non-ideal, but it's convoluted a bit by the fact that the sliderSmartFee value is not the actual confirmation target. I'd rather not make an additional change here, but when we make a change to support estimates greater than 25, we'll have to figure out the right way to do it...

Right, I agree it is convoluted. Everything would probably look clearer if the "fast" side of the bar was on the left or if we at least had a function that does the conversion ie

return ui->sliderSmartFee->maximum() - ui->sliderSmartFee->value() + 2

But I don't see how a constant would make it any less readable.
At the same time, it doesn't mean it cannot be done later.

jtimon · 2016-12-01T20:58:28Z

utACK e878689 besides nits.

sdaftuar · 2016-12-01T21:56:24Z

lightly tested ACK e878689

e878689 Make GUI incapable of setting tx confirm target of 1 (Alex Morcos) d824ad0 Disable fee estimates for a confirm target of 1 block (Alex Morcos)

laanwj · 2016-12-02T07:27:21Z

The GUI changes may be difficult to backport here because the fee slider behavior changed in master in the meantime.

jonasschnelli · 2016-12-02T07:28:46Z

What about backporting #8989 (this woulds simplify a backport of this PR)

morcos · 2016-12-02T13:07:41Z

@jonasschnelli I think it is perfectly fine to backport without the GUI changes. As mentioned to @jtimon, that was the intent. Will create separate PR.

EDIT: Oh it's already a separate commit. You can just grab the first commit.
EDIT2: made separate backport

Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.

maflcko · 2016-12-14T12:03:10Z

This was already backported in #9267

Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.

e878689 Make GUI incapable of setting tx confirm target of 1 (Alex Morcos) d824ad0 Disable fee estimates for a confirm target of 1 block (Alex Morcos)

morcos added 2 commits November 29, 2016 12:18

Disable fee estimates for a confirm target of 1 block

d824ad0

Make GUI incapable of setting tx confirm target of 1

e878689

fanquake added the TX fees and policy label Nov 30, 2016

jonasschnelli reviewed Nov 30, 2016

View reviewed changes

jonasschnelli added this to the 0.14.0 milestone Dec 1, 2016

jonasschnelli added the Needs backport label Dec 1, 2016

jtimon reviewed Dec 1, 2016

View reviewed changes

laanwj modified the milestones: 0.13.2, 0.14.0 Dec 2, 2016

laanwj merged commit e878689 into bitcoin:master Dec 2, 2016

laanwj added a commit that referenced this pull request Dec 2, 2016

Merge #9239: Disable fee estimates for 1 block target

3fbf079

e878689 Make GUI incapable of setting tx confirm target of 1 (Alex Morcos) d824ad0 Disable fee estimates for a confirm target of 1 block (Alex Morcos)

morcos added a commit to morcos/bitcoin that referenced this pull request Dec 2, 2016

Disable fee estimates for a confirm target of 1 block

3688866

Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.

morcos mentioned this pull request Dec 2, 2016

[0.13] Disable fee estimates for a confirm target of 1 block #9267

Merged

maflcko removed the Needs backport label Dec 14, 2016

sdaftuar mentioned this pull request Jan 10, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

luke-jr mentioned this pull request Jan 13, 2017

[RPC] Simplified bumpfee command. #8456

Merged

lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 13, 2018

Disable fee estimates for a confirm target of 1 block

092922c

Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable fee estimates for 1 block target #9239

Disable fee estimates for 1 block target #9239

morcos commented Nov 29, 2016

sipa commented Nov 29, 2016

TheBlueMatt commented Nov 29, 2016

jonasschnelli Nov 30, 2016

jonasschnelli Nov 30, 2016

morcos Nov 30, 2016

luke-jr commented Nov 30, 2016

jonasschnelli commented Nov 30, 2016

morcos commented Nov 30, 2016 •

edited

gmaxwell commented Nov 30, 2016 •

edited

jtimon Dec 1, 2016 •

edited

morcos Dec 2, 2016

jtimon Dec 2, 2016

morcos Dec 2, 2016 •

edited

jtimon Dec 1, 2016 •

edited

morcos Dec 2, 2016

jtimon Dec 2, 2016

jtimon commented Dec 1, 2016

sdaftuar commented Dec 1, 2016

laanwj commented Dec 2, 2016

jonasschnelli commented Dec 2, 2016

morcos commented Dec 2, 2016 •

edited

maflcko commented Dec 14, 2016

Disable fee estimates for 1 block target #9239

Disable fee estimates for 1 block target #9239

Conversation

morcos commented Nov 29, 2016

sipa commented Nov 29, 2016

TheBlueMatt commented Nov 29, 2016

jonasschnelli Nov 30, 2016

Choose a reason for hiding this comment

jonasschnelli Nov 30, 2016

Choose a reason for hiding this comment

morcos Nov 30, 2016

Choose a reason for hiding this comment

luke-jr commented Nov 30, 2016

jonasschnelli commented Nov 30, 2016

morcos commented Nov 30, 2016 • edited

gmaxwell commented Nov 30, 2016 • edited

jtimon Dec 1, 2016 • edited

Choose a reason for hiding this comment

morcos Dec 2, 2016

Choose a reason for hiding this comment

jtimon Dec 2, 2016

Choose a reason for hiding this comment

morcos Dec 2, 2016 • edited

Choose a reason for hiding this comment

jtimon Dec 1, 2016 • edited

Choose a reason for hiding this comment

morcos Dec 2, 2016

Choose a reason for hiding this comment

jtimon Dec 2, 2016

Choose a reason for hiding this comment

jtimon commented Dec 1, 2016

sdaftuar commented Dec 1, 2016

laanwj commented Dec 2, 2016

jonasschnelli commented Dec 2, 2016

morcos commented Dec 2, 2016 • edited

maflcko commented Dec 14, 2016

morcos commented Nov 30, 2016 •

edited

gmaxwell commented Nov 30, 2016 •

edited

jtimon Dec 1, 2016 •

edited

morcos Dec 2, 2016 •

edited

jtimon Dec 1, 2016 •

edited

morcos commented Dec 2, 2016 •

edited