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

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.

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.

andremralves force-pushed on Nov 23, 2024

DrahtBot added the label CI failed on Nov 23, 2024

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.

andremralves force-pushed on Nov 23, 2024

DrahtBot removed the label CI failed on Nov 23, 2024

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.

laanwj added the label Tests on Nov 23, 2024

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.

andremralves force-pushed on Nov 24, 2024

andremralves marked this as ready for review on Nov 24, 2024

DrahtBot added the label CI failed on Nov 24, 2024

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.

andremralves force-pushed on Nov 24, 2024

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]))

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.

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

andremralves force-pushed on Nov 24, 2024

DrahtBot removed the label CI failed on Nov 24, 2024

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

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:

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

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.

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)

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.

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.

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?

DrahtBot added the label Needs rebase on Nov 29, 2024

DrahtBot commented at 2:30 pm on November 29, 2024: contributor

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

fanquake commented at 4:58 pm on December 4, 2024: member

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

fanquake closed this on Dec 4, 2024

Add and use satToBtc and btcToSat util functions #31356

Code Coverage & Benchmarks

Reviews

Conflicts

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