Repository: bitcoin/bitcoin (language: cpp)

Issue:
Header inconsistency after invalidate/reconsider block
Demonstration using regtest.
```bash
# getblockchaininfo
  "chain": "regtest",
  "blocks": 0,
  "headers": 0,
```

Mine a block
```bash
# getblockchaininfo
  "chain": "regtest",
  "blocks": 1,
  "headers": 1,
```
Call invalidateblock
```bash
# getblockchaininfo
  "chain": "regtest",
  "blocks": 0,
  "headers": 0,
# getchaintips
    "height": 1,
    "hash": "06702ef73c7d8629294f8b6386646f888d60979932ba2ee45e3495d8f25006b5",
    "branchlen": 1,
    "status": "invalid"
  },
  {
    "height": 0,
    "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
    "branchlen": 0,
    "status": "active"
```
Call reconsiderblock
```bash
# getchaintips
    "height": 1,
    "hash": "06702ef73c7d8629294f8b6386646f888d60979932ba2ee45e3495d8f25006b5",
    "branchlen": 0,
    "status": "active"
```

Header count returned by getblockchaininfo is now < # of blocks.
```bash
# getblockchaininfo
  "chain": "regtest",
  "blocks": 1,
  "headers": 0,
```


Discussion / hints:
Is this a regression?
> Is this a regression?

Can recreate the same behaviour with builds of 23.x and 22.x.

Fix patch:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 7ffd03e71af5a..4a7af39b15144 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1648,6 +1648,7 @@ void ReconsiderBlock(ChainstateManager& chainman, uint256 block_hash) {
         }
 
         chainman.ActiveChainstate().ResetBlockFailureFlags(pblockindex);
+        chainman.RecalculateBestHeader();
     }
 
     BlockValidationState state;
diff --git a/src/validation.cpp b/src/validation.cpp
index 8e4ea8eda2f62..dcaa8dba26602 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2045,8 +2045,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__,
@@ -3785,6 +3786,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);
 
@@ -6396,6 +6408,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);
diff --git a/src/validation.h b/src/validation.h
index 059ae52bdf4ca..29ff6bd94cd4e 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -735,6 +735,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);
 
@@ -1321,6 +1324,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 patch:
diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py
index 1bd4413ce1ec2..226e995307cef 100755
--- a/test/functional/rpc_getchaintips.py
+++ b/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()
diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py
index db79d55259007..e2eb033f09c75 100755
--- a/test/functional/rpc_invalidateblock.py
+++ b/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)