validation: Guard the active_chainstate with cs_main

dongcarl · dongcarl · commit f92dc6557a15 · 2021-01-28T14:14:22.000-05:00
This avoids a potential race-condition where a thread is reading the ChainstateManager::m_active_chainstate pointer while another one is writing to it. There is no portable guarantee that reading/writing the pointer is thread-safe. This is also done in way that mimics ::ChainstateActive(), so the transition from that function to this method is easy. More discussion: 1. #20749 (comment) 2. #19806 (comment) 3. #19806 (comment) 4. #19806 (comment)
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -5143,6 +5143,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
 }
 
 Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
+    LOCK(::cs_main);  // for m_active_chainstate access
     if (m_active_chainstate != nullptr) {
         // If a snapshot chainstate exists, it will always be our active.
         return m_active_chainstate->m_from_snapshot_blockhash;
@@ -5189,13 +5190,14 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
 
 CChainState& ChainstateManager::ActiveChainstate() const
 {
+    LOCK(::cs_main);
     assert(m_active_chainstate);
     return *m_active_chainstate;
 }
 
 bool ChainstateManager::IsSnapshotActive() const
 {
-    return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();
+    return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();
 }
 
 CChainState& ChainstateManager::ValidatedChainstate() const
@@ -5226,7 +5228,10 @@ void ChainstateManager::Reset()
 {
     m_ibd_chainstate.reset();
     m_snapshot_chainstate.reset();
-    m_active_chainstate = nullptr;
+    {
+        LOCK(::cs_main);
+        m_active_chainstate = nullptr;
+    }
     m_snapshot_validated = false;
 }
 
diff --git a/src/validation.h b/src/validation.h
@@ -817,7 +817,7 @@ class ChainstateManager
     //! 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};
+    CChainState* m_active_chainstate GUARDED_BY(::cs_main) {nullptr};
 
     //! If true, the assumed-valid chainstate has been fully validated
     //! by the background validation chainstate.

Original file line number	Diff line number	Diff line change
`@@ -5143,6 +5143,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin`
`5143`	`5143`	`}`
`5144`	`5144`
`5145`	`5145`	`Optional<uint256> ChainstateManager::SnapshotBlockhash() const {`
	`5146`	`+ LOCK(::cs_main); // for m_active_chainstate access`
`5146`	`5147`	`if (m_active_chainstate != nullptr) {`
`5147`	`5148`	`// If a snapshot chainstate exists, it will always be our active.`
`5148`	`5149`	`return m_active_chainstate->m_from_snapshot_blockhash;`
`@@ -5189,13 +5190,14 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const`
`5189`	`5190`
`5190`	`5191`	`CChainState& ChainstateManager::ActiveChainstate() const`
`5191`	`5192`	`{`
	`5193`	`+ LOCK(::cs_main);`
`5192`	`5194`	`assert(m_active_chainstate);`
`5193`	`5195`	`return *m_active_chainstate;`
`5194`	`5196`	`}`
`5195`	`5197`
`5196`	`5198`	`bool ChainstateManager::IsSnapshotActive() const`
`5197`	`5199`	`{`
`5198`		`- return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();`
	`5200`	`+ return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();`
`5199`	`5201`	`}`
`5200`	`5202`
`5201`	`5203`	`CChainState& ChainstateManager::ValidatedChainstate() const`
`@@ -5226,7 +5228,10 @@ void ChainstateManager::Reset()`
`5226`	`5228`	`{`
`5227`	`5229`	`m_ibd_chainstate.reset();`
`5228`	`5230`	`m_snapshot_chainstate.reset();`
`5229`		`- m_active_chainstate = nullptr;`
	`5231`	`+ {`
	`5232`	`+ LOCK(::cs_main);`
	`5233`	`+ m_active_chainstate = nullptr;`
	`5234`	`+ }`
`5230`	`5235`	`m_snapshot_validated = false;`
`5231`	`5236`	`}`
`5232`	`5237`