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

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33154.

   <!--021abf342d371248e50ceaed478a90ca-->

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

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:

           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. l0rinc force-pushed on Aug 8, 2025
10. 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

11. 
    stickies-v commented at 10:03 AM on August 12, 2025: contributor

    Approach ACK

12. test: use local `CBlockIndex` in block read hash mismatch test to avoid data race

    Co-authored-by: stickies-v <stickies-v@protonmail.com>

    cb173b8e93
13. l0rinc force-pushed on Aug 12, 2025
14. stickies-v approved
15. 
    stickies-v commented at 10:08 PM on August 12, 2025: contributor

    ACK cb173b8e939d63821a966d0d76b299f20742c619

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

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

    lgtm ACK cb173b8e939d63821a966d0d76b299f20742c619

18. maflcko added this to the milestone 30.0 on Aug 20, 2025
19. fanquake merged this on Aug 20, 2025
20. fanquake closed this on Aug 20, 2025

    ---

Contributors

l0rincDrahtBotmaflckostickies-v

Milestone
30.0