Disable fee estimates for 1 block target

morcos commented at 6:14 pm on November 29, 2016: member

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.

Disable fee estimates for a confirm target of 1 block d824ad030e

Make GUI incapable of setting tx confirm target of 1 e878689e55

sipa commented at 7:28 pm on November 29, 2016: member

Concept ACK

TheBlueMatt commented at 7:50 pm on November 29, 2016: member

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

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

in src/qt/sendcoinsdialog.cpp: in e878689e55

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

jonasschnelli commented at 10:33 am on November 30, 2016:

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

jonasschnelli commented at 10:38 am on November 30, 2016:

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

morcos commented at 1:41 pm on November 30, 2016:

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 commented at 10:37 am on November 30, 2016: member

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 commented at 10:39 am on November 30, 2016: contributor

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

morcos commented at 1:50 pm on November 30, 2016: member

@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 commented at 3:23 pm on November 30, 2016: contributor

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.

jonasschnelli added this to the milestone 0.14.0 on Dec 1, 2016

jonasschnelli added the label Needs backport on Dec 1, 2016

in src/policy/fees.cpp: in d824ad030e outdated

423@@ -423,6 +424,10 @@ CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoun
424     if (confTarget <= 0 || (unsigned int)confTarget > feeStats.GetMaxConfirms())
425         return CFeeRate(0);
426 
427+    // It's not possible to get reasonable estimates for confTarget of 1
428+    if (confTarget == 1)

jtimon commented at 8:52 pm on December 1, 2016:

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

morcos commented at 0:33 am on December 2, 2016:

@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.

jtimon commented at 1:15 am on December 2, 2016:

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?

morcos commented at 1:55 am on December 2, 2016:

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.

in src/qt/sendcoinsdialog.cpp: in e878689e55

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

jtimon commented at 8:56 pm on December 1, 2016:

A constant would be nice

0#define MIN_CONFIRM_TARGET 2

?

morcos commented at 0:35 am on December 2, 2016:

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…

jtimon commented at 1:23 am on December 2, 2016:

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

0return 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 commented at 8:58 pm on December 1, 2016: contributor

utACK e878689 besides nits.

sdaftuar commented at 9:56 pm on December 1, 2016: member

lightly tested ACK e878689e5539d8de30283d1461d6466eac65f894

laanwj added this to the milestone 0.13.2 on Dec 2, 2016

laanwj removed this from the milestone 0.14.0 on Dec 2, 2016

laanwj merged this on Dec 2, 2016

laanwj closed this on Dec 2, 2016

laanwj referenced this in commit 3fbf079262 on Dec 2, 2016

laanwj commented at 7:27 am on December 2, 2016: member

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

jonasschnelli commented at 7:28 am on December 2, 2016: contributor

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

morcos commented at 1:07 pm on December 2, 2016: member

@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

morcos referenced this in commit 3688866880 on Dec 2, 2016

MarcoFalke commented at 12:03 pm on December 14, 2016: member

This was already backported in #9267

MarcoFalke removed the label Needs backport on Dec 14, 2016

lateminer referenced this in commit 092922c063 on Jan 13, 2018

codablock referenced this in commit 28e363fbeb on Jan 16, 2018

codablock referenced this in commit 452c8bf69b on Jan 16, 2018

codablock referenced this in commit a95622c0b0 on Jan 17, 2018

andvgal referenced this in commit 34b13b7e8d on Jan 6, 2019

CryptoCentric referenced this in commit 7b08f1f2ae on Feb 25, 2019

DrahtBot locked this on Sep 8, 2021

Disable fee estimates for 1 block target #9239