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

a82829f test: simplify (w)txid checks by avoiding .calc_sha256 calls (Sebastian Falbesoner)
346a099 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 a82829f 🐘
  rkrux:
    Concept and utACK a82829f
  ryanofsky:
    Code review ACK a82829f. Nice changes, and sorry about the false bug report

Tree-SHA512: bb0465802d743a495207800f922b65f49ed0d20552f95bb0bee764944664092aad74812e29df6e01ef40bcb8f9bc6c84c7e9cbbe6f008ee1a14d94ed88e698b4

Author: ryanofsky

test/functional/feature_segwit.py

@@ -267,7 +267,7 @@ def run_test(self):
         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 @@ def run_test(self):
         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 @@ def run_test(self):
         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())

test/functional/p2p_compactblocks.py

@@ -347,13 +347,11 @@ def check_compactblock_construction_from_block(self, header_and_shortids, block_
 
         # 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 @@ def test_getblocktxn_handler(self, test_node):
                 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
 

test/functional/p2p_segwit.py

@@ -336,8 +336,7 @@ def test_unnecessary_witness_before_segwit_activation(self):
 
         # 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 @@ def test_tx_relay_after_segwit_activation(self):
         # 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)

test/functional/test_framework/blocktools.py

@@ -28,7 +28,6 @@
     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):

test/functional/test_framework/script.py

@@ -17,9 +17,7 @@
     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