Eliminate one block header hash calculation per block-read by reusing the hash for:
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32487.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
1035 | const FlatFilePos block_pos{WITH_LOCK(cs_main, return index.GetBlockPos())};
1036 | -
1037 | - if (!ReadBlock(block, block_pos)) {
1038 | - return false;
1039 | - }
1040 | - if (block.GetHash() != index.GetBlockHash()) {
Is there really a scenario where we could be reading a block, but not have its block index entry? Maybe this should just be an Assume instead.
We're not always checking this, only when the index is available. But after this change this check is for free, no need to recalculate the header's hash anymore.
Is there really a scenario where we could be reading a block, but not have its block index entry?
I can't see any, but this would require some more refactors
I can't see any
Most usages have a hash available, but I'm not sure about e.g. LoadExternalBlockFile - but even if I would, I'd prefer tackling it separately, I'm not comfortable adding new exceptions to consensus code.
re: #32487 (review)
I don't think I understand suggestion to use Assume. But if the hash is always available I guess expected_hash could be a required parameter instead of an optional one. Would seem fine to expand the PR, or just keep it focused on not calculating the hash twice.
Code review ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851. Nice new test coverage and simple optimization
1021 | @@ -1019,21 +1022,18 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const 1022 | return false; 1023 | } 1024 | 1025 | + if (expected_hash && block_hash != *expected_hash) { 1026 | + LogError("GetHash() doesn't match index at %s while reading block", pos.ToString());
This error log change no longer provides info about the index that doesn't match.
Examples from the unit test log:
$ ./build/bin/test_bitcoin --run_test=blockmanager_tests -l test_suite -- DEBUG_LOG_OUT
before
025-05-23T18:42:55.808136Z (mocktime: 2020-08-31T15:34:12Z) [test] [node/blockstorage.cpp:1033] [ReadBlock] [error] GetHash() doesn't match index for CBlockIndex(pprev=0x6000019085e0, nHeight=100, merkle=851b8e14be513191d6dd7df5ac3c997c2b0b1f58c99143905eac61b296d01169, hashBlock=0000000000000000000000000000000000000000000000000000000000000001) at FlatFilePos(nFile=0, nPos=27015) while reading block
after
2025-05-23T18:44:21.481359Z (mocktime: 2020-08-31T15:34:12Z) [test] [node/blockstorage.cpp:1026] [ReadBlock] [error] GetHash() doesn't match index at FlatFilePos(nFile=0, nPos=27015) while reading block
(Thank you for adding the unit test!)
Yes, there is no index available, only the desired hash, wherever it comes from. Would it help if we included the two different hashes in the error?
Would it help if we included the two different hashes in the error?
Sure, why not, since we have them available.
I'm planning a follow-up to this, will do it there
Decided to do it here instead:
[ReadBlock] [error] GetHash() doesn't match index at FlatFilePos(nFile=0, nPos=8) while reading block (000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f != 0000000000000000000000000000000000000000000000000000000000000001) test/blockmanager_tests.cpp:147: info: check !m_node.chainman->m_blockman.ReadBlock(dummy, *fake_index) has passed
Concept ACK
ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851
I guess checking the hash could also protect against some weird file corruption, where a different block occupies the same position? Could this happen if e.g. the files are renamed?
136 | @@ -137,6 +137,16 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) 137 | BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block)); 138 | } 139 | 140 | +BOOST_FIXTURE_TEST_CASE(blockmanager_readblock_hash_mismatch, TestChain100Setup)
nit: Can drop the 100 blocks and just use TestingSetup to half the duration of the test.
Will do it in a follow up, thanks
Did it here instead - thanks
review ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851 🌋
<details><summary>Show signature</summary>
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851 🌋
12TE8ZKu4avgseXLh/q3bD12L6JUoKwRM4JRSVJomoSPCLwU7x9A91dE0TdOMtRbkIOg6/qKppZ6YA6uZ8jIDQ==
</details>
LGTM, perhaps take some of the review suggestions if you're willing
Ensure `ReadBlock` rejects a block when the tip’s `phashBlock` differs from the expected hash.
Eliminate one SHA‑256 double‑hash computation of the header per block read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
lgtm ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
1021 | @@ -1019,21 +1022,19 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const 1022 | return false; 1023 | } 1024 | 1025 | + if (expected_hash && block_hash != *expected_hash) { 1026 | + LogError("GetHash() doesn't match index at %s while reading block (%s != %s)",
If you retouch now or in another pull, would be clearer to state which blockhash is expected vs actual.
LogError("GetHash() doesn't match index at %s while reading block (actual %s != expected %s)",
I didn't want to make it that verbose, people can check the source code if they see a failure here :)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Is there a reason we don't just cache the hash as a member of CBlockHeader and compute-or-return that value in GetHash()?
@pinheadmz, that's the first thing I though of, but unfortunately the header is as mutable as it gets (see e.g. while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce;), so it would be dangerous to cache the hash, given that all fields are accessible from the outside and there's no simple way to invalidate the pre-computed hash, unless we expose them all to getter/setters which would invalidate the hash on change (and which would slow down nNonce exploration). Or alternatively to convert to an immutable header after we're done mutating - but that's also likely a huge change.
So, for now, this seemed to be both simpler and slightly more performant than before.
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to Block.GetHash() when reading a block from disk by BlockManager. The hash is checked against the expected result from a CBlockIndex. Covered by a new unit test.
<details><summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmg18rgACgkQ5+KYS2KJ
yTpYuA//S3kDsTQA0vhOf5RwbyLWnRBj/6raPmUNz8H/54Jy1ltT53KEVE/HHgOD
zAlOKVxfH2lmgAhcbFF5DdcUtR+z0mBrMNzXBt+ELWVep+7gV2lCA/XKNSpMBAEh
id84hJE7haKB+gjvYuYBsQG9mvhZ9QrgWVD/RkvjGjcQtO4zclTOMiOdL8c/5JU5
V7yCNF25ymoEv7m+58JTzWqvM5bfoYlbLUpoLUX6mWvdFVq8UdOzpM+3Pg4/HdQR
2jkv/oxNq+cuDypSonQlF5SWjQENzGMQVO29NZ4iE/DA34Z/5Xe3ouRZUzgWG7xp
aK/x4d/hlHvo/l+PCWgV7iABnK+3avdsWza+GBwG08GTE3s6xVqga0DG9S3TCrNq
0rM01t5FjzOK0Hwj5rnUr3EBPvGUrWC4DqszUrQ6EpO3pJQvhrXgqWTHusERo3B7
uLCvvg+QaxUlfxli4KEbELq2UO4hWlxATTUSa5unbdwNaD2nW3QH7s2tin+jhPbt
a+rCJdqggK83QDtEtesqzTQQmI++dCKP9qA9oOexrUZAB93ndxzs9omMDQlFrkPV
Bl0J58bI/extHJgwqe/QpFiHboCaxyWkpi3QyLuW0a9GpBMRk+LREM0Hl6d/FphW
A5gjkCAhHisOxz8w4tzdXq+oJAls6LGTSQwh6XlqvBLUCF0LIQI=
=XxuP
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
</details>
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0