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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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 ...
Recursive is already clear from the variable type and I plan to make it non-recursive
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:
/**
* This mutex needs to be locked when accessing mapTx.
*
* [@par](/github-metadata-backup-bitcoin-bitcoin/contributor/par/) Relationship to cs_main
* Locking both mempool.cs and cs_main gives a view of the mempool which is
* compatible with the current chain tip. To guarantee this, both mutexes need
* to be locked when adding transactions to the mempool or changing the
* chain tip. But locking mempool.cs by itself is sufficient for reading
* transactions from the mempool, or removing transactions from it.
*/
It could also be used to guard other members, like prioritization data, no?
Otherwise done
Concept ACK. Title prefix suggests this is documentation only change, which is not. Some nits though.
Fixed up some of the language. Any suggestions for the pull request subject line?
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.
Done
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."
Done
utACK fa64bb42e97b6c6c58e83badca88d386f99dbcd2
ACK
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:
/**
* This mutex needs to be locked when accessing `mapTx` or other members
* that are guarded by it.
*
* [@par](/github-metadata-backup-bitcoin-bitcoin/contributor/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](/github-metadata-backup-bitcoin-bitcoin/contributor/par/) Consistency bug
*
* The second guarantee above is not currently enforced, but
* [#14193](/github-metadata-backup-bitcoin-bitcoin/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.
*/
Wow, excellent write-up. Will pull that in to replace my clumsy explanation
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
Should say "may also need to be locked" instead of "may also be locked".
utACK fa5c7671aa4c916b191d0423309f89b69568c7f9. The only changes since my previous review were applying suggestions.
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);
Changer order of 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);
Changer order of cs and cs_main here to make locking order consistent?
utACK fa5e373365b8e88fc9894f53f618d3e89f1bec14 modulo nits
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
We call the "transaction pool" the "mempool" in so many places in this file, I find the usage of "transaction pool" to be odd...
utACK fa5e373365b8e88fc9894f53f618d3e89f1bec14