Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable fee estimates for 1 block target #9239

Merged
merged 2 commits into from Dec 2, 2016

Conversation

@morcos
Copy link
Member

commented Nov 29, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

Concept ACK

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2016

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

@@ -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);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 30, 2016

Member

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

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 30, 2016

Member

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

This comment has been minimized.

Copy link
@morcos

morcos Nov 30, 2016

Author Member

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

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

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

bildschirmfoto 2016-11-30 um 11 30 35

@morcos

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2016

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 jonasschnelli added this to the 0.14.0 milestone Dec 1, 2016

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

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 1, 2016

Member

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

This comment has been minimized.

Copy link
@morcos

morcos Dec 2, 2016

Author Member

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

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 2, 2016

Member

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?

This comment has been minimized.

Copy link
@morcos

morcos Dec 2, 2016

Author Member

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.

@@ -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);

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 1, 2016

Member

A constant would be nice

#define MIN_CONFIRM_TARGET 2

?

This comment has been minimized.

Copy link
@morcos

morcos Dec 2, 2016

Author Member

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

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 2, 2016

Member

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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

utACK e878689 besides nits.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Dec 1, 2016

lightly tested ACK e878689

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Dec 2, 2016
Merge #9239: Disable fee estimates for 1 block target
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

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

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

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

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

@morcos

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

@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 added a commit to morcos/bitcoin that referenced this pull request Dec 2, 2016
Disable fee estimates for a confirm target of 1 block
Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 14, 2016

This was already backported in #9267

@sdaftuar sdaftuar referenced this pull request Jan 10, 2017
16 of 18 tasks complete
lateminer added a commit to lateminer/bitcoin that referenced this pull request Jan 13, 2018
Disable fee estimates for a confirm target of 1 block
Backport of bitcoin#9239 without GUI changes and fixing conflicts in tests.
codablock added a commit to codablock/dash that referenced this pull request Jan 17, 2018
Merge bitcoin#9239: Disable fee estimates for 1 block target
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)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#9239: Disable fee estimates for 1 block target
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)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 25, 2019
Merge bitcoin#9239: Disable fee estimates for 1 block target
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.