From 9f6bb539359b98d5b39482ab8a28a68608f0c645 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Sat, 12 Jun 2021 10:52:40 -0400 Subject: [PATCH 01/12] validation: add chainman ref to CChainState Add an upwards reference to chainstate instances to the owning ChainstateManager. This is necessary because there are a number of `this_chainstate == chainman.ActiveChainstate()` checks that will happen (as a result of assumeutxo) in functions that otherwise don't have an easily-accessible reference to the chainstate's ChainManager. --- src/test/validation_flush_tests.cpp | 4 ++-- src/validation.cpp | 14 ++++++++++---- src/validation.h | 6 ++++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp index 22aafcaa6c..9136c497ea 100644 --- a/src/test/validation_flush_tests.cpp +++ b/src/test/validation_flush_tests.cpp @@ -9,7 +9,7 @@ #include -BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, BasicTestingSetup) +BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, ChainTestingSetup) //! Test utilities for detecting when we need to flush the coins cache based //! on estimated memory usage. @@ -20,7 +20,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate) { CTxMemPool mempool; BlockManager blockman{}; - CChainState chainstate{&mempool, blockman}; + CChainState chainstate{&mempool, blockman, *Assert(m_node.chainman)}; chainstate.InitCoinsDB(/*cache_size_bytes*/ 1 << 10, /*in_memory*/ true, /*should_wipe*/ false); WITH_LOCK(::cs_main, chainstate.InitCoinsCache(1 << 10)); diff --git a/src/validation.cpp b/src/validation.cpp index 26333d7026..756e2ba905 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1209,10 +1209,15 @@ void CoinsViews::InitCache() m_cacheview = std::make_unique(&m_catcherview); } -CChainState::CChainState(CTxMemPool* mempool, BlockManager& blockman, std::optional from_snapshot_blockhash) +CChainState::CChainState( + CTxMemPool* mempool, + BlockManager& blockman, + ChainstateManager& chainman, + std::optional from_snapshot_blockhash) : m_mempool(mempool), m_params(::Params()), m_blockman(blockman), + m_chainman(chainman), m_from_snapshot_blockhash(from_snapshot_blockhash) {} void CChainState::InitCoinsDB( @@ -4699,7 +4704,7 @@ CChainState& ChainstateManager::InitializeChainstate( if (to_modify) { throw std::logic_error("should not be overwriting a chainstate"); } - to_modify.reset(new CChainState(mempool, m_blockman, snapshot_blockhash)); + to_modify.reset(new CChainState(mempool, m_blockman, *this, snapshot_blockhash)); // Snapshot chainstates and initial IBD chaintates always become active. if (is_snapshot || (!is_snapshot && !m_active_chainstate)) { @@ -4768,8 +4773,9 @@ bool ChainstateManager::ActivateSnapshot( static_cast(current_coinsdb_cache_size * IBD_CACHE_PERC)); } - auto snapshot_chainstate = WITH_LOCK(::cs_main, return std::make_unique( - /* mempool */ nullptr, m_blockman, base_blockhash)); + auto snapshot_chainstate = WITH_LOCK(::cs_main, + return std::make_unique( + /* mempool */ nullptr, m_blockman, *this, base_blockhash)); { LOCK(::cs_main); diff --git a/src/validation.h b/src/validation.h index 9a2be3ad97..b024699fe8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -601,9 +601,15 @@ public: //! CChainState instances. BlockManager& m_blockman; + //! The chainstate manager that owns this chainstate. The reference is + //! necessary so that this instance can check whether it is the active + //! chainstate within deeply nested method calls. + ChainstateManager& m_chainman; + explicit CChainState( CTxMemPool* mempool, BlockManager& blockman, + ChainstateManager& chainman, std::optional from_snapshot_blockhash = std::nullopt); /** From ac4051d891e2d5c8ac130da16b85b9d880b44720 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 15 Jul 2021 10:40:28 -0400 Subject: [PATCH 02/12] refactor: remove unused assumeutxo methods After feedback from Russ, I realized that there are some extraneous assumeutxo methods that are not necessary and probably just overly confusing. These include - `Validated*()` - `IsBackgroundIBD()` and they can be removed. --- .../validation_chainstatemanager_tests.cpp | 34 ++++++------------- src/validation.cpp | 16 --------- src/validation.h | 19 ----------- 3 files changed, 10 insertions(+), 59 deletions(-) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 0bd378631b..ecd3513fc7 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -44,7 +44,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) BOOST_CHECK(!manager.IsSnapshotActive()); BOOST_CHECK(!manager.IsSnapshotValidated()); - BOOST_CHECK(!manager.IsBackgroundIBD(&c1)); auto all = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); @@ -57,9 +56,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) auto exp_tip = c1.m_chain.Tip(); BOOST_CHECK_EQUAL(active_tip, exp_tip); - auto& validated_cs = manager.ValidatedChainstate(); - BOOST_CHECK_EQUAL(&validated_cs, &c1); - BOOST_CHECK(!manager.SnapshotBlockhash().has_value()); // Create a snapshot-based chainstate. @@ -81,8 +77,8 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) BOOST_CHECK(manager.IsSnapshotActive()); BOOST_CHECK(!manager.IsSnapshotValidated()); - BOOST_CHECK(manager.IsBackgroundIBD(&c1)); - BOOST_CHECK(!manager.IsBackgroundIBD(&c2)); + BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); + BOOST_CHECK(&c1 != &manager.ActiveChainstate()); auto all2 = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end()); @@ -99,16 +95,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) // CCoinsViewCache instances. BOOST_CHECK(exp_tip != exp_tip2); - auto& validated_cs2 = manager.ValidatedChainstate(); - BOOST_CHECK_EQUAL(&validated_cs2, &c1); - - auto& validated_chain = manager.ValidatedChain(); - BOOST_CHECK_EQUAL(&validated_chain, &c1.m_chain); - - auto validated_tip = manager.ValidatedTip(); - exp_tip = c1.m_chain.Tip(); - BOOST_CHECK_EQUAL(validated_tip, exp_tip); - // Let scheduler events finish running to avoid accessing memory that is going to be unloaded SyncWithValidationInterfaceQueue(); @@ -321,27 +307,27 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup) { LOCK(::cs_main); size_t coins_in_active{0}; - size_t coins_in_ibd{0}; - size_t coins_missing_ibd{0}; + size_t coins_in_background{0}; + size_t coins_missing_from_background{0}; for (CChainState* chainstate : chainman.GetAll()) { BOOST_TEST_MESSAGE("Checking coins in " << chainstate->ToString()); CCoinsViewCache& coinscache = chainstate->CoinsTip(); - bool is_ibd = chainman.IsBackgroundIBD(chainstate); + bool is_background = chainstate != &chainman.ActiveChainstate(); for (CTransactionRef& txn : m_coinbase_txns) { COutPoint op{txn->GetHash(), 0}; if (coinscache.HaveCoin(op)) { - (is_ibd ? coins_in_ibd : coins_in_active)++; - } else if (is_ibd) { - coins_missing_ibd++; + (is_background ? coins_in_background : coins_in_active)++; + } else if (is_background) { + coins_missing_from_background++; } } } BOOST_CHECK_EQUAL(coins_in_active, initial_total_coins + new_coins); - BOOST_CHECK_EQUAL(coins_in_ibd, initial_total_coins); - BOOST_CHECK_EQUAL(coins_missing_ibd, new_coins); + BOOST_CHECK_EQUAL(coins_in_background, initial_total_coins); + BOOST_CHECK_EQUAL(coins_missing_from_background, new_coins); } // Snapshot should refuse to load after one has already loaded. diff --git a/src/validation.cpp b/src/validation.cpp index 756e2ba905..bed97d05c5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5007,22 +5007,6 @@ bool ChainstateManager::IsSnapshotActive() const return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get(); } -CChainState& ChainstateManager::ValidatedChainstate() const -{ - LOCK(::cs_main); - if (m_snapshot_chainstate && IsSnapshotValidated()) { - return *m_snapshot_chainstate.get(); - } - assert(m_ibd_chainstate); - return *m_ibd_chainstate.get(); -} - -bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const -{ - LOCK(::cs_main); - return (m_snapshot_chainstate && chainstate == m_ibd_chainstate.get()); -} - void ChainstateManager::Unload() { for (CChainState* chainstate : this->GetAll()) { diff --git a/src/validation.h b/src/validation.h index b024699fe8..4fc46439cc 100644 --- a/src/validation.h +++ b/src/validation.h @@ -862,12 +862,6 @@ private: * *Background IBD chainstate*: an IBD chainstate for which the * IBD process is happening in the background while use of the * active (snapshot) chainstate allows the rest of the system to function. - * - * *Validated chainstate*: the most-work chainstate which has been validated - * locally via initial block download. This will be the snapshot chainstate - * if a snapshot was loaded and all blocks up to the snapshot starting point - * have been downloaded and validated (via background validation), otherwise - * it will be the IBD chainstate. */ class ChainstateManager { @@ -986,19 +980,6 @@ public: //! Is there a snapshot in use and has it been fully validated? bool IsSnapshotValidated() const { return m_snapshot_validated; } - //! @returns true if this chainstate is being used to validate an active - //! snapshot in the background. - bool IsBackgroundIBD(CChainState* chainstate) const; - - //! Return the most-work chainstate that has been fully validated. - //! - //! During background validation of a snapshot, this is the IBD chain. After - //! background validation has completed, this is the snapshot chain. - CChainState& ValidatedChainstate() const; - - CChain& ValidatedChain() const { return ValidatedChainstate().m_chain; } - CBlockIndex* ValidatedTip() const { return ValidatedChain().Tip(); } - /** * Process an incoming block. This only returns after the best known valid * block is made active. Note that it does not, however, guarantee that the From 665072a36df2e4c88705fedd4ac7c955d7f6a488 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 15 Sep 2021 15:46:29 -0400 Subject: [PATCH 03/12] doc: add comment for g_best_block --- src/validation.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.h b/src/validation.h index 4fc46439cc..d457f1ac65 100644 --- a/src/validation.h +++ b/src/validation.h @@ -109,6 +109,7 @@ extern RecursiveMutex cs_main; typedef std::unordered_map BlockMap; extern Mutex g_best_block_mutex; extern std::condition_variable g_best_block_cv; +/** Used to notify getblocktemplate RPC of new tips. */ extern uint256 g_best_block; /** Whether there are dedicated script-checking threads running. * False indicates all script checking is done on the main threadMessageHandler thread. From b217020df78bc981d221fe04497c831120ef969f Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Mon, 16 Sep 2019 16:32:18 -0400 Subject: [PATCH 04/12] validation: change UpdateTip for multiple chainstates Only perform certain behavior (namely that related to servicing the getblocktemplate RPC call) for the active chainstate when calling UpdateTip. Co-authored-by: Jon Atack --- src/validation.cpp | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index bed97d05c5..69f461202b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2208,8 +2208,42 @@ static void AppendWarning(bilingual_str& res, const bilingual_str& warn) res += warn; } +static void UpdateTipLog( + const CCoinsViewCache& coins_tip, + const CBlockIndex* tip, + const CChainParams& params, + const std::string& func_name, + const std::string& prefix, + const std::string& warning_messages) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) +{ + + AssertLockHeld(::cs_main); + LogPrintf("%s%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n", + prefix, func_name, + tip->GetBlockHash().ToString(), tip->nHeight, tip->nVersion, + log(tip->nChainWork.getdouble()) / log(2.0), (unsigned long)tip->nChainTx, + FormatISO8601DateTime(tip->GetBlockTime()), + GuessVerificationProgress(params.TxData(), tip), + coins_tip.DynamicMemoryUsage() * (1.0 / (1 << 20)), + coins_tip.GetCacheSize(), + !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : ""); +} + void CChainState::UpdateTip(const CBlockIndex* pindexNew) { + const auto& coins_tip = this->CoinsTip(); + + // The remainder of the function isn't relevant if we are not acting on + // the active chainstate, so return if need be. + if (this != &m_chainman.ActiveChainstate()) { + // Only log every so often so that we don't bury log messages at the tip. + constexpr int BACKGROUND_LOG_INTERVAL = 2000; + if (pindexNew->nHeight % BACKGROUND_LOG_INTERVAL == 0) { + UpdateTipLog(coins_tip, pindexNew, m_params, __func__, "[background validation] ", ""); + } + return; + } + // New best block if (m_mempool) { m_mempool->AddTransactionsUpdated(1); @@ -2237,12 +2271,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew) } } } - LogPrintf("%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n", __func__, - pindexNew->GetBlockHash().ToString(), pindexNew->nHeight, pindexNew->nVersion, - log(pindexNew->nChainWork.getdouble())/log(2.0), (unsigned long)pindexNew->nChainTx, - FormatISO8601DateTime(pindexNew->GetBlockTime()), - GuessVerificationProgress(m_params.TxData(), pindexNew), this->CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)), this->CoinsTip().GetCacheSize(), - !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages.original) : ""); + UpdateTipLog(coins_tip, pindexNew, m_params, __func__, "", warning_messages.original); } /** Disconnect m_chain's tip. From 42b2520db93fd9feb3df4101654391fa7d3e2140 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 21 Jul 2021 13:31:14 -0400 Subject: [PATCH 05/12] chain: add BLOCK_ASSUMED_VALID for use with assumeutxo Instead of (ab)using the existing BLOCK_VALID_* flags to mark CBlockIndex entries which we haven't yet fully validated (but assume validity for use with UTXO snapshot loading), introduce a status flag that specifically marks an assumed-valid state. This state is then removed in RaiseValidity() when the block has actually been validated. This distinction will allow us to make the necessary changes to various parts of the system to facilitate assumeutxo/background chainstate validation but without leaking details like snapshot height, as we had done previously. Changes that actually make use of this flag follow in future commits. --- src/chain.h | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/chain.h b/src/chain.h index 84a3a4e1e7..365a7f79b6 100644 --- a/src/chain.h +++ b/src/chain.h @@ -126,7 +126,15 @@ enum BlockStatus: uint32_t { BLOCK_FAILED_CHILD = 64, //!< descends from failed block BLOCK_FAILED_MASK = BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD, - BLOCK_OPT_WITNESS = 128, //!< block data in blk*.data was received with a witness-enforcing client + BLOCK_OPT_WITNESS = 128, //!< block data in blk*.dat was received with a witness-enforcing client + + /** + * If set, this indicates that the block index entry is assumed-valid. + * Certain diagnostics will be skipped in e.g. CheckBlockIndex(). + * It almost certainly means that the block's full validation is pending + * on a background chainstate. See `doc/assumeutxo.md`. + */ + BLOCK_ASSUMED_VALID = 256, }; /** The block chain is a tree shaped structure starting with the @@ -300,14 +308,24 @@ public: return ((nStatus & BLOCK_VALID_MASK) >= nUpTo); } + //! @returns true if the block is assumed-valid; this means it is queued to be + //! validated by a background chainstate. + bool IsAssumedValid() const { return nStatus & BLOCK_ASSUMED_VALID; } + //! Raise the validity level of this block index entry. //! Returns true if the validity was changed. bool RaiseValidity(enum BlockStatus nUpTo) { assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed. - if (nStatus & BLOCK_FAILED_MASK) - return false; + if (nStatus & BLOCK_FAILED_MASK) return false; + if ((nStatus & BLOCK_VALID_MASK) < nUpTo) { + // If this block had been marked assumed-valid and we're raising + // its validity to a certain point, there is no longer an assumption. + if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) { + nStatus &= ~BLOCK_ASSUMED_VALID; + } + nStatus = (nStatus & ~BLOCK_VALID_MASK) | nUpTo; return true; } From 01a9b8fe719efab2c268dc738bc93cfbdf92edb7 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 21 Jul 2021 14:08:14 -0400 Subject: [PATCH 06/12] validation: set BLOCK_ASSUMED_VALID during snapshot load Mark the block index entries that are beneath the snapshot base block as assumed-valid. Subsequent commits will make use of this flag in other parts of the system. --- src/validation.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 69f461202b..d34f4d636c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5007,11 +5007,25 @@ bool ChainstateManager::PopulateAndValidateSnapshot( // Fake nChainTx so that GuessVerificationProgress reports accurately index->nChainTx = index->pprev ? index->pprev->nChainTx + index->nTx : 1; + // Mark unvalidated block index entries beneath the snapshot base block as assumed-valid. + if (!index->IsValid(BLOCK_VALID_SCRIPTS)) { + // This flag will be removed once the block is fully validated by a + // background chainstate. + index->nStatus |= BLOCK_ASSUMED_VALID; + } + // Fake BLOCK_OPT_WITNESS so that CChainState::NeedsRedownload() // won't ask to rewind the entire assumed-valid chain on startup. if (index->pprev && DeploymentActiveAt(*index, ::Params().GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) { index->nStatus |= BLOCK_OPT_WITNESS; } + + setDirtyBlockIndex.insert(index); + // Changes to the block index will be flushed to disk after this call + // returns in `ActivateSnapshot()`, when `MaybeRebalanceCaches()` is + // called, since we've added a snapshot chainstate and therefore will + // have to downsize the IBD chainstate, which will result in a call to + // `FlushStateToDisk(ALWAYS)`. } assert(index); From 5a807736dacfc3e6fa57231219336acf08be38fb Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 21 Jul 2021 14:13:33 -0400 Subject: [PATCH 07/12] validation: insert assumed-valid block index entries into candidates --- src/validation.cpp | 4 +++- src/validation.h | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d34f4d636c..fc498f6e9b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3773,7 +3773,9 @@ bool BlockManager::LoadBlockIndex( pindex->nStatus |= BLOCK_FAILED_CHILD; setDirtyBlockIndex.insert(pindex); } - if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr)) { + if (pindex->IsAssumedValid() || + (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && + (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))) { block_index_candidates.insert(pindex); } if (pindex->nStatus & BLOCK_FAILED_MASK && (!pindexBestInvalid || pindex->nChainWork > pindexBestInvalid->nChainWork)) diff --git a/src/validation.h b/src/validation.h index d457f1ac65..fb12f65144 100644 --- a/src/validation.h +++ b/src/validation.h @@ -647,9 +647,10 @@ public: const std::optional m_from_snapshot_blockhash; /** - * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and - * as good as our current tip or better. Entries may be failed, though, and pruning nodes may be - * missing the data for the block. + * The set of all CBlockIndex entries with either BLOCK_VALID_TRANSACTIONS (for + * itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using background + * chainstates) and as good as our current tip or better. Entries may be failed, + * though, and pruning nodes may be missing the data for the block. */ std::set setBlockIndexCandidates; From 8f5710fd0ac5173b577e5d00708485170b321bcc Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 8 Apr 2021 10:04:08 -0400 Subject: [PATCH 08/12] validation: fix CheckBlockIndex for multiple chainstates Adjust CheckBlockIndex to account for - assumed-valid block indexes lacking transaction data, and - setBlockIndexCandidates for the background chainstate not containing certain entries which rely on assumed-valid ancestors. --- src/validation.cpp | 52 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index fc498f6e9b..a11dec00a4 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4354,12 +4354,33 @@ void CChainState::CheckBlockIndex() while (pindex != nullptr) { nNodes++; if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex; - if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex; + // Assumed-valid index entries will not have data since we haven't downloaded the + // full block yet. + if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA) && !pindex->IsAssumedValid()) { + pindexFirstMissing = pindex; + } if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex; if (pindex->pprev != nullptr && pindexFirstNotTreeValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TREE) pindexFirstNotTreeValid = pindex; - if (pindex->pprev != nullptr && pindexFirstNotTransactionsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS) pindexFirstNotTransactionsValid = pindex; - if (pindex->pprev != nullptr && pindexFirstNotChainValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_CHAIN) pindexFirstNotChainValid = pindex; - if (pindex->pprev != nullptr && pindexFirstNotScriptsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_SCRIPTS) pindexFirstNotScriptsValid = pindex; + + if (pindex->pprev != nullptr && !pindex->IsAssumedValid()) { + // Skip validity flag checks for BLOCK_ASSUMED_VALID index entries, since these + // *_VALID_MASK flags will not be present for index entries we are temporarily assuming + // valid. + if (pindexFirstNotTransactionsValid == nullptr && + (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS) { + pindexFirstNotTransactionsValid = pindex; + } + + if (pindexFirstNotChainValid == nullptr && + (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_CHAIN) { + pindexFirstNotChainValid = pindex; + } + + if (pindexFirstNotScriptsValid == nullptr && + (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_SCRIPTS) { + pindexFirstNotScriptsValid = pindex; + } + } // Begin: actual consistency checks. if (pindex->pprev == nullptr) { @@ -4370,7 +4391,9 @@ void CChainState::CheckBlockIndex() if (!pindex->HaveTxsDownloaded()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock) // 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 (!fHavePruned) { + // Unless these indexes are assumed valid and pending block download on a + // background chainstate. + if (!fHavePruned && !pindex->IsAssumedValid()) { // If we've never pruned, then HAVE_DATA should be equivalent to nTx > 0 assert(!(pindex->nStatus & BLOCK_HAVE_DATA) == (pindex->nTx == 0)); assert(pindexFirstMissing == pindexFirstNeverProcessed); @@ -4379,7 +4402,16 @@ void CChainState::CheckBlockIndex() if (pindex->nStatus & BLOCK_HAVE_DATA) assert(pindex->nTx > 0); } if (pindex->nStatus & BLOCK_HAVE_UNDO) assert(pindex->nStatus & BLOCK_HAVE_DATA); - assert(((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TRANSACTIONS) == (pindex->nTx > 0)); // This is pruning-independent. + if (pindex->IsAssumedValid()) { + // Assumed-valid blocks should have some nTx value. + assert(pindex->nTx > 0); + // Assumed-valid blocks should connect to the main chain. + assert((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TREE); + } else { + // Otherwise there should only be an nTx value if we have + // actually seen a block's transactions. + assert(((pindex->nStatus & BLOCK_VALID_MASK) >= BLOCK_VALID_TRANSACTIONS) == (pindex->nTx > 0)); // This is pruning-independent. + } // All parents having had data (at some point) is equivalent to all parents being VALID_TRANSACTIONS, which is equivalent to HaveTxsDownloaded(). assert((pindexFirstNeverProcessed == nullptr) == pindex->HaveTxsDownloaded()); assert((pindexFirstNotTransactionsValid == nullptr) == pindex->HaveTxsDownloaded()); @@ -4396,11 +4428,17 @@ void CChainState::CheckBlockIndex() } if (!CBlockIndexWorkComparator()(pindex, m_chain.Tip()) && pindexFirstNeverProcessed == nullptr) { if (pindexFirstInvalid == nullptr) { + const bool is_active = this == &m_chainman.ActiveChainstate(); + // If this block sorts at least as good as the current tip and // is valid and we have all data for its parents, it must be in // setBlockIndexCandidates. m_chain.Tip() must also be there // even if some data has been pruned. - if (pindexFirstMissing == nullptr || pindex == m_chain.Tip()) { + // + // Don't perform this check for the background chainstate since + // its setBlockIndexCandidates shouldn't have some entries (i.e. those past the + // snapshot block) which do exist in the block index for the active chainstate. + if (is_active && (pindexFirstMissing == nullptr || pindex == m_chain.Tip())) { assert(setBlockIndexCandidates.count(pindex)); } // If some parent is missing, then it could be that this block was in From 071200993f3a9412821ce5387851d659baf85327 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 7 Apr 2021 11:48:02 -0400 Subject: [PATCH 09/12] move-only: unittest: add test/util/chainstate.h and move `CreateAndActivateUTXOSnapshot()` into it for reuse in future test modules. --- src/Makefile.test_util.include | 1 + src/test/util/chainstate.h | 54 +++++++++++++++++++ .../validation_chainstatemanager_tests.cpp | 32 +---------- 3 files changed, 56 insertions(+), 31 deletions(-) create mode 100644 src/test/util/chainstate.h diff --git a/src/Makefile.test_util.include b/src/Makefile.test_util.include index 85e50ebf70..0a3b99e7d2 100644 --- a/src/Makefile.test_util.include +++ b/src/Makefile.test_util.include @@ -9,6 +9,7 @@ EXTRA_LIBRARIES += \ TEST_UTIL_H = \ test/util/blockfilter.h \ + test/util/chainstate.h \ test/util/logging.h \ test/util/mining.h \ test/util/net.h \ diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h new file mode 100644 index 0000000000..38a6be7e9a --- /dev/null +++ b/src/test/util/chainstate.h @@ -0,0 +1,54 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. +// +#ifndef BITCOIN_TEST_UTIL_CHAINSTATE_H +#define BITCOIN_TEST_UTIL_CHAINSTATE_H + +#include +#include +#include +#include +#include +#include + +#include + +#include + +auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){}; + +/** + * Create and activate a UTXO snapshot, optionally providing a function to + * malleate the snapshot. + */ +template +static bool +CreateAndActivateUTXOSnapshot(NodeContext& node, const fs::path root, F malleation = NoMalleation) +{ + // Write out a snapshot to the test's tempdir. + // + int height; + WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight()); + fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height); + FILE* outfile{fsbridge::fopen(snapshot_path, "wb")}; + CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION}; + + UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), auto_outfile); + BOOST_TEST_MESSAGE( + "Wrote UTXO snapshot to " << snapshot_path.make_preferred().string() << ": " << result.write()); + + // Read the written snapshot in and then activate it. + // + FILE* infile{fsbridge::fopen(snapshot_path, "rb")}; + CAutoFile auto_infile{infile, SER_DISK, CLIENT_VERSION}; + SnapshotMetadata metadata; + auto_infile >> metadata; + + malleation(auto_infile, metadata); + + return node.chainman->ActivateSnapshot(auto_infile, metadata, /*in_memory*/ true); +} + + +#endif // BITCOIN_TEST_UTIL_CHAINSTATE_H diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index ecd3513fc7..2b53c10fe9 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -9,12 +9,12 @@ #include #include #include +#include #include #include #include #include -#include #include @@ -154,36 +154,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches) BOOST_CHECK_CLOSE(c2.m_coinsdb_cache_size_bytes, max_cache * 0.95, 1); } -auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){}; - -template -static bool -CreateAndActivateUTXOSnapshot(NodeContext& node, const fs::path root, F malleation = NoMalleation) -{ - // Write out a snapshot to the test's tempdir. - // - int height; - WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight()); - fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height); - FILE* outfile{fsbridge::fopen(snapshot_path, "wb")}; - CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION}; - - UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), auto_outfile); - BOOST_TEST_MESSAGE( - "Wrote UTXO snapshot to " << snapshot_path.make_preferred().string() << ": " << result.write()); - - // Read the written snapshot in and then activate it. - // - FILE* infile{fsbridge::fopen(snapshot_path, "rb")}; - CAutoFile auto_infile{infile, SER_DISK, CLIENT_VERSION}; - SnapshotMetadata metadata; - auto_infile >> metadata; - - malleation(auto_infile, metadata); - - return node.chainman->ActivateSnapshot(auto_infile, metadata, /*in_memory*/ true); -} - //! Test basic snapshot activation. BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup) { From 298bf5d563cc740c6ae71750d86942e0278b22d6 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Wed, 15 Sep 2021 15:39:40 -0400 Subject: [PATCH 10/12] test: refactor: declare NoMalleation const auto To avoid linker error on some platforms: https://github.com/bitcoin/bitcoin/pull/21526#discussion_r709404714 Co-authored-by: Russ Yanofsky --- src/test/util/chainstate.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h index 38a6be7e9a..81ea4c38f5 100644 --- a/src/test/util/chainstate.h +++ b/src/test/util/chainstate.h @@ -16,7 +16,7 @@ #include -auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){}; +const auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){}; /** * Create and activate a UTXO snapshot, optionally providing a function to From 2705570109a2a90ecfd3f4180944498626fc2707 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 8 Apr 2021 10:06:10 -0400 Subject: [PATCH 11/12] test: refactor: separate CreateBlock in TestChain100Setup This is so we can create blocks within unittests and have them be processed by specific chainstates (instead of the just the active one). --- src/test/util/setup_common.cpp | 21 +++++++++++++++++-- src/test/util/setup_common.h | 13 +++++++++++- .../validation_chainstatemanager_tests.cpp | 2 +- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index d9d236be1d..b3342aa424 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -237,11 +237,14 @@ void TestChain100Setup::mineBlocks(int num_blocks) } } -CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector& txns, const CScript& scriptPubKey) +CBlock TestChain100Setup::CreateBlock( + const std::vector& txns, + const CScript& scriptPubKey, + CChainState& chainstate) { const CChainParams& chainparams = Params(); CTxMemPool empty_pool; - CBlock block = BlockAssembler(m_node.chainman->ActiveChainstate(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block; + CBlock block = BlockAssembler(chainstate, empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block; Assert(block.vtx.size() == 1); for (const CMutableTransaction& tx : txns) { @@ -251,6 +254,20 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector& txns, + const CScript& scriptPubKey, + CChainState* chainstate) +{ + if (!chainstate) { + chainstate = &Assert(m_node.chainman)->ActiveChainstate(); + } + + const CChainParams& chainparams = Params(); + const CBlock block = this->CreateBlock(txns, scriptPubKey, *chainstate); std::shared_ptr shared_pblock = std::make_shared(block); Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index 5d12dc2323..acb96e54df 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -119,9 +119,20 @@ struct TestChain100Setup : public RegTestingSetup { /** * Create a new block with just given transactions, coinbase paying to * scriptPubKey, and try to add it to the current chain. + * If no chainstate is specified, default to the active. */ CBlock CreateAndProcessBlock(const std::vector& txns, - const CScript& scriptPubKey); + const CScript& scriptPubKey, + CChainState* chainstate = nullptr); + + /** + * Create a new block with just given transactions, coinbase paying to + * scriptPubKey. + */ + CBlock CreateBlock( + const std::vector& txns, + const CScript& scriptPubKey, + CChainState& chainstate); //! Mine a series of new blocks on the active chain. void mineBlocks(int num_blocks); diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 2b53c10fe9..be9e05a65e 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -8,8 +8,8 @@ #include #include #include -#include #include +#include #include #include #include From 673a5bd3377929a0a6a62eda8b560e47bc2cca0c Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Thu, 8 Apr 2021 10:07:58 -0400 Subject: [PATCH 12/12] test: validation: add unittest for UpdateTip behavior --- src/test/validation_chainstate_tests.cpp | 76 ++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 2893b412fb..2514e65194 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -2,10 +2,13 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. // +#include #include #include #include #include +#include +#include #include #include @@ -72,4 +75,77 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches) WITH_LOCK(::cs_main, manager.Unload()); } +//! Test UpdateTip behavior for both active and background chainstates. +//! +//! When run on the background chainstate, UpdateTip should do a subset +//! of what it does for the active chainstate. +BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) +{ + ChainstateManager& chainman = *Assert(m_node.chainman); + uint256 curr_tip = ::g_best_block; + + // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can + // be found. + mineBlocks(10); + + // After adding some blocks to the tip, best block should have changed. + BOOST_CHECK(::g_best_block != curr_tip); + + BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root)); + + // Ensure our active chain is the snapshot chainstate. + BOOST_CHECK(chainman.IsSnapshotActive()); + + curr_tip = ::g_best_block; + + // Mine a new block on top of the activated snapshot chainstate. + mineBlocks(1); // Defined in TestChain100Setup. + + // After adding some blocks to the snapshot tip, best block should have changed. + BOOST_CHECK(::g_best_block != curr_tip); + + curr_tip = ::g_best_block; + + CChainState* background_cs; + + BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2); + for (CChainState* cs : chainman.GetAll()) { + if (cs != &chainman.ActiveChainstate()) { + background_cs = cs; + } + } + BOOST_CHECK(background_cs); + + // 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); + BlockValidationState state; + CBlockIndex* pindex = nullptr; + const CChainParams& chainparams = Params(); + bool newblock = false; + + // TODO: much of this is inlined from ProcessNewBlock(); just reuse PNB() + // once it is changed to support multiple chainstates. + { + LOCK(::cs_main); + bool checked = CheckBlock(*pblock, state, chainparams.GetConsensus()); + BOOST_CHECK(checked); + bool accepted = background_cs->AcceptBlock( + pblock, state, &pindex, true, nullptr, &newblock); + BOOST_CHECK(accepted); + } + // UpdateTip is called here + bool block_added = background_cs->ActivateBestChain(state, pblock); + + // Ensure tip is as expected + BOOST_CHECK_EQUAL(background_cs->m_chain.Tip()->GetBlockHash(), validation_block.GetHash()); + + // g_best_block should be unchanged after adding a block to the background + // validation chain. + BOOST_CHECK(block_added); + BOOST_CHECK_EQUAL(curr_tip, ::g_best_block); +} + BOOST_AUTO_TEST_SUITE_END()