Improve usage of fee estimation code

morcos commented at 8:39 pm on May 13, 2015: member

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)

laanwj added the label Wallet on May 15, 2015

jgarzik commented at 5:32 pm on September 15, 2015: contributor

Seems quite reasonable - ut ACK

MarcoFalke commented at 4:57 pm on September 20, 2015: member

Concept ACK

morcos force-pushed on Oct 16, 2015

morcos commented at 4:48 pm on October 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.

morcos force-pushed on Nov 10, 2015

morcos commented at 11:29 pm on November 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

jonasschnelli commented at 7:58 pm on November 13, 2015: contributor

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 commented at 10:04 pm on November 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.

morcos force-pushed on Nov 14, 2015

morcos commented at 7:02 pm on November 14, 2015: member

Rebased

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

gmaxwell commented at 8:56 pm on November 14, 2015: contributor

Concept(s) ACK.

morcos commented at 5:59 pm on November 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.

jonasschnelli commented at 8:07 pm on November 16, 2015: contributor

Concept ACK. Plans to test this soon.

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.

22eca7da22

Change wallet and GUI code to use new smart fee estimation calls. 4fe28236c0

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.

f22ac4a22c

EstimateSmart functions consider mempool min fee 6303051470

add estimateSmartFee to the unit test e93a236d7a

Expose RPC calls for estimatesmart functions

Also add testing for estimatesmartfee in smartfees.py

56106a3300

morcos force-pushed on Nov 16, 2015

morcos commented at 8:44 pm on November 16, 2015: member

Rebased with a name change to estimateSmartFee instead of Approx

sdaftuar commented at 5:50 pm on November 17, 2015: member

ACK apart from those nits

in src/policy/fees.cpp: in 56106a3300 outdated

520+
521+    if (answerFoundAtTarget)
522+        *answerFoundAtTarget = confTarget - 1;
523+
524+    // If mempool is limiting txs , return at least the min fee from the mempool
525+    CAmount minPoolFee = pool->GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();

jtimon commented at 3:47 pm on November 18, 2015:

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

morcos commented at 4:06 pm on November 18, 2015:

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?

jtimon commented at 4:36 pm on November 18, 2015:

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

morcos commented at 4:42 pm on November 18, 2015:

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.

jtimon commented at 5:09 pm on November 18, 2015:

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 commented at 9:41 pm on November 18, 2015:

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 commented at 3:49 pm on November 18, 2015: contributor

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

in src/policy/fees.cpp: in 56106a3300 outdated

504@@ -504,6 +505,33 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget)
505     return CFeeRate(median);
506 }
507 
508+CFeeRate CBlockPolicyEstimator::estimateSmartFee(int confTarget, int *answerFoundAtTarget, const CTxMemPool *pool)

jtimon commented at 3:50 pm on November 18, 2015:

style nit: https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L41

in src/policy/fees.cpp: in 56106a3300 outdated

559+        median = priStats.EstimateMedianVal(confTarget++, SUFFICIENT_PRITXS, MIN_SUCCESS_PCT, true, nBestSeenHeight);
560+    }
561+
562+    if (answerFoundAtTarget)
563+        *answerFoundAtTarget = confTarget - 1;
564+

jtimon commented at 3:52 pm on November 18, 2015:

style nit: too many new lines

MarcoFalke commented at 3:56 pm on November 18, 2015:

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

jtimon commented at 7:06 pm on November 19, 2015:

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

MarcoFalke commented at 7:18 pm on November 19, 2015:

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…

jtimon commented at 2:31 pm on November 20, 2015:

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 commented at 1:52 pm on November 24, 2015: member

Addressed nits.

Pass reference to estimateSmartFee and cleanup whitespace e30443244a

jtimon commented at 2:28 pm on November 24, 2015: contributor

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 https://github.com/jtimon/bitcoin/commit/6963b9ccb2c889564a2e0239aa73f03f6d406784 ).

laanwj merged this on Nov 27, 2015

laanwj closed this on Nov 27, 2015

laanwj referenced this in commit e92377fa7f on Nov 27, 2015

jtimon commented at 12:42 pm on November 27, 2015: contributor

I guess I’ll create a new PR removing the new and unnecessary circular dependency introduced…

jtimon commented at 2:04 pm on November 27, 2015: contributor

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 commented at 12:07 pm on December 1, 2015: contributor

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 commented at 12:11 pm on December 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.

jtimon commented at 12:16 pm on December 1, 2015: contributor

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 commented at 12:28 pm on December 1, 2015: member

He does not nack removing the circular dependency. He disagrees with making the estimation code users responsible for knowing there is a limit.

jtimon commented at 12:35 pm on December 1, 2015: contributor

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.

furszy referenced this in commit 7c102142c7 on Jun 2, 2020

random-zebra referenced this in commit 4b1f3eb792 on Aug 18, 2020

MarcoFalke locked this on Sep 8, 2021

Improve usage of fee estimation code #6134