From a06e5a84f125a7f4624cb043efae953dc0a437d8 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 12 Mar 2024 16:04:20 -0400 Subject: [PATCH 1/5] Updates CBlockIndexWorkComparator outdated comment --- src/node/blockstorage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 6f1a96c8279..20540f0c75d 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -158,7 +158,7 @@ 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; From 05fb4f8aeed14abff123509468e5f357818d6b07 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 19 Jan 2024 16:02:57 -0500 Subject: [PATCH 2/5] test: add functional test for complex reorgs --- test/functional/feature_chain_tiebreaks.py | 103 +++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 104 insertions(+) create mode 100755 test/functional/feature_chain_tiebreaks.py diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py new file mode 100755 index 00000000000..a18d9a8cdb9 --- /dev/null +++ b/test/functional/feature_chain_tiebreaks.py @@ -0,0 +1,103 @@ +#!/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 run_test(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) + +if __name__ == '__main__': + ChainTiebreaksTest(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b51e40483e6..a454e0917a1 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -371,6 +371,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', From c8f5e6234ff95bbd960065a344be6b6d5e921973 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 14 Mar 2024 13:48:43 -0400 Subject: [PATCH 3/5] Set the same best tip on restart if two candidates have the same work Before this, if we had two (or more) same work tip candidates and restarted our node, it could be the case that the block set as tip after bootstrap didn't match the one before stopping. That's because the work and `nSequenceId` of both block will be the same (the latter is only kept in memory), so the active chain after restart would have depended on what tip candidate was loaded first. This makes sure that we are consistent over reboots. --- src/chain.h | 4 +++- src/node/blockstorage.cpp | 5 +++-- src/validation.cpp | 19 ++++++++++++++++--- src/validation.h | 7 ++++--- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/chain.h b/src/chain.h index 13f7582385d..1a8925eeecf 100644 --- a/src/chain.h +++ b/src/chain.h @@ -191,7 +191,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 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}; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 20540f0c75d..67490764d93 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -163,7 +163,8 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn 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; @@ -214,7 +215,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); diff --git a/src/validation.cpp b/src/validation.cpp index 64588e802d7..f6977e1a983 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 = 0; + 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(), @@ -5366,7 +5379,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) { diff --git a/src/validation.h b/src/validation.h index 9e4fdbe6809..7dd69c6fa7c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1021,8 +1021,9 @@ 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 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. */ @@ -1033,7 +1034,7 @@ public: void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 1; + nBlockSequenceId = 2; nBlockReverseSequenceId = -1; } From c7f9061d55412e4cd9c99768094ce9483f3c9ec6 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Tue, 28 Jan 2025 12:12:36 -0500 Subject: [PATCH 4/5] Make nSequenceId init value constants Make it easier to follow what the values come without having to go over the comments, plus easier to maintain --- src/chain.h | 9 ++++++--- src/node/blockstorage.cpp | 2 +- src/validation.cpp | 6 ++++-- src/validation.h | 9 +++++---- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/chain.h b/src/chain.h index 1a8925eeecf..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,9 +194,9 @@ public: uint32_t nNonce{0}; //! (memory only) Sequential id assigned to distinguish order in which blocks are received. - //! 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}; + //! 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 67490764d93..f9cdb37f05a 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -215,7 +215,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 = 1; + 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 f6977e1a983..17bedd16afe 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4724,7 +4724,7 @@ bool Chainstate::LoadChainTip() // to maintain a consistent best tip over reboots auto target = tip; while (target) { - target->nSequenceId = 0; + target->nSequenceId = SEQ_ID_BEST_CHAIN_FROM_DISK; target = target->pprev; } @@ -5379,7 +5379,9 @@ void ChainstateManager::CheckBlockIndex() } } } - 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) + // 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 7dd69c6fa7c..121ddcfb266 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1021,9 +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 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; + /** 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. */ @@ -1034,7 +1035,7 @@ public: void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 2; + nBlockSequenceId = SEQ_ID_INIT_FROM_DISK + 1; nBlockReverseSequenceId = -1; } From 177d07f65915ffce0c52818934e3ceddf574fd84 Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 14 Mar 2024 13:53:09 -0400 Subject: [PATCH 5/5] test: Adds block tiebreak over restarts tests Adds tests to make sure we are consistent on activating the same chain over a node restart if two or more candidates have the same work when the node is shutdown --- test/functional/feature_chain_tiebreaks.py | 50 +++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py index a18d9a8cdb9..f75f7983f77 100755 --- a/test/functional/feature_chain_tiebreaks.py +++ b/test/functional/feature_chain_tiebreaks.py @@ -23,7 +23,7 @@ class ChainTiebreaksTest(BitcoinTestFramework): # announcement. node.submitheader(hexdata=CBlockHeader(block).serialize().hex()) - def run_test(self): + def test_chain_split_in_memory(self): node = self.nodes[0] # Add P2P connection to bitcoind peer = node.add_p2p_connection(P2PDataStore()) @@ -99,5 +99,53 @@ class ChainTiebreaksTest(BitcoinTestFramework): # 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()