test: use local `CBlockIndex` in block read hash mismatch check #33154

pull l0rinc wants to merge 1 commits into bitcoin:master from l0rinc:l0rinc/readblock-hash-mismatch-test changing 1 files +10 −4

l0rinc commented at 9:17 PM on August 7, 2025: contributor

Avoid mutating the shared active tip CBlockIndex in the blockmanager_readblock_hash_mismatch test. Instead, construct a local CBlockIndex with only the required fields set, ensuring the test remains self-contained and hopefully eliminating the data race reported in #33150.
DrahtBot added the label Tests on Aug 7, 2025
DrahtBot commented at 9:17 PM on August 7, 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/33154.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK stickies-v, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
l0rinc marked this as ready for review on Aug 7, 2025
in src/test/blockmanager_tests.cpp:146 in 1f7821b4f5 outdated
```
 143 | -    fake_index->phashBlock = &uint256::ONE; // invalid block hash
 144 | +    CBlockIndex index;
 145 | +    {
 146 | +        LOCK(cs_main);
 147 | +        index.nStatus = BLOCK_HAVE_DATA;
 148 | +        index.nDataPos = STORAGE_HEADER_BYTES;
```
maflcko commented at 9:17 AM on August 8, 2025:

index.nDataPos = m_node.chainman->ActiveTip()->nDataPos;

nit: Would be nice to document explicitly what block this reads (and drop the hardcoded implementation detail) and also use the m_node.chainman->GetMutex() for new code.
l0rinc commented at 11:15 PM on August 8, 2025:

to document explicitly what block this reads

Used the tip for the rest of the values.

use the m_node.chainman->GetMutex() for new code

It seems to me that nStatus and nDataPos need an explicit cs_main lock - can you point to a similar code which does it the way you think should be done here as well?

Type	Reviewers
ACK	stickies-v, maflcko

in src/test/blockmanager_tests.cpp:145 in 1f7821b4f5 outdated

 138 | @@ -139,12 +139,17 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 139 |  
 140 |  BOOST_FIXTURE_TEST_CASE(blockmanager_readblock_hash_mismatch, TestingSetup)
 141 |  {
 142 | -    CBlockIndex* fake_index{WITH_LOCK(m_node.chainman->GetMutex(), return m_node.chainman->ActiveChain().Tip())};
 143 | -    fake_index->phashBlock = &uint256::ONE; // invalid block hash
 144 | +    CBlockIndex index;
 145 | +    {
 146 | +        LOCK(cs_main);
 147 | +        index.nStatus = BLOCK_HAVE_DATA;

maflcko commented at 9:18 AM on August 8, 2025:

nit: Can be removed?

l0rinc commented at 11:10 PM on August 8, 2025:

Is the test passing for you that way? I'm getting:

unknown location:0: fatal error: in "blockmanager_tests/blockmanager_readblock_hash_mismatch": std::runtime_error: 'GetHash() doesn't match index' not found in debug log

maflcko approved
maflcko commented at 9:18 AM on August 8, 2025: member

lgtm
l0rinc force-pushed on Aug 8, 2025
in src/test/blockmanager_tests.cpp:148 in 73e0f070dd outdated
```
 145 | +    {
 146 | +        LOCK(cs_main);
 147 | +        const auto tip{m_node.chainman->ActiveTip()};
 148 | +        index.nStatus = tip->nStatus;
 149 | +        index.nDataPos = tip->nDataPos;
 150 | +        index.phashBlock = tip->phashBlock + 1; // invalid block hash
```
stickies-v commented at 10:02 AM on August 12, 2025:

I think you meant to increment the hash with one, rather than doing pointer arithmetic? Currently, this looks like UB to me. I think sticking to &uint256::ONE here is the preferable approach, no need to allocate a new hash imo.

index.phashBlock = &uint256::ONE; // mismatched block hash
l0rinc commented at 6:35 PM on August 12, 2025:

Good point, thanks, added you as coauthor
stickies-v commented at 10:03 AM on August 12, 2025: contributor

Approach ACK
test: use local `CBlockIndex` in block read hash mismatch test to avoid data race
Co-authored-by: stickies-v <stickies-v@protonmail.com>
cb173b8e93
l0rinc force-pushed on Aug 12, 2025
stickies-v approved
stickies-v commented at 10:08 PM on August 12, 2025: contributor

ACK cb173b8e939d63821a966d0d76b299f20742c619

in src/test/blockmanager_tests.cpp:144 in cb173b8e93

 138 | @@ -139,12 +139,18 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 139 |  
 140 |  BOOST_FIXTURE_TEST_CASE(blockmanager_readblock_hash_mismatch, TestingSetup)
 141 |  {
 142 | -    CBlockIndex* fake_index{WITH_LOCK(m_node.chainman->GetMutex(), return m_node.chainman->ActiveChain().Tip())};
 143 | -    fake_index->phashBlock = &uint256::ONE; // invalid block hash
 144 | +    CBlockIndex index;
 145 | +    {
 146 | +        LOCK(cs_main);

maflcko commented at 2:41 PM on August 20, 2025:

nit: Could use the m_node.chainman->GetMutex() alias for new code.

maflcko commented at 2:41 PM on August 20, 2025: member

lgtm ACK cb173b8e939d63821a966d0d76b299f20742c619
maflcko added this to the milestone 30.0 on Aug 20, 2025
fanquake merged this on Aug 20, 2025
fanquake closed this on Aug 20, 2025
alexanderwiederin referenced this in commit 67b164b083 on Aug 21, 2025
yuvicc referenced this in commit 22f55cf11d on Aug 26, 2025
stringintech referenced this in commit ccb9779390 on Aug 27, 2025
bug-castercv502 referenced this in commit 5ccbb9e900 on Sep 28, 2025
stickies-v referenced this in commit a19c56cd7c on Nov 4, 2025
Kino1994 referenced this in commit 5da9f9f391 on Jun 28, 2026
BigcoinBGC referenced this in commit b7ae6918c1 on Jun 30, 2026
Kino1994 referenced this in commit 3e1baf5339 on Aug 19, 2026
bitcoin locked this on Aug 20, 2026

Contributors

Labels

Milestone
30.0

Linked (view graph)

#33150 intermittent TSAN failure in lockmanager_tests::blockmanager_readblock_hash_mismatch

test: use local `CBlockIndex` in block read hash mismatch check #33154

Code Coverage & Benchmarks

Reviews