Cleanup fee estimation code #19630

darosior · 2020-07-30T19:32:54Z

This (does not change behaviour and) cleans up a bit of unused code in CBlockPolicyEstimator and friends, and slightly improves readability of the rest (comment correction etc.). The last commit is a small reformatting one which I could not resist but am happy to remove at will.

darosior · 2020-07-31T18:42:47Z

Added a trivial commit which de-duplicates some duplicated for-loops.

src/policy/fees.h

src/policy/fees.cpp

laanwj · 2020-08-06T17:07:32Z

Code review ACK besides some minor nits

darosior · 2020-08-06T17:15:15Z

Amended after review (the unnecessary variables declaration move and the pre-decrement).

laanwj · 2020-08-06T17:20:16Z

FYI it looks the last usage of requireGreater=false went away in b2322e0

theStack

LGTM, very nice unused code finding and cleanup!
I think the last two commits could be squashed, but no strong objective.

src/policy/fees.cpp

darosior · 2020-08-07T10:57:21Z

@theStack i think the for loops unification are not "small readability improvements", and that the readability improvements are not "unify some duplicated for loops". Also, fixed the string format, thanks!

src/policy/fees.cpp

darosior · 2020-08-08T14:47:26Z

Tests pass but the s390X Travis build is always stuck and eventually results in a failure after one day (i've tried to force-push above but it won't trigger it).

edit: reopened to kick it back..

practicalswift

Concept ACK

src/policy/fees.cpp

jnewbery

Concept ACK

jnewbery · 2020-09-11T15:30:03Z

src/policy/fees.h

Remove the number entirely from the comment. No need for a comment that says "x is x"

Suggested change

/** Decay of .9952 is a half-life of 144 blocks or about 1 day */

/** This decay gives a half-life of 144 blocks or about 1 day */

Same for other comments here.

I don't think it's a trivial "x is x" comment: it's not trivial to me that a decay of say .99931 is equivalent to a half-life of 1008 blocks.

I agree! That's why I'm saying remove the literal value that's in the code and leave the rest.

Ah, ok: will remove if i have to retouch

src/policy/fees.cpp

This was confusing: which one is the good one ? After testing the value is right but not the comment, so fix it. Signed-off-by: Antoine Poinsot <[email protected]>

It was always passed as true, and complicates the (already complex) logic of the function. Signed-off-by: Antoine Poinsot <[email protected]>

Signed-off-by: Antoine Poinsot <[email protected]>

Reported-by: practicalswift <[email protected]> Signed-off-by: Antoine Poinsot <[email protected]>

darosior · 2020-09-14T14:25:14Z

Thanks for the review. Cleaned up the logprint, used the m_ prefix, and made some variables const.

jnewbery · 2020-09-14T15:20:44Z

utACK a3abeec

ariard

Code Review ACK a3abeec.

ariard · 2020-09-25T16:32:53Z

src/policy/fees.cpp

                                const std::map<double, unsigned int>& defaultBucketMap,
                               unsigned int maxPeriods, double _decay, unsigned int _scale)
-    : buckets(defaultBuckets), bucketMap(defaultBucketMap)
+    : buckets(defaultBuckets), bucketMap(defaultBucketMap), decay(_decay), scale(_scale)


If you feel it you can comment scale parameter usage at function declaration.

ariard · 2020-09-25T16:36:43Z

src/policy/fees.cpp

-    // feerate such that all lower values fail, and we go in the opposite direction.
-    unsigned int startbucket = requireGreater ? maxbucketindex : 0;
-    int step = requireGreater ? -1 : 1;
+    const int periodTarget = (confTarget + scale - 1) / scale;


Maybe you can keep comment first halve and precise what the highers values reference is about. I believe it means higher buckets of transactions.

maflcko

ACK a3abeec 💹

Show signature and timestamp

Signature:

Timestamp of file with hash d9cd36b4e6a9b525b00d5cee9eb8107f4bc33c22ac73950c8b6164e5c2d50eda -

maflcko · 2020-09-28T09:38:12Z

src/policy/fees.cpp

-    int maxbucketindex = buckets.size() - 1;
-
-    // requireGreater means we are looking for the lowest feerate such that all higher
-    // values pass, so we start at maxbucketindex (highest feerate) and look at successively


nit in commit 5b8cb35:

I think this line in the comment can be kept

a3abeec policy/fees: remove a floating-point division by zero (Antoine Poinsot) c36869b policy/fees: unify some duplicated for loops (Antoine Poinsot) 569d92a policy/fees: small readability improvements (Antoine Poinsot) 5b8cb35 policy/fee: remove requireGreater parameter in EstimateMedianVal() (Antoine Poinsot) dba8196 policy/fees: correct decay explanation comments (Antoine Poinsot) Pull request description: This (*does not* change behaviour and) cleans up a bit of unused code in `CBlockPolicyEstimator` and friends, and slightly improves readability of the rest (comment correction etc.). The last commit is a small reformatting one which I could not resist but am happy to remove at will. ACKs for top commit: jnewbery: utACK a3abeec MarcoFalke: ACK a3abeec 💹 ariard: Code Review ACK a3abeec. Tree-SHA512: b7620bcd23a2ffa8f7ed859467868fc0f6488279e3ee634f6d408872cb866ad086a037e8ace76599a05b7e9c07768adf5016b0ae782d153196b9c030db4c34a5

Summary: [[bitcoin/bitcoin#20379 | core#20379]] > Remove no longer needed UBSan suppression. > The float divide-by-zero in validation.cpp was fixed by instagibbs in D8857. The other suppression was removed by [[bitcoin/bitcoin#19630 | core#19630]] after fixing fee estimation code that does not exist in Bitcoin ABC. We can assume that we do not need this suppression, and add it back if needed. This is a backport of [[bitcoin/bitcoin#20379 | core#20379]] Test Plan: With UBSAN: `ninja && ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10728

DrahtBot added the TX fees and policy label Jul 30, 2020

darosior changed the title ~~Refactor fee estimation code~~ Cleanup fee estimation code Aug 1, 2020

laanwj reviewed Aug 6, 2020

View reviewed changes

src/policy/fees.h Outdated Show resolved Hide resolved

laanwj reviewed Aug 6, 2020

View reviewed changes