Improve usage of fee estimation code #6134

Merged
merged 7 commits into from Nov 27, 2015

Conversation

Projects
None yet
9 participants
@morcos
Member

morcos commented May 13, 2015

When automatically adding a fee estimate to a transaction and in the event no estimate is available for the desired confirmation target, it makes more sense to default to a fee estimate for a worse confirmation target rather than the hard coded minimum.

I'm open to a different way of exposing this functionality if there is a better suggestion.

@cozz I don't know anything about QT code, so I think what I did in sendcoinsdialog.cpp is pretty hacky, although it appears to have the desired effect. Perhaps you could take a look and suggest a better approach? This is my attempt to address the concern I mentioned in this comment: #5200 (comment)

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 15, 2015

Contributor

Seems quite reasonable - ut ACK

Contributor

jgarzik commented Sep 15, 2015

Seems quite reasonable - ut ACK

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 20, 2015

Member

Concept ACK

Member

MarcoFalke commented Sep 20, 2015

Concept ACK

@morcos morcos referenced this pull request Oct 15, 2015

Closed

Fee estimate patch #6618

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Oct 16, 2015

Member

OK I reworked this to be a bit better, and combined it with the patch that increases the success threshold to 95%.

@jonasschnelli I did something much simpler in the interface. The slider just indicates the # of blocks for which your fee estimate was valid. Perhaps we need to consider some kind of additional UI element that would explain to people why they aren't able to get an estimate for a smaller # of blocks. I think it'll be very rare and maybe never that you'll successfully get an estimate for 1 block, which people will probably try to do.

Member

morcos commented Oct 16, 2015

OK I reworked this to be a bit better, and combined it with the patch that increases the success threshold to 95%.

@jonasschnelli I did something much simpler in the interface. The slider just indicates the # of blocks for which your fee estimate was valid. Perhaps we need to consider some kind of additional UI element that would explain to people why they aren't able to get an estimate for a smaller # of blocks. I think it'll be very rare and maybe never that you'll successfully get an estimate for 1 block, which people will probably try to do.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 10, 2015

Member

The last commit is as of yet untested, but should resolve some of the issues around accidentally generating transactions that wouldn't be accepted to your own mempool

Member

morcos commented Nov 10, 2015

The last commit is as of yet untested, but should resolve some of the issues around accidentally generating transactions that wouldn't be accepted to your own mempool

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 13, 2015

Member

Needs rebase (trivial main.h conflict).
Tested a bit.

QT: Probably not related to this PR, but is there a reason why the slider – by default – is on the left side "slow confirmation time" and not at the very right side? I guess less then <1% of transactions aim for confirmation >=25blocks.

QT: In my case (mainnet at current height), num-blocks 25 till 18 had the same fee. This feels bad for users, because they might think, "why should I pay a higher fee than i really need for my target?". We need to aggregate num-blocks with the same fee (stop at block 18, don't go further down with the fee, up with the num block target). Also num-blocks 10 and 11 had the same fee, these should be aggregated.

There should be something when calling estimatefee 1. I know that there are to many unknowns for a precise calculation. But it's an estimation. We should not return -1. We should return a value as accurate as possible with the parameters that are available (even is there are hight risks that the estimation is wrong).

Member

jonasschnelli commented Nov 13, 2015

Needs rebase (trivial main.h conflict).
Tested a bit.

QT: Probably not related to this PR, but is there a reason why the slider – by default – is on the left side "slow confirmation time" and not at the very right side? I guess less then <1% of transactions aim for confirmation >=25blocks.

QT: In my case (mainnet at current height), num-blocks 25 till 18 had the same fee. This feels bad for users, because they might think, "why should I pay a higher fee than i really need for my target?". We need to aggregate num-blocks with the same fee (stop at block 18, don't go further down with the fee, up with the num block target). Also num-blocks 10 and 11 had the same fee, these should be aggregated.

There should be something when calling estimatefee 1. I know that there are to many unknowns for a precise calculation. But it's an estimation. We should not return -1. We should return a value as accurate as possible with the parameters that are available (even is there are hight risks that the estimation is wrong).

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 13, 2015

Member

@jonasschnelli This PR should prevent getting a -1 unless you directly call the RPC call estimatefee. I might still expose estimateapproxfee, but for anyone using GUI or wallet code that automatically generates fee, they should now get the best answer possible. I figured that for outside users, they could emulate the logic of estimateapproxfee themselves for now. In the future I agree there should be an RPC call which just does the right thing, but I wasn't ready to expose that now as I'm not sure exactly what it should look like yet.

Member

morcos commented Nov 13, 2015

@jonasschnelli This PR should prevent getting a -1 unless you directly call the RPC call estimatefee. I might still expose estimateapproxfee, but for anyone using GUI or wallet code that automatically generates fee, they should now get the best answer possible. I figured that for outside users, they could emulate the logic of estimateapproxfee themselves for now. In the future I agree there should be an RPC call which just does the right thing, but I wasn't ready to expose that now as I'm not sure exactly what it should look like yet.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 14, 2015

Member

Rebased

Member

morcos commented Nov 14, 2015

Rebased

@gmaxwell gmaxwell added this to the 0.12.0 milestone Nov 14, 2015

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 14, 2015

Member

Concept(s) ACK.

Member

gmaxwell commented Nov 14, 2015

Concept(s) ACK.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 15, 2015

Member

added some checks in the unit test and exposed the functions via RPC (with a warning) and tested in the RPC test.

Member

morcos commented Nov 15, 2015

added some checks in the unit test and exposed the functions via RPC (with a warning) and tested in the RPC test.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 16, 2015

Member

Concept ACK. Plans to test this soon.

Member

jonasschnelli commented Nov 16, 2015

Concept ACK. Plans to test this soon.

morcos added some commits Nov 16, 2015

Add smart fee estimation functions
These are more useful fee and priority estimation functions. If there is no fee/pri high enough for the target you are aiming for, it will give you the estimate for the lowest target that you can reliably obtain.  This is better than defaulting to the minimum.  It will also pass back the target for which it returned an answer.
Increase success threshold for fee estimation to 95%
This provides more conservative estimates and reacts more quickly to a backlog.
Unfortunately the unit test for fee estimation depends on the success threshold (and the decay) chosen; also modify the unit test for the new default success thresholds.
Expose RPC calls for estimatesmart functions
Also add testing for estimatesmartfee in smartfees.py
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 16, 2015

Member

Rebased with a name change to estimateSmartFee instead of Approx

Member

morcos commented Nov 16, 2015

Rebased with a name change to estimateSmartFee instead of Approx

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 17, 2015

Collaborator

nit: whitespace

Collaborator

sdaftuar commented on src/wallet/wallet.cpp in 4fe2823 Nov 17, 2015

nit: whitespace

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 17, 2015

Collaborator

nit: might be better to pass by reference here and below to make clear this can't be NULL, or perhaps update the comment for these functions to document the requirement.

Collaborator

sdaftuar commented on src/policy/fees.h in 6303051 Nov 17, 2015

nit: might be better to pass by reference here and below to make clear this can't be NULL, or perhaps update the comment for these functions to document the requirement.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 17, 2015

Member

ACK apart from those nits

Member

sdaftuar commented Nov 17, 2015

ACK apart from those nits

src/policy/fees.cpp
+ *answerFoundAtTarget = confTarget - 1;
+
+ // If mempool is limiting txs , return at least the min fee from the mempool
+ CAmount minPoolFee = pool->GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

Can GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) be an attribute so that it can be calculated just once in the constructor?

@jtimon

jtimon Nov 18, 2015

Member

Can GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) be an attribute so that it can be calculated just once in the constructor?

This comment has been minimized.

@morcos

morcos Nov 18, 2015

Member

Sure. But honestly it seems a bit silly to me that this is not an attribute of the mempool. I understand the point of keeping policy out of the mempool, but why should you need to be aware of the max size of the mempool just to ask what it's min fee is. The one advantage I can see for the current structure is that you can call TrimToSize with some other number to do testing.
Perhaps TrimToSize should take a sizelimit, and set an internal attribute of the mempool that GetMinFee uses?

@morcos

morcos Nov 18, 2015

Member

Sure. But honestly it seems a bit silly to me that this is not an attribute of the mempool. I understand the point of keeping policy out of the mempool, but why should you need to be aware of the max size of the mempool just to ask what it's min fee is. The one advantage I can see for the current structure is that you can call TrimToSize with some other number to do testing.
Perhaps TrimToSize should take a sizelimit, and set an internal attribute of the mempool that GetMinFee uses?

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

Yes, you are right: it should probably be an attribute of CTxMemPool instead, and minPoolFee could be passed here instead of the CTxMemPool pointer. In fact, CTxMemPool depend on CBlockPolicyEstimator, so CBlockPolicyEstimator shouldn't depend on CTxMemPool too (sigh, I had a branch in which neither one depended on the other at least once but now just https://github.com/jtimon/bitcoin/commits/policy-minrelayfee-0.12.99 ...).

@jtimon

jtimon Nov 18, 2015

Member

Yes, you are right: it should probably be an attribute of CTxMemPool instead, and minPoolFee could be passed here instead of the CTxMemPool pointer. In fact, CTxMemPool depend on CBlockPolicyEstimator, so CBlockPolicyEstimator shouldn't depend on CTxMemPool too (sigh, I had a branch in which neither one depended on the other at least once but now just https://github.com/jtimon/bitcoin/commits/policy-minrelayfee-0.12.99 ...).

This comment has been minimized.

@morcos

morcos Nov 18, 2015

Member

You can't pass the minFee it's dynamic, you have to ask the mempool for it.
I think the CBlockPolicyEstimator should come out of the mempool though for sure.

@morcos

morcos Nov 18, 2015

Member

You can't pass the minFee it's dynamic, you have to ask the mempool for it.
I think the CBlockPolicyEstimator should come out of the mempool though for sure.

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

What I'm saying is you can ask the mempool before calling this (ie rplace the pool pointer parameter with const CAmount& minPoolFee in this new method).

@jtimon

jtimon Nov 18, 2015

Member

What I'm saying is you can ask the mempool before calling this (ie rplace the pool pointer parameter with const CAmount& minPoolFee in this new method).

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

Please look at https://github.com/jtimon/bitcoin/tree/6134-nits
By the way, we should solve the circular dependency by separating CTxMemPoolEntry. That way txmempool.cpp includes policy/fees.h, and policy/fees.cpp includes txmempoolentry.h (but not txmempool.h). Just came to mind again, not saying is within the scope of this PR.

@jtimon

jtimon Nov 18, 2015

Member

Please look at https://github.com/jtimon/bitcoin/tree/6134-nits
By the way, we should solve the circular dependency by separating CTxMemPoolEntry. That way txmempool.cpp includes policy/fees.h, and policy/fees.cpp includes txmempoolentry.h (but not txmempool.h). Just came to mind again, not saying is within the scope of this PR.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 18, 2015

Member

Concept ACK with a very fast review. Again, this would be simpler if there was no priority...

Member

jtimon commented Nov 18, 2015

Concept ACK with a very fast review. Again, this would be simpler if there was no priority...

src/policy/fees.cpp
@@ -504,6 +505,33 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget)
return CFeeRate(median);
}
+CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoundAtTarget, const CTxMemPool *pool)
+
+ if (answerFoundAtTarget)
+ *answerFoundAtTarget = confTarget - 1;
+

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

style nit: too many new lines

@jtimon

jtimon Nov 18, 2015

Member

style nit: too many new lines

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 18, 2015

Member

Someone should prepare clang-format-diff.py for use in bitcoin?

@MarcoFalke

MarcoFalke Nov 18, 2015

Member

Someone should prepare clang-format-diff.py for use in bitcoin?

This comment has been minimized.

@jtimon

jtimon Nov 19, 2015

Member

Yes, we have talked about this many times, but so far nobody has done it.

@jtimon

jtimon Nov 19, 2015

Member

Yes, we have talked about this many times, but so far nobody has done it.

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 19, 2015

Member

I could do it (it's just like https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-formatpy) but when no one is actually using it...

@MarcoFalke

MarcoFalke Nov 19, 2015

Member

I could do it (it's just like https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-formatpy) but when no one is actually using it...

This comment has been minimized.

@jtimon

jtimon Nov 20, 2015

Member

I didn't even knew that already existed, thanks. The problem with that is that "This should only be applied to new files or files which are currently not actively developed on". It would be nice (but more complicated) to have one that, applied to a specific commit, applies the format rules only on the lines you are touching (otherwise your diffs can get use).
EDIT: anyway, we can take this to a new issue or to the ml.

@jtimon

jtimon Nov 20, 2015

Member

I didn't even knew that already existed, thanks. The problem with that is that "This should only be applied to new files or files which are currently not actively developed on". It would be nice (but more complicated) to have one that, applied to a specific commit, applies the format rules only on the lines you are touching (otherwise your diffs can get use).
EDIT: anyway, we can take this to a new issue or to the ml.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 24, 2015

Member

Addressed nits.

Member

sdaftuar commented Nov 24, 2015

Addressed nits.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 24, 2015

Member

My main nit hasn't been addressed, which is not creating a new circular dependency (currently CTxMemPool depends on CBlockPolicyEstimator, but CBlockPolicyEstimator doesn't depend on CTxMemPool until this PR) that is unnecessary (as shown in jtimon@6963b9c ).

Member

jtimon commented Nov 24, 2015

My main nit hasn't been addressed, which is not creating a new circular dependency (currently CTxMemPool depends on CBlockPolicyEstimator, but CBlockPolicyEstimator doesn't depend on CTxMemPool until this PR) that is unnecessary (as shown in jtimon@6963b9c ).

@laanwj laanwj merged commit e304432 into bitcoin:master Nov 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 27, 2015

Merge pull request #6134
e304432 Pass reference to estimateSmartFee and cleanup whitespace (Suhas Daftuar)
56106a3 Expose RPC calls for estimatesmart functions (Alex Morcos)
e93a236 add estimateSmartFee to the unit test (Alex Morcos)
6303051 EstimateSmart functions consider mempool min fee (Alex Morcos)
f22ac4a Increase success threshold for fee estimation to 95% (Alex Morcos)
4fe2823 Change wallet and GUI code to use new smart fee estimation calls. (Alex Morcos)
22eca7d Add smart fee estimation functions (Alex Morcos)
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 27, 2015

Member

I guess I'll create a new PR removing the new and unnecessary circular dependency introduced...

Member

jtimon commented Nov 27, 2015

I guess I'll create a new PR removing the new and unnecessary circular dependency introduced...

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 27, 2015

Member

Opened #7115 to fix the new circular dependency introduced despite my insistence and despite coding the commit to be squashed here to avoid that circular dependency...

Member

jtimon commented Nov 27, 2015

Opened #7115 to fix the new circular dependency introduced despite my insistence and despite coding the commit to be squashed here to avoid that circular dependency...

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Dec 1, 2015

Member

For future reference NACK this PR. I'm really disappointed about @morcos being so stubborn and insisting on introducing the unnecessary circular dependency and that he can even accept the most minimal way to remove it (see the first commit in #7115 ), but I like circular discussions even less than I like circular dependencies and nobody else seems to care enough. So let's leave the circular dependency, but then I need to at least do what I should have done from the beginning and will do next time: nack instead of coding nits to be ignored. NACK nack nacking...

Member

jtimon commented Dec 1, 2015

For future reference NACK this PR. I'm really disappointed about @morcos being so stubborn and insisting on introducing the unnecessary circular dependency and that he can even accept the most minimal way to remove it (see the first commit in #7115 ), but I like circular discussions even less than I like circular dependencies and nobody else seems to care enough. So let's leave the circular dependency, but then I need to at least do what I should have done from the beginning and will do next time: nack instead of coding nits to be ignored. NACK nack nacking...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

but then I need to at least do what I should have done from the beginning and will do next time: nack instead of coding nits to be ignored. NACK nack nacking...

The practical problem is that we can't hold up required changes forever because they have some coding nits. There's, unfortunately, a compromise between accumulating some technical debt or having any single change take forever because of architectural concerns.

Member

laanwj commented Dec 1, 2015

but then I need to at least do what I should have done from the beginning and will do next time: nack instead of coding nits to be ignored. NACK nack nacking...

The practical problem is that we can't hold up required changes forever because they have some coding nits. There's, unfortunately, a compromise between accumulating some technical debt or having any single change take forever because of architectural concerns.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Dec 1, 2015

Member

This was merged without my nit and morcos insists on nacking any code that removes the circular dependency he unnecessarily introduced. I think it has little to do with "prs taking forever". But hey I'm in favor of merging bip68 and other PRs that are taking forever if you want to change the subject.

Member

jtimon commented Dec 1, 2015

This was merged without my nit and morcos insists on nacking any code that removes the circular dependency he unnecessarily introduced. I think it has little to do with "prs taking forever". But hey I'm in favor of merging bip68 and other PRs that are taking forever if you want to change the subject.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 1, 2015

Member
Member

sipa commented Dec 1, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Dec 1, 2015

Member

See yesterday's IRC discussion, yes, he even nacks the most minimal way to remove the circular dependency (the first commit currently in #7115 ) and he will not provide an alternative because he does not want to remove the dependency.

Member

jtimon commented Dec 1, 2015

See yesterday's IRC discussion, yes, he even nacks the most minimal way to remove the circular dependency (the first commit currently in #7115 ) and he will not provide an alternative because he does not want to remove the dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment