Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
cs_main and mempool::cs
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
218+ * or changing the chainstate.
219+ *
220+ * This must also be locked when updating the transaction pool, e.g. on
221+ * AcceptToMemoryPool.
222+ *
223+ * The transaction pool has a separate lock to allow rading it and the
rading it -> reading from it
209@@ -210,9 +210,17 @@ class CChainState {
210 bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
211 } g_chainstate;
212
213-
214-
215-CCriticalSection cs_main;
216+/**
217+ * Mutex to guard access to validation specific variables, such as reading
such as -> in cases such as?
Recursive mutex to ...
484@@ -485,7 +485,13 @@ class CTxMemPool
485 >
486 > indexed_transaction_set;
487
488- mutable CCriticalSection cs;
489+ /**
490+ * Mutex that guards reads.
491+ *
492+ * The validation mutex cs_main should be locked in addition to this lock
... should be locked before in addition ...
484@@ -485,7 +485,13 @@ class CTxMemPool
485 >
486 > indexed_transaction_set;
487
488- mutable CCriticalSection cs;
489+ /**
490+ * Mutex that guards reads.
Recursive mutex ...
In commit “doc: Add comment to cs_main and mempool::cs” (fa64bb42e97b6c6c58e83badca88d386f99dbcd2)
I think making a comparison to read/write locks makes this more confusing than it needs to be. Read locks normally are shared, not exclusive. And the requirement to lock cs_main for write access seems too broad, and doesn’t give a justification.
I would suggest a comment that just says when you need to lock the mutexes and why:
0/**
1 * This mutex needs to be locked when accessing mapTx.
2 *
3 * [@par](/bitcoin-bitcoin/contributor/par/) Relationship to cs_main
4 * Locking both mempool.cs and cs_main gives a view of the mempool which is
5 * compatible with the current chain tip. To guarantee this, both mutexes need
6 * to be locked when adding transactions to the mempool or changing the
7 * chain tip. But locking mempool.cs by itself is sufficient for reading
8 * transactions from the mempool, or removing transactions from it.
9 */
I think the most important change is mempool: Add missing locks to `cs_main` , the remaining change is a consequence of that.
Could also say:
107@@ -108,7 +108,7 @@ static void MempoolEviction(benchmark::State& state)
108 tx7.vout[1].nValue = 10 * COIN;
109
110 CTxMemPool pool;
111- LOCK(pool.cs);
112+ LOCK2(cs_main, pool.cs);
In commit “doc: Add comment to cs_main and mempool::cs” (fa64bb42e97b6c6c58e83badca88d386f99dbcd2)
Would suggest moving the test and bench changes in this commit into a different commit, since they aren’t really what you’d normally think of as documentation changes.
215-CCriticalSection cs_main;
216+/**
217+ * Mutex to guard access to validation specific variables, such as reading
218+ * or changing the chainstate.
219+ *
220+ * This must also be locked when updating the transaction pool, e.g. on
In commit “doc: Add comment to cs_main and mempool::cs” (fa64bb42e97b6c6c58e83badca88d386f99dbcd2)
I would say “may also need to be locked” instead of “must also be locked”, and add “see CTxMemPool::cs comment for details.”
495+ * compatible with the current chain tip. To guarantee this, both mutexes need
496+ * to be locked when adding transactions to the mempool or changing the
497+ * chain tip. But locking mempool.cs by itself is sufficient for reading
498+ * transactions from the mempool, or removing transactions from it.
499+ */
500+ mutable RecursiveMutex cs;
This comment comes from #14963 (review) and I think it is still ok as a description of how the code currently works. But talking to @morcos, @sdaftuar, and you Monday, it sounds like this is not actually how things are intended to work. After that conversation, I think the following might be more accurate:
0/**
1 * This mutex needs to be locked when accessing `mapTx` or other members
2 * that are guarded by it.
3 *
4 * [@par](/bitcoin-bitcoin/contributor/par/) Consistency guarantees
5 *
6 * By design, it is guaranteed that:
7 *
8 * 1. Locking both `cs_main` and `mempool.cs` will give a view of mempool
9 * that is consistent with current chain tip (`chainActive` and
10 * `pcoinsTip`) and is fully populated. Fully populated means that if the
11 * current active chain is missing transactions that were present in a
12 * previously active chain, all the missing transactions will have been
13 * re-added to the mempool and should be present if they meet size and
14 * consistency constraints.
15 *
16 * 2. Locking `mempool.cs` without `cs_main` will give a view of a mempool
17 * consistent with some chain that was active since `cs_main` was last
18 * locked, and that is fully populated as described above. It is ok for
19 * code that only needs to query or remove transactions from the mempool
20 * to lock just `mempool.cs` without `cs_main`.
21 *
22 * To provide these guarantees, it is necessary to lock both `cs_main` and
23 * `mempool.cs` whenever adding transactions to the mempool and whenever
24 * changing the chain tip. It's necessary to keep both mutexes locked until
25 * the mempool is consistent with the new chain tip and fully populated.
26 *
27 * [@par](/bitcoin-bitcoin/contributor/par/) Consistency bug
28 *
29 * The second guarantee above is not currently enforced, but
30 * [#14193](/bitcoin-bitcoin/14193/) will fix it. No known code
31 * in bitcoin currently depends on second guarantee, but it is important to
32 * fix for third party code that needs be able to frequently poll the
33 * mempool without locking `cs_main` and without encountering missing
34 * transactions during reorgs.
35 */
215-CCriticalSection cs_main;
216+/**
217+ * Mutex to guard access to validation specific variables, such as reading
218+ * or changing the chainstate.
219+ *
220+ * This may also be locked when updating the transaction pool, e.g. on
utACK fa5e373365b8e88fc9894f53f618d3e89f1bec14. Changes since last review: adding longer mempool.cs comment and adding a commit with some new locking annotations.
Also, I don’t think this needs to hold up merging, but I think it would be good if a reviewer with more mempool expertise could read the new code comments and ACK if they are correct.
576@@ -541,8 +577,8 @@ class CTxMemPool
577 // Note that addUnchecked is ONLY called from ATMP outside of tests
578 // and any other callers may break wallet's in-mempool tracking (due to
579 // lack of CValidationInterface::TransactionAddedToMempool callbacks).
580- void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
581- void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
582+ void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
cs and cs_main here to make locking order consistent?
576@@ -541,8 +577,8 @@ class CTxMemPool
577 // Note that addUnchecked is ONLY called from ATMP outside of tests
578 // and any other callers may break wallet's in-mempool tracking (due to
579 // lack of CValidationInterface::TransactionAddedToMempool callbacks).
580- void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
581- void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
582+ void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
583+ void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
cs and cs_main here to make locking order consistent?
215-CCriticalSection cs_main;
216+/**
217+ * Mutex to guard access to validation specific variables, such as reading
218+ * or changing the chainstate.
219+ *
220+ * This may also need to be locked when updating the transaction pool, e.g. on
