From 15fa5b5a908d1019fc1b3042901b42bee0a1bd95 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sat, 30 Nov 2024 23:46:10 -0500 Subject: [PATCH 1/7] validation: call InvalidBlockFound also from AcceptBlock When a block it found invalid during acceptance (but before connection) we now mark its descendants with BLOCK_FAILED_CHILD and update m_best_header when these things weren't done reliably before. This does not fix a serious bug because the flags and m_best_header were being set on a best-effort basis before and not used for anything critical. Setting these reliably has a slight performance cost (iterating over the entire block index) but leads to more consistency in validation and allows removing m_failed_blocks in a later commit. This can only be triggered by providing a block with sufficient PoW that is otherwise invalid, so it is not a DoS vector. --- src/validation.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1213d8be9f9..df690f2548b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4537,9 +4537,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, if (!CheckBlock(block, state, params.GetConsensus()) || !ContextualCheckBlock(block, state, *this, pindex->pprev)) { - if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { - pindex->nStatus |= BLOCK_FAILED_VALID; - m_blockman.m_dirty_blockindex.insert(pindex); + if (Assume(state.IsInvalid())) { + ActiveChainstate().InvalidBlockFound(pindex, state); } LogError("%s: %s\n", __func__, state.ToString()); return false; From 4c29326183ba3c9d0b198cb2cec37c3119862c19 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 18 Mar 2025 16:30:52 -0400 Subject: [PATCH 2/7] validation: cache all headers with enough PoW in invalidateblock We now include blocks without HaveNumChainTxs() / lower validation status than BLOCK_VALID_TRANSACTIONS. These checks are still performed at the spot where we use the cache to insert into setBlockIndexCandidates. This is in preparation for using the cache for more things than just setBlockIndexCandidates candidates in the following commits. Co-authored-by: stickies-v --- src/validation.cpp | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index df690f2548b..067e9b1695f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3690,7 +3690,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // 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; + std::multimap highpow_outofchain_headers; { LOCK(cs_main); @@ -3699,13 +3699,12 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // 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. + // Instead, consider only non-active-chain blocks that score + // at least as good with CBlockIndexWorkComparator as the new tip. if (!m_chain.Contains(candidate) && - !CBlockIndexWorkComparator()(candidate, pindex->pprev) && - candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && - candidate->HaveNumChainTxs()) { - candidate_blocks_by_work.insert(std::make_pair(candidate->nChainWork, candidate)); + !CBlockIndexWorkComparator()(candidate, pindex->pprev) && + !(candidate->nStatus & BLOCK_FAILED_MASK)) { + highpow_outofchain_headers.insert({candidate->nChainWork, candidate}); } } } @@ -3755,11 +3754,14 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } // 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); + auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork); + while (candidate_it != highpow_outofchain_headers.end()) { + CBlockIndex* candidate{candidate_it->second}; + if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) && + candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && + candidate->HaveNumChainTxs()) { + setBlockIndexCandidates.insert(candidate); + candidate_it = highpow_outofchain_headers.erase(candidate_it); } else { ++candidate_it; } From 8e39f2d20d09592035ae048d0cfe955c733310d9 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sat, 30 Nov 2024 15:39:55 -0500 Subject: [PATCH 3/7] 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. Entries from highpow_outofchain_headers are now only removed if made invalid, no longer after inserting into setBlockIndexCandidates, because they might still become invalid later in the second case. This means that blocks could be inserted multiple times now into setBlockIndexCandidates - this won't have any effect, so behavior isn't changed. --- src/validation.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 067e9b1695f..941e433f008 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3753,18 +3753,30 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde m_blockman.m_dirty_blockindex.insert(to_mark_failed); } - // Add any equal or more work headers to setBlockIndexCandidates + // Mark out-of-chain descendants of the invalidated block as invalid + // (possibly replacing a pre-existing BLOCK_FAILED_VALID with BLOCK_FAILED_CHILD) + // Add any equal or more work headers that are not invalidated to setBlockIndexCandidates auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork); while (candidate_it != highpow_outofchain_headers.end()) { CBlockIndex* candidate{candidate_it->second}; + if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) { + // Children of failed blocks should be marked as BLOCK_FAILED_CHILD instead. + candidate->nStatus &= ~BLOCK_FAILED_VALID; + candidate->nStatus |= BLOCK_FAILED_CHILD; + m_blockman.m_dirty_blockindex.insert(candidate); + // If invalidated, the block is irrelevant for setBlockIndexCandidates + // and can be removed from the cache. + candidate_it = highpow_outofchain_headers.erase(candidate_it); + continue; + } if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) && candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && candidate->HaveNumChainTxs()) { setBlockIndexCandidates.insert(candidate); - candidate_it = highpow_outofchain_headers.erase(candidate_it); - } else { - ++candidate_it; + // Do not remove candidate from the highpow_outofchain_headers cache, because it might be a descendant of the block being invalidated + // which needs to be marked failed later. } + ++candidate_it; } // Track the last disconnected block, so we can correct its BLOCK_FAILED_CHILD status in future From 9a70883002e1fee76c24810808af4fb43f2c8cf5 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 25 Nov 2024 17:07:53 -0500 Subject: [PATCH 4/7] 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. Co-authored-by: stringintech --- src/validation.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 941e433f008..6e6de0ea6e4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3756,7 +3756,15 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // Mark out-of-chain descendants of the invalidated block as invalid // (possibly replacing a pre-existing BLOCK_FAILED_VALID with BLOCK_FAILED_CHILD) // Add any equal or more work headers that are not invalidated to setBlockIndexCandidates + // Recalculate m_best_header if it became invalid. auto candidate_it = highpow_outofchain_headers.lower_bound(invalid_walk_tip->pprev->nChainWork); + + const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip}; + if (best_header_needs_update) { + // pprev is definitely still valid at this point, but there may be better ones + m_chainman.m_best_header = invalid_walk_tip->pprev; + } + while (candidate_it != highpow_outofchain_headers.end()) { CBlockIndex* candidate{candidate_it->second}; if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) { @@ -3765,7 +3773,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde candidate->nStatus |= BLOCK_FAILED_CHILD; m_blockman.m_dirty_blockindex.insert(candidate); // If invalidated, the block is irrelevant for setBlockIndexCandidates - // and can be removed from the cache. + // and for m_best_header and can be removed from the cache. candidate_it = highpow_outofchain_headers.erase(candidate_it); continue; } @@ -3776,6 +3784,10 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // Do not remove candidate from the highpow_outofchain_headers cache, because it might be a descendant of the block being invalidated // which needs to be marked failed later. } + if (best_header_needs_update && + m_chainman.m_best_header->nChainWork < candidate->nChainWork) { + m_chainman.m_best_header = candidate; + } ++candidate_it; } From ed764ea2b4edb3cf1925a4bff5f39567a8be54ac Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 2 Dec 2024 11:22:26 -0500 Subject: [PATCH 5/7] 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 6e6de0ea6e4..52c636c8151 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5305,6 +5305,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; @@ -5418,6 +5419,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); // Invalid blocks and their descendants must be marked as invalid } // Make sure m_chain_tx_count sum is correctly computed. if (!pindex->pprev) { @@ -5431,6 +5434,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 ee673b9aa0157d94220722491f135aef23142521 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Sun, 1 Dec 2024 00:08:51 -0500 Subject: [PATCH 6/7] validation: remove m_failed_blocks After changes in previous commits, we now mark all blocks that descend from an invalid block immediately as the block is found invalid. This happens both in the AcceptBlock and ConnectBlock stages of block validation. As a result, the pindexPrev->nStatus check in AcceptBlockHeader is now sufficient to detect invalid blocks and checking m_failed_blocks there is no longer necessary. --- src/validation.cpp | 43 ------------------------------------------- src/validation.h | 21 --------------------- 2 files changed, 64 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 52c636c8151..55cc721a04b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2091,7 +2091,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); @@ -3810,7 +3809,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); setBlockIndexCandidates.erase(pindex); - m_chainman.m_failed_blocks.insert(pindex); } // If any new blocks somehow arrived while we were disconnecting @@ -3878,7 +3876,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); } } @@ -3887,7 +3884,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; } @@ -4383,45 +4379,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 e361c7af101..a3951a58c66 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1038,27 +1038,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}; From f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 5 Jun 2025 13:54:27 -0400 Subject: [PATCH 7/7] doc: Improve m_best_header documentation --- src/validation.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/validation.h b/src/validation.h index a3951a58c66..1e8770745db 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1038,7 +1038,10 @@ public: } - /** Best header we've seen so far (used for getheaders queries' starting points). */ + /** Best header we've seen so far for which the block is not known to be invalid + (used, among others, for getheaders queries' starting points). + In case of multiple best headers with the same work, it could point to any + because CBlockIndexWorkComparator tiebreaker rules are not applied. */ CBlockIndex* m_best_header GUARDED_BY(::cs_main){nullptr}; //! The total number of bytes available for us to use across all in-memory