From d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 24 May 2023 13:38:32 -0400 Subject: [PATCH] Tighten requirements for adding elements to setBlockIndexCandidates When using assumeutxo, we only need the background chainstate to consider blocks that are on the chain leading to the snapshotted block. Note that this introduces the new invariant that we can only have an assumeutxo snapshot where the snapshotted blockhash is in our block index. Unknown block hashes that are somehow passed in will cause assertion failures when processing new blocks. Includes test fixes and improvements by Andrew Chow and Fabian Jahr. --- src/test/validation_chainstate_tests.cpp | 21 ++++++----- .../validation_chainstatemanager_tests.cpp | 36 +++++++++++-------- src/validation.cpp | 19 ++++++++-- 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 3ea87143b0..fe2d2ba592 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -77,6 +77,13 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // After adding some blocks to the tip, best block should have changed. BOOST_CHECK(::g_best_block != curr_tip); + // Grab block 1 from disk; we'll add it to the background chain later. + std::shared_ptr pblockone = std::make_shared(); + { + LOCK(::cs_main); + chainman.m_blockman.ReadBlockFromDisk(*pblockone, *chainman.ActiveChain()[1]); + } + BOOST_REQUIRE(CreateAndActivateUTXOSnapshot( this, NoMalleation, /*reset_chainstate=*/ true)); @@ -104,11 +111,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) assert(false); }()}; - // Create a block to append to the validation chain. - std::vector noTxns; - CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG; - CBlock validation_block = this->CreateBlock(noTxns, scriptPubKey, background_cs); - auto pblock = std::make_shared(validation_block); + // Append the first block to the background chain. BlockValidationState state; CBlockIndex* pindex = nullptr; const CChainParams& chainparams = Params(); @@ -118,18 +121,18 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) // once it is changed to support multiple chainstates. { LOCK(::cs_main); - bool checked = CheckBlock(*pblock, state, chainparams.GetConsensus()); + bool checked = CheckBlock(*pblockone, state, chainparams.GetConsensus()); BOOST_CHECK(checked); bool accepted = chainman.AcceptBlock( - pblock, state, &pindex, true, nullptr, &newblock, true); + pblockone, state, &pindex, true, nullptr, &newblock, true); BOOST_CHECK(accepted); } // UpdateTip is called here - bool block_added = background_cs.ActivateBestChain(state, pblock); + bool block_added = background_cs.ActivateBestChain(state, pblockone); // Ensure tip is as expected - BOOST_CHECK_EQUAL(background_cs.m_chain.Tip()->GetBlockHash(), validation_block.GetHash()); + BOOST_CHECK_EQUAL(background_cs.m_chain.Tip()->GetBlockHash(), pblockone->GetHash()); // g_best_block should be unchanged after adding a block to the background // validation chain. diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 442c7036e6..e7cd1043a9 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -49,6 +49,9 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) c1.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23)); + c1.LoadGenesisBlock(); + BlockValidationState val_state; + BOOST_CHECK(c1.ActivateBestChain(val_state, nullptr)); BOOST_CHECK(!manager.IsSnapshotActive()); BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); @@ -58,7 +61,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) auto& active_chain = WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()); BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain); - BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), -1); + BOOST_CHECK_EQUAL(WITH_LOCK(manager.GetMutex(), return manager.ActiveHeight()), 0); auto active_tip = WITH_LOCK(manager.GetMutex(), return manager.ActiveTip()); auto exp_tip = c1.m_chain.Tip(); @@ -68,7 +71,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) // Create a snapshot-based chainstate. // - const uint256 snapshot_blockhash = GetRandHash(); + const uint256 snapshot_blockhash = active_tip->GetBlockHash(); Chainstate& c2 = WITH_LOCK(::cs_main, return manager.ActivateExistingSnapshot( &mempool, snapshot_blockhash)); chainstates.push_back(&c2); @@ -78,8 +81,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) c2.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); WITH_LOCK(::cs_main, c2.InitCoinsCache(1 << 23)); - // Unlike c1, which doesn't have any blocks. Gets us different tip, height. - c2.LoadGenesisBlock(); + c2.m_chain.SetTip(*active_tip); BlockValidationState _; BOOST_CHECK(c2.ActivateBestChain(_, nullptr)); @@ -99,16 +101,14 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) auto exp_tip2 = c2.m_chain.Tip(); BOOST_CHECK_EQUAL(active_tip2, exp_tip2); - // Ensure that these pointers actually correspond to different - // CCoinsViewCache instances. - BOOST_CHECK(exp_tip != exp_tip2); + BOOST_CHECK_EQUAL(exp_tip, exp_tip2); // Let scheduler events finish running to avoid accessing memory that is going to be unloaded SyncWithValidationInterfaceQueue(); } //! Test rebalancing the caches associated with each chainstate. -BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) +BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup) { ChainstateManager& manager = *m_node.chainman; CTxMemPool& mempool = *m_node.mempool; @@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) // Create a legacy (IBD) chainstate. // - Chainstate& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(&mempool)); + Chainstate& c1 = manager.ActiveChainstate(); chainstates.push_back(&c1); c1.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); @@ -129,8 +129,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) { LOCK(::cs_main); c1.InitCoinsCache(1 << 23); - BOOST_REQUIRE(c1.LoadGenesisBlock()); - c1.CoinsTip().SetBestBlock(InsecureRand256()); manager.MaybeRebalanceCaches(); } @@ -139,7 +137,8 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) // Create a snapshot-based chainstate. // - Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(&mempool, GetRandHash())); + CBlockIndex* snapshot_base{WITH_LOCK(manager.GetMutex(), return manager.ActiveChain()[manager.ActiveChain().Height() / 2])}; + Chainstate& c2 = WITH_LOCK(cs_main, return manager.ActivateExistingSnapshot(&mempool, *snapshot_base->phashBlock)); chainstates.push_back(&c2); c2.InitCoinsDB( /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false); @@ -147,8 +146,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) { LOCK(::cs_main); c2.InitCoinsCache(1 << 23); - BOOST_REQUIRE(c2.LoadGenesisBlock()); - c2.CoinsTip().SetBestBlock(InsecureRand256()); manager.MaybeRebalanceCaches(); } @@ -430,6 +427,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup) const int assumed_valid_start_idx = last_assumed_valid_idx - expected_assumed_valid; CBlockIndex* validated_tip{nullptr}; + CBlockIndex* assumed_base{nullptr}; CBlockIndex* assumed_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveChain().Tip())}; auto reload_all_block_indexes = [&]() { @@ -468,12 +466,20 @@ 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) { + assumed_base = index; + BOOST_CHECK(!index->IsAssumedValid()); + } } BOOST_CHECK_EQUAL(expected_assumed_valid, num_assumed_valid); Chainstate& cs2 = WITH_LOCK(::cs_main, - return chainman.ActivateExistingSnapshot(&mempool, GetRandHash())); + 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); reload_all_block_indexes(); diff --git a/src/validation.cpp b/src/validation.cpp index 16cec9198a..a232ceb30f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3419,9 +3419,24 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex) { AssertLockHeld(cs_main); - // If the block has more work than our tip, then it should be a candidate for most-work-chain. - if (m_chain.Tip() == nullptr || !setBlockIndexCandidates.value_comp()(pindex, m_chain.Tip())) { + // The block only is a candidate for the most-work-chain if it has more work than our current tip. + if (m_chain.Tip() != nullptr && setBlockIndexCandidates.value_comp()(pindex, m_chain.Tip())) { + return; + } + + bool is_active_chainstate = this == &m_chainman.ActiveChainstate(); + if (is_active_chainstate) { + // The active chainstate should always add entries that have more + // work than the tip. setBlockIndexCandidates.insert(pindex); + } else if (!m_disabled) { + // For the background chainstate, we only consider connecting blocks + // towards the snapshot base (which can't be nullptr or else we'll + // never make progress). + const CBlockIndex* snapshot_base = Assert(m_chainman.GetSnapshotBaseBlock()); + if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) { + setBlockIndexCandidates.insert(pindex); + } } }