Fix block-connection performance regression #9014

pull TheBlueMatt wants to merge 6 commits into bitcoin:master from TheBlueMatt:2016-10-fix-tx-regression changing 7 files +60 −40
  1. TheBlueMatt commented at 3:32 PM on October 25, 2016: member

    @JeremyRubin found a performance regression introduced by b3b3c2a5623d5c942d2b3565cc2d833c65105555 which causes us to make copies of every transaction we connect to tip a while back, but no one had gotten around to fixing it yet. By switching to using shared_ptrs for blocks in several places we not only fix the performance regression but open up the possibility of switching GetMainSignals() callbacks to background threads much more easily.

  2. in src/main.cpp:None in d0f3b60785 outdated 
    2798 | @@ -2799,20 +2799,40 @@ static int64_t nTimeFlush = 0;
2799 |  static int64_t nTimeChainState = 0;
2800 |  static int64_t nTimePostConnect = 0;
2801 |  
2802 | +struct CompareBlockIndexByWork {
2803 | +    bool operator()(const CBlockIndex* a, const CBlockIndex* b) const {
2804 | +        return a->nChainWork < b->nChainWork;
    sipa commented at 3:35 PM on October 25, 2016:

    Don't you need a fallback for when the chainwork is identical?

    TheBlueMatt commented at 4:16 PM on October 25, 2016:

    Since its only used in each ActivateBestChainStep call individually, there is only ever one thing per given height/total work, but the map came out of an older version of the code and is now useless...so I'm replacing it with a vector.

  3. in src/main.cpp:None in d0f3b60785 outdated 
    2830 | -    if (!pblock) {
2831 | -        if (!ReadBlockFromDisk(block, pindexNew, chainparams.GetConsensus()))
2832 | +    const CBlock *pblock;
2833 | +    auto it = blockTrace.mapBlocks.find(pindexNew);
2834 | +    if (it == blockTrace.mapBlocks.end()) {
2835 | +        CBlock *pblockNew = new CBlock();
    JeremyRubin commented at 4:15 PM on October 25, 2016:

    nit: maybe do this as

    it = blockTrace.mapBlocks.emplace(pindexNew, std::shared_ptr<const CBlock>(new CBlock())).first;

    Or

    auto pblockNew = std::make_shared<CBlock>();

    and update following code accordingly

    TheBlueMatt commented at 6:29 PM on October 25, 2016:

    The new version should be cleaner.

  4. in src/main.cpp:None in d0f3b60785 outdated 
    6049 | @@ -6023,23 +6050,24 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
6050 |  
6051 |      else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex) // Ignore blocks received while importing
6052 |      {
6053 | -        CBlock block;
6054 | -        vRecv >> block;
6055 | +        CBlock *pblock = new CBlock();
    JeremyRubin commented at 4:50 PM on October 25, 2016:

    See other comment about using make_shared

  5. in src/rpc/mining.cpp:None in d0f3b60785 outdated 
     131 | @@ -132,7 +132,8 @@ UniValue generateBlocks(boost::shared_ptr<CReserveScript> coinbaseScript, int nG
 132 |              continue;
 133 |          }
 134 |          CValidationState state;
 135 | -        if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL))
 136 | +        std::shared_ptr<const CBlock> shared_pblock(new CBlock(*pblock));
    JeremyRubin commented at 4:51 PM on October 25, 2016:

    make_shared

  6. in src/rpc/mining.cpp:None in d0f3b60785 outdated 
     726 | @@ -726,7 +727,9 @@ UniValue submitblock(const JSONRPCRequest& request)
 727 |              + HelpExampleRpc("submitblock", "\"mydata\"")
 728 |          );
 729 |  
 730 | -    CBlock block;
 731 | +    CBlock* blockptr = new CBlock();
    JeremyRubin commented at 4:51 PM on October 25, 2016:

    make_shared

  7. in src/test/miner_tests.cpp:None in d0f3b60785 outdated 
     222 | @@ -223,7 +223,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 223 |          pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
 224 |          pblock->nNonce = blockinfo[i].nonce;
 225 |          CValidationState state;
 226 | -        BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL));
 227 | +        std::shared_ptr<const CBlock> shared_pblock(new CBlock(*pblock));
    JeremyRubin commented at 4:52 PM on October 25, 2016:

    make_shared

  8. in src/test/test_bitcoin.cpp:None in d0f3b60785 outdated 
     126 | @@ -127,7 +127,8 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>&
 127 |      while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
 128 |  
 129 |      CValidationState state;
 130 | -    ProcessNewBlock(state, chainparams, NULL, &block, true, NULL);
 131 | +    std::shared_ptr<const CBlock> shared_pblock(new CBlock(block));
    JeremyRubin commented at 4:52 PM on October 25, 2016:

    make_shared

  9. in src/main.cpp:None in d0f3b60785 outdated 
    3765 | @@ -3740,7 +3766,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha
3766 |      return true;
3767 |  }
3768 |  
3769 | -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp)
3770 | +bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, std::shared_ptr<const CBlock>& pblock, bool fForceProcessing, const CDiskBlockPos* dbp)
    JeremyRubin commented at 4:58 PM on October 25, 2016:

    Semantics of passing shared_ptr by reference are a bit wonky. I think it is correct in this use case though. Perhaps it would be better for maintainability to eat the performance cost of an atomic increment and pass by value, or pass by const reference (if possible?).

    sipa commented at 1:32 AM on October 26, 2016:

    In my opinion, passing by reference is preferred at any time when you're sure the shared_ptr itself (not the object it points to) is only accessed from a single thread. The cost of the atomic increment is negligable here though, so I don't care in this particular case.

  10. in src/main.cpp:None in d0f3b60785 outdated 
    5854 | @@ -5829,7 +5855,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5855 |          BlockTransactions resp;
5856 |          vRecv >> resp;
5857 |  
5858 | -        CBlock block;
5859 | +        CBlock *pblock = new CBlock();
    JeremyRubin commented at 4:58 PM on October 25, 2016:

    make_shared

    TheBlueMatt commented at 5:13 PM on October 25, 2016:

    The shared_ptrs are to a const CBlock, and IIRC you cant blindly cast a shared_ptr<CBlock> to a shared_ptr<const CBlock>, but you need to access the block as non-const, and I kinda prefer this over a const_cast.

    JeremyRubin commented at 5:32 PM on October 25, 2016:

    implicit copy-cast should work...

    #include <memory>
int main() {
    auto a = std::make_shared<int>(10);
    std::shared_ptr<const int> b = a;
}
    TheBlueMatt commented at 6:06 PM on October 25, 2016:

    Cool, didnt realize that was a thing!

  11. in src/main.cpp:None in d0f3b60785 outdated 
    2808 | +// Used in ActivateBestChain/ConnectBlock to keep track of blocks connected
2809 | +// and notify listeners in the right order.
2810 | +// Note that since mapBlocks is sorted lowest-work to highest-work, iterating
2811 | +// over it will give you the order in which the blocks were connected
2812 | +struct BlockConnectTrace {
2813 | +    std::map<CBlockIndex*, std::shared_ptr<const CBlock>, CompareBlockIndexByWork> mapBlocks;
    JeremyRubin commented at 5:06 PM on October 25, 2016:

    I think it may be worthwhile for you to make this a class and keep the implementation details hidden...

    TheBlueMatt commented at 6:29 PM on October 25, 2016:

    I added txConflicted here to make diffs of individual commits easier, which now requires a lot of access to the contents directly :/ Not sure its worth it.

  12. JeremyRubin commented at 5:10 PM on October 25, 2016: contributor

    utACK

    couple things that could be cleaned up, but I think this is the right approach for this and is OK as is, certainly better than txChanged.

  13. TheBlueMatt force-pushed on Oct 25, 2016
  14. TheBlueMatt commented at 6:30 PM on October 25, 2016: member

    OK, ended up rewriting the PR quite a bit, cleaning it up and resolving the nits folks had raised - it now no longer has this crazy map thing that can be used as both a paramter and a return value.

  15. TheBlueMatt force-pushed on Oct 25, 2016
  16. TheBlueMatt force-pushed on Oct 25, 2016
  17. TheBlueMatt force-pushed on Oct 25, 2016
  18. JeremyRubin commented at 7:26 PM on October 25, 2016: contributor

    re-utack, much cleaner!

  19. TheBlueMatt force-pushed on Oct 25, 2016
  20. in src/main.cpp:None in 34d0c884d6 outdated 
    2798 | @@ -2799,11 +2799,18 @@ static int64_t nTimeFlush = 0;
2799 |  static int64_t nTimeChainState = 0;
2800 |  static int64_t nTimePostConnect = 0;
2801 |  
2802 | +// Used to track conflicted transactions removed from mempool and transactions
    sipa commented at 1:06 AM on October 26, 2016:

    Can you use a doxygen compatible comment here?

  21. in src/main.cpp:None in a0d30f80f3 outdated 
    2974 | @@ -2974,6 +2975,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c
2975 |                      state = CValidationState();
2976 |                      fInvalidFound = true;
2977 |                      fContinue = false;
2978 | +                    // If we didn't actually connect the block, don't notify listeners about it
    sipa commented at 1:17 AM on October 26, 2016:

    This logic needs to be duplicated in the disconnect logic above. It's possible that an invalid block is received which triggers a reorg. So we start from chain A-B, and receive A-C-D, with D invalid. We'll first disconnect B, then successfully connect C, but fail when connecting D. We'll loop back then, disconnect C, and reconnect B. Your code will (I believe) report both C and B as newly connected, while it should be nothing at all.

    Perhaps it's easier to do a filtering step in ActivateBestChain that removes the errorneously blocks in the trace.

    TheBlueMatt commented at 3:22 AM on October 26, 2016:

    Hmm? I dont believe so? Note that the callbacks in question are called from ABC after each ABCS call, and might happen for blocks which will be reorged out in the next ABCS call (this is the existing behavior).

    sipa commented at 4:44 AM on November 23, 2016:

    Ok (answered elsewhere).

  22. in src/main.cpp:None in 051b9968cf outdated 
    2805 | @@ -2806,11 +2806,13 @@ struct ConnectTrace {
2806 |      std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
2807 |  };
2808 |  
2809 | +
    sipa commented at 1:22 AM on October 26, 2016:

    Unintentional new lines?

  23. in src/main.cpp:None in 5095970c97 outdated 
    5840 | @@ -5846,7 +5841,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5841 |          BlockTransactions resp;
5842 |          vRecv >> resp;
5843 |  
5844 | -        CBlock block;
5845 | +        CBlock *pblock = new CBlock();
5846 | +        std::shared_ptr<const CBlock> shared_pblock(pblock); // pblock will be delted by the shared_ptr
    sipa commented at 3:07 AM on October 26, 2016:

    Typo: deleted

  24. TheBlueMatt force-pushed on Oct 26, 2016
  25. in src/main.cpp:None in 3f78bc176a outdated 
    2975 |                      if (!state.CorruptionPossible())
2976 |                          InvalidChainFound(vpindexToConnect.back());
2977 |                      state = CValidationState();
2978 |                      fInvalidFound = true;
2979 |                      fContinue = false;
2980 | +                    // If we didn't actually connect the block, don't notify listeners about it
    sipa commented at 4:27 AM on October 26, 2016:

    No, cs_main is held during the entirety of the attempted reorg here, and the callbacks only fire after the whole thing (I think).

    TheBlueMatt commented at 2:24 PM on October 26, 2016:

    I'm very confused. ActivateBestChain has always locked cs_main, called ActivateBestChainStep once, and then unlocked cs_main and made callbacks before repeating. In the old code we pushed transactions to update onto the callback queue at the end of ConnectTip and never removed them (since there will never be a DisconnectTip call within the same cs_main in ActivateBestChainStep since all DisconnectTips happen before ConnectTip).

    sipa commented at 5:21 PM on October 26, 2016:

    It seems that failed reorgs are an exception to what I said before, so ignore what I said. I think we should fix this (it could lead to GBT working on the forking point of a failed reorg), but that's outside of the scope here.

  26. laanwj added the label Validation on Oct 26, 2016
  27. sipa commented at 3:40 PM on October 26, 2016: member

    Before headers first, reorgs were dealt with by accumulating the changes in another cache level, which was only flushed if the reorg succeeded.

    Now there is much less a concept of reorgs, and only attempts to improve the tip. ABC finds what block we want and like to be the tip, and calls ABCS to make progress towards that. In case of failure, we find what other block we'd like to become the tip (which may be the previous one), and make progress towards that. cs_main is only released once we're in a strictly better state than what we started off in, as otherwise you'd briefly expose the forking point to GBT during a reorg.

    EDIT: see #9014 (review) - I don't think there is an issue for this PR.

  28. in src/main.cpp:None in 3f78bc176a outdated 
    2821 | -    CBlock block;
2822 |      if (!pblock) {
2823 | -        if (!ReadBlockFromDisk(block, pindexNew, chainparams.GetConsensus()))
2824 | +        std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
2825 | +        connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew);
2826 | +        if (!ReadBlockFromDisk(*pblockNew, pindexNew, chainparams.GetConsensus()))
    sipa commented at 5:29 PM on October 26, 2016:

    Ultranit: if you swap this line with the one above, you can pass std::move(pblockNew) to emplace_back, avoiding an atomic increment + decrement when pblockNew goes out of scope.

    TheBlueMatt commented at 6:20 PM on October 26, 2016:

    No, because pblockNew is a std::shared_ptr<CBlock> not an std::shared_ptr<const CBlock>, so you must call the std::shared_ptr constructor to convert it.

    sipa commented at 6:28 PM on October 26, 2016:

    I see!

    JeremyRubin commented at 6:49 PM on October 26, 2016:

    (if anyone else gets confused, Matt meant std::shared_ptr not std::atomic above)

  29. sipa commented at 5:57 PM on October 26, 2016: member

    utACK 3f78bc176a3c612f320bce4516aadcd6234c0850. My nit is just a suggestion.

  30. sipa commented at 10:26 PM on November 7, 2016: member

    Needs rebase.

  31. TheBlueMatt force-pushed on Nov 7, 2016
  32. TheBlueMatt commented at 10:48 PM on November 7, 2016: member

    Rebased.

  33. fanquake commented at 12:29 AM on November 8, 2016: member

    Travis failures:

      CXX      libbitcoin_server_a-rest.o
../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());

      CXX      libbitcoin_server_a-torcontrol.o
../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());

    ../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());
  34. paveljanik commented at 7:14 AM on November 8, 2016: contributor

    Looks like this is not rebased correctly - GH shows 5 conflicting files (and they match my own tests).

  35. TheBlueMatt commented at 1:19 AM on November 9, 2016: member

    It just needs rebased again, closing for now and will rebase on top of #9075 after that gets merged.

  36. TheBlueMatt closed this on Nov 9, 2016
  37. TheBlueMatt reopened this on Nov 17, 2016
  38. TheBlueMatt force-pushed on Nov 17, 2016
  39. TheBlueMatt commented at 11:28 PM on November 17, 2016: member

    Rebased on #9075.

  40. TheBlueMatt force-pushed on Nov 22, 2016
  41. TheBlueMatt commented at 2:51 AM on November 22, 2016: member

    Rebased after #9125 was merged. Note that #9125 should have fixed most of the performance regression here (and was likely the source of its major reorg speedup, though it should help in more real-world conditions as well), though this should still help some, and is a good step towards #9027 as well as improving callback concurrency in the future.

  42. TheBlueMatt force-pushed on Nov 23, 2016
  43. in src/main.cpp:None in 7b717cdf7f outdated 
    3126 |          // ... and about transactions that got confirmed:
3127 | -        for (unsigned int i = 0; i < txChanged.size(); i++)
3128 | -            GetMainSignals().SyncTransaction(*std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i]));
3129 | +        for (const auto& pair : connectTrace.blocksConnected) {
3130 | +            assert(pair.second);
3131 | +            const CBlock& block = *(pair.second);
    sipa commented at 5:01 AM on November 23, 2016:

    This loop could be rewritten as

    for (const auto& ptx : pair.second->vtx)
    GetMainSignals().SyncTransaction(*ptx, pair.first, i);
  44. sipa commented at 5:02 AM on November 23, 2016: member

    utACK

  45. gmaxwell commented at 8:59 PM on November 24, 2016: contributor

    utACK

  46. in src/main.cpp:None in eb33aa1412 outdated 
    3018 | @@ -3018,6 +3019,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c
3019 |                      state = CValidationState();
3020 |                      fInvalidFound = true;
3021 |                      fContinue = false;
3022 | +                    // If we didn't actually connect the block, don't notify listeners about it
3023 | +                    connectTrace.blocksConnected.pop_back();
    sdaftuar commented at 2:13 AM on November 29, 2016:

    nit: slightly weird that the caller of ConnectTip() has to know to do this on failures. Perhaps document this somewhere?

    TheBlueMatt commented at 5:26 AM on November 30, 2016:

    Fixed

  47. sdaftuar commented at 2:21 AM on November 29, 2016: member

    Code review ACK, will test.

  48. sdaftuar commented at 10:33 PM on November 29, 2016: member

    ACK 7b717cd

  49. Add struct to track block-connect-time-generated info for callbacks 6fdd43b968
  50. Keep blocks as shared_ptrs, instead of copying txn in ConnectTip fd9d89070a
  51. Create a shared_ptr for the block we're connecting in ActivateBCS ae4db44d03
  52. Make the optional pblock in ActivateBestChain a shared_ptr 2736c44c8e
  53. Switch pblock in ProcessNewBlock to a shared_ptr
    This (finally) fixes a performance regression in
b3b3c2a5623d5c942d2b3565cc2d833c65105555
    2d6e5619af
  54. Document ConnectBlock connectTrace postconditions dd0df81ebd
  55. TheBlueMatt force-pushed on Dec 4, 2016
  56. TheBlueMatt commented at 8:21 AM on December 4, 2016: member

    Rebased after #9260.

  57. reinier1991 approved
  58. laanwj merged this on Dec 5, 2016
  59. laanwj closed this on Dec 5, 2016
  60. laanwj referenced this in commit d04aebaec7 on Dec 5, 2016
  61. codablock referenced this in commit c4d15c113a on Jan 16, 2018
  62. codablock referenced this in commit fdbb9b6501 on Jan 16, 2018
  63. codablock referenced this in commit b5389c09b3 on Jan 17, 2018
  64. gladcow referenced this in commit 59ec8eafa9 on Mar 5, 2018
  65. gladcow referenced this in commit ad83a65b02 on Mar 8, 2018
  66. gladcow referenced this in commit 60f18b7806 on Mar 13, 2018
  67. gladcow referenced this in commit aea4ba0e85 on Mar 14, 2018
  68. gladcow referenced this in commit 9fe17f4666 on Mar 15, 2018
  69. gladcow referenced this in commit a14a312856 on Mar 15, 2018
  70. gladcow referenced this in commit 37aa7dc04f on Mar 15, 2018
  71. gladcow referenced this in commit 1f5b5c562a on Mar 15, 2018
  72. gladcow referenced this in commit f3a4879534 on Mar 24, 2018
  73. gladcow referenced this in commit ca694e861a on Apr 4, 2018
  74. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  75. andvgal referenced this in commit c8b7bc1a78 on Jan 6, 2019
  76. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  77. CryptoCentric referenced this in commit b13b4f0860 on Feb 25, 2019
  78. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  79. furszy referenced this in commit 8a9b92c0ed on Oct 17, 2020
  80. DrahtBot locked this on Sep 8, 2021
Contributors
TheBlueMattsipaJeremyRubinfanquakepaveljanikgmaxwellsdaftuarreinier1991
Labels
Validation
Linked (view graph)
#9027 Unbounded reorg memory usage#9273 Remove unused CDiskBlockPos* argument from ProcessNewBlock
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-22 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me