Merge bitcoin/bitcoin#34884: validation: remove unused code in FindMostWorkChain

ba01b00 refactor: use for loops in FindMostWorkChain (stratospher)
aa0eef7 test: add InvalidateBlock/ReconsiderBlock asymmetry test (stratospher)
1b0b3e2 validation: remove redundant marking in FindMostWorkChain (stratospher)

Pull request description:

  recent PRs like #31405, #30666 mark all `m_block_index` descendants as invalid immediately whenever an invalid block is encountered in `SetBlockFailureFlags`. so by the time we reach `FindMostWorkChain`, the block in `setBlockIndexCandidates` already has `BLOCK_FAILED_VALID` set on it - not just on its ancestor. this means `pindexTest = pindexFailed` whenever `fFailedChain` fires, and the inner `while (pindexTest != pindexFailed)` loop body is never reached!

  I think we can remove it but I've just replaced it with `Assume` in this PR for safety + good to document this invariant in case the code changes in future. (noticed by @ stickies-v in bitcoin/bitcoin#32950 (comment))

  the second commit is unrelated and adds a unit test for the situation in bitcoin/bitcoin#32173

ACKs for top commit:
  fjahr:
    re-ACK ba01b00
  optout21:
    crACK ba01b00
  w0xlt:
    ACK ba01b00
  ryanofsky:
    Code review ACK ba01b00, just tweaking comment and for loop condition since last review.

Tree-SHA512: a8be3c30b1c41b76690d16d850e87e9e71fa6a1ecaa8b90ec997ffee1aace48b336a7009a480cd016103759d79c964b3d761a13ae936523808b2930beb68dae5

Author: ryanofsky

src/test/validation_chainstatemanager_tests.cpp

@@ -618,6 +618,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)

src/validation.cpp

@@ -3127,9 +3127,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
@@ -3143,27 +3142,21 @@ 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) {
-                    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.
+                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
+                        // we can try adding to setBlockIndexCandidates again.
                         m_blockman.m_blocks_unlinked.insert(
                             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;