From 8b91883a23aac64a37d929eeae81325e221d177d Mon Sep 17 00:00:00 2001 From: Sergi Delgado Segura Date: Thu, 14 Mar 2024 13:48:43 -0400 Subject: [PATCH] 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 | 18 ++++++++++++++++-- src/validation.h | 7 ++++--- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/chain.h b/src/chain.h index f5bfdb2fb4b..d348fefe2a2 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 81b55c2440f..ac8839854b8 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -166,7 +166,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; @@ -217,7 +218,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 b62691ca4c0..f9a50d84e6b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4680,7 +4680,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; @@ -4692,6 +4692,20 @@ 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 in case of a tie. + 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(); @@ -5343,7 +5357,7 @@ void ChainstateManager::CheckBlockIndex() const } } } - 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 c25dd2de2d9..de3711b5c8c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1034,8 +1034,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. */ @@ -1046,7 +1047,7 @@ public: void ResetBlockSequenceCounters() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); - nBlockSequenceId = 1; + nBlockSequenceId = 2; nBlockReverseSequenceId = -1; }