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

1. 
   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.
2. DrahtBot added the label Tests on Aug 7, 2025
3. 
   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. A summary of reviews will appear here.

4. l0rinc marked this as ready for review on Aug 7, 2025
5. 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:

   0        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?

6. 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

7. maflcko approved
8. 
   maflcko commented at 9:18 am on August 8, 2025: member

   lgtm
9. test: use local `CBlockIndex` in block read hash mismatch test to avoid data race 73e0f070dd
10. l0rinc force-pushed on Aug 8, 2025

Contributors
l0rinc DrahtBot maflcko

Labels
Tests