From 6f0646a2629c8d6bf0f6df795698f5713e8cb12b Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 7 Mar 2019 17:39:54 +0100 Subject: [PATCH 1/4] Merge #15402: Granular invalidateblock and RewindBlockIndex 519b0bc5dc5155b6f7e2362c2105552bb7618ad0 Make last disconnected block BLOCK_FAILED_VALID, even when aborted (Pieter Wuille) 8d220417cd7bc34464e28a4861a885193ec091c2 Optimization: don't add txn back to mempool after 10 invalidates (Pieter Wuille) 9ce9c37004440d6a329874dbf66b51666d497dcb Prevent callback overruns in InvalidateBlock and RewindBlockIndex (Pieter Wuille) 9bb32eb571a846b66ed3bac493f55cee11a3a1b9 Release cs_main during InvalidateBlock iterations (Pieter Wuille) 9b1ff5c742dec0a6e0d6aab29b0bb771ad6d8135 Call InvalidateBlock without cs_main held (Pieter Wuille) 241b2c74ac8c4c3000e778554da1271e3f293e5d Make RewindBlockIndex interruptible (Pieter Wuille) 880ce7d46b51835c00d77a366ec28f54a05239df Call RewindBlockIndex without cs_main held (Pieter Wuille) 436f7d735f1c37e77d42ff59d4cbb1bd76d5fcfb Release cs_main during RewindBlockIndex operation (Pieter Wuille) 1d342875c21b5d0a17cf4d176063bb14b35b657e Merge the disconnection and erasing loops in RewindBlockIndex (Pieter Wuille) 32b2696ab4b079db736074b57bbc24deaee0b3d9 Move erasure of non-active blocks to a separate loop in RewindBlockIndex (Pieter Wuille) 9d6dcc52c6cb0cdcda220fddccaabb0ffd40068d Abstract EraseBlockData out of RewindBlockIndex (Pieter Wuille) Pull request description: This PR makes a number of improvements to the InvalidateBlock (`invalidateblock` RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition: * They're made safely interruptible (`bitcoind` can be shutdown, and no progress in either will be lost, though if incomplete, `invalidateblock` won't continue after restart and will need to be called again) * The validation queue is prevented from overflowing (meaning `invalidateblock` on a very old block will not drive bitcoind OOM) (see #14289). * `invalidateblock` won't bother to move transactions back into the mempool after 10 blocks (optimization). This is not an optimal solution, as we're relying on the scheduler call sites to make sure the scheduler doesn't overflow. Ideally, the scheduler would guarantee this directly, but that needs a few further changes (moving the signal emissions out of cs_main) to prevent deadlocks. I have manually tested the `invalidateblock` changes (including interrupting, and running with -checkblockindex and -checkmempool), but haven't tried the rewinding (which is probably becoming increasingly unnecessary, as very few pre-0.13.1 nodes remain that would care to upgrade). Tree-SHA512: 692e42758bd3d3efc2eb701984a8cb5db25fbeee32e7575df0183a00d0c2c30fdf72ce64c7625c32ad8c8bdc56313da72a7471658faeb0d39eefe39c4b8b8474 --- src/init.cpp | 6 +- src/llmq/instantsend.cpp | 3 +- src/rpc/blockchain.cpp | 6 +- src/test/miner_tests.cpp | 6 +- src/validation.cpp | 148 +++++++++++++++++++++++---------------- src/validation.h | 7 +- 6 files changed, 104 insertions(+), 72 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index e0e33ec284..3f36d5be70 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1972,11 +1972,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); // This statement makes ::ChainstateActive() usable. g_chainstate = MakeUnique(); UnloadBlockIndex(); @@ -2099,7 +2099,7 @@ bool AppInitMain(InitInterfaces& interfaces) break; } - bool is_coinsview_empty = fReset || fReindexChainState || + is_coinsview_empty = fReset || fReindexChainState || ::ChainstateActive().CoinsTip().GetBestBlock().IsNull(); if (!is_coinsview_empty) { // LoadChainTip initializes the chain based on CoinsTip()'s best block diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index c7d3b52047..6c57b85394 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -1430,10 +1430,9 @@ void CInstantSendManager::ResolveBlockConflicts(const uint256& islockHash, const LogPrintf("CInstantSendManager::%s -- invalidating block %s\n", __func__, pindex->GetBlockHash().ToString()); - LOCK(cs_main); CValidationState state; // need non-const pointer - auto pindex2 = LookupBlockIndex(pindex->GetBlockHash()); + auto pindex2 = WITH_LOCK(::cs_main, return LookupBlockIndex(pindex->GetBlockHash())); if (!InvalidateBlock(state, Params(), pindex2)) { LogPrintf("CInstantSendManager::%s -- InvalidateBlock failed: %s\n", __func__, FormatStateMessage(state)); // This should not have happened and we are in a state were it's not safe to continue anymore diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 5c03524a29..43f8817f48 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1778,15 +1778,15 @@ static UniValue invalidateblock(const JSONRPCRequest& request) uint256 hash(uint256S(strHash)); 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/test/miner_tests.cpp b/src/test/miner_tests.cpp index 5a2a42c1a0..5c0e283588 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -518,20 +518,22 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) // Mine an empty block createAndProcessEmptyBlock(); + { LOCK(cs_main); SetMockTime(::ChainActive().Tip()->GetMedianTimePast() + 1); BOOST_CHECK(pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey)); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 5U); + } // unlock cs_main while calling InvalidateBlock CValidationState state; - InvalidateBlock(state, chainparams, ::ChainActive().Tip()); + InvalidateBlock(state, chainparams, WITH_LOCK(cs_main, return ::ChainActive().Tip())); SetMockTime(0); mempool.clear(); - LOCK(::mempool.cs); + LOCK2(cs_main, ::mempool.cs); TestPackageSelection(chainparams, scriptPubKey, txFirst); fCheckpointsEnabled = true; diff --git a/src/validation.cpp b/src/validation.cpp index 524360e7c9..96c069708c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3034,6 +3034,14 @@ static void NotifyHeaderTip() LOCKS_EXCLUDED(cs_main) { } } +static void LimitValidationInterfaceQueue() { + AssertLockNotHeld(cs_main); + + if (GetMainSignals().CallbacksPending() > 10) { + SyncWithValidationInterfaceQueue(); + } +} + bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr pblock) { // Note that while we're often called here from ProcessNewBlock, this is // far from a guarantee. Things in the P2P/RPC will often end up calling @@ -3055,15 +3063,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); @@ -3181,76 +3187,98 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) { - AssertLockHeld(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 = m_chain.Tip(); + int disconnected = 0; - if (pindex == pindexBestHeader) { - pindexBestInvalid = pindexBestHeader; - pindexBestHeader = pindexBestHeader->pprev; - } + // Disconnect (descendants of) pindex, and mark them invalid. + while (true) { + if (ShutdownRequested()) break; - DisconnectedBlockTransactions disconnectpool; - while (m_chain.Contains(pindex)) { - const CBlockIndex* pindexOldTip = m_chain.Tip(); + // Make sure the queue of validation callbacks doesn't grow unboundedly. + LimitValidationInterfaceQueue(); + + LOCK(cs_main); + if (!m_chain.Contains(pindex)) break; pindex_was_in_chain = true; + CBlockIndex *invalid_walk_tip = m_chain.Tip(); + const CBlockIndex* pindexOldTip = m_chain.Tip(); + + if (pindex == pindexBestHeader) { + pindexBestInvalid = pindexBestHeader; + pindexBestHeader = pindexBestHeader->pprev; + } + // ActivateBestChain considers blocks already in m_chain // 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, + // 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 == m_chain.Tip()); + if (pindexOldTip == pindexBestHeader) { pindexBestInvalid = pindexBestHeader; pindexBestHeader = pindexBestHeader->pprev; } - } - // 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) { - invalid_walk_tip->nStatus |= BLOCK_FAILED_CHILD; + // 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_VALID; setDirtyBlockIndex.insert(invalid_walk_tip); setBlockIndexCandidates.erase(invalid_walk_tip); - invalid_walk_tip = invalid_walk_tip->pprev; - } - - // Mark the block itself as invalid. - pindex->nStatus |= BLOCK_FAILED_VALID; - setDirtyBlockIndex.insert(pindex); - setBlockIndexCandidates.erase(pindex); - m_blockman.m_failed_blocks.insert(pindex); - - // DisconnectTip will add transactions to disconnectpool; try to add these - // back to the mempool. - UpdateMempoolForReorg(disconnectpool, true); - - // The resulting new best tip may not be in setBlockIndexCandidates anymore, so - // add it again. - BlockMap::iterator it = g_blockman.m_block_index.begin(); - while (it != g_blockman.m_block_index.end()) { - if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { - setBlockIndexCandidates.insert(it->second); + 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); } - it++; + + // 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; } - InvalidChainFound(pindex); - GetMainSignals().SynchronousUpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); - GetMainSignals().UpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); + { + LOCK(cs_main); + if (m_chain.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); + m_blockman.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 = g_blockman.m_block_index.begin(); + while (it != g_blockman.m_block_index.end()) { + if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { + setBlockIndexCandidates.insert(it->second); + } + it++; + } + + InvalidChainFound(to_mark_failed); + GetMainSignals().SynchronousUpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); + GetMainSignals().UpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); + } // 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; } diff --git a/src/validation.h b/src/validation.h index 3a303065f5..8366365761 100644 --- a/src/validation.h +++ b/src/validation.h @@ -692,7 +692,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); bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -739,6 +739,9 @@ private: 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); }; /** Mark a block as precious and reorganize. @@ -749,7 +752,7 @@ private: 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); /** Mark a block as conflicting. */ bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From fba2a546f6e4625490c11d52811c75975f260f95 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 2 Oct 2019 13:39:34 +0200 Subject: [PATCH 2/4] Merge #16849: Fix block index inconsistency in InvalidateBlock() 2a4e60b48261d3f0ec3d85f97af998ef989134e0 Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar) Pull request description: Previously, we could release `cs_main` while leaving the block index in a state that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being fully populated before releasing `cs_main`. ACKs for top commit: TheBlueMatt: utACK 2a4e60b48261d3f0ec3d85f97af998ef989134e0. I also discovered another issue in InvalidateBlock while reviewing, see #16856. Sjors: ACK 2a4e60b. Tested on top of #16899. Also tested `invalidateblock` with `-checkblockindex=1`. fjahr: ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`. Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612 --- src/validation.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 96c069708c..b3d7de84a2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3191,6 +3191,38 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c bool pindex_was_in_chain = false; int disconnected = 0; + // We do not allow ActivateBestChain() to run while InvalidateBlock() is + // running, as that could cause the tip to change while we disconnect + // blocks. + LOCK(m_cs_chainstate); + + // We'll be acquiring and releasing cs_main below, to allow the validation + // callbacks to run. However, we should keep the block index in a + // consistent state as we disconnect blocks -- in particular we need to + // add equal-work blocks to setBlockIndexCandidates as we disconnect. + // To avoid walking the block index repeatedly in search of candidates, + // build a map once so that we can look up candidate blocks by chain + // work as we go. + std::multimap candidate_blocks_by_work; + + { + LOCK(cs_main); + for (const auto& entry : m_blockman.m_block_index) { + CBlockIndex *candidate = entry.second; + // We don't need to put anything in our active chain into the + // multimap, because those candidates will be found and considered + // as we disconnect. + // Instead, consider only non-active-chain blocks that have at + // least as much work as where we expect the new tip to end up. + if (!m_chain.Contains(candidate) && + !CBlockIndexWorkComparator()(candidate, pindex->pprev) && + candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && + candidate->HaveTxsDownloaded()) { + candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate)); + } + } + } + // Disconnect (descendants of) pindex, and mark them invalid. while (true) { if (ShutdownRequested()) break; @@ -3243,11 +3275,24 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c setDirtyBlockIndex.insert(to_mark_failed); } + // Add any equal or more work headers to setBlockIndexCandidates + auto candidate_it = candidate_blocks_by_work.lower_bound(invalid_walk_tip->pprev->nChainWork); + while (candidate_it != candidate_blocks_by_work.end()) { + if (!CBlockIndexWorkComparator()(candidate_it->second, invalid_walk_tip->pprev)) { + setBlockIndexCandidates.insert(candidate_it->second); + candidate_it = candidate_blocks_by_work.erase(candidate_it); + } else { + ++candidate_it; + } + } + // 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; } + CheckBlockIndex(chainparams.GetConsensus()); + { LOCK(cs_main); if (m_chain.Contains(to_mark_failed)) { @@ -3261,8 +3306,13 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c setBlockIndexCandidates.erase(to_mark_failed); m_blockman.m_failed_blocks.insert(to_mark_failed); - // The resulting new best tip may not be in setBlockIndexCandidates anymore, so - // add it again. + // If any new blocks somehow arrived while we were disconnecting + // (above), then the pre-calculation of what should go into + // setBlockIndexCandidates may have missed entries. This would + // technically be an inconsistency in the block index, but if we clean + // it up here, this should be an essentially unobservable error. + // Loop back over all block index entries and add any missing entries + // to setBlockIndexCandidates. BlockMap::iterator it = g_blockman.m_block_index.begin(); while (it != g_blockman.m_block_index.end()) { if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && !(it->second->nStatus & BLOCK_CONFLICT_CHAINLOCK) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) { From 42fb46d470e3fde21a3558b4d306779e533a172e Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 25 Oct 2021 15:22:30 +0300 Subject: [PATCH 3/4] Fix block index inconsistency in MarkConflictingBlock() Same idea as in 16849 --- src/llmq/chainlocks.cpp | 39 ++++++--------------------------------- src/validation.cpp | 39 +++++++++++++++++++++++++++++++++++---- src/validation.h | 9 ++++++--- 3 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/llmq/chainlocks.cpp b/src/llmq/chainlocks.cpp index 589430a15a..66ebd547f5 100644 --- a/src/llmq/chainlocks.cpp +++ b/src/llmq/chainlocks.cpp @@ -509,43 +509,16 @@ void CChainLocksHandler::EnforceBestChainLock() } } - bool activateNeeded; CValidationState state; const auto ¶ms = Params(); - { - LOCK(cs_main); - // Go backwards through the chain referenced by clsig until we find a block that is part of the main chain. - // For each of these blocks, check if there are children that are NOT part of the chain referenced by clsig - // and mark all of them as conflicting. - while (pindex && !::ChainActive().Contains(pindex)) { - // Mark all blocks that have the same prevBlockHash but are not equal to blockHash as conflicting - auto itp = ::PrevBlockIndex().equal_range(pindex->pprev->GetBlockHash()); - for (auto jt = itp.first; jt != itp.second; ++jt) { - if (jt->second == pindex) { - continue; - } - if (!MarkConflictingBlock(state, params, jt->second)) { - LogPrintf("CChainLocksHandler::%s -- MarkConflictingBlock failed: %s\n", __func__, FormatStateMessage(state)); - // This should not have happened and we are in a state were it's not safe to continue anymore - assert(false); - } - LogPrintf("CChainLocksHandler::%s -- CLSIG (%s) marked block %s as conflicting\n", - __func__, clsig->ToString(), jt->second->GetBlockHash().ToString()); - } + // Go backwards through the chain referenced by clsig until we find a block that is part of the main chain. + // For each of these blocks, check if there are children that are NOT part of the chain referenced by clsig + // and mark all of them as conflicting. + LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- enforcing block %s via CLSIG (%s)\n", __func__, pindex->GetBlockHash().ToString(), clsig->ToString()); + EnforceBlock(state, params, pindex); - pindex = pindex->pprev; - } - // In case blocks from the correct chain are invalid at the moment, reconsider them. The only case where this - // can happen right now is when missing superblock triggers caused the main chain to be dismissed first. When - // the trigger later appears, this should bring us to the correct chain eventually. Please note that this does - // NOT enforce invalid blocks in any way, it just causes re-validation. - if (!currentBestChainLockBlockIndex->IsValid()) { - ResetBlockFailureFlags(LookupBlockIndex(currentBestChainLockBlockIndex->GetBlockHash())); - } - - activateNeeded = ::ChainActive().Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex; - } + bool activateNeeded = WITH_LOCK(::cs_main, return ::ChainActive().Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight)) != currentBestChainLockBlockIndex; if (activateNeeded) { if(!ActivateBestChain(state, params)) { diff --git a/src/validation.cpp b/src/validation.cpp index b3d7de84a2..134a8ecc17 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3337,6 +3337,41 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C return ::ChainstateActive().InvalidateBlock(state, chainparams, pindex); } +void CChainState::EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex *pindex) +{ + AssertLockNotHeld(::cs_main); + + LOCK2(m_cs_chainstate, ::cs_main); + + const CBlockIndex* pindex_walk = pindex; + + while (pindex_walk && !::ChainActive().Contains(pindex_walk)) { + // Mark all blocks that have the same prevBlockHash but are not equal to blockHash as conflicting + auto itp = ::PrevBlockIndex().equal_range(pindex_walk->pprev->GetBlockHash()); + for (auto jt = itp.first; jt != itp.second; ++jt) { + if (jt->second == pindex_walk) { + continue; + } + if (!MarkConflictingBlock(state, chainparams, jt->second)) { + LogPrintf("CChainState::%s -- MarkConflictingBlock failed: %s\n", __func__, FormatStateMessage(state)); + // This should not have happened and we are in a state were it's not safe to continue anymore + assert(false); + } + LogPrintf("CChainState::%s -- marked block %s as conflicting\n", + __func__, jt->second->GetBlockHash().ToString()); + } + pindex_walk = pindex_walk->pprev; + } + // In case blocks from the enforced chain are invalid at the moment, reconsider them. + if (!pindex->IsValid()) { + ResetBlockFailureFlags(LookupBlockIndex(pindex->GetBlockHash())); + } +} + +void EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex *pindex) { + return ::ChainstateActive().EnforceBlock(state, chainparams, pindex); +} + bool CChainState::MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) { AssertLockHeld(cs_main); @@ -3404,10 +3439,6 @@ bool CChainState::MarkConflictingBlock(CValidationState& state, const CChainPara return true; } -bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) { - return ::ChainstateActive().MarkConflictingBlock(state, chainparams, pindex); -} - void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index 8366365761..6347efdac0 100644 --- a/src/validation.h +++ b/src/validation.h @@ -693,7 +693,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); - bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Replay blocks that aren't fully applied to the database. */ @@ -740,6 +740,9 @@ private: bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + //! Mark a block as conflicting + bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + //! Mark a block as not having block data void EraseBlockData(CBlockIndex* index) EXCLUSIVE_LOCKS_REQUIRED(cs_main); }; @@ -754,8 +757,8 @@ bool PreciousBlock(CValidationState& state, const CChainParams& params, CBlockIn /** Mark a block as invalid. */ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex); -/** Mark a block as conflicting. */ -bool MarkConflictingBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +/** Enforce a block marking all the other chains as conflicting. */ +void EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); /** Remove invalidity status from a block and its descendants. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From 847a75ba4a58405e2cc65db04a5bc7855a2e5f9d Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 8 May 2019 09:19:27 -0400 Subject: [PATCH 4/4] Merge #15971: validation: Add compile-time checking for negative locking requirement in LimitValidationInterfaceQueue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 62d50ef308 Add LOCKS_EXCLUDED(cs_main) to LimitValidationInterfaceQueue(...) which does AssertLockNotHeld(cs_main) (practicalswift) Pull request description: This PR adds compile-time checking for negative locking requirements that follow from the run-time locking requirement `AssertLockNotHeld(cs_main)` in `LimitValidationInterfaceQueue(...)`. Changes: * Add `LOCKS_EXCLUDED(cs_main)` to `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)` * Add `LOCKS_EXCLUDED(cs_main)` to `CChainState::ActivateBestChain(…)`, `CChainState:: InvalidateBlock(…)` and `CChainState::RewindBlockIndex(…)` which all call `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)` * Add `LOCKS_EXCLUDED(cs_main)` to `InvalidateBlock(…)` which calls `CChainState::InvalidateBlock(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)` * Add `LOCKS_EXCLUDED(cs_main)` to `RewindBlockIndex(…)` which calls `CChainState::RewindBlockIndex(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)` ACKs for commit 62d50e: MarcoFalke: utACK 62d50ef308 Tree-SHA512: 73d092ccd08c851ae3c5d60370c369fc030c5793f5507e2faccb6f91c851ddc0ce059fbea3899f2856330d7a8c78f2ac6a2988e8268b03154f946be9e60e3be1 --- src/validation.cpp | 2 +- src/validation.h | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 134a8ecc17..d1a4e238bf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3034,7 +3034,7 @@ static void NotifyHeaderTip() LOCKS_EXCLUDED(cs_main) { } } -static void LimitValidationInterfaceQueue() { +static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) { AssertLockNotHeld(cs_main); if (GetMainSignals().CallbacksPending() > 10) { diff --git a/src/validation.h b/src/validation.h index 6347efdac0..38158dc4aa 100644 --- a/src/validation.h +++ b/src/validation.h @@ -240,7 +240,7 @@ bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::P * May not be called with cs_main held. May not be called in a * validationinterface callback. */ -bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr pblock = std::shared_ptr()); +bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr pblock = std::shared_ptr()) LOCKS_EXCLUDED(cs_main); double ConvertBitsToDouble(unsigned int nBits); CAmount GetBlockSubsidy(int nBits, int nHeight, const Consensus::Params& consensusParams, bool fSuperblockPartOnly = false); @@ -463,7 +463,6 @@ public: CBlockTreeDB& blocktree, std::set& block_index_candidates) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** Clear all data members. */ void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -679,7 +678,7 @@ public: bool ActivateBestChain( CValidationState& state, const CChainParams& chainparams, - std::shared_ptr pblock); + std::shared_ptr pblock) LOCKS_EXCLUDED(cs_main); bool AcceptBlock(const std::shared_ptr& pblock, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -692,7 +691,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); + bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); void EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -755,7 +754,7 @@ private: 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); +bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); /** Enforce a block marking all the other chains as conflicting. */ void EnforceBlock(CValidationState& state, const CChainParams& chainparams, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main);