Retropex/bitcoin
1
0
Fork 0
mirror of https://github.com/Retropex/bitcoin.git synced 2025-05-12 19:20:42 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#32050: test: avoid treating hash results as integers

Browse Source 
a82829f37e test: simplify (w)txid checks by avoiding .calc_sha256 calls (Sebastian Falbesoner)
346a099fc1 test: avoid unneeded hash -> uint256 -> hash roundtrips (Sebastian Falbesoner)

Pull request description:

  In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and there is usually no need to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC-context for e.g. key derivation.

  I'd hence argue that most uses of `ser_uint256`/`uint256_from_str` and txid conversions via `int(txid/blockhash, 16)` are potential code smells and should be reduced to a minimum long-term if possible. This PR is a first step into this direction, intentionally kept small with (what I think) uncontroversial changes for demonstration purposes, to check out if other contributors are interested in this. A next step could be to change the classes of primitives (CTransaction, CBlock etc.) and network messages (msg_) to store hash results as actual bytes (maybe in a class wrapping the bytes that offers conversion from/to human-readable strings [1], for easier interaction with RPC calls and debug outputs) rather than ints. But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.

  [1] unfortunately, txids and block hashes are shown to user in reverse byte order, so e.g. a txid_bytes->txid_str conversion is not just a simple `txid_bytes.hex()`, but a `txid_bytes[::-1].hex()`

ACKs for top commit:
  maflcko:
    review ACK a82829f37e 🐘
  rkrux:
    Concept and utACK a82829f37e
  ryanofsky:
    Code review ACK a82829f37e. Nice changes, and sorry about the false bug report

Tree-SHA512: bb0465802d743a495207800f922b65f49ed0d20552f95bb0bee764944664092aad74812e29df6e01ef40bcb8f9bc6c84c7e9cbbe6f008ee1a14d94ed88e698b4
This commit is contained in:
Ryan Ofsky 2025-03-27 10:00:51 -04:00
commit bcb316bd88
No known key found for this signature in database
GPG Key ID: 46800E30FC748A66
5 changed files with 22 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
test/functional/feature_segwit.py
View File

@ -267,7 +267,7 @@ class SegWitTest(BitcoinTestFramework):
tx1 = tx_from_hex(tx1_hex)
# Check that wtxid is properly reported in mempool entry (txid1)
assert_equal(int(self.nodes[0].getmempoolentry(txid1)["wtxid"], 16), tx1.calc_sha256(True))
assert_equal(self.nodes[0].getmempoolentry(txid1)["wtxid"], tx1.getwtxid())
# Check that weight and vsize are properly reported in mempool entry (txid1)
assert_equal(self.nodes[0].getmempoolentry(txid1)["vsize"], tx1.get_vsize())
@ -283,7 +283,7 @@ class SegWitTest(BitcoinTestFramework):
assert not tx.wit.is_null()
# Check that wtxid is properly reported in mempool entry (txid2)
assert_equal(int(self.nodes[0].getmempoolentry(txid2)["wtxid"], 16), tx.calc_sha256(True))
assert_equal(self.nodes[0].getmempoolentry(txid2)["wtxid"], tx.getwtxid())
# Check that weight and vsize are properly reported in mempool entry (txid2)
assert_equal(self.nodes[0].getmempoolentry(txid2)["vsize"], tx.get_vsize())
@ -306,7 +306,7 @@ class SegWitTest(BitcoinTestFramework):
assert txid3 in template_txids
# Check that wtxid is properly reported in mempool entry (txid3)
assert_equal(int(self.nodes[0].getmempoolentry(txid3)["wtxid"], 16), tx.calc_sha256(True))
assert_equal(self.nodes[0].getmempoolentry(txid3)["wtxid"], tx.getwtxid())
# Check that weight and vsize are properly reported in mempool entry (txid3)
assert_equal(self.nodes[0].getmempoolentry(txid3)["vsize"], tx.get_vsize())

11
test/functional/p2p_compactblocks.py
View File

@ -347,13 +347,11 @@ class CompactBlocksTest(BitcoinTestFramework):
# Check that all prefilled_txn entries match what's in the block.
for entry in header_and_shortids.prefilled_txn:
entry.tx.calc_sha256()
# This checks the non-witness parts of the tx agree
assert_equal(entry.tx.sha256, block.vtx[entry.index].sha256)
assert_equal(entry.tx.rehash(), block.vtx[entry.index].rehash())
# And this checks the witness
wtxid = entry.tx.calc_sha256(True)
assert_equal(wtxid, block.vtx[entry.index].calc_sha256(True))
assert_equal(entry.tx.getwtxid(), block.vtx[entry.index].getwtxid())
# Check that the cmpctblock message announced all the transactions.
assert_equal(len(header_and_shortids.prefilled_txn) + len(header_and_shortids.shortids), len(block.vtx))
@ -590,10 +588,9 @@ class CompactBlocksTest(BitcoinTestFramework):
all_indices = msg.block_txn_request.to_absolute()
for index in all_indices:
tx = test_node.last_message["blocktxn"].block_transactions.transactions.pop(0)
tx.calc_sha256()
assert_equal(tx.sha256, block.vtx[index].sha256)
assert_equal(tx.rehash(), block.vtx[index].rehash())
# Check that the witness matches
assert_equal(tx.calc_sha256(True), block.vtx[index].calc_sha256(True))
assert_equal(tx.getwtxid(), block.vtx[index].getwtxid())
test_node.last_message.pop("blocktxn", None)
current_height -= 1

5
test/functional/p2p_segwit.py
View File

@ -336,8 +336,7 @@ class SegWitTest(BitcoinTestFramework):
# Verify the hash with witness differs from the txid
# (otherwise our testing framework must be broken!)
tx.rehash()
assert tx.sha256 != tx.calc_sha256(with_witness=True)
assert tx.rehash() != tx.getwtxid()
# Construct a block that includes the transaction.
block = self.build_next_block()
@ -1293,7 +1292,7 @@ class SegWitTest(BitcoinTestFramework):
# Test that getrawtransaction returns correct witness information
# hash, size, vsize
raw_tx = self.nodes[0].getrawtransaction(tx3.hash, 1)
assert_equal(int(raw_tx["hash"], 16), tx3.calc_sha256(True))
assert_equal(raw_tx["hash"], tx3.getwtxid())
assert_equal(raw_tx["size"], len(tx3.serialize_with_witness()))
vsize = tx3.get_vsize()
assert_equal(raw_tx["vsize"], vsize)

5
test/functional/test_framework/blocktools.py
View File

@ -28,7 +28,6 @@ from .messages import (
ser_uint256,
tx_from_hex,
uint256_from_compact,
uint256_from_str,
WITNESS_SCALE_FACTOR,
)
from .script import (
@ -111,8 +110,8 @@ def create_block(hashprev=None, coinbase=None, ntime=None, *, version=None, tmpl
return block
def get_witness_script(witness_root, witness_nonce):
witness_commitment = uint256_from_str(hash256(ser_uint256(witness_root) + ser_uint256(witness_nonce)))
output_data = WITNESS_COMMITMENT_HEADER + ser_uint256(witness_commitment)
witness_commitment = hash256(ser_uint256(witness_root) + ser_uint256(witness_nonce))
output_data = WITNESS_COMMITMENT_HEADER + witness_commitment
return CScript([OP_RETURN, output_data])
def add_witness_commitment(block, nonce=0):

23
test/functional/test_framework/script.py
View File

@ -17,9 +17,7 @@ from .messages import (
CTxOut,
hash256,
ser_string,
ser_uint256,
sha256,
uint256_from_str,
)
from .crypto.ripemd160 import ripemd160
@ -711,41 +709,42 @@ def sign_input_segwitv0(tx, input_index, input_scriptpubkey, input_amount, privk
# Note that this corresponds to sigversion == 1 in EvalScript, which is used
# for version 0 witnesses.
def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
ZERO_HASH = bytes([0]*32)
hashPrevouts = 0
hashSequence = 0
hashOutputs = 0
hashPrevouts = ZERO_HASH
hashSequence = ZERO_HASH
hashOutputs = ZERO_HASH
if not (hashtype & SIGHASH_ANYONECANPAY):
serialize_prevouts = bytes()
for i in txTo.vin:
serialize_prevouts += i.prevout.serialize()
hashPrevouts = uint256_from_str(hash256(serialize_prevouts))
hashPrevouts = hash256(serialize_prevouts)
if (not (hashtype & SIGHASH_ANYONECANPAY) and (hashtype & 0x1f) != SIGHASH_SINGLE and (hashtype & 0x1f) != SIGHASH_NONE):
serialize_sequence = bytes()
for i in txTo.vin:
serialize_sequence += i.nSequence.to_bytes(4, "little")
hashSequence = uint256_from_str(hash256(serialize_sequence))
hashSequence = hash256(serialize_sequence)
if ((hashtype & 0x1f) != SIGHASH_SINGLE and (hashtype & 0x1f) != SIGHASH_NONE):
serialize_outputs = bytes()
for o in txTo.vout:
serialize_outputs += o.serialize()
hashOutputs = uint256_from_str(hash256(serialize_outputs))
hashOutputs = hash256(serialize_outputs)
elif ((hashtype & 0x1f) == SIGHASH_SINGLE and inIdx < len(txTo.vout)):
serialize_outputs = txTo.vout[inIdx].serialize()
hashOutputs = uint256_from_str(hash256(serialize_outputs))
hashOutputs = hash256(serialize_outputs)
ss = bytes()
ss += txTo.version.to_bytes(4, "little")
ss += ser_uint256(hashPrevouts)
ss += ser_uint256(hashSequence)
ss += hashPrevouts
ss += hashSequence
ss += txTo.vin[inIdx].prevout.serialize()
ss += ser_string(script)
ss += amount.to_bytes(8, "little", signed=True)
ss += txTo.vin[inIdx].nSequence.to_bytes(4, "little")
ss += ser_uint256(hashOutputs)
ss += hashOutputs
ss += txTo.nLockTime.to_bytes(4, "little")
ss += hashtype.to_bytes(4, "little")
return ss