Skip to content

Add and use satToBtc and btcToSat util functions #31356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
andremralves wants to merge 1 commit into from
Closed

Add and use satToBtc and btcToSat util functions #31356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions test/functional/feature_bip68_sequence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
script_to_p2wsh_script,
)
from test_framework.messages import (
COIN,
COutPoint,
CTransaction,
CTxIn,
CTxInWitness,
CTxOut,
tx_from_hex,
btcToSat,
)
from test_framework.script import (
CScript,
Expand Down Expand Up @@ -91,7 +91,7 @@ def test_disable_flag(self):
utxo = self.wallet.send_self_transfer(from_node=self.nodes[0])["new_utxo"]

tx1 = CTransaction()
value = int((utxo["value"] - self.relayfee) * COIN)
value = btcToSat(utxo["value"] - self.relayfee)

# Check that the disable flag disables relative locktime.
# If sequence locks were used, this would require 1 block for the
Expand All @@ -112,7 +112,7 @@ def test_disable_flag(self):
tx2.vin = [CTxIn(COutPoint(tx1_id, 0), nSequence=sequence_value)]
tx2.wit.vtxinwit = [CTxInWitness()]
tx2.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
tx2.vout = [CTxOut(int(value - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
tx2.vout = [CTxOut(int(value - btcToSat(self.relayfee)), SCRIPT_W0_SH_OP_TRUE)]
tx2.rehash()

assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx2.serialize().hex())
Expand Down Expand Up @@ -197,10 +197,10 @@ def test_sequence_lock_confirmed_inputs(self):
sequence_value = ((cur_time - orig_time) >> SEQUENCE_LOCKTIME_GRANULARITY)+1
sequence_value |= SEQUENCE_LOCKTIME_TYPE_FLAG
tx.vin.append(CTxIn(COutPoint(int(utxos[j]["txid"], 16), utxos[j]["vout"]), nSequence=sequence_value))
value += utxos[j]["value"]*COIN
value += btcToSat(utxos[j]["value"])
# 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))
Copy link
Contributor

@rkrux rkrux Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rkrux rkrux Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tx.vout.append(CTxOut(int(value - btcToSat(self.relayfee * tx_size) / 1000), SCRIPT_W0_SH_OP_TRUE))
self.wallet.sign_tx(tx=tx)

if (using_sequence_locks and not should_pass):
Expand Down Expand Up @@ -230,7 +230,7 @@ def test_sequence_lock_unconfirmed_inputs(self):
tx2 = CTransaction()
tx2.version = 2
tx2.vin = [CTxIn(COutPoint(tx1.sha256, 0), nSequence=0)]
tx2.vout = [CTxOut(int(tx1.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
tx2.vout = [CTxOut(int(tx1.vout[0].nValue - btcToSat(self.relayfee)), SCRIPT_W0_SH_OP_TRUE)]
self.wallet.sign_tx(tx=tx2)
tx2_raw = tx2.serialize().hex()
tx2.rehash()
Expand All @@ -250,7 +250,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
tx.vin = [CTxIn(COutPoint(orig_tx.sha256, 0), nSequence=sequence_value)]
tx.wit.vtxinwit = [CTxInWitness()]
tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
tx.vout = [CTxOut(int(orig_tx.vout[0].nValue - relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
tx.vout = [CTxOut(int(orig_tx.vout[0].nValue - btcToSat(relayfee)), SCRIPT_W0_SH_OP_TRUE)]
tx.rehash()

if (orig_tx.hash in node.getrawmempool()):
Expand All @@ -267,7 +267,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):

# Now mine some blocks, but make sure tx2 doesn't get mined.
# Use prioritisetransaction to lower the effective feerate to 0
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=int(-self.relayfee*COIN))
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=btcToSat(-self.relayfee))
cur_time = int(time.time())
for _ in range(10):
self.nodes[0].setmocktime(cur_time + 600)
Expand All @@ -280,7 +280,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=False)

# Mine tx2, and then try again
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=int(self.relayfee*COIN))
self.nodes[0].prioritisetransaction(txid=tx2.hash, fee_delta=btcToSat(self.relayfee))

# Advance the time on the node so that we can test timelocks
self.nodes[0].setmocktime(cur_time+600)
Expand All @@ -307,7 +307,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):

utxo = self.wallet.get_utxo()
tx5.vin.append(CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), nSequence=1))
tx5.vout[0].nValue += int(utxo["value"]*COIN)
tx5.vout[0].nValue += btcToSat(utxo["value"])
self.wallet.sign_tx(tx=tx5)

assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx5.serialize().hex())
Expand Down Expand Up @@ -362,7 +362,7 @@ def test_bip68_not_consensus(self):
tx2 = CTransaction()
tx2.version = 1
tx2.vin = [CTxIn(COutPoint(tx1.sha256, 0), nSequence=0)]
tx2.vout = [CTxOut(int(tx1.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
tx2.vout = [CTxOut(int(tx1.vout[0].nValue - btcToSat(self.relayfee)), SCRIPT_W0_SH_OP_TRUE)]

# sign tx2
self.wallet.sign_tx(tx=tx2)
Expand All @@ -380,7 +380,7 @@ def test_bip68_not_consensus(self):
tx3.vin = [CTxIn(COutPoint(tx2.sha256, 0), nSequence=sequence_value)]
tx3.wit.vtxinwit = [CTxInWitness()]
tx3.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
tx3.vout = [CTxOut(int(tx2.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
tx3.vout = [CTxOut(int(tx2.vout[0].nValue - btcToSat(self.relayfee)), SCRIPT_W0_SH_OP_TRUE)]
tx3.rehash()

assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx3.serialize().hex())
Expand Down
10 changes: 5 additions & 5 deletions test/functional/feature_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
)
from test_framework.messages import (
CBlock,
COIN,
COutPoint,
CTransaction,
CTxIn,
Expand All @@ -24,6 +23,7 @@
SEQUENCE_FINAL,
uint256_from_compact,
uint256_from_str,
btcToSat,
)
from test_framework.p2p import P2PDataStore
from test_framework.script import (
Expand Down Expand Up @@ -809,7 +809,7 @@ def run_test(self):
self.log.info("Reject a block with a transaction with outputs > inputs")
self.move_tip(57)
self.next_block(59)
tx = self.create_and_sign_transaction(out[17], 51 * COIN)
tx = self.create_and_sign_transaction(out[17], btcToSat(51))
b59 = self.update_block(59, [tx])
self.send_blocks([b59], success=False, reject_reason='bad-txns-in-belowout', reconnect=True)

Expand Down Expand Up @@ -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))
Copy link
Member

@maflcko maflcko Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rkrux rkrux Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

b77 = self.update_block(77, [tx77])
self.send_blocks([b77], True)
self.save_spendable_output()

self.next_block(78)
tx78 = self.create_tx(tx77, 0, 9 * COIN)
tx78 = self.create_tx(tx77, 0, btcToSat(9))
b78 = self.update_block(78, [tx78])
self.send_blocks([b78], True)

self.next_block(79)
tx79 = self.create_tx(tx78, 0, 8 * COIN)
tx79 = self.create_tx(tx78, 0, btcToSat(8))
b79 = self.update_block(79, [tx79])
self.send_blocks([b79], True)

Expand Down
8 changes: 4 additions & 4 deletions test/functional/feature_coinstatsindex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
create_coinbase,
)
from test_framework.messages import (
COIN,
CTxOut,
btcToSat,
)
from test_framework.script import (
CScript,
Expand Down Expand Up @@ -152,7 +152,7 @@ def _test_coin_stats_index(self):
tx1 = self.wallet.send_to(
from_node=node,
scriptPubKey=self.wallet.get_scriptPubKey(),
amount=21 * COIN,
amount=btcToSat(21),
)

# Find the right position of the 21 BTC output
Expand All @@ -161,7 +161,7 @@ def _test_coin_stats_index(self):
# Generate and send another tx with an OP_RETURN output (which is unspendable)
tx2 = self.wallet.create_self_transfer(utxo_to_spend=tx1_out_21)['tx']
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))]
Comment on lines 163 to +164
Copy link
Contributor

@rkrux rkrux Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@laanwj laanwj Dec 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tx2_hex = tx2.serialize().hex()
self.nodes[0].sendrawtransaction(tx2_hex, 0, tx2_val)

Expand Down Expand Up @@ -189,7 +189,7 @@ def _test_coin_stats_index(self):
# Create a coinbase that does not claim full subsidy and also
# has two outputs
cb = create_coinbase(109, nValue=35)
cb.vout.append(CTxOut(5 * COIN, CScript([OP_FALSE])))
cb.vout.append(CTxOut(btcToSat(5), CScript([OP_FALSE])))
cb.rehash()

# Generate a block that includes previous coinbase
Expand Down
4 changes: 2 additions & 2 deletions test/functional/feature_dbcrash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

from test_framework.blocktools import COINBASE_MATURITY
from test_framework.messages import (
COIN,
btcToSat,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
Expand Down Expand Up @@ -189,7 +189,7 @@ def generate_small_transactions(self, node, count, utxo_list):
random.shuffle(utxo_list)
while len(utxo_list) >= 2 and num_transactions < count:
utxos_to_spend = [utxo_list.pop() for _ in range(2)]
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]))
if input_amount < FEE:
# Sanity check -- if we chose inputs that are too small, skip
continue
Expand Down
10 changes: 6 additions & 4 deletions test/functional/feature_fee_estimation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

from test_framework.messages import (
COIN,
satToBtc,
btcToSat,
)
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
Expand Down Expand Up @@ -57,9 +59,9 @@ def small_txpuzzle_randfee(
utxos_to_spend=utxos_to_spend,
fee_per_output=0,
)["tx"]
tx.vout[0].nValue = int((total_in - amount - fee) * COIN)
tx.vout[0].nValue = btcToSat((total_in - amount - fee))
tx.vout.append(deepcopy(tx.vout[0]))
tx.vout[1].nValue = int(amount * COIN)
tx.vout[1].nValue = btcToSat(amount)
tx.rehash()
txid = tx.hash
tx_hex = tx.serialize().hex()
Expand Down Expand Up @@ -125,7 +127,7 @@ def make_tx(wallet, utxo, feerate):
"""Create a 1in-1out transaction with a specific input and feerate (sat/vb)."""
return wallet.create_self_transfer(
utxo_to_spend=utxo,
fee_rate=Decimal(feerate * 1000) / COIN,
fee_rate=satToBtc(feerate * 1000),
)

def check_fee_estimates_btw_modes(node, expected_conservative, expected_economical):
Expand Down Expand Up @@ -298,7 +300,7 @@ def sanity_check_rbf_estimates(self, utxos):

# Only 10% of the transactions were really confirmed with a low feerate,
# the rest needed to be RBF'd. We must return the 90% conf rate feerate.
high_feerate_kvb = Decimal(high_feerate) / COIN * 10 ** 3
high_feerate_kvb = satToBtc(high_feerate) * 10 ** 3
est_feerate = node.estimatesmartfee(2)["feerate"]
assert_equal(est_feerate, high_feerate_kvb)

Expand Down
Loading
Loading