Merge bitcoin/bitcoin#32421: test: refactor: overhaul (w)txid determination for CTransaction objects

4ef6253 test: avoid unneeded (w)txid hex -> integer conversions (Sebastian Falbesoner)
472f377 scripted-diff: test: rename CTransaction `.getwtxid()` -> `wtxid_hex` for consistency (Sebastian Falbesoner)
81af433 test: rename CTransaction `.sha256` -> `.txid_int` for consistency (Sebastian Falbesoner)
ce83924 test: rename CTransaction `.rehash()`/`.hash` -> `.txid_hex` for consistency (Sebastian Falbesoner)
e9cdaef test: introduce and use CTransaction `.wtxid_int` property (Sebastian Falbesoner)
9b3dce2 test: remove bare CTransaction `.rehash()`/`.calc_sha256()` calls (Sebastian Falbesoner)
a2724e3 test: remove txid caching in CTransaction class (Sebastian Falbesoner)

Pull request description:

  In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
  * removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and bitcoin/bitcoin#32050 (comment)). This change in theory decreases the performance, as the involved serialization and hashing involved might be called more often than previously, but I couldn't find a functional test where this leads to a measurable run-time increase on my machine.
  * introduce consistent naming that shows the type of the returned txid, i.e. hex string vs. test-framework-internal representation [currently integers] (see remaining commits)

  Summary table showing (w)txid determaination before/after this PR:

  | Task                   | master                 | PR           |
  |:-----------------------|:-----------------------|:-------------|
  | get TXID (hex string)  | `.rehash()` / `.hash`[1] | `.txid_hex`  |
  | get TXID (integer)     | `.sha256`[1]             | `.txid_int`  |
  | get WTXID (hex string) | `.getwtxid()`          | `.wtxid_hex` |
  | get WTXID (integer)    | `.calc_sha256(True)`   | `.wtxid_int` |

  Unfortunately, most renames can't be done with a scripted-diff, as the property names (`.hash`, `.sha256`) are also used for blocks and other message types. The PR is rather invasive and touches a lot of files, but I think it's worth to do it, also to make life easier for new contributors. Future tasks like e.g. doing the same overhaul for block (header) objects or getting rid of the integer representation (see bitcoin/bitcoin#32050) become easier should become easier after this one.

  [1] = returned value might be out-of-date, if rehashing function wasn't called after modification

ACKs for top commit:
  maflcko:
    re-ACK 4ef6253 🏈
  achow101:
    ACK 4ef6253
  marcofleon:
    code review ACK 4ef6253

Tree-SHA512: 4b472c31d169966b6f6878911a8404d25bf3e503b6e8ef30f36a7415d21ad4bc1265083af2d3ead6edfcd9fac9ccb0a8be57e1b0739ad431b836413070d7d583

Author: achow101

contrib/signet/miner

@@ -40,8 +40,7 @@ def signet_txs(block, challenge):
     txs[0].vout[-1].scriptPubKey += CScriptOp.encode_op_pushdata(SIGNET_HEADER)
     hashes = []
     for tx in txs:
-        tx.rehash()
-        hashes.append(ser_uint256(tx.sha256))
+        hashes.append(ser_uint256(tx.txid_int))
     mroot = block.get_merkle_root(hashes)
 
     sd = b""
@@ -55,12 +54,11 @@ def signet_txs(block, challenge):
     to_spend.nLockTime = 0
     to_spend.vin = [CTxIn(COutPoint(0, 0xFFFFFFFF), b"\x00" + CScriptOp.encode_op_pushdata(sd), 0)]
     to_spend.vout = [CTxOut(0, challenge)]
-    to_spend.rehash()
 
     spend = CTransaction()
     spend.version = 0
     spend.nLockTime = 0
-    spend.vin = [CTxIn(COutPoint(to_spend.sha256, 0), b"", 0)]
+    spend.vin = [CTxIn(COutPoint(to_spend.txid_int, 0), b"", 0)]
     spend.vout = [CTxOut(0, b"\x6a")]
 
     return spend, to_spend
@@ -88,7 +86,6 @@ def finish_block(block, signet_solution, grind_cmd):
         pass # Don't need to add a signet commitment if there's no signet signature needed
     else:
         block.vtx[0].vout[-1].scriptPubKey += CScriptOp.encode_op_pushdata(SIGNET_HEADER + signet_solution)
-        block.vtx[0].rehash()
         block.hashMerkleRoot = block.calc_merkle_root()
     if grind_cmd is None:
         block.solve()
@@ -111,7 +108,6 @@ def new_block(tmpl, reward_spk, *, blocktime=None, poolid=None):
     cbtx.vin = [CTxIn(COutPoint(0, 0xffffffff), scriptSig, MAX_SEQUENCE_NONFINAL)]
     cbtx.vout = [CTxOut(tmpl["coinbasevalue"], reward_spk)]
     cbtx.vin[0].nSequence = 2**32-2
-    cbtx.rehash()
 
     block = CBlock()
     block.nVersion = tmpl["version"]
@@ -130,7 +126,6 @@ def new_block(tmpl, reward_spk, *, blocktime=None, poolid=None):
     block.vtx[0].wit.vtxinwit = [cbwit]
     block.vtx[0].vout.append(CTxOut(0, bytes(get_witness_script(witroot, witnonce))))
 
-    block.vtx[0].rehash()
     block.hashMerkleRoot = block.calc_merkle_root()
 
     return block

test/functional/data/invalid_txs.py

@@ -83,7 +83,7 @@ class BadTxTemplate:
     def __init__(self, *, spend_tx=None, spend_block=None):
         self.spend_tx = spend_block.vtx[0] if spend_block else spend_tx
         self.spend_avail = sum(o.nValue for o in self.spend_tx.vout)
-        self.valid_txin = CTxIn(COutPoint(self.spend_tx.sha256, 0), b"", SEQUENCE_FINAL)
+        self.valid_txin = CTxIn(COutPoint(self.spend_tx.txid_int, 0), b"", SEQUENCE_FINAL)
 
     @abc.abstractmethod
     def get_tx(self, *args, **kwargs):
@@ -98,7 +98,6 @@ class OutputMissing(BadTxTemplate):
     def get_tx(self):
         tx = CTransaction()
         tx.vin.append(self.valid_txin)
-        tx.calc_sha256()
         return tx
 
 
@@ -113,7 +112,6 @@ class InputMissing(BadTxTemplate):
     # rather than the input count check.
     def get_tx(self):
         tx = CTransaction()
-        tx.calc_sha256()
         return tx
 
 
@@ -130,7 +128,6 @@ def get_tx(self):
         tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 2)))))
         assert len(tx.serialize_without_witness()) == 64
         assert MIN_STANDARD_TX_NONWITNESS_SIZE - 1 == 64
-        tx.calc_sha256()
         return tx
 
 
@@ -147,9 +144,8 @@ def get_tx(self):
         bad_idx = num_indices + 100
 
         tx = CTransaction()
-        tx.vin.append(CTxIn(COutPoint(self.spend_tx.sha256, bad_idx), b"", SEQUENCE_FINAL))
+        tx.vin.append(CTxIn(COutPoint(self.spend_tx.txid_int, bad_idx), b"", SEQUENCE_FINAL))
         tx.vout.append(CTxOut(0, basic_p2sh))
-        tx.calc_sha256()
         return tx
 
 
@@ -162,7 +158,6 @@ def get_tx(self):
         tx.vin.append(self.valid_txin)
         tx.vin.append(self.valid_txin)
         tx.vout.append(CTxOut(1, basic_p2sh))
-        tx.calc_sha256()
         return tx
 
 
@@ -175,7 +170,6 @@ def get_tx(self):
         tx.vin.append(self.valid_txin)
         tx.vin.append(CTxIn(COutPoint(hash=0, n=0xffffffff)))
         tx.vout.append(CTxOut(1, basic_p2sh))
-        tx.calc_sha256()
         return tx
 
 
@@ -187,10 +181,9 @@ class NonexistentInput(BadTxTemplate):
 
     def get_tx(self):
         tx = CTransaction()
-        tx.vin.append(CTxIn(COutPoint(self.spend_tx.sha256 + 1, 0), b"", SEQUENCE_FINAL))
+        tx.vin.append(CTxIn(COutPoint(self.spend_tx.txid_int + 1, 0), b"", SEQUENCE_FINAL))
         tx.vin.append(self.valid_txin)
         tx.vout.append(CTxOut(1, basic_p2sh))
-        tx.calc_sha256()
         return tx
 
 
@@ -226,7 +219,6 @@ class CreateSumTooLarge(BadTxTemplate):
     def get_tx(self):
         tx = create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY)
         tx.vout = [tx.vout[0]] * 2
-        tx.calc_sha256()
         return tx
 
 
@@ -274,7 +266,6 @@ def get_tx(self):
         vin.scriptSig = CScript([opcode])
         tx.vin.append(vin)
         tx.vout.append(CTxOut(1, basic_p2sh))
-        tx.calc_sha256()
         return tx
 
     return type('DisabledOpcode_' + str(opcode), (BadTxTemplate,), {

test/functional/feature_assumeutxo.py

@@ -605,7 +605,7 @@ def check_tx_counts(final: bool) -> None:
         privkey = n0.get_deterministic_priv_key().key
         raw_tx = n1.createrawtransaction([prevout], {getnewdestination()[2]: 24.99})
         signed_tx = n1.signrawtransactionwithkey(raw_tx, [privkey], [prevout])['hex']
-        signed_txid = tx_from_hex(signed_tx).rehash()
+        signed_txid = tx_from_hex(signed_tx).txid_hex
 
         assert n1.gettxout(prev_tx['txid'], 0) is not None
         n1.sendrawtransaction(signed_tx)

test/functional/feature_assumevalid.py

@@ -117,9 +117,8 @@ def run_test(self):
 
         # Create a transaction spending the coinbase output with an invalid (null) signature
         tx = CTransaction()
-        tx.vin.append(CTxIn(COutPoint(self.block1.vtx[0].sha256, 0), scriptSig=b""))
+        tx.vin.append(CTxIn(COutPoint(self.block1.vtx[0].txid_int, 0), scriptSig=b""))
         tx.vout.append(CTxOut(49 * 100000000, CScript([OP_TRUE])))
-        tx.calc_sha256()
 
         block102 = create_block(self.tip, create_coinbase(height), self.block_time, txlist=[tx])
         self.block_time += 1

test/functional/feature_bip68_sequence.py

@@ -110,7 +110,6 @@ def test_disable_flag(self):
         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.rehash()
 
         assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx2.serialize().hex())
 
@@ -220,17 +219,15 @@ def test_sequence_lock_unconfirmed_inputs(self):
         # Create a mempool tx.
         self.wallet.rescan_utxos()
         tx1 = self.wallet.send_self_transfer(from_node=self.nodes[0])["tx"]
-        tx1.rehash()
 
         # Anyone-can-spend mempool tx.
         # Sequence lock of 0 should pass.
         tx2 = CTransaction()
         tx2.version = 2
-        tx2.vin = [CTxIn(COutPoint(tx1.sha256, 0), nSequence=0)]
+        tx2.vin = [CTxIn(COutPoint(tx1.txid_int, 0), nSequence=0)]
         tx2.vout = [CTxOut(int(tx1.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
         self.wallet.sign_tx(tx=tx2)
         tx2_raw = tx2.serialize().hex()
-        tx2.rehash()
 
         self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx2_raw)
 
@@ -244,13 +241,12 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
 
             tx = CTransaction()
             tx.version = 2
-            tx.vin = [CTxIn(COutPoint(orig_tx.sha256, 0), nSequence=sequence_value)]
+            tx.vin = [CTxIn(COutPoint(orig_tx.txid_int, 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.rehash()
 
-            if (orig_tx.hash in node.getrawmempool()):
+            if (orig_tx.txid_hex in node.getrawmempool()):
                 # sendrawtransaction should fail if the tx is in the mempool
                 assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=node, tx_hex=tx.serialize().hex())
             else:
@@ -264,43 +260,43 @@ 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.txid_hex, fee_delta=int(-self.relayfee*COIN))
         cur_time = int(time.time())
         for _ in range(10):
             self.nodes[0].setmocktime(cur_time + 600)
             self.generate(self.wallet, 1, sync_fun=self.no_op)
             cur_time += 600
 
-        assert tx2.hash in self.nodes[0].getrawmempool()
+        assert tx2.txid_hex in self.nodes[0].getrawmempool()
 
         test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=True)
         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.txid_hex, fee_delta=int(self.relayfee*COIN))
 
         # Advance the time on the node so that we can test timelocks
         self.nodes[0].setmocktime(cur_time+600)
         # Save block template now to use for the reorg later
         tmpl = self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
         self.generate(self.nodes[0], 1)
-        assert tx2.hash not in self.nodes[0].getrawmempool()
+        assert tx2.txid_hex not in self.nodes[0].getrawmempool()
 
         # Now that tx2 is not in the mempool, a sequence locked spend should
         # succeed
         tx3 = test_nonzero_locks(tx2, self.nodes[0], self.relayfee, use_height_lock=False)
-        assert tx3.hash in self.nodes[0].getrawmempool()
+        assert tx3.txid_hex in self.nodes[0].getrawmempool()
 
         self.generate(self.nodes[0], 1)
-        assert tx3.hash not in self.nodes[0].getrawmempool()
+        assert tx3.txid_hex not in self.nodes[0].getrawmempool()
 
         # One more test, this time using height locks
         tx4 = test_nonzero_locks(tx3, self.nodes[0], self.relayfee, use_height_lock=True)
-        assert tx4.hash in self.nodes[0].getrawmempool()
+        assert tx4.txid_hex in self.nodes[0].getrawmempool()
 
         # Now try combining confirmed and unconfirmed inputs
         tx5 = test_nonzero_locks(tx4, self.nodes[0], self.relayfee, use_height_lock=True)
-        assert tx5.hash not in self.nodes[0].getrawmempool()
+        assert tx5.txid_hex not in self.nodes[0].getrawmempool()
 
         utxo = self.wallet.get_utxo()
         tx5.vin.append(CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), nSequence=1))
@@ -319,8 +315,8 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
         # If we invalidate the tip, tx3 should get added to the mempool, causing
         # tx4 to be removed (fails sequence-lock).
         self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
-        assert tx4.hash not in self.nodes[0].getrawmempool()
-        assert tx3.hash in self.nodes[0].getrawmempool()
+        assert tx4.txid_hex not in self.nodes[0].getrawmempool()
+        assert tx3.txid_hex in self.nodes[0].getrawmempool()
 
         # Now mine 2 empty blocks to reorg out the current tip (labeled tip-1 in
         # diagram above).
@@ -337,8 +333,8 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
             cur_time += 1
 
         mempool = self.nodes[0].getrawmempool()
-        assert tx3.hash not in mempool
-        assert tx2.hash in mempool
+        assert tx3.txid_hex not in mempool
+        assert tx2.txid_hex in mempool
 
         # Reset the chain and get rid of the mocktimed-blocks
         self.nodes[0].setmocktime(0)
@@ -353,19 +349,17 @@ def test_bip68_not_consensus(self):
         assert not softfork_active(self.nodes[0], 'csv')
 
         tx1 = self.wallet.send_self_transfer(from_node=self.nodes[0])["tx"]
-        tx1.rehash()
 
         # Make an anyone-can-spend transaction
         tx2 = CTransaction()
         tx2.version = 1
-        tx2.vin = [CTxIn(COutPoint(tx1.sha256, 0), nSequence=0)]
+        tx2.vin = [CTxIn(COutPoint(tx1.txid_int, 0), nSequence=0)]
         tx2.vout = [CTxOut(int(tx1.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
 
         # sign tx2
         self.wallet.sign_tx(tx=tx2)
         tx2_raw = tx2.serialize().hex()
         tx2 = tx_from_hex(tx2_raw)
-        tx2.rehash()
 
         self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=tx2_raw)
 
@@ -374,11 +368,10 @@ def test_bip68_not_consensus(self):
 
         tx3 = CTransaction()
         tx3.version = 2
-        tx3.vin = [CTxIn(COutPoint(tx2.sha256, 0), nSequence=sequence_value)]
+        tx3.vin = [CTxIn(COutPoint(tx2.txid_int, 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.rehash()
 
         assert_raises_rpc_error(-26, NOT_FINAL_ERROR, self.wallet.sendrawtransaction, from_node=self.nodes[0], tx_hex=tx3.serialize().hex())