(Though, I agree, that would still be better than double).
Dependencies:
- Fees: Tests: Check CFeeRate internal precision in mempool_tests.cpp #7728
=Long story “short”=
This is a pretty minimal way to double CFeeRate’s internal precision.
Among other reasons, this should not be merged as it is because it hits performance more than necessary for its use, but if there’s any interest in moving in this direction at this point, please ping me here on github and I will happily review.
Otherwise this PR should just serve to collect nits in case any of the code is useful in the future and maybe inspire someone else to do the painful task of “fixing CFeeRate”.
What I mean by “fixing CFeeRate” is to change it’s interface and usage in a way that would make increasing the internal precision transparent if that was ever needed. This PR should hopefully show how easy it could be to increase the stored precision to an arbitrary value as long as is multiple of the current value KB = 1000. To see how painful it will be in reallity, just change KB*2 to KB*10 in this branch. Once you fix everything, go KB*100 and I bet there will be different things to fix.
And that’s my “long story short” for what I think “CFeeRate is not generic”.
Of course I also strongly dislike the name of the attribute as pointed out by @MarcoFalke in #7728 (review) But separating “style symptoms” from “real potential future plans for extending functionality” was the part of point about using code instead of words for expressing my idea: if it could be confused with reason and/or rational numbers, my initial fear of it being confused with a purely stylistic preference was more than justified.
In the end, my surprising discovery “1004 is right, 1005 is not” in one particular line (see #7727) turned what I expected to take maybe a couple of hours to say “hey guys, this branch but properly simplifying GetFee and with a lot more work before it is what I meant” turned into 3 PRs and many words. I hope this temporal usage of PRs is not too disrupting, and that I had labelled the 3 PRs properly. Only one of them I think is mergeable. Incidentally the 2 branches I don’t want merged are the ones I need other people to see passing travis to make my main point. The additional test in #7728 is just an accident. For those who may be interested in potentially relevant (but probably not) details, even more words:
=Long story long for #7727 #7728 and this PR=
This comes from a discussion that started with #7660 (which was later replaced by #7705 , which I think is much better). I don’t remember the exact conversation (and I’m lazy enough not to search the logs), but basically at some point I complained about CFeeRate not being generic enough and how its design makes it disruptive to increase its internal precision. This got misinterpreted as me proposing to turn CFeeRate into a rational number internally, which is not what I meant. I thought it would be faster for me to draft some code that would very likely contain bugs but would easily pass all the tests and then make many of them fail just by changing one constant. That sounded easy and I vaguely remembered having done it before playing around just to spot certain parts of the code that was interested in reading for whatever reason.
My first mistake was starting with KB+1. Everything seemed to worked as long PRECISION_MULTIPLIER = KB, but when I tried PRECISION_MULTIPLIER = KB it didn’t fail with my known-to-be-almost-certainly-buggy-code, which was very disturbing to me.
Although extremely doubtful to me, it was remotely possible that my first draft thinking (and not deeply) about only 1 in 4 cases and duplicating code in an ugly way for the other 3 cases, maybe fixing one by randomly replacing / for * and > for < around. This code wasn’t intended to be correct, that was besides my point, but it was still possible that it was correct. Not only that, this also assumes that my point about CFeeRate not having its internal precision properly encapsulated invalid.
That forced me to look at my draft code again. [miracle1]
Maybe KB+1 just too small to be captured by C++ unittests. Surely the RPC python tests will easily catch this. I just need more constants to play around to make the same point with unittests:
0static const size_t MB = KB * 1000;
1static const size_t GB = MB * 1000;
2static const size_t TB = GB * 1000;
3static const size_t WEBSCALE = TB * 1000;
4static const size_t PRECISION_MULTIPLIER = WEBSCALE;
5// Use a different constant in the constructor and GetFee to make the point more clear
6static const size_t IMPLICIT_UGLINESS = PRECISION_MULTIPLIER;
This should do it. But it didn’t. This passed the unittests locally. Ok, if I could just modify CFeeRate like this without failing unittests that’s actually worrying. But let’s make sure it passes the extended RPC tests too, that won’t happen. But it happened [miracle2].
Ok, there’s something really wrong in my code and I need to understand what before I can show this to anyone if it even makes any sense after that. Before showing anyone, it would probably be nice to remove some gratis disruption that I just introduced to make the point. For example, GetFeePerK() doesn’t need to be eliminated, and the WEBSCALE constant is just a stupid joke: KB+1 shouldn’t pass the rpc tests either (this last part was what allowed me to move in circles, and #7728 could have potentially saved me from).
Starting from scratch, miracle1 wasn’t a miracle, as expected. It was just #7727 (I didn’t recovered the time spent moving in circles, but I recovered 100 sanity points with that).
I haven’t been able to reproduce miracle2, though. My suspicion is that when I webscaled I was simultaneously doing #7727 plus all the buggy code, but the buggy code was still encapsulated out of the tests/reality without noticing (I sometimes do on purpose temporarily for convenience, but I always noticed before, and in fact that was the kind of mistake I was after when looking for miracle2 without starting from scratch). I don’t think miracle2 was a miracle, probably was just miraculous stupidity, but I’m done trying to reproduce it.
Sorry for the wall of text, I used many warnings for you not to read the whole story, and yet you did. If you think there’s something else I could learn from the long and kind of embarrassing last part, that’s the main reason I shared so many probably-meaningless-but-potentially-useful details in the last long part.