From ecc3c4a019e6db30e208b8554b1a3658dcb9a80a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 9 Oct 2017 11:19:10 -0400 Subject: Do not unlock cs_main in ABC unless we've actually made progress. Technically, some internal datastructures may be in an inconsistent state if we do this, though there are no known bugs there. Still, for future safety, its much better to only unlock cs_main if we've made progress (not just tried a reorg which may make progress). --- src/validation.cpp | 65 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 28 deletions(-) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index daa33d3f5..477b3e155 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2666,45 +2666,53 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& SyncWithValidationInterfaceQueue(); } - const CBlockIndex *pindexFork; - bool fInitialDownload; { LOCK(cs_main); - ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + CBlockIndex* starting_tip = chainActive.Tip(); + bool blocks_connected = false; + do { + // We absolutely may not unlock cs_main until we've made forward progress + // (with the exception of shutdown due to hardware issues, low disk space, etc). + ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + + if (pindexMostWork == nullptr) { + pindexMostWork = FindMostWorkChain(); + } - CBlockIndex *pindexOldTip = chainActive.Tip(); - if (pindexMostWork == nullptr) { - pindexMostWork = FindMostWorkChain(); - } + // Whether we have anything to do at all. + if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) { + break; + } - // Whether we have anything to do at all. - if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) - return true; + bool fInvalidFound = false; + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) + return false; + blocks_connected = true; - bool fInvalidFound = false; - std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) - return false; + if (fInvalidFound) { + // Wipe cache, we may need another branch now. + pindexMostWork = nullptr; + } + pindexNewTip = chainActive.Tip(); - if (fInvalidFound) { - // Wipe cache, we may need another branch now. - pindexMostWork = nullptr; - } - pindexNewTip = chainActive.Tip(); - pindexFork = chainActive.FindFork(pindexOldTip); - fInitialDownload = IsInitialBlockDownload(); + for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { + assert(trace.pblock && trace.pindex); + GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); + } + } while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip))); + if (!blocks_connected) return true; - for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { - assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); - } + const CBlockIndex* pindexFork = chainActive.FindFork(starting_tip); + bool fInitialDownload = IsInitialBlockDownload(); // Notify external listeners about the new tip. // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - - // Always notify the UI if a new block tip was connected if (pindexFork != pindexNewTip) { + // Notify ValidationInterface subscribers + GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); + + // Always notify the UI if a new block tip was connected uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); } } @@ -2728,6 +2736,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& return true; } + bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr pblock) { return g_chainstate.ActivateBestChain(state, chainparams, std::move(pblock)); } -- cgit v1.2.3 From a3ae8e68739023e5dba9e5cb190e707ed4603316 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Thu, 10 May 2018 15:57:16 -0400 Subject: Fix concurrency-related bugs in ActivateBestChain If multiple threads are invoking ActivateBestChain, it was possible to have them working towards different tips, and we could arrive at a less work tip than we should. Fix this by introducing a ChainState lock which must be held for the entire duration of ActivateBestChain to enforce exclusion in ABC. --- src/validation.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'src/validation.cpp') diff --git a/src/validation.cpp b/src/validation.cpp index 477b3e155..405f21baa 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -144,6 +144,12 @@ private: */ std::set g_failed_blocks; + /** + * the ChainState CriticalSection + * A lock that must be held when modifying this ChainState - held in ActivateBestChain() + */ + CCriticalSection m_cs_chainstate; + public: CChain chainActive; BlockMap mapBlockIndex; @@ -2542,6 +2548,7 @@ void CChainState::PruneBlockIndexCandidates() { bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); + const CBlockIndex *pindexOldTip = chainActive.Tip(); const CBlockIndex *pindexFork = chainActive.FindFork(pindexMostWork); @@ -2653,6 +2660,12 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& // sanely for performance or correctness! AssertLockNotHeld(cs_main); + // ABC maintains a fair degree of expensive-to-calculate internal state + // because this function periodically releases cs_main so that it does not lock up other threads for too long + // during large connects - and to allow for e.g. the callback queue to drain + // we use m_cs_chainstate to enforce mutual exclusion so that only one caller may execute this function at a time + LOCK(m_cs_chainstate); + CBlockIndex *pindexMostWork = nullptr; CBlockIndex *pindexNewTip = nullptr; int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT); -- cgit v1.2.3