From 256b9b77a252e2871b943dd0cddbc2ea8982be07 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 15 Aug 2016 11:49:12 +0200 Subject: [PATCH] Merge #7946: Reduce cs_main locks during ConnectTip/SyncWithWallets b3b3c2a Reduce cs_main locks during ConnectTip/SyncWithWallets (Jonas Schnelli) --- src/validation.cpp | 39 ++++++++++++++----------- src/validationinterface.cpp | 4 +++ src/validationinterface.h | 4 +-- src/wallet/wallet.cpp | 43 +++++++++++----------------- src/wallet/wallet.h | 6 ++-- src/zmq/zmqnotificationinterface.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 2 +- 7 files changed, 49 insertions(+), 51 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 68a3fd722..c750f24ac 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1014,7 +1014,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } if(!fDryRun) - GetMainSignals().SyncTransaction(tx, NULL, NULL); + GetMainSignals().SyncTransaction(tx, NULL); return true; } @@ -2559,7 +2559,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: BOOST_FOREACH(const CTransaction &tx, block.vtx) { - GetMainSignals().SyncTransaction(tx, pindexDelete->pprev, NULL); + GetMainSignals().SyncTransaction(tx, pindexDelete->pprev); } return true; } @@ -2574,7 +2574,7 @@ static int64_t nTimePostConnect = 0; * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock) +bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, std::list &txConflicted, std::vector > &txChanged) { assert(pindexNew->pprev == chainActive.Tip()); // Read block from disk. @@ -2609,20 +2609,13 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, return false; int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4; LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); - // Remove conflicting transactions from the mempool. - list txConflicted; + // Remove conflicting transactions from the mempool.; mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); - // Tell wallet about transactions that went from mempool - // to conflicted: - BOOST_FOREACH(const CTransaction &tx, txConflicted) { - GetMainSignals().SyncTransaction(tx, pindexNew, NULL); - } - // ... and about transactions that got confirmed: - BOOST_FOREACH(const CTransaction &tx, pblock->vtx) { - GetMainSignals().SyncTransaction(tx, pindexNew, pblock); - } + + for(unsigned int i=0; i < pblock->vtx.size(); i++) + txChanged.push_back(std::make_tuple(pblock->vtx[i], pindexNew, i)); int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint("bench", " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); @@ -2747,7 +2740,7 @@ static void PruneBlockIndexCandidates() { * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound) +static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, std::list& txConflicted, std::vector >& txChanged) { AssertLockHeld(cs_main); const CBlockIndex *pindexOldTip = chainActive.Tip(); @@ -2780,7 +2773,7 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Connect new blocks. BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL)) { + if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, txConflicted, txChanged)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2855,6 +2848,8 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, break; const CBlockIndex *pindexFork; + std::list txConflicted; + std::vector > txChanged; bool fInitialDownload; { LOCK(cs_main); @@ -2868,7 +2863,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, return true; bool fInvalidFound = false; - if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fInvalidFound)) + if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fInvalidFound, txConflicted, txChanged)) return false; if (fInvalidFound) { @@ -2883,6 +2878,16 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, // Notifications/callbacks that can run without cs_main + // throw all transactions though the signal-interface + // while _not_ holding the cs_main lock + BOOST_FOREACH(const CTransaction &tx, txConflicted) + { + GetMainSignals().SyncTransaction(tx, pindexNewTip); + } + // ... and about transactions that got confirmed: + for(unsigned int i = 0; i < txChanged.size(); i++) + GetMainSignals().SyncTransaction(std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i])); + // Notify external listeners about the new tip. GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 7c97f2c5b..808c9016a 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -56,3 +56,7 @@ void UnregisterAllValidationInterfaces() { g_signals.NotifyHeaderTip.disconnect_all_slots(); g_signals.AcceptedBlockHeader.disconnect_all_slots(); } + +void SyncWithWallets(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) { + g_signals.SyncTransaction(tx, pindex, posInBlock); +} diff --git a/src/validationinterface.h b/src/validationinterface.h index 5c15f80bb..c3e2d0712 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -34,7 +34,7 @@ protected: virtual void AcceptedBlockHeader(const CBlockIndex *pindexNew) {} virtual void NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) {} virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} - virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, const CBlock *pblock) {} + virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) {} virtual void NotifyTransactionLock(const CTransaction &tx) {} virtual void SetBestChain(const CBlockLocator &locator) {} virtual bool UpdatedTransaction(const uint256 &hash) { return false;} @@ -56,7 +56,7 @@ struct CMainSignals { /** Notifies listeners of updated block chain tip */ boost::signals2::signal UpdatedBlockTip; /** Notifies listeners of updated transaction data (transaction, and optionally the block it is found in. */ - boost::signals2::signal SyncTransaction; + boost::signals2::signal SyncTransaction; /** Notifies listeners of an updated transaction lock without new data. */ boost::signals2::signal NotifyTransactionLock; /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fe39a3bf0..842e849e9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1112,18 +1112,18 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn) * pblock is optional, but should be provided if the transaction is known to be in a block. * If fUpdate is true, existing transactions will be updated. */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate) { { AssertLockHeld(cs_wallet); - if (pblock) { + if (posInBlock != -1) { BOOST_FOREACH(const CTxIn& txin, tx.vin) { std::pair range = mapTxSpends.equal_range(txin.prevout); while (range.first != range.second) { if (range.first->second != tx.GetHash()) { - LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), pblock->GetHash().ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(pblock->GetHash(), range.first->second); + LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), pIndex->GetBlockHash().ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); + MarkConflicted(pIndex->GetBlockHash(), range.first->second); } range.first++; } @@ -1137,8 +1137,8 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl CWalletTx wtx(this,tx); // Get merkle branch if transaction was found in a block - if (pblock) - wtx.SetMerkleBranch(*pblock); + if (posInBlock != -1) + wtx.SetMerkleBranch(pIndex, posInBlock); return AddToWallet(wtx, false); } @@ -1269,11 +1269,11 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) fAnonymizableTallyCachedNonDenom = false; } -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); - if (!AddToWalletIfInvolvingMe(tx, pblock, true)) + if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -1778,9 +1778,10 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) CBlock block; ReadBlockFromDisk(block, pindex, Params().GetConsensus()); - BOOST_FOREACH(CTransaction& tx, block.vtx) + int posInBlock; + for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++) { - if (AddToWalletIfInvolvingMe(tx, &block, fUpdate)) + if (AddToWalletIfInvolvingMe(block.vtx[posInBlock], pindex, posInBlock, fUpdate)) ret++; } pindex = chainActive.Next(pindex); @@ -4960,31 +4961,19 @@ CWalletKey::CWalletKey(int64_t nExpires) nTimeExpires = nExpires; } -int CMerkleTx::SetMerkleBranch(const CBlock& block) +int CMerkleTx::SetMerkleBranch(const CBlockIndex* pindex, int posInBlock) { AssertLockHeld(cs_main); CBlock blockTmp; // Update the tx's hashBlock - hashBlock = block.GetHash(); + hashBlock = pindex->GetBlockHash(); - // Locate the transaction - for (nIndex = 0; nIndex < (int)block.vtx.size(); nIndex++) - if (block.vtx[nIndex] == *(CTransaction*)this) - break; - if (nIndex == (int)block.vtx.size()) - { - nIndex = -1; - LogPrintf("ERROR: SetMerkleBranch(): couldn't find tx in block\n"); - return 0; - } + // set the position of the transaction in the block + nIndex = posInBlock; // Is the tx in a block that's in the main chain - BlockMap::iterator mi = mapBlockIndex.find(hashBlock); - if (mi == mapBlockIndex.end()) - return 0; - const CBlockIndex* pindex = (*mi).second; - if (!pindex || !chainActive.Contains(pindex)) + if (!chainActive.Contains(pindex)) return 0; return chainActive.Height() - pindex->nHeight + 1; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 10ab59a33..65f91ecf2 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -241,7 +241,7 @@ public: READWRITE(nIndex); } - int SetMerkleBranch(const CBlock& block); + int SetMerkleBranch(const CBlockIndex* pIndex, int posInBlock); /** * Return depth of transaction in blockchain: @@ -857,8 +857,8 @@ public: void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); bool LoadToWallet(const CWalletTx& wtxIn); - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, const CBlock* pblock); - bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pblock, bool fUpdate); + void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock); + bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); int ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); void ReacceptWalletTransactions(); void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 30f180dda..09bfea87d 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -146,7 +146,7 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co } } -void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, const CBlock* pblock) +void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) { for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) { diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 264f34c59..f90875582 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -24,7 +24,7 @@ protected: void Shutdown(); // CValidationInterface - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, const CBlock* pblock); + void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock); void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); void NotifyTransactionLock(const CTransaction &tx);