Merge bitcoin/bitcoin#32868: test: refactor: overhaul block hash determination for CBlock{,Header} objects

5fa3495 test: avoid unneeded block header hash -> integer conversions (Sebastian Falbesoner)
2118301 test: rename CBlockHeader `.hash` -> `.hash_hex` for consistency (Sebastian Falbesoner)
23be0ec test: rename CBlockHeader `.rehash()`/`.sha256` -> `.hash_int` for consistency (Sebastian Falbesoner)
8b09cc3 test: remove bare CBlockHeader `.rehash()`/`.calc_sha256()` calls (Sebastian Falbesoner)
0716382 test: remove header hash caching in CBlockHeader class (Sebastian Falbesoner)
0f044e8 test: avoid direct block header modification in feature_block.py (Sebastian Falbesoner)
f3c791d test: refactor: dedup `CBlockHeader` serialization (Sebastian Falbesoner)

Pull request description:

  Similar to what #32421 did for `CTransaction` instances, this PR aims to improve the block hash determination of `CBlockHeader`/`CBlock` (the latter is a subclass of the former) instances by removing the block header caching mechanism and introducing consistent naming. Without the statefulness, sneaky testing bugs like #32742 and #32823 are less likely to happen in the future. Note that performance is even less of an issue here compared to `CTransaction`, as we only need to hash 80 bytes, which is less than typical standard transaction sizes [2].
  The only instance where the testing logic was relying on caching (i.e. we want to return an outdated value) is tackled in the second commit, the rest should be straight-forward to review, especially for contributors who already reviewed #32421.

  Summary table showing block hash determaination before/after this PR:
  | Task                               | master                   | PR           |
  |:-----------------------------------|:-------------------------|:-------------|
  | get block header hash (hex string) | `.hash`[1]               | `.hash_hex`  |
  | get block header hash (integer)    | `rehash()`, `.sha256`[1] | `.hash_int`  |

  [1] = returned value might be `None` or out-of-date, if rehashing function wasn't called after modification
  [2] = the only exception I could think of are transaction with pay-to-anchor (P2A) outputs

ACKs for top commit:
  rkrux:
    re-ACK 5fa3495 modulo failing CI due to silent merge conflict.
  maflcko:
    re-ACK 5fa3495 🎩
  danielabrozzoni:
    reACK 5fa3495

Tree-SHA512: 3d13540012654effa063846958a3166d56c1bcb58e1321f52ca4d5c3bcb7abdea72c54d1fb566d04e636d84d06a41d293e16232dbe5d5e78a73c903bb6ffc80d

Author: fanquake

contrib/signet/miner

@@ -95,7 +95,6 @@ def finish_block(block, signet_solution, grind_cmd):
         newheadhex = subprocess.run(cmd, stdout=subprocess.PIPE, input=b"", check=True).stdout.strip()
         newhead = from_hex(CBlockHeader(), newheadhex.decode('utf8'))
         block.nNonce = newhead.nNonce
-        block.rehash()
     return block
 
 def new_block(tmpl, reward_spk, *, blocktime=None, poolid=None):
@@ -482,15 +481,15 @@ def do_generate(args):
         # report
         bstr = "block" if gen.is_mine else "backup block"
 
-        next_delta = gen.next_block_delta(block.nBits, block.hash)
+        next_delta = gen.next_block_delta(block.nBits, block.hash_hex)
         next_delta += block.nTime - time.time()
-        next_is_mine = gen.next_block_is_mine(block.hash)
+        next_is_mine = gen.next_block_is_mine(block.hash_hex)
 
-        logging.debug("Block hash %s payout to %s", block.hash, reward_addr)
+        logging.debug("Block hash %s payout to %s", block.hash_hex, reward_addr)
         logging.info("Mined %s at height %d; next in %s (%s)", bstr, tmpl["height"], seconds_to_hms(next_delta), ("mine" if next_is_mine else "backup"))
         if r != "":
-            logging.warning("submitblock returned %s for height %d hash %s", r, tmpl["height"], block.hash)
-        lastheader = block.hash
+            logging.warning("submitblock returned %s for height %d hash %s", r, tmpl["height"], block.hash_hex)
+        lastheader = block.hash_hex
 
 def do_calibrate(args):
     if args.nbits is not None and args.seconds is not None:

test/functional/example_test.py

@@ -57,8 +57,7 @@ def on_block(self, message):
         """Override the standard on_block callback
 
         Store the hash of a received block in the dictionary."""
-        message.block.calc_sha256()
-        self.block_receive_map[message.block.sha256] += 1
+        self.block_receive_map[message.block.hash_int] += 1
 
     def on_inv(self, message):
         """Override the standard on_inv callback"""
@@ -182,7 +181,7 @@ def run_test(self):
             block_message = msg_block(block)
             # Send message is used to send a P2P message to the node over our P2PInterface
             peer_messaging.send_without_ping(block_message)
-            self.tip = block.sha256
+            self.tip = block.hash_int
             blocks.append(self.tip)
             self.block_time += 1
             height += 1

test/functional/feature_assumeutxo.py

@@ -272,7 +272,7 @@ def test_snapshot_not_on_most_work_chain(self, dump_output_path):
         block_time = node0.getblock(node0.getbestblockhash())['time'] + 1
         fork_block1 = create_block(int(parent_block_hash, 16), create_coinbase(SNAPSHOT_BASE_HEIGHT), block_time)
         fork_block1.solve()
-        fork_block2 = create_block(fork_block1.sha256, create_coinbase(SNAPSHOT_BASE_HEIGHT + 1), block_time + 1)
+        fork_block2 = create_block(fork_block1.hash_int, create_coinbase(SNAPSHOT_BASE_HEIGHT + 1), block_time + 1)
         fork_block2.solve()
         node1.submitheader(fork_block1.serialize().hex())
         node1.submitheader(fork_block2.serialize().hex())

test/functional/feature_assumevalid.py

@@ -103,15 +103,15 @@ def run_test(self):
         block.solve()
         # Save the coinbase for later
         self.block1 = block
-        self.tip = block.sha256
+        self.tip = block.hash_int
         height += 1
 
         # Bury the block 100 deep so the coinbase output is spendable
         for _ in range(100):
             block = create_block(self.tip, create_coinbase(height), self.block_time)
             block.solve()
             self.blocks.append(block)
-            self.tip = block.sha256
+            self.tip = block.hash_int
             self.block_time += 1
             height += 1
 
@@ -124,7 +124,7 @@ def run_test(self):
         self.block_time += 1
         block102.solve()
         self.blocks.append(block102)
-        self.tip = block102.sha256
+        self.tip = block102.hash_int
         self.block_time += 1
         height += 1
 
@@ -133,13 +133,13 @@ def run_test(self):
             block = create_block(self.tip, create_coinbase(height), self.block_time)
             block.solve()
             self.blocks.append(block)
-            self.tip = block.sha256
+            self.tip = block.hash_int
             self.block_time += 1
             height += 1
 
         # Start node1 and node2 with assumevalid so they accept a block with a bad signature.
-        self.start_node(1, extra_args=["-assumevalid=" + block102.hash])
-        self.start_node(2, extra_args=["-assumevalid=" + block102.hash])
+        self.start_node(1, extra_args=["-assumevalid=" + block102.hash_hex])
+        self.start_node(2, extra_args=["-assumevalid=" + block102.hash_hex])
 
         p2p0 = self.nodes[0].add_p2p_connection(BaseNode())
         p2p0.send_header_for_blocks(self.blocks[0:2000])

test/functional/feature_bip68_sequence.py

@@ -327,7 +327,7 @@ def test_nonzero_locks(orig_tx, node, relayfee, use_height_lock):
         for i in range(2):
             block = create_block(tmpl=tmpl, ntime=cur_time)
             block.solve()
-            tip = block.sha256
+            tip = block.hash_int
             assert_equal(None if i == 1 else 'inconclusive', self.nodes[0].submitblock(block.serialize().hex()))
             tmpl = self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
             tmpl['previousblockhash'] = '%x' % tip
@@ -383,7 +383,7 @@ def test_bip68_not_consensus(self):
         block.solve()
 
         assert_equal(None, self.nodes[0].submitblock(block.serialize().hex()))
-        assert_equal(self.nodes[0].getbestblockhash(), block.hash)
+        assert_equal(self.nodes[0].getbestblockhash(), block.hash_hex)
 
     def activateCSV(self):
         # activation should happen at block height 432 (3 periods)

test/functional/feature_block.py

@@ -280,7 +280,7 @@ def run_test(self):
         self.send_blocks([b12, b13, b14], success=False, reject_reason='bad-cb-amount', reconnect=True)
 
         # New tip should be b13.
-        assert_equal(node.getbestblockhash(), b13.hash)
+        assert_equal(node.getbestblockhash(), b13.hash_hex)
 
         # Add a block with MAX_BLOCK_SIGOPS and one with one more sigop
         #     genesis -> b1 (0) -> b2 (1) -> b5 (2) -> b6  (3)
@@ -609,32 +609,32 @@ def run_test(self):
         # The next few blocks are going to be created "by hand" since they'll do funky things, such as having
         # the first transaction be non-coinbase, etc.  The purpose of b44 is to make sure this works.
         self.log.info("Build block 44 manually")
-        height = self.block_heights[self.tip.sha256] + 1
+        height = self.block_heights[self.tip.hash_int] + 1
         coinbase = create_coinbase(height, self.coinbase_pubkey)
         b44 = CBlock()
         b44.nTime = self.tip.nTime + 1
-        b44.hashPrevBlock = self.tip.sha256
+        b44.hashPrevBlock = self.tip.hash_int
         b44.nBits = REGTEST_N_BITS
         b44.vtx.append(coinbase)
         tx = self.create_and_sign_transaction(out[14], 1)
         b44.vtx.append(tx)
         b44.hashMerkleRoot = b44.calc_merkle_root()
         b44.solve()
         self.tip = b44
-        self.block_heights[b44.sha256] = height
+        self.block_heights[b44.hash_int] = height
         self.blocks[44] = b44
         self.send_blocks([b44], True)
 
         self.log.info("Reject a block with a non-coinbase as the first tx")
         non_coinbase = self.create_tx(out[15], 0, 1)
         b45 = CBlock()
         b45.nTime = self.tip.nTime + 1
-        b45.hashPrevBlock = self.tip.sha256
+        b45.hashPrevBlock = self.tip.hash_int
         b45.nBits = REGTEST_N_BITS
         b45.vtx.append(non_coinbase)
         b45.hashMerkleRoot = b45.calc_merkle_root()
         b45.solve()
-        self.block_heights[b45.sha256] = self.block_heights[self.tip.sha256] + 1
+        self.block_heights[b45.hash_int] = self.block_heights[self.tip.hash_int] + 1
         self.tip = b45
         self.blocks[45] = b45
         self.send_blocks([b45], success=False, reject_reason='bad-cb-missing', reconnect=True)
@@ -643,12 +643,12 @@ def run_test(self):
         self.move_tip(44)
         b46 = CBlock()
         b46.nTime = b44.nTime + 1
-        b46.hashPrevBlock = b44.sha256
+        b46.hashPrevBlock = b44.hash_int
         b46.nBits = REGTEST_N_BITS
         b46.vtx = []
         b46.hashMerkleRoot = 0
         b46.solve()
-        self.block_heights[b46.sha256] = self.block_heights[b44.sha256] + 1
+        self.block_heights[b46.hash_int] = self.block_heights[b44.hash_int] + 1
         self.tip = b46
         assert 46 not in self.blocks
         self.blocks[46] = b46
@@ -658,10 +658,9 @@ def run_test(self):
         self.move_tip(44)
         b47 = self.next_block(47)
         target = uint256_from_compact(b47.nBits)
-        while b47.sha256 <= target:
+        while b47.hash_int <= target:
             # Rehash nonces until an invalid too-high-hash block is found.
             b47.nNonce += 1
-            b47.rehash()
         self.send_blocks([b47], False, force_send=True, reject_reason='high-hash', reconnect=True)
 
         self.log.info("Reject a block with a timestamp >2 hours in the future")
@@ -719,8 +718,7 @@ def run_test(self):
         # valid timestamp
         self.move_tip(53)
         b55 = self.next_block(55, spend=out[15])
-        b55.nTime = b35.nTime
-        self.update_block(55, [])
+        self.update_block(55, [], nTime=b35.nTime)
         self.send_blocks([b55], True)
         self.save_spendable_output()
 
@@ -733,10 +731,10 @@ def run_test(self):
         self.log.info("Accept a previously rejected future block at a later time")
         node.setmocktime(int(time.time()) + 2*60*60)
         self.move_tip(48)
-        self.block_heights[b48.sha256] = self.block_heights[b44.sha256] + 1 # b48 is a parent of b44
+        self.block_heights[b48.hash_int] = self.block_heights[b44.hash_int] + 1 # b48 is a parent of b44
         b48p = self.next_block("48p")
         self.send_blocks([b48, b48p], success=True) # Reorg to the longer chain
-        node.invalidateblock(b48p.hash) # mark b48p as invalid
+        node.invalidateblock(b48p.hash_hex) # mark b48p as invalid
         node.setmocktime(0)
 
         # Test Merkle tree malleability
@@ -780,7 +778,7 @@ def run_test(self):
         self.blocks[56] = b56
         assert_equal(len(b56.vtx), 3)
         b56 = self.update_block(56, [tx1])
-        assert_equal(b56.hash, b57.hash)
+        assert_equal(b56.hash_hex, b57.hash_hex)
         self.send_blocks([b56], success=False, reject_reason='bad-txns-duplicate', reconnect=True)
 
         # b57p2 - a good block with 6 tx'es, don't submit until end
@@ -798,7 +796,7 @@ def run_test(self):
         self.move_tip(55)
         b56p2 = copy.deepcopy(b57p2)
         self.blocks["b56p2"] = b56p2
-        assert_equal(b56p2.hash, b57p2.hash)
+        assert_equal(b56p2.hash_hex, b57p2.hash_hex)
         assert_equal(len(b56p2.vtx), 6)
         b56p2 = self.update_block("b56p2", [tx3, tx4])
         self.send_blocks([b56p2], success=False, reject_reason='bad-txns-duplicate', reconnect=True)
@@ -956,7 +954,7 @@ def run_test(self):
         self.move_tip('dup_2')
         b64 = CBlock(b64a)
         b64.vtx = copy.deepcopy(b64a.vtx)
-        assert_equal(b64.hash, b64a.hash)
+        assert_equal(b64.hash_hex, b64a.hash_hex)
         assert_equal(b64.get_weight(), MAX_BLOCK_WEIGHT)
         self.blocks[64] = b64
         b64 = self.update_block(64, [])
@@ -1060,12 +1058,12 @@ def run_test(self):
         b72 = self.update_block(72, [tx1, tx2])  # now tip is 72
         b71 = copy.deepcopy(b72)
         b71.vtx.append(tx2)   # add duplicate tx2
-        self.block_heights[b71.sha256] = self.block_heights[b69.sha256] + 1  # b71 builds off b69
+        self.block_heights[b71.hash_int] = self.block_heights[b69.hash_int] + 1  # b71 builds off b69
         self.blocks[71] = b71
 
         assert_equal(len(b71.vtx), 4)
         assert_equal(len(b72.vtx), 3)
-        assert_equal(b72.sha256, b71.sha256)
+        assert_equal(b72.hash_int, b71.hash_int)
 
         self.move_tip(71)
         self.send_blocks([b71], success=False, reject_reason='bad-txns-duplicate', reconnect=True)
@@ -1370,7 +1368,7 @@ def next_block(self, number, spend=None, additional_coinbase_value=0, *, script=
             base_block_hash = self.genesis_hash
             block_time = int(time.time()) + 1
         else:
-            base_block_hash = self.tip.sha256
+            base_block_hash = self.tip.hash_int
             block_time = self.tip.nTime + 1
         # First create the coinbase
         height = self.block_heights[base_block_hash] + 1
@@ -1388,7 +1386,7 @@ def next_block(self, number, spend=None, additional_coinbase_value=0, *, script=
         # Block is created. Find a valid nonce.
         block.solve()
         self.tip = block
-        self.block_heights[block.sha256] = height
+        self.block_heights[block.hash_int] = height
         assert number not in self.blocks
         self.blocks[number] = block
         return block
@@ -1408,17 +1406,19 @@ def move_tip(self, number):
         self.tip = self.blocks[number]
 
     # adds transactions to the block and updates state
-    def update_block(self, block_number, new_transactions):
+    def update_block(self, block_number, new_transactions, *, nTime=None):
         block = self.blocks[block_number]
         self.add_transactions_to_block(block, new_transactions)
-        old_sha256 = block.sha256
+        old_hash_int = block.hash_int
+        if nTime is not None:
+            block.nTime = nTime
         block.hashMerkleRoot = block.calc_merkle_root()
         block.solve()
         # Update the internal state just like in next_block
         self.tip = block
-        if block.sha256 != old_sha256:
-            self.block_heights[block.sha256] = self.block_heights[old_sha256]
-            del self.block_heights[old_sha256]
+        if block.hash_int != old_hash_int:
+            self.block_heights[block.hash_int] = self.block_heights[old_hash_int]
+            del self.block_heights[old_hash_int]
         self.blocks[block_number] = block
         return block
 

test/functional/feature_cltv.py

@@ -127,15 +127,15 @@ def run_test(self):
         self.test_cltv_info(is_active=False)  # Not active as of current tip and next block does not need to obey rules
         peer.send_and_ping(msg_block(block))
         self.test_cltv_info(is_active=True)  # Not active as of current tip, but next block must obey rules
-        assert_equal(self.nodes[0].getbestblockhash(), block.hash)
+        assert_equal(self.nodes[0].getbestblockhash(), block.hash_hex)
 
         self.log.info("Test that blocks must now be at least version 4")
-        tip = block.sha256
+        tip = block.hash_int
         block_time += 1
         block = create_block(tip, create_coinbase(CLTV_HEIGHT), block_time, version=3)
         block.solve()
 
-        with self.nodes[0].assert_debug_log(expected_msgs=[f'{block.hash}, bad-version(0x00000003)']):
+        with self.nodes[0].assert_debug_log(expected_msgs=[f'{block.hash_hex}, bad-version(0x00000003)']):
             peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
             peer.sync_with_ping()
@@ -196,7 +196,7 @@ def run_test(self):
         self.test_cltv_info(is_active=True)  # Not active as of current tip, but next block must obey rules
         peer.send_and_ping(msg_block(block))
         self.test_cltv_info(is_active=True)  # Active as of current tip
-        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
+        assert_equal(self.nodes[0].getbestblockhash(), block.hash_hex)
 
 
 if __name__ == '__main__':

test/functional/feature_csv_activation.py

@@ -165,7 +165,7 @@ def generate_blocks(self, number):
             block = self.create_test_block([])
             test_blocks.append(block)
             self.last_block_time += 600
-            self.tip = block.sha256
+            self.tip = block.hash_int
             self.tipheight += 1
         return test_blocks
 

test/functional/feature_dersig.py

@@ -92,15 +92,15 @@ def run_test(self):
         peer.send_and_ping(msg_block(block))
         assert_equal(self.nodes[0].getblockcount(), DERSIG_HEIGHT - 1)
         self.test_dersig_info(is_active=True)  # Not active as of current tip, but next block must obey rules
-        assert_equal(self.nodes[0].getbestblockhash(), block.hash)
+        assert_equal(self.nodes[0].getbestblockhash(), block.hash_hex)
 
         self.log.info("Test that blocks must now be at least version 3")
-        tip = block.sha256
+        tip = block.hash_int
         block_time += 1
         block = create_block(tip, create_coinbase(DERSIG_HEIGHT), block_time, version=2)
         block.solve()
 
-        with self.nodes[0].assert_debug_log(expected_msgs=[f'{block.hash}, bad-version(0x00000002)']):
+        with self.nodes[0].assert_debug_log(expected_msgs=[f'{block.hash_hex}, bad-version(0x00000002)']):
             peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
             peer.sync_with_ping()
@@ -146,7 +146,7 @@ def run_test(self):
         self.test_dersig_info(is_active=True)  # Not active as of current tip, but next block must obey rules
         peer.send_and_ping(msg_block(block))
         self.test_dersig_info(is_active=True)  # Active as of current tip
-        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
+        assert_equal(self.nodes[0].getbestblockhash(), block.hash_hex)
 
 
 if __name__ == '__main__':

test/functional/feature_maxuploadtarget.py

@@ -41,8 +41,7 @@ def on_inv(self, message):
         pass
 
     def on_block(self, message):
-        message.block.calc_sha256()
-        self.block_receive_map[message.block.sha256] += 1
+        self.block_receive_map[message.block.hash_int] += 1
 
 class MaxUploadTest(BitcoinTestFramework):