From 8e2ecfe2496d8a015f3ee8723025a438feffbd28 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Wed, 11 Dec 2019 16:41:40 -0500
Subject: validation: add CChainState.m_from_snapshot_blockhash

This parameter is unused, but in future commits will allow ChainstateManager to
differentiate between chainstates created from a UTXO snapshot from those that
weren't.
---
 src/validation.cpp |  8 +++++++-
 src/validation.h   | 11 +++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

(limited to 'src')

diff --git a/src/validation.cpp b/src/validation.cpp
index a5b68e4ebd..31b78380af 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1244,7 +1244,9 @@ void CoinsViews::InitCache()
 
 // NOTE: for now m_blockman is set to a global, but this will be changed
 // in a future commit.
-CChainState::CChainState() : m_blockman(g_blockman) {}
+CChainState::CChainState(uint256 from_snapshot_blockhash)
+    : m_blockman(g_blockman),
+      m_from_snapshot_blockhash(from_snapshot_blockhash) {}
 
 
 void CChainState::InitCoinsDB(
@@ -1253,6 +1255,10 @@ void CChainState::InitCoinsDB(
     bool should_wipe,
     std::string leveldb_name)
 {
+    if (!m_from_snapshot_blockhash.IsNull()) {
+        leveldb_name += "_" + m_from_snapshot_blockhash.ToString();
+    }
+
     m_coins_views = MakeUnique<CoinsViews>(
         leveldb_name, cache_size_bytes, in_memory, should_wipe);
 }
diff --git a/src/validation.h b/src/validation.h
index a5335edc43..07d90ab110 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -591,8 +591,8 @@ private:
     std::unique_ptr<CoinsViews> m_coins_views;
 
 public:
-    CChainState(BlockManager& blockman) : m_blockman(blockman) {}
-    CChainState();
+    explicit CChainState(BlockManager& blockman) : m_blockman(blockman) {}
+    explicit CChainState(uint256 from_snapshot_blockhash = uint256());
 
     /**
      * Initialize the CoinsViews UTXO set database management data structures. The in-memory
@@ -620,6 +620,13 @@ public:
     //! @see CChain, CBlockIndex.
     CChain m_chain;
 
+    /**
+     * The blockhash which is the base of the snapshot this chainstate was created from.
+     *
+     * IsNull() if this chainstate was not created from a snapshot.
+     */
+    const uint256 m_from_snapshot_blockhash{};
+
     /**
      * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and
      * as good as our current tip or better. Entries may be failed, though, and pruning nodes may be
-- 
cgit v1.2.3


From 89cdf4d5692d396b8c7177b3918aa9dab07f9624 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Thu, 12 Dec 2019 10:20:44 -0500
Subject: validation: introduce unused ChainstateManager

ChainstateManager is responsible for creating and managing multiple
chainstates, and will provide a high-level interface for accessing the
appropriate chainstate based upon a certain use.

Incorporates feedback from Marco Falke. Additional documentation written
by Russ Yanofsky.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
---
 src/validation.cpp |  96 +++++++++++++++++++++++++++++++++++++++++++++
 src/validation.h   | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+)

(limited to 'src')

diff --git a/src/validation.cpp b/src/validation.cpp
index 31b78380af..a6b1344792 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -20,6 +20,7 @@
 #include <index/txindex.h>
 #include <logging.h>
 #include <logging/timer.h>
+#include <optional.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <policy/settings.h>
@@ -4950,6 +4951,14 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
     assert(nNodes == forward.size());
 }
 
+std::string CChainState::ToString()
+{
+    CBlockIndex* tip = m_chain.Tip();
+    return strprintf("Chainstate [%s] @ height %d (%s)",
+        m_from_snapshot_blockhash.IsNull() ? "ibd" : "snapshot",
+        tip ? tip->nHeight : -1, tip ? tip->GetBlockHash().ToString() : "null");
+}
+
 std::string CBlockFileInfo::ToString() const
 {
     return strprintf("CBlockFileInfo(blocks=%u, size=%u, heights=%u...%u, time=%s...%s)", nBlocks, nSize, nHeightFirst, nHeightLast, FormatISO8601Date(nTimeFirst), FormatISO8601Date(nTimeLast));
@@ -5144,3 +5153,90 @@ public:
     }
 };
 static CMainCleanup instance_of_cmaincleanup;
+
+Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
+    if (m_active_chainstate != nullptr) {
+        // If a snapshot chainstate exists, it will always be our active.
+        return m_active_chainstate->m_from_snapshot_blockhash;
+    }
+    return {};
+}
+
+std::vector<CChainState*> ChainstateManager::GetAll()
+{
+    std::vector<CChainState*> out;
+
+    if (!IsSnapshotValidated() && m_ibd_chainstate) {
+        out.push_back(m_ibd_chainstate.get());
+    }
+
+    if (m_snapshot_chainstate) {
+        out.push_back(m_snapshot_chainstate.get());
+    }
+
+    return out;
+}
+
+CChainState& ChainstateManager::InitializeChainstate(const uint256& snapshot_blockhash)
+{
+    bool is_snapshot = !snapshot_blockhash.IsNull();
+    std::unique_ptr<CChainState>& to_modify =
+        is_snapshot ? m_snapshot_chainstate : m_ibd_chainstate;
+
+    if (to_modify) {
+        throw std::logic_error("should not be overwriting a chainstate");
+    }
+
+    to_modify.reset(new CChainState(snapshot_blockhash));
+
+    // Snapshot chainstates and initial IBD chaintates always become active.
+    if (is_snapshot || (!is_snapshot && !m_active_chainstate)) {
+        LogPrintf("Switching active chainstate to %s\n", to_modify->ToString());
+        m_active_chainstate = to_modify.get();
+    } else {
+        throw std::logic_error("unexpected chainstate activation");
+    }
+
+    return *to_modify;
+}
+
+CChain& ChainstateManager::ActiveChain() const
+{
+    assert(m_active_chainstate);
+    return m_active_chainstate->m_chain;
+}
+
+bool ChainstateManager::IsSnapshotActive() const
+{
+    return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();
+}
+
+CChainState& ChainstateManager::ValidatedChainstate() const
+{
+    if (m_snapshot_chainstate && IsSnapshotValidated()) {
+        return *m_snapshot_chainstate.get();
+    }
+    assert(m_ibd_chainstate);
+    return *m_ibd_chainstate.get();
+}
+
+bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const
+{
+    return (m_snapshot_chainstate && chainstate == m_ibd_chainstate.get());
+}
+
+void ChainstateManager::Unload()
+{
+    for (CChainState* chainstate : this->GetAll()) {
+        chainstate->m_chain.SetTip(nullptr);
+        chainstate->UnloadBlockIndex();
+    }
+}
+
+void ChainstateManager::Reset()
+{
+    m_ibd_chainstate.reset();
+    m_snapshot_chainstate.reset();
+    m_active_chainstate = nullptr;
+    m_snapshot_validated = false;
+}
diff --git a/src/validation.h b/src/validation.h
index 07d90ab110..35e4380bc0 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -14,6 +14,7 @@
 #include <coins.h>
 #include <crypto/common.h> // for ReadLE64
 #include <fs.h>
+#include <optional.h>
 #include <policy/feerate.h>
 #include <protocol.h> // For CMessageHeader::MessageStartChars
 #include <script/script_error.h>
@@ -539,6 +540,9 @@ enum class CoinsCacheSizeState
     OK = 0
 };
 
+// Defined below, but needed for `friend` usage in CChainState.
+class ChainstateManager;
+
 /**
  * CChainState stores and provides an API to update our local knowledge of the
  * current best chain.
@@ -748,6 +752,8 @@ public:
         size_t max_coins_cache_size_bytes,
         size_t max_mempool_size_bytes) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    std::string ToString() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
 private:
     bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
     bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
@@ -760,6 +766,8 @@ private:
 
     //! Mark a block as not having block data
     void EraseBlockData(CBlockIndex* index) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    friend ChainstateManager;
 };
 
 /** Mark a block as precious and reorganize.
@@ -775,6 +783,111 @@ bool InvalidateBlock(BlockValidationState& state, const CChainParams& chainparam
 /** Remove invalidity status from a block and its descendants. */
 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
+/**
+ * Provides an interface for creating and interacting with one or two
+ * chainstates: an IBD chainstate generated by downloading blocks, and
+ * an optional snapshot chainstate loaded from a UTXO snapshot. Managed
+ * chainstates can be maintained at different heights simultaneously.
+ *
+ * This class provides abstractions that allow the retrieval of the current
+ * most-work chainstate ("Active") as well as chainstates which may be in
+ * background use to validate UTXO snapshots.
+ *
+ * Definitions:
+ *
+ * *IBD chainstate*: a chainstate whose current state has been "fully"
+ *   validated by the initial block download process.
+ *
+ * *Snapshot chainstate*: a chainstate populated by loading in an
+ *    assumeutxo UTXO snapshot.
+ *
+ * *Active chainstate*: the chainstate containing the current most-work
+ *    chain. Consulted by most parts of the system (net_processing,
+ *    wallet) as a reflection of the current chain and UTXO set.
+ *    This may either be an IBD chainstate or a snapshot chainstate.
+ *
+ * *Background IBD chainstate*: an IBD chainstate for which the
+ *    IBD process is happening in the background while use of the
+ *    active (snapshot) chainstate allows the rest of the system to function.
+ *
+ * *Validated chainstate*: the most-work chainstate which has been validated
+ *   locally via initial block download. This will be the snapshot chainstate
+ *   if a snapshot was loaded and all blocks up to the snapshot starting point
+ *   have been downloaded and validated (via background validation), otherwise
+ *   it will be the IBD chainstate.
+ */
+class ChainstateManager
+{
+private:
+    //! The chainstate used under normal operation (i.e. "regular" IBD) or, if
+    //! a snapshot is in use, for background validation.
+    //!
+    //! Its contents (including on-disk data) will be deleted *upon shutdown*
+    //! after background validation of the snapshot has completed. We do not
+    //! free the chainstate contents immediately after it finishes validation
+    //! to cautiously avoid a case where some other part of the system is still
+    //! using this pointer (e.g. net_processing).
+    std::unique_ptr<CChainState> m_ibd_chainstate;
+
+    //! A chainstate initialized on the basis of a UTXO snapshot. If this is
+    //! non-null, it is always our active chainstate.
+    std::unique_ptr<CChainState> m_snapshot_chainstate;
+
+    //! Points to either the ibd or snapshot chainstate; indicates our
+    //! most-work chain.
+    CChainState* m_active_chainstate{nullptr};
+
+    //! If true, the assumed-valid chainstate has been fully validated
+    //! by the background validation chainstate.
+    bool m_snapshot_validated{false};
+
+    // For access to m_active_chainstate.
+    friend CChain& ChainActive();
+
+public:
+    //! Instantiate a new chainstate and assign it based upon whether it is
+    //! from a snapshot.
+    //!
+    //! @param[in] snapshot_blockhash   If given, signify that this chainstate
+    //!                                 is based on a snapshot.
+    CChainState& InitializeChainstate(const uint256& snapshot_blockhash = uint256())
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+    //! Get all chainstates currently being used.
+    std::vector<CChainState*> GetAll();
+
+    //! The most-work chain.
+    CChain& ActiveChain() const;
+    int ActiveHeight() const { return ActiveChain().Height(); }
+    CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
+
+    bool IsSnapshotActive() const;
+
+    Optional<uint256> SnapshotBlockhash() const;
+
+    //! Is there a snapshot in use and has it been fully validated?
+    bool IsSnapshotValidated() const { return m_snapshot_validated; }
+
+    //! @returns true if this chainstate is being used to validate an active
+    //!          snapshot in the background.
+    bool IsBackgroundIBD(CChainState* chainstate) const;
+
+    //! Return the most-work chainstate that has been fully validated.
+    //!
+    //! During background validation of a snapshot, this is the IBD chain. After
+    //! background validation has completed, this is the snapshot chain.
+    CChainState& ValidatedChainstate() const;
+
+    CChain& ValidatedChain() const { return ValidatedChainstate().m_chain; }
+    CBlockIndex* ValidatedTip() const { return ValidatedChain().Tip(); }
+
+    //! Unload block index and chain data before shutdown.
+    void Unload() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+    //! Clear (deconstruct) chainstate data.
+    void Reset();
+};
+
 /** @returns the most-work valid chainstate. */
 CChainState& ChainstateActive();
 
-- 
cgit v1.2.3


From 5b690f0aae21e7d46cbefe3f5be645842ac4ae3b Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Thu, 12 Dec 2019 10:28:14 -0500
Subject: refactor: move RewindBlockIndex to CChainState

This is in preparation for multiple chainstate initialization in init.
---
 src/init.cpp       |  2 +-
 src/validation.cpp | 27 ++++++++-------------------
 src/validation.h   |  3 ---
 3 files changed, 9 insertions(+), 23 deletions(-)

(limited to 'src')

diff --git a/src/init.cpp b/src/init.cpp
index 97640b0658..765c61b3db 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1614,7 +1614,7 @@ bool AppInitMain(NodeContext& node)
                 // It both disconnects blocks based on ::ChainActive(), and drops block data in
                 // BlockIndex() based on lack of available witness data.
                 uiInterface.InitMessage(_("Rewinding blocks...").translated);
-                if (!RewindBlockIndex(chainparams)) {
+                if (!::ChainstateActive().RewindBlockIndex(chainparams)) {
                     strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain").translated;
                     break;
                 }
diff --git a/src/validation.cpp b/src/validation.cpp
index a6b1344792..9d4cdf6eb9 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4538,26 +4538,15 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
             PruneBlockIndexCandidates();
 
             CheckBlockIndex(params.GetConsensus());
-        }
-    }
-
-    return true;
-}
-
-bool RewindBlockIndex(const CChainParams& params) {
-    if (!::ChainstateActive().RewindBlockIndex(params)) {
-        return false;
-    }
 
-    LOCK(cs_main);
-    if (::ChainActive().Tip() != nullptr) {
-        // FlushStateToDisk can possibly read ::ChainActive(). Be conservative
-        // and skip it here, we're about to -reindex-chainstate anyway, so
-        // it'll get called a bunch real soon.
-        BlockValidationState state;
-        if (!::ChainstateActive().FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
-            LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
-            return false;
+            // FlushStateToDisk can possibly read ::ChainActive(). Be conservative
+            // and skip it here, we're about to -reindex-chainstate anyway, so
+            // it'll get called a bunch real soon.
+            BlockValidationState state;
+            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
+                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
+                return false;
+            }
         }
     }
 
diff --git a/src/validation.h b/src/validation.h
index 35e4380bc0..3b160ac799 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -380,9 +380,6 @@ bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainpar
  *  Note that transaction witness validation rules are always enforced when P2SH is enforced. */
 bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& params);
 
-/** When there are blocks in the active chain with missing data, rewind the chainstate and remove them from the block index */
-bool RewindBlockIndex(const CChainParams& params) LOCKS_EXCLUDED(cs_main);
-
 /** Compute at which vout of the block's coinbase transaction the witness commitment occurs, or -1 if not found */
 int GetWitnessCommitmentIndex(const CBlock& block);
 
-- 
cgit v1.2.3


From 4ae29f5f0c5117032debb722d7049664fdceeae8 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Thu, 12 Dec 2019 10:48:28 -0500
Subject: use ChainstateManager to initialize chainstate

This allows us to easily initialize multiple chainstates on startup in future
commits. It retires the g_chainstate global in lieu of g_chainman.
---
 src/init.cpp                   | 193 +++++++++++++++++++++++++----------------
 src/rpc/blockchain.cpp         |   2 +-
 src/test/util/setup_common.cpp |   4 +-
 src/validation.cpp             |  68 +++++++--------
 src/validation.h               |  20 +++--
 5 files changed, 165 insertions(+), 122 deletions(-)

(limited to 'src')

diff --git a/src/init.cpp b/src/init.cpp
index 765c61b3db..56b63c8101 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -233,13 +233,12 @@ void Shutdown(NodeContext& node)
     }
 
     // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing
-    //
-    // g_chainstate is referenced here directly (instead of ::ChainstateActive()) because it
-    // may not have been initialized yet.
     {
         LOCK(cs_main);
-        if (g_chainstate && g_chainstate->CanFlushToDisk()) {
-            g_chainstate->ForceFlushStateToDisk();
+        for (CChainState* chainstate : g_chainman.GetAll()) {
+            if (chainstate->CanFlushToDisk()) {
+                chainstate->ForceFlushStateToDisk();
+            }
         }
     }
 
@@ -263,9 +262,11 @@ void Shutdown(NodeContext& node)
 
     {
         LOCK(cs_main);
-        if (g_chainstate && g_chainstate->CanFlushToDisk()) {
-            g_chainstate->ForceFlushStateToDisk();
-            g_chainstate->ResetCoinsViews();
+        for (CChainState* chainstate : g_chainman.GetAll()) {
+            if (chainstate->CanFlushToDisk()) {
+                chainstate->ForceFlushStateToDisk();
+                chainstate->ResetCoinsViews();
+            }
         }
         pblocktree.reset();
     }
@@ -1502,17 +1503,18 @@ bool AppInitMain(NodeContext& node)
     bool fLoaded = false;
     while (!fLoaded && !ShutdownRequested()) {
         bool fReset = fReindex;
+        auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+            return fReset || fReindexChainState || chainstate->CoinsTip().GetBestBlock().IsNull();
+        };
         std::string strLoadError;
 
         uiInterface.InitMessage(_("Loading block index...").translated);
 
         do {
             const int64_t load_block_index_start_time = GetTimeMillis();
-            bool is_coinsview_empty;
             try {
                 LOCK(cs_main);
-                // This statement makes ::ChainstateActive() usable.
-                g_chainstate = MakeUnique<CChainState>();
+                g_chainman.InitializeChainstate();
                 UnloadBlockIndex();
 
                 // new CBlockTreeDB tries to delete the existing file, which
@@ -1565,43 +1567,53 @@ bool AppInitMain(NodeContext& node)
                 // At this point we're either in reindex or we've loaded a useful
                 // block tree into BlockIndex()!
 
-                ::ChainstateActive().InitCoinsDB(
-                    /* cache_size_bytes */ nCoinDBCache,
-                    /* in_memory */ false,
-                    /* should_wipe */ fReset || fReindexChainState);
-
-                ::ChainstateActive().CoinsErrorCatcher().AddReadErrCallback([]() {
-                    uiInterface.ThreadSafeMessageBox(
-                        _("Error reading from database, shutting down.").translated,
-                        "", CClientUIInterface::MSG_ERROR);
-                });
-
-                // If necessary, upgrade from older database format.
-                // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
-                if (!::ChainstateActive().CoinsDB().Upgrade()) {
-                    strLoadError = _("Error upgrading chainstate database").translated;
-                    break;
-                }
-
-                // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
-                if (!::ChainstateActive().ReplayBlocks(chainparams)) {
-                    strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.").translated;
-                    break;
-                }
-
-                // The on-disk coinsdb is now in a good state, create the cache
-                ::ChainstateActive().InitCoinsCache();
-                assert(::ChainstateActive().CanFlushToDisk());
+                bool failed_chainstate_init = false;
+
+                for (CChainState* chainstate : g_chainman.GetAll()) {
+                    LogPrintf("Initializing chainstate %s\n", chainstate->ToString());
+                    chainstate->InitCoinsDB(
+                        /* cache_size_bytes */ nCoinDBCache,
+                        /* in_memory */ false,
+                        /* should_wipe */ fReset || fReindexChainState);
+
+                    chainstate->CoinsErrorCatcher().AddReadErrCallback([]() {
+                        uiInterface.ThreadSafeMessageBox(
+                            _("Error reading from database, shutting down.").translated,
+                            "", CClientUIInterface::MSG_ERROR);
+                    });
+
+                    // If necessary, upgrade from older database format.
+                    // This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
+                    if (!chainstate->CoinsDB().Upgrade()) {
+                        strLoadError = _("Error upgrading chainstate database").translated;
+                        failed_chainstate_init = true;
+                        break;
+                    }
 
-                is_coinsview_empty = fReset || fReindexChainState ||
-                    ::ChainstateActive().CoinsTip().GetBestBlock().IsNull();
-                if (!is_coinsview_empty) {
-                    // LoadChainTip initializes the chain based on CoinsTip()'s best block
-                    if (!::ChainstateActive().LoadChainTip(chainparams)) {
-                        strLoadError = _("Error initializing block database").translated;
+                    // ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
+                    if (!chainstate->ReplayBlocks(chainparams)) {
+                        strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.").translated;
+                        failed_chainstate_init = true;
                         break;
                     }
-                    assert(::ChainActive().Tip() != nullptr);
+
+                    // The on-disk coinsdb is now in a good state, create the cache
+                    chainstate->InitCoinsCache();
+                    assert(chainstate->CanFlushToDisk());
+
+                    if (!is_coinsview_empty(chainstate)) {
+                        // LoadChainTip initializes the chain based on CoinsTip()'s best block
+                        if (!chainstate->LoadChainTip(chainparams)) {
+                            strLoadError = _("Error initializing block database").translated;
+                            failed_chainstate_init = true;
+                            break; // out of the per-chainstate loop
+                        }
+                        assert(chainstate->m_chain.Tip() != nullptr);
+                    }
+                }
+
+                if (failed_chainstate_init) {
+                    break; // out of the chainstate activation do-while
                 }
             } catch (const std::exception& e) {
                 LogPrintf("%s\n", e.what());
@@ -1609,49 +1621,75 @@ bool AppInitMain(NodeContext& node)
                 break;
             }
 
-            if (!fReset) {
-                // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
-                // It both disconnects blocks based on ::ChainActive(), and drops block data in
-                // BlockIndex() based on lack of available witness data.
-                uiInterface.InitMessage(_("Rewinding blocks...").translated);
-                if (!::ChainstateActive().RewindBlockIndex(chainparams)) {
-                    strLoadError = _("Unable to rewind the database to a pre-fork state. You will need to redownload the blockchain").translated;
-                    break;
+            bool failed_rewind{false};
+
+            for (CChainState* chainstate : g_chainman.GetAll()) {
+                if (!fReset) {
+                    // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
+                    // It both disconnects blocks based on the chainstate, and drops block data in
+                    // BlockIndex() based on lack of available witness data.
+                    uiInterface.InitMessage(_("Rewinding blocks...").translated);
+                    if (!chainstate->RewindBlockIndex(chainparams)) {
+                        strLoadError = _(
+                            "Unable to rewind the database to a pre-fork state. "
+                            "You will need to redownload the blockchain").translated;
+                        failed_rewind = true;
+                        break; // out of the per-chainstate loop
+                    }
                 }
             }
 
+            if (failed_rewind) {
+                break; // out of the chainstate activation do-while
+            }
+
+            bool failed_verification = false;
+
             try {
                 LOCK(cs_main);
-                if (!is_coinsview_empty) {
-                    uiInterface.InitMessage(_("Verifying blocks...").translated);
-                    if (fHavePruned && gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) {
-                        LogPrintf("Prune: pruned datadir may not have more than %d blocks; only checking available blocks\n",
-                            MIN_BLOCKS_TO_KEEP);
-                    }
 
-                    CBlockIndex* tip = ::ChainActive().Tip();
-                    RPCNotifyBlockChange(true, tip);
-                    if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
-                        strLoadError = _("The block database contains a block which appears to be from the future. "
-                                "This may be due to your computer's date and time being set incorrectly. "
-                                "Only rebuild the block database if you are sure that your computer's date and time are correct").translated;
-                        break;
-                    }
-
-                    if (!CVerifyDB().VerifyDB(chainparams, &::ChainstateActive().CoinsDB(), gArgs.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
-                                  gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) {
-                        strLoadError = _("Corrupted block database detected").translated;
-                        break;
+                for (CChainState* chainstate : g_chainman.GetAll()) {
+                    if (!is_coinsview_empty(chainstate)) {
+                        uiInterface.InitMessage(_("Verifying blocks...").translated);
+                        if (fHavePruned && gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) {
+                            LogPrintf("Prune: pruned datadir may not have more than %d blocks; only checking available blocks\n",
+                                MIN_BLOCKS_TO_KEEP);
+                        }
+
+                        const CBlockIndex* tip = chainstate->m_chain.Tip();
+                        RPCNotifyBlockChange(true, tip);
+                        if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
+                            strLoadError = _("The block database contains a block which appears to be from the future. "
+                                    "This may be due to your computer's date and time being set incorrectly. "
+                                    "Only rebuild the block database if you are sure that your computer's date and time are correct").translated;
+                            failed_verification = true;
+                            break;
+                        }
+
+                        // Only verify the DB of the active chainstate. This is fixed in later
+                        // work when we allow VerifyDB to be parameterized by chainstate.
+                        if (&::ChainstateActive() == chainstate &&
+                                !CVerifyDB().VerifyDB(
+                                chainparams, &chainstate->CoinsDB(),
+                                gArgs.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
+                                gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) {
+                            strLoadError = _("Corrupted block database detected").translated;
+                            failed_verification = true;
+                            break;
+                        }
                     }
                 }
             } catch (const std::exception& e) {
                 LogPrintf("%s\n", e.what());
                 strLoadError = _("Error opening block database").translated;
+                failed_verification = true;
                 break;
             }
 
-            fLoaded = true;
-            LogPrintf(" block index %15dms\n", GetTimeMillis() - load_block_index_start_time);
+            if (!failed_verification) {
+                fLoaded = true;
+                LogPrintf(" block index %15dms\n", GetTimeMillis() - load_block_index_start_time);
+            }
         } while(false);
 
         if (!fLoaded && !ShutdownRequested()) {
@@ -1715,8 +1753,11 @@ bool AppInitMain(NodeContext& node)
         LogPrintf("Unsetting NODE_NETWORK on prune mode\n");
         nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK);
         if (!fReindex) {
-            uiInterface.InitMessage(_("Pruning blockstore...").translated);
-            ::ChainstateActive().PruneAndFlush();
+            LOCK(cs_main);
+            for (CChainState* chainstate : g_chainman.GetAll()) {
+                uiInterface.InitMessage(_("Pruning blockstore...").translated);
+                chainstate->PruneAndFlush();
+            }
         }
     }
 
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 256876a3d1..9de2e2e5a1 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1319,7 +1319,7 @@ static UniValue getchaintips(const JSONRPCRequest& request)
     /*
      * Idea:  the set of chain tips is ::ChainActive().tip, plus orphan blocks which do not have another orphan building off of them.
      * Algorithm:
-     *  - Make one pass through g_blockman.m_block_index, picking out the orphan blocks, and also storing a set of the orphan block's pprev pointers.
+     *  - Make one pass through BlockIndex(), picking out the orphan blocks, and also storing a set of the orphan block's pprev pointers.
      *  - Iterate through the orphan blocks. If the block isn't pointed to by another orphan, it is a chain tip.
      *  - add ::ChainActive().Tip()
      */
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index e19a96eafc..dacdce0acb 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -111,7 +111,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
     GetMainSignals().RegisterBackgroundSignalScheduler(*g_rpc_node->scheduler);
 
     pblocktree.reset(new CBlockTreeDB(1 << 20, true));
-    g_chainstate = MakeUnique<CChainState>();
+    g_chainman.InitializeChainstate();
     ::ChainstateActive().InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
     assert(!::ChainstateActive().CanFlushToDisk());
@@ -153,7 +153,7 @@ TestingSetup::~TestingSetup()
     m_node.mempool = nullptr;
     m_node.scheduler.reset();
     UnloadBlockIndex();
-    g_chainstate.reset();
+    g_chainman.Reset();
     pblocktree.reset();
 }
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 9d4cdf6eb9..77b6e7c051 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -77,20 +77,17 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
     return false;
 }
 
-namespace {
-BlockManager g_blockman;
-} // anon namespace
-
-std::unique_ptr<CChainState> g_chainstate;
+ChainstateManager g_chainman;
 
-CChainState& ChainstateActive() {
-    assert(g_chainstate);
-    return *g_chainstate;
+CChainState& ChainstateActive()
+{
+    assert(g_chainman.m_active_chainstate);
+    return *g_chainman.m_active_chainstate;
 }
 
-CChain& ChainActive() {
-    assert(g_chainstate);
-    return g_chainstate->m_chain;
+CChain& ChainActive()
+{
+    return ::ChainstateActive().m_chain;
 }
 
 /**
@@ -152,8 +149,8 @@ namespace {
 CBlockIndex* LookupBlockIndex(const uint256& hash)
 {
     AssertLockHeld(cs_main);
-    BlockMap::const_iterator it = g_blockman.m_block_index.find(hash);
-    return it == g_blockman.m_block_index.end() ? nullptr : it->second;
+    BlockMap::const_iterator it = g_chainman.BlockIndex().find(hash);
+    return it == g_chainman.BlockIndex().end() ? nullptr : it->second;
 }
 
 CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator)
@@ -1243,13 +1240,10 @@ void CoinsViews::InitCache()
     m_cacheview = MakeUnique<CCoinsViewCache>(&m_catcherview);
 }
 
-// NOTE: for now m_blockman is set to a global, but this will be changed
-// in a future commit.
-CChainState::CChainState(uint256 from_snapshot_blockhash)
-    : m_blockman(g_blockman),
+CChainState::CChainState(BlockManager& blockman, uint256 from_snapshot_blockhash)
+    : m_blockman(blockman),
       m_from_snapshot_blockhash(from_snapshot_blockhash) {}
 
-
 void CChainState::InitCoinsDB(
     size_t cache_size_bytes,
     bool in_memory,
@@ -1301,7 +1295,7 @@ static CBlockIndex *pindexBestForkTip = nullptr, *pindexBestForkBase = nullptr;
 
 BlockMap& BlockIndex()
 {
-    return g_blockman.m_block_index;
+    return g_chainman.m_blockman.m_block_index;
 }
 
 static void AlertNotify(const std::string& strMessage)
@@ -3471,7 +3465,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
     if (fCheckpointsEnabled) {
         // Don't accept any forks from the main chain prior to last checkpoint.
         // GetLastCheckpoint finds the last checkpoint in MapCheckpoints that's in our
-        // g_blockman.m_block_index.
+        // BlockIndex().
         CBlockIndex* pcheckpoint = GetLastCheckpoint(params.Checkpoints());
         if (pcheckpoint && nHeight < pcheckpoint->nHeight) {
             LogPrintf("ERROR: %s: forked chain older than last checkpoint (height %d)\n", __func__, nHeight);
@@ -3679,7 +3673,8 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, BlockValid
         LOCK(cs_main);
         for (const CBlockHeader& header : headers) {
             CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
-            bool accepted = g_blockman.AcceptBlockHeader(header, state, chainparams, &pindex);
+            bool accepted = g_chainman.m_blockman.AcceptBlockHeader(
+                header, state, chainparams, &pindex);
             ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus());
 
             if (!accepted) {
@@ -3881,7 +3876,7 @@ void PruneOneBlockFile(const int fileNumber)
 {
     LOCK(cs_LastBlockFile);
 
-    for (const auto& entry : g_blockman.m_block_index) {
+    for (const auto& entry : g_chainman.BlockIndex()) {
         CBlockIndex* pindex = entry.second;
         if (pindex->nFile == fileNumber) {
             pindex->nStatus &= ~BLOCK_HAVE_DATA;
@@ -3895,12 +3890,12 @@ void PruneOneBlockFile(const int fileNumber)
             // to be downloaded again in order to consider its chain, at which
             // point it would be considered as a candidate for
             // m_blocks_unlinked or setBlockIndexCandidates.
-            auto range = g_blockman.m_blocks_unlinked.equal_range(pindex->pprev);
+            auto range = g_chainman.m_blockman.m_blocks_unlinked.equal_range(pindex->pprev);
             while (range.first != range.second) {
                 std::multimap<CBlockIndex *, CBlockIndex *>::iterator _it = range.first;
                 range.first++;
                 if (_it->second == pindex) {
-                    g_blockman.m_blocks_unlinked.erase(_it);
+                    g_chainman.m_blockman.m_blocks_unlinked.erase(_it);
                 }
             }
         }
@@ -4137,9 +4132,11 @@ void BlockManager::Unload() {
 
 bool static LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
-    if (!g_blockman.LoadBlockIndex(
-            chainparams.GetConsensus(), *pblocktree, ::ChainstateActive().setBlockIndexCandidates))
+    if (!g_chainman.m_blockman.LoadBlockIndex(
+            chainparams.GetConsensus(), *pblocktree,
+            ::ChainstateActive().setBlockIndexCandidates)) {
         return false;
+    }
 
     // Load block file info
     pblocktree->ReadLastBlockFile(nLastBlockFile);
@@ -4161,7 +4158,7 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_RE
     // Check presence of blk files
     LogPrintf("Checking all blk files are present...\n");
     std::set<int> setBlkDataFiles;
-    for (const std::pair<const uint256, CBlockIndex*>& item : g_blockman.m_block_index)
+    for (const std::pair<const uint256, CBlockIndex*>& item : g_chainman.BlockIndex())
     {
         CBlockIndex* pindex = item.second;
         if (pindex->nStatus & BLOCK_HAVE_DATA) {
@@ -4564,8 +4561,7 @@ void CChainState::UnloadBlockIndex() {
 void UnloadBlockIndex()
 {
     LOCK(cs_main);
-    ::ChainActive().SetTip(nullptr);
-    g_blockman.Unload();
+    g_chainman.Unload();
     pindexBestInvalid = nullptr;
     pindexBestHeader = nullptr;
     mempool.clear();
@@ -4578,8 +4574,6 @@ void UnloadBlockIndex()
         warningcache[b].clear();
     }
     fHavePruned = false;
-
-    ::ChainstateActive().UnloadBlockIndex();
 }
 
 bool LoadBlockIndex(const CChainParams& chainparams)
@@ -4589,7 +4583,7 @@ bool LoadBlockIndex(const CChainParams& chainparams)
     if (!fReindex) {
         bool ret = LoadBlockIndexDB(chainparams);
         if (!ret) return false;
-        needs_init = g_blockman.m_block_index.empty();
+        needs_init = g_chainman.m_blockman.m_block_index.empty();
     }
 
     if (needs_init) {
@@ -5135,10 +5129,10 @@ public:
     CMainCleanup() {}
     ~CMainCleanup() {
         // block headers
-        BlockMap::iterator it1 = g_blockman.m_block_index.begin();
-        for (; it1 != g_blockman.m_block_index.end(); it1++)
+        BlockMap::iterator it1 = g_chainman.BlockIndex().begin();
+        for (; it1 != g_chainman.BlockIndex().end(); it1++)
             delete (*it1).second;
-        g_blockman.m_block_index.clear();
+        g_chainman.BlockIndex().clear();
     }
 };
 static CMainCleanup instance_of_cmaincleanup;
@@ -5176,7 +5170,7 @@ CChainState& ChainstateManager::InitializeChainstate(const uint256& snapshot_blo
         throw std::logic_error("should not be overwriting a chainstate");
     }
 
-    to_modify.reset(new CChainState(snapshot_blockhash));
+    to_modify.reset(new CChainState(m_blockman, snapshot_blockhash));
 
     // Snapshot chainstates and initial IBD chaintates always become active.
     if (is_snapshot || (!is_snapshot && !m_active_chainstate)) {
@@ -5220,6 +5214,8 @@ void ChainstateManager::Unload()
         chainstate->m_chain.SetTip(nullptr);
         chainstate->UnloadBlockIndex();
     }
+
+    m_blockman.Unload();
 }
 
 void ChainstateManager::Reset()
diff --git a/src/validation.h b/src/validation.h
index 3b160ac799..901f6d22bd 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -592,8 +592,7 @@ private:
     std::unique_ptr<CoinsViews> m_coins_views;
 
 public:
-    explicit CChainState(BlockManager& blockman) : m_blockman(blockman) {}
-    explicit CChainState(uint256 from_snapshot_blockhash = uint256());
+    explicit CChainState(BlockManager& blockman, uint256 from_snapshot_blockhash = uint256());
 
     /**
      * Initialize the CoinsViews UTXO set database management data structures. The in-memory
@@ -839,9 +838,14 @@ private:
     bool m_snapshot_validated{false};
 
     // For access to m_active_chainstate.
+    friend CChainState& ChainstateActive();
     friend CChain& ChainActive();
 
 public:
+    //! A single BlockManager instance is shared across each constructed
+    //! chainstate to avoid duplicating block metadata.
+    BlockManager m_blockman GUARDED_BY(::cs_main);
+
     //! Instantiate a new chainstate and assign it based upon whether it is
     //! from a snapshot.
     //!
@@ -858,6 +862,11 @@ public:
     int ActiveHeight() const { return ActiveChain().Height(); }
     CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
 
+    BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        return m_blockman.m_block_index;
+    }
+
     bool IsSnapshotActive() const;
 
     Optional<uint256> SnapshotBlockhash() const;
@@ -885,6 +894,8 @@ public:
     void Reset();
 };
 
+extern ChainstateManager g_chainman;
+
 /** @returns the most-work valid chainstate. */
 CChainState& ChainstateActive();
 
@@ -894,11 +905,6 @@ CChain& ChainActive();
 /** @returns the global block index map. */
 BlockMap& BlockIndex();
 
-// Most often ::ChainstateActive() should be used instead of this, but some code
-// may not be able to assume that this has been initialized yet and so must use it
-// directly, e.g. init.cpp.
-extern std::unique_ptr<CChainState> g_chainstate;
-
 /** Global variable that points to the active block tree (protected by cs_main) */
 extern std::unique_ptr<CBlockTreeDB> pblocktree;
 
-- 
cgit v1.2.3


From 2b081c4568e8019886fdb0f2a57babc73d7487f7 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Thu, 12 Dec 2019 14:11:03 -0500
Subject: test: add basic tests for ChainstateManager

Feedback incorporated from Russell Yanofsky.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
---
 src/Makefile.test.include                       |   1 +
 src/qt/test/apptests.cpp                        |   1 +
 src/test/util/setup_common.cpp                  |   1 +
 src/test/validation_chainstatemanager_tests.cpp | 104 ++++++++++++++++++++++++
 4 files changed, 107 insertions(+)
 create mode 100644 src/test/validation_chainstatemanager_tests.cpp

(limited to 'src')

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 9697a157fb..9c0aa347d4 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -231,6 +231,7 @@ BITCOIN_TESTS =\
   test/uint256_tests.cpp \
   test/util_tests.cpp \
   test/validation_block_tests.cpp \
+  test/validation_chainstatemanager_tests.cpp \
   test/validation_flush_tests.cpp \
   test/versionbits_tests.cpp
 
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
index 14a75b23f3..f9eb4cde30 100644
--- a/src/qt/test/apptests.cpp
+++ b/src/qt/test/apptests.cpp
@@ -82,6 +82,7 @@ void AppTests::appTests()
     // Reset global state to avoid interfering with later tests.
     AbortShutdown();
     UnloadBlockIndex();
+    g_chainman.Reset();
 }
 
 //! Entry point for BitcoinGUI tests.
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index dacdce0acb..c77f59e050 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -111,6 +111,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
     GetMainSignals().RegisterBackgroundSignalScheduler(*g_rpc_node->scheduler);
 
     pblocktree.reset(new CBlockTreeDB(1 << 20, true));
+
     g_chainman.InitializeChainstate();
     ::ChainstateActive().InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
new file mode 100644
index 0000000000..6e7186db22
--- /dev/null
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -0,0 +1,104 @@
+// Copyright (c) 2019 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 <chainparams.h>
+#include <random.h>
+#include <uint256.h>
+#include <consensus/validation.h>
+#include <sync.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+#include <vector>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, TestingSetup)
+
+//! Basic tests for ChainstateManager.
+//!
+//! First create a legacy (IBD) chainstate, then create a snapshot chainstate.
+BOOST_AUTO_TEST_CASE(chainstatemanager)
+{
+    ChainstateManager manager;
+    std::vector<CChainState*> chainstates;
+    const CChainParams& chainparams = Params();
+
+    // Create a legacy (IBD) chainstate.
+    //
+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c1 = manager.InitializeChainstate();
+    LEAVE_CRITICAL_SECTION(cs_main);
+    chainstates.push_back(&c1);
+    c1.InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+    WITH_LOCK(::cs_main, c1.InitCoinsCache());
+
+    BOOST_CHECK(!manager.IsSnapshotActive());
+    BOOST_CHECK(!manager.IsSnapshotValidated());
+    BOOST_CHECK(!manager.IsBackgroundIBD(&c1));
+    auto all = manager.GetAll();
+    BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end());
+
+    auto& active_chain = manager.ActiveChain();
+    BOOST_CHECK_EQUAL(&active_chain, &c1.m_chain);
+
+    BOOST_CHECK_EQUAL(manager.ActiveHeight(), -1);
+
+    auto active_tip = manager.ActiveTip();
+    auto exp_tip = c1.m_chain.Tip();
+    BOOST_CHECK_EQUAL(active_tip, exp_tip);
+
+    auto& validated_cs = manager.ValidatedChainstate();
+    BOOST_CHECK_EQUAL(&validated_cs, &c1);
+
+    // Create a snapshot-based chainstate.
+    //
+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
+    LEAVE_CRITICAL_SECTION(cs_main);
+    chainstates.push_back(&c2);
+    c2.InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+    WITH_LOCK(::cs_main, c2.InitCoinsCache());
+    // Unlike c1, which doesn't have any blocks. Gets us different tip, height.
+    c2.LoadGenesisBlock(chainparams);
+    BlockValidationState _;
+    BOOST_CHECK(c2.ActivateBestChain(_, chainparams, nullptr));
+
+    BOOST_CHECK(manager.IsSnapshotActive());
+    BOOST_CHECK(!manager.IsSnapshotValidated());
+    BOOST_CHECK(manager.IsBackgroundIBD(&c1));
+    BOOST_CHECK(!manager.IsBackgroundIBD(&c2));
+    auto all2 = manager.GetAll();
+    BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end());
+
+    auto& active_chain2 = manager.ActiveChain();
+    BOOST_CHECK_EQUAL(&active_chain2, &c2.m_chain);
+
+    BOOST_CHECK_EQUAL(manager.ActiveHeight(), 0);
+
+    auto active_tip2 = manager.ActiveTip();
+    auto exp_tip2 = c2.m_chain.Tip();
+    BOOST_CHECK_EQUAL(active_tip2, exp_tip2);
+
+    // Ensure that these pointers actually correspond to different
+    // CCoinsViewCache instances.
+    BOOST_CHECK(exp_tip != exp_tip2);
+
+    auto& validated_cs2 = manager.ValidatedChainstate();
+    BOOST_CHECK_EQUAL(&validated_cs2, &c1);
+
+    auto& validated_chain = manager.ValidatedChain();
+    BOOST_CHECK_EQUAL(&validated_chain, &c1.m_chain);
+
+    auto validated_tip = manager.ValidatedTip();
+    exp_tip = c1.m_chain.Tip();
+    BOOST_CHECK_EQUAL(validated_tip, exp_tip);
+
+    // Avoid triggering the address sanitizer.
+    WITH_LOCK(::cs_main, manager.Unload());
+}
+
+BOOST_AUTO_TEST_SUITE_END()
-- 
cgit v1.2.3


From c9017ce3bc27665594c9d80f395780d40755bb22 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@pm.me>
Date: Wed, 29 Jan 2020 09:57:56 -0500
Subject: protect g_chainman with cs_main

I'd previously attempted to create a specialized lock for ChainstateManager,
but it turns out that because that lock would be required for functions like
ChainActive() and ChainstateActive(), it created irreconcilable lock inversions
since those functions are used so broadly throughout the codebase.

Instead, I'm just using cs_main to protect the contents of g_chainman.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
---
 src/init.cpp                     | 21 ++++++++++++++-------
 src/qt/test/apptests.cpp         |  2 +-
 src/validation.cpp               |  5 ++++-
 src/validation.h                 | 29 ++++++++++++++++++++++++++++-
 src/wallet/test/wallet_tests.cpp |  2 +-
 5 files changed, 48 insertions(+), 11 deletions(-)

(limited to 'src')

diff --git a/src/init.cpp b/src/init.cpp
index 56b63c8101..f0ececbb39 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -709,11 +709,17 @@ static void ThreadImport(std::vector<fs::path> vImportFiles)
     }
 
     // scan for better chains in the block chain database, that are not yet connected in the active best chain
-    BlockValidationState state;
-    if (!ActivateBestChain(state, chainparams)) {
-        LogPrintf("Failed to connect best block (%s)\n", state.ToString());
-        StartShutdown();
-        return;
+
+    // We can't hold cs_main during ActivateBestChain even though we're accessing
+    // the g_chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
+    // the relevant pointers before the ABC call.
+    for (CChainState* chainstate : WITH_LOCK(::cs_main, return g_chainman.GetAll())) {
+        BlockValidationState state;
+        if (!chainstate->ActivateBestChain(state, chainparams, nullptr)) {
+            LogPrintf("Failed to connect best block (%s)\n", state.ToString());
+            StartShutdown();
+            return;
+        }
     }
 
     if (gArgs.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
@@ -1622,8 +1628,9 @@ bool AppInitMain(NodeContext& node)
             }
 
             bool failed_rewind{false};
-
-            for (CChainState* chainstate : g_chainman.GetAll()) {
+            // Can't hold cs_main while calling RewindBlockIndex, so retrieve the relevant
+            // chainstates beforehand.
+            for (CChainState* chainstate : WITH_LOCK(::cs_main, return g_chainman.GetAll())) {
                 if (!fReset) {
                     // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
                     // It both disconnects blocks based on the chainstate, and drops block data in
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
index f9eb4cde30..064b9ceb18 100644
--- a/src/qt/test/apptests.cpp
+++ b/src/qt/test/apptests.cpp
@@ -82,7 +82,7 @@ void AppTests::appTests()
     // Reset global state to avoid interfering with later tests.
     AbortShutdown();
     UnloadBlockIndex();
-    g_chainman.Reset();
+    WITH_LOCK(::cs_main, g_chainman.Reset());
 }
 
 //! Entry point for BitcoinGUI tests.
diff --git a/src/validation.cpp b/src/validation.cpp
index 77b6e7c051..84b93180d6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -81,12 +81,14 @@ ChainstateManager g_chainman;
 
 CChainState& ChainstateActive()
 {
+    LOCK(::cs_main);
     assert(g_chainman.m_active_chainstate);
     return *g_chainman.m_active_chainstate;
 }
 
 CChain& ChainActive()
 {
+    LOCK(::cs_main);
     return ::ChainstateActive().m_chain;
 }
 
@@ -1295,6 +1297,7 @@ static CBlockIndex *pindexBestForkTip = nullptr, *pindexBestForkBase = nullptr;
 
 BlockMap& BlockIndex()
 {
+    LOCK(::cs_main);
     return g_chainman.m_blockman.m_block_index;
 }
 
@@ -4704,7 +4707,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi
                 // Activate the genesis block so normal node progress can continue
                 if (hash == chainparams.GetConsensus().hashGenesisBlock) {
                     BlockValidationState state;
-                    if (!ActivateBestChain(state, chainparams)) {
+                    if (!ActivateBestChain(state, chainparams, nullptr)) {
                         break;
                     }
                 }
diff --git a/src/validation.h b/src/validation.h
index 901f6d22bd..dbf7aa28db 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -823,14 +823,41 @@ private:
     //! free the chainstate contents immediately after it finishes validation
     //! to cautiously avoid a case where some other part of the system is still
     //! using this pointer (e.g. net_processing).
+    //!
+    //! Once this pointer is set to a corresponding chainstate, it will not
+    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire
+    //! the contents of this pointer with ::cs_main held, release the lock,
+    //! and then use the reference without concern of it being deconstructed.
+    //!
+    //! This is especially important when, e.g., calling ActivateBestChain()
+    //! on all chainstates because we are not able to hold ::cs_main going into
+    //! that call.
     std::unique_ptr<CChainState> m_ibd_chainstate;
 
     //! A chainstate initialized on the basis of a UTXO snapshot. If this is
     //! non-null, it is always our active chainstate.
+    //!
+    //! Once this pointer is set to a corresponding chainstate, it will not
+    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire
+    //! the contents of this pointer with ::cs_main held, release the lock,
+    //! and then use the reference without concern of it being deconstructed.
+    //!
+    //! This is especially important when, e.g., calling ActivateBestChain()
+    //! on all chainstates because we are not able to hold ::cs_main going into
+    //! that call.
     std::unique_ptr<CChainState> m_snapshot_chainstate;
 
     //! Points to either the ibd or snapshot chainstate; indicates our
     //! most-work chain.
+    //!
+    //! Once this pointer is set to a corresponding chainstate, it will not
+    //! be reset until init.cpp:Shutdown(). This means it is safe to acquire
+    //! the contents of this pointer with ::cs_main held, release the lock,
+    //! and then use the reference without concern of it being deconstructed.
+    //!
+    //! This is especially important when, e.g., calling ActivateBestChain()
+    //! on all chainstates because we are not able to hold ::cs_main going into
+    //! that call.
     CChainState* m_active_chainstate{nullptr};
 
     //! If true, the assumed-valid chainstate has been fully validated
@@ -894,7 +921,7 @@ public:
     void Reset();
 };
 
-extern ChainstateManager g_chainman;
+extern ChainstateManager g_chainman GUARDED_BY(::cs_main);
 
 /** @returns the most-work valid chainstate. */
 CChainState& ChainstateActive();
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index a487e9e2e0..13e10c1eab 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -454,7 +454,7 @@ public:
         CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
         wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock());
         {
-            LOCK(wallet->cs_wallet);
+            LOCK2(::cs_main, wallet->cs_wallet);
             wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
         }
         bool firstRun;
-- 
cgit v1.2.3