From d19ffd613a018f3780529979bbaa808087c035cc Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 5 Jul 2024 09:54:38 +0000 Subject: [PATCH 01/12] merge bitcoin#22278: Add LIFETIMEBOUND to CScript where needed --- src/script/script.h | 9 +++++---- src/validation.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/script/script.h b/src/script/script.h index 940664e9fca4a..873f1e1d8806a 100644 --- a/src/script/script.h +++ b/src/script/script.h @@ -6,6 +6,7 @@ #ifndef BITCOIN_SCRIPT_SCRIPT_H #define BITCOIN_SCRIPT_SCRIPT_H +#include #include #include #include @@ -426,9 +427,9 @@ class CScript : public CScriptBase /** Delete non-existent operator to defend against future introduction */ CScript& operator<<(const CScript& b) = delete; - CScript& operator<<(int64_t b) { return push_int64(b); } + CScript& operator<<(int64_t b) LIFETIMEBOUND { return push_int64(b); } - CScript& operator<<(opcodetype opcode) + CScript& operator<<(opcodetype opcode) LIFETIMEBOUND { if (opcode < 0 || opcode > 0xff) throw std::runtime_error("CScript::operator<<(): invalid opcode"); @@ -436,13 +437,13 @@ class CScript : public CScriptBase return *this; } - CScript& operator<<(const CScriptNum& b) + CScript& operator<<(const CScriptNum& b) LIFETIMEBOUND { *this << b.getvch(); return *this; } - CScript& operator<<(const std::vector& b) + CScript& operator<<(const std::vector& b) LIFETIMEBOUND { if (b.size() < OP_PUSHDATA1) { diff --git a/src/validation.h b/src/validation.h index ec9237f267aa6..e2aa77ffe77f3 100644 --- a/src/validation.h +++ b/src/validation.h @@ -897,7 +897,7 @@ class ChainstateManager const std::unique_ptr& clhandler, const std::unique_ptr& isman, const std::optional& snapshot_blockhash = std::nullopt) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Get all chainstates currently being used. std::vector GetAll(); From edc665c80da2baf329955b91e102894ad84839eb Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 4 Jul 2024 14:26:26 +0000 Subject: [PATCH 02/12] merge bitcoin#24103: Replace RecursiveMutex m_cs_chainstate with Mutex, and rename it --- src/validation.cpp | 13 +++++++++---- src/validation.h | 13 +++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 6e9f7149604e5..c7c76d9f9e930 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2934,6 +2934,8 @@ static void LimitValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main) { bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr pblock) { + AssertLockNotHeld(m_chainstate_mutex); + // Note that while we're often called here from ProcessNewBlock, this is // far from a guarantee. Things in the P2P/RPC will often end up calling // us in the middle of ProcessNewBlock - do not assume pblock is set @@ -2943,8 +2945,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr // ABC maintains a fair degree of expensive-to-calculate internal state // because this function periodically releases cs_main so that it does not lock up other threads for too long // during large connects - and to allow for e.g. the callback queue to drain - // we use m_cs_chainstate to enforce mutual exclusion so that only one caller may execute this function at a time - LOCK(m_cs_chainstate); + // we use m_chainstate_mutex to enforce mutual exclusion so that only one caller may execute this function at a time + LOCK(m_chainstate_mutex); auto start = Now(); @@ -3070,6 +3072,8 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) { + AssertLockNotHeld(m_chainstate_mutex); + // Genesis block can't be invalidated assert(pindex); if (pindex->nHeight == 0) return false; @@ -3081,7 +3085,7 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind // We do not allow ActivateBestChain() to run while InvalidateBlock() is // running, as that could cause the tip to change while we disconnect // blocks. - LOCK(m_cs_chainstate); + LOCK(m_chainstate_mutex); // We'll be acquiring and releasing cs_main below, to allow the validation // callbacks to run. However, we should keep the block index in a @@ -3223,9 +3227,10 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind void CChainState::EnforceBlock(BlockValidationState& state, const CBlockIndex *pindex) { + AssertLockNotHeld(m_chainstate_mutex); AssertLockNotHeld(::cs_main); - LOCK2(m_cs_chainstate, ::cs_main); + LOCK2(m_chainstate_mutex, ::cs_main); const CBlockIndex* pindex_walk = pindex; diff --git a/src/validation.h b/src/validation.h index e2aa77ffe77f3..fef6bfe62d501 100644 --- a/src/validation.h +++ b/src/validation.h @@ -468,10 +468,11 @@ class CChainState arith_uint256 nLastPreciousChainwork = 0; /** - * the ChainState CriticalSection - * A lock that must be held when modifying this ChainState - held in ActivateBestChain() + * The ChainState Mutex + * A lock that must be held when modifying this ChainState - held in ActivateBestChain() and + * InvalidateBlock() */ - RecursiveMutex m_cs_chainstate; + Mutex m_chainstate_mutex; /** * Whether this chainstate is undergoing initial block download. @@ -643,7 +644,7 @@ class CChainState */ bool ActivateBestChain( BlockValidationState& state, - std::shared_ptr pblock = nullptr) LOCKS_EXCLUDED(cs_main); + std::shared_ptr pblock = nullptr) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); bool AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -661,9 +662,9 @@ class CChainState */ bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); /** Mark a block as invalid. */ - bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); + bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); /** Enforce a block marking all the other chains as conflicting. */ - void EnforceBlock(BlockValidationState& state, const CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); + void EnforceBlock(BlockValidationState& state, const CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); /** Remove invalidity status from a block and its descendants. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From 678e67c5053751827879d7c05edca36e8bf09cb6 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 2 Feb 2022 09:08:00 +0100 Subject: [PATCH 03/12] merge bitcoin#24235: use stronger EXCLUSIVE_LOCKS_REQUIRED() --- src/validation.h | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/validation.h b/src/validation.h index fef6bfe62d501..3c886bcb63cf0 100644 --- a/src/validation.h +++ b/src/validation.h @@ -602,7 +602,8 @@ class CChainState EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Import blocks from an external file */ - void LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp = nullptr); + void LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex); /** * Update the on-disk chain state. @@ -644,7 +645,9 @@ class CChainState */ bool ActivateBestChain( BlockValidationState& state, - std::shared_ptr pblock = nullptr) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); + std::shared_ptr pblock = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); bool AcceptBlock(const std::shared_ptr& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -660,11 +663,20 @@ class CChainState * * May not be called in a validationinterface callback. */ - bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(cs_main); + bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + /** Mark a block as invalid. */ - bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); + bool InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + /** Enforce a block marking all the other chains as conflicting. */ - void EnforceBlock(BlockValidationState& state, const CBlockIndex* pindex) LOCKS_EXCLUDED(m_chainstate_mutex, cs_main); + void EnforceBlock(BlockValidationState& state, const CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + /** Remove invalidity status from a block and its descendants. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); From 18aa55be1ee049ced84c3d85d6b70bc7b5659046 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 4 Jul 2024 18:05:22 +0000 Subject: [PATCH 04/12] merge bitcoin#24177: add missing thread safety lock assertions --- src/test/util/setup_common.cpp | 20 ++++++++----- src/validation.cpp | 53 +++++++++++++++++++++++++++++----- src/validation.h | 14 ++++++--- 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 3e8fa5521c82a..a20143f012fc7 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -269,14 +269,18 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vectorInitializeChainstate(m_node.mempool.get(), *m_node.evodb, m_node.chain_helper, llmq::chainLocksHandler, llmq::quorumInstantSendManager); - m_node.chainman->ActiveChainstate().InitCoinsDB( - /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false); - assert(!m_node.chainman->ActiveChainstate().CanFlushToDisk()); - m_node.chainman->ActiveChainstate().InitCoinsCache(1 << 23); - assert(m_node.chainman->ActiveChainstate().CanFlushToDisk()); - if (!m_node.chainman->ActiveChainstate().LoadGenesisBlock()) { - throw std::runtime_error("LoadGenesisBlock failed."); + { + LOCK(::cs_main); + + m_node.chainman->InitializeChainstate(m_node.mempool.get(), *m_node.evodb, m_node.chain_helper, llmq::chainLocksHandler, llmq::quorumInstantSendManager); + m_node.chainman->ActiveChainstate().InitCoinsDB( + /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false); + assert(!m_node.chainman->ActiveChainstate().CanFlushToDisk()); + m_node.chainman->ActiveChainstate().InitCoinsCache(1 << 23); + assert(m_node.chainman->ActiveChainstate().CanFlushToDisk()); + if (!m_node.chainman->ActiveChainstate().LoadGenesisBlock()) { + throw std::runtime_error("LoadGenesisBlock failed."); + } } m_node.banman = std::make_unique(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); diff --git a/src/validation.cpp b/src/validation.cpp index c7c76d9f9e930..bef1efee5ea68 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -324,8 +324,10 @@ static bool ContextualCheckTransaction(const CTransaction& tx, TxValidationState static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams); static void LimitMempoolSize(CTxMemPool& pool, CCoinsViewCache& coins_cache, size_t limit, std::chrono::seconds age) - EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs) { + AssertLockHeld(::cs_main); + AssertLockHeld(pool.cs); int expired = pool.Expire(GetTime() - age); if (expired != 0) { LogPrint(BCLog::MEMPOOL, "Expired %i transactions from the memory pool\n", expired); @@ -520,8 +522,10 @@ class MemPoolAccept bool Finalize(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Compare a package's feerate against minimum allowed. - bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs) + bool CheckFeeRate(size_t package_size, CAmount package_fee, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) { + AssertLockHeld(::cs_main); + AssertLockHeld(m_pool.cs); CAmount mempoolRejectFee = m_pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size); if (mempoolRejectFee > 0 && package_fee < mempoolRejectFee) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool min fee not met", strprintf("%d < %d", package_fee, mempoolRejectFee)); @@ -552,6 +556,8 @@ class MemPoolAccept bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) { + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); const CTransactionRef& ptx = ws.m_ptx; const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; @@ -756,6 +762,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) { + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); const CTransaction& tx = *ws.m_ptx; TxValidationState& state = ws.m_state; @@ -772,6 +780,8 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, Prec bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) { + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; TxValidationState& state = ws.m_state; @@ -803,6 +813,8 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, P bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) { + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; TxValidationState& state = ws.m_state; @@ -941,8 +953,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef &tx, int64_t nAcceptTime, bool bypass_limits, bool test_accept) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); std::vector coins_to_uncache; MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept }; @@ -1177,6 +1190,7 @@ CoinsViews::CoinsViews( void CoinsViews::InitCache() { + AssertLockHeld(::cs_main); m_cacheview = std::make_unique(&m_catcherview); } @@ -1214,6 +1228,7 @@ void CChainState::InitCoinsDB( void CChainState::InitCoinsCache(size_t cache_size_bytes) { + AssertLockHeld(::cs_main); assert(m_coins_views != nullptr); m_coinstip_cache_size_bytes = cache_size_bytes; m_coins_views->InitCache(); @@ -1287,6 +1302,7 @@ void CChainState::CheckForkWarningConditions() // Called both upon regular invalid block discovery *and* InvalidateBlock void CChainState::InvalidChainFound(CBlockIndex* pindexNew) { + AssertLockHeld(cs_main); statsClient.inc("warnings.InvalidChainFound", 1.0f); @@ -1310,6 +1326,7 @@ void CChainState::InvalidChainFound(CBlockIndex* pindexNew) void CChainState::ConflictingChainFound(CBlockIndex* pindexNew) { + AssertLockHeld(cs_main); statsClient.inc("warnings.ConflictingChainFound", 1.0f); @@ -1328,6 +1345,8 @@ void CChainState::ConflictingChainFound(CBlockIndex* pindexNew) // which does its own setBlockIndexCandidates manageent. void CChainState::InvalidBlockFound(CBlockIndex *pindex, const BlockValidationState &state) { + AssertLockHeld(cs_main); + statsClient.inc("warnings.InvalidBlockFound", 1.0f); if (state.GetResult() != BlockValidationResult::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; @@ -2275,6 +2294,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, CoinsCacheSizeState CChainState::GetCoinsCacheSizeState() { + AssertLockHeld(::cs_main); return this->GetCoinsCacheSizeState( m_coinstip_cache_size_bytes, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); @@ -2284,6 +2304,7 @@ CoinsCacheSizeState CChainState::GetCoinsCacheSizeState( size_t max_coins_cache_size_bytes, size_t max_mempool_size_bytes) { + AssertLockHeld(::cs_main); const int64_t nMempoolUsage = m_mempool ? m_mempool->DynamicMemoryUsage() : 0; int64_t cacheSize = CoinsTip().DynamicMemoryUsage(); int64_t nTotalSpace = @@ -2498,6 +2519,7 @@ static void UpdateTipLog( void CChainState::UpdateTip(const CBlockIndex* pindexNew) { + AssertLockHeld(::cs_main); const auto& coins_tip = this->CoinsTip(); // The remainder of the function isn't relevant if we are not acting on @@ -2731,7 +2753,9 @@ bool CChainState::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew * Return the tip of the chain with the most work in it, that isn't * known to be invalid (it's however far from certain to be valid). */ -CBlockIndex* CChainState::FindMostWorkChain() { +CBlockIndex* CChainState::FindMostWorkChain() +{ + AssertLockHeld(::cs_main); do { CBlockIndex *pindexNew = nullptr; @@ -2940,7 +2964,7 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr // far from a guarantee. Things in the P2P/RPC will often end up calling // us in the middle of ProcessNewBlock - do not assume pblock is set // sanely for performance or correctness! - AssertLockNotHeld(cs_main); + AssertLockNotHeld(::cs_main); // ABC maintains a fair degree of expensive-to-calculate internal state // because this function periodically releases cs_main so that it does not lock up other threads for too long @@ -3043,6 +3067,8 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) { + AssertLockNotHeld(m_chainstate_mutex); + AssertLockNotHeld(::cs_main); { LOCK(cs_main); if (pindex->nChainWork < m_chain.Tip()->nChainWork) { @@ -3073,6 +3099,7 @@ bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) { AssertLockNotHeld(m_chainstate_mutex); + AssertLockNotHeld(::cs_main); // Genesis block can't be invalidated assert(pindex); @@ -3389,6 +3416,7 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) { /** Mark a block as having its data received and checked (up to BLOCK_VALID_TRANSACTIONS). */ void CChainState::ReceivedBlockTransactions(const CBlock& block, CBlockIndex* pindexNew, const FlatFilePos& pos) { + AssertLockHeld(cs_main); pindexNew->nTx = block.vtx.size(); pindexNew->nChainTx = 0; pindexNew->nFile = pos.nFile; @@ -3529,8 +3557,9 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu * in ConnectBlock(). * Note that -reindex-chainstate skips the validation that happens here! */ -static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidationState& state, BlockManager& blockman, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidationState& state, BlockManager& blockman, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); assert(pindexPrev != nullptr); const int nHeight = pindexPrev->nHeight + 1; @@ -4153,6 +4182,8 @@ bool CVerifyDB::VerifyDB( /** Apply the effects of a block on the utxo cache, ignoring that it may already have been applied. */ bool CChainState::RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs) { + AssertLockHeld(cs_main); + assert(m_chain_helper); // TODO: merge with ConnectBlock @@ -4332,7 +4363,9 @@ bool CChainState::ReplayBlocks() return true; } -void CChainState::UnloadBlockIndex() { +void CChainState::UnloadBlockIndex() +{ + AssertLockHeld(::cs_main); nBlockSequenceId = 1; setBlockIndexCandidates.clear(); } @@ -4500,6 +4533,7 @@ bool CChainState::LoadGenesisBlock() void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) { + AssertLockNotHeld(m_chainstate_mutex); // Map of disk positions for blocks with unknown parent (only used for reindex) static std::multimap mapBlocksUnknownParent; int64_t nStart = GetTimeMillis(); @@ -4848,6 +4882,7 @@ void CChainState::CheckBlockIndex() std::string CChainState::ToString() { + AssertLockHeld(::cs_main); CBlockIndex* tip = m_chain.Tip(); return strprintf("Chainstate [%s] @ height %d (%s)", m_from_snapshot_blockhash ? "snapshot" : "ibd", @@ -4856,6 +4891,7 @@ std::string CChainState::ToString() bool CChainState::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size) { + AssertLockHeld(::cs_main); if (coinstip_size == m_coinstip_cache_size_bytes && coinsdb_size == m_coinsdb_cache_size_bytes) { // Cache sizes are unchanged, no need to continue. @@ -5085,6 +5121,7 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool* mempool, const std::unique_ptr& isman, const std::optional& snapshot_blockhash) { + AssertLockHeld(::cs_main); bool is_snapshot = snapshot_blockhash.has_value(); std::unique_ptr& to_modify = is_snapshot ? m_snapshot_chainstate : m_ibd_chainstate; @@ -5419,6 +5456,7 @@ bool ChainstateManager::IsSnapshotActive() const void ChainstateManager::Unload() { + AssertLockHeld(::cs_main); for (CChainState* chainstate : this->GetAll()) { chainstate->m_chain.SetTip(nullptr); chainstate->UnloadBlockIndex(); @@ -5441,6 +5479,7 @@ void ChainstateManager::Reset() void ChainstateManager::MaybeRebalanceCaches() { + AssertLockHeld(::cs_main); if (m_ibd_chainstate && !m_snapshot_chainstate) { LogPrintf("[snapshot] allocating all cache to the IBD chainstate\n"); // Allocate everything to the IBD chainstate. diff --git a/src/validation.h b/src/validation.h index 3c886bcb63cf0..b1bb8ed3d3693 100644 --- a/src/validation.h +++ b/src/validation.h @@ -534,7 +534,9 @@ class CChainState //! @returns whether or not the CoinsViews object has been fully initialized and we can //! safely flush this object to disk. - bool CanFlushToDisk() const EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + bool CanFlushToDisk() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) + { + AssertLockHeld(::cs_main); return m_coins_views && m_coins_views->m_cacheview; } @@ -568,22 +570,25 @@ class CChainState } //! @returns A reference to the in-memory cache of the UTXO set. - CCoinsViewCache& CoinsTip() EXCLUSIVE_LOCKS_REQUIRED(cs_main) + CCoinsViewCache& CoinsTip() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); assert(m_coins_views->m_cacheview); return *m_coins_views->m_cacheview.get(); } //! @returns A reference to the on-disk UTXO set database. - CCoinsViewDB& CoinsDB() EXCLUSIVE_LOCKS_REQUIRED(cs_main) + CCoinsViewDB& CoinsDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); return m_coins_views->m_dbview; } //! @returns A reference to a wrapped view of the in-memory UTXO set that //! handles disk read errors gracefully. - CCoinsViewErrorCatcher& CoinsErrorCatcher() EXCLUSIVE_LOCKS_REQUIRED(cs_main) + CCoinsViewErrorCatcher& CoinsErrorCatcher() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); return m_coins_views->m_catcherview; } @@ -939,6 +944,7 @@ class ChainstateManager BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { + AssertLockHeld(::cs_main); return m_blockman.m_block_index; } From e10ca27fa6da3f5dcb3f134a9fa4e6058a564aca Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 4 Jul 2024 14:19:21 +0000 Subject: [PATCH 05/12] merge bitcoin#24299: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups --- src/init.cpp | 4 ++-- src/test/util/setup_common.cpp | 3 +-- src/validation.cpp | 11 +---------- src/validation.h | 8 ++------ 4 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 04b4876f711f0..628539d2dfd89 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1835,13 +1835,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) try { LOCK(cs_main); + node.evodb.reset(); node.evodb = std::make_unique(nEvoDbCache, false, fReset || fReindexChainState); + node.mnhf_manager.reset(); node.mnhf_manager = std::make_unique(*node.evodb); - - chainman.Reset(); chainman.InitializeChainstate(Assert(node.mempool.get()), *node.evodb, node.chain_helper, llmq::chainLocksHandler, llmq::quorumInstantSendManager); chainman.m_total_coinstip_cache = nCoinCacheUsage; chainman.m_total_coinsdb_cache = nCoinDBCache; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index a20143f012fc7..04d24d64ff7b6 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -254,10 +254,9 @@ ChainTestingSetup::~ChainTestingSetup() m_node.connman.reset(); m_node.addrman.reset(); m_node.args = nullptr; - UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman); + WITH_LOCK(::cs_main, UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman)); m_node.mempool.reset(); m_node.scheduler.reset(); - m_node.chainman->Reset(); m_node.chainman.reset(); } diff --git a/src/validation.cpp b/src/validation.cpp index bef1efee5ea68..76dd1c8253b86 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4375,7 +4375,7 @@ void CChainState::UnloadBlockIndex() // block index state void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) { - LOCK(cs_main); + AssertLockHeld(::cs_main); chainman.Unload(); if (mempool) mempool->clear(); g_versionbitscache.Clear(); @@ -5468,15 +5468,6 @@ void ChainstateManager::Unload() m_best_invalid = nullptr; } -void ChainstateManager::Reset() -{ - LOCK(::cs_main); - m_ibd_chainstate.reset(); - m_snapshot_chainstate.reset(); - m_active_chainstate = nullptr; - m_snapshot_validated = false; -} - void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index b1bb8ed3d3693..a0e5cdbc1fcc5 100644 --- a/src/validation.h +++ b/src/validation.h @@ -152,7 +152,7 @@ extern arith_uint256 nMinimumChainWork; extern const std::vector CHECKLEVEL_DOC; /** Unload database information */ -void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman); +void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Run instances of script checking worker threads */ void StartScriptCheckWorkerThreads(int threads_num); /** Stop all of the script checking worker threads */ @@ -1003,17 +1003,13 @@ class ChainstateManager //! Unload block index and chain data before shutdown. void Unload() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Clear (deconstruct) chainstate data. - void Reset(); - //! Check to see if caches are out of balance and if so, call //! ResizeCoinsCaches() as needed. void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); ~ChainstateManager() { LOCK(::cs_main); - UnloadBlockIndex(/* mempool */ nullptr, *this); - Reset(); + UnloadBlockIndex(/*mempool=*/nullptr, *this); } }; From 916b3f00414ff96e32d241e321889ace6ab6b5c8 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 4 Jul 2024 12:39:00 +0000 Subject: [PATCH 06/12] merge bitcoin#24917: Make BlockManager::LoadBlockIndex private --- src/node/blockstorage.h | 15 +++++++-------- src/validation.h | 1 - 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 7e5087a6b2eca..5b1f1649a92b8 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -90,6 +90,13 @@ class BlockManager friend ChainstateManager; private: + /** + * Load the blocktree off disk and into memory. Populate certain metadata + * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral + * collections like m_dirty_blockindex. + */ + bool LoadBlockIndex(const Consensus::Params& consensus_params) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); void FlushBlockFile(bool fFinalize = false, bool finalize_undo = false); void FlushUndoFile(int block_file, bool finalize = false); bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown); @@ -147,14 +154,6 @@ class BlockManager bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** - * Load the blocktree off disk and into memory. Populate certain metadata - * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral - * collections like m_dirty_blockindex. - */ - bool LoadBlockIndex(const Consensus::Params& consensus_params) - EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** Clear all data members. */ void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/validation.h b/src/validation.h index a0e5cdbc1fcc5..b39e6f382b16c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -845,7 +845,6 @@ class ChainstateManager bool m_snapshot_validated{false}; CBlockIndex* m_best_invalid; - friend bool BlockManager::LoadBlockIndex(const Consensus::Params&); //! Internal helper for ActivateSnapshot(). [[nodiscard]] bool PopulateAndValidateSnapshot( From eca0a64ea10359756508a7ed02bfaca69aca5e64 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 23 Aug 2024 06:52:46 +0000 Subject: [PATCH 07/12] merge bitcoin#22564: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager Co-authored-by: UdjinM6 --- src/init.cpp | 75 ++++++++++--------- src/node/blockstorage.cpp | 15 ---- src/node/blockstorage.h | 8 -- src/test/util/setup_common.cpp | 1 - src/test/validation_chainstate_tests.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 4 +- src/validation.cpp | 44 ++++------- src/validation.h | 16 +--- 8 files changed, 59 insertions(+), 106 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 628539d2dfd89..88e981898acca 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -105,6 +105,7 @@ #include +#include #include #include #include @@ -1567,31 +1568,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // as they would never get updated. if (!ignores_incoming_txs) node.fee_estimator = std::make_unique(); - assert(!node.mempool); - int check_ratio = std::min(std::max(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); - node.mempool = std::make_unique(node.fee_estimator.get(), check_ratio); - - assert(!node.chainman); - node.chainman = std::make_unique(); - ChainstateManager& chainman = *node.chainman; - assert(!node.mn_metaman); node.mn_metaman = std::make_unique(); assert(!node.netfulfilledman); node.netfulfilledman = std::make_unique(); - /** - * The manager needs to be constructed regardless of whether governance - * validation is needed or not. - * - * Instead, we decide whether to initialize its database based on whether we - * need it or not further down and then query if the database is initialized - * to check if validation is enabled. - */ const bool is_governance_enabled{!args.GetBoolArg("-disablegovernance", !DEFAULT_GOVERNANCE_ENABLE)}; - assert(!node.govman); - node.govman = std::make_unique(*node.mn_metaman, *node.netfulfilledman, *node.chainman, node.dmnman, node.mn_sync); assert(!node.sporkman); node.sporkman = std::make_unique(); @@ -1620,9 +1603,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - assert(!node.mn_sync); - node.mn_sync = std::make_unique(*node.connman, *node.netfulfilledman, *node.govman); - fMasternodeMode = false; std::string strMasterNodeBLSPrivKey = args.GetArg("-masternodeblsprivkey", ""); if (!strMasterNodeBLSPrivKey.empty()) { @@ -1638,13 +1618,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } } - assert(!node.peerman); - node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(), - chainman, *node.mempool, *node.mn_metaman, *node.mn_sync, - *node.govman, *node.sporkman, node.mn_activeman.get(), node.dmnman, - node.cj_ctx, node.llmq_ctx, ignores_incoming_txs); - RegisterValidationInterface(node.peerman.get()); - // sanitize comments per BIP-0014, format user agent and check total size std::vector uacomments; @@ -1768,11 +1741,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } #endif - pdsNotificationInterface = new CDSNotificationInterface( - *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx - ); - RegisterValidationInterface(pdsNotificationInterface); - // ********************************************************* Step 7a: Load sporks if (!node.sporkman->LoadCache()) { @@ -1818,9 +1786,30 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) LogPrintf("* Using %.1f MiB for chain state database\n", nCoinDBCache * (1.0 / 1024 / 1024)); LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", nCoinCacheUsage * (1.0 / 1024 / 1024), nMempoolSizeMax * (1.0 / 1024 / 1024)); - bool fLoaded = false; + assert(!node.mempool); + assert(!node.chainman); + assert(!node.govman); + assert(!node.mn_sync); + const int mempool_check_ratio = std::clamp(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0, 1000000); + + for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) { + node.mempool = std::make_unique(node.fee_estimator.get(), mempool_check_ratio); + + node.chainman = std::make_unique(); + ChainstateManager& chainman = *node.chainman; + + /** + * The manager needs to be constructed regardless of whether governance + * validation is needed or not. + * + * Instead, we decide whether to initialize its database based on whether we + * need it or not further down and then query if the database is initialized + * to check if validation is enabled. + */ + node.govman = std::make_unique(*node.mn_metaman, *node.netfulfilledman, *node.chainman, node.dmnman, node.mn_sync); + + node.mn_sync = std::make_unique(*node.connman, *node.netfulfilledman, *node.govman); - while (!fLoaded && !ShutdownRequested()) { const bool fReset = fReindex; auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull(); @@ -1846,8 +1835,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) chainman.m_total_coinstip_cache = nCoinCacheUsage; chainman.m_total_coinsdb_cache = nCoinDBCache; - UnloadBlockIndex(node.mempool.get(), chainman); - auto& pblocktree{chainman.m_blockman.m_block_tree_db}; // new CBlockTreeDB tries to delete the existing file, which // fails if it's still open from the previous loop. Close it first: @@ -2116,6 +2103,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return false; } + ChainstateManager& chainman = *Assert(node.chainman); + + assert(!node.peerman); + node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(), + chainman, *node.mempool, *node.mn_metaman, *node.mn_sync, + *node.govman, *node.sporkman, node.mn_activeman.get(), node.dmnman, + node.cj_ctx, node.llmq_ctx, ignores_incoming_txs); + RegisterValidationInterface(node.peerman.get()); + + pdsNotificationInterface = new CDSNotificationInterface( + *node.connman, *node.mn_sync, *node.govman, *node.peerman, chainman, node.mn_activeman.get(), node.dmnman, node.llmq_ctx, node.cj_ctx + ); + RegisterValidationInterface(pdsNotificationInterface); + // ********************************************************* Step 7c: Setup CoinJoin node.cj_ctx = std::make_unique(chainman.ActiveChainstate(), *node.connman, *node.dmnman, *node.mn_metaman, *node.mempool, diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 3ae23ef9bfc29..f55d041073af6 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -314,21 +314,6 @@ bool BlockManager::LoadBlockIndex(const Consensus::Params& consensus_params) return true; } -void BlockManager::Unload() -{ - m_blocks_unlinked.clear(); - - m_block_index.clear(); - m_prev_block_index.clear(); - - m_blockfile_info.clear(); - m_last_blockfile = 0; - m_dirty_blockindex.clear(); - m_dirty_fileinfo.clear(); - - m_have_pruned = false; -} - bool BlockManager::WriteBlockIndexDB() { AssertLockHeld(::cs_main); diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 5b1f1649a92b8..a6dfe7bccd1f9 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -154,9 +154,6 @@ class BlockManager bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** Clear all data members. */ - void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main); - CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const uint256& hash, CBlockIndex*& best_header, enum BlockStatus nStatus = BLOCK_VALID_TREE) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -195,11 +192,6 @@ class BlockManager * This is also true for mempool checks. */ int GetSpendHeight(const CCoinsViewCache& inputs) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - - ~BlockManager() - { - Unload(); - } }; void CleanupBlockRevFiles(); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 04d24d64ff7b6..7bff856ff1d86 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -254,7 +254,6 @@ ChainTestingSetup::~ChainTestingSetup() m_node.connman.reset(); m_node.addrman.reset(); m_node.args = nullptr; - WITH_LOCK(::cs_main, UnloadBlockIndex(m_node.mempool.get(), *m_node.chainman)); m_node.mempool.reset(); m_node.scheduler.reset(); m_node.chainman.reset(); diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index a64b2c092fad4..860124161a0da 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -97,7 +97,7 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root)); // Ensure our active chain is the snapshot chainstate. - BOOST_CHECK(chainman.IsSnapshotActive()); + BOOST_CHECK(WITH_LOCK(::cs_main, return chainman.IsSnapshotActive())); curr_tip = ::g_best_block; diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index d3374dcdc636a..735365ef8962f 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) DashTestSetup(m_node, chainparams); BOOST_CHECK(!manager.IsSnapshotActive()); - BOOST_CHECK(!manager.IsSnapshotValidated()); + BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); auto all = manager.GetAll(); BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end()); @@ -91,7 +91,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager) BOOST_CHECK(c2.ActivateBestChain(dummy_state, nullptr)); BOOST_CHECK(manager.IsSnapshotActive()); - BOOST_CHECK(!manager.IsSnapshotValidated()); + BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated())); BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate()); BOOST_CHECK(&c1 != &manager.ActiveChainstate()); auto all2 = manager.GetAll(); diff --git a/src/validation.cpp b/src/validation.cpp index 76dd1c8253b86..16739b8413591 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1769,7 +1769,7 @@ class WarningBitsConditionChecker : public AbstractThresholdConditionChecker } }; -static ThresholdConditionCache warningcache[VERSIONBITS_NUM_BITS] GUARDED_BY(cs_main); +static std::array warningcache GUARDED_BY(cs_main); static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& consensusparams) { @@ -2550,7 +2550,7 @@ void CChainState::UpdateTip(const CBlockIndex* pindexNew) const CBlockIndex* pindex = pindexNew; for (int bit = 0; bit < VERSIONBITS_NUM_BITS; bit++) { WarningBitsConditionChecker checker(bit); - ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache[bit]); + ThresholdState state = checker.GetStateFor(pindex, m_params.GetConsensus(), warningcache.at(bit)); if (state == ThresholdState::ACTIVE || state == ThresholdState::LOCKED_IN) { const bilingual_str warning = strprintf(_("Unknown new rules activated (versionbit %i)"), bit); if (state == ThresholdState::ACTIVE) { @@ -4370,20 +4370,6 @@ void CChainState::UnloadBlockIndex() setBlockIndexCandidates.clear(); } -// May NOT be used after any connections are up as much -// of the peer-processing logic assumes a consistent -// block index state -void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) -{ - AssertLockHeld(::cs_main); - chainman.Unload(); - if (mempool) mempool->clear(); - g_versionbitscache.Clear(); - for (int b = 0; b < VERSIONBITS_NUM_BITS; b++) { - warningcache[b].clear(); - } -} - bool ChainstateManager::LoadBlockIndex() { AssertLockHeld(cs_main); @@ -5454,20 +5440,6 @@ bool ChainstateManager::IsSnapshotActive() const return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get(); } -void ChainstateManager::Unload() -{ - AssertLockHeld(::cs_main); - for (CChainState* chainstate : this->GetAll()) { - chainstate->m_chain.SetTip(nullptr); - chainstate->UnloadBlockIndex(); - } - - m_failed_blocks.clear(); - m_blockman.Unload(); - m_best_header = nullptr; - m_best_invalid = nullptr; -} - void ChainstateManager::MaybeRebalanceCaches() { AssertLockHeld(::cs_main); @@ -5498,3 +5470,15 @@ void ChainstateManager::MaybeRebalanceCaches() } } } + +ChainstateManager::~ChainstateManager() +{ + LOCK(::cs_main); + + // TODO: The version bits cache and warning cache should probably become + // non-globals + g_versionbitscache.Clear(); + for (auto& i : warningcache) { + i.clear(); + } +} diff --git a/src/validation.h b/src/validation.h index b39e6f382b16c..b0d5c6b8dff0d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -151,8 +151,6 @@ extern arith_uint256 nMinimumChainWork; /** Documentation for argument 'checklevel'. */ extern const std::vector CHECKLEVEL_DOC; -/** Unload database information */ -void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Run instances of script checking worker threads */ void StartScriptCheckWorkerThreads(int threads_num); /** Stop all of the script checking worker threads */ @@ -842,9 +840,9 @@ class ChainstateManager //! If true, the assumed-valid chainstate has been fully validated //! by the background validation chainstate. - bool m_snapshot_validated{false}; + bool m_snapshot_validated GUARDED_BY(::cs_main){false}; - CBlockIndex* m_best_invalid; + CBlockIndex* m_best_invalid GUARDED_BY(::cs_main){nullptr}; //! Internal helper for ActivateSnapshot(). [[nodiscard]] bool PopulateAndValidateSnapshot( @@ -959,7 +957,7 @@ class ChainstateManager std::optional SnapshotBlockhash() const; //! Is there a snapshot in use and has it been fully validated? - bool IsSnapshotValidated() const { return m_snapshot_validated; } + bool IsSnapshotValidated() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { return m_snapshot_validated; } /** * Process an incoming block. This only returns after the best known valid @@ -999,17 +997,11 @@ class ChainstateManager //! Load the block tree and coins database from disk, initializing state if we're running with -reindex bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main); - //! Unload block index and chain data before shutdown. - void Unload() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - //! Check to see if caches are out of balance and if so, call //! ResizeCoinsCaches() as needed. void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - ~ChainstateManager() { - LOCK(::cs_main); - UnloadBlockIndex(/*mempool=*/nullptr, *this); - } + ~ChainstateManager(); }; /** From 70a91e1d3fcf2953f0cdfa2d324ee9df6c5f9c53 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 16 Jul 2022 13:22:46 +0200 Subject: [PATCH 08/12] merge bitcoin#25349: CBlockIndex/CDiskBlockIndex improvements for safety, consistent behavior --- src/chain.cpp | 14 +------------- src/chain.h | 7 ++++--- src/test/fuzz/chain.cpp | 3 +-- src/txdb.cpp | 2 +- src/validation.cpp | 1 - 5 files changed, 7 insertions(+), 20 deletions(-) diff --git a/src/chain.cpp b/src/chain.cpp index a6263eb1e86d7..7c36457f02386 100644 --- a/src/chain.cpp +++ b/src/chain.cpp @@ -7,22 +7,10 @@ #include -std::string CDiskBlockIndex::ToString() const -{ - std::string str = "CDiskBlockIndex("; - str += CBlockIndex::ToString(); - str += strprintf("\n hashBlock=%s, hashPrev=%s)", - GetBlockHash().ToString(), - hashPrev.ToString()); - return str; -} - std::string CBlockIndex::ToString() const { return strprintf("CBlockIndex(pprev=%p, nHeight=%d, merkle=%s, hashBlock=%s)", - pprev, nHeight, - hashMerkleRoot.ToString(), - GetBlockHash().ToString()); + pprev, nHeight, hashMerkleRoot.ToString(), GetBlockHash().ToString()); } /** diff --git a/src/chain.h b/src/chain.h index 810d68261a474..b3622121de90f 100644 --- a/src/chain.h +++ b/src/chain.h @@ -257,6 +257,7 @@ class CBlockIndex uint256 GetBlockHash() const { + assert(phashBlock != nullptr); return *phashBlock; } @@ -393,7 +394,7 @@ class CDiskBlockIndex : public CBlockIndex READWRITE(obj.nNonce); } - uint256 GetBlockHash() const + uint256 ConstructBlockHash() const { if(hash != uint256()) return hash; // should never really get here, keeping this as a fallback @@ -407,8 +408,8 @@ class CDiskBlockIndex : public CBlockIndex return block.GetHash(); } - std::string ToString() const; - + uint256 GetBlockHash() = delete; + std::string ToString() = delete; }; /** An in-memory indexed chain of blocks. */ diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 1e8d71adeaba2..6982b562d6f76 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -23,7 +23,7 @@ FUZZ_TARGET(chain) disk_block_index->phashBlock = &zero; { LOCK(::cs_main); - (void)disk_block_index->GetBlockHash(); + (void)disk_block_index->ConstructBlockHash(); (void)disk_block_index->GetBlockPos(); (void)disk_block_index->GetBlockTime(); (void)disk_block_index->GetBlockTimeMax(); @@ -31,7 +31,6 @@ FUZZ_TARGET(chain) (void)disk_block_index->GetUndoPos(); (void)disk_block_index->HaveTxsDownloaded(); (void)disk_block_index->IsValid(); - (void)disk_block_index->ToString(); } const CBlockHeader block_header = disk_block_index->GetBlockHeader(); diff --git a/src/txdb.cpp b/src/txdb.cpp index bdaeda42c90a5..329f6b3bddca2 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -420,7 +420,7 @@ bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, CDiskBlockIndex diskindex; if (pcursor->GetValue(diskindex)) { // Construct block index object - CBlockIndex* pindexNew = insertBlockIndex(diskindex.GetBlockHash()); + CBlockIndex* pindexNew = insertBlockIndex(diskindex.ConstructBlockHash()); pindexNew->pprev = insertBlockIndex(diskindex.hashPrev); pindexNew->nHeight = diskindex.nHeight; pindexNew->nFile = diskindex.nFile; diff --git a/src/validation.cpp b/src/validation.cpp index 16739b8413591..5f39722ec5d76 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2268,7 +2268,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, int64_t nTime7 = GetTimeMicros(); nTimeIndexWrite += nTime7 - nTime6; LogPrint(BCLog::BENCHMARK, " - Index writing: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime7 - nTime6), nTimeIndexWrite * MICRO, nTimeIndexWrite * MILLI / nBlocksTotal); - assert(pindex->phashBlock); // add this block to the view's block chain view.SetBestBlock(pindex->GetBlockHash()); m_evoDb.WriteBestBlock(pindex->GetBlockHash()); From 2c758f4ba2535d6fc53ee2569aaf2dc118e9aa3e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 26 Jul 2020 23:43:01 +0300 Subject: [PATCH 09/12] merge bitcoin#25571: Make mapBlocksUnknownParent local, and rename it --- src/node/blockstorage.cpp | 7 ++++- src/test/fuzz/load_external_block_file.cpp | 11 ++++++-- src/validation.cpp | 23 +++++++++++----- src/validation.h | 32 ++++++++++++++++++++-- 4 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f55d041073af6..d8e59b88ab0db 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -23,6 +23,8 @@ #include #include +#include + std::atomic_bool fImporting(false); std::atomic_bool fReindex(false); bool fPruneMode = false; @@ -818,6 +820,9 @@ void ThreadImport(ChainstateManager& chainman, CDeterministicMNManager& dmnman, // -reindex if (fReindex) { int nFile = 0; + // Map of disk positions for blocks with unknown parent (only used for reindex); + // parent hash -> child disk position, multiple children can have the same parent. + std::multimap blocks_with_unknown_parent; while (true) { FlatFilePos pos(nFile, 0); if (!fs::exists(GetBlockPosFilename(pos))) { @@ -828,7 +833,7 @@ void ThreadImport(ChainstateManager& chainman, CDeterministicMNManager& dmnman, break; // This error is logged in OpenBlockFile } LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile); - chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos); + chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); if (ShutdownRequested()) { LogPrintf("Shutdown requested. Exit %s\n", __func__); return; diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index 7a2795695db2d..16b916dc70043 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -31,6 +31,13 @@ FUZZ_TARGET_INIT(load_external_block_file, initialize_load_external_block_file) if (fuzzed_block_file == nullptr) { return; } - FlatFilePos flat_file_pos; - g_setup->m_node.chainman->ActiveChainstate().LoadExternalBlockFile(fuzzed_block_file, fuzzed_data_provider.ConsumeBool() ? &flat_file_pos : nullptr); + if (fuzzed_data_provider.ConsumeBool()) { + // Corresponds to the -reindex case (track orphan blocks across files). + FlatFilePos flat_file_pos; + std::multimap blocks_with_unknown_parent; + g_setup->m_node.chainman->ActiveChainstate().LoadExternalBlockFile(fuzzed_block_file, &flat_file_pos, &blocks_with_unknown_parent); + } else { + // Corresponds to the -loadblock= case (orphan blocks aren't tracked across files). + g_setup->m_node.chainman->ActiveChainstate().LoadExternalBlockFile(fuzzed_block_file); + } } diff --git a/src/validation.cpp b/src/validation.cpp index 5f39722ec5d76..b4f7c2c422ba1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -68,6 +68,7 @@ #include #include +#include #include #include #include @@ -4516,11 +4517,16 @@ bool CChainState::LoadGenesisBlock() return true; } -void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) +void CChainState::LoadExternalBlockFile( + FILE* fileIn, + FlatFilePos* dbp, + std::multimap* blocks_with_unknown_parent) { AssertLockNotHeld(m_chainstate_mutex); - // Map of disk positions for blocks with unknown parent (only used for reindex) - static std::multimap mapBlocksUnknownParent; + + // Either both should be specified (-reindex), or neither (-loadblock). + assert(!dbp == !blocks_with_unknown_parent); + int64_t nStart = GetTimeMillis(); int nLoaded = 0; @@ -4571,8 +4577,9 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) if (hash != m_params.GetConsensus().hashGenesisBlock && !m_blockman.LookupBlockIndex(block.hashPrevBlock)) { LogPrint(BCLog::REINDEX, "%s: Out of order block %s, parent %s not known\n", __func__, hash.ToString(), block.hashPrevBlock.ToString()); - if (dbp) - mapBlocksUnknownParent.insert(std::make_pair(block.hashPrevBlock, *dbp)); + if (dbp && blocks_with_unknown_parent) { + blocks_with_unknown_parent->emplace(block.hashPrevBlock, *dbp); + } continue; } @@ -4601,13 +4608,15 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) NotifyHeaderTip(*this); + if (!blocks_with_unknown_parent) continue; + // Recursively process earlier encountered successors of this block std::deque queue; queue.push_back(hash); while (!queue.empty()) { uint256 head = queue.front(); queue.pop_front(); - std::pair::iterator, std::multimap::iterator> range = mapBlocksUnknownParent.equal_range(head); + auto range = blocks_with_unknown_parent->equal_range(head); while (range.first != range.second) { std::multimap::iterator it = range.first; std::shared_ptr pblockrecursive = std::make_shared(); @@ -4622,7 +4631,7 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp) } } range.first++; - mapBlocksUnknownParent.erase(it); + blocks_with_unknown_parent->erase(it); NotifyHeaderTip(*this); } } diff --git a/src/validation.h b/src/validation.h index b0d5c6b8dff0d..3a103709528ad 100644 --- a/src/validation.h +++ b/src/validation.h @@ -604,8 +604,36 @@ class CChainState bool ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** Import blocks from an external file */ - void LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp = nullptr) + /** + * Import blocks from an external file + * + * During reindexing, this function is called for each block file (datadir/blocks/blk?????.dat). + * It reads all blocks contained in the given file and attempts to process them (add them to the + * block index). The blocks may be out of order within each file and across files. Often this + * function reads a block but finds that its parent hasn't been read yet, so the block can't be + * processed yet. The function will add an entry to the blocks_with_unknown_parent map (which is + * passed as an argument), so that when the block's parent is later read and processed, this + * function can re-read the child block from disk and process it. + * + * Because a block's parent may be in a later file, not just later in the same file, the + * blocks_with_unknown_parent map must be passed in and out with each call. It's a multimap, + * rather than just a map, because multiple blocks may have the same parent (when chain splits + * or stale blocks exist). It maps from parent-hash to child-disk-position. + * + * This function can also be used to read blocks from user-specified block files using the + * -loadblock= option. There's no unknown-parent tracking, so the last two arguments are omitted. + * + * + * @param[in] fileIn FILE handle to file containing blocks to read + * @param[in] dbp (optional) Disk block position (only for reindex) + * @param[in,out] blocks_with_unknown_parent (optional) Map of disk positions for blocks with + * unknown parent, key is parent block hash + * (only used for reindex) + * */ + void LoadExternalBlockFile( + FILE* fileIn, + FlatFilePos* dbp = nullptr, + std::multimap* blocks_with_unknown_parent = nullptr) EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex); /** From 7d837ea5c55db9ed441f7fcc31c0c0bbe79cf3d0 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 3 Dec 2019 13:25:35 -0500 Subject: [PATCH 10/12] merge bitcoin#17487: allow write to disk without cache drop --- src/coins.cpp | 47 ++++++-- src/coins.h | 20 +++- src/test/coins_tests.cpp | 242 +++++++++++++++++++++++++++++++++++++-- src/txdb.cpp | 5 +- src/txdb.h | 2 +- 5 files changed, 288 insertions(+), 28 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index e3db2ea540e37..3cb5ef1125e88 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -12,7 +12,7 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } -bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return false; } +bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; } std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const @@ -27,7 +27,7 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { return base->BatchWrite(mapCoins, hashBlock); } +bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); } std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } @@ -163,8 +163,10 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn) { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) { +bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) { + for (CCoinsMap::iterator it = mapCoins.begin(); + it != mapCoins.end(); + it = erase ? mapCoins.erase(it) : std::next(it)) { // Ignore non-dirty entries (optimization). if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) { continue; @@ -177,7 +179,14 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn // Create the coin in the parent cache, move the data up // and mark it as dirty. CCoinsCacheEntry& entry = cacheCoins[it->first]; - entry.coin = std::move(it->second.coin); + if (erase) { + // The `move` call here is purely an optimization; we rely on the + // `mapCoins.erase` call in the `for` expression to actually remove + // the entry from the child map. + entry.coin = std::move(it->second.coin); + } else { + entry.coin = it->second.coin; + } cachedCoinsUsage += entry.coin.DynamicMemoryUsage(); entry.flags = CCoinsCacheEntry::DIRTY; // We can mark it FRESH in the parent if it was FRESH in the child @@ -205,7 +214,14 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } else { // A normal modification. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); - itUs->second.coin = std::move(it->second.coin); + if (erase) { + // The `move` call here is purely an optimization; we rely on the + // `mapCoins.erase` call in the `for` expression to actually remove + // the entry from the child map. + itUs->second.coin = std::move(it->second.coin); + } else { + itUs->second.coin = it->second.coin; + } cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage(); itUs->second.flags |= CCoinsCacheEntry::DIRTY; // NOTE: It isn't safe to mark the coin as FRESH in the parent @@ -220,12 +236,29 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } bool CCoinsViewCache::Flush() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock); + bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ true); cacheCoins.clear(); cachedCoinsUsage = 0; return fOk; } +bool CCoinsViewCache::Sync() +{ + bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ false); + // Instead of clearing `cacheCoins` as we would in Flush(), just clear the + // FRESH/DIRTY flags of any coin that isn't spent. + for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) { + if (it->second.coin.IsSpent()) { + cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); + it = cacheCoins.erase(it); + } else { + it->second.flags = 0; + ++it; + } + } + return fOk; +} + void CCoinsViewCache::Uncache(const COutPoint& hash) { CCoinsMap::iterator it = cacheCoins.find(hash); diff --git a/src/coins.h b/src/coins.h index 3151a260d9467..acd5d2a4f5ff9 100644 --- a/src/coins.h +++ b/src/coins.h @@ -177,7 +177,7 @@ class CCoinsView //! Do a bulk modification (multiple Coin changes + BestBlock change). //! The passed mapCoins can be modified. - virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); + virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true); //! Get a cursor to iterate over the whole state virtual std::unique_ptr Cursor() const; @@ -203,7 +203,7 @@ class CCoinsViewBacked : public CCoinsView uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; + bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; std::unique_ptr Cursor() const override; size_t EstimateSize() const override; }; @@ -236,7 +236,7 @@ class CCoinsViewCache : public CCoinsViewBacked bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; + bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; std::unique_ptr Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } @@ -283,12 +283,22 @@ class CCoinsViewCache : public CCoinsViewBacked bool SpendCoin(const COutPoint &outpoint, Coin* moveto = nullptr); /** - * Push the modifications applied to this cache to its base. - * Failure to call this method before destruction will cause the changes to be forgotten. + * Push the modifications applied to this cache to its base and wipe local state. + * Failure to call this method or Sync() before destruction will cause the changes + * to be forgotten. * If false is returned, the state of this cache (and its backing view) will be undefined. */ bool Flush(); + /** + * Push the modifications applied to this cache to its base while retaining + * the contents of this cache (except for spent coins, which we erase). + * Failure to call this method or Flush() before destruction will cause the changes + * to be forgotten. + * If false is returned, the state of this cache (and its backing view) will be undefined. + */ + bool Sync(); + /** * Removes the UTXO with the given outpoint from the cache, if it is * not modified. diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 0a3f9391285f6..bf3519cee0aaf 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -53,9 +53,9 @@ class CCoinsViewTest : public CCoinsView uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock) override + bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) { + for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : ++it) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; @@ -64,7 +64,6 @@ class CCoinsViewTest : public CCoinsView map_.erase(it->first); } } - mapCoins.erase(it++); } if (!hashBlock.IsNull()) hashBestBlock_ = hashBlock; @@ -126,6 +125,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) bool found_an_entry = false; bool missed_an_entry = false; bool uncached_an_entry = false; + bool flushed_without_erase = false; // A simple map to track what we expect the cache stack to represent. std::map result; @@ -154,9 +154,16 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) bool test_havecoin_after = InsecureRandBits(2) == 0; bool result_havecoin = test_havecoin_before ? stack.back()->HaveCoin(COutPoint(txid, 0)) : false; - const Coin& entry = (InsecureRandRange(500) == 0) ? AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0)); + + // Infrequently, test usage of AccessByTxid instead of AccessCoin - the + // former just delegates to the latter and returns the first unspent in a txn. + const Coin& entry = (InsecureRandRange(500) == 0) ? + AccessByTxid(*stack.back(), txid) : stack.back()->AccessCoin(COutPoint(txid, 0)); BOOST_CHECK(coin == entry); - BOOST_CHECK(!test_havecoin_before || result_havecoin == !entry.IsSpent()); + + if (test_havecoin_before) { + BOOST_CHECK(result_havecoin == !entry.IsSpent()); + } if (test_havecoin_after) { bool ret = stack.back()->HaveCoin(COutPoint(txid, 0)); @@ -167,24 +174,29 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) Coin newcoin; newcoin.out.nValue = InsecureRand32(); newcoin.nHeight = 1; + + // Infrequently test adding unspendable coins. if (InsecureRandRange(16) == 0 && coin.IsSpent()) { newcoin.out.scriptPubKey.assign(1 + InsecureRandBits(6), OP_RETURN); BOOST_CHECK(newcoin.out.scriptPubKey.IsUnspendable()); added_an_unspendable_entry = true; } else { - newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); // Random sizes so we can test memory usage accounting + // Random sizes so we can test memory usage accounting + newcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); (coin.IsSpent() ? added_an_entry : updated_an_entry) = true; coin = newcoin; } - stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), !coin.IsSpent() || InsecureRand32() & 1); + bool is_overwrite = !coin.IsSpent() || InsecureRand32() & 1; + stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), is_overwrite); } else { + // Spend the coin. removed_an_entry = true; coin.Clear(); BOOST_CHECK(stack.back()->SpendCoin(COutPoint(txid, 0))); } } - // One every 10 iterations, remove a random entry from the cache + // Once every 10 iterations, remove a random entry from the cache if (InsecureRandRange(10) == 0) { COutPoint out(txids[InsecureRand32() % txids.size()], 0); int cacheid = InsecureRand32() % stack.size(); @@ -216,7 +228,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) if (stack.size() > 1 && InsecureRandBool() == 0) { unsigned int flushIndex = InsecureRandRange(stack.size() - 1); if (fake_best_block) stack[flushIndex]->SetBestBlock(InsecureRand256()); - BOOST_CHECK(stack[flushIndex]->Flush()); + bool should_erase = InsecureRandRange(4) < 3; + BOOST_CHECK(should_erase ? stack[flushIndex]->Flush() : stack[flushIndex]->Sync()); + flushed_without_erase |= !should_erase; } } if (InsecureRandRange(100) == 0) { @@ -224,7 +238,9 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) if (stack.size() > 0 && InsecureRandBool() == 0) { //Remove the top cache if (fake_best_block) stack.back()->SetBestBlock(InsecureRand256()); - BOOST_CHECK(stack.back()->Flush()); + bool should_erase = InsecureRandRange(4) < 3; + BOOST_CHECK(should_erase ? stack.back()->Flush() : stack.back()->Sync()); + flushed_without_erase |= !should_erase; delete stack.back(); stack.pop_back(); } @@ -260,6 +276,7 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) BOOST_CHECK(found_an_entry); BOOST_CHECK(missed_an_entry); BOOST_CHECK(uncached_an_entry); + BOOST_CHECK(flushed_without_erase); } // Run the above simulation for multiple base types. @@ -589,9 +606,9 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags) return inserted.first->second.coin.DynamicMemoryUsage(); } -void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags) +void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags, const COutPoint& outp = OUTPOINT) { - auto it = map.find(OUTPOINT); + auto it = map.find(outp); if (it == map.end()) { value = ABSENT; flags = NO_ENTRY; @@ -877,4 +894,205 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(parent_value, child_value, parent_value, parent_flags, child_flags, parent_flags); } + +Coin MakeCoin() +{ + Coin coin; + coin.out.nValue = InsecureRand32(); + coin.nHeight = InsecureRandRange(4096); + coin.fCoinBase = 0; + return coin; +} + + +//! For CCoinsViewCache instances backed by either another cache instance or +//! leveldb, test cache behavior and flag state (DIRTY/FRESH) by +//! +//! 1. Adding a random coin to the child-most cache, +//! 2. Flushing all caches (without erasing), +//! 3. Ensure the entry still exists in the cache and has been written to parent, +//! 4. (if `do_erasing_flush`) Flushing the caches again (with erasing), +//! 5. (if `do_erasing_flush`) Ensure the entry has been written to the parent and is no longer in the cache, +//! 6. Spend the coin, ensure it no longer exists in the parent. +//! +void TestFlushBehavior( + CCoinsViewCacheTest* view, + CCoinsViewDB& base, + std::vector& all_caches, + bool do_erasing_flush) +{ + CAmount value; + char flags; + size_t cache_usage; + + auto flush_all = [&all_caches](bool erase) { + // Flush in reverse order to ensure that flushes happen from children up. + for (auto i = all_caches.rbegin(); i != all_caches.rend(); ++i) { + auto cache = *i; + // hashBlock must be filled before flushing to disk; value is + // unimportant here. This is normally done during connect/disconnect block. + cache->SetBestBlock(InsecureRand256()); + erase ? cache->Flush() : cache->Sync(); + } + }; + + uint256 txid = InsecureRand256(); + COutPoint outp = COutPoint(txid, 0); + Coin coin = MakeCoin(); + // Ensure the coins views haven't seen this coin before. + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!view->HaveCoin(outp)); + + // --- 1. Adding a random coin to the child cache + // + view->AddCoin(outp, Coin(coin), false); + + cache_usage = view->DynamicMemoryUsage(); + // `base` shouldn't have coin (no flush yet) but `view` should have cached it. + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(view->HaveCoin(outp)); + + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + + // --- 2. Flushing all caches (without erasing) + // + flush_all(/*erase=*/ false); + + // CoinsMap usage should be unchanged since we didn't erase anything. + BOOST_CHECK_EQUAL(cache_usage, view->DynamicMemoryUsage()); + + // --- 3. Ensuring the entry still exists in the cache and has been written to parent + // + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, 0); // Flags should have been wiped. + + // Both views should now have the coin. + BOOST_CHECK(base.HaveCoin(outp)); + BOOST_CHECK(view->HaveCoin(outp)); + + if (do_erasing_flush) { + // --- 4. Flushing the caches again (with erasing) + // + flush_all(/*erase=*/ true); + + // Memory usage should have gone down. + BOOST_CHECK(view->DynamicMemoryUsage() < cache_usage); + + // --- 5. Ensuring the entry is no longer in the cache + // + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, ABSENT); + BOOST_CHECK_EQUAL(flags, NO_ENTRY); + + view->AccessCoin(outp); + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin.out.nValue); + BOOST_CHECK_EQUAL(flags, 0); + } + + // Can't overwrite an entry without specifying that an overwrite is + // expected. + BOOST_CHECK_THROW( + view->AddCoin(outp, Coin(coin), /*possible_overwrite=*/ false), + std::logic_error); + + // --- 6. Spend the coin. + // + BOOST_CHECK(view->SpendCoin(outp)); + + // The coin should be in the cache, but spent and marked dirty. + GetCoinsMapEntry(view->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, SPENT); + BOOST_CHECK_EQUAL(flags, DIRTY); + BOOST_CHECK(!view->HaveCoin(outp)); // Coin should be considered spent in `view`. + BOOST_CHECK(base.HaveCoin(outp)); // But coin should still be unspent in `base`. + + flush_all(/*erase=*/ false); + + // Coin should be considered spent in both views. + BOOST_CHECK(!view->HaveCoin(outp)); + BOOST_CHECK(!base.HaveCoin(outp)); + + // Spent coin should not be spendable. + BOOST_CHECK(!view->SpendCoin(outp)); + + // --- Bonus check: ensure that a coin added to the base view via one cache + // can be spent by another cache which has never seen it. + // + txid = InsecureRand256(); + outp = COutPoint(txid, 0); + coin = MakeCoin(); + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + all_caches[0]->AddCoin(outp, std::move(coin), false); + all_caches[0]->Sync(); + BOOST_CHECK(base.HaveCoin(outp)); + BOOST_CHECK(all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoinInCache(outp)); + + BOOST_CHECK(all_caches[1]->SpendCoin(outp)); + flush_all(/*erase=*/ false); + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + flush_all(/*erase=*/ true); // Erase all cache content. + + // --- Bonus check 2: ensure that a FRESH, spent coin is deleted by Sync() + // + txid = InsecureRand256(); + outp = COutPoint(txid, 0); + coin = MakeCoin(); + CAmount coin_val = coin.out.nValue; + BOOST_CHECK(!base.HaveCoin(outp)); + BOOST_CHECK(!all_caches[0]->HaveCoin(outp)); + BOOST_CHECK(!all_caches[1]->HaveCoin(outp)); + + // Add and spend from same cache without flushing. + all_caches[0]->AddCoin(outp, std::move(coin), false); + + // Coin should be FRESH in the cache. + GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, coin_val); + BOOST_CHECK_EQUAL(flags, DIRTY|FRESH); + + // Base shouldn't have seen coin. + BOOST_CHECK(!base.HaveCoin(outp)); + + BOOST_CHECK(all_caches[0]->SpendCoin(outp)); + all_caches[0]->Sync(); + + // Ensure there is no sign of the coin after spend/flush. + GetCoinsMapEntry(all_caches[0]->map(), value, flags, outp); + BOOST_CHECK_EQUAL(value, ABSENT); + BOOST_CHECK_EQUAL(flags, NO_ENTRY); + BOOST_CHECK(!all_caches[0]->HaveCoinInCache(outp)); + BOOST_CHECK(!base.HaveCoin(outp)); +} + +BOOST_AUTO_TEST_CASE(ccoins_flush_behavior) +{ + // Create two in-memory caches atop a leveldb view. + CCoinsViewDB base{"test", /*nCacheSize=*/ 1 << 23, /*fMemory=*/ true, /*fWipe=*/ false}; + std::vector caches; + caches.push_back(new CCoinsViewCacheTest(&base)); + caches.push_back(new CCoinsViewCacheTest(caches.back())); + + for (CCoinsViewCacheTest* view : caches) { + TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ false); + TestFlushBehavior(view, base, caches, /*do_erasing_flush=*/ true); + } + + // Clean up the caches. + while (caches.size() > 0) { + delete caches.back(); + caches.pop_back(); + } +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/txdb.cpp b/src/txdb.cpp index 329f6b3bddca2..840c1aeb6e4ad 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -84,7 +84,7 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { +bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; @@ -119,8 +119,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) { changed++; } count++; - CCoinsMap::iterator itOld = it++; - mapCoins.erase(itOld); + it = erase ? mapCoins.erase(it) : std::next(it); if (batch.SizeEstimate() > batch_size) { LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); diff --git a/src/txdb.h b/src/txdb.h index d00e5629ee04a..4ea97c5237aec 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -62,7 +62,7 @@ class CCoinsViewDB final : public CCoinsView bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override; + bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; std::unique_ptr Cursor() const override; //! Attempt to update from an older database format. Returns whether an error occurred. From 1d0e410280584b50c55b065814b0094ce84fb100 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 30 Jan 2023 11:32:59 -0500 Subject: [PATCH 11/12] merge bitcoin#26999: A few follow-ups to bitcoin#17487 --- src/coins.cpp | 9 ++++++--- src/test/coins_tests.cpp | 2 +- src/test/fuzz/coins_view.cpp | 3 +++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 3cb5ef1125e88..5ce4f16346007 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -236,15 +236,18 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } bool CCoinsViewCache::Flush() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ true); - cacheCoins.clear(); + bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true); + if (fOk && !cacheCoins.empty()) { + /* BatchWrite must erase all cacheCoins elements when erase=true. */ + throw std::logic_error("Not all cached coins were erased"); + } cachedCoinsUsage = 0; return fOk; } bool CCoinsViewCache::Sync() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/ false); + bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false); // Instead of clearing `cacheCoins` as we would in Flush(), just clear the // FRESH/DIRTY flags of any coin that isn't spent. for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) { diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index bf3519cee0aaf..2c8e0b10133db 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,7 +55,7 @@ class CCoinsViewTest : public CCoinsView bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : ++it) { + for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) { if (it->second.flags & CCoinsCacheEntry::DIRTY) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index f5fcbcd84c5a2..5bd9c8a0c77e1 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -76,6 +76,9 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view) [&] { (void)coins_view_cache.Flush(); }, + [&] { + (void)coins_view_cache.Sync(); + }, [&] { coins_view_cache.SetBestBlock(ConsumeUInt256(fuzzed_data_provider)); }, From b65863783f0cb0ef9ad01284ee0c89adc2fac99b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 23 Aug 2024 07:00:39 +0000 Subject: [PATCH 12/12] merge bitcoin#27011: Add simulation-based `CCoinsViewCache` fuzzer --- src/Makefile.test.include | 1 + src/coins.cpp | 24 +- src/coins.h | 10 +- src/test/fuzz/coins_view.cpp | 2 +- src/test/fuzz/coinscache_sim.cpp | 460 +++++++++++++++++++++++++++++++ src/util/hasher.cpp | 5 +- src/util/hasher.h | 2 +- 7 files changed, 497 insertions(+), 7 deletions(-) create mode 100644 src/test/fuzz/coinscache_sim.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index f907a398ccbd6..d7ccc53c5d2b5 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -252,6 +252,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/chain.cpp \ test/fuzz/checkqueue.cpp \ test/fuzz/coins_view.cpp \ + test/fuzz/coinscache_sim.cpp \ test/fuzz/connman.cpp \ test/fuzz/crypto.cpp \ test/fuzz/crypto_aes256.cpp \ diff --git a/src/coins.cpp b/src/coins.cpp index 5ce4f16346007..431d7223d8898 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -31,7 +31,10 @@ bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } -CCoinsViewCache::CCoinsViewCache(CCoinsView *baseIn) : CCoinsViewBacked(baseIn), cachedCoinsUsage(0) {} +CCoinsViewCache::CCoinsViewCache(CCoinsView* baseIn, bool deterministic) : + CCoinsViewBacked(baseIn), m_deterministic(deterministic), + cacheCoins(0, SaltedOutpointHasher(/*deterministic=*/deterministic)) +{} size_t CCoinsViewCache::DynamicMemoryUsage() const { return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage; @@ -292,7 +295,24 @@ void CCoinsViewCache::ReallocateCache() // Cache should be empty when we're calling this. assert(cacheCoins.size() == 0); cacheCoins.~CCoinsMap(); - ::new (&cacheCoins) CCoinsMap(); + ::new (&cacheCoins) CCoinsMap(0, SaltedOutpointHasher(/*deterministic=*/m_deterministic)); +} + +void CCoinsViewCache::SanityCheck() const +{ + size_t recomputed_usage = 0; + for (const auto& [_, entry] : cacheCoins) { + unsigned attr = 0; + if (entry.flags & CCoinsCacheEntry::DIRTY) attr |= 1; + if (entry.flags & CCoinsCacheEntry::FRESH) attr |= 2; + if (entry.coin.IsSpent()) attr |= 4; + // Only 5 combinations are possible. + assert(attr != 2 && attr != 4 && attr != 7); + + // Recompute cachedCoinsUsage. + recomputed_usage += entry.coin.DynamicMemoryUsage(); + } + assert(recomputed_usage == cachedCoinsUsage); } static const size_t MAX_OUTPUTS_PER_BLOCK = MaxBlockSize() / ::GetSerializeSize(CTxOut(), PROTOCOL_VERSION); diff --git a/src/coins.h b/src/coins.h index acd5d2a4f5ff9..3dbe23aea5820 100644 --- a/src/coins.h +++ b/src/coins.h @@ -212,6 +212,9 @@ class CCoinsViewBacked : public CCoinsView /** CCoinsView that adds a memory cache for transactions to another CCoinsView */ class CCoinsViewCache : public CCoinsViewBacked { +private: + const bool m_deterministic; + protected: /** * Make mutable so that we can "fill the cache" even from Get-methods @@ -221,10 +224,10 @@ class CCoinsViewCache : public CCoinsViewBacked mutable CCoinsMap cacheCoins; /* Cached dynamic memory usage for the inner Coin objects. */ - mutable size_t cachedCoinsUsage; + mutable size_t cachedCoinsUsage{0}; public: - CCoinsViewCache(CCoinsView *baseIn); + CCoinsViewCache(CCoinsView *baseIn, bool deterministic = false); /** * By deleting the copy constructor, we prevent accidentally using it when one intends to create a cache on top of a base cache. @@ -321,6 +324,9 @@ class CCoinsViewCache : public CCoinsViewBacked //! See: https://stackoverflow.com/questions/42114044/how-to-release-unordered-map-memory void ReallocateCache(); + //! Run an internal sanity check on the cache data structure. */ + void SanityCheck() const; + private: /** * @note this is marked const, but may actually append to `cacheCoins`, increasing diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 5bd9c8a0c77e1..9de85f9bf8c8e 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -48,7 +48,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CCoinsView backend_coins_view; - CCoinsViewCache coins_view_cache{&backend_coins_view}; + CCoinsViewCache coins_view_cache{&backend_coins_view, /*deterministic=*/true}; COutPoint random_out_point; Coin random_coin; CMutableTransaction random_mutable_transaction; diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp new file mode 100644 index 0000000000000..911d96450844b --- /dev/null +++ b/src/test/fuzz/coinscache_sim.cpp @@ -0,0 +1,460 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// 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 +#include +#include + +namespace { + +/** Number of distinct COutPoint values used in this test. */ +constexpr uint32_t NUM_OUTPOINTS = 256; +/** Number of distinct Coin values used in this test (ignoring nHeight). */ +constexpr uint32_t NUM_COINS = 256; +/** Maximum number CCoinsViewCache objects used in this test. */ +constexpr uint32_t MAX_CACHES = 4; +/** Data type large enough to hold NUM_COINS-1. */ +using coinidx_type = uint8_t; + +struct PrecomputedData +{ + //! Randomly generated COutPoint values. + COutPoint outpoints[NUM_OUTPOINTS]; + + //! Randomly generated Coin values. + Coin coins[NUM_COINS]; + + PrecomputedData() + { + static const uint8_t PREFIX_O[1] = {'o'}; /** Hash prefix for outpoint hashes. */ + static const uint8_t PREFIX_S[1] = {'s'}; /** Hash prefix for coins scriptPubKeys. */ + static const uint8_t PREFIX_M[1] = {'m'}; /** Hash prefix for coins nValue/fCoinBase. */ + + for (uint32_t i = 0; i < NUM_OUTPOINTS; ++i) { + uint32_t idx = (i * 1200U) >> 12; /* Map 3 or 4 entries to same txid. */ + const uint8_t ser[4] = {uint8_t(idx), uint8_t(idx >> 8), uint8_t(idx >> 16), uint8_t(idx >> 24)}; + CSHA256().Write(PREFIX_O, 1).Write(ser, sizeof(ser)).Finalize(outpoints[i].hash.begin()); + outpoints[i].n = i; + } + + for (uint32_t i = 0; i < NUM_COINS; ++i) { + const uint8_t ser[4] = {uint8_t(i), uint8_t(i >> 8), uint8_t(i >> 16), uint8_t(i >> 24)}; + uint256 hash; + CSHA256().Write(PREFIX_S, 1).Write(ser, sizeof(ser)).Finalize(hash.begin()); + /* Convert hash to scriptPubkeys (of different lengths, so SanityCheck's cached memory + * usage check has a chance to detect mismatches). */ + switch (i % 2U) { + case 0: /* P2PKH */ + coins[i].out.scriptPubKey.resize(25); + coins[i].out.scriptPubKey[0] = OP_DUP; + coins[i].out.scriptPubKey[1] = OP_HASH160; + coins[i].out.scriptPubKey[2] = 20; + std::copy(hash.begin(), hash.begin() + 20, coins[i].out.scriptPubKey.begin() + 3); + coins[i].out.scriptPubKey[23] = OP_EQUALVERIFY; + coins[i].out.scriptPubKey[24] = OP_CHECKSIG; + break; + case 1: /* P2SH */ + coins[i].out.scriptPubKey.resize(23); + coins[i].out.scriptPubKey[0] = OP_HASH160; + coins[i].out.scriptPubKey[1] = 20; + std::copy(hash.begin(), hash.begin() + 20, coins[i].out.scriptPubKey.begin() + 2); + coins[i].out.scriptPubKey[12] = OP_EQUAL; + break; + } + /* Hash again to construct nValue and fCoinBase. */ + CSHA256().Write(PREFIX_M, 1).Write(ser, sizeof(ser)).Finalize(hash.begin()); + coins[i].out.nValue = CAmount(hash.GetUint64(0) % MAX_MONEY); + coins[i].fCoinBase = (hash.GetUint64(1) & 7) == 0; + coins[i].nHeight = 0; /* Real nHeight used in simulation is set dynamically. */ + } + } +}; + +enum class EntryType : uint8_t +{ + /* This entry in the cache does not exist (so we'd have to look in the parent cache). */ + NONE, + + /* This entry in the cache corresponds to an unspent coin. */ + UNSPENT, + + /* This entry in the cache corresponds to a spent coin. */ + SPENT, +}; + +struct CacheEntry +{ + /* Type of entry. */ + EntryType entrytype; + + /* Index in the coins array this entry corresponds to (only if entrytype == UNSPENT). */ + coinidx_type coinidx; + + /* nHeight value for this entry (so the coins[coinidx].nHeight value is ignored; only if entrytype == UNSPENT). */ + uint32_t height; +}; + +struct CacheLevel +{ + CacheEntry entry[NUM_OUTPOINTS]; + + void Wipe() { + for (uint32_t i = 0; i < NUM_OUTPOINTS; ++i) { + entry[i].entrytype = EntryType::NONE; + } + } +}; + +/** Class for the base of the hierarchy (roughly simulating a memory-backed CCoinsViewDB). + * + * The initial state consists of the empty UTXO set, though coins whose output index + * is 3 (mod 5) always have GetCoin() succeed (but returning an IsSpent() coin unless a UTXO + * exists). Coins whose output index is 4 (mod 5) have GetCoin() always succeed after being spent. + * This exercises code paths with spent, non-DIRTY cache entries. + */ +class CoinsViewBottom final : public CCoinsView +{ + std::map m_data; + +public: + bool GetCoin(const COutPoint& outpoint, Coin& coin) const final + { + auto it = m_data.find(outpoint); + if (it == m_data.end()) { + if ((outpoint.n % 5) == 3) { + coin.Clear(); + return true; + } + return false; + } else { + coin = it->second; + return true; + } + } + + bool HaveCoin(const COutPoint& outpoint) const final + { + return m_data.count(outpoint); + } + + uint256 GetBestBlock() const final { return {}; } + std::vector GetHeadBlocks() const final { return {}; } + std::unique_ptr Cursor() const final { return {}; } + size_t EstimateSize() const final { return m_data.size(); } + + bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final + { + for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) { + if (it->second.flags & CCoinsCacheEntry::DIRTY) { + if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) { + m_data.erase(it->first); + } else if (erase) { + m_data[it->first] = std::move(it->second.coin); + } else { + m_data[it->first] = it->second.coin; + } + } else { + /* For non-dirty entries being written, compare them with what we have. */ + auto it2 = m_data.find(it->first); + if (it->second.coin.IsSpent()) { + assert(it2 == m_data.end() || it2->second.IsSpent()); + } else { + assert(it2 != m_data.end()); + assert(it->second.coin.out == it2->second.out); + assert(it->second.coin.fCoinBase == it2->second.fCoinBase); + assert(it->second.coin.nHeight == it2->second.nHeight); + } + } + } + return true; + } +}; + +} // namespace + +FUZZ_TARGET(coinscache_sim) +{ + /** Precomputed COutPoint and CCoins values. */ + static const PrecomputedData data; + + /** Dummy coinsview instance (base of the hierarchy). */ + CoinsViewBottom bottom; + /** Real CCoinsViewCache objects. */ + std::vector> caches; + /** Simulated cache data (sim_caches[0] matches bottom, sim_caches[i+1] matches caches[i]). */ + CacheLevel sim_caches[MAX_CACHES + 1]; + /** Current height in the simulation. */ + uint32_t current_height = 1U; + + // Initialize bottom simulated cache. + sim_caches[0].Wipe(); + + /** Helper lookup function in the simulated cache stack. */ + auto lookup = [&](uint32_t outpointidx, int sim_idx = -1) -> std::optional> { + uint32_t cache_idx = sim_idx == -1 ? caches.size() : sim_idx; + while (true) { + const auto& entry = sim_caches[cache_idx].entry[outpointidx]; + if (entry.entrytype == EntryType::UNSPENT) { + return {{entry.coinidx, entry.height}}; + } else if (entry.entrytype == EntryType::SPENT) { + return std::nullopt; + }; + if (cache_idx == 0) break; + --cache_idx; + } + return std::nullopt; + }; + + /** Flush changes in top cache to the one below. */ + auto flush = [&]() { + assert(caches.size() >= 1); + auto& cache = sim_caches[caches.size()]; + auto& prev_cache = sim_caches[caches.size() - 1]; + for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) { + if (cache.entry[outpointidx].entrytype != EntryType::NONE) { + prev_cache.entry[outpointidx] = cache.entry[outpointidx]; + cache.entry[outpointidx].entrytype = EntryType::NONE; + } + } + }; + + // Main simulation loop: read commands from the fuzzer input, and apply them + // to both the real cache stack and the simulation. + FuzzedDataProvider provider(buffer.data(), buffer.size()); + LIMITED_WHILE(provider.remaining_bytes(), 10000) { + // Every operation (except "Change height") moves current height forward, + // so it functions as a kind of epoch, making ~all UTXOs unique. + ++current_height; + // Make sure there is always at least one CCoinsViewCache. + if (caches.empty()) { + caches.emplace_back(new CCoinsViewCache(&bottom, /*deterministic=*/true)); + sim_caches[caches.size()].Wipe(); + } + + // Execute command. + CallOneOf( + provider, + + [&]() { // GetCoin + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Look up in simulation data. + auto sim = lookup(outpointidx); + // Look up in real caches. + Coin realcoin; + auto real = caches.back()->GetCoin(data.outpoints[outpointidx], realcoin); + // Compare results. + if (!sim.has_value()) { + assert(!real || realcoin.IsSpent()); + } else { + assert(real && !realcoin.IsSpent()); + const auto& simcoin = data.coins[sim->first]; + assert(realcoin.out == simcoin.out); + assert(realcoin.fCoinBase == simcoin.fCoinBase); + assert(realcoin.nHeight == sim->second); + } + }, + + [&]() { // HaveCoin + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Look up in simulation data. + auto sim = lookup(outpointidx); + // Look up in real caches. + auto real = caches.back()->HaveCoin(data.outpoints[outpointidx]); + // Compare results. + assert(sim.has_value() == real); + }, + + [&]() { // HaveCoinInCache + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Invoke on real cache (there is no equivalent in simulation, so nothing to compare result with). + (void)caches.back()->HaveCoinInCache(data.outpoints[outpointidx]); + }, + + [&]() { // AccessCoin + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Look up in simulation data. + auto sim = lookup(outpointidx); + // Look up in real caches. + const auto& realcoin = caches.back()->AccessCoin(data.outpoints[outpointidx]); + // Compare results. + if (!sim.has_value()) { + assert(realcoin.IsSpent()); + } else { + assert(!realcoin.IsSpent()); + const auto& simcoin = data.coins[sim->first]; + assert(simcoin.out == realcoin.out); + assert(simcoin.fCoinBase == realcoin.fCoinBase); + assert(realcoin.nHeight == sim->second); + } + }, + + [&]() { // AddCoin (only possible_overwrite if necessary) + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + uint32_t coinidx = provider.ConsumeIntegralInRange(0, NUM_COINS - 1); + // Look up in simulation data (to know whether we must set possible_overwrite or not). + auto sim = lookup(outpointidx); + // Invoke on real caches. + Coin coin = data.coins[coinidx]; + coin.nHeight = current_height; + caches.back()->AddCoin(data.outpoints[outpointidx], std::move(coin), sim.has_value()); + // Apply to simulation data. + auto& entry = sim_caches[caches.size()].entry[outpointidx]; + entry.entrytype = EntryType::UNSPENT; + entry.coinidx = coinidx; + entry.height = current_height; + }, + + [&]() { // AddCoin (always possible_overwrite) + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + uint32_t coinidx = provider.ConsumeIntegralInRange(0, NUM_COINS - 1); + // Invoke on real caches. + Coin coin = data.coins[coinidx]; + coin.nHeight = current_height; + caches.back()->AddCoin(data.outpoints[outpointidx], std::move(coin), true); + // Apply to simulation data. + auto& entry = sim_caches[caches.size()].entry[outpointidx]; + entry.entrytype = EntryType::UNSPENT; + entry.coinidx = coinidx; + entry.height = current_height; + }, + + [&]() { // SpendCoin (moveto = nullptr) + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Invoke on real caches. + caches.back()->SpendCoin(data.outpoints[outpointidx], nullptr); + // Apply to simulation data. + sim_caches[caches.size()].entry[outpointidx].entrytype = EntryType::SPENT; + }, + + [&]() { // SpendCoin (with moveto) + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Look up in simulation data (to compare the returned *moveto with). + auto sim = lookup(outpointidx); + // Invoke on real caches. + Coin realcoin; + caches.back()->SpendCoin(data.outpoints[outpointidx], &realcoin); + // Apply to simulation data. + sim_caches[caches.size()].entry[outpointidx].entrytype = EntryType::SPENT; + // Compare *moveto with the value expected based on simulation data. + if (!sim.has_value()) { + assert(realcoin.IsSpent()); + } else { + assert(!realcoin.IsSpent()); + const auto& simcoin = data.coins[sim->first]; + assert(simcoin.out == realcoin.out); + assert(simcoin.fCoinBase == realcoin.fCoinBase); + assert(realcoin.nHeight == sim->second); + } + }, + + [&]() { // Uncache + uint32_t outpointidx = provider.ConsumeIntegralInRange(0, NUM_OUTPOINTS - 1); + // Apply to real caches (there is no equivalent in our simulation). + caches.back()->Uncache(data.outpoints[outpointidx]); + }, + + [&]() { // Add a cache level (if not already at the max). + if (caches.size() != MAX_CACHES) { + // Apply to real caches. + caches.emplace_back(new CCoinsViewCache(&*caches.back(), /*deterministic=*/true)); + // Apply to simulation data. + sim_caches[caches.size()].Wipe(); + } + }, + + [&]() { // Remove a cache level. + // Apply to real caches (this reduces caches.size(), implicitly doing the same on the simulation data). + caches.back()->SanityCheck(); + caches.pop_back(); + }, + + [&]() { // Flush. + // Apply to simulation data. + flush(); + // Apply to real caches. + caches.back()->Flush(); + }, + + [&]() { // Sync. + // Apply to simulation data (note that in our simulation, syncing and flushing is the same thing). + flush(); + // Apply to real caches. + caches.back()->Sync(); + }, + + [&]() { // Flush + ReallocateCache. + // Apply to simulation data. + flush(); + // Apply to real caches. + caches.back()->Flush(); + caches.back()->ReallocateCache(); + }, + + [&]() { // GetCacheSize + (void)caches.back()->GetCacheSize(); + }, + + [&]() { // DynamicMemoryUsage + (void)caches.back()->DynamicMemoryUsage(); + }, + + [&]() { // Change height + current_height = provider.ConsumeIntegralInRange(1, current_height - 1); + } + ); + } + + // Sanity check all the remaining caches + for (const auto& cache : caches) { + cache->SanityCheck(); + } + + // Full comparison between caches and simulation data, from bottom to top, + // as AccessCoin on a higher cache may affect caches below it. + for (unsigned sim_idx = 1; sim_idx <= caches.size(); ++sim_idx) { + auto& cache = *caches[sim_idx - 1]; + size_t cache_size = 0; + + for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) { + cache_size += cache.HaveCoinInCache(data.outpoints[outpointidx]); + const auto& real = cache.AccessCoin(data.outpoints[outpointidx]); + auto sim = lookup(outpointidx, sim_idx); + if (!sim.has_value()) { + assert(real.IsSpent()); + } else { + assert(!real.IsSpent()); + assert(real.out == data.coins[sim->first].out); + assert(real.fCoinBase == data.coins[sim->first].fCoinBase); + assert(real.nHeight == sim->second); + } + } + + // HaveCoinInCache ignores spent coins, so GetCacheSize() may exceed it. */ + assert(cache.GetCacheSize() >= cache_size); + } + + // Compare the bottom coinsview (not a CCoinsViewCache) with sim_cache[0]. + for (uint32_t outpointidx = 0; outpointidx < NUM_OUTPOINTS; ++outpointidx) { + Coin realcoin; + bool real = bottom.GetCoin(data.outpoints[outpointidx], realcoin); + auto sim = lookup(outpointidx, 0); + if (!sim.has_value()) { + assert(!real || realcoin.IsSpent()); + } else { + assert(real && !realcoin.IsSpent()); + assert(realcoin.out == data.coins[sim->first].out); + assert(realcoin.fCoinBase == data.coins[sim->first].fCoinBase); + assert(realcoin.nHeight == sim->second); + } + } +} diff --git a/src/util/hasher.cpp b/src/util/hasher.cpp index 5900daf0500f1..8e833214a9a1c 100644 --- a/src/util/hasher.cpp +++ b/src/util/hasher.cpp @@ -9,7 +9,10 @@ SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits::max())), k1(GetRand(std::numeric_limits::max())) {} -SaltedOutpointHasher::SaltedOutpointHasher() : k0(GetRand(std::numeric_limits::max())), k1(GetRand(std::numeric_limits::max())) {} +SaltedOutpointHasher::SaltedOutpointHasher(bool deterministic) : + k0(deterministic ? 0x8e819f2607a18de6 : GetRand(std::numeric_limits::max())), + k1(deterministic ? 0xf4020d2e3983b0eb : GetRand(std::numeric_limits::max())) +{} SaltedSipHasher::SaltedSipHasher() : m_k0(GetRand(std::numeric_limits::max())), m_k1(GetRand(std::numeric_limits::max())) {} diff --git a/src/util/hasher.h b/src/util/hasher.h index fa2fea30d82fb..cb7d26968bbd9 100644 --- a/src/util/hasher.h +++ b/src/util/hasher.h @@ -30,7 +30,7 @@ class SaltedOutpointHasher const uint64_t k0, k1; public: - SaltedOutpointHasher(); + SaltedOutpointHasher(bool deterministic = false); /** * This *must* return size_t. With Boost 1.46 on 32-bit systems the