Add and use satToBtc and btcToSat util functions #31356

pull andremralves wants to merge 1 commits into bitcoin:master from andremralves:btc-to-sat changing 36 files +224 βˆ’202
  1. andremralves commented at 7:13 am on November 23, 2024: contributor

    Introduce utility functions satToBtc and btcToSat to simplify and standardize conversions between satoshis and bitcoins in functional tests

    #closes #31345

    This PR implements the solution proposed in the issue. The goal is to simplify conversions between satoshis and bitcoins in the functional tests by using utility functions. For example, code like fee_rate=Decimal(feerate * 1000) / COIN could become fee_rate=satToBtc(feerate * 1000) in the proposed solution.

    An additional benefit of satToBtc() is that it ensures the use of Decimal.

    This issue was briefly mentioned in this discussion, where the conversion Decimal(value) / COIN was noted to be repetitive.

    While some may prefer to continue using the / COIN and * COIN conversions, I don’t have a strong preference either way. I’d like to hear your opinions and, if needed, implement the approach that works best.

  2. DrahtBot commented at 7:13 am on November 23, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31356.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31371 (doc, test: more ephemeral dust follow-ups by theStack)
    • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
    • #28676 ([WIP] Cluster mempool implementation by sdaftuar)
    • #28121 (include verbose “reject-details” field in testmempoolaccept response by pinheadmz)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. andremralves force-pushed on Nov 23, 2024
  4. DrahtBot added the label CI failed on Nov 23, 2024
  5. DrahtBot commented at 7:24 am on November 23, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33416833622

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  6. andremralves force-pushed on Nov 23, 2024
  7. DrahtBot removed the label CI failed on Nov 23, 2024
  8. laanwj commented at 11:16 am on November 23, 2024: member

    Concept -0, it seems like a huge change to factor out what is just a multiply/division. imo this only makes sense if the intent is to add extra validity checks to the functions.

    Could at least make sure that the return value from satsToBtc in BTC is a right-precision Decimal and not a float.

  9. laanwj added the label Tests on Nov 23, 2024
  10. in test/functional/test_framework/messages.py:238 in 632766aee6 outdated
    233@@ -234,6 +234,14 @@ def from_binary(cls, stream):
    234     return obj
    235 
    236 
    237+def satToBtc(sat_value):
    238+    return sat_value / COIN
    


    laanwj commented at 11:27 am on November 23, 2024:

    so something like

    0def satToBtc(sat_value: int) -> Decimal:
    1    return Decimal(sat_value) / COIN
    2
    3def btcToSat(btc_value: Decimal) -> int:
    4    return int(btc_value * COIN)
    

    andremralves commented at 4:34 pm on November 24, 2024:
    Yes, I think this might be better. I’ve updated the PR.
  11. andremralves force-pushed on Nov 24, 2024
  12. andremralves marked this as ready for review on Nov 24, 2024
  13. DrahtBot added the label CI failed on Nov 24, 2024
  14. DrahtBot commented at 1:36 am on November 24, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33430954182

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  15. andremralves force-pushed on Nov 24, 2024
  16. andremralves force-pushed on Nov 24, 2024
  17. in test/functional/feature_block.py:1162 in 8aaa4ad214 outdated
    1158@@ -1159,18 +1159,18 @@ def run_test(self):
    1159         self.log.info("Test transaction resurrection during a re-org")
    1160         self.move_tip(76)
    1161         self.next_block(77)
    1162-        tx77 = self.create_and_sign_transaction(out[24], 10 * COIN)
    1163+        tx77 = self.create_and_sign_transaction(out[24], btcToSat(10))
    


    maflcko commented at 2:10 pm on November 24, 2024:

    Not sure about this. I think writing 10 * COIN with the meaning of “10 bitcoins” is self-explanatory and couldn’t be clearer. btcToSat(10) just seems like an extra step and extra complexity without a benefit?

    Also, the “new” style isn’t enforced, so you’ll end up with both styles being used in the future, increasing the confusing further.

    Finally, I had the impression that python code is using snake_case, so btcToSat would be confusing in that regard as well.


    rkrux commented at 9:58 am on November 25, 2024:

    While 10 * COIN seems self-explanatory, I find the util function usage in the following diff more readable and easier on the eyes. Instead of having to process 3 things in one go, namely int conversion, sum() with an argument that’s being calculated, multiplication with COIN, I have to process only one function call with the said argument.

    https://github.com/bitcoin/bitcoin/pull/31356/files#diff-9e79d56c581ca71e62a898ee0d2afda253081118ebcd1744ba9b3d16f0958a80L192

    0- input_amount = int(sum([utxo['value'] for utxo in utxos_to_spend]) * COIN)
    1+ input_amount = btcToSat(sum([utxo['value'] for utxo in utxos_to_spend]))
    
  18. maflcko commented at 2:14 pm on November 24, 2024: member

    Instead of repeating the diff in the pull request description (if someone was interested in the changes in detail, they could just look at the diff themselves), it would be better to mention the benefits/goals and how they are expected to be achieved for new code written after this was merged.

    Possibly there is a case to have feerate conversion helpers, or a class to hold feerates. However, any “global” change here would need a solid motivation and long-term upside.

  19. Add and use `satToBtc` and `btcToSat` util functions
    Introduce utility functions `satToBtc` and `btcToSat` to
    simplify and standardize conversions between satoshis and bitcoins
    in functional tests
    a15e9aeee1
  20. andremralves force-pushed on Nov 24, 2024
  21. DrahtBot removed the label CI failed on Nov 24, 2024
  22. in test/functional/test_framework/messages.py:243 in a15e9aeee1
    234@@ -234,6 +235,14 @@ def from_binary(cls, stream):
    235     return obj
    236 
    237 
    238+def satToBtc(sat_value: int) -> Decimal:
    239+    return Decimal(sat_value) / COIN
    240+
    241+
    242+def btcToSat(btc_value: Decimal) -> int:
    243+    return int(btc_value * COIN)
    


    rkrux commented at 9:37 am on November 25, 2024:
    I see that COIN is defined in this file but not sure if this is the right file to put these functions in, maybe util.py?

    rkrux commented at 9:38 am on November 25, 2024:
    Sorry for using camelCase in the issue description, snake_case is preferred in functional tests.

    Julian128 commented at 10:47 pm on December 2, 2024:
    this is definitely the wrong file, I couldn’t even find this change
  23. in test/functional/rpc_rawtransaction.py:397 in a15e9aeee1
    398 
    399         # Test a transaction where our burn equals maxburnamount
    400         tx = self.wallet.create_self_transfer()['tx']
    401         tx_val = 0.001
    402-        tx.vout = [CTxOut(int(Decimal(tx_val) * COIN), CScript([OP_RETURN] + [OP_FALSE] * 30))]
    403+        tx.vout = [CTxOut(btcToSat(Decimal(tx_val)), CScript([OP_RETURN] + [OP_FALSE] * 30))]
    


    rkrux commented at 9:42 am on November 25, 2024:
    Maybe this utility function can become more useful if it accepts float too, thereby getting rid of explicit conversion while calling?

    rkrux commented at 10:13 am on November 25, 2024:
  24. in test/functional/feature_rbf.py:177 in a15e9aeee1
    174         tx0_outpoint = self.make_utxo(self.nodes[0], initial_nValue)
    175 
    176-        def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None):
    177+        def branch(prevout, initial_value, max_txs, tree_width=5, fee=None, _total_txs=None):
    178+            if fee is None:
    179+                fee = btcToSat(0.00001)
    


    rkrux commented at 9:49 am on November 25, 2024:
    0- def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None):
    1+ def branch(prevout, initial_value, max_txs, tree_width=5, fee=btcToSat(0.00001), _total_txs=None):
    

    Seems to work.

  25. in test/functional/feature_bip68_sequence.py:203 in a15e9aeee1
    196@@ -197,10 +197,10 @@ def test_sequence_lock_confirmed_inputs(self):
    197                             sequence_value = ((cur_time - orig_time) >> SEQUENCE_LOCKTIME_GRANULARITY)+1
    198                         sequence_value |= SEQUENCE_LOCKTIME_TYPE_FLAG
    199                 tx.vin.append(CTxIn(COutPoint(int(utxos[j]["txid"], 16), utxos[j]["vout"]), nSequence=sequence_value))
    200-                value += utxos[j]["value"]*COIN
    201+                value += btcToSat(utxos[j]["value"])
    202             # Overestimate the size of the tx - signatures should be less than 120 bytes, and leave 50 for the output
    203             tx_size = len(tx.serialize().hex())//2 + 120*num_inputs + 50
    204-            tx.vout.append(CTxOut(int(value - self.relayfee * tx_size * COIN / 1000), SCRIPT_W0_SH_OP_TRUE))
    


    rkrux commented at 10:03 am on November 25, 2024:

    Possibly there is a case to have feerate conversion helpers, or a class to hold feerates.

    Agree, a fee rate conversion helper here would make reading this far easier.


    rkrux commented at 10:03 am on November 25, 2024:
    This conversion was not intuitive to me in the first glance: #30079 (review)
  26. in test/functional/feature_coinstatsindex.py:164 in a15e9aeee1
    160@@ -161,7 +161,7 @@ def _test_coin_stats_index(self):
    161         # Generate and send another tx with an OP_RETURN output (which is unspendable)
    162         tx2 = self.wallet.create_self_transfer(utxo_to_spend=tx1_out_21)['tx']
    163         tx2_val = '20.99'
    164-        tx2.vout = [CTxOut(int(Decimal(tx2_val) * COIN), CScript([OP_RETURN] + [OP_FALSE] * 30))]
    165+        tx2.vout = [CTxOut(btcToSat(Decimal(tx2_val)), CScript([OP_RETURN] + [OP_FALSE] * 30))]
    


    rkrux commented at 10:07 am on November 25, 2024:
    Maybe accepting a string with internal string to decimal checking and conversion would also make it more flexible? The explicit Decimal conversion could be removed here then.

    laanwj commented at 12:17 pm on December 1, 2024:
    An alternative idea would be to add a btcStringToSat function, instead of overloading btcToSat on type, as this makes type checking harder.
  27. in test/functional/mining_prioritisetransaction.py:101 in a15e9aeee1
    102-        self.nodes[0].prioritisetransaction(txid=txid_b, fee_delta=int(fee_delta_b * COIN))
    103-        self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_1 * COIN))
    104-        self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_2 * COIN))
    105+        fee_delta_b = satToBtc(Decimal(9999))
    106+        fee_delta_c_1 = satToBtc(Decimal(-1234))
    107+        fee_delta_c_2 = satToBtc(Decimal(8888))
    


    rkrux commented at 10:09 am on November 25, 2024:
    Doesn’t satToBtc accept an int?

    laanwj commented at 12:17 pm on December 1, 2024:
    Yes, passing sats as a decimal should never be necessary.
  28. rkrux commented at 10:13 am on November 25, 2024: none
    Thanks for picking this up! Left few suggestions. Can the PR be split into 2 commits for each util function that will make it easier to process piece by piece?
  29. DrahtBot added the label Needs rebase on Nov 29, 2024
  30. DrahtBot commented at 2:30 pm on November 29, 2024: contributor

    πŸ™ This pull request conflicts with the target branch and needs rebase.


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: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me