Fix estimatesmartfee rounding display issue #11303

pull TheBlueMatt wants to merge 3 commits into bitcoin:master from TheBlueMatt:2017-09-estimatesmartfee-round changing 9 files +23 −21
  1. TheBlueMatt commented at 7:54 PM on September 11, 2017: member

    This fixes an issue where estimatesmartfee which matches at the min relay fee will return 999 sat/byte instead of 1000 sat/byte due to a float rounding issue. I went ahead and made all float <-> int conversion outside of test/qt explicit (test only had one or two more, Qt had quite a few, including many in the Qt headers themselves) and added overloads to CFeeRate to force callers to do an explicit round themselves. Easy to test with -Wfloat-conversion.

  2. Remove countMaskInv caching in bench framework
    We were saving a div by caching the inverse as a float, but this
ended up requiring a int -> float -> int conversion, which takes
almost as much time as the difference between float mul and div.

There are lots of other more pressing issues with the bench
framework which probably require simply removing the adaptive
iteration count stuff anyway.
    0b1b9148cd
  3. Make float <-> int casts explicit outside of test, qt, CFeeRate 53a6590f49
  4. TheBlueMatt force-pushed on Sep 11, 2017
  5. laanwj commented at 10:07 PM on September 11, 2017: member

    I'm scared that we use floats for monetary values in these places, even when just for display. This is ok for a workaround, but in the long run it would be preferable for these to be converted to fixed point math.

  6. Force explicit double -> int conversion for CFeeRate constructor
    This resolves an issue where estimatesmartfee would return 999
sat/byte instead of 1000, due to floating point loss of precision

Thanks to sipa for suggesting is_integral.
    1789e4675b
  7. TheBlueMatt force-pushed on Sep 11, 2017
  8. in src/bench/bench.cpp:58 in 0b1b9148cd outdated 
      54 | @@ -55,21 +55,20 @@ bool benchmark::State::KeepRunning()
  55 |      else {
  56 |          now = gettimedouble();
  57 |          double elapsed = now - lastTime;
  58 | -        double elapsedOne = elapsed * countMaskInv;
  59 | +        double elapsedOne = elapsed / (countMask + 1);
    sipa commented at 12:39 AM on September 12, 2017:

    In commit "Remove countMaskInv caching in bench framework".

    Isn't countMask always a power of two minus one? If so, we could just remember the number of bits in it, and use a shift instead of a division?

    EDIT: this doesn't apply here, as you can't shift a double, but does in line 64.

    TheBlueMatt commented at 12:00 AM on September 29, 2017:

    I'm gonna leave this be, I think we need to re-write the bench crap anyway (there is a very massive difference between runs based on the number of iterations, in many of my tests, so all of this garbage needs to just be removed).

  9. in src/policy/fees.cpp:1046 in 53a6590f49 outdated 
    1042 | @@ -1043,5 +1043,5 @@ CAmount FeeFilterRounder::round(CAmount currentMinFee)
1043 |      if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) {
1044 |          it--;
1045 |      }
1046 | -    return *it;
1047 | +    return static_cast<CAmount>(*it);
    sipa commented at 12:42 AM on September 12, 2017:

    In commit "Make float <-> int casts explicit outside of test, qt, CFeeRate"

    Nit: for primitive types we usually use C-style casts for brevity (in many places in this commit).

    TheBlueMatt commented at 12:50 AM on September 12, 2017:

    Heh, I was assuming based on #10498 that we want to migrate. I dont care either way.

    sipa commented at 12:51 AM on September 12, 2017:

    Note that #10498 is only about non-primitive types.

    I don't care strongly either (it's not in the developer notes) - just pointing out that it differs from common style.

    TheBlueMatt commented at 12:55 AM on September 12, 2017:

    Ah, heh, well CAmount we stradle the line...cause its a primitive typedef we often pass it by reference. I'll leave it for now since its a bit more clear what going on.

  10. MarcoFalke commented at 7:13 AM on September 12, 2017: member

    Concept ACK 1789e4675b17f274fcb0761321e6fd249a102f40. I think it is fine to use double for fee estimation, as it is an approximation anyway. The rounding should take care of not violating the edge cases.

  11. MarcoFalke commented at 7:14 AM on September 12, 2017: member

    The bench changes seem unrelated to estimatefee. Shouldn't those go to another pull?

  12. morcos commented at 1:37 PM on September 12, 2017: member

    +1 splitting the changes (EDIT: maybe not worth it now, but i would have preferred that) @laanwj I don't think it makes sense to use integer math for fee estimation. The bug here wasn't necessarily caused by floating point math but by the fact that we were implicitly taking a floor to round the answer to an integer whereas we should have been rounding or ceiling. But we would still need to have made a similar choice even if we weren't using floating point.

  13. TheBlueMatt commented at 2:40 PM on September 12, 2017: member

    All of the changes are from -Wfloat-conversion, so I figured I'd dump them all together, otherwise I would have just skipped the bench/wallet/etc changes and only done the CFeeRate ones.

  14. morcos commented at 2:47 PM on September 12, 2017: member

    ACK 1789e46

    This doesn't fix all cases of 999 estimates though, my guess is the rest are caused by tracking fee estimation for transactions which enter mempool via reorg. Will fix that separately.

  15. MarcoFalke referenced this in commit 93d20a734d on Sep 29, 2017
  16. MarcoFalke commented at 4:07 PM on September 30, 2017: member

    Tested ACK 1789e4675b17f274fcb0761321e6fd249a102f40

  17. MarcoFalke added the label TX fees and policy on Sep 30, 2017
  18. MarcoFalke merged this on Sep 30, 2017
  19. MarcoFalke closed this on Sep 30, 2017
  20. MarcoFalke referenced this in commit e542728cde on Sep 30, 2017
  21. PastaPastaPasta referenced this in commit f243e47073 on Jan 17, 2020
  22. PastaPastaPasta referenced this in commit 8b135747a2 on Jan 22, 2020
  23. PastaPastaPasta referenced this in commit 87c9f2f6c6 on Jan 22, 2020
  24. zkbot referenced this in commit aa225ebb0b on Jan 24, 2020
  25. zkbot referenced this in commit 74ff73abab on Jan 24, 2020
  26. PastaPastaPasta referenced this in commit a3f93afb98 on Jan 29, 2020
  27. PastaPastaPasta referenced this in commit 845cb0d263 on Jan 29, 2020
  28. PastaPastaPasta referenced this in commit 74c5415a28 on Jan 31, 2020
  29. PastaPastaPasta referenced this in commit 2208b6320b on Mar 14, 2020
  30. PastaPastaPasta referenced this in commit df3e0e6ebc on Mar 15, 2020
  31. PastaPastaPasta referenced this in commit 0aa6508e31 on Mar 16, 2020
  32. PastaPastaPasta referenced this in commit 75f32fb50f on Mar 16, 2020
  33. furszy referenced this in commit 4ed15cc69d on Jun 8, 2020
  34. ckti referenced this in commit 1b94a0183b on Mar 28, 2021
  35. ckti referenced this in commit 9bb144fee3 on Mar 28, 2021
  36. gades referenced this in commit 6b257f67c3 on Jun 24, 2021
  37. gades referenced this in commit 1733872eda on Jun 30, 2021
  38. DrahtBot locked this on Sep 8, 2021
Contributors
TheBlueMattlaanwjsipaMarcoFalkemorcos
Labels
TX fees and policy
Linked (view graph)
#11309 Minor cleanups for AcceptToMemoryPool
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me