test: refactor: overhaul block hash determination for `CBlock{,Header}` objects

theStack commented at 12:39 pm on July 3, 2025: contributor

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

DrahtBot added the label Tests on Jul 3, 2025

DrahtBot commented at 12:39 pm on July 3, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32868.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, maflcko, danielabrozzoni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32587 (test: Fix reorg patterns in tests to use proper fork-based approach by yuvicc)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#32180 (p2p: Advance pindexLastCommonBlock early after connecting blocks by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theStack force-pushed on Jul 3, 2025

danielabrozzoni commented at 2:51 pm on July 10, 2025: contributor

tACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40

Nice! Dropping calc_sha256 and rehash makes the code cleaner and less error-prone. I also like the new hash_int and hash_hex names, they’re easier to remember than the old hash/sha256, which always got me confused.

Thanks for separating the commits and adding TODOs in the intermediate ones, it really helped make the PR easier to follow.

I checked that the remaining .sha256 and .hash usages are unrelated. To quickly grep for .hash entries that aren’t .hash_int or .hash_hex, I used this command (it’s AI-generated, it looks good to me, but don’t trust it too much):

0git grep -P '\.hash(?!_(int|hex))\b' test/functional/

One small nit: I noticed that feature_assumeutxo.py defines a Block class with a hash member; it might be worth renaming it to hash_hex for consistency.

rkrux commented at 2:15 pm on July 15, 2025: contributor

utACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40

This is the followup to the similar CTransaction cleanup PR. As I realised in the previous PR, this is a significant improvement in code readability and helps in getting rid of sneaky bugs too.

DrahtBot added the label CI failed on Jul 16, 2025

DrahtBot commented at 5:13 am on July 17, 2025: contributor

0                                     File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/rpc_invalidateblock.py", line 106, in run_test
1[16:30:23.107]                                        self.nodes[0].reconsiderblock(block.hash)
2[16:30:23.107]                                    AttributeError: 'CBlock' object has no attribute 'hash'

maflcko commented at 7:23 am on July 17, 2025: member

very nice, but it would be good to rebase and fix the ci failure.

review ACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40 🏋

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40 🏋
31AdwqBpKUS0ThRi8ki58iQ/NLbuZpUQCF3X9UOUB1UkrnUJO6yhG7aKAqw2gbt4yARMmVXkKWtfJdnCesbpkBg==

test: refactor: dedup `CBlockHeader` serialization

Note that we can't call `.serialize()` directly in
the `.calc_sha256()` method, as this could wrongly lead
to the serialization of the derived class (CBlock) if
called from an instance there.

f3c791d2e3

test: avoid direct block header modification in feature_block.py

This is a preparatory commit for removing the header hash
caching in the CBlockHeader class. In order to not lose the
old block hash, necessary for updating the internal state of
the test (represented by `self.block_heights` and `self.blocks`),
we should only modify it within the `update_block` method.

0f044e82bd

test: remove header hash caching in CBlockHeader class

Rather than block hashes (represented by the fields `.sha256` and
`.hash`) being stateful, simply compute them on-the-fly. This ensures
that the correct values are always returned and takes the burden of
rehashing from test writers, making the code shorter overall.  In a
first step, the fields are kept at the same name with @property
functions as drop-in replacements, for a minimal diff. In later commits,
the names are changed to be more descriptive and indicating the return
type of the block hash.

0716382c20

test: remove bare CBlockHeader `.rehash()`/`.calc_sha256()` calls

Since the previous commit, CBlockHeader/CBlock object calls to the
methods `.rehash()` and `.calc_sha256()` are effectively no-ops
if the returned value is not used, so we can just remove them.

8b09cc350a

test: rename CBlockHeader `.rehash()`/`.sha256` -> `.hash_int` for consistency

Note that we unfortunately can't use a scripted diff here, as the
`sha256` symbol is also used for other instances (e.g. as function
in hashlib, or in the `UTXO` class in p2p_segwit.py).

23be0ec2f0

test: rename CBlockHeader `.hash` -> `.hash_hex` for consistency

Note that we unfortunately can't use a scripted diff here, as the
`.hash` symbol is also used for other instances (e.g. CInv).

2118301d77

test: avoid unneeded block header hash -> integer conversions 5fa34951ea

theStack force-pushed on Jul 17, 2025

theStack commented at 11:21 am on July 17, 2025: contributor

Thanks for the reviews! Rebased on master as suggested (to fix the silent merge conflict related to #32968).

rkrux commented at 12:03 pm on July 17, 2025: contributor

re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 modulo failing CI due to silent merge conflict.

maflcko removed the label CI failed on Jul 17, 2025

maflcko commented at 5:00 pm on July 17, 2025: member

all good now

re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 🎩

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 🎩
3R6OOF6mVpb8npwFpqx1dJ9oobRWV8TvIDcAFT1hp+ZWMiRsJ0bqrZGQu8bCTLCGIw2FX8Qn8xsUtsSEsLdnKBg==

DrahtBot requested review from danielabrozzoni on Jul 17, 2025

danielabrozzoni commented at 1:09 pm on July 18, 2025: contributor

reACK 5fa34951ead2eebcced919537f5e27526f61d909

fanquake merged this on Jul 18, 2025

fanquake closed this on Jul 18, 2025

theStack deleted the branch on Jul 18, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts

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