Merge #21025: validation: Guard chainman chainstates with cs_main

20677ffa22e93e7408daadbd15d433f1e42faa86 validation: Guard all chainstates with cs_main (Carl Dong)

Pull request description:

  ```
  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. https://github.com/bitcoin/bitcoin/pull/20749#discussion_r559544027
  2. https://github.com/bitcoin/bitcoin/pull/19806#discussion_r561023961
  3. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768946522
  4. https://github.com/bitcoin/bitcoin/pull/19806#issuecomment-768955695
  ```

  Basically this PR removes the loaded-but-unfired footgun, which:
  - Is multiplied (but still unshot) in the chainman deglobalization PRs (#20158)
  - Is shot in the test framework in the au.activate PR (#19806)

ACKs for top commit:
  jnewbery:
    code review ACK 20677ffa22e93e7408daadbd15d433f1e42faa86. I've verified by eye that neither of these members are accessed without cs_main.
  ryanofsky:
    Code review ACK 20677ffa22e93e7408daadbd15d433f1e42faa86. It is safer to have these new `GUARDED_BY` annotations and locks than not to have them, but in the longer run I think every `LOCK(cs_main)` added here and added earlier in f92dc6557a153b390a1ae1d0808ff7ed5d02c66e from #20749 should be removed and replaced with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` on the accessor methods instead. `cs_main` is a high level lock that should be explicitly acquired at a high level to prevent the chain state from changing. It shouldn't be acquired recursively in low-level methods just to read pointer values atomically.

Tree-SHA512: 68a3a46d79a407b774eab77e1d682a97e95f1672db0a5fcb877572e188bec09f3a7b47c5d0cc1f2769ea276896dcbe97cb35c861acf7d8e3e513e955dc773f89

2 files changed, 10 insertions, 8 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index 38df71b994..8f7d36bfd3 100644
--- 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
index fc7add85b7..e85c7bbf1a 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -802,7 +802,7 @@ private:
     //! 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 @@ private:
     //! 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.