Reduce cs_main locks during ConnectTip/SyncWithWallets #7946

jonasschnelli · 2016-04-26T12:03:07Z

DisconnectTip and AcceptToMempool are still holding cs_main (not intended to improve this in this PR)

sipa · 2016-04-26T12:05:52Z

src/main.cpp

+            SyncWithWallets(tx, pindexNewTip, NULL);
+        }
+        // ... and about transactions that got confirmed:
+        BOOST_FOREACH(const CTransaction &tx, pblock->vtx) {


This is not correct, it needs to iterate through all the blocks that got connected, not just the new tip.

jonasschnelli · 2016-04-26T13:42:48Z

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.

kazcw · 2016-04-27T14:17:16Z

ActivateBestChainStep reuses a vector for ConnectTip's txConflicted to combine results from multiple blocks. ConnectTip passes txConflicted to mempoool.removeForBlock every time.

kazcw · 2016-04-27T14:19:52Z

src/main.cpp

+        // ... and about transactions that got confirmed:
+        for(unsigned int i = 0; i < txChanged.size(); i++)
+            SyncWithWallets(txChanged[i].get<0>(), txChanged[i].get<1>(), txChanged[i].get<2>());
+
        // When we reach this point, we switched to a new tip (stored in pindexNewTip).

        // Notifications/callbacks that can run without cs_main


Could put the new loops below this comment with the other non-cs_main notifications

jonasschnelli · 2016-05-06T09:48:09Z

@kazcw Thanks for the detailed code review!

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

kazcw · 2016-05-06T13:57:38Z

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.

sipa · 2016-05-25T14:18:50Z

Needs rebase.

sipa · 2016-05-25T14:19:48Z

src/main.cpp

@@ -44,6 +44,7 @@
 #include <boost/filesystem/fstream.hpp>
 #include <boost/math/distributions/poisson.hpp>
 #include <boost/thread.hpp>
+#include <boost/tuple/tuple.hpp>


Use std::tuple instead?

jonasschnelli · 2016-05-25T14:34:57Z

Rebased, fixed sipas found issue and switched to std::tuple

sipa · 2016-05-25T14:36:24Z

src/main.cpp

-    mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload());
+    list<CTransaction> txConflictedCurrent;
+    mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflictedCurrent, !IsInitialBlockDownload());
+    txConflicted.insert(txConflicted.end(), txConflictedCurrent.begin(), txConflictedCurrent.end());


Why a temporary variable, and not just pass vchConflicted to mempool.removeForBlock()? txConflictedCurrent is not used elsewhere.

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.

"Work through"? All that function does is append deleted CTransactions to it.

Yes. Your right. Removed the temp. var.

sipa · 2016-05-25T15:44:53Z

src/main.cpp

@@ -2919,6 +2912,8 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,

        CBlockIndex *pindexNewTip = NULL;
        const CBlockIndex *pindexFork;
+        std::list<CTransaction> txConflicted;
+        std::vector<std::tuple<CTransaction,CBlockIndex*,int> > txChanged;


Sorry for changing my mind, but can you define a struct for this instead? Larger tuples don't really have good readability.

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.

Fair enough.

c++ needs collections.namedtuple :)

http://www.boost.org/doc/libs/1_58_0/libs/fusion/doc/html/fusion/container/map.html

given the repeated use, maybe just typedef the std::tuple? tuple has some nice conveniences over a struct alternative.

sipa · 2016-05-25T17:21:28Z

src/wallet/wallet.cpp

@@ -912,11 +912,11 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
    }
 }

-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, const CBlock* pblock)
+void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
 {
    LOCK2(cs_main, cs_wallet);


This cs_main is unfortunate, but I guess we can get rid of it in a follow-up.

sipa · 2016-05-25T17:22:04Z

utACK fdf8a5efe03004f4a581ba41b87737a4e1f1ff44

dcousens · 2016-06-03T00:32:29Z

utACK fdf8a5e

laanwj · 2016-06-10T13:32:46Z

utACK fdf8a5e, but the squashme commits need squashing

jonasschnelli · 2016-06-10T15:09:57Z

Squashed.

This runs BlockConnectedDisconnected, TransactionAddedToMempool, SetBestChain, and Inventory on the background scheduler thread. This partially reverts bitcoin#9583, re-enabling some of the gains from bitcoin#7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed.

This runs Block{Connected,Disconnected}, SetBestChain, Inventory, and TransactionAddedToMempool on the background scheduler thread. Of those, only BlockConnected is used outside of Wallet/ZMQ, and is used only for orphan transaction removal in net_processing, something which does not need to be synchronous with anything else. This partially reverts bitcoin#9583, re-enabling some of the gains from bitcoin#7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed.

…ut cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946. This concludes the work of #9725, #10178, and #10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

…llets b3b3c2a Reduce cs_main locks during ConnectTip/SyncWithWallets (Jonas Schnelli)

…ely reverts bitcoin#7946) 9899893 Move wallet callbacks into cs_main (this effectively reverts bitcoin#7946) (Matt Corallo)

This runs Block{Connected,Disconnected}, SetBestChain, Inventory, and TransactionAddedToMempool on the background scheduler thread. Of those, only BlockConnected is used outside of Wallet/ZMQ, and is used only for orphan transaction removal in net_processing, something which does not need to be synchronous with anything else. This partially reverts bitcoin#9583, re-enabling some of the gains from bitcoin#7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed.

…llets b3b3c2a Reduce cs_main locks during ConnectTip/SyncWithWallets (Jonas Schnelli)

…ely reverts bitcoin#7946) 9899893 Move wallet callbacks into cs_main (this effectively reverts bitcoin#7946) (Matt Corallo)

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

…WithWallets 9d1beb3 Move wallet callbacks into cs_main. (furszy) 4b4858b Remove SyncWithWallets wrapper function (Matt Corallo) 3b2807f Performance Regression Fix: Pre-Allocate txChanged vector (furszy) 1fe0be6 trivial: remove unused variable (Daniel Kraft) f1eb073 Reduce cs_main locks during ConnectTip/SyncWithWallets (furszy) 5e155b4 Remove unused pwalletdb from CommitTransaction (furszy) d8cc80a Remove CWalletDB* parameter from CWallet::AddToWallet (furszy) Pull request description: Work done on top of #1717 . Essentially an improvement over `SyncWithWallets` & `SyncTransaction` flow to run without locking `cs_main`. (made as less modifications as possible over the zpiv wallet/chain files, no real need to improve that code.. most of it will disappear soon) It's an adapted version of bitcoin#7946. ACKs for top commit: random-zebra: utACK 9d1beb3 Fuzzbawls: ACK 9d1beb3 Tree-SHA512: 8fc494a1d94a7fdf048fd6d08eb3f831693c9cf252a871cee83405777f40ff36e2e9455efa66ed78d3d56e528336179de2ed54b8675cf819a00d8dafa19e909a

This runs Block{Connected,Disconnected}, SetBestChain, Inventory, and TransactionAddedToMempool on the background scheduler thread. Of those, only BlockConnected is used outside of Wallet/ZMQ, and is used only for orphan transaction removal in net_processing, something which does not need to be synchronous with anything else. This partially reverts bitcoin#9583, re-enabling some of the gains from bitcoin#7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed.

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

jonasschnelli added the Refactoring label Apr 26, 2016

sipa reviewed Apr 26, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch 2 times, most recently from fdbee7d to 22d9380 Compare April 26, 2016 13:31

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch 2 times, most recently from 6416d91 to 086654e Compare April 26, 2016 20:25

kazcw reviewed Apr 27, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 6fc0be6 to e8ea3dd Compare May 6, 2016 09:47

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from e8ea3dd to 9471f32 Compare May 25, 2016 14:33

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 9471f32 to 580a665 Compare May 25, 2016 14:47

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 580a665 to fdf8a5e Compare May 25, 2016 15:58

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from fdf8a5e to d31dd19 Compare June 10, 2016 15:09

codablock pushed a commit to codablock/dash that referenced this pull request Jan 8, 2018

Merge bitcoin#7946: Reduce cs_main locks during ConnectTip/SyncWithWa…

256b9b7

…llets b3b3c2a Reduce cs_main locks during ConnectTip/SyncWithWallets (Jonas Schnelli)

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#7946: Reduce cs_main locks during ConnectTip/SyncWithWa…

818ffd8

…llets b3b3c2a Reduce cs_main locks during ConnectTip/SyncWithWallets (Jonas Schnelli)

furszy mentioned this pull request Jul 1, 2020

[Validation] Reduce cs_main locks during ConnectTip/SyncWithWallets PIVX-Project/PIVX#1721

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Reduce cs_main locks during ConnectTip/SyncWithWallets #7946

Reduce cs_main locks during ConnectTip/SyncWithWallets #7946

Uh oh!

Conversation

jonasschnelli commented Apr 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 26, 2016

Uh oh!

kazcw commented Apr 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 6, 2016

Uh oh!

kazcw commented May 6, 2016

Uh oh!

sipa commented May 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 25, 2016

Uh oh!

dcousens commented Jun 3, 2016

Uh oh!

laanwj commented Jun 10, 2016

Uh oh!

jonasschnelli commented Jun 10, 2016

Uh oh!

Uh oh!

jonasschnelli commented May 25, 2016 •

edited

Loading

sipa May 25, 2016 •

edited

Loading

jonasschnelli May 25, 2016 •

edited

Loading