Merge bitcoin/bitcoin#30666: validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment

0bd53d9 test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149dd validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb73 validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e96 rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e917 validation: add RecalculateBestHeader() function (Martin Zumsande)

Pull request description:

  `m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.

  We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.

  This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
  It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.

  One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
  While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
  Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.

  Fixes  #26245

ACKs for top commit:
  fjahr:
    reACK 0bd53d9
  achow101:
    ACK 0bd53d9
  TheCharlatan:
    Re-ACK 0bd53d9

Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445

Author: achow101

src/rpc/blockchain.cpp

@@ -1647,6 +1647,7 @@ void ReconsiderBlock(ChainstateManager& chainman, uint256 block_hash) {
         }
 
         chainman.ActiveChainstate().ResetBlockFailureFlags(pblockindex);
+        chainman.RecalculateBestHeader();
     }
 
     BlockValidationState state;

src/validation.cpp

@@ -2056,8 +2056,9 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew)
     if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) {
         m_chainman.m_best_invalid = pindexNew;
     }
+    SetBlockFailureFlags(pindexNew);
     if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) {
-        m_chainman.m_best_header = m_chain.Tip();
+        m_chainman.RecalculateBestHeader();
     }
 
     LogPrintf("%s: invalid block=%s  height=%d  log2_work=%f  date=%s\n", __func__,
@@ -3793,6 +3794,17 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
     return true;
 }
 
+void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block)
+{
+    AssertLockHeld(cs_main);
+
+    for (auto& [_, block_index] : m_blockman.m_block_index) {
+        if (block_index.GetAncestor(invalid_block->nHeight) == invalid_block && !(block_index.nStatus & BLOCK_FAILED_MASK)) {
+            block_index.nStatus |= BLOCK_FAILED_CHILD;
+        }
+    }
+}
+
 void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) {
     AssertLockHeld(cs_main);
 
@@ -6404,6 +6416,17 @@ std::optional<int> ChainstateManager::GetSnapshotBaseHeight() const
     return base ? std::make_optional(base->nHeight) : std::nullopt;
 }
 
+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) {
+            m_best_header = &entry.second;
+        }
+    }
+}
+
 bool ChainstateManager::ValidatedSnapshotCleanup()
 {
     AssertLockHeld(::cs_main);

src/validation.h

@@ -729,6 +729,9 @@ class Chainstate
         EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
         LOCKS_EXCLUDED(::cs_main);
 
+    /** 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. */
     void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -1314,6 +1317,11 @@ class ChainstateManager
     //! nullopt.
     std::optional<int> GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    //! If, due to invalidation / reconsideration of blocks, the previous
+    //! best header is no longer valid / guaranteed to be the most-work
+    //! header in our block-index not known to be invalid, recalculate it.
+    void RecalculateBestHeader() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     CCheckQueue<CScriptCheck>& GetCheckQueue() { return m_script_check_queue; }
 
     ~ChainstateManager();

test/functional/rpc_getchaintips.py

@@ -10,6 +10,10 @@
 - verify that getchaintips now returns two chain tips.
 """
 
+from test_framework.blocktools import (
+    create_block,
+    create_coinbase,
+)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
@@ -18,44 +22,73 @@ def set_test_params(self):
         self.num_nodes = 4
 
     def run_test(self):
+        self.log.info("Test getchaintips behavior with two chains of different length")
         tips = self.nodes[0].getchaintips()
         assert_equal(len(tips), 1)
         assert_equal(tips[0]['branchlen'], 0)
         assert_equal(tips[0]['height'], 200)
         assert_equal(tips[0]['status'], 'active')
 
-        # Split the network and build two chains of different lengths.
+        self.log.info("Split the network and build two chains of different lengths.")
         self.split_network()
         self.generate(self.nodes[0], 10, sync_fun=lambda: self.sync_all(self.nodes[:2]))
         self.generate(self.nodes[2], 20, sync_fun=lambda: self.sync_all(self.nodes[2:]))
 
-        tips = self.nodes[1].getchaintips ()
-        assert_equal (len (tips), 1)
+        tips = self.nodes[1].getchaintips()
+        assert_equal(len(tips), 1)
         shortTip = tips[0]
-        assert_equal (shortTip['branchlen'], 0)
-        assert_equal (shortTip['height'], 210)
-        assert_equal (tips[0]['status'], 'active')
+        assert_equal(shortTip['branchlen'], 0)
+        assert_equal(shortTip['height'], 210)
+        assert_equal(tips[0]['status'], 'active')
 
-        tips = self.nodes[3].getchaintips ()
-        assert_equal (len (tips), 1)
+        tips = self.nodes[3].getchaintips()
+        assert_equal(len(tips), 1)
         longTip = tips[0]
-        assert_equal (longTip['branchlen'], 0)
-        assert_equal (longTip['height'], 220)
-        assert_equal (tips[0]['status'], 'active')
+        assert_equal(longTip['branchlen'], 0)
+        assert_equal(longTip['height'], 220)
+        assert_equal(tips[0]['status'], 'active')
 
-        # Join the network halves and check that we now have two tips
+        self.log.info("Join the network halves and check that we now have two tips")
         # (at least at the nodes that previously had the short chain).
-        self.join_network ()
+        self.join_network()
 
-        tips = self.nodes[0].getchaintips ()
-        assert_equal (len (tips), 2)
-        assert_equal (tips[0], longTip)
+        tips = self.nodes[0].getchaintips()
+        assert_equal(len(tips), 2)
+        assert_equal(tips[0], longTip)
 
-        assert_equal (tips[1]['branchlen'], 10)
-        assert_equal (tips[1]['status'], 'valid-fork')
+        assert_equal(tips[1]['branchlen'], 10)
+        assert_equal(tips[1]['status'], 'valid-fork')
         tips[1]['branchlen'] = 0
         tips[1]['status'] = 'active'
-        assert_equal (tips[1], shortTip)
+        assert_equal(tips[1], shortTip)
+
+        self.log.info("Test getchaintips behavior with invalid blocks")
+        self.disconnect_nodes(0, 1)
+        n0 = self.nodes[0]
+        tip = int(n0.getbestblockhash(), 16)
+        start_height = self.nodes[0].getblockcount()
+        # Create invalid block (too high coinbase)
+        block_time = n0.getblock(n0.getbestblockhash())['time'] + 1
+        invalid_block = create_block(tip, create_coinbase(start_height+1, nValue=100), block_time)
+        invalid_block.solve()
+
+        block_time += 1
+        block2 = create_block(invalid_block.sha256, create_coinbase(2), block_time, version=4)
+        block2.solve()
+
+        self.log.info("Submit headers-only chain")
+        n0.submitheader(invalid_block.serialize().hex())
+        n0.submitheader(block2.serialize().hex())
+        tips = n0.getchaintips()
+        assert_equal(len(tips), 3)
+        assert_equal(tips[0]['status'], 'headers-only')
+
+        self.log.info("Submit invalid block that invalidates the headers-only chain")
+        n0.submitblock(invalid_block.serialize().hex())
+        tips = n0.getchaintips()
+        assert_equal(len(tips), 3)
+        assert_equal(tips[0]['status'], 'invalid')
+
 
 if __name__ == '__main__':
     GetChainTipsTest(__file__).main()

test/functional/rpc_invalidateblock.py

@@ -6,6 +6,10 @@
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR
+from test_framework.blocktools import (
+    create_block,
+    create_coinbase,
+)
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
@@ -35,12 +39,33 @@ def run_test(self):
         self.connect_nodes(0, 1)
         self.sync_blocks(self.nodes[0:2])
         assert_equal(self.nodes[0].getblockcount(), 6)
-        badhash = self.nodes[1].getblockhash(2)
+
+        # Add a header to the tip of node 0 without submitting the block. This shouldn't
+        # affect results since this chain will be invalidated next.
+        tip = self.nodes[0].getbestblockhash()
+        block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time'] + 1
+        block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
+        block.solve()
+        self.nodes[0].submitheader(block.serialize().hex())
+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1)
 
         self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain")
+        badhash = self.nodes[1].getblockhash(2)
+        self.nodes[0].invalidateblock(badhash)
+        assert_equal(self.nodes[0].getblockcount(), 4)
+        assert_equal(self.nodes[0].getbestblockhash(), besthash_n0)
+        # Should report consistent blockchain info
+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"])
+
+        self.log.info("Reconsider block 6 on node 0 again and verify that the best header is set correctly")
+        self.nodes[0].reconsiderblock(tip)
+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1)
+
+        self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain again")
         self.nodes[0].invalidateblock(badhash)
         assert_equal(self.nodes[0].getblockcount(), 4)
         assert_equal(self.nodes[0].getbestblockhash(), besthash_n0)
+        assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"])
 
         self.log.info("Make sure we won't reorg to a lower work chain:")
         self.connect_nodes(1, 2)
@@ -83,6 +108,8 @@ def run_test(self):
         self.nodes[1].reconsiderblock(blocks[-4])
         # Should be back at the tip by now
         assert_equal(self.nodes[1].getbestblockhash(), blocks[-1])
+        # Should report consistent blockchain info
+        assert_equal(self.nodes[1].getblockchaininfo()["headers"], self.nodes[1].getblockchaininfo()["blocks"])
 
         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)