From fa5c346c5a336ccdce8e50befb53746c280f053e Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Sat, 15 Dec 2018 11:25:05 +1300
Subject: [PATCH] doc: Add comment to cs_main and mempool::cs

---
 src/txmempool.h    | 38 +++++++++++++++++++++++++++++++++++++-
 src/validation.cpp | 14 +++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/src/txmempool.h b/src/txmempool.h
index fadb554723..9416885125 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -485,7 +485,43 @@ public:
         >
     > indexed_transaction_set;
 
-    mutable CCriticalSection cs;
+    /**
+     * This mutex needs to be locked when accessing `mapTx` or other members
+     * that are guarded by it.
+     *
+     * @par Consistency guarantees
+     *
+     * By design, it is guaranteed that:
+     *
+     * 1. Locking both `cs_main` and `mempool.cs` will give a view of mempool
+     *    that is consistent with current chain tip (`chainActive` and
+     *    `pcoinsTip`) and is fully populated. Fully populated means that if the
+     *    current active chain is missing transactions that were present in a
+     *    previously active chain, all the missing transactions will have been
+     *    re-added to the mempool and should be present if they meet size and
+     *    consistency constraints.
+     *
+     * 2. Locking `mempool.cs` without `cs_main` will give a view of a mempool
+     *    consistent with some chain that was active since `cs_main` was last
+     *    locked, and that is fully populated as described above. It is ok for
+     *    code that only needs to query or remove transactions from the mempool
+     *    to lock just `mempool.cs` without `cs_main`.
+     *
+     * To provide these guarantees, it is necessary to lock both `cs_main` and
+     * `mempool.cs` whenever adding transactions to the mempool and whenever
+     * changing the chain tip. It's necessary to keep both mutexes locked until
+     * the mempool is consistent with the new chain tip and fully populated.
+     *
+     * @par Consistency bug
+     *
+     * The second guarantee above is not currently enforced, but
+     * https://github.com/bitcoin/bitcoin/pull/14193 will fix it. No known code
+     * in bitcoin currently depends on second guarantee, but it is important to
+     * fix for third party code that needs be able to frequently poll the
+     * mempool without locking `cs_main` and without encountering missing
+     * transactions during reorgs.
+     */
+    mutable RecursiveMutex cs;
     indexed_transaction_set mapTx GUARDED_BY(cs);
 
     using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
diff --git a/src/validation.cpp b/src/validation.cpp
index 5696684ed6..25818e496d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -210,9 +210,17 @@ private:
     bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 } g_chainstate;
 
-
-
-CCriticalSection cs_main;
+/**
+ * Mutex to guard access to validation specific variables, such as reading
+ * or changing the chainstate.
+ *
+ * This may also need to be locked when updating the transaction pool, e.g. on
+ * AcceptToMemoryPool. See CTxMemPool::cs comment for details.
+ *
+ * The transaction pool has a separate lock to allow reading from it and the
+ * chainstate at the same time.
+ */
+RecursiveMutex cs_main;
 
 BlockMap& mapBlockIndex = g_chainstate.mapBlockIndex;
 CChain& chainActive = g_chainstate.chainActive;