test: update satoshi_round function #29566

pull naiyoma wants to merge 1 commits into bitcoin:master from naiyoma:test/delete_satoshi_round changing 2 files +9 −6
  1. naiyoma commented at 12:47 PM on March 5, 2024: contributor

    This PR refactors satoshi_round to accept different rounding modes and make rounding a required argument.

    Continuation of #23225

  2. DrahtBot commented at 12:47 PM on March 5, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK AngusP, maflcko, achow101
    Concept ACK BrandonOdiwuor, kristapsk
    Stale ACK rkrux, ismaelsadeeq

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Mar 5, 2024
  5. ismaelsadeeq commented at 4:44 PM on March 11, 2024: member

    Concept ACK

    We generally prevent fee calculation with floats to maintain precision as I have seen in a few places.

    https://github.com/bitcoin/bitcoin/blob/a945f09fa6e0f94cc424da9e06516f9cfa192545/test/functional/test_framework/util.py#L252

    Using Decimal/Integer will be more predictable than using float which are prone to rounding errors.

    If this is the scope of this PR, there are still places where we use float to represent fee rate and output values, you can refactor them to use Decimal or Integer but I am not sure how to maintain this and prevent adding fee/feerate representation or calculation in floats.

  6. maflcko commented at 4:58 PM on March 11, 2024: member

    lgtm. Seems fine to force the call site to specify the rounding when turning a random float into a satoshi amout.

    Note that you are not really "removing" satoshi_round, as it is still used, just inlined.

    An alternative could be to make rounding a required argument.

  7. naiyoma commented at 7:08 AM on March 12, 2024: contributor

    lgtm. Seems fine to force the call site to specify the rounding when turning a random float into a satoshi amout.

    Note that you are not really "removing" satoshi_round, as it is still used, just inlined.

    An alternative could be to make rounding a required argument.

    I agree with the alternative suggestion, I'll restore the satoshi_round function and make rounding a requirement.

  8. naiyoma commented at 7:22 AM on March 12, 2024: contributor

    If this is the scope of this PR, there are still places where we use float to represent fee rate and output values, you can refactor them to use Decimal or Integer but I am not sure how to maintain this and prevent adding fee/feerate representation or calculation in floats.

    I'll look through the tests and enforce the use of Decimal or Integer types. I am not sure if we can explicitly prevent floats in fee calculations within tests, but a good place to start would be type hinting

  9. naiyoma force-pushed on Mar 13, 2024
  10. BrandonOdiwuor commented at 3:51 PM on April 6, 2024: contributor

    Concept ACK moving from float to Decimal

  11. in test/functional/feature_fee_estimation.py:83 in b7a4a8118b outdated 
      82 | +            feerate = Decimal(e["feerate"])
  83 |              assert_greater_than(feerate, 0)
  84 |  
  85 | -            if feerate + delta < min(fees_seen) or feerate - delta > max(fees_seen):
  86 | +            min_fees_seen = Decimal(min(fees_seen)).quantize(Decimal('0.00000001'))
  87 | +            max_fees_seen = Decimal(max(fees_seen)).quantize(Decimal('0.00000001'))
    rkrux commented at 10:22 AM on April 10, 2024:

    We can't use satoshi_round() here because it will by default round down and that's not what's needed here?

    naiyoma commented at 2:18 PM on April 15, 2024:

    For this function, I wasn't sure which rounding type to use. That's why I opted to keep the implementation and just change to Decimal

    AngusP commented at 10:52 AM on May 10, 2024:

    Default Decimal rounding is ROUND_HALF_EVEN -- "Round to nearest with ties going to nearest even integer.":

    >>> from decimal import getcontext
>>> getcontext().rounding
ROUND_HALF_EVEN

    I wasn't super sure what you are trying to do here, but this example helps:

    >>> Decimal(0.004).quantize(Decimal('0.01'))
Decimal('0.00')

>>> Decimal(0.008).quantize(Decimal('0.01'))
Decimal('0.01')

    So you're still getting 'some' rounding behaviour, but it's not explicit which one...

    I think it would be clearer if you did use satoshi_round:

                min_fees_seen = satoshi_round(min(fees_seen), rounding=ROUND_DOWN)
            max_fees_seen = satoshi_round(max(fees_seen), rounding=ROUND_UP)

    ROUND_DOWN for min and ROUND_UP for max feels intuitively right to me, but you could also use e.g. ROUND_HALF_UP for both (as a bit of a satoshi is more than no satoshis)?

    naiyoma commented at 1:40 PM on May 14, 2024:

    Thanks for the breakdown, seems ideal to also use satoshi_round here and to use round down for min and round up for max, helps with greater precision

  12. rkrux approved
  13. rkrux commented at 10:23 AM on April 10, 2024: contributor

    tACK b7a4a81

    Make successful, all functional tests successful.

    To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.

    1. As per the second commit, the satoshi_round is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
    2. Should we consider squashing the first 2 commits into one because after the review the function was put back?
  14. DrahtBot requested review from ismaelsadeeq on Apr 10, 2024
  15. DrahtBot requested review from BrandonOdiwuor on Apr 10, 2024
  16. in test/functional/feature_fee_estimation.py:41 in b7a4a8118b outdated 
      37 | @@ -38,9 +38,9 @@ def small_txpuzzle_randfee(
  38 |      # It's best to exponentially distribute our random fees
  39 |      # because the buckets are exponentially spaced.
  40 |      # Exponentially distributed from 1-128 * fee_increment
  41 | -    rand_fee = float(fee_increment) * (1.1892 ** random.randint(0, 28))
  42 | +    rand_fee = satoshi_round(fee_increment * Decimal(1.1892 ** random.randint(0, 28)), rounding=ROUND_DOWN)
    maflcko commented at 12:53 PM on April 11, 2024: 
        rand_fee = satoshi_round(fee_increment * (1.1892 ** random.randint(0, 28)), rounding=ROUND_DOWN)

    Why the decimal?

    naiyoma commented at 2:39 PM on April 15, 2024:

    Because multiplying a Decimal object (fee_increment) with a float (the result of 1.1892 ** random.randint(0, 28))

    results in an error TypeError: unsupported operand type(s) for *: 'decimal.Decimal' and 'float'

    ismaelsadeeq commented at 3:17 PM on July 29, 2024:

    @naiyoma thats because 1.1892 is in float, I think you should instead convert it to decimal and do the exponentiation in decimal?

        rand_fee = satoshi_round(fee_increment * (Decimal('1.1892') ** random.randint(0, 28)), rounding=ROUND_DOWN)
    ismaelsadeeq commented at 3:26 PM on July 29, 2024:

    why round down?

    naiyoma commented at 6:25 PM on July 30, 2024:

    @ismaelsadeeq for consistency since the initial implementation uses ROUND_DOWN implicitly within the satoshi_round function. https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L265

    naiyoma commented at 7:27 PM on July 30, 2024:

    thanks, makes sense to convert (Decimal('1.1892') first.

  17. in test/functional/test_framework/util.py:240 in b7a4a8118b outdated 
     236 | @@ -237,8 +237,9 @@ def get_fee(tx_size, feerate_btc_kvb):
 237 |      return target_fee_sat / Decimal(1e8) # Return result in  BTC
 238 |  
 239 |  
 240 | -def satoshi_round(amount):
 241 | -    return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 242 | +def satoshi_round(amount, rounding=ROUND_DOWN) -> Decimal:
    maflcko commented at 12:53 PM on April 11, 2024: 
    def satoshi_round(amount, *, rounding) -> Decimal:

    why the default, if the goal is to not use a default?

  18. naiyoma commented at 5:13 PM on April 15, 2024: contributor

    tACK b7a4a81

    Make successful, all functional tests successful.

    To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.

    1. As per the second commit, the satoshi_round is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
    2. Should we consider squashing the first 2 commits into one because after the review the function was put back?

    Thanks for the review I have updated the PR Description and will squash commits as suggested.

  19. naiyoma force-pushed on Apr 16, 2024
  20. DrahtBot commented at 10:35 PM on April 16, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/23898692372</sub>

  21. DrahtBot added the label CI failed on Apr 16, 2024
  22. ismaelsadeeq commented at 10:09 PM on April 21, 2024: member

    The C.I failure here is due to unused import @naiyoma

    {'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
^---- failure generated from lint-python.py

^---- ⚠️ Failure generated from lint-*.py scripts!
  23. naiyoma force-pushed on Apr 23, 2024
  24. naiyoma commented at 2:56 PM on April 23, 2024: contributor

    The C.I failure here is due to unused import @naiyoma

    {'-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubsequence', '-zmqpubrawblock', '-zmqpubhashtxhwm', '-zmqpubhashblockhwm', '-includeconf', '-zmqpubhashtx', '-zmqpubrawtx', '-testdatadir', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
test/functional/test_framework/util.py:8:1: F401 'decimal.ROUND_DOWN' imported but unused
^---- failure generated from lint-python.py

^---- ⚠️ Failure generated from lint-*.py scripts!

    Deleted the Unused import, thanks

  25. ismaelsadeeq commented at 3:51 PM on April 23, 2024: member

    utACK 870e71cb8e66232c98b821e38b472e3b8ab9c1c9 🛸

    Should also update the PR Title!

  26. DrahtBot requested review from rkrux on Apr 23, 2024
  27. DrahtBot removed the label CI failed on Apr 23, 2024
  28. naiyoma renamed this:
    test: Refactor fee calculation to remove satoshi_round function
    test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float
    on Apr 23, 2024
  29. in test/functional/feature_fee_estimation.py:7 in 870e71cb8e outdated 
       3 | @@ -4,7 +4,7 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test fee estimation code."""
   6 |  from copy import deepcopy
   7 | -from decimal import Decimal
   8 | +from decimal import Decimal,ROUND_DOWN
    AngusP commented at 10:44 AM on May 10, 2024:

    Nit:

    from decimal import Decimal, ROUND_DOWN
    naiyoma commented at 1:55 PM on May 14, 2024:

    resolved

  30. in test/functional/test_framework/util.py:242 in 870e71cb8e outdated 
     236 | @@ -237,8 +237,9 @@ def get_fee(tx_size, feerate_btc_kvb):
 237 |      return target_fee_sat / Decimal(1e8) # Return result in  BTC
 238 |  
 239 |  
 240 | -def satoshi_round(amount):
 241 | -    return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
 242 | +def satoshi_round(amount, *, rounding) -> Decimal:
 243 | +    """Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
 244 | +    return Decimal(amount).quantize(Decimal('0.00000001'), rounding=rounding)
    AngusP commented at 11:04 AM on May 10, 2024:

    nit:

    SATS_PRECISION = Decimal('0.00000001')
...

def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
    """Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
    return Decimal(amount).quantize(SATS_PRECISION, rounding=rounding)

    The Decimal('0.00000001') appears in a few places, would be good to extract it as a constant SATS_PRECISION (or whatever name you prefer) for readability and to make it less likely someone accidentally sets the wrong number of zeroes when making a change

  31. kristapsk commented at 11:28 AM on May 10, 2024: contributor

    Concept ACK

  32. naiyoma force-pushed on May 14, 2024
  33. AngusP commented at 4:23 PM on May 14, 2024: contributor

    ACK aa46f0ec82c65fc8bb67c544c6b16296940d96dd

  34. DrahtBot requested review from ismaelsadeeq on May 14, 2024
  35. in test/functional/feature_fee_estimation.py:5 in aa46f0ec82 outdated 
       3 | @@ -4,7 +4,7 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test fee estimation code."""
    ismaelsadeeq commented at 3:05 PM on July 29, 2024:

    nit: this commit message and PR title are a bit long see https://chris.beams.io/posts/git-commit/ that was recommended in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

  36. in test/functional/feature_fee_estimation.py:6 in aa46f0ec82 outdated 
       3 | @@ -4,7 +4,7 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test fee estimation code."""
   6 |  from copy import deepcopy
    ismaelsadeeq commented at 3:36 PM on July 29, 2024:

    There is still a single fee calculation in feature_fee_estimation.py done with float in transact_and_mine method.

    naiyoma commented at 10:06 PM on July 30, 2024:

    added it here c96568e4e812ad59ae7c922dac3cea0ee880341c I think this suffices, though I could also use satoshi_round function and round to the nearest half.

  37. DrahtBot requested review from ismaelsadeeq on Jul 29, 2024
  38. naiyoma force-pushed on Jul 30, 2024
  39. naiyoma renamed this:
    test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float
    test: Use Decimal for fee calculations in feature_fee_estimation.py
    on Jul 30, 2024
  40. ismaelsadeeq approved
  41. ismaelsadeeq commented at 10:01 PM on July 31, 2024: member

    re-ACK c96568e4e812ad59ae7c922dac3cea0ee880341c via diff

    The recent review comments were all addressed.

  42. DrahtBot requested review from AngusP on Jul 31, 2024
  43. in test/functional/feature_fee_estimation.py:76 in c96568e4e8 outdated 
      72 | @@ -73,36 +73,38 @@ def small_txpuzzle_randfee(
  73 |  
  74 |  def check_raw_estimates(node, fees_seen):
  75 |      """Call estimaterawfee and verify that the estimates meet certain invariants."""
  76 | -
  77 | -    delta = 1.0e-6  # account for rounding error
  78 | +    delta = Decimal('0.000001')  # account for rounding error
    maflcko commented at 12:44 PM on August 2, 2024:

    not sure this needs to change. Fee estimation uses double internally, so it can't be 100% exact. Seems odd to change test code to imply that setting the rounding error needs to be exact somehow.

    It isn't wrong, but it seems a bit confusing.

    I think, the earlier version of this pull where the only change was to make the rounding mode of satoshi_round a mandatory keyword argument made sense and looked good.

    ismaelsadeeq commented at 1:02 PM on August 2, 2024:

    Agreed might be an overkill because check_raw_estimates function tests for a range, ensuring the returned fee rate is sane.

    Sounds good to me to limit this to @maflcko suggestion

    AngusP commented at 5:37 PM on August 2, 2024:

    Slight +1 on not making delta a Decimal, which suggests there's something important about its precision?

    naiyoma commented at 8:30 PM on August 6, 2024:

    I was aiming for exact precision because of this comment #23225 (comment), but I get your point. Maybe if fee estimation changes to decimals internally in future, the tests can be modified accordingly.

    naiyoma commented at 8:35 PM on August 6, 2024:

    @maflcko IIUC reverting to the earlier version would mean that I also need to discard all the changes made to the functions check_raw_estimates and check_smart_estimates, right?

    maflcko commented at 6:34 AM on August 7, 2024:

    Yes, sounds good to leave the functions completely untouched in this pull. When internally double is changed to something else, the Python code can also use something other than float (which has double precision). Thanks for providing context.

  44. maflcko changes_requested
  45. maflcko commented at 12:44 PM on August 2, 2024: member

    not sure on the latest changes

  46. naiyoma force-pushed on Aug 6, 2024
  47. naiyoma force-pushed on Aug 6, 2024
  48. DrahtBot added the label CI failed on Aug 6, 2024
  49. DrahtBot removed the label CI failed on Aug 7, 2024
  50. naiyoma force-pushed on Aug 7, 2024
  51. naiyoma renamed this:
    test: Use Decimal for fee calculations in feature_fee_estimation.py
    test: test: update satoshi_round function
    on Aug 7, 2024
  52. naiyoma renamed this:
    test: test: update satoshi_round function
    test: update satoshi_round function
    on Aug 7, 2024
  53. naiyoma force-pushed on Aug 7, 2024
  54. DrahtBot added the label CI failed on Aug 7, 2024
  55. DrahtBot commented at 11:43 AM on August 7, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/28457526653</sub>

    <details><summary>Hints</summary>

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  56. maflcko commented at 11:44 AM on August 7, 2024: member

    review ACK 7d7671ad5c83c3d1220d84110259c63b5ed7405d

    The pull request description will need to be updated to reflect the current state.

  57. DrahtBot requested review from ismaelsadeeq on Aug 7, 2024
  58. test: update satoshi_round function
    Refactor satoshi_round function to accept different rounding modes.
Updated call site to use the revised `satoshi_round` function.

Co-authored-by: Kate Salazar <52637275+katesalazar@users.noreply.github.com>
    ec317bc44b
  59. naiyoma force-pushed on Aug 7, 2024
  60. DrahtBot removed the label CI failed on Aug 7, 2024
  61. naiyoma commented at 7:01 PM on August 7, 2024: contributor

    The pull request description will need to be updated to reflect the current state. @maflcko Description updated. Also, reverted fee_increment to a float and then applied satoshi_round with ROUND_DOWN at the call site. This is much cleaner and more readable.

  62. AngusP approved
  63. AngusP commented at 9:43 PM on August 12, 2024: contributor

    ACK ec317bc44b0cb089419d809b5fef38ecb9423051

  64. DrahtBot requested review from maflcko on Aug 12, 2024
  65. maflcko commented at 5:38 AM on August 13, 2024: member

    review ACK ec317bc44b0cb089419d809b5fef38ecb9423051

  66. achow101 commented at 5:26 PM on September 4, 2024: member

    ACK ec317bc44b0cb089419d809b5fef38ecb9423051

  67. achow101 merged this on Sep 4, 2024
  68. achow101 closed this on Sep 4, 2024
  69. TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024
  70. TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024
  71. bitcoin locked this on Sep 4, 2025
Contributors
naiyomaDrahtBotglozowismaelsadeeqmaflckoBrandonOdiwuorrkruxAngusPkristapskachow101


rkruxmaflckoBrandonOdiwuorismaelsadeeq

Labels
Tests
Linked (view graph)
#23225 test: reject float at satoshi_round
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-05-02 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me