Add LookupBlockIndex #11041

pull promag wants to merge 5 commits into bitcoin:master from promag:2017-08-lookup-block-index changing 14 files +169 −152
  1. promag commented at 4:24 PM on August 13, 2017: member

    Replace all mapBlockIndex lookups with the new LookupBlockIndex(). In some cases it avoids a second lookup.

  2. promag force-pushed on Aug 13, 2017
  3. promag force-pushed on Aug 13, 2017
  4. in src/rest.cpp:215 in 171213cb32 outdated 
     210 | @@ -212,10 +211,11 @@ static bool rest_block(HTTPRequest* req,
 211 |      CBlockIndex* pblockindex = NULL;
 212 |      {
 213 |          LOCK(cs_main);
 214 | -        if (mapBlockIndex.count(hash) == 0)
 215 | +        pblockindex = LookupBlockIndex(hash);
 216 | +        if (pblockindex) {
    promag commented at 5:39 PM on August 13, 2017:

    !pblockindex

  5. in src/rpc/blockchain.cpp:673 in 171213cb32 outdated 
     668 | @@ -669,10 +669,10 @@ UniValue getblockheader(const JSONRPCRequest& request)
 669 |      if (!request.params[1].isNull())
 670 |          fVerbose = request.params[1].get_bool();
 671 |  
 672 | -    if (mapBlockIndex.count(hash) == 0)
 673 | +    CBlockIndex* pblockindex = LookupBlockIndex(hash);
 674 | +    if (pblockindex == nullptr) {
    promag commented at 5:40 PM on August 13, 2017:

    Same as above.

  6. in src/rpc/blockchain.cpp:820 in 171213cb32 outdated 
     815 | @@ -817,7 +816,9 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
 816 |      stats.hashBlock = pcursor->GetBestBlock();
 817 |      {
 818 |          LOCK(cs_main);
 819 | -        stats.nHeight = mapBlockIndex.find(stats.hashBlock)->second->nHeight;
 820 | +        const CBlockIndex* pindex = LookupBlockIndex(stats.hashBlock);
 821 | +        assert(pindex);
    promag commented at 5:40 PM on August 13, 2017:

    Remove assert.

  7. promag force-pushed on Aug 13, 2017
  8. promag force-pushed on Aug 13, 2017
  9. promag force-pushed on Aug 13, 2017
  10. fanquake added the label Refactoring on Aug 14, 2017
  11. jonasschnelli commented at 10:27 AM on August 14, 2017: contributor

    Concept ACK

  12. TheBlueMatt commented at 2:45 PM on August 14, 2017: member

    This may make #10692 a bit simpler, though I'm not sure it matters what order they come in.

  13. promag force-pushed on Aug 14, 2017
  14. promag force-pushed on Aug 15, 2017
  15. promag commented at 3:37 PM on August 15, 2017: member

    Fixed bug and rebased.

  16. promag commented at 2:56 PM on August 22, 2017: member

    @laanwj I believe it's ready.

  17. laanwj commented at 3:49 PM on August 22, 2017: member

    Concept ACK

  18. in src/net_processing.cpp:909 in c5153911f5 outdated 
     905 | @@ -904,7 +906,7 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 906 |          }
 907 |      case MSG_BLOCK:
 908 |      case MSG_WITNESS_BLOCK:
 909 | -        return mapBlockIndex.count(inv.hash);
 910 | +        return LookupBlockIndex(inv.hash);
    meshcollider commented at 1:20 AM on August 23, 2017:

    Maybe explicit cast to bool here for clarity

    promag commented at 3:26 AM on August 23, 2017:

    Prefer this?

    return LookupBlockIndex(inv.hash) != nullptr;
    meshcollider commented at 4:33 AM on August 23, 2017:

    Yeah or a ternary or even an if statement would be a lot clearer

  19. meshcollider commented at 1:21 AM on August 23, 2017: contributor

    utACK

  20. promag force-pushed on Aug 25, 2017
  21. promag commented at 2:38 PM on August 25, 2017: member

    Rebased and fixed @MeshCollider comment.

  22. in src/validation.h:434 in 1d1768f297 outdated 
     424 | @@ -425,6 +425,12 @@ class CVerifyDB {
 425 |  /** Replay blocks that aren't fully applied to the database. */
 426 |  bool ReplayBlocks(const CChainParams& params, CCoinsView* view);
 427 |  
 428 | +inline CBlockIndex* LookupBlockIndex(const uint256& hash)
 429 | +{
 430 | +    BlockMap::const_iterator it = mapBlockIndex.find(hash);
    promag commented at 2:45 PM on October 30, 2017:

    Missing assert lock is held.

    practicalswift commented at 7:25 PM on October 30, 2017:

    Missing EXCLUSIVE_LOCKS_REQUIRED(cs_main) too :-)

  23. in src/wallet/wallet.cpp:3822 in 1d1768f297 outdated 
    3790 | @@ -3793,7 +3791,8 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
3791 |  {
3792 |      unsigned int nTimeSmart = wtx.nTimeReceived;
3793 |      if (!wtx.hashUnset()) {
3794 | -        if (mapBlockIndex.count(wtx.hashBlock)) {
3795 | +        const CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock);
3796 | +        if (pindex) {
    ryanofsky commented at 4:25 PM on November 2, 2017:

    In some of these places you could do

    if (const CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock))

    to reduce the scope of the pindex variables.

    kallewoof commented at 7:05 PM on March 6, 2018:

    I didn't know you could declare inside an if statement. Cool.

  24. ryanofsky commented at 4:29 PM on November 2, 2017: member

    utACK 1d1768f2978bafaf166bfa218c899ddaeeccad2d

  25. promag commented at 4:39 PM on November 2, 2017: member

    Thanks @ryanofsky, I have to rebase and then I'll take into account your comment above.

  26. promag force-pushed on Nov 3, 2017
  27. promag force-pushed on Nov 3, 2017
  28. in src/init.cpp:1460 in a029cd4093 outdated 
    1433 | @@ -1434,8 +1434,9 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1434 |  
1435 |                  // If the loaded chain has a wrong genesis, bail out immediately
1436 |                  // (we're likely using a testnet datadir, or the other way around).
1437 | -                if (!mapBlockIndex.empty() && mapBlockIndex.count(chainparams.GetConsensus().hashGenesisBlock) == 0)
    jimpo commented at 11:13 PM on December 6, 2017:

    Need to keep the !mapBlockIndex.empty().

  29. in src/net_processing.cpp:2282 in a029cd4093 outdated 
    2279 |                  connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256()));
2280 |              return true;
2281 |          }
2282 |  
2283 | -        if (mapBlockIndex.find(cmpctblock.header.GetHash()) == mapBlockIndex.end()) {
2284 | +        if (LookupBlockIndex(cmpctblock.header.GetHash())) {
    jimpo commented at 11:17 PM on December 6, 2017:

    Needs a !

  30. jimpo commented at 11:33 PM on December 6, 2017: contributor

    Concept ACK. This is much cleaner.

  31. promag commented at 11:39 PM on December 6, 2017: member

    This needs rebase. I can do it and adapt the new code if there is interest in merging it quick.

    Maybe it should also make BlockMap mapBlockIndex static?

    cc @MarcoFalke @laanwj

  32. TheBlueMatt commented at 3:42 PM on December 12, 2017: member

    Concept ACK. Would like to see this rebased sooner rather than later.

  33. promag force-pushed on Dec 13, 2017
  34. promag commented at 3:42 PM on December 13, 2017: member

    Forgot to tackle @ryanofsky comment above..

  35. promag force-pushed on Dec 13, 2017
  36. MarcoFalke commented at 7:00 PM on December 13, 2017: member

    travis failure:

    Assertion failed: lock cs_main not held in ./validation.h:433; locks held:
  37. promag force-pushed on Dec 14, 2017
  38. promag force-pushed on Dec 14, 2017
  39. in src/qt/transactionrecord.cpp:170 in cbff072b60 outdated 
     166 | @@ -167,10 +167,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx)
 167 |      // Determine transaction status
 168 |  
 169 |      // Find the block the tx is in
 170 | -    CBlockIndex* pindex = nullptr;
 171 | -    BlockMap::iterator mi = mapBlockIndex.find(wtx.hashBlock);
 172 | -    if (mi != mapBlockIndex.end())
 173 | -        pindex = (*mi).second;
 174 | +    CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock);
    laanwj commented at 10:46 AM on December 14, 2017:

    const?

  40. in src/rpc/blockchain.cpp:677 in cbff072b60 outdated 
     673 | @@ -674,10 +674,10 @@ UniValue getblockheader(const JSONRPCRequest& request)
 674 |      if (!request.params[1].isNull())
 675 |          fVerbose = request.params[1].get_bool();
 676 |  
 677 | -    if (mapBlockIndex.count(hash) == 0)
 678 | +    CBlockIndex* pblockindex = LookupBlockIndex(hash);
    laanwj commented at 10:49 AM on December 14, 2017:

    const CBlockIndex*?

  41. in src/rpc/blockchain.cpp:756 in cbff072b60 outdated 
     752 | @@ -753,12 +753,12 @@ UniValue getblock(const JSONRPCRequest& request)
 753 |              verbosity = request.params[1].get_bool() ? 1 : 0;
 754 |      }
 755 |  
 756 | -    if (mapBlockIndex.count(hash) == 0)
 757 | +    CBlockIndex* pblockindex = LookupBlockIndex(hash);
    laanwj commented at 10:50 AM on December 14, 2017:

    Another one that could be const. There are more, but I'll stop commenting every one.

  42. laanwj commented at 10:56 AM on December 14, 2017: member
    • I think it would be preferable to use const CBlockIndex* where it's possible.
    • There are some cs_main locking changes; can you please explain them? Were locks missing there before, making this a bugfix as well as refactor?
  43. laanwj added the label Bug on Dec 14, 2017
  44. promag commented at 11:30 AM on December 14, 2017: member

    I think it would be preferable to use const CBlockIndex* where it's possible

    Agree.

    There are some cs_main locking changes; can you please explain them? Were locks missing there before, making this a bugfix as well as refactor?

    Yes, for instance FindForkInGlobalIndex.

    I'll check if adding annotations helps identifying more missing locks. I'll update accordingly.

  45. TheBlueMatt commented at 3:52 PM on December 14, 2017: member

    re: const CBlockIndex (and associated things): see #10692.

  46. in src/checkpoints.cpp:24 in cbff072b60 outdated 
      20 | @@ -21,9 +21,9 @@ namespace Checkpoints {
  21 |          for (const MapCheckpoints::value_type& i : reverse_iterate(checkpoints))
  22 |          {
  23 |              const uint256& hash = i.second;
  24 | -            BlockMap::const_iterator t = mapBlockIndex.find(hash);
  25 | -            if (t != mapBlockIndex.end())
  26 | -                return t->second;
  27 | +            if (CBlockIndex* const pindex = LookupBlockIndex(hash)) {
    TheBlueMatt commented at 4:04 PM on December 14, 2017:

    I dont think that is the type of const you meant. Probably just leave it non-const for now and I'll fix it up in #10692 (or you can start pulling stuff out of there into this PR if you prefer, but that probably just ends up taking more time).

  47. in src/init.cpp:1493 in cbff072b60 outdated 
    1491 | @@ -1487,6 +1492,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1492 |  
1493 |                  bool is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull();
1494 |                  if (!is_coinsview_empty) {
1495 | +                    LOCK(cs_main);
    TheBlueMatt commented at 4:06 PM on December 14, 2017:

    This seems unrelated. Maybe leave it for #11596 instead? Or just do as I suggested at #11596 (review) and put a cs_main at the top of the load-chain-data block here.

    promag commented at 10:04 AM on December 17, 2017:

    It's needed for the assert below (L1502).

    TheBlueMatt commented at 7:41 PM on December 17, 2017:

    Looks like you can remove this line now?

  48. in src/validation.cpp:262 in cbff072b60 outdated 
     258 | @@ -259,12 +259,12 @@ namespace {
 259 |  
 260 |  CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator)
 261 |  {
 262 | +    LOCK(cs_main);
    TheBlueMatt commented at 4:25 PM on December 14, 2017:

    Looks like you're just adding this for CreateWalletFromFile, which already needs cs_main (and is missing it there). This should be an AssertLockHeld.

    promag commented at 12:56 AM on December 17, 2017:

    And why does CreateWalletFromFile needs cs_main?

    TheBlueMatt commented at 5:15 AM on December 17, 2017:

    Hmm, there was some discussion of this in some other PR recently, maybe a @practicalswift PR? In any case, there are a number of things in CreateWalletFromFile that need cs_main (though possibly we want to avoid it and just take it for parts to avoid cs_main during the rescan after #11281), eg chainActive.GetLocator(), chainActive.Genesis (maybe not technically, but probably), FindForkInGlobalIndex, chainActive.Tip(), etc. So definitely worth just taking cs_main there and making this an AssertLockHeld.

  49. in src/validation.cpp:1817 in cbff072b60 outdated 
    1812 | @@ -1813,9 +1813,9 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1813 |          //  relative to a piece of software is an objective fact these defaults can be easily reviewed.
1814 |          // This setting doesn't force the selection of any particular chain but makes validating some faster by
1815 |          //  effectively caching the result of part of the verification.
1816 | -        BlockMap::const_iterator  it = mapBlockIndex.find(hashAssumeValid);
1817 | -        if (it != mapBlockIndex.end()) {
1818 | -            if (it->second->GetAncestor(pindex->nHeight) == pindex &&
1819 | +        const CBlockIndex* pindex_valid = LookupBlockIndex(hashAssumeValid);
    TheBlueMatt commented at 4:26 PM on December 14, 2017:

    Dunno if its worth bothering for things in CChainState. The whole goal here is to make mapBlockIndex a private in CChainState and expose LookupBlockIndex externally.

    promag commented at 10:11 AM on December 17, 2017:

    I'd prefer to avoid special cases for now. Eventually LookupBlockIndex will loose inline, so do it then?

    TheBlueMatt commented at 7:55 PM on December 17, 2017:

    For cases where we are fine with a const CBlockIndex* result, I dont think it matters much, but I'd kinda like to have LookupBlockIndex only return const CBlockIndex*es, so for things like CChainState::AddToBlockIndex, we'd have to partially revert this change then, easier to not do it in the first place IMO.

    promag commented at 2:13 AM on January 3, 2018:

    have LookupBlockIndex only return const CBlockIndex*es

    If you look closely you will see that a lot of function signatures require non-const CBlockIndex*, although a const should do the work (almost?) everywhere. I'm happy to do that in a follow up and deal with the non-const cases there. At least this PR is complete.

    TheBlueMatt commented at 6:26 PM on January 11, 2018:

    Making everything const isnt so hard, see #10692, which did it mostly in scripted-diffs.

  50. in src/validation.cpp:3643 in cbff072b60 outdated 
    3639 | @@ -3641,17 +3640,20 @@ CBlockIndex * CChainState::InsertBlockIndex(const uint256& hash)
3640 |      if (hash.IsNull())
3641 |          return nullptr;
3642 |  
3643 | +    LOCK(cs_main);
    TheBlueMatt commented at 4:36 PM on December 14, 2017:

    This should absolutely be an AssertLockHeld, if anything. If you add the LOCK(cs_main) in the block-index-loading code in init.cpp this should be able to go away.

    promag commented at 12:55 AM on December 17, 2017:

    Do we want to lock that long?

    TheBlueMatt commented at 5:17 AM on December 17, 2017:

    It doesnt matter, its early in init long before anything else is going to take cs_main, so taking it should have no impact.

  51. in src/validation.cpp:4254 in cbff072b60 outdated 
    4244 | -                    continue;
4245 | -                }
4246 |  
4247 | -                // process in case the block isn't known yet
4248 | -                if (mapBlockIndex.count(hash) == 0 || (mapBlockIndex[hash]->nStatus & BLOCK_HAVE_DATA) == 0) {
4249 | +                {
    TheBlueMatt commented at 4:37 PM on December 14, 2017:

    Same here. Can go away with an init.cpp LOCK(cs_main).

    promag commented at 10:16 AM on December 17, 2017:

    I don't think so, this can be called from RPC. Also, below there is ReadBlockFromDisk and we want to avoid cs_main lock there right?

  52. TheBlueMatt commented at 4:38 PM on December 14, 2017: member

    Dont know how much its worth focusing on making all the CBlockIndex*s const until we make LookupBlockIndex return a const pointer, which requires a few additional commits that will just delay this PR. For the ones that can be done trivially it'd be nice, but I wouldn't say its worth delaying this PR for it as they'll just be scripted-diff'd in #10692.

  53. in src/validation.cpp:2771 in 69bfe5b435 outdated 
    2769 | -    BlockMap::iterator it = mapBlockIndex.find(hash);
2770 | -    if (it != mapBlockIndex.end())
2771 | -        return it->second;
2772 | +    CBlockIndex* pindex = LookupBlockIndex(hash);
2773 | +    if (pindex) {
2774 | +      return pindex;
    TheBlueMatt commented at 7:54 PM on December 17, 2017:

    nit: bad spacing here.

  54. in src/wallet/wallet.cpp:4023 in 69bfe5b435 outdated 
    3962 | @@ -3960,6 +3963,8 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3963 |      // Try to top up keypool. No-op if the wallet is locked.
3964 |      walletInstance->TopUpKeyPool();
3965 |  
3966 | +    LOCK(cs_main);
    TheBlueMatt commented at 7:59 PM on December 17, 2017:

    Hmm, this is gonna conflict partially with #11281. The neccessary refactors seem somewhat unrelated, so I'd be fine with pushing the conflict resolution onto #11281, but if #11281 lands first this is gonna need a bit of tweaking...up to you if you want to do it now.

  55. promag force-pushed on Jan 2, 2018
  56. promag force-pushed on Jan 3, 2018
  57. MarcoFalke deleted a comment on Jan 3, 2018
  58. ryanofsky commented at 8:23 PM on January 8, 2018: member

    utACK 77cc5b7f3dd1c663d6a16946563c7b59d0b3184a. Main change since last review is locking cs_main in AppInitMain, adding lock asserts.

  59. in src/init.cpp:1427 in 77cc5b7f3d outdated 
    1407 | @@ -1408,6 +1408,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1408 |  
1409 |          uiInterface.InitMessage(_("Loading block index..."));
1410 |  
1411 | +        LOCK(cs_main);
    TheBlueMatt commented at 6:08 PM on January 11, 2018:

    This should be in a separate commit.

    promag commented at 1:00 AM on January 12, 2018:

    Done.

  60. in src/validation.cpp:263 in 77cc5b7f3d outdated 
     259 | @@ -260,12 +260,12 @@ namespace {
 260 |  
 261 |  CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator)
 262 |  {
 263 | +    AssertLockHeld(cs_main);
    TheBlueMatt commented at 6:24 PM on January 11, 2018:

    This could go in a separate commit.

    promag commented at 1:00 AM on January 12, 2018:

    Done.

  61. in src/validation.cpp:2779 in 77cc5b7f3d outdated 
    2777 |      // Check for duplicate
2778 |      uint256 hash = block.GetHash();
2779 | -    BlockMap::iterator it = mapBlockIndex.find(hash);
2780 | -    if (it != mapBlockIndex.end())
2781 | -        return it->second;
2782 | +    CBlockIndex* pindex = LookupBlockIndex(hash);
    TheBlueMatt commented at 6:27 PM on January 11, 2018:

    Again, please do not use LookupBlockIndex here (and a few other places). If the next step is to make LookupBlockIndex return a const CBlockIndex* (which I think it clearly is), this part of the patch just has to get reverted. There is no reason to avoid using mapBlockIndex in CChainState members, that's what CChainState is there for :).

    promag commented at 11:50 PM on January 11, 2018:

    There is no reason to avoid using mapBlockIndex in CChainState members, that's what CChainState is there for

    At that point there can be CBlockIndex* CChainState::LookupBlockIndex()?

    TheBlueMatt commented at 6:42 PM on January 14, 2018:

    Why? I see no reason to have a LookupBlockIndex at all except to avoid exposing mapBlockIndex/non-const CBlockIndexes outside of CChainState/validation.cpp. If we're inside of CChainState, doing a LookupBlockIndex only serves to add needless indirection.

  62. TheBlueMatt commented at 6:29 PM on January 11, 2018: member

    Please make locking changes at least be a separate commit. Please don't use LookupBlockIndex when the CBlockIndex can't be made const ala #10692.

  63. promag force-pushed on Jan 12, 2018
  64. promag force-pushed on Jan 12, 2018
  65. promag commented at 3:30 PM on January 15, 2018: member

    Again, please do not use LookupBlockIndex here (and a few other places).

    As requested, last commit reverts changes to CChainState. Please review.

  66. promag force-pushed on Jan 15, 2018
  67. ryanofsky commented at 10:16 PM on February 5, 2018: member

    utACK 7326660786b71af254c2c4bc40c7563be060db8a. Changes up to the second to last commit (d889c036cd...18144408b6) are identical to what I previously reviewed modulo a rebase, commit splitting, and reverting a whitespace change. Very last commit (7326660786b71af254c2c4bc40c7563be060db8a) reverts some changes in the second to last commit, as requested by Matt. Looks fine.

  68. promag commented at 10:30 PM on February 5, 2018: member

    Thanks @ryanofsky.

  69. in src/rpc/mining.cpp:399 in 18144408b6 outdated 
     400 | @@ -401,9 +401,8 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
 401 |                  throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
 402 |  
 403 |              uint256 hash = block.GetHash();
 404 | -            BlockMap::iterator mi = mapBlockIndex.find(hash);
 405 | -            if (mi != mapBlockIndex.end()) {
 406 | -                CBlockIndex *pindex = mi->second;
 407 | +            const CBlockIndex *pindex = LookupBlockIndex(hash);
    kallewoof commented at 6:59 PM on March 6, 2018:

    Nit: CBlockIndex* pindex

  70. in src/rpc/mining.cpp:729 in 18144408b6 outdated 
     730 | @@ -732,9 +731,8 @@ UniValue submitblock(const JSONRPCRequest& request)
 731 |      bool fBlockPresent = false;
 732 |      {
 733 |          LOCK(cs_main);
 734 | -        BlockMap::iterator mi = mapBlockIndex.find(hash);
 735 | -        if (mi != mapBlockIndex.end()) {
 736 | -            CBlockIndex *pindex = mi->second;
 737 | +        const CBlockIndex *pindex = LookupBlockIndex(hash);
    kallewoof commented at 6:59 PM on March 6, 2018:

    Nit: CBlockIndex* pindex

  71. in src/rpc/mining.cpp:744 in 18144408b6 outdated 
     745 | @@ -748,9 +746,9 @@ UniValue submitblock(const JSONRPCRequest& request)
 746 |  
 747 |      {
 748 |          LOCK(cs_main);
 749 | -        BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock);
 750 | -        if (mi != mapBlockIndex.end()) {
 751 | -            UpdateUncommittedBlockStructures(block, mi->second, Params().GetConsensus());
 752 | +        const CBlockIndex *pindex = LookupBlockIndex(block.hashPrevBlock);
    kallewoof commented at 6:59 PM on March 6, 2018:

    Nit: CBlockIndex* pindex

  72. kallewoof commented at 7:06 PM on March 6, 2018: member

    utACK -- Nice clean up!

  73. Assert cs_main is held when accessing mapBlockIndex 02de6a6bcd
  74. Lock cs_main while loading block index in AppInitMain c651df8b32
  75. Fix cs_main lock in LoadExternalBlockFile
    When accessing mapBlockIndex cs_main must be held.
    f814a3e8fa
  76. Add missing cs_lock in CreateWalletFromFile 43a32b7395
  77. promag force-pushed on Mar 6, 2018
  78. promag commented at 7:30 PM on March 6, 2018: member

    Rebased. @kallewoof why drop const qualifier?

  79. kallewoof commented at 7:41 PM on March 6, 2018: member

    @promag Sorry, I only meant "move the *"

  80. Add LookupBlockIndex function 92fabcd443
  81. promag force-pushed on Mar 6, 2018
  82. promag commented at 7:52 PM on March 6, 2018: member

    @kallewoof no problem, done.

  83. kallewoof commented at 7:57 PM on March 6, 2018: member

    utACK 92fabcd

  84. ryanofsky commented at 9:29 PM on March 12, 2018: member

    utACK 92fabcd443322dcfdf2b3477515fae79e8647d86. No changes since last review except rebase and squash.

  85. laanwj merged this on Mar 13, 2018
  86. laanwj closed this on Mar 13, 2018
  87. laanwj referenced this in commit d42a4fe5aa on Mar 13, 2018
  88. in src/wallet/wallet.cpp:3819 in 92fabcd443 
    3813 | @@ -3816,7 +3814,12 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
3814 |  {
3815 |      unsigned int nTimeSmart = wtx.nTimeReceived;
3816 |      if (!wtx.hashUnset()) {
3817 | -        if (mapBlockIndex.count(wtx.hashBlock)) {
3818 | +        const CBlockIndex* pindex = nullptr;
3819 | +        {
3820 | +            LOCK(cs_main);
    ryanofsky commented at 10:29 PM on March 13, 2018:

    I think this new lock is causing test failures on master. If I run

    ./configure --enable-debug
make -j12 -C src test/test_bitcoin && src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ComputeTimeSmart

    I see:

    Entering test case "ComputeTimeSmart"
test_bitcoin: sync.cpp:100: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed.
unknown location(0): fatal error in "ComputeTimeSmart": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp(566): last checkpoint

    The problem is that ComputeTimeSmart is called by CWallet::AddToWallet, which only acquires cs_wallet, so cs_main will wind up being acquired after cs_wallet during the test.

  89. MarcoFalke referenced this in commit 18462960c0 on Mar 14, 2018
  90. promag deleted the branch on Jun 10, 2018
  91. UdjinM6 referenced this in commit 3f87bff4e0 on Jul 29, 2020
  92. UdjinM6 referenced this in commit 8c9b446ffd on Jul 29, 2020
  93. PastaPastaPasta referenced this in commit e659cdc8ee on Dec 12, 2020
  94. PastaPastaPasta referenced this in commit 93b5a2018d on Dec 12, 2020
  95. PastaPastaPasta referenced this in commit df77902611 on Dec 15, 2020
  96. PastaPastaPasta referenced this in commit daa7b95794 on Dec 15, 2020
  97. PastaPastaPasta referenced this in commit 0efc03deb0 on Dec 15, 2020
  98. PastaPastaPasta referenced this in commit c8476ea708 on Dec 15, 2020
  99. xdustinface referenced this in commit 5fb9f6d5f9 on Dec 15, 2020
  100. PastaPastaPasta referenced this in commit e0acd39ce2 on Dec 15, 2020
  101. PastaPastaPasta referenced this in commit e723edb4e6 on Dec 15, 2020
  102. PastaPastaPasta referenced this in commit a6423bbf2e on Dec 18, 2020
  103. PastaPastaPasta referenced this in commit 22e955b463 on Dec 18, 2020
  104. PastaPastaPasta referenced this in commit dad966e9fa on Dec 18, 2020
  105. LarryRuane referenced this in commit 377af7ef25 on Mar 22, 2021
  106. LarryRuane referenced this in commit 24f47818b6 on Mar 22, 2021
  107. LarryRuane referenced this in commit f983f85c04 on Mar 22, 2021
  108. LarryRuane referenced this in commit 8ce5c189ee on Mar 22, 2021
  109. LarryRuane referenced this in commit aaa5482233 on Apr 26, 2021
  110. LarryRuane referenced this in commit da9b866b9e on Apr 26, 2021
  111. LarryRuane referenced this in commit 5829056837 on Apr 26, 2021
  112. LarryRuane referenced this in commit a3e7c0d5a3 on Apr 26, 2021
  113. rebroad commented at 10:23 AM on May 11, 2021: contributor

    @promag What's the reason for avoiding mapBlockIndex lookups?

  114. promag commented at 12:05 PM on May 11, 2021: member

    @rebroad it avoids duplicate lookups - its unnecessary work.

  115. LarryRuane referenced this in commit 95fb09c964 on May 27, 2021
  116. LarryRuane referenced this in commit e9f7ea019d on May 27, 2021
  117. LarryRuane referenced this in commit 643504c9b2 on May 27, 2021
  118. LarryRuane referenced this in commit 297ab4cbe3 on May 27, 2021
  119. str4d referenced this in commit 6018014021 on Sep 23, 2021
  120. str4d referenced this in commit 0335203149 on Sep 23, 2021
  121. str4d referenced this in commit ad9cd8296d on Sep 23, 2021
  122. str4d referenced this in commit 75093609a2 on Sep 23, 2021
  123. gades referenced this in commit d5ccf58591 on Mar 13, 2022
  124. gades referenced this in commit e7d403fb94 on Mar 22, 2022
  125. gades referenced this in commit 68a775c20c on Mar 22, 2022
  126. str4d referenced this in commit d2b4978589 on May 14, 2022
  127. str4d referenced this in commit 192f27472a on May 14, 2022
  128. str4d referenced this in commit 2895116ca4 on May 14, 2022
  129. str4d referenced this in commit f64b70bdac on May 14, 2022
  130. DrahtBot locked this on Aug 18, 2022
Contributors
promagjonasschnelliTheBlueMattlaanwjmeshcolliderpracticalswiftryanofskykallewoofjimpoMarcoFalkerebroad
Labels
BugRefactoring
Linked (view graph)
#12681 Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER#12802 Colonel Mustard in the Lounge with the Lead Pipe (killed mapBlockIndex outside of validation)
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me