Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)

src/chain.h

@@ -192,7 +192,9 @@ class CBlockIndex
     uint32_t nNonce{0};
 
     //! (memory only) Sequential id assigned to distinguish order in which blocks are received.
-    int32_t nSequenceId{0};
+    //! Initialized to 1 when loading blocks from disk, except for blocks belonging to the best chain
+    //! which overwrite it to 0.
+    int32_t nSequenceId{1};
 
     //! (memory only) Maximum nTime in the chain up to and including this block.
     unsigned int nTimeMax{0};

src/node/blockstorage.cpp

@@ -158,12 +158,12 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn
     if (pa->nChainWork > pb->nChainWork) return false;
     if (pa->nChainWork < pb->nChainWork) return true;
 
-    // ... then by earliest time received, ...
+    // ... then by earliest activatable time, ...
     if (pa->nSequenceId < pb->nSequenceId) return false;
     if (pa->nSequenceId > pb->nSequenceId) return true;
 
     // Use pointer address as tie breaker (should only happen with blocks
-    // loaded from disk, as those all have id 0).
+    // loaded from disk, as those all have id 1).
     if (pa < pb) return false;
     if (pa > pb) return true;
 
@@ -214,7 +214,7 @@ CBlockIndex* BlockManager::AddToBlockIndex(const CBlockHeader& block, CBlockInde
     // We assign the sequence id to blocks only when the full data is available,
     // to avoid miners withholding blocks but broadcasting headers, to get a
     // competitive advantage.
-    pindexNew->nSequenceId = 0;
+    pindexNew->nSequenceId = 1;
 
     pindexNew->phashBlock = &((*mi).first);
     BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock);

src/validation.cpp

@@ -4481,7 +4481,7 @@ bool Chainstate::LoadChainTip()
     AssertLockHeld(cs_main);
     const CCoinsViewCache& coins_cache = CoinsTip();
     assert(!coins_cache.GetBestBlock().IsNull()); // Never called when the coins view is empty
-    const CBlockIndex* tip = m_chain.Tip();
+    CBlockIndex* tip = m_chain.Tip();
 
     if (tip && tip->GetBlockHash() == coins_cache.GetBestBlock()) {
         return true;
@@ -4501,6 +4501,22 @@ bool Chainstate::LoadChainTip()
               m_chain.Height(),
               FormatISO8601DateTime(tip->GetBlockTime()),
               GuessVerificationProgress(m_chainman.GetParams().TxData(), tip));
+
+    // Make sure our chain tip before shutting down scores better than any other candidate
+    // to maintain a consistent best tip over reboots
+    auto target = tip;
+    while (target) {
+        target->nSequenceId = 0;
+        target = target->pprev;
+    }
+
+    // Block index candidates are loaded before the chain tip, so we need to replae this entry
+    // Otherwise the scoring will be based on the memory address location instrad of the nSequenceId
+    setBlockIndexCandidates.erase(tip);
+    TryAddBlockIndexCandidate(tip);
+    // We also need to re-prune blocks that now score potentially worse than before
+    PruneBlockIndexCandidates();
+
     return true;
 }
 
@@ -5130,7 +5146,7 @@ void ChainstateManager::CheckBlockIndex()
                 }
             }
         }
-        if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock)
+        if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= 1); // nSequenceId can't be set higher than 1 for blocks that aren't linked (negative is used for preciousblock, 0 for active chain)
         // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred).
         // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred.
         if (!m_blockman.m_have_pruned) {

src/validation.h

@@ -978,8 +978,9 @@ class ChainstateManager
      * Every received block is assigned a unique and increasing identifier, so we
      * know which one to give priority in case of a fork.
      */
-    /** Blocks loaded from disk are assigned id 0, so start the counter at 1. */
-    int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 1;
+    /** Blocks loaded from disk are assigned id 1 (0 if they belong to the best
+     * chain loaded from disk), so start the counter at 2. **/
+    int32_t nBlockSequenceId GUARDED_BY(::cs_main) = 2;
     /** Decreasing counter (used by subsequent preciousblock calls). */
     int32_t nBlockReverseSequenceId = -1;
     /** chainwork for the last block that preciousblock has been applied to. */
@@ -990,7 +991,7 @@ class ChainstateManager
     void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);
-        nBlockSequenceId = 1;
+        nBlockSequenceId = 2;
         nBlockReverseSequenceId = -1;
     }
 

test/functional/feature_chain_tiebreaks.py

@@ -0,0 +1,151 @@
+#!/usr/bin/env python3
+# Copyright (c) The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test that the correct active block is chosen in complex reorgs."""
+
+from test_framework.blocktools import create_block
+from test_framework.messages import CBlockHeader
+from test_framework.p2p import P2PDataStore
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+class ChainTiebreaksTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = True
+
+    @staticmethod
+    def send_headers(node, blocks):
+        """Submit headers for blocks to node."""
+        for block in blocks:
+            # Use RPC rather than P2P, to prevent the message from being interpreted as a block
+            # announcement.
+            node.submitheader(hexdata=CBlockHeader(block).serialize().hex())
+
+    def test_chain_split_in_memory(self):
+        node = self.nodes[0]
+        # Add P2P connection to bitcoind
+        peer = node.add_p2p_connection(P2PDataStore())
+
+        self.log.info('Precomputing blocks')
+        #
+        #          /- B3 -- B7
+        #        B1      \- B8
+        #       /  \
+        #      /    \ B4 -- B9
+        #   B0           \- B10
+        #      \
+        #       \  /- B5
+        #        B2
+        #          \- B6
+        #
+        blocks = []
+
+        # Construct B0, building off genesis.
+        start_height = node.getblockcount()
+        blocks.append(create_block(
+            hashprev=int(node.getbestblockhash(), 16),
+            tmpl={"height": start_height + 1}
+        ))
+        blocks[-1].solve()
+
+        # Construct B1-B10.
+        for i in range(1, 11):
+            blocks.append(create_block(
+                hashprev=int(blocks[(i - 1) >> 1].hash, 16),
+                tmpl={
+                    "height": start_height + (i + 1).bit_length(),
+                    # Make sure each block has a different hash.
+                    "curtime": blocks[-1].nTime + 1,
+                }
+            ))
+            blocks[-1].solve()
+
+        self.log.info('Make sure B0 is accepted normally')
+        peer.send_blocks_and_test([blocks[0]], node, success=True)
+        # B0 must be active chain now.
+        assert_equal(node.getbestblockhash(), blocks[0].hash)
+
+        self.log.info('Send B1 and B2 headers, and then blocks in opposite order')
+        self.send_headers(node, blocks[1:3])
+        peer.send_blocks_and_test([blocks[2]], node, success=True)
+        peer.send_blocks_and_test([blocks[1]], node, success=False)
+        # B2 must be active chain now, as full data for B2 was received first.
+        assert_equal(node.getbestblockhash(), blocks[2].hash)
+
+        self.log.info('Send all further headers in order')
+        self.send_headers(node, blocks[3:])
+        # B2 is still the active chain, headers don't change this.
+        assert_equal(node.getbestblockhash(), blocks[2].hash)
+
+        self.log.info('Send blocks B7-B10')
+        peer.send_blocks_and_test([blocks[7]], node, success=False)
+        peer.send_blocks_and_test([blocks[8]], node, success=False)
+        peer.send_blocks_and_test([blocks[9]], node, success=False)
+        peer.send_blocks_and_test([blocks[10]], node, success=False)
+        # B2 is still the active chain, as B7-B10 have missing parents.
+        assert_equal(node.getbestblockhash(), blocks[2].hash)
+
+        self.log.info('Send parents B3-B4 of B8-B10 in reverse order')
+        peer.send_blocks_and_test([blocks[4]], node, success=False, force_send=True)
+        peer.send_blocks_and_test([blocks[3]], node, success=False, force_send=True)
+        # B9 is now active. Despite B7 being received earlier, the missing parent.
+        assert_equal(node.getbestblockhash(), blocks[9].hash)
+
+        self.log.info('Invalidate B9-B10')
+        node.invalidateblock(blocks[9].hash)
+        node.invalidateblock(blocks[10].hash)
+        # B7 is now active.
+        assert_equal(node.getbestblockhash(), blocks[7].hash)
+
+        # Invalidate blocks to start fresh on the next test
+        node.invalidateblock(blocks[0].hash)
+
+    def test_chain_split_from_disk(self):
+        node = self.nodes[0]
+        peer = node.add_p2p_connection(P2PDataStore())
+
+        self.log.info('Precomputing blocks')
+        #
+        #      A1
+        #     /
+        #   G
+        #     \
+        #      A2
+        #
+        blocks = []
+
+        # Construct two blocks building from genesis.
+        start_height = node.getblockcount()
+        genesis_block = node.getblock(node.getblockhash(start_height))
+        prev_time = genesis_block["time"]
+
+        for i in range(0, 2):
+            blocks.append(create_block(
+                hashprev=int(genesis_block["hash"], 16),
+                tmpl={"height": start_height + 1,
+                # Make sure each block has a different hash.
+                "curtime": prev_time + i + 1,
+                }
+            ))
+            blocks[-1].solve()
+
+        # Send blocks and test the last one is not connected
+        self.log.info('Send A1 and A2. Make sure than only the former connects')
+        peer.send_blocks_and_test([blocks[0]], node, success=True)
+        peer.send_blocks_and_test([blocks[1]], node, success=False)
+
+        self.log.info('Restart the node and check that the best tip before restarting matched the ones afterwards')
+        # Restart and check enough times to this to eventually fail if the logic is broken
+        for _ in range(10):
+            self.restart_node(0)
+            assert_equal(blocks[0].hash, node.getbestblockhash())
+
+    def run_test(self):
+        self.test_chain_split_in_memory()
+        self.test_chain_split_from_disk()
+
+
+if __name__ == '__main__':
+    ChainTiebreaksTest().main()

test/functional/test_runner.py

@@ -361,6 +361,7 @@
     'feature_includeconf.py',
     'feature_addrman.py',
     'feature_asmap.py',
+    'feature_chain_tiebreaks.py',
     'feature_fastprune.py',
     'mempool_unbroadcast.py',
     'mempool_compatibility.py',