validation: Guard all chainstates with cs_main

dongcarl · dongcarl · commit 20677ffa22e9 · 2021-02-01T22:09:03.000-05:00
Since these chainstates are:

1. Also vulnerable to the race condition described in the previous
   commit
2. Documented as having similar semantics as m_active_chainstate

we should also protect them with ::cs_main.
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -5159,7 +5159,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
 }
 
 Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
-    LOCK(::cs_main);  // for m_active_chainstate access
+    LOCK(::cs_main);
     if (m_active_chainstate != nullptr) {
         // If a snapshot chainstate exists, it will always be our active.
         return m_active_chainstate->m_from_snapshot_blockhash;
@@ -5169,6 +5169,7 @@ Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
 
 std::vector<CChainState*> ChainstateManager::GetAll()
 {
+    LOCK(::cs_main);
     std::vector<CChainState*> out;
 
     if (!IsSnapshotValidated() && m_ibd_chainstate) {
@@ -5213,11 +5214,13 @@ CChainState& ChainstateManager::ActiveChainstate() const
 
 bool ChainstateManager::IsSnapshotActive() const
 {
-    return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();
+    LOCK(::cs_main);
+    return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();
 }
 
 CChainState& ChainstateManager::ValidatedChainstate() const
 {
+    LOCK(::cs_main);
     if (m_snapshot_chainstate && IsSnapshotValidated()) {
         return *m_snapshot_chainstate.get();
     }
@@ -5227,6 +5230,7 @@ CChainState& ChainstateManager::ValidatedChainstate() const
 
 bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const
 {
+    LOCK(::cs_main);
     return (m_snapshot_chainstate && chainstate == m_ibd_chainstate.get());
 }
 
@@ -5242,12 +5246,10 @@ void ChainstateManager::Unload()
 
 void ChainstateManager::Reset()
 {
+    LOCK(::cs_main);
     m_ibd_chainstate.reset();
     m_snapshot_chainstate.reset();
-    {
-        LOCK(::cs_main);
-        m_active_chainstate = nullptr;
-    }
+    m_active_chainstate = nullptr;
     m_snapshot_validated = false;
 }
 
diff --git a/src/validation.h b/src/validation.h
@@ -802,7 +802,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.
-    std::unique_ptr<CChainState> m_ibd_chainstate;
+    std::unique_ptr<CChainState> m_ibd_chainstate GUARDED_BY(::cs_main);
 
     //! A chainstate initialized on the basis of a UTXO snapshot. If this is
     //! non-null, it is always our active chainstate.
@@ -815,7 +815,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.
-    std::unique_ptr<CChainState> m_snapshot_chainstate;
+    std::unique_ptr<CChainState> m_snapshot_chainstate GUARDED_BY(::cs_main);
 
     //! Points to either the ibd or snapshot chainstate; indicates our
     //! most-work chain.

Original file line number	Diff line number	Diff line change
`@@ -5159,7 +5159,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin`
`5159`	`5159`	`}`
`5160`	`5160`
`5161`	`5161`	`Optional<uint256> ChainstateManager::SnapshotBlockhash() const {`
`5162`		`- LOCK(::cs_main); // for m_active_chainstate access`
	`5162`	`+ LOCK(::cs_main);`
`5163`	`5163`	`if (m_active_chainstate != nullptr) {`
`5164`	`5164`	`// If a snapshot chainstate exists, it will always be our active.`
`5165`	`5165`	`return m_active_chainstate->m_from_snapshot_blockhash;`
`@@ -5169,6 +5169,7 @@ Optional<uint256> ChainstateManager::SnapshotBlockhash() const {`
`5169`	`5169`
`5170`	`5170`	`std::vector<CChainState*> ChainstateManager::GetAll()`
`5171`	`5171`	`{`
	`5172`	`+ LOCK(::cs_main);`
`5172`	`5173`	`std::vector<CChainState*> out;`
`5173`	`5174`
`5174`	`5175`	`if (!IsSnapshotValidated() && m_ibd_chainstate) {`
`@@ -5213,11 +5214,13 @@ CChainState& ChainstateManager::ActiveChainstate() const`
`5213`	`5214`
`5214`	`5215`	`bool ChainstateManager::IsSnapshotActive() const`
`5215`	`5216`	`{`
`5216`		`- return m_snapshot_chainstate && WITH_LOCK(::cs_main, return m_active_chainstate) == m_snapshot_chainstate.get();`
	`5217`	`+ LOCK(::cs_main);`
	`5218`	`+ return m_snapshot_chainstate && m_active_chainstate == m_snapshot_chainstate.get();`
`5217`	`5219`	`}`
`5218`	`5220`
`5219`	`5221`	`CChainState& ChainstateManager::ValidatedChainstate() const`
`5220`	`5222`	`{`
	`5223`	`+ LOCK(::cs_main);`
`5221`	`5224`	`if (m_snapshot_chainstate && IsSnapshotValidated()) {`
`5222`	`5225`	`return *m_snapshot_chainstate.get();`
`5223`	`5226`	`}`
`@@ -5227,6 +5230,7 @@ CChainState& ChainstateManager::ValidatedChainstate() const`
`5227`	`5230`
`5228`	`5231`	`bool ChainstateManager::IsBackgroundIBD(CChainState* chainstate) const`
`5229`	`5232`	`{`
	`5233`	`+ LOCK(::cs_main);`
`5230`	`5234`	`return (m_snapshot_chainstate && chainstate == m_ibd_chainstate.get());`
`5231`	`5235`	`}`
`5232`	`5236`
`@@ -5242,12 +5246,10 @@ void ChainstateManager::Unload()`
`5242`	`5246`
`5243`	`5247`	`void ChainstateManager::Reset()`
`5244`	`5248`	`{`
	`5249`	`+ LOCK(::cs_main);`
`5245`	`5250`	`m_ibd_chainstate.reset();`
`5246`	`5251`	`m_snapshot_chainstate.reset();`
`5247`		`- {`
`5248`		`- LOCK(::cs_main);`
`5249`		`- m_active_chainstate = nullptr;`
`5250`		`- }`
	`5252`	`+ m_active_chainstate = nullptr;`
`5251`	`5253`	`m_snapshot_validated = false;`
`5252`	`5254`	`}`
`5253`	`5255`