From 1b0b3e2c2cbb60bb6a4e352c9bad221ec416e340 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 19 Mar 2026 22:07:26 +0530 Subject: [PATCH 1/3] validation: remove redundant marking in FindMostWorkChain since ed764ea, all descendants in m_block_index are required to be marked BLOCK_FAILED_VALID when an invalid block is encountered, as enforced by a CheckBlockIndex assert. remove the now-redundant marking in FindMostWorkChain's inner loop, so it is only responsible for cleaning up setBlockIndexCandidates, not for modifying block validity state --- src/validation.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 00c1bab501f..e0a8c1b38db 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3142,13 +3142,10 @@ CBlockIndex* Chainstate::FindMostWorkChain() CBlockIndex *pindexFailed = pindexNew; // Remove the entire chain from the set. while (pindexTest != pindexFailed) { - if (fFailedChain) { - 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, - // so that if the block arrives in the future we can try adding - // to setBlockIndexCandidates again. + if (fMissingData && !fFailedChain) { + // If we're missing data and not a descendant of an invalid block, + // then add back to m_blocks_unlinked, so that if the block arrives in the future + // we can try adding to setBlockIndexCandidates again. m_blockman.m_blocks_unlinked.insert( std::make_pair(pindexFailed->pprev, pindexFailed)); } From aa0eef735b1c2705b5ecb5adb667da25b59d46fa Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Thu, 19 Mar 2026 22:13:11 +0530 Subject: [PATCH 2/3] test: add InvalidateBlock/ReconsiderBlock asymmetry test InvalidateBlock propagates to all descendants in m_block_index, but ReconsiderBlock only clears blocks on the same chain as the reconsidered block (its direct ancestors and descendants). Blocks on sibling forks are not cleared. This asymmetry can lead to edge cases like in issue #32173 (fixed in 37bc207). Add a unit test for this scenario to safeguard against future regressions. --- .../validation_chainstatemanager_tests.cpp | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 2847116976f..4924f66e69d 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -625,6 +625,94 @@ BOOST_FIXTURE_TEST_CASE(loadblockindex_invalid_descendants, TestChain100Setup) BOOST_CHECK(child->nStatus & BLOCK_FAILED_VALID); } +//! Verify that ReconsiderBlock clears failure flags for the target block, its ancestors, and descendants, +//! but not for sibling forks that diverge from a shared ancestor. +BOOST_FIXTURE_TEST_CASE(invalidate_block_and_reconsider_fork, TestChain100Setup) +{ + ChainstateManager& chainman = *Assert(m_node.chainman); + Chainstate& chainstate = chainman.ActiveChainstate(); + + // we have a chain of 100 blocks: genesis(0) <- ... <- block98 <- block99 <- block100 + CBlockIndex* block98; + CBlockIndex* block99; + CBlockIndex* block100; + { + LOCK(chainman.GetMutex()); + block98 = chainman.ActiveChain()[98]; + block99 = chainman.ActiveChain()[99]; + block100 = chainman.ActiveChain()[100]; + } + + // create the following block constellation: + // genesis(0) <- ... <- block98 <- block99 <- block100 + // <- block99' <- block100' + // by temporarily invalidating block99. the chain tip now falls to block98, + // mine 2 new blocks on top of block 98 (block99' and block100') and then restore block99 and block 100. + BlockValidationState state; + BOOST_REQUIRE(chainstate.InvalidateBlock(state, block99)); + BOOST_REQUIRE(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()) == block98); + CScript coinbase_script = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG; + for (int i = 0; i < 2; ++i) { + CreateAndProcessBlock({}, coinbase_script); + } + const CBlockIndex* fork_block99; + const CBlockIndex* fork_block100; + { + LOCK(chainman.GetMutex()); + fork_block99 = chainman.ActiveChain()[99]; + BOOST_REQUIRE(fork_block99->pprev == block98); + fork_block100 = chainman.ActiveChain()[100]; + BOOST_REQUIRE(fork_block100->pprev == fork_block99); + } + // Restore original block99 and block100 + { + LOCK(chainman.GetMutex()); + chainstate.ResetBlockFailureFlags(block99); + chainman.RecalculateBestHeader(); + } + chainstate.ActivateBestChain(state); + BOOST_REQUIRE(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip()) == block100); + + { + LOCK(chainman.GetMutex()); + BOOST_CHECK(!(block100->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(!(block99->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(!(fork_block100->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(!(fork_block99->nStatus & BLOCK_FAILED_VALID)); + } + + // Invalidate block98 + BOOST_REQUIRE(chainstate.InvalidateBlock(state, block98)); + + { + LOCK(chainman.GetMutex()); + // block98 and all descendants of block98 are marked BLOCK_FAILED_VALID + BOOST_CHECK(block98->nStatus & BLOCK_FAILED_VALID); + BOOST_CHECK(block99->nStatus & BLOCK_FAILED_VALID); + BOOST_CHECK(block100->nStatus & BLOCK_FAILED_VALID); + BOOST_CHECK(fork_block99->nStatus & BLOCK_FAILED_VALID); + BOOST_CHECK(fork_block100->nStatus & BLOCK_FAILED_VALID); + } + + // Reconsider block99. ResetBlockFailureFlags clears BLOCK_FAILED_VALID from + // block99 and its ancestors (block98) and descendants (block100) + // but NOT from block99' and block100' (not a direct ancestor/descendant) + { + LOCK(chainman.GetMutex()); + chainstate.ResetBlockFailureFlags(block99); + chainman.RecalculateBestHeader(); + } + chainstate.ActivateBestChain(state); + { + LOCK(chainman.GetMutex()); + BOOST_CHECK(!(block98->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(!(block99->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(!(block100->nStatus & BLOCK_FAILED_VALID)); + BOOST_CHECK(fork_block99->nStatus & BLOCK_FAILED_VALID); + BOOST_CHECK(fork_block100->nStatus & BLOCK_FAILED_VALID); + } +} + //! Ensure that snapshot chainstate can be loaded when found on disk after a //! restart, and that new blocks can be connected to both chainstates. BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) From ba01b00d45a062d64f56aa201b5940327c9ada85 Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Tue, 7 Apr 2026 23:38:28 +0530 Subject: [PATCH 3/3] refactor: use for loops in FindMostWorkChain Co-authored-by: Ryan Ofsky --- src/validation.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e0a8c1b38db..ebad99f0922 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3123,9 +3123,8 @@ CBlockIndex* Chainstate::FindMostWorkChain() // Check whether all blocks on the path between the currently active chain and the candidate are valid. // Just going until the active chain is an optimization, as we know all blocks in it are valid already. - CBlockIndex *pindexTest = pindexNew; bool fInvalidAncestor = false; - while (pindexTest && !m_chain.Contains(pindexTest)) { + for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(pindexTest); pindexTest = pindexTest->pprev) { assert(pindexTest->HaveNumChainTxs() || pindexTest->nHeight == 0); // Pruned nodes may have entries in setBlockIndexCandidates for @@ -3139,9 +3138,8 @@ CBlockIndex* Chainstate::FindMostWorkChain() if (fFailedChain && (m_chainman.m_best_invalid == nullptr || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork)) { m_chainman.m_best_invalid = pindexNew; } - CBlockIndex *pindexFailed = pindexNew; // Remove the entire chain from the set. - while (pindexTest != pindexFailed) { + for (CBlockIndex *pindexFailed = pindexNew; pindexFailed != pindexTest; pindexFailed = pindexFailed->pprev) { if (fMissingData && !fFailedChain) { // If we're missing data and not a descendant of an invalid block, // then add back to m_blocks_unlinked, so that if the block arrives in the future @@ -3150,13 +3148,11 @@ CBlockIndex* Chainstate::FindMostWorkChain() std::make_pair(pindexFailed->pprev, pindexFailed)); } setBlockIndexCandidates.erase(pindexFailed); - pindexFailed = pindexFailed->pprev; } setBlockIndexCandidates.erase(pindexTest); fInvalidAncestor = true; break; } - pindexTest = pindexTest->pprev; } if (!fInvalidAncestor) return pindexNew;