Reduce cs_main locks during ConnectTip/SyncWithWallets #7946

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/04/cs_main_wallet changing 7 files +49 −54
  1. jonasschnelli commented at 12:03 pm on April 26, 2016: contributor
    DisconnectTip and AcceptToMempool are still holding cs_main (not intended to improve this in this PR)
  2. jonasschnelli added the label Refactoring on Apr 26, 2016
  3. in src/main.cpp: in 480ed4f30f outdated 
    2917+        // while _not_ holding the cs_main lock
2918+        BOOST_FOREACH(const CTransaction &tx, txConflicted) {
2919+            SyncWithWallets(tx, pindexNewTip, NULL);
2920+        }
2921+        // ... and about transactions that got confirmed:
2922+        BOOST_FOREACH(const CTransaction &tx, pblock->vtx) {
    sipa commented at 12:05 pm on April 26, 2016:
    This is not correct, it needs to iterate through all the blocks that got connected, not just the new tip.
  4. jonasschnelli force-pushed on Apr 26, 2016
  5. jonasschnelli force-pushed on Apr 26, 2016
  6. jonasschnelli commented at 1:42 pm on April 26, 2016: contributor

    As always when you want to improve the locking a little bit, the change gets quickly more invasive.

    Update serval things:

    • Fixed @sipa point (iterate though all connected blocks).
    • Removed the CBlock * parameter from the SyncTransaction signal and added CBlockIndex* together with int posInBlock.
    • Updated the wallet logic that it can handle the slightly different signal.
  7. jonasschnelli force-pushed on Apr 26, 2016
  8. jonasschnelli force-pushed on Apr 26, 2016
  9. kazcw commented at 2:17 pm on April 27, 2016: contributor
    ActivateBestChainStep reuses a vector for ConnectTip’s txConflicted to combine results from multiple blocks. ConnectTip passes txConflicted to mempoool.removeForBlock every time.
  10. in src/main.cpp: in 086654e4fd outdated 
    2926+        for(unsigned int i = 0; i < txChanged.size(); i++)
2927+            SyncWithWallets(txChanged[i].get<0>(), txChanged[i].get<1>(), txChanged[i].get<2>());
2928+
2929         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
2930 
2931         // Notifications/callbacks that can run without cs_main
    kazcw commented at 2:19 pm on April 27, 2016:
    Could put the new loops below this comment with the other non-cs_main notifications
  11. in src/wallet/wallet.cpp: in 086654e4fd outdated 
    1223@@ -1224,7 +1224,14 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate)
1224             ReadBlockFromDisk(block, pindex, Params().GetConsensus());
1225             BOOST_FOREACH(CTransaction& tx, block.vtx)
    kazcw commented at 2:33 pm on April 27, 2016:
    Why not just change this loop to be numeric and then use the i?
    jonasschnelli commented at 9:40 am on May 6, 2016:
    Right. This loop is stupid at this point. The transaction always have a posInBlock. Will update.
  12. in src/wallet/wallet.cpp: in 086654e4fd outdated 
    3309     // Is the tx in a block that's in the main chain
3310-    BlockMap::iterator mi = mapBlockIndex.find(hashBlock);
3311-    if (mi == mapBlockIndex.end())
3312-        return 0;
3313-    const CBlockIndex* pindex = (*mi).second;
3314     if (!pindex || !chainActive.Contains(pindex))
    kazcw commented at 2:39 pm on April 27, 2016:
    Could remove the null-check; pindex has already been dereferenced
  13. jonasschnelli force-pushed on May 6, 2016
  14. jonasschnelli commented at 9:48 am on May 6, 2016: contributor

    @kazcw Thanks for the detailed code review!

    Added a squashme commit with fixed for issues found by @kazcw.

  15. kazcw commented at 1:57 pm on May 6, 2016: contributor
    You know what, I misread. txConflicted is an out-only parameter all the way down the call stack, so it was fine without the txConflictedCurrent. Sorry about that.
  16. sipa commented at 2:18 pm on May 25, 2016: member
    Needs rebase.
  17. in src/main.cpp: in e8ea3dd5ea outdated 
    43@@ -44,6 +44,7 @@
44 #include <boost/filesystem/fstream.hpp>
45 #include <boost/math/distributions/poisson.hpp>
46 #include <boost/thread.hpp>
47+#include <boost/tuple/tuple.hpp>
    sipa commented at 2:19 pm on May 25, 2016:
    Use std::tuple instead?
  18. in src/main.cpp: in e8ea3dd5ea outdated 
    2723@@ -2723,19 +2724,14 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2724     int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
2725     LogPrint("bench", "  - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001);
2726     // Remove conflicting transactions from the mempool.
2727-    list<CTransaction> txConflicted;
2728+    list<CTransaction> txConflictedCurrent;
2729     mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload());
2730+    txConflicted.insert(txConflicted.end(), txConflictedCurrent.begin(), txConflictedCurrent.end());
    sipa commented at 2:22 pm on May 25, 2016:
    This looks like it is not doing anything, as txConflictedCurrent will always be empty?
    jonasschnelli commented at 2:34 pm on May 25, 2016:
    Oh. Right. L2728 should make use of the txConflictedCurrent. Fixed.
  19. jonasschnelli force-pushed on May 25, 2016
  20. jonasschnelli commented at 2:34 pm on May 25, 2016: contributor
    Rebased, fixed sipas found issue and switched to std::tuple
  21. in src/main.cpp: in 9471f32a8f outdated 
    2716@@ -2717,19 +2717,14 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2717     int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4;
2718     LogPrint("bench", "  - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001);
2719     // Remove conflicting transactions from the mempool.
2720-    list<CTransaction> txConflicted;
2721-    mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload());
2722+    list<CTransaction> txConflictedCurrent;
2723+    mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflictedCurrent, !IsInitialBlockDownload());
2724+    txConflicted.insert(txConflicted.end(), txConflictedCurrent.begin(), txConflictedCurrent.end());
    sipa commented at 2:36 pm on May 25, 2016:
    Why a temporary variable, and not just pass vchConflicted to mempool.removeForBlock()? txConflictedCurrent is not used elsewhere.
    jonasschnelli commented at 2:39 pm on May 25, 2016:

    Without a temporary variable, mempool.removeForBlock would always work though the combined list txConflicted that was initialized here.

    Although I’m not sure if this would be a problem, but at least it would slightly change current masters behavior.

    sipa commented at 2:44 pm on May 25, 2016:
    “Work through”? All that function does is append deleted CTransactions to it.
    jonasschnelli commented at 2:48 pm on May 25, 2016:
    Yes. Your right. Removed the temp. var.
  22. jonasschnelli force-pushed on May 25, 2016
  23. in src/main.cpp: in 580a665a97 outdated 
    2911@@ -2919,6 +2912,8 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2912 
2913         CBlockIndex *pindexNewTip = NULL;
2914         const CBlockIndex *pindexFork;
2915+        std::list<CTransaction> txConflicted;
2916+        std::vector<std::tuple<CTransaction,CBlockIndex*,int> > txChanged;
    sipa commented at 3:44 pm on May 25, 2016:
    Sorry for changing my mind, but can you define a struct for this instead? Larger tuples don’t really have good readability.
    jonasschnelli commented at 3:57 pm on May 25, 2016:
    IMO tuple is ideal for a such use case. It’s only a temporary variable and its scope is very small. IMO an custom struct seems to be over complex.
    sipa commented at 5:25 pm on May 25, 2016:
    Fair enough.
    laanwj commented at 7:50 pm on June 2, 2016:
    c++ needs collections.namedtuple :)
    dcousens commented at 0:31 am on June 3, 2016:
    given the repeated use, maybe just typedef the std::tuple? tuple has some nice conveniences over a struct alternative.
  24. in src/main.cpp: in 580a665a97 outdated 
    2940@@ -2946,6 +2941,15 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2941         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
2942 
2943         // Notifications/callbacks that can run without cs_main
2944+
2945+        // throw all transactions though the signal-interface
2946+        // while _not_ holding the cs_main lock
2947+        BOOST_FOREACH(const CTransaction &tx, txConflicted)
2948+        SyncWithWallets(tx, pindexNewTip);
    sipa commented at 3:47 pm on May 25, 2016:
    Indentation (and braces).
    jonasschnelli commented at 3:58 pm on May 25, 2016:
    Fixed Indentation and braces.
  25. jonasschnelli force-pushed on May 25, 2016
  26. in src/wallet/wallet.cpp: in fdf8a5efe0 outdated 
    911@@ -912,11 +912,11 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
912     }
913 }
914 
915-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, const CBlock* pblock)
916+void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
917 {
918     LOCK2(cs_main, cs_wallet);
    sipa commented at 5:21 pm on May 25, 2016:
    This cs_main is unfortunate, but I guess we can get rid of it in a follow-up.
  27. sipa commented at 5:22 pm on May 25, 2016: member
    utACK fdf8a5efe03004f4a581ba41b87737a4e1f1ff44
  28. dcousens commented at 0:32 am on June 3, 2016: contributor
    utACK fdf8a5e
  29. laanwj commented at 1:32 pm on June 10, 2016: member
    utACK fdf8a5e, but the squashme commits need squashing
  30. jonasschnelli force-pushed on Jun 10, 2016
  31. jonasschnelli commented at 3:09 pm on June 10, 2016: contributor
    Squashed.
  32. in src/wallet/wallet.cpp: in d31dd19655 outdated 
    762 {
763     {
764         AssertLockHeld(cs_wallet);
765 
766-        if (pblock) {
767+        if (pIndex) {
    sipa commented at 2:28 pm on June 11, 2016:
    Shouldn’t this be if (posInBlock != -1) { instead? pindex is always passed, even when the transaction was removed.
  33. jonasschnelli force-pushed on Jul 20, 2016
  34. jonasschnelli commented at 12:37 pm on July 20, 2016: contributor
    Squashed the squashme commit. Anything holding this back?
  35. jtimon commented at 7:29 pm on July 20, 2016: contributor
    Decide on struct vs typedef of std::tuple<CTransaction,CBlockIndex*,int> (or just leave it as it is now, I guess)? No strong opinion on that. Fast review ACK 0d54074
  36. laanwj commented at 9:56 am on August 3, 2016: member
    Needs rebase.
  37. Reduce cs_main locks during ConnectTip/SyncWithWallets b3b3c2a562
  38. jonasschnelli force-pushed on Aug 12, 2016
  39. jonasschnelli commented at 12:54 pm on August 12, 2016: contributor
    Rebased.
  40. sipa merged this on Aug 15, 2016
  41. sipa closed this on Aug 15, 2016
  42. sipa referenced this in commit d727f77e39 on Aug 15, 2016
  43. TheBlueMatt referenced this in commit 3127935b54 on Jan 19, 2017
  44. TheBlueMatt referenced this in commit f73bd2c370 on Jan 19, 2017
  45. TheBlueMatt referenced this in commit 989989354b on Jan 19, 2017
  46. TheBlueMatt referenced this in commit e97b94fc67 on Jan 20, 2017
  47. TheBlueMatt referenced this in commit 14dece1801 on Jan 20, 2017
  48. TheBlueMatt referenced this in commit a07db1f0fb on Jan 20, 2017
  49. TheBlueMatt referenced this in commit b9c04b76a3 on Jan 20, 2017
  50. TheBlueMatt referenced this in commit e5802a1864 on Jan 20, 2017
  51. TheBlueMatt referenced this in commit 402c3368f8 on Jan 20, 2017
  52. TheBlueMatt referenced this in commit dda1f3c78c on Jan 23, 2017
  53. laanwj referenced this in commit eafba4e273 on Jan 23, 2017
  54. TheBlueMatt referenced this in commit 80ac497c9a on Jan 23, 2017
  55. TheBlueMatt referenced this in commit 997f1d8643 on Jan 25, 2017
  56. TheBlueMatt referenced this in commit acd7b69d15 on Feb 8, 2017
  57. TheBlueMatt referenced this in commit a61a6de7ce on Feb 8, 2017
  58. TheBlueMatt referenced this in commit 1eb9e4cbb0 on Feb 8, 2017
  59. TheBlueMatt referenced this in commit e1c2b46e1c on Feb 18, 2017
  60. TheBlueMatt referenced this in commit 57ad383265 on Feb 18, 2017
  61. TheBlueMatt referenced this in commit c5e8a0b92a on Mar 6, 2017
  62. TheBlueMatt referenced this in commit 6266b75cc0 on Mar 7, 2017
  63. TheBlueMatt referenced this in commit dcfd01cf1c on Mar 8, 2017
  64. TheBlueMatt referenced this in commit 647bd58b95 on Apr 10, 2017
  65. TheBlueMatt referenced this in commit 76856f237d on Apr 17, 2017
  66. TheBlueMatt referenced this in commit ea402b2097 on Apr 26, 2017
  67. TheBlueMatt referenced this in commit fac8c1f0ad on Apr 26, 2017
  68. TheBlueMatt referenced this in commit a3e3a757e4 on Apr 26, 2017
  69. TheBlueMatt referenced this in commit 31091c5cfb on Apr 26, 2017
  70. TheBlueMatt referenced this in commit b6a3a6eecf on Apr 26, 2017
  71. TheBlueMatt referenced this in commit ba1890ec30 on Apr 26, 2017
  72. TheBlueMatt referenced this in commit 7da495a80a on Apr 26, 2017
  73. TheBlueMatt referenced this in commit 7407d36589 on Apr 26, 2017
  74. TheBlueMatt referenced this in commit 36b1a253d2 on Apr 27, 2017
  75. TheBlueMatt referenced this in commit e0681b45f2 on Apr 27, 2017
  76. TheBlueMatt referenced this in commit 4115319fe1 on Apr 27, 2017
  77. TheBlueMatt referenced this in commit b7317f8b8e on May 3, 2017
  78. TheBlueMatt referenced this in commit c2b16b9a30 on May 4, 2017
  79. TheBlueMatt referenced this in commit 2f1a5e82b5 on May 5, 2017
  80. TheBlueMatt referenced this in commit 48689173e9 on Jun 8, 2017
  81. TheBlueMatt referenced this in commit 0a81eaeaa6 on Jun 8, 2017
  82. TheBlueMatt referenced this in commit 699fb26fdd on Jun 8, 2017
  83. TheBlueMatt referenced this in commit f17a525765 on Jun 9, 2017
  84. TheBlueMatt referenced this in commit 1f3234a4e7 on Jun 9, 2017
  85. TheBlueMatt referenced this in commit 5230dedad5 on Jun 21, 2017
  86. TheBlueMatt referenced this in commit 547223bffd on Jun 21, 2017
  87. TheBlueMatt referenced this in commit f10d4a3c58 on Jun 21, 2017
  88. TheBlueMatt referenced this in commit 0c457ac81e on Jun 21, 2017
  89. TheBlueMatt referenced this in commit 3879786467 on Jul 7, 2017
  90. TheBlueMatt referenced this in commit 417066ed81 on Jul 7, 2017
  91. TheBlueMatt referenced this in commit 62e416d714 on Jul 11, 2017
  92. TheBlueMatt referenced this in commit 2b24e583d1 on Aug 14, 2017
  93. TheBlueMatt referenced this in commit 2a41f6aed6 on Aug 17, 2017
  94. TheBlueMatt referenced this in commit 4b6a0c9aad on Sep 12, 2017
  95. jnewbery referenced this in commit e28cb7409e on Sep 13, 2017
  96. codablock referenced this in commit a467a0ff53 on Sep 19, 2017
  97. TheBlueMatt referenced this in commit 63afb38340 on Oct 1, 2017
  98. TheBlueMatt referenced this in commit 51ca6003ca on Oct 1, 2017
  99. TheBlueMatt referenced this in commit e6e43a85c8 on Oct 13, 2017
  100. TheBlueMatt referenced this in commit 2960f2f22a on Oct 13, 2017
  101. TheBlueMatt referenced this in commit e545dedf72 on Oct 13, 2017
  102. laanwj referenced this in commit 927a1d7d08 on Nov 15, 2017
  103. codablock referenced this in commit da704c58b5 on Dec 29, 2017
  104. codablock referenced this in commit 256b9b77a2 on Jan 8, 2018
  105. codablock referenced this in commit 8420dd2073 on Jan 19, 2018
  106. codablock referenced this in commit 5f211ce25f on Jan 20, 2018
  107. codablock referenced this in commit 9ba8a31d03 on Jan 21, 2018
  108. HashUnlimited referenced this in commit 50ade5feeb on Mar 13, 2018
  109. andvgal referenced this in commit 818ffd8f72 on Jan 6, 2019
  110. andvgal referenced this in commit 56ce909b20 on Jan 6, 2019
  111. CryptoCentric referenced this in commit 9ee01b7423 on Feb 27, 2019
  112. PastaPastaPasta referenced this in commit 61758874b8 on Feb 13, 2020
  113. PastaPastaPasta referenced this in commit 76ed385c03 on Feb 13, 2020
  114. PastaPastaPasta referenced this in commit 3eb80fdcd5 on Feb 29, 2020
  115. PastaPastaPasta referenced this in commit 803530c83d on Feb 29, 2020
  116. random-zebra referenced this in commit 8e19562dc4 on Aug 5, 2020
  117. furszy referenced this in commit a4738c5301 on Feb 10, 2021
  118. furszy referenced this in commit aec5755671 on Feb 18, 2021
  119. furszy referenced this in commit a0b8ac4ea9 on Feb 19, 2021
  120. furszy referenced this in commit c7ab4901f1 on Feb 21, 2021
  121. ckti referenced this in commit 38a9ad2b41 on Mar 28, 2021
  122. gades referenced this in commit 1459aa5a72 on Jun 30, 2021
  123. MarcoFalke locked this on Sep 8, 2021


jonasschnelli sipa kazcw laanwj dcousens jtimon

Labels
Refactoring

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: 2025-04-28 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me