diff --git a/src/validation.h b/src/validation.h --- a/src/validation.h +++ b/src/validation.h @@ -832,16 +832,17 @@ CBlockIndex *pindex) LOCKS_EXCLUDED(cs_main); /** Mark a block as invalid. */ bool InvalidateBlock(const Config &config, BlockValidationState &state, - CBlockIndex *pindex); + CBlockIndex *pindex) LOCKS_EXCLUDED(m_cs_chainstate); /** Park a block. */ bool ParkBlock(const Config &config, BlockValidationState &state, - CBlockIndex *pindex); + CBlockIndex *pindex) LOCKS_EXCLUDED(m_cs_chainstate); /** * Finalize a block. * A finalized block can not be reorged in any way. */ bool FinalizeBlock(const Config &config, BlockValidationState &state, - CBlockIndex *pindex) LOCKS_EXCLUDED(cs_main); + CBlockIndex *pindex) + LOCKS_EXCLUDED(cs_main, m_cs_chainstate); void ResetBlockFailureFlags(CBlockIndex *pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); template @@ -911,7 +912,8 @@ const Consensus::Params ¶ms) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool UnwindBlock(const Config &config, BlockValidationState &state, - CBlockIndex *pindex, bool invalidate); + CBlockIndex *pindex, bool invalidate) + EXCLUSIVE_LOCKS_REQUIRED(m_cs_chainstate); }; /** diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2968,6 +2968,42 @@ CBlockIndex *to_mark_failed_or_parked = pindex; bool pindex_was_in_chain = false; int disconnected = 0; + const CChainParams &chainparams = config.GetChainParams(); + + // We do not allow ActivateBestChain() to run while UnwindBlock() is + // running, as that could cause the tip to change while we disconnect + // blocks. (Note for backport of Core PR16849: we acquire + // LOCK(m_cs_chainstate) in the Park, Invalidate and FinalizeBlock functions + // due to differences in our code) + AssertLockHeld(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 : mapBlockIndex) { + 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(BlockValidity::TRANSACTIONS) && + candidate->HaveTxsDownloaded()) { + candidate_blocks_by_work.insert( + std::make_pair(candidate->nChainWork, candidate)); + } + } + } // Disconnect (descendants of) pindex, and mark them invalid. while (true) { @@ -2989,8 +3025,7 @@ DisconnectedBlockTransactions disconnectpool; - bool ret = - DisconnectTip(config.GetChainParams(), state, &disconnectpool); + bool ret = DisconnectTip(chainparams, state, &disconnectpool); // DisconnectTip will add transactions to disconnectpool. // Adjust the mempool to be consistent with the new tip, adding @@ -3035,12 +3070,27 @@ setDirtyBlockIndex.insert(to_mark_failed_or_parked); } + // 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 // FailedParent (or ParkedParent) status in future iterations, or, if // it's the last one, call InvalidChainFound on it. to_mark_failed_or_parked = invalid_walk_tip; } + CheckBlockIndex(chainparams.GetConsensus()); + { LOCK(cs_main); if (m_chain.Contains(to_mark_failed_or_parked)) { @@ -3059,8 +3109,13 @@ m_failed_blocks.insert(to_mark_failed_or_parked); } - // 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. for (const std::pair &it : mapBlockIndex) { CBlockIndex *i = it.second; @@ -3087,17 +3142,29 @@ bool CChainState::InvalidateBlock(const Config &config, BlockValidationState &state, CBlockIndex *pindex) { + AssertLockNotHeld(m_cs_chainstate); + // See 'Note for backport of Core PR16849' in CChainState::UnwindBlock + LOCK(m_cs_chainstate); + return UnwindBlock(config, state, pindex, true); } bool CChainState::ParkBlock(const Config &config, BlockValidationState &state, CBlockIndex *pindex) { + AssertLockNotHeld(m_cs_chainstate); + // See 'Note for backport of Core PR16849' in CChainState::UnwindBlock + LOCK(m_cs_chainstate); + return UnwindBlock(config, state, pindex, false); } bool CChainState::FinalizeBlock(const Config &config, BlockValidationState &state, CBlockIndex *pindex) { + AssertLockNotHeld(m_cs_chainstate); + // See 'Note for backport of Core PR16849' in CChainState::UnwindBlock + LOCK(m_cs_chainstate); + AssertLockNotHeld(cs_main); CBlockIndex *pindexToInvalidate = nullptr; {