ReadBlock
#32487
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32487.
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.
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()) {
Assume instead.
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.
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
0025-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
02025-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
Would it help if we included the two different hashes in the error?
Sure, why not, since we have them available.
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
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)
TestingSetup to half the duration of the test.
review ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851 🌋
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 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851 🌋
312TE8ZKu4avgseXLh/q3bD12L6JUoKwRM4JRSVJomoSPCLwU7x9A91dE0TdOMtRbkIOg6/qKppZ6YA6uZ8jIDQ==
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.
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.
0 LogError("GetHash() doesn't match index at %s while reading block (actual %s != expected %s)",
CBlockHeader and compute-or-return that value in GetHash()?
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.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmg18rgACgkQ5+KYS2KJ
7yTpYuA//S3kDsTQA0vhOf5RwbyLWnRBj/6raPmUNz8H/54Jy1ltT53KEVE/HHgOD
8zAlOKVxfH2lmgAhcbFF5DdcUtR+z0mBrMNzXBt+ELWVep+7gV2lCA/XKNSpMBAEh
9id84hJE7haKB+gjvYuYBsQG9mvhZ9QrgWVD/RkvjGjcQtO4zclTOMiOdL8c/5JU5
10V7yCNF25ymoEv7m+58JTzWqvM5bfoYlbLUpoLUX6mWvdFVq8UdOzpM+3Pg4/HdQR
112jkv/oxNq+cuDypSonQlF5SWjQENzGMQVO29NZ4iE/DA34Z/5Xe3ouRZUzgWG7xp
12aK/x4d/hlHvo/l+PCWgV7iABnK+3avdsWza+GBwG08GTE3s6xVqga0DG9S3TCrNq
130rM01t5FjzOK0Hwj5rnUr3EBPvGUrWC4DqszUrQ6EpO3pJQvhrXgqWTHusERo3B7
14uLCvvg+QaxUlfxli4KEbELq2UO4hWlxATTUSa5unbdwNaD2nW3QH7s2tin+jhPbt
15a+rCJdqggK83QDtEtesqzTQQmI++dCKP9qA9oOexrUZAB93ndxzs9omMDQlFrkPV
16Bl0J58bI/extHJgwqe/QpFiHboCaxyWkpi3QyLuW0a9GpBMRk+LREM0Hl6d/FphW
17A5gjkCAhHisOxz8w4tzdXq+oJAls6LGTSQwh6XlqvBLUCF0LIQI=
18=XxuP
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on openpgp.org
