From d121579abc40a9a995fa0fe81880f2b03e114dc3 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Wed, 15 Jan 2025 15:14:16 +0530 Subject: [PATCH 1/6] validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock - there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and it's unnecessary code complexity to correctly categorise them. --- src/validation.cpp | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3cfcd2728cd..0957b86ec78 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3729,15 +3729,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde m_blockman.m_dirty_blockindex.insert(invalid_walk_tip); setBlockIndexCandidates.erase(invalid_walk_tip); setBlockIndexCandidates.insert(invalid_walk_tip->pprev); - if (invalid_walk_tip == to_mark_failed->pprev && (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; - m_blockman.m_dirty_blockindex.insert(to_mark_failed); - } // 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); @@ -3751,9 +3744,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde 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; + // Children of failed blocks are marked as BLOCK_FAILED_VALID. + candidate->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(candidate); // If invalidated, the block is irrelevant for setBlockIndexCandidates // and for m_best_header and can be removed from the cache. @@ -3774,8 +3766,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde ++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. + // Track the last disconnected block to call InvalidChainFound on it. to_mark_failed = invalid_walk_tip; } From f284834170d625b86de05d7b98d91eb8c39ab55e Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 16 Jan 2025 13:17:12 +0530 Subject: [PATCH 2/6] refactor/validation: remove to_mark_failed to_mark_failed was useful to track last disconnected block so that it's children can be marked as BLOCK_FAILED_CHILD. since the previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock, the existing variable invalid_walk_tip is sufficient. Improve upon the variable name for `invalid_walk_tip` to make the InvalidateBlock logic easier to read. Block tip before disconnection is now tracked directly via `disconnected_tip`, and `new_tip` is the tip after the disconnect. Co-authored-by: stickies-v --- src/validation.cpp | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 0957b86ec78..013ae632e2c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3657,10 +3657,6 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde assert(pindex); if (pindex->nHeight == 0) return false; - CBlockIndex* to_mark_failed = pindex; - 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. @@ -3692,6 +3688,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } } + bool pindex_was_in_chain = false; + int disconnected = 0; + // Disconnect (descendants of) pindex, and mark them invalid. while (true) { if (m_chainman.m_interrupt) break; @@ -3705,8 +3704,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde LOCK(MempoolMutex()); if (!m_chain.Contains(pindex)) break; pindex_was_in_chain = true; - CBlockIndex *invalid_walk_tip = m_chain.Tip(); + CBlockIndex* disconnected_tip{m_chain.Tip()}; // ActivateBestChain considers blocks already in m_chain // unconditionally valid already, so force disconnect away from it. DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES}; @@ -3718,32 +3717,33 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // keeping the mempool up to date is probably futile anyway). MaybeUpdateMempoolForReorg(disconnectpool, /* fAddToMempool = */ (++disconnected <= 10) && ret); if (!ret) return false; - assert(invalid_walk_tip->pprev == m_chain.Tip()); + CBlockIndex* new_tip{m_chain.Tip()}; + assert(disconnected_tip->pprev == new_tip); // 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; - m_blockman.m_dirty_blockindex.insert(invalid_walk_tip); - setBlockIndexCandidates.erase(invalid_walk_tip); - setBlockIndexCandidates.insert(invalid_walk_tip->pprev); + disconnected_tip->nStatus |= BLOCK_FAILED_VALID; + m_blockman.m_dirty_blockindex.insert(disconnected_tip); + setBlockIndexCandidates.erase(disconnected_tip); + setBlockIndexCandidates.insert(new_tip); // Mark out-of-chain descendants of the invalidated block as invalid // 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); + auto candidate_it = highpow_outofchain_headers.lower_bound(new_tip->nChainWork); - const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip}; + const bool best_header_needs_update{m_chainman.m_best_header->GetAncestor(disconnected_tip->nHeight) == disconnected_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; + m_chainman.m_best_header = new_tip; } while (candidate_it != highpow_outofchain_headers.end()) { CBlockIndex* candidate{candidate_it->second}; - if (candidate->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip) { + if (candidate->GetAncestor(disconnected_tip->nHeight) == disconnected_tip) { // Children of failed blocks are marked as BLOCK_FAILED_VALID. candidate->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(candidate); @@ -3752,7 +3752,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde candidate_it = highpow_outofchain_headers.erase(candidate_it); continue; } - if (!CBlockIndexWorkComparator()(candidate, invalid_walk_tip->pprev) && + if (!CBlockIndexWorkComparator()(candidate, new_tip) && candidate->IsValid(BLOCK_VALID_TRANSACTIONS) && candidate->HaveNumChainTxs()) { setBlockIndexCandidates.insert(candidate); @@ -3765,16 +3765,13 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } ++candidate_it; } - - // Track the last disconnected block to call InvalidChainFound on it. - to_mark_failed = invalid_walk_tip; } m_chainman.CheckBlockIndex(); { LOCK(cs_main); - if (m_chain.Contains(to_mark_failed)) { + if (m_chain.Contains(pindex)) { // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed. return false; } @@ -3799,7 +3796,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } } - InvalidChainFound(to_mark_failed); + InvalidChainFound(pindex); } // Only notify about a new block tip if the active chain was modified. @@ -3813,8 +3810,8 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // changes. (void)m_chainman.GetNotifications().blockTip( /*state=*/GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed), - /*index=*/*to_mark_failed->pprev, - /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(to_mark_failed->pprev))); + /*index=*/*pindex->pprev, + /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(pindex->pprev))); // Fire ActiveTipChange now for the current chain tip to make sure clients are notified. // ActivateBestChain may call this as well, but not necessarily. From 350b2ad29c07327d719c0a3420a10f3a15baaad3 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 16 Jan 2025 16:55:28 +0530 Subject: [PATCH 3/6] validation: remove BLOCK_FAILED_CHILD even though we have a distinction between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD in the codebase, we don't use it for anything. since there's no functional difference between them and it's unnecessary code complexity to categorise them correctly, just mark as BLOCK_FAILED_VALID instead. --- src/chain.h | 2 +- src/node/blockstorage.cpp | 2 +- src/test/blockchain_tests.cpp | 11 ++--------- src/test/fuzz/chain.cpp | 1 - src/validation.cpp | 4 ++-- 5 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/chain.h b/src/chain.h index f5bfdb2fb4b..5810dcec107 100644 --- a/src/chain.h +++ b/src/chain.h @@ -123,7 +123,7 @@ enum BlockStatus : uint32_t { BLOCK_HAVE_MASK = BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO, BLOCK_FAILED_VALID = 32, //!< stage after last reached validness failed - BLOCK_FAILED_CHILD = 64, //!< descends from failed block + BLOCK_FAILED_CHILD = 64, //!< Unused flag that was previously set when descending from failed block BLOCK_FAILED_MASK = BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD, BLOCK_OPT_WITNESS = 128, //!< block data in blk*.dat was received with a witness-enforcing client diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ba205a9c693..490ba0609ec 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -464,7 +464,7 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha } } if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { - pindex->nStatus |= BLOCK_FAILED_CHILD; + pindex->nStatus |= BLOCK_FAILED_VALID; m_dirty_blockindex.insert(pindex); } if (pindex->pprev) { diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index 5da2841c4dd..a38d41374ef 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -130,7 +130,6 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) // tip_to_invalidate just got invalidated, so it's BLOCK_FAILED_VALID WITH_LOCK(::cs_main, assert(tip_to_invalidate->nStatus & BLOCK_FAILED_VALID)); - WITH_LOCK(::cs_main, assert((tip_to_invalidate->nStatus & BLOCK_FAILED_CHILD) == 0)); // check all ancestors of the invalidated block are validated up to BLOCK_VALID_TRANSACTIONS and are not invalid auto pindex = tip_to_invalidate->pprev; @@ -140,18 +139,12 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) pindex = pindex->pprev; } - // check all descendants of the invalidated block are BLOCK_FAILED_CHILD + // check all descendants of the invalidated block are BLOCK_FAILED_VALID pindex = orig_tip; while (pindex && pindex != tip_to_invalidate) { - WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0)); - WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_CHILD)); + WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_VALID)); pindex = pindex->pprev; } - - // don't mark already invalidated block (orig_tip is BLOCK_FAILED_CHILD) with BLOCK_FAILED_VALID again - m_node.chainman->ActiveChainstate().InvalidateBlock(state, orig_tip); - WITH_LOCK(::cs_main, assert(orig_tip->nStatus & BLOCK_FAILED_CHILD)); - WITH_LOCK(::cs_main, assert((orig_tip->nStatus & BLOCK_FAILED_VALID) == 0)); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 09053e4815c..5e9be6bc8ce 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -50,7 +50,6 @@ FUZZ_TARGET(chain) BlockStatus::BLOCK_HAVE_UNDO, BlockStatus::BLOCK_HAVE_MASK, BlockStatus::BLOCK_FAILED_VALID, - BlockStatus::BLOCK_FAILED_CHILD, BlockStatus::BLOCK_FAILED_MASK, BlockStatus::BLOCK_OPT_WITNESS, }); diff --git a/src/validation.cpp b/src/validation.cpp index 013ae632e2c..61bad4e4df8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3285,7 +3285,7 @@ CBlockIndex* Chainstate::FindMostWorkChain() // Remove the entire chain from the set. while (pindexTest != pindexFailed) { if (fFailedChain) { - pindexFailed->nStatus |= BLOCK_FAILED_CHILD; + pindexFailed->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindexFailed); } else if (fMissingData) { // If we're missing data, then add back to m_blocks_unlinked, @@ -3828,7 +3828,7 @@ void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block) for (auto& [_, block_index] : m_blockman.m_block_index) { if (invalid_block != &block_index && block_index.GetAncestor(invalid_block->nHeight) == invalid_block) { - block_index.nStatus = (block_index.nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD; + block_index.nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(&block_index); } } From 451f83467666ae0a56fd930957e83b24c030bb83 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 16 Jan 2025 17:00:58 +0530 Subject: [PATCH 4/6] validation: remove BLOCK_FAILED_MASK since it's the same as BLOCK_FAILED_VALID now --- src/chain.h | 5 ++--- src/node/blockstorage.cpp | 2 +- src/rpc/blockchain.cpp | 2 +- src/rpc/mining.cpp | 2 +- src/test/blockchain_tests.cpp | 2 +- src/test/fuzz/chain.cpp | 1 - src/validation.cpp | 28 ++++++++++++++-------------- 7 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/chain.h b/src/chain.h index 5810dcec107..87938d3710e 100644 --- a/src/chain.h +++ b/src/chain.h @@ -124,7 +124,6 @@ enum BlockStatus : uint32_t { BLOCK_FAILED_VALID = 32, //!< stage after last reached validness failed BLOCK_FAILED_CHILD = 64, //!< Unused flag that was previously set when descending from failed block - BLOCK_FAILED_MASK = BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD, BLOCK_OPT_WITNESS = 128, //!< block data in blk*.dat was received with a witness-enforcing client @@ -297,7 +296,7 @@ public: { AssertLockHeld(::cs_main); assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed. - if (nStatus & BLOCK_FAILED_MASK) + if (nStatus & BLOCK_FAILED_VALID) return false; return ((nStatus & BLOCK_VALID_MASK) >= nUpTo); } @@ -308,7 +307,7 @@ public: { AssertLockHeld(::cs_main); assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed. - if (nStatus & BLOCK_FAILED_MASK) return false; + if (nStatus & BLOCK_FAILED_VALID) return false; if ((nStatus & BLOCK_VALID_MASK) < nUpTo) { nStatus = (nStatus & ~BLOCK_VALID_MASK) | nUpTo; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 490ba0609ec..ecc061a7d98 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -463,7 +463,7 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha pindex->m_chain_tx_count = pindex->nTx; } } - if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { + if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) { pindex->nStatus |= BLOCK_FAILED_VALID; m_dirty_blockindex.insert(pindex); } diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ca872cf6ac8..aa40faa7753 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1558,7 +1558,7 @@ static RPCHelpMan getchaintips() if (active_chain.Contains(block)) { // This block is part of the currently active chain. status = "active"; - } else if (block->nStatus & BLOCK_FAILED_MASK) { + } else if (block->nStatus & BLOCK_FAILED_VALID) { // This block or one of its ancestors is invalid. status = "invalid"; } else if (!block->HaveNumChainTxs()) { diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index f11305f16b8..cee92f72889 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -739,7 +739,7 @@ static RPCHelpMan getblocktemplate() if (pindex) { if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) return "duplicate"; - if (pindex->nStatus & BLOCK_FAILED_MASK) + if (pindex->nStatus & BLOCK_FAILED_VALID) return "duplicate-invalid"; return "duplicate-inconclusive"; } diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index a38d41374ef..0f8ac3c4424 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -135,7 +135,7 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) auto pindex = tip_to_invalidate->pprev; while (pindex) { WITH_LOCK(::cs_main, assert(pindex->IsValid(BLOCK_VALID_TRANSACTIONS))); - WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0)); + WITH_LOCK(::cs_main, assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0)); pindex = pindex->pprev; } diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 5e9be6bc8ce..1b347aee8e4 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -50,7 +50,6 @@ FUZZ_TARGET(chain) BlockStatus::BLOCK_HAVE_UNDO, BlockStatus::BLOCK_HAVE_MASK, BlockStatus::BLOCK_FAILED_VALID, - BlockStatus::BLOCK_FAILED_MASK, BlockStatus::BLOCK_OPT_WITNESS, }); if (block_status & ~BLOCK_VALID_MASK) { diff --git a/src/validation.cpp b/src/validation.cpp index 61bad4e4df8..074e489f964 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3274,7 +3274,7 @@ CBlockIndex* Chainstate::FindMostWorkChain() // which block files have been deleted. Remove those as candidates // for the most work chain if we come across them; we can't switch // to a chain unless we have all the non-active-chain parent blocks. - bool fFailedChain = pindexTest->nStatus & BLOCK_FAILED_MASK; + bool fFailedChain = pindexTest->nStatus & BLOCK_FAILED_VALID; bool fMissingData = !(pindexTest->nStatus & BLOCK_HAVE_DATA); if (fFailedChain || fMissingData) { // Candidate chain is not usable (either invalid or missing data) @@ -3682,7 +3682,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // at least as good with CBlockIndexWorkComparator as the new tip. if (!m_chain.Contains(candidate) && !CBlockIndexWorkComparator()(candidate, pindex->pprev) && - !(candidate->nStatus & BLOCK_FAILED_MASK)) { + !(candidate->nStatus & BLOCK_FAILED_VALID)) { highpow_outofchain_headers.insert({candidate->nChainWork, candidate}); } } @@ -3777,7 +3777,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde } // Mark pindex as invalid if it never was in the main chain - if (!pindex_was_in_chain && !(pindex->nStatus & BLOCK_FAILED_MASK)) { + if (!pindex_was_in_chain && !(pindex->nStatus & BLOCK_FAILED_VALID)) { pindex->nStatus |= BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(pindex); setBlockIndexCandidates.erase(pindex); @@ -3841,8 +3841,8 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { // Remove the invalidity flag from this block and all its descendants and ancestors. for (auto& [_, block_index] : m_blockman.m_block_index) { - if ((block_index.nStatus & BLOCK_FAILED_MASK) && (block_index.GetAncestor(nHeight) == pindex || pindex->GetAncestor(block_index.nHeight) == &block_index)) { - block_index.nStatus &= ~BLOCK_FAILED_MASK; + if ((block_index.nStatus & BLOCK_FAILED_VALID) && (block_index.GetAncestor(nHeight) == pindex || pindex->GetAncestor(block_index.nHeight) == &block_index)) { + block_index.nStatus &= ~BLOCK_FAILED_VALID; m_blockman.m_dirty_blockindex.insert(&block_index); if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveNumChainTxs() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &block_index)) { setBlockIndexCandidates.insert(&block_index); @@ -4319,7 +4319,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida CBlockIndex* pindex = &(miSelf->second); if (ppindex) *ppindex = pindex; - if (pindex->nStatus & BLOCK_FAILED_MASK) { + if (pindex->nStatus & BLOCK_FAILED_VALID) { LogDebug(BCLog::VALIDATION, "%s: block %s is marked invalid\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_CACHED_INVALID, "duplicate-invalid"); } @@ -4339,7 +4339,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida return state.Invalid(BlockValidationResult::BLOCK_MISSING_PREV, "prev-blk-not-found"); } pindexPrev = &((*mi).second); - if (pindexPrev->nStatus & BLOCK_FAILED_MASK) { + if (pindexPrev->nStatus & BLOCK_FAILED_VALID) { 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"); } @@ -5004,7 +5004,7 @@ bool ChainstateManager::LoadBlockIndex() chainstate->TryAddBlockIndexCandidate(pindex); } } - if (pindex->nStatus & BLOCK_FAILED_MASK && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) { + if (pindex->nStatus & BLOCK_FAILED_VALID && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) { m_best_invalid = pindex; } if (pindex->IsValid(BLOCK_VALID_TREE) && (m_best_header == nullptr || CBlockIndexWorkComparator()(m_best_header, pindex))) @@ -5252,7 +5252,7 @@ void ChainstateManager::CheckBlockIndex() const // are not yet validated. CChain best_hdr_chain; assert(m_best_header); - assert(!(m_best_header->nStatus & BLOCK_FAILED_MASK)); + assert(!(m_best_header->nStatus & BLOCK_FAILED_VALID)); best_hdr_chain.SetTip(*m_best_header); std::multimap forward; @@ -5365,9 +5365,9 @@ void ChainstateManager::CheckBlockIndex() const if ((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_SCRIPTS) assert(pindexFirstNotScriptsValid == nullptr); // SCRIPTS valid implies all parents are SCRIPTS valid 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. + assert((pindex->nStatus & BLOCK_FAILED_VALID) == 0); // The failed flag 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 + assert(pindex->nStatus & BLOCK_FAILED_VALID); // Invalid blocks and their descendants must be marked as invalid } // Make sure m_chain_tx_count sum is correctly computed. if (!pindex->pprev) { @@ -5382,7 +5382,7 @@ void ChainstateManager::CheckBlockIndex() const 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); + assert((pindex->nStatus & BLOCK_FAILED_VALID) || pindex->nChainWork <= m_best_header->nChainWork); // Chainstate-specific checks on setBlockIndexCandidates for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { @@ -5726,7 +5726,7 @@ util::Result ChainstateManager::ActivateSnapshot( base_blockhash.ToString()))}; } - bool start_block_invalid = snapshot_start_block->nStatus & BLOCK_FAILED_MASK; + bool start_block_invalid = snapshot_start_block->nStatus & BLOCK_FAILED_VALID; if (start_block_invalid) { return util::Error{Untranslated(strprintf("The base block header (%s) is part of an invalid chain", base_blockhash.ToString()))}; } @@ -6439,7 +6439,7 @@ void ChainstateManager::RecalculateBestHeader() AssertLockHeld(cs_main); m_best_header = ActiveChain().Tip(); for (auto& entry : m_blockman.m_block_index) { - if (!(entry.second.nStatus & BLOCK_FAILED_MASK) && m_best_header->nChainWork < entry.second.nChainWork) { + if (!(entry.second.nStatus & BLOCK_FAILED_VALID) && m_best_header->nChainWork < entry.second.nChainWork) { m_best_header = &entry.second; } } From 87833a18ed72dc055fbbefda52b201fe479d3b8f Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 7 Feb 2025 12:56:09 +0530 Subject: [PATCH 5/6] validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk - there maybe existing block indexes stored in disk with BLOCK_FAILED_CHILD - since they don't exist anymore, clean up block index entries with BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID. --- src/node/blockstorage.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ecc061a7d98..47a7d656a8f 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -464,7 +464,8 @@ bool BlockManager::LoadBlockIndex(const std::optional& snapshot_blockha } } if (!(pindex->nStatus & BLOCK_FAILED_VALID) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_VALID)) { - pindex->nStatus |= BLOCK_FAILED_VALID; + // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID. + pindex->nStatus = (pindex->nStatus & ~BLOCK_FAILED_CHILD) | BLOCK_FAILED_VALID; m_dirty_blockindex.insert(pindex); } if (pindex->pprev) { From cfadc8038c08f9804c81af7950164483761f1db5 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Wed, 8 Oct 2025 15:23:53 +0530 Subject: [PATCH 6/6] test: check LoadBlockIndex correctly recomputes invalidity flags Add a test for block index transitioning from legacy BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID behavior. In the scenario where a valid block has a BLOCK_FAILED_CHILD parent and a BLOCK_FAILED_VALID grandparent, ensure that all three blocks are correctly marked as BLOCK_FAILED_VALID after reloading the block index. --- src/test/blockchain_tests.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index 0f8ac3c4424..54220954040 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -124,6 +124,11 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) // Check BlockStatus when doing InvalidateBlock() BlockValidationState state; auto* orig_tip = active.Tip(); + + CBlockIndex* block_100 = active[100]; + CBlockIndex* block_99 = active[99]; + CBlockIndex* block_98 = active[98]; + int height_to_invalidate = orig_tip->nHeight - 10; auto* tip_to_invalidate = active[height_to_invalidate]; m_node.chainman->ActiveChainstate().InvalidateBlock(state, tip_to_invalidate); @@ -145,6 +150,26 @@ BOOST_FIXTURE_TEST_CASE(invalidate_block, TestChain100Setup) WITH_LOCK(::cs_main, assert(pindex->nStatus & BLOCK_FAILED_VALID)); pindex = pindex->pprev; } + + { + // consider the chain of blocks block_98 <- block_99 <- block_100 + // intentionally mark: + // - block_98: BLOCK_FAILED_VALID (already marked as BLOCK_FAILED_VALID from previous test) + // - block_99: BLOCK_FAILED_CHILD (clear BLOCK_FAILED_VALID from previous test and mark as BLOCK_FAILED_CHILD) + // - block_100: not invalid (clear BLOCK_FAILED_VALID from previous test) + LOCK(::cs_main); + assert(block_98->nStatus & BLOCK_FAILED_VALID); + block_99->nStatus = (block_99->nStatus & ~BLOCK_FAILED_VALID) | BLOCK_FAILED_CHILD; + block_100->nStatus = (block_100->nStatus & ~BLOCK_FAILED_VALID); + + // Reload block index to recompute block status validity flags from disk. + m_node.chainman->LoadBlockIndex(); + + // check block_98, block_99, block_100 is marked as BLOCK_FAILED_VALID after reloading from disk + assert(block_98->nStatus & BLOCK_FAILED_VALID); + assert(block_99->nStatus & BLOCK_FAILED_VALID); + assert(block_100->nStatus & BLOCK_FAILED_VALID); + } } BOOST_AUTO_TEST_SUITE_END()