diff --git a/src/chain.h b/src/chain.h index 13f7582385d..0e86239b098 100644 --- a/src/chain.h +++ b/src/chain.h @@ -35,6 +35,9 @@ static constexpr int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60; * MAX_FUTURE_BLOCK_TIME. */ static constexpr int64_t TIMESTAMP_WINDOW = MAX_FUTURE_BLOCK_TIME; + //! Init values for CBlockIndex nSequenceId when loaded from disk +static constexpr int32_t SEQ_ID_BEST_CHAIN_FROM_DISK = 0; +static constexpr int32_t SEQ_ID_INIT_FROM_DISK = 1; /** * Maximum gap between node time and block time used @@ -191,7 +194,9 @@ public: uint32_t nNonce{0}; //! (memory only) Sequential id assigned to distinguish order in which blocks are received. - int32_t nSequenceId{0}; + //! Initialized to SEQ_ID_INIT_FROM_DISK{1} when loading blocks from disk, except for blocks + //! belonging to the best chain which overwrite it to SEQ_ID_BEST_CHAIN_FROM_DISK{0}. + int32_t nSequenceId{SEQ_ID_INIT_FROM_DISK}; //! (memory only) Maximum nTime in the chain up to and including this block. unsigned int nTimeMax{0}; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 372395dd24d..2a22a62235c 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -159,12 +159,13 @@ 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 share the same id: 0 for blocks on the + // best chain, 1 for all others). if (pa < pb) return false; if (pa > pb) return true; @@ -215,7 +216,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 = SEQ_ID_INIT_FROM_DISK; pindexNew->phashBlock = &((*mi).first); BlockMap::iterator miPrev = m_block_index.find(block.hashPrevBlock); diff --git a/src/validation.cpp b/src/validation.cpp index a1ac4e1e145..801c6636291 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4706,7 +4706,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; @@ -4718,9 +4718,22 @@ bool Chainstate::LoadChainTip() return false; } m_chain.SetTip(*pindex); + tip = m_chain.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 = SEQ_ID_BEST_CHAIN_FROM_DISK; + target = target->pprev; + } + + // Block index candidates are loaded before the chain tip, so we need to replace this entry + // Otherwise the scoring will be based on the memory address location instead of the nSequenceId + setBlockIndexCandidates.erase(tip); + TryAddBlockIndexCandidate(tip); PruneBlockIndexCandidates(); - tip = m_chain.Tip(); LogPrintf("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f\n", tip->GetBlockHash().ToString(), m_chain.Height(), @@ -5365,7 +5378,9 @@ 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) + // nSequenceId can't be set higher than SEQ_ID_INIT_FROM_DISK{1} for blocks that aren't linked + // (negative is used for preciousblock, SEQ_ID_BEST_CHAIN_FROM_DISK{0} for active chain when loaded from disk) + if (!pindex->HaveNumChainTxs()) assert(pindex->nSequenceId <= SEQ_ID_INIT_FROM_DISK); // 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) { diff --git a/src/validation.h b/src/validation.h index f6cbee28fc5..17273ee00b3 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1021,8 +1021,10 @@ public: * 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 SEQ_ID_INIT_FROM_DISK{1} + * (SEQ_ID_BEST_CHAIN_FROM_DISK{0} if they belong to the best chain loaded from disk), + * so start the counter after that. **/ + int32_t nBlockSequenceId GUARDED_BY(::cs_main) = SEQ_ID_INIT_FROM_DISK + 1; /** Decreasing counter (used by subsequent preciousblock calls). */ int32_t nBlockReverseSequenceId = -1; /** chainwork for the last block that preciousblock has been applied to. */ @@ -1033,7 +1035,7 @@ public: void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 1; + nBlockSequenceId = SEQ_ID_INIT_FROM_DISK + 1; nBlockReverseSequenceId = -1; } diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py new file mode 100755 index 00000000000..f75f7983f77 --- /dev/null +++ b/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(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 1fa22b1cc61..8be8341ecbe 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -373,6 +373,7 @@ BASE_SCRIPTS = [ 'feature_includeconf.py', 'feature_addrman.py', 'feature_asmap.py', + 'feature_chain_tiebreaks.py', 'feature_fastprune.py', 'feature_framework_miniwallet.py', 'mempool_unbroadcast.py',