Fixes a race condition in CreateNewBlock and a future null deref on testnet. #1953

1. 
   gmaxwell commented at 5:53 AM on October 24, 2012: contributor

   CreateNewBlock was reading pindexBest at the start before taking the lock so it was possible to have the the block content not match the prevheader and this can also trigger a newly added assert in ConnectBlock.

   I noticed this during a code review after twobitcoins reported that ab91bf39 (BIP30 for all blocks) could cause a null dereference on a modified node that mined during the IBD, or on testnet when it reached heights 91842 and 91880 due to CreateNewBlock calling ConnectBlock with pindex->phashBlock NULL.

2. 
   gmaxwell commented at 6:01 AM on October 24, 2012: contributor

   Tested by reproducing both issues while syncing testnet under valgrind, and by mining on testnet.

3. 
   Diapolo commented at 6:02 AM on October 24, 2012: none

   @gmaxwell The part with the LOCK is also described in #1946. I can confirm this fixes the assertion I observed a few times while doing some testnet tests.

4. Fixes a race condition in CreateNewBlock and a future null deref on testnet.

   CreateNewBlock was reading pindexBest at the start before taking the lock
   so it was possible to have the the block content not match the prevheader
   and this can also trigger a newly added assert in ConnectBlock.
   
   I noticed this during a code review after twobitcoins reported that ab91bf39
   (BIP30 for all blocks) could cause a null dereference on a modified node
   that mined during the IBD, or on testnet when it reached heights 91842 and
   91880 due to CreateNewBlock calling ConnectBlock with pindex->phashBlock NULL.

   faff50d129
5. 
   laanwj commented at 6:16 AM on October 24, 2012: member

   ACK

6. 
   gmaxwell commented at 6:17 AM on October 24, 2012: contributor

   @Diapolo Duh, I should have paid attention to the issues, it would have saved me a few minutes. :P

7. 
   Diapolo commented at 6:19 AM on October 24, 2012: none

   Perhaps it sunk in my pull-request spam :-D, but nice it gets fixed.

8. in src/main.cpp:None in faff50d129

   1557 | @@ -1558,7 +1558,8 @@ bool CBlock::ConnectBlock(CBlockIndex* pindex, CCoinsViewCache &view, bool fJust
   1558 |      // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the
   1559 |      // two in the chain that violate it. This prevents exploiting the issue against nodes in their
   1560 |      // initial block download.
   1561 | -    bool fEnforceBIP30 = !((pindex->nHeight==91842 && pindex->GetBlockHash() == uint256("0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec")) ||
   

   ---

   ---

   

   sipa commented at 9:14 AM on October 24, 2012:

   I don't really like the fact that the hash in CBlockIndex::phashBlock is optional. We have a GetBlockHash()... why doesn't it just calculate the hash if it's not available?

   ---

   

   gmaxwell commented at 1:10 PM on October 24, 2012:

   Hm. In the case of a CreateNewBlock JustCheck it would just be pointless computation on an operation which is supposed to be fast. Effectively in this case we don't know— and can't know— the hash yet. Perhaps generally GetBlockHash should do that though. At least in this case it only gets called for those two heights.

   ---

   

   sipa commented at 1:36 PM on October 24, 2012:

   Agree, in this case, it's an optimization not needing to calculate the hash.

9. 
   gavinandresen commented at 2:17 PM on October 24, 2012: contributor

   ACK

10. sipa referenced this in commit 66444558a5 on Oct 25, 2012
11. sipa merged this on Oct 25, 2012
12. sipa closed this on Oct 25, 2012

    ---

13. laudney referenced this in commit 492a000579 on Mar 19, 2014
14. KolbyML referenced this in commit 1b7d182bb5 on Dec 5, 2020
15. DrahtBot locked this on Sep 8, 2021

Contributors

gmaxwellDiapololaanwjsipagavinandresen

Milestone
0.8.0