From 768690b7ce551cd403f8e2a099372915f6022ad4 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 24 May 2023 13:56:37 -0400 Subject: [PATCH] Fix initialization of setBlockIndexCandidates when working with multiple chainstates When using assumeutxo and multiple chainstates are active, the background chainstate should consider all HAVE_DATA blocks that are ancestors of the snapshotted block and that have more work than the tip as potential candidates. --- .../validation_chainstatemanager_tests.cpp | 37 +++++++----- src/validation.cpp | 57 +++---------------- 2 files changed, 30 insertions(+), 64 deletions(-) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index e7cd1043a9e..160a807f69e 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -412,7 +412,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, SnapshotTestSetup) //! - Then mark a region of the chain BLOCK_ASSUMED_VALID and introduce a second chainstate //! that will tolerate assumed-valid blocks. Run LoadBlockIndex() and ensure that the first //! chainstate only contains fully validated blocks and the other chainstate contains all blocks, -//! even those assumed-valid. +//! except those marked assume-valid, because those entries don't HAVE_DATA. //! BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) { @@ -444,16 +444,17 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) WITH_LOCK(::cs_main, chainman.LoadBlockIndex()); }; - // Ensure that without any assumed-valid BlockIndex entries, all entries are considered - // tip candidates. + // Ensure that without any assumed-valid BlockIndex entries, only the current tip is + // considered as a candidate. reload_all_block_indexes(); - BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), cs1.m_chain.Height() + 1); + BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), 1); - // Mark some region of the chain assumed-valid. + // Mark some region of the chain assumed-valid, and remove the HAVE_DATA flag. for (int i = 0; i <= cs1.m_chain.Height(); ++i) { LOCK(::cs_main); auto index = cs1.m_chain[i]; + // Blocks with heights in range [20, 40) are marked ASSUMED_VALID if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) { index->nStatus = BlockStatus::BLOCK_VALID_TREE | BlockStatus::BLOCK_ASSUMED_VALID; } @@ -466,33 +467,41 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) validated_tip = index; BOOST_CHECK(!index->IsAssumedValid()); } - // Note the block after the last assumed valid block as the snapshot base - if (i == last_assumed_valid_idx) { + // Note the last assumed valid block as the snapshot base + if (i == last_assumed_valid_idx - 1) { assumed_base = index; + BOOST_CHECK(index->IsAssumedValid()); + } else if (i == last_assumed_valid_idx) { BOOST_CHECK(!index->IsAssumedValid()); } } BOOST_CHECK_EQUAL(expected_assumed_valid, num_assumed_valid); + // Note: cs2's tip is not set when ActivateExistingSnapshot is called. Chainstate& cs2 = WITH_LOCK(::cs_main, return chainman.ActivateExistingSnapshot(&mempool, *assumed_base->phashBlock)); // Set tip of the fully validated chain to be the validated tip cs1.m_chain.SetTip(*validated_tip); + // Set tip of the assume-valid-based chain to the assume-valid block + cs2.m_chain.SetTip(*assumed_base); + reload_all_block_indexes(); - // The fully validated chain only has candidates up to the start of the assumed-valid - // blocks. + // The fully validated chain should have the current validated tip + // and the assumed valid base as candidates. + BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), 2); BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.count(validated_tip), 1); - BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.count(assumed_tip), 0); - BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.size(), assumed_valid_start_idx); + BOOST_CHECK_EQUAL(cs1.setBlockIndexCandidates.count(assumed_base), 1); - // The assumed-valid tolerant chain has all blocks as candidates. - BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(validated_tip), 1); + // The assumed-valid tolerant chain has the assumed valid base as a + // candidate, but otherwise has none of the assumed-valid (which do not + // HAVE_DATA) blocks as candidates. + BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(validated_tip), 0); BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_tip), 1); - BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes); + BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.size(), num_indexes - last_assumed_valid_idx + 1); } //! Ensure that snapshot chainstates initialize properly when found on disk. diff --git a/src/validation.cpp b/src/validation.cpp index a232ceb30f9..88da9164ff9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4432,62 +4432,19 @@ bool ChainstateManager::LoadBlockIndex() std::sort(vSortedByHeight.begin(), vSortedByHeight.end(), CBlockIndexHeightOnlyComparator()); - // Find start of assumed-valid region. - int first_assumed_valid_height = std::numeric_limits::max(); - - for (const CBlockIndex* block : vSortedByHeight) { - if (block->IsAssumedValid()) { - auto chainstates = GetAll(); - - // If we encounter an assumed-valid block index entry, ensure that we have - // one chainstate that tolerates assumed-valid entries and another that does - // not (i.e. the background validation chainstate), since assumed-valid - // entries should always be pending validation by a fully-validated chainstate. - auto any_chain = [&](auto fnc) { return std::any_of(chainstates.cbegin(), chainstates.cend(), fnc); }; - assert(any_chain([](auto chainstate) { return chainstate->reliesOnAssumedValid(); })); - assert(any_chain([](auto chainstate) { return !chainstate->reliesOnAssumedValid(); })); - - first_assumed_valid_height = block->nHeight; - LogPrintf("Saw first assumedvalid block at height %d (%s)\n", - first_assumed_valid_height, block->ToString()); - break; - } - } - for (CBlockIndex* pindex : vSortedByHeight) { if (m_interrupt) return false; - if (pindex->IsAssumedValid() || + // If we have an assumeutxo-based chainstate, then the snapshot + // block will be a candidate for the tip, but it may not be + // VALID_TRANSACTIONS (eg if we haven't yet downloaded the block), + // so we special-case the snapshot block as a potential candidate + // here. + if (pindex == GetSnapshotBaseBlock() || (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) { - // Fill each chainstate's block candidate set. Only add assumed-valid - // blocks to the tip candidate set if the chainstate is allowed to rely on - // assumed-valid blocks. - // - // If all setBlockIndexCandidates contained the assumed-valid blocks, the - // background chainstate's ActivateBestChain() call would add assumed-valid - // blocks to the chain (based on how FindMostWorkChain() works). Obviously - // we don't want this since the purpose of the background validation chain - // is to validate assued-valid blocks. - // - // Note: This is considering all blocks whose height is greater or equal to - // the first assumed-valid block to be assumed-valid blocks, and excluding - // them from the background chainstate's setBlockIndexCandidates set. This - // does mean that some blocks which are not technically assumed-valid - // (later blocks on a fork beginning before the first assumed-valid block) - // might not get added to the background chainstate, but this is ok, - // because they will still be attached to the active chainstate if they - // actually contain more work. - // - // Instead of this height-based approach, an earlier attempt was made at - // detecting "holistically" whether the block index under consideration - // relied on an assumed-valid ancestor, but this proved to be too slow to - // be practical. for (Chainstate* chainstate : GetAll()) { - if (chainstate->reliesOnAssumedValid() || - pindex->nHeight < first_assumed_valid_height) { - chainstate->setBlockIndexCandidates.insert(pindex); - } + chainstate->TryAddBlockIndexCandidate(pindex); } } if (pindex->nStatus & BLOCK_FAILED_MASK && (!m_best_invalid || pindex->nChainWork > m_best_invalid->nChainWork)) {