Add and use `satToBtc` and `btcToSat` util functions #31356

andremralves · 2024-11-23T07:13:34Z

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.

DrahtBot · 2024-11-23T07:13:38Z

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.

DrahtBot · 2024-11-23T07:24:59Z

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

Hints

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.

laanwj · 2024-11-23T11:16:34Z

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.

laanwj · 2024-11-23T11:27:05Z

test/functional/test_framework/messages.py

@@ -234,6 +234,14 @@ def from_binary(cls, stream):
    return obj


+def satToBtc(sat_value):
+    return sat_value / COIN


so something like

def satToBtc(sat_value: int) -> Decimal: return Decimal(sat_value) / COIN def btcToSat(btc_value: Decimal) -> int: return int(btc_value * COIN)

Yes, I think this might be better. I've updated the PR.

DrahtBot · 2024-11-24T01:36:23Z

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

Hints

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.

maflcko

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.

maflcko · 2024-11-24T14:10:36Z

test/functional/feature_block.py

@@ -1159,18 +1159,18 @@ def run_test(self):
        self.log.info("Test transaction resurrection during a re-org")
        self.move_tip(76)
        self.next_block(77)
-        tx77 = self.create_and_sign_transaction(out[24], 10 * COIN)
+        tx77 = self.create_and_sign_transaction(out[24], btcToSat(10))


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.

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

- input_amount = int(sum([utxo['value'] for utxo in utxos_to_spend]) * COIN) + input_amount = btcToSat(sum([utxo['value'] for utxo in utxos_to_spend]))

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

rkrux

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?

rkrux · 2024-11-25T09:37:21Z

test/functional/test_framework/messages.py

+def satToBtc(sat_value: int) -> Decimal:
+    return Decimal(sat_value) / COIN
+
+
+def btcToSat(btc_value: Decimal) -> int:
+    return int(btc_value * COIN)


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?

Sorry for using camelCase in the issue description, snake_case is preferred in functional tests.

this is definitely the wrong file, I couldn't even find this change

rkrux · 2024-11-25T09:42:50Z

test/functional/rpc_rawtransaction.py

        tx_val = 0.001
-        tx.vout = [CTxOut(int(Decimal(tx_val) * COIN), CScript([OP_RETURN] + [OP_FALSE] * 30))]
+        tx.vout = [CTxOut(btcToSat(Decimal(tx_val)), CScript([OP_RETURN] + [OP_FALSE] * 30))]


Maybe this utility function can become more useful if it accepts float too, thereby getting rid of explicit conversion while calling?

Python float to decimal conversion via string: https://stackoverflow.com/questions/316238/python-float-to-decimal-conversion#comment107437133_316253

rkrux · 2024-11-25T09:49:48Z

test/functional/feature_rbf.py

-        def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None):
+        def branch(prevout, initial_value, max_txs, tree_width=5, fee=None, _total_txs=None):
+            if fee is None:
+                fee = btcToSat(0.00001)


- def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None): + def branch(prevout, initial_value, max_txs, tree_width=5, fee=btcToSat(0.00001), _total_txs=None):

Seems to work.

rkrux · 2024-11-25T09:58:20Z

test/functional/feature_block.py

@@ -1159,18 +1159,18 @@ def run_test(self):
        self.log.info("Test transaction resurrection during a re-org")
        self.move_tip(76)
        self.next_block(77)
-        tx77 = self.create_and_sign_transaction(out[24], 10 * COIN)
+        tx77 = self.create_and_sign_transaction(out[24], btcToSat(10))


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

- input_amount = int(sum([utxo['value'] for utxo in utxos_to_spend]) * COIN) + input_amount = btcToSat(sum([utxo['value'] for utxo in utxos_to_spend]))

rkrux · 2024-11-25T10:03:12Z

test/functional/feature_bip68_sequence.py

            # Overestimate the size of the tx - signatures should be less than 120 bytes, and leave 50 for the output
            tx_size = len(tx.serialize().hex())//2 + 120*num_inputs + 50
-            tx.vout.append(CTxOut(int(value - self.relayfee * tx_size * COIN / 1000), SCRIPT_W0_SH_OP_TRUE))


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.

This conversion was not intuitive to me in the first glance: #30079 (comment)

rkrux · 2024-11-25T10:07:52Z

test/functional/feature_coinstatsindex.py

        tx2_val = '20.99'
-        tx2.vout = [CTxOut(int(Decimal(tx2_val) * COIN), CScript([OP_RETURN] + [OP_FALSE] * 30))]
+        tx2.vout = [CTxOut(btcToSat(Decimal(tx2_val)), CScript([OP_RETURN] + [OP_FALSE] * 30))]


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.

An alternative idea would be to add a btcStringToSat function, instead of overloading btcToSat on type, as this makes type checking harder.

rkrux · 2024-11-25T10:09:56Z

test/functional/mining_prioritisetransaction.py

+        fee_delta_b = satToBtc(Decimal(9999))
+        fee_delta_c_1 = satToBtc(Decimal(-1234))
+        fee_delta_c_2 = satToBtc(Decimal(8888))


Doesn't satToBtc accept an int?

Yes, passing sats as a decimal should never be necessary.

DrahtBot · 2024-11-29T14:30:15Z

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake · 2024-12-04T16:58:06Z

Closing, given the author has said in #31345 that someone else can take it over.

andremralves force-pushed the btc-to-sat branch from 2dff8fa to 8ef6811 Compare November 23, 2024 07:24

DrahtBot added the CI failed label Nov 23, 2024

andremralves force-pushed the btc-to-sat branch from 8ef6811 to 632766a Compare November 23, 2024 07:37

DrahtBot removed the CI failed label Nov 23, 2024

laanwj added the Tests label Nov 23, 2024

laanwj reviewed Nov 23, 2024

View reviewed changes

This was referenced Nov 23, 2024

wallet: Disable creating and loading legacy wallets #31250

Merged

Cluster mempool implementation #28676

Open

include verbose "reject-details" field in testmempoolaccept response #28121

Merged

andremralves force-pushed the btc-to-sat branch from 632766a to 42c0623 Compare November 24, 2024 01:01

andremralves marked this pull request as ready for review November 24, 2024 01:02

DrahtBot added the CI failed label Nov 24, 2024

andremralves force-pushed the btc-to-sat branch 2 times, most recently from 32a5b18 to 8aaa4ad Compare November 24, 2024 02:31

maflcko reviewed Nov 24, 2024

View reviewed changes

Add and use satToBtc and btcToSat util functions

a15e9ae

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

andremralves force-pushed the btc-to-sat branch from 8aaa4ad to a15e9ae Compare November 24, 2024 14:51

DrahtBot removed the CI failed label Nov 24, 2024

rkrux reviewed Nov 25, 2024

View reviewed changes

This was referenced Nov 26, 2024

doc, test: more ephemeral dust follow-ups #31371

Merged

Fee Estimation via Fee rate Forecasters #30157

Draft

DrahtBot added the Needs rebase label Nov 29, 2024

rkrux mentioned this pull request Dec 2, 2024

Add sat_to_btc() and conversely btc_to_sat() util functions in functional tests #31345

Closed

fanquake closed this Dec 4, 2024

Add and use satToBtc and btcToSat util functions #31356

Add and use satToBtc and btcToSat util functions #31356

Uh oh!

Conversation

andremralves commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Nov 23, 2024

Uh oh!

laanwj commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 24, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkrux Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 29, 2024

Uh oh!

fanquake commented Dec 4, 2024

Uh oh!

Uh oh!

Add and use `satToBtc` and `btcToSat` util functions #31356

Add and use `satToBtc` and `btcToSat` util functions #31356

andremralves commented Nov 23, 2024 •

edited

Loading

DrahtBot commented Nov 23, 2024 •

edited

Loading

laanwj commented Nov 23, 2024 •

edited

Loading

laanwj Nov 23, 2024 •

edited

Loading

rkrux Nov 25, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading

rkrux Nov 25, 2024 •

edited

Loading