From 61e8d9631b668542973bad02e804b9ff943623ef Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sat, 30 Nov 2024 23:46:10 -0500 Subject: [PATCH 1/5] validation: call InvalidChainfound also from AcceptBlock In this scenario we have stored a valid header with an invalid (but not mutated!) block, so it can only be triggered by doing the necessary PoW. The added call ensures that descendant blocks failure flags and m_best_header will be set correctly - at the cost of looping over the entire block index, which, because of the mentioned necessary PoW, is not a DoS vector. --- src/validation.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.cpp b/src/validation.cpp index 95f3bc58d7d..48a7188e718 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4520,6 +4520,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); + ActiveChainstate().InvalidChainFound(pindex); } LogError("%s: %s\n", __func__, state.ToString()); return false; From 7f88af8b240f1f83d0b99fe53cac606104bb416c Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sat, 30 Nov 2024 15:39:55 -0500 Subject: [PATCH 2/5] validation: in invalidateblock, mark children as invalid right away Before, they would be marked as invalid only after disconnecting multiple blocks, letting go of cs_main in the meantime. This is in preparation for adding a check to CheckBlockIndex() requiring that descendants of invalid block indexes are always marked as invalid. --- src/validation.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 48a7188e718..094119cc74f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3666,6 +3666,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // build a map once so that we can look up candidate blocks by chain // work as we go. std::multimap candidate_blocks_by_work; + // Map to cache candidates not in the main chain that might need invalidating. + // Maps fork block in chain to the candidates for invalidation. + std::multimap cand_invalid_descendants; { LOCK(cs_main); @@ -3682,6 +3685,11 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde candidate->HaveNumChainTxs()) { candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate)); } + // Similarly, populate cache for blocks not in main chain to invalidate + if (!m_chain.Contains(candidate) && + !CBlockIndexWorkComparator()(candidate, pindex->pprev)) { + cand_invalid_descendants.insert(std::make_pair(m_chain.FindFork(candidate), candidate)); + } } } @@ -3729,6 +3737,12 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde m_blockman.m_dirty_blockindex.insert(to_mark_failed); } + // Mark descendants of the invalidated block as invalid + auto range = cand_invalid_descendants.equal_range(invalid_walk_tip); + for (auto it = range.first; it != range.second; ++it) { + it->second->nStatus |= BLOCK_FAILED_CHILD; + } + // 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()) { From 89ce4b0cdb6f73b23f71e566a69f50a0928fca77 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 25 Nov 2024 17:07:53 -0500 Subject: [PATCH 3/5] validation: in invalidateblock, calculate m_best_header right away Before, m_best_header would be calculated only after disconnecting multiple blocks, letting go of cs_main in the meantime. This is in preparation for adding checks to CheckBlockIndex() requiring that m_best_header is the most-work header not known to be invalid. --- src/validation.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 094119cc74f..ee0828149e9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3669,6 +3669,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // Map to cache candidates not in the main chain that might need invalidating. // Maps fork block in chain to the candidates for invalidation. std::multimap cand_invalid_descendants; + // Map to cache candidates for m_best_header + std::multimap best_header_blocks_by_work; { LOCK(cs_main); @@ -3689,6 +3691,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde if (!m_chain.Contains(candidate) && !CBlockIndexWorkComparator()(candidate, pindex->pprev)) { cand_invalid_descendants.insert(std::make_pair(m_chain.FindFork(candidate), candidate)); + best_header_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate)); } } } @@ -3743,6 +3746,22 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde it->second->nStatus |= BLOCK_FAILED_CHILD; } + // Determine new best header + if (m_chainman.m_best_header->nStatus & BLOCK_FAILED_MASK) { + m_chainman.m_best_header = invalid_walk_tip->pprev; + auto best_header_it = best_header_blocks_by_work.lower_bound(m_chainman.m_best_header->nChainWork); + while (best_header_it != best_header_blocks_by_work.end()) { + if (best_header_it->second->nStatus & BLOCK_FAILED_MASK) { + best_header_it = best_header_blocks_by_work.erase(best_header_it); + } else { + if (!CBlockIndexWorkComparator()(best_header_it->second, m_chainman.m_best_header)) { + m_chainman.m_best_header = best_header_it->second; + } + ++best_header_it; + } + } + } + // 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()) { From 528b8cc98d28d198ab8876174a2550cf731f0b8d Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 2 Dec 2024 11:22:26 -0500 Subject: [PATCH 4/5] validation: Add more checks to CheckBlockIndex() This adds checks that 1) Descendants of invalid block indexes are also marked invalid 2) m_best_header cannot be invalid, and there can be no valid block with more work than it. --- src/validation.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index ee0828149e9..8e9f11d425c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5288,6 +5288,7 @@ void ChainstateManager::CheckBlockIndex() // are not yet validated. CChain best_hdr_chain; assert(m_best_header); + assert(!(m_best_header->nStatus & BLOCK_FAILED_MASK)); best_hdr_chain.SetTip(*m_best_header); std::multimap forward; @@ -5401,6 +5402,8 @@ void ChainstateManager::CheckBlockIndex() if (pindexFirstInvalid == nullptr) { // Checks for not-invalid blocks. assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents. + } else { + assert((pindex->nStatus & BLOCK_FAILED_MASK)); // Descendants of invalid blocks must also be marked as invalid } // Make sure m_chain_tx_count sum is correctly computed. if (!pindex->pprev) { @@ -5414,6 +5417,8 @@ void ChainstateManager::CheckBlockIndex() // block, and must be set if it is. assert((pindex->m_chain_tx_count != 0) == (pindex == snap_base)); } + // There should be no block with more work than m_best_header, unless it's known to be invalid + assert((pindex->nStatus & BLOCK_FAILED_MASK) || pindex->nChainWork <= m_best_header->nChainWork); // Chainstate-specific checks on setBlockIndexCandidates for (auto c : GetAll()) { From b2136da98df61f10ff8283d42f112f69f041fa7a Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sun, 1 Dec 2024 00:08:51 -0500 Subject: [PATCH 5/5] validation: remove m_failed_blocks We now mark all blocks that descend from an invalid block immediately as the invalid block is encountered (by iterating over the entire block index). As a result, m_failed_blocks, which was a heuristic to only mark descendants of failed blocks as failed when necessary, (i.e., when we have do decide whether to add another descendant or not) is no longer required. --- src/validation.cpp | 43 ------------------------------------------- src/validation.h | 21 --------------------- 2 files changed, 64 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 8e9f11d425c..b571d1d6b41 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2081,7 +2081,6 @@ void Chainstate::InvalidBlockFound(CBlockIndex* pindex, const BlockValidationSta AssertLockHeld(cs_main); if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; - m_chainman.m_failed_blocks.insert(pindex); m_blockman.m_dirty_blockindex.insert(pindex); setBlockIndexCandidates.erase(pindex); InvalidChainFound(pindex); @@ -3791,7 +3790,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde to_mark_failed->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(to_mark_failed); setBlockIndexCandidates.erase(to_mark_failed); - m_chainman.m_failed_blocks.insert(to_mark_failed); // If any new blocks somehow arrived while we were disconnecting // (above), then the pre-calculation of what should go into @@ -3857,7 +3855,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { // Reset invalid block marker if it was pointing to one of those. m_chainman.m_best_invalid = nullptr; } - m_chainman.m_failed_blocks.erase(&block_index); } } @@ -3866,7 +3863,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { if (pindex->nStatus & BLOCK_FAILED_MASK) { pindex->nStatus &= ~BLOCK_FAILED_MASK; m_blockman.m_dirty_blockindex.insert(pindex); - m_chainman.m_failed_blocks.erase(pindex); } pindex = pindex->pprev; } @@ -4370,45 +4366,6 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida LogDebug(BCLog::VALIDATION, "%s: Consensus::ContextualCheckBlockHeader: %s, %s\n", __func__, hash.ToString(), state.ToString()); return false; } - - /* Determine if this block descends from any block which has been found - * invalid (m_failed_blocks), then mark pindexPrev and any blocks between - * them as failed. For example: - * - * D3 - * / - * B2 - C2 - * / \ - * A D2 - E2 - F2 - * \ - * B1 - C1 - D1 - E1 - * - * In the case that we attempted to reorg from E1 to F2, only to find - * C2 to be invalid, we would mark D2, E2, and F2 as BLOCK_FAILED_CHILD - * but NOT D3 (it was not in any of our candidate sets at the time). - * - * In any case D3 will also be marked as BLOCK_FAILED_CHILD at restart - * in LoadBlockIndex. - */ - if (!pindexPrev->IsValid(BLOCK_VALID_SCRIPTS)) { - // The above does not mean "invalid": it checks if the previous block - // hasn't been validated up to BLOCK_VALID_SCRIPTS. This is a performance - // optimization, in the common case of adding a new block to the tip, - // we don't need to iterate over the failed blocks list. - for (const CBlockIndex* failedit : m_failed_blocks) { - if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { - assert(failedit->nStatus & BLOCK_FAILED_VALID); - CBlockIndex* invalid_walk = pindexPrev; - while (invalid_walk != failedit) { - invalid_walk->nStatus |= BLOCK_FAILED_CHILD; - m_blockman.m_dirty_blockindex.insert(invalid_walk); - invalid_walk = invalid_walk->pprev; - } - LogDebug(BCLog::VALIDATION, "header %s has prev block invalid: %s\n", hash.ToString(), block.hashPrevBlock.ToString()); - return state.Invalid(BlockValidationResult::BLOCK_INVALID_PREV, "bad-prevblk"); - } - } - } } if (!min_pow_checked) { LogDebug(BCLog::VALIDATION, "%s: not adding new block header %s, missing anti-dos proof-of-work validation\n", __func__, hash.ToString()); diff --git a/src/validation.h b/src/validation.h index 723babca315..257f85c4fc7 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1041,27 +1041,6 @@ public: } - /** - * In order to efficiently track invalidity of headers, we keep the set of - * blocks which we tried to connect and found to be invalid here (ie which - * were set to BLOCK_FAILED_VALID since the last restart). We can then - * walk this set and check if a new header is a descendant of something in - * this set, preventing us from having to walk m_block_index when we try - * to connect a bad block and fail. - * - * While this is more complicated than marking everything which descends - * from an invalid block as invalid at the time we discover it to be - * invalid, doing so would require walking all of m_block_index to find all - * descendants. Since this case should be very rare, keeping track of all - * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as - * well. - * - * Because we already walk m_block_index in height-order at startup, we go - * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, - * instead of putting things in this set. - */ - std::set m_failed_blocks; - /** Best header we've seen so far (used for getheaders queries' starting points). */ CBlockIndex* m_best_header GUARDED_BY(::cs_main){nullptr};