From 9d6dcc52c6cb0cdcda220fddccaabb0ffd40068d Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 24 Feb 2019 12:07:10 -0800 Subject: Abstract EraseBlockData out of RewindBlockIndex Note that the former 'else' branch in RewindBlockIndex is now dealt with more naturally inside the EraseBlockData call (by checking whether the parent needs to be re-added as candidate after deleting a child). --- src/validation.cpp | 67 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 27 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 1806bc1268..d1872c961c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -206,8 +206,10 @@ private: CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main); void ReceivedBlockTransactions(const CBlock& block, CBlockIndex* pindexNew, const CDiskBlockPos& pos, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + + //! Mark a block as not having block data + void EraseBlockData(CBlockIndex* index) EXCLUSIVE_LOCKS_REQUIRED(cs_main); } g_chainstate; /** @@ -4168,6 +4170,42 @@ bool ReplayBlocks(const CChainParams& params, CCoinsView* view) { return g_chainstate.ReplayBlocks(params, view); } +//! Helper for CChainState::RewindBlockIndex +void CChainState::EraseBlockData(CBlockIndex* index) +{ + AssertLockHeld(cs_main); + assert(!chainActive.Contains(index)); // Make sure this block isn't active + + // Reduce validity + index->nStatus = std::min(index->nStatus & BLOCK_VALID_MASK, BLOCK_VALID_TREE) | (index->nStatus & ~BLOCK_VALID_MASK); + // Remove have-data flags. + index->nStatus &= ~(BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO); + // Remove storage location. + index->nFile = 0; + index->nDataPos = 0; + index->nUndoPos = 0; + // Remove various other things + index->nTx = 0; + index->nChainTx = 0; + index->nSequenceId = 0; + // Make sure it gets written. + setDirtyBlockIndex.insert(index); + // Update indexes + setBlockIndexCandidates.erase(index); + std::pair::iterator, std::multimap::iterator> ret = mapBlocksUnlinked.equal_range(index->pprev); + while (ret.first != ret.second) { + if (ret.first->second == index) { + mapBlocksUnlinked.erase(ret.first++); + } else { + ++ret.first; + } + } + // Mark parent as eligible for main chain again + if (index->pprev && index->pprev->IsValid(BLOCK_VALID_TRANSACTIONS) && index->pprev->HaveTxsDownloaded()) { + setBlockIndexCandidates.insert(index->pprev); + } +} + bool CChainState::RewindBlockIndex(const CChainParams& params) { LOCK(cs_main); @@ -4219,32 +4257,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) // rewind all the way. Blocks remaining on chainActive at this point // must not have their validity reduced. if (IsWitnessEnabled(pindexIter->pprev, params.GetConsensus()) && !(pindexIter->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(pindexIter)) { - // Reduce validity - pindexIter->nStatus = std::min(pindexIter->nStatus & BLOCK_VALID_MASK, BLOCK_VALID_TREE) | (pindexIter->nStatus & ~BLOCK_VALID_MASK); - // Remove have-data flags. - pindexIter->nStatus &= ~(BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO); - // Remove storage location. - pindexIter->nFile = 0; - pindexIter->nDataPos = 0; - pindexIter->nUndoPos = 0; - // Remove various other things - pindexIter->nTx = 0; - pindexIter->nChainTx = 0; - pindexIter->nSequenceId = 0; - // Make sure it gets written. - setDirtyBlockIndex.insert(pindexIter); - // Update indexes - setBlockIndexCandidates.erase(pindexIter); - std::pair::iterator, std::multimap::iterator> ret = mapBlocksUnlinked.equal_range(pindexIter->pprev); - while (ret.first != ret.second) { - if (ret.first->second == pindexIter) { - mapBlocksUnlinked.erase(ret.first++); - } else { - ++ret.first; - } - } - } else if (pindexIter->IsValid(BLOCK_VALID_TRANSACTIONS) && pindexIter->HaveTxsDownloaded()) { - setBlockIndexCandidates.insert(pindexIter); + EraseBlockData(pindexIter); } } -- cgit v1.2.3 From 32b2696ab4b079db736074b57bbc24deaee0b3d9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 24 Feb 2019 12:36:07 -0800 Subject: Move erasure of non-active blocks to a separate loop in RewindBlockIndex This lets us simplify the iteration to just walking back in the chain, rather than looping over all of mapBlockIndex. --- src/validation.cpp | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index d1872c961c..a2d97299e9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4209,9 +4209,18 @@ void CChainState::EraseBlockData(CBlockIndex* index) bool CChainState::RewindBlockIndex(const CChainParams& params) { LOCK(cs_main); - // Note that during -reindex-chainstate we are called with an empty chainActive! + // First erase all post-segwit blocks without witness not in the main chain, + // as this can we done without costly DisconnectTip calls. Active + // blocks will be dealt with below. + for (const auto& entry : mapBlockIndex) { + if (IsWitnessEnabled(entry.second->pprev, params.GetConsensus()) && !(entry.second->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(entry.second)) { + EraseBlockData(entry.second); + } + } + + // Find what height we need to reorganize to. int nHeight = 1; while (nHeight <= chainActive.Height()) { // Although SCRIPT_VERIFY_WITNESS is now generally enforced on all @@ -4226,6 +4235,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1 CValidationState state; CBlockIndex* pindex = chainActive.Tip(); + CBlockIndex* tip = pindex; while (chainActive.Height() >= nHeight) { if (fPruneMode && !(chainActive.Tip()->nStatus & BLOCK_HAVE_DATA)) { // If pruning, don't try rewinding past the HAVE_DATA point; @@ -4248,17 +4258,16 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) // Reduce validity flag and have-data flags. // We do this after actual disconnecting, otherwise we'll end up writing the lack of data // to disk before writing the chainstate, resulting in a failure to continue if interrupted. - for (const auto& entry : mapBlockIndex) { - CBlockIndex* pindexIter = entry.second; - + while (tip->nHeight > chainActive.Height()) { // Note: If we encounter an insufficiently validated block that // is on chainActive, it must be because we are a pruning node, and // this block or some successor doesn't HAVE_DATA, so we were unable to // rewind all the way. Blocks remaining on chainActive at this point // must not have their validity reduced. - if (IsWitnessEnabled(pindexIter->pprev, params.GetConsensus()) && !(pindexIter->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(pindexIter)) { - EraseBlockData(pindexIter); + if (IsWitnessEnabled(tip->pprev, params.GetConsensus()) && !(tip->nStatus & BLOCK_OPT_WITNESS)) { + EraseBlockData(tip); } + tip = tip->pprev; } if (chainActive.Tip() != nullptr) { -- cgit v1.2.3 From 1d342875c21b5d0a17cf4d176063bb14b35b657e Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 24 Feb 2019 12:54:53 -0800 Subject: Merge the disconnection and erasing loops in RewindBlockIndex --- src/validation.cpp | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index a2d97299e9..040994c7ce 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4234,10 +4234,13 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1 CValidationState state; - CBlockIndex* pindex = chainActive.Tip(); - CBlockIndex* tip = pindex; - while (chainActive.Height() >= nHeight) { - if (fPruneMode && !(chainActive.Tip()->nStatus & BLOCK_HAVE_DATA)) { + CBlockIndex* tip = chainActive.Tip(); + // Loop until the tip is below nHeight, or we reach a pruned block. + while (true) { + // Make sure nothing changed from under us (this won't happen because RewindBlockIndex runs before importing/network are active) + assert(tip == chainActive.Tip()); + if (tip == nullptr || tip->nHeight < nHeight) break; + if (fPruneMode && !(tip->nStatus & BLOCK_HAVE_DATA)) { // If pruning, don't try rewinding past the HAVE_DATA point; // since older blocks can't be served anyway, there's // no need to walk further, and trying to DisconnectTip() @@ -4245,29 +4248,29 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) // of the blockchain). break; } + + // Disconnect block if (!DisconnectTip(state, params, nullptr)) { - return error("RewindBlockIndex: unable to disconnect block at height %i (%s)", pindex->nHeight, FormatStateMessage(state)); - } - // Occasionally flush state to disk. - if (!FlushStateToDisk(params, state, FlushStateMode::PERIODIC)) { - LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", FormatStateMessage(state)); - return false; + return error("RewindBlockIndex: unable to disconnect block at height %i (%s)", tip->nHeight, FormatStateMessage(state)); } - } - // Reduce validity flag and have-data flags. - // We do this after actual disconnecting, otherwise we'll end up writing the lack of data - // to disk before writing the chainstate, resulting in a failure to continue if interrupted. - while (tip->nHeight > chainActive.Height()) { + // Reduce validity flag and have-data flags. + // We do this after actual disconnecting, otherwise we'll end up writing the lack of data + // to disk before writing the chainstate, resulting in a failure to continue if interrupted. // Note: If we encounter an insufficiently validated block that // is on chainActive, it must be because we are a pruning node, and // this block or some successor doesn't HAVE_DATA, so we were unable to // rewind all the way. Blocks remaining on chainActive at this point // must not have their validity reduced. - if (IsWitnessEnabled(tip->pprev, params.GetConsensus()) && !(tip->nStatus & BLOCK_OPT_WITNESS)) { - EraseBlockData(tip); - } + EraseBlockData(tip); + tip = tip->pprev; + + // Occasionally flush state to disk. + if (!FlushStateToDisk(params, state, FlushStateMode::PERIODIC)) { + LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", FormatStateMessage(state)); + return false; + } } if (chainActive.Tip() != nullptr) { -- cgit v1.2.3 From 436f7d735f1c37e77d42ff59d4cbb1bd76d5fcfb Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 15:32:34 -0800 Subject: Release cs_main during RewindBlockIndex operation --- src/validation.cpp | 102 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 57 insertions(+), 45 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 040994c7ce..da92a1d1ca 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4208,64 +4208,73 @@ void CChainState::EraseBlockData(CBlockIndex* index) bool CChainState::RewindBlockIndex(const CChainParams& params) { - LOCK(cs_main); // Note that during -reindex-chainstate we are called with an empty chainActive! // First erase all post-segwit blocks without witness not in the main chain, // as this can we done without costly DisconnectTip calls. Active - // blocks will be dealt with below. - for (const auto& entry : mapBlockIndex) { - if (IsWitnessEnabled(entry.second->pprev, params.GetConsensus()) && !(entry.second->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(entry.second)) { - EraseBlockData(entry.second); + // blocks will be dealt with below (releasing cs_main in between). + { + LOCK(cs_main); + for (const auto& entry : mapBlockIndex) { + if (IsWitnessEnabled(entry.second->pprev, params.GetConsensus()) && !(entry.second->nStatus & BLOCK_OPT_WITNESS) && !chainActive.Contains(entry.second)) { + EraseBlockData(entry.second); + } } } // Find what height we need to reorganize to. + CBlockIndex *tip; int nHeight = 1; - while (nHeight <= chainActive.Height()) { - // Although SCRIPT_VERIFY_WITNESS is now generally enforced on all - // blocks in ConnectBlock, we don't need to go back and - // re-download/re-verify blocks from before segwit actually activated. - if (IsWitnessEnabled(chainActive[nHeight - 1], params.GetConsensus()) && !(chainActive[nHeight]->nStatus & BLOCK_OPT_WITNESS)) { - break; + { + LOCK(cs_main); + while (nHeight <= chainActive.Height()) { + // Although SCRIPT_VERIFY_WITNESS is now generally enforced on all + // blocks in ConnectBlock, we don't need to go back and + // re-download/re-verify blocks from before segwit actually activated. + if (IsWitnessEnabled(chainActive[nHeight - 1], params.GetConsensus()) && !(chainActive[nHeight]->nStatus & BLOCK_OPT_WITNESS)) { + break; + } + nHeight++; } - nHeight++; - } + tip = chainActive.Tip(); + } // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1 + CValidationState state; - CBlockIndex* tip = chainActive.Tip(); // Loop until the tip is below nHeight, or we reach a pruned block. while (true) { - // Make sure nothing changed from under us (this won't happen because RewindBlockIndex runs before importing/network are active) - assert(tip == chainActive.Tip()); - if (tip == nullptr || tip->nHeight < nHeight) break; - if (fPruneMode && !(tip->nStatus & BLOCK_HAVE_DATA)) { - // If pruning, don't try rewinding past the HAVE_DATA point; - // since older blocks can't be served anyway, there's - // no need to walk further, and trying to DisconnectTip() - // will fail (and require a needless reindex/redownload - // of the blockchain). - break; - } - - // Disconnect block - if (!DisconnectTip(state, params, nullptr)) { - return error("RewindBlockIndex: unable to disconnect block at height %i (%s)", tip->nHeight, FormatStateMessage(state)); - } + { + LOCK(cs_main); + // Make sure nothing changed from under us (this won't happen because RewindBlockIndex runs before importing/network are active) + assert(tip == chainActive.Tip()); + if (tip == nullptr || tip->nHeight < nHeight) break; + if (fPruneMode && !(tip->nStatus & BLOCK_HAVE_DATA)) { + // If pruning, don't try rewinding past the HAVE_DATA point; + // since older blocks can't be served anyway, there's + // no need to walk further, and trying to DisconnectTip() + // will fail (and require a needless reindex/redownload + // of the blockchain). + break; + } - // Reduce validity flag and have-data flags. - // We do this after actual disconnecting, otherwise we'll end up writing the lack of data - // to disk before writing the chainstate, resulting in a failure to continue if interrupted. - // Note: If we encounter an insufficiently validated block that - // is on chainActive, it must be because we are a pruning node, and - // this block or some successor doesn't HAVE_DATA, so we were unable to - // rewind all the way. Blocks remaining on chainActive at this point - // must not have their validity reduced. - EraseBlockData(tip); + // Disconnect block + if (!DisconnectTip(state, params, nullptr)) { + return error("RewindBlockIndex: unable to disconnect block at height %i (%s)", tip->nHeight, FormatStateMessage(state)); + } - tip = tip->pprev; + // Reduce validity flag and have-data flags. + // We do this after actual disconnecting, otherwise we'll end up writing the lack of data + // to disk before writing the chainstate, resulting in a failure to continue if interrupted. + // Note: If we encounter an insufficiently validated block that + // is on chainActive, it must be because we are a pruning node, and + // this block or some successor doesn't HAVE_DATA, so we were unable to + // rewind all the way. Blocks remaining on chainActive at this point + // must not have their validity reduced. + EraseBlockData(tip); + tip = tip->pprev; + } // Occasionally flush state to disk. if (!FlushStateToDisk(params, state, FlushStateMode::PERIODIC)) { LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", FormatStateMessage(state)); @@ -4273,12 +4282,15 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) } } - if (chainActive.Tip() != nullptr) { - // We can't prune block index candidates based on our tip if we have - // no tip due to chainActive being empty! - PruneBlockIndexCandidates(); + { + LOCK(cs_main); + if (chainActive.Tip() != nullptr) { + // We can't prune block index candidates based on our tip if we have + // no tip due to chainActive being empty! + PruneBlockIndexCandidates(); - CheckBlockIndex(params.GetConsensus()); + CheckBlockIndex(params.GetConsensus()); + } } return true; -- cgit v1.2.3 From 880ce7d46b51835c00d77a366ec28f54a05239df Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 15:39:32 -0800 Subject: Call RewindBlockIndex without cs_main held --- src/init.cpp | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/init.cpp b/src/init.cpp index 8b831a726f..dc46c80f69 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1474,11 +1474,11 @@ bool AppInitMain(InitInterfaces& interfaces) uiInterface.InitMessage(_("Loading block index...")); - LOCK(cs_main); - do { const int64_t load_block_index_start_time = GetTimeMillis(); + bool is_coinsview_empty; try { + LOCK(cs_main); UnloadBlockIndex(); pcoinsTip.reset(); pcoinsdbview.reset(); @@ -1550,7 +1550,7 @@ bool AppInitMain(InitInterfaces& interfaces) // The on-disk coinsdb is now in a good state, create the cache pcoinsTip.reset(new CCoinsViewCache(pcoinscatcher.get())); - bool is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull(); + is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull(); if (!is_coinsview_empty) { // LoadChainTip sets chainActive based on pcoinsTip's best block if (!LoadChainTip(chainparams)) { @@ -1559,18 +1559,25 @@ bool AppInitMain(InitInterfaces& interfaces) } assert(chainActive.Tip() != nullptr); } + } catch (const std::exception& e) { + LogPrintf("%s\n", e.what()); + strLoadError = _("Error opening block database"); + break; + } - if (!fReset) { - // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate. - // It both disconnects blocks based on chainActive, and drops block data in - // mapBlockIndex based on lack of available witness data. - uiInterface.InitMessage(_("Rewinding blocks...")); - if (!RewindBlockIndex(chainparams)) { - strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain"); - break; - } + if (!fReset) { + // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate. + // It both disconnects blocks based on chainActive, and drops block data in + // mapBlockIndex based on lack of available witness data. + uiInterface.InitMessage(_("Rewinding blocks...")); + if (!RewindBlockIndex(chainparams)) { + strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain"); + break; } + } + try { + LOCK(cs_main); if (!is_coinsview_empty) { uiInterface.InitMessage(_("Verifying blocks...")); if (fHavePruned && gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) { -- cgit v1.2.3 From 241b2c74ac8c4c3000e778554da1271e3f293e5d Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 17:20:56 -0800 Subject: Make RewindBlockIndex interruptible --- src/validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index da92a1d1ca..1de2cda5cd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4243,7 +4243,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) CValidationState state; // Loop until the tip is below nHeight, or we reach a pruned block. - while (true) { + while (!ShutdownRequested()) { { LOCK(cs_main); // Make sure nothing changed from under us (this won't happen because RewindBlockIndex runs before importing/network are active) -- cgit v1.2.3 From 9b1ff5c742dec0a6e0d6aab29b0bb771ad6d8135 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 15:53:24 -0800 Subject: Call InvalidateBlock without cs_main held --- src/rpc/blockchain.cpp | 6 +++--- src/validation.cpp | 4 ++-- src/validation.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7fb9ff2eaf..1169446eff 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1583,15 +1583,15 @@ static UniValue invalidateblock(const JSONRPCRequest& request) uint256 hash(ParseHashV(request.params[0], "blockhash")); CValidationState state; + CBlockIndex* pblockindex; { LOCK(cs_main); - CBlockIndex* pblockindex = LookupBlockIndex(hash); + pblockindex = LookupBlockIndex(hash); if (!pblockindex) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); } - - InvalidateBlock(state, Params(), pblockindex); } + InvalidateBlock(state, Params(), pblockindex); if (state.IsValid()) { ActivateBestChain(state, Params()); diff --git a/src/validation.cpp b/src/validation.cpp index 1de2cda5cd..dd4436c9cf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -177,7 +177,7 @@ public: // Manual block validity manipulation: bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); - bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex); void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool ReplayBlocks(const CChainParams& params, CCoinsView* view); @@ -2789,7 +2789,7 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) { - AssertLockHeld(cs_main); + LOCK(cs_main); // We first disconnect backwards and then mark the blocks as invalid. // This prevents a case where pruned nodes may fail to invalidateblock diff --git a/src/validation.h b/src/validation.h index 1975846b69..65a378d8ba 100644 --- a/src/validation.h +++ b/src/validation.h @@ -448,7 +448,7 @@ CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& loc bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIndex *pindex) LOCKS_EXCLUDED(cs_main); /** Mark a block as invalid. */ -bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex); /** Remove invalidity status from a block and its descendants. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); -- cgit v1.2.3 From 9bb32eb571a846b66ed3bac493f55cee11a3a1b9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 16:37:30 -0800 Subject: Release cs_main during InvalidateBlock iterations --- src/validation.cpp | 90 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 40 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index dd4436c9cf..8f7e9fba69 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2789,64 +2789,74 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) { - LOCK(cs_main); - - // We first disconnect backwards and then mark the blocks as invalid. - // This prevents a case where pruned nodes may fail to invalidateblock - // and be left unable to start as they have no tip candidates (as there - // are no blocks that meet the "have data and are not invalid per - // nStatus" criteria for inclusion in setBlockIndexCandidates). - + CBlockIndex* to_mark_failed = pindex; bool pindex_was_in_chain = false; - CBlockIndex *invalid_walk_tip = chainActive.Tip(); - DisconnectedBlockTransactions disconnectpool; - while (chainActive.Contains(pindex)) { + // Disconnect (descendants of) pindex, and mark them invalid. + while (true) { + if (ShutdownRequested()) break; + + LOCK(cs_main); + if (!chainActive.Contains(pindex)) break; pindex_was_in_chain = true; + CBlockIndex *invalid_walk_tip = chainActive.Tip(); + // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. - if (!DisconnectTip(state, chainparams, &disconnectpool)) { - // It's probably hopeless to try to make the mempool consistent - // here if DisconnectTip failed, but we can try. - UpdateMempoolForReorg(disconnectpool, false); - return false; - } - } + DisconnectedBlockTransactions disconnectpool; + bool ret = DisconnectTip(state, chainparams, &disconnectpool); + // DisconnectTip will add transactions to disconnectpool. + // Adjust the mempool to be consistent with the new tip, adding + // transactions back to the mempool if disconnecting was succesful. + UpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ ret); + if (!ret) return false; + assert(invalid_walk_tip->pprev == chainActive.Tip()); - // Now mark the blocks we just disconnected as descendants invalid - // (note this may not be all descendants). - while (pindex_was_in_chain && invalid_walk_tip != pindex) { + // We immediately mark the disconnected blocks as invalid. + // This prevents a case where pruned nodes may fail to invalidateblock + // and be left unable to start as they have no tip candidates (as there + // are no blocks that meet the "have data and are not invalid per + // nStatus" criteria for inclusion in setBlockIndexCandidates). invalid_walk_tip->nStatus |= BLOCK_FAILED_CHILD; setDirtyBlockIndex.insert(invalid_walk_tip); setBlockIndexCandidates.erase(invalid_walk_tip); - invalid_walk_tip = invalid_walk_tip->pprev; + setBlockIndexCandidates.insert(invalid_walk_tip->pprev); + + // If we abort invalidation after this iteration, make sure + // the last disconnected block gets marked failed (rather than + // just child of failed) + to_mark_failed = invalid_walk_tip; } - // Mark the block itself as invalid. - pindex->nStatus |= BLOCK_FAILED_VALID; - setDirtyBlockIndex.insert(pindex); - setBlockIndexCandidates.erase(pindex); - m_failed_blocks.insert(pindex); + { + // Mark pindex (or the last disconnected block) as invalid, regardless of whether it was in the main chain or not. + LOCK(cs_main); + if (chainActive.Contains(to_mark_failed)) { + // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed. + return false; + } - // DisconnectTip will add transactions to disconnectpool; try to add these - // back to the mempool. - UpdateMempoolForReorg(disconnectpool, true); + to_mark_failed->nStatus |= BLOCK_FAILED_VALID; + setDirtyBlockIndex.insert(to_mark_failed); + setBlockIndexCandidates.erase(to_mark_failed); + m_failed_blocks.insert(to_mark_failed); - // The resulting new best tip may not be in setBlockIndexCandidates anymore, so - // add it again. - BlockMap::iterator it = mapBlockIndex.begin(); - while (it != mapBlockIndex.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) { - setBlockIndexCandidates.insert(it->second); + // The resulting new best tip may not be in setBlockIndexCandidates anymore, so + // add it again. + BlockMap::iterator it = mapBlockIndex.begin(); + while (it != mapBlockIndex.end()) { + if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, chainActive.Tip())) { + setBlockIndexCandidates.insert(it->second); + } + it++; } - it++; - } - InvalidChainFound(pindex); + InvalidChainFound(to_mark_failed); + } // Only notify about a new block tip if the active chain was modified. if (pindex_was_in_chain) { - uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev); + uiInterface.NotifyBlockTip(IsInitialBlockDownload(), to_mark_failed->pprev); } return true; } -- cgit v1.2.3 From 9ce9c37004440d6a329874dbf66b51666d497dcb Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 17:17:59 -0800 Subject: Prevent callback overruns in InvalidateBlock and RewindBlockIndex --- src/validation.cpp | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index 8f7e9fba69..d0d2227664 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2642,6 +2642,14 @@ static void NotifyHeaderTip() LOCKS_EXCLUDED(cs_main) { } } +static void LimitValidationInterfaceQueue() { + AssertLockNotHeld(cs_main); + + if (GetMainSignals().CallbacksPending() > 10) { + SyncWithValidationInterfaceQueue(); + } +} + /** * Make the best chain active, in multiple steps. The result is either failure * or an activated best chain. pblock is either nullptr or a pointer to a block @@ -2670,15 +2678,13 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& do { boost::this_thread::interruption_point(); - if (GetMainSignals().CallbacksPending() > 10) { - // Block until the validation queue drains. This should largely - // never happen in normal operation, however may happen during - // reindex, causing memory blowup if we run too far ahead. - // Note that if a validationinterface callback ends up calling - // ActivateBestChain this may lead to a deadlock! We should - // probably have a DEBUG_LOCKORDER test for this in the future. - SyncWithValidationInterfaceQueue(); - } + // Block until the validation queue drains. This should largely + // never happen in normal operation, however may happen during + // reindex, causing memory blowup if we run too far ahead. + // Note that if a validationinterface callback ends up calling + // ActivateBestChain this may lead to a deadlock! We should + // probably have a DEBUG_LOCKORDER test for this in the future. + LimitValidationInterfaceQueue(); { LOCK(cs_main); @@ -2796,6 +2802,9 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c while (true) { if (ShutdownRequested()) break; + // Make sure the queue of validation callbacks doesn't grow unboundedly. + LimitValidationInterfaceQueue(); + LOCK(cs_main); if (!chainActive.Contains(pindex)) break; pindex_was_in_chain = true; @@ -4285,6 +4294,9 @@ bool CChainState::RewindBlockIndex(const CChainParams& params) tip = tip->pprev; } + // Make sure the queue of validation callbacks doesn't grow unboundedly. + LimitValidationInterfaceQueue(); + // Occasionally flush state to disk. if (!FlushStateToDisk(params, state, FlushStateMode::PERIODIC)) { LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", FormatStateMessage(state)); -- cgit v1.2.3 From 8d220417cd7bc34464e28a4861a885193ec091c2 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 13 Feb 2019 18:07:16 -0800 Subject: Optimization: don't add txn back to mempool after 10 invalidates --- src/validation.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index d0d2227664..c112fbdcc0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2797,6 +2797,7 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c { CBlockIndex* to_mark_failed = pindex; bool pindex_was_in_chain = false; + int disconnected = 0; // Disconnect (descendants of) pindex, and mark them invalid. while (true) { @@ -2816,8 +2817,10 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c bool ret = DisconnectTip(state, chainparams, &disconnectpool); // DisconnectTip will add transactions to disconnectpool. // Adjust the mempool to be consistent with the new tip, adding - // transactions back to the mempool if disconnecting was succesful. - UpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ ret); + // transactions back to the mempool if disconnecting was succesful, + // and we're not doing a very deep invalidation (in which case + // keeping the mempool up to date is probably futile anyway). + UpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ (++disconnected <= 10) && ret); if (!ret) return false; assert(invalid_walk_tip->pprev == chainActive.Tip()); -- cgit v1.2.3 From 519b0bc5dc5155b6f7e2362c2105552bb7618ad0 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sun, 3 Mar 2019 13:01:26 -0800 Subject: Make last disconnected block BLOCK_FAILED_VALID, even when aborted --- src/validation.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/validation.cpp b/src/validation.cpp index c112fbdcc0..55fd8d99a2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2829,25 +2829,30 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c // and be left unable to start as they have no tip candidates (as there // are no blocks that meet the "have data and are not invalid per // nStatus" criteria for inclusion in setBlockIndexCandidates). - invalid_walk_tip->nStatus |= BLOCK_FAILED_CHILD; + invalid_walk_tip->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(invalid_walk_tip); setBlockIndexCandidates.erase(invalid_walk_tip); setBlockIndexCandidates.insert(invalid_walk_tip->pprev); + if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) { + // We only want to mark the last disconnected block as BLOCK_FAILED_VALID; its children + // need to be BLOCK_FAILED_CHILD instead. + to_mark_failed->nStatus = (to_mark_failed->nStatus ^ BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(to_mark_failed); + } - // If we abort invalidation after this iteration, make sure - // the last disconnected block gets marked failed (rather than - // just child of failed) + // Track the last disconnected block, so we can correct its BLOCK_FAILED_CHILD status in future + // iterations, or, if it's the last one, call InvalidChainFound on it. to_mark_failed = invalid_walk_tip; } { - // Mark pindex (or the last disconnected block) as invalid, regardless of whether it was in the main chain or not. LOCK(cs_main); if (chainActive.Contains(to_mark_failed)) { // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed. return false; } + // Mark pindex (or the last disconnected block) as invalid, even when it never was in the main chain to_mark_failed->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(to_mark_failed); setBlockIndexCandidates.erase(to_mark_failed); -- cgit v1.2.3