protect g_chainman with cs_main

jamesob · ryanofsky · jamesob · commit c9017ce3bc27 · 2020-03-17T14:07:58.000-04:00
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 &lt;russ@yanofsky.org&gt;
diff --git 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
@@ -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
@@ -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
@@ -823,14 +823,41 @@ class ChainstateManager
     //! 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 @@ class ChainstateManager
     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
@@ -454,7 +454,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
         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;

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ void AppTests::appTests()`
`82`	`82`	`// Reset global state to avoid interfering with later tests.`
`83`	`83`	`AbortShutdown();`
`84`	`84`	`UnloadBlockIndex();`
`85`		`- g_chainman.Reset();`
	`85`	`+ WITH_LOCK(::cs_main, g_chainman.Reset());`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`//! Entry point for BitcoinGUI tests.`
Original file line number	Diff line number	Diff line change
`@@ -81,12 +81,14 @@ ChainstateManager g_chainman;`
`81`	`81`
`82`	`82`	`CChainState& ChainstateActive()`
`83`	`83`	`{`
	`84`	`+ LOCK(::cs_main);`
`84`	`85`	`assert(g_chainman.m_active_chainstate);`
`85`	`86`	`return *g_chainman.m_active_chainstate;`
`86`	`87`	`}`
`87`	`88`
`88`	`89`	`CChain& ChainActive()`
`89`	`90`	`{`
	`91`	`+ LOCK(::cs_main);`
`90`	`92`	`return ::ChainstateActive().m_chain;`
`91`	`93`	`}`
`92`	`94`
`@@ -1295,6 +1297,7 @@ static CBlockIndex pindexBestForkTip = nullptr, pindexBestForkBase = nullptr;`
`1295`	`1297`
`1296`	`1298`	`BlockMap& BlockIndex()`
`1297`	`1299`	`{`
	`1300`	`+ LOCK(::cs_main);`
`1298`	`1301`	`return g_chainman.m_blockman.m_block_index;`
`1299`	`1302`	`}`
`1300`	`1303`
`@@ -4704,7 +4707,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi`
`4704`	`4707`	`// Activate the genesis block so normal node progress can continue`
`4705`	`4708`	`if (hash == chainparams.GetConsensus().hashGenesisBlock) {`
`4706`	`4709`	`BlockValidationState state;`
`4707`		`- if (!ActivateBestChain(state, chainparams)) {`
	`4710`	`+ if (!ActivateBestChain(state, chainparams, nullptr)) {`
`4708`	`4711`	`break;`
`4709`	`4712`	`}`
`4710`	`4713`	`}`
Original file line number	Diff line number	Diff line change
`@@ -454,7 +454,7 @@ class ListCoinsTestingSetup : public TestChain100Setup`
`454`	`454`	`CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));`
`455`	`455`	`wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock());`
`456`	`456`	`{`
`457`		`- LOCK(wallet->cs_wallet);`
	`457`	`+ LOCK2(::cs_main, wallet->cs_wallet);`
`458`	`458`	`wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());`
`459`	`459`	`}`
`460`	`460`	`bool firstRun;`