Merge bitcoin/bitcoin#30479: validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates

8cc3ac6 validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b9 test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55 validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)

Pull request description:

  When we call `reconsiderblock` for some block,  `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.

  I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
  ``` diff
  diff --git a/src/validation.cpp b/src/validation.cpp
  index 7b04bd9a5b..ff0c3c9f58 100644
  --- a/src/validation.cpp
  +++ b/src/validation.cpp
  @@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
               }
           }
           // When we reach this point, we switched to a new tip (stored in pindexNewTip).
  +        m_chainman.CheckBlockIndex();
   
           if (exited_ibd) {
               // If a background chainstate is in use, we may need to rebalance our
  ```
  makes `rpc_invalidateblock.py` fail on master.

  Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.

  Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).

  Fixes #16444

ACKs for top commit:
  achow101:
    ACK 8cc3ac6
  TheCharlatan:
    Re-ACK 8cc3ac6
  stratospher:
    reACK 8cc3ac6.

Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7

Author: achow101

src/chain.h

@@ -292,7 +292,7 @@ class CBlockIndex
     std::string ToString() const;
 
     //! Check whether this block index entry is valid up to the passed validity level.
-    bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const
+    bool IsValid(enum BlockStatus nUpTo) const
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);

src/test/fuzz/chain.cpp

@@ -30,7 +30,7 @@ FUZZ_TARGET(chain)
         (void)disk_block_index->GetMedianTimePast();
         (void)disk_block_index->GetUndoPos();
         (void)disk_block_index->HaveNumChainTxs();
-        (void)disk_block_index->IsValid();
+        (void)disk_block_index->IsValid(BLOCK_VALID_TRANSACTIONS);
     }
 
     const CBlockHeader block_header = disk_block_index->GetBlockHeader();

src/validation.cpp

@@ -3851,9 +3851,9 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
 
     int nHeight = pindex->nHeight;
 
-    // Remove the invalidity flag from this block and all its descendants.
+    // 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.IsValid() && block_index.GetAncestor(nHeight) == pindex) {
+        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;
             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)) {
@@ -3865,15 +3865,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
             }
         }
     }
-
-    // Remove the invalidity flag from all ancestors too.
-    while (pindex != nullptr) {
-        if (pindex->nStatus & BLOCK_FAILED_MASK) {
-            pindex->nStatus &= ~BLOCK_FAILED_MASK;
-            m_blockman.m_dirty_blockindex.insert(pindex);
-        }
-        pindex = pindex->pprev;
-    }
 }
 
 void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex)

src/validation.h

@@ -745,7 +745,7 @@ class Chainstate
     /** Set invalidity status to all descendants of a block */
     void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
-    /** Remove invalidity status from a block and its descendants. */
+    /** Remove invalidity status from a block, its descendants and ancestors and reconsider them for activation */
     void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /** Replay blocks that aren't fully applied to the database. */

test/functional/rpc_invalidateblock.py

@@ -85,6 +85,23 @@ def run_test(self):
         self.wait_until(lambda: self.nodes[0].getblockcount() == 4, timeout=5)
         self.wait_until(lambda: self.nodes[1].getblockcount() == 4, timeout=5)
 
+        self.log.info("Verify that ancestors can become chain tip candidates when we reconsider blocks")
+        # Invalidate node0's current chain (1' -> 2' -> 3' -> 4') so that we don't reorg back to it in this test
+        badhash = self.nodes[0].getblockhash(1)
+        self.nodes[0].invalidateblock(badhash)
+        # Reconsider the tip so that node0's chain becomes this chain again : 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> header 7
+        self.nodes[0].reconsiderblock(tip)
+        blockhash_3 = self.nodes[0].getblockhash(3)
+        blockhash_4 = self.nodes[0].getblockhash(4)
+        blockhash_6 = self.nodes[0].getblockhash(6)
+        assert_equal(self.nodes[0].getbestblockhash(), blockhash_6)
+
+        # Invalidate block 4 so that chain becomes : 1 -> 2 -> 3
+        self.nodes[0].invalidateblock(blockhash_4)
+        assert_equal(self.nodes[0].getbestblockhash(), blockhash_3)
+        assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 3)
+        assert_equal(self.nodes[0].getblockchaininfo()['headers'], 3)
+
         self.log.info("Verify that we reconsider all ancestors as well")
         blocks = self.generatetodescriptor(self.nodes[1], 10, ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR, sync_fun=self.no_op)
         assert_equal(self.nodes[1].getbestblockhash(), blocks[-1])
@@ -111,6 +128,14 @@ def run_test(self):
         # Should report consistent blockchain info
         assert_equal(self.nodes[1].getblockchaininfo()["headers"], self.nodes[1].getblockchaininfo()["blocks"])
 
+        # Reconsider the header
+        self.nodes[0].reconsiderblock(block.hash)
+        # Since header doesn't have block data, it can't be chain tip
+        # Check if it's possible for an ancestor (with block data) to be the chain tip
+        assert_equal(self.nodes[0].getbestblockhash(), blockhash_6)
+        assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 6)
+        assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7)
+
         self.log.info("Verify that invalidating an unknown block throws an error")
         assert_raises_rpc_error(-5, "Block not found", self.nodes[1].invalidateblock, "00" * 32)
         assert_equal(self.nodes[1].getbestblockhash(), blocks[-1])