Push cs_main locks down in ProcessBlock #4148

1. 
   sipa commented at 8:57 PM on May 7, 2014: member

   For now, built on top of #3884.

2. 
   laanwj commented at 8:30 AM on May 8, 2014: member

   Fixes at least #3966.

3. laanwj added this to the milestone 0.10.0 on May 8, 2014
4. in src/main.cpp:None in ed85993dc1 outdated

   2188 | +
   2189 | +bool ActivateBestChain(CValidationState &state) {
   2190 | +    CBlockIndex *pindexNewTip = NULL;
   2191 | +    CBlockIndex *pindexMostWork = NULL;
   2192 | +    do {
   2193 | +        bool fInitialDownload;
   

   ---

   ---

   

   laanwj commented at 8:33 PM on May 8, 2014:

   Should we have a boost::this_thread::interruption_point() here?

   ---

   

   sipa commented at 9:39 PM on May 8, 2014:

   Good idea!

5. 
   sipa commented at 10:26 PM on May 8, 2014: member

   Rebased and added an interruption point in ActivateBestChain, as suggested by @laanwj.

6. 
   laanwj commented at 1:22 PM on May 19, 2014: member

   ACK on code changes Also I've run with this patch for about a week now on my node. No problems.

7. 
   sipa commented at 1:30 PM on May 19, 2014: member

   I'm running this patch together with #2784, #4100 and #4177, and I'm seeing occasional segfaults. I believe this patch is the most likely cause, so I'll investigate first.

8. 
   jgarzik commented at 1:46 PM on May 19, 2014: contributor

   initial review ACK (untested).

   I will try some lock-debug testing later this week.

9. in src/main.cpp:None in f6a6adcc43 outdated

   2225 | +            uint256 hashNewTip = pindexNewTip->GetBlockHash();
   2226 | +            // Relay inventory, but don't relay old inventory during initial block download.
   2227 | +            int nBlockEstimate = Checkpoints::GetTotalBlocksEstimate();
   2228 | +            LOCK(cs_vNodes);
   2229 | +            BOOST_FOREACH(CNode* pnode, vNodes)
   2230 | +                if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate))
   

   ---

   ---

   

   cozz commented at 6:24 PM on May 19, 2014:

   Isn't chainActive here and a few lines below accessed without cs_main?

   ---

   

   sipa commented at 11:37 AM on May 22, 2014:

   Nice catch. However, chainActive should never be modified anywhere but in the thread calling this method (the message handler), so it can't be the cause for the segfault i observed (perhaps #4204 fixed it, though). I'll fix it.

10. 
    sipa commented at 10:25 AM on May 23, 2014: member

    @cozz Updated.

11. 
    sipa commented at 10:27 AM on May 23, 2014: member

    I haven't seen any segfaults anymore since running on top of #4204.

12. 
    laanwj commented at 10:52 AM on May 23, 2014: member

    I still haven't had any segfaults running with this.

13. Get rid of the static chainMostWork (optimization) 77339e5aec
14. Allow ActivateBestChain to release its lock on cs_main 4e0eed88ac
15. Move all post-chaintip-change notifications to ActivateBestChain 202e01941c
16. Push cs_mains down in ProcessBlock 18e72167dd
17. 
    BitcoinPullTester commented at 12:38 AM on June 9, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/18e72167ddfeaea95253b62994c6d64b55b35005 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

18. 
    laanwj commented at 8:11 AM on June 9, 2014: member

    ACK

19. laanwj merged this on Jun 9, 2014
20. laanwj closed this on Jun 9, 2014

    ---

21. laanwj referenced this in commit 07b233a1b6 on Jun 9, 2014
22. in src/main.cpp:None in 18e72167dd

    2663 | @@ -2649,7 +2664,11 @@ bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBl
    2664 |          mapOrphanBlocksByPrev.erase(hashPrev);
    2665 |      }
    2666 |  
    2667 | -    LogPrintf("ProcessBlock: ACCEPTED\n");
    

    ---

    ---

    

    rebroad commented at 1:59 PM on June 20, 2014:

    Ahem! #3984 did this!

23. DrahtBot locked this on Sep 8, 2021

Contributors

sipalaanwjjgarzikcozzBitcoinPullTesterrebroad

Milestone
0.10.0