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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
theStack force-pushed on Jul 3, 2025
danielabrozzoni
commented at 2:51 PM on July 10, 2025:
contributor
tACK50b41bcfbfd93f103cf6e2af1999c43947bc3f40
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):
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
utACK50b41bcfbfd93f103cf6e2af1999c43947bc3f40
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
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/rpc_invalidateblock.py", line 106, in run_test
[16:30:23.107] self.nodes[0].reconsiderblock(block.hash)
[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.
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).
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-04-30 21:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
