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-
jonasschnelli commented at 12:03 pm on April 26, 2016: contributorDisconnectTip and AcceptToMempool are still holding cs_main (not intended to improve this in this PR)
-
jonasschnelli added the label Refactoring on Apr 26, 2016
-
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. -
jonasschnelli force-pushed on Apr 26, 2016
-
jonasschnelli force-pushed on Apr 26, 2016
-
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 theSyncTransaction
signal and addedCBlockIndex*
together withint posInBlock
. - Updated the wallet logic that it can handle the slightly different signal.
-
jonasschnelli force-pushed on Apr 26, 2016
-
jonasschnelli force-pushed on Apr 26, 2016
-
kazcw commented at 2:17 pm on April 27, 2016: contributorActivateBestChainStep reuses a vector for ConnectTip’s txConflicted to combine results from multiple blocks. ConnectTip passes txConflicted to mempoool.removeForBlock every time.
-
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 -
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 thei
?
jonasschnelli commented at 9:40 am on May 6, 2016:Right. This loop is stupid at this point. The transaction always have aposInBlock
. Will update. -
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 -
jonasschnelli force-pushed on May 6, 2016
-
jonasschnelli commented at 9:48 am on May 6, 2016: contributor
-
kazcw commented at 1:57 pm on May 6, 2016: contributorYou know what, I misread.
txConflicted
is an out-only parameter all the way down the call stack, so it was fine without thetxConflictedCurrent
. Sorry about that. -
sipa commented at 2:18 pm on May 25, 2016: memberNeeds rebase.
-
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? -
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 thetxConflictedCurrent
. Fixed. -
jonasschnelli force-pushed on May 25, 2016
-
jonasschnelli commented at 2:34 pm on May 25, 2016: contributorRebased, fixed sipas found issue and switched to
std::tuple
-
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 listtxConflicted
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. -
jonasschnelli force-pushed on May 25, 2016
-
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 :)
sipa commented at 8:02 pm on June 2, 2016:
dcousens commented at 0:31 am on June 3, 2016:given the repeated use, maybe justtypedef
thestd::tuple
?tuple
has some nice conveniences over astruct
alternative. -
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. -
jonasschnelli force-pushed on May 25, 2016
-
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. -
sipa commented at 5:22 pm on May 25, 2016: memberutACK fdf8a5efe03004f4a581ba41b87737a4e1f1ff44
-
dcousens commented at 0:32 am on June 3, 2016: contributorutACK fdf8a5e
-
laanwj commented at 1:32 pm on June 10, 2016: memberutACK fdf8a5e, but the squashme commits need squashing
-
jonasschnelli force-pushed on Jun 10, 2016
-
jonasschnelli commented at 3:09 pm on June 10, 2016: contributorSquashed.
-
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 beif (posInBlock != -1) {
instead? pindex is always passed, even when the transaction was removed. -
jonasschnelli force-pushed on Jul 20, 2016
-
jonasschnelli commented at 12:37 pm on July 20, 2016: contributorSquashed the squashme commit. Anything holding this back?
-
jtimon commented at 7:29 pm on July 20, 2016: contributorDecide 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
-
laanwj commented at 9:56 am on August 3, 2016: memberNeeds rebase.
-
Reduce cs_main locks during ConnectTip/SyncWithWallets b3b3c2a562
-
jonasschnelli force-pushed on Aug 12, 2016
-
jonasschnelli commented at 12:54 pm on August 12, 2016: contributorRebased.
-
sipa merged this on Aug 15, 2016
-
sipa closed this on Aug 15, 2016
-
sipa referenced this in commit d727f77e39 on Aug 15, 2016
-
TheBlueMatt referenced this in commit 3127935b54 on Jan 19, 2017
-
TheBlueMatt referenced this in commit f73bd2c370 on Jan 19, 2017
-
TheBlueMatt referenced this in commit 989989354b on Jan 19, 2017
-
TheBlueMatt referenced this in commit e97b94fc67 on Jan 20, 2017
-
TheBlueMatt referenced this in commit 14dece1801 on Jan 20, 2017
-
TheBlueMatt referenced this in commit a07db1f0fb on Jan 20, 2017
-
TheBlueMatt referenced this in commit b9c04b76a3 on Jan 20, 2017
-
TheBlueMatt referenced this in commit e5802a1864 on Jan 20, 2017
-
TheBlueMatt referenced this in commit 402c3368f8 on Jan 20, 2017
-
TheBlueMatt referenced this in commit dda1f3c78c on Jan 23, 2017
-
laanwj referenced this in commit eafba4e273 on Jan 23, 2017
-
TheBlueMatt referenced this in commit 80ac497c9a on Jan 23, 2017
-
TheBlueMatt referenced this in commit 997f1d8643 on Jan 25, 2017
-
TheBlueMatt referenced this in commit acd7b69d15 on Feb 8, 2017
-
TheBlueMatt referenced this in commit a61a6de7ce on Feb 8, 2017
-
TheBlueMatt referenced this in commit 1eb9e4cbb0 on Feb 8, 2017
-
TheBlueMatt referenced this in commit e1c2b46e1c on Feb 18, 2017
-
TheBlueMatt referenced this in commit 57ad383265 on Feb 18, 2017
-
TheBlueMatt referenced this in commit c5e8a0b92a on Mar 6, 2017
-
TheBlueMatt referenced this in commit 6266b75cc0 on Mar 7, 2017
-
TheBlueMatt referenced this in commit dcfd01cf1c on Mar 8, 2017
-
TheBlueMatt referenced this in commit 647bd58b95 on Apr 10, 2017
-
TheBlueMatt referenced this in commit 76856f237d on Apr 17, 2017
-
TheBlueMatt referenced this in commit ea402b2097 on Apr 26, 2017
-
TheBlueMatt referenced this in commit fac8c1f0ad on Apr 26, 2017
-
TheBlueMatt referenced this in commit a3e3a757e4 on Apr 26, 2017
-
TheBlueMatt referenced this in commit 31091c5cfb on Apr 26, 2017
-
TheBlueMatt referenced this in commit b6a3a6eecf on Apr 26, 2017
-
TheBlueMatt referenced this in commit ba1890ec30 on Apr 26, 2017
-
TheBlueMatt referenced this in commit 7da495a80a on Apr 26, 2017
-
TheBlueMatt referenced this in commit 7407d36589 on Apr 26, 2017
-
TheBlueMatt referenced this in commit 36b1a253d2 on Apr 27, 2017
-
TheBlueMatt referenced this in commit e0681b45f2 on Apr 27, 2017
-
TheBlueMatt referenced this in commit 4115319fe1 on Apr 27, 2017
-
TheBlueMatt referenced this in commit b7317f8b8e on May 3, 2017
-
TheBlueMatt referenced this in commit c2b16b9a30 on May 4, 2017
-
TheBlueMatt referenced this in commit 2f1a5e82b5 on May 5, 2017
-
TheBlueMatt referenced this in commit 48689173e9 on Jun 8, 2017
-
TheBlueMatt referenced this in commit 0a81eaeaa6 on Jun 8, 2017
-
TheBlueMatt referenced this in commit 699fb26fdd on Jun 8, 2017
-
TheBlueMatt referenced this in commit f17a525765 on Jun 9, 2017
-
TheBlueMatt referenced this in commit 1f3234a4e7 on Jun 9, 2017
-
TheBlueMatt referenced this in commit 5230dedad5 on Jun 21, 2017
-
TheBlueMatt referenced this in commit 547223bffd on Jun 21, 2017
-
TheBlueMatt referenced this in commit f10d4a3c58 on Jun 21, 2017
-
TheBlueMatt referenced this in commit 0c457ac81e on Jun 21, 2017
-
TheBlueMatt referenced this in commit 3879786467 on Jul 7, 2017
-
TheBlueMatt referenced this in commit 417066ed81 on Jul 7, 2017
-
TheBlueMatt referenced this in commit 62e416d714 on Jul 11, 2017
-
TheBlueMatt referenced this in commit 2b24e583d1 on Aug 14, 2017
-
TheBlueMatt referenced this in commit 2a41f6aed6 on Aug 17, 2017
-
TheBlueMatt referenced this in commit 4b6a0c9aad on Sep 12, 2017
-
jnewbery referenced this in commit e28cb7409e on Sep 13, 2017
-
codablock referenced this in commit a467a0ff53 on Sep 19, 2017
-
TheBlueMatt referenced this in commit 63afb38340 on Oct 1, 2017
-
TheBlueMatt referenced this in commit 51ca6003ca on Oct 1, 2017
-
TheBlueMatt referenced this in commit e6e43a85c8 on Oct 13, 2017
-
TheBlueMatt referenced this in commit 2960f2f22a on Oct 13, 2017
-
TheBlueMatt referenced this in commit e545dedf72 on Oct 13, 2017
-
laanwj referenced this in commit 927a1d7d08 on Nov 15, 2017
-
codablock referenced this in commit da704c58b5 on Dec 29, 2017
-
codablock referenced this in commit 256b9b77a2 on Jan 8, 2018
-
codablock referenced this in commit 8420dd2073 on Jan 19, 2018
-
codablock referenced this in commit 5f211ce25f on Jan 20, 2018
-
codablock referenced this in commit 9ba8a31d03 on Jan 21, 2018
-
HashUnlimited referenced this in commit 50ade5feeb on Mar 13, 2018
-
andvgal referenced this in commit 818ffd8f72 on Jan 6, 2019
-
andvgal referenced this in commit 56ce909b20 on Jan 6, 2019
-
CryptoCentric referenced this in commit 9ee01b7423 on Feb 27, 2019
-
PastaPastaPasta referenced this in commit 61758874b8 on Feb 13, 2020
-
PastaPastaPasta referenced this in commit 76ed385c03 on Feb 13, 2020
-
PastaPastaPasta referenced this in commit 3eb80fdcd5 on Feb 29, 2020
-
PastaPastaPasta referenced this in commit 803530c83d on Feb 29, 2020
-
random-zebra referenced this in commit 8e19562dc4 on Aug 5, 2020
-
furszy referenced this in commit a4738c5301 on Feb 10, 2021
-
furszy referenced this in commit aec5755671 on Feb 18, 2021
-
furszy referenced this in commit a0b8ac4ea9 on Feb 19, 2021
-
furszy referenced this in commit c7ab4901f1 on Feb 21, 2021
-
ckti referenced this in commit 38a9ad2b41 on Mar 28, 2021
-
gades referenced this in commit 1459aa5a72 on Jun 30, 2021
-
MarcoFalke locked this on Sep 8, 2021