mempool, validation: Explain cs_main locking semantics #14963

maflcko · 2018-12-14T22:07:08Z

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:

Writing to the chain state or adding transactions to the transaction pool -> Take both cs_main and mempool::cs
Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock

DrahtBot · 2018-12-15T00:27:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#11652 (Add missing locks: validation.cpp + related by practicalswift)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

src/validation.cpp

promag

Concept ACK. Title prefix suggests this is documentation only change, which is not. Some nits though.

src/txmempool.h

src/validation.cpp

maflcko · 2018-12-17T16:17:58Z

Fixed up some of the language. Any suggestions for the pull request subject line?

promag · 2018-12-17T16:26:41Z

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:

only tests are affected
fix related annotations and improve documentation

ryanofsky

utACK fa64bb42e97b6c6c58e83badca88d386f99dbcd2

src/bench/mempool_eviction.cpp

src/validation.cpp

src/txmempool.h

laanwj · 2018-12-19T10:01:53Z

ACK

ryanofsky

utACK fa5c7671aa4c916b191d0423309f89b69568c7f9. The only changes since my previous review were applying suggestions.

src/txmempool.h

src/validation.cpp

ryanofsky

utACK fa5e373. 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.

practicalswift · 2019-01-05T16:36:06Z

src/txmempool.h

    // lack of CValidationInterface::TransactionAddedToMempool callbacks).
-    void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
-    void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    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?

practicalswift · 2019-01-05T16:36:13Z

src/txmempool.h

-    void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
-    void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
+    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?

practicalswift · 2019-01-05T16:36:53Z

utACK fa5e373 modulo nits

sdaftuar

utACK fa5e373

sdaftuar · 2019-01-15T18:19:04Z

src/validation.cpp

+ * 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


We call the "transaction pool" the "mempool" in so many places in this file, I find the usage of "transaction pool" to be odd...

fa5e373 validation: Add cs_main locking annotations (MarcoFalke) fa5c346 doc: Add comment to cs_main and mempool::cs (MarcoFalke) fafe941 test: Add missing validation locks (MarcoFalke) fac4558 sync: Add RecursiveMutex type alias (MarcoFalke) Pull request description: 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: * Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs` * Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c

Summary: 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: Backport of Bitcoin Core PR14963 bitcoin/bitcoin#14963 Test Plan: 1. Build with Clang in Debug mode: ``` CXX=clang++ CC=clang cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug ninja check-all ``` 2. Verify that the compiler has not emitted a `thread-safety` warning. 3. Run the node: `./src/bitcoind -regtest` 4. Verify that text similar to `"Assertion failed: lock ... not held ..."` is not printed on `stderr`. 5. Run benchmarks: ``` cmake .. -GNinja ninja bench-bitcoin ./src/bench/bitcoin-bench ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4227

…antics fa5e373 validation: Add cs_main locking annotations (MarcoFalke) fa5c346 doc: Add comment to cs_main and mempool::cs (MarcoFalke) fafe941 test: Add missing validation locks (MarcoFalke) fac4558 sync: Add RecursiveMutex type alias (MarcoFalke) Pull request description: 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: * Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs` * Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c

sync: Add RecursiveMutex type alias

fac4558

maflcko added the Docs label Dec 14, 2018

maflcko force-pushed the Mf1812-docValLocks branch 3 times, most recently from c7a872f to fa5ed69 Compare December 14, 2018 22:26

ch4ot1c suggested changes Dec 15, 2018

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

promag reviewed Dec 15, 2018

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the Mf1812-docValLocks branch from fa5ed69 to fa64bb4 Compare December 17, 2018 16:17

maflcko changed the title ~~doc: Add comment to cs_main and mempool::cs~~ mempool, validation: Explain cs_main locking semantics Dec 17, 2018

maflcko added Validation Mempool and removed Docs labels Dec 17, 2018

ryanofsky approved these changes Dec 17, 2018

View reviewed changes

src/bench/mempool_eviction.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Dec 17, 2018

validation: Add missing mempool locks #14193

Merged

test: Add missing validation locks

fafe941

maflcko force-pushed the Mf1812-docValLocks branch 2 times, most recently from fa70aeb to fa5c767 Compare December 17, 2018 19:46

ryanofsky approved these changes Dec 19, 2018

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

doc: Add comment to cs_main and mempool::cs

fa5c346

maflcko force-pushed the Mf1812-docValLocks branch 3 times, most recently from fa0e072 to fa5f6ca Compare December 19, 2018 20:06

validation: Add cs_main locking annotations

fa5e373

maflcko force-pushed the Mf1812-docValLocks branch from fa5f6ca to fa5e373 Compare December 22, 2018 14:23

ryanofsky approved these changes Jan 4, 2019

View reviewed changes

practicalswift reviewed Jan 5, 2019

View reviewed changes

sdaftuar approved these changes Jan 15, 2019

View reviewed changes

maflcko merged commit fa5e373 into bitcoin:master Jan 15, 2019

maflcko deleted the Mf1812-docValLocks branch January 15, 2019 18:47

ryanofsky mentioned this pull request Jan 15, 2019

Add fee_est tool for debugging fee estimation code #10443

Closed

practicalswift mentioned this pull request May 24, 2019

scripted-diff: Complete the move from CCriticalSection to identical RecursiveMutex (both are AnnotatedMixin<std::recursive_mutex>) #16084

Closed

maflcko mentioned this pull request Apr 29, 2020

P2P: Mempool tracks locally submitted transactions to improve wallet privacy #18038

Merged

str4d mentioned this pull request Apr 12, 2021

Sync backports zcash/zcash#5057

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

mempool, validation: Explain cs_main locking semantics #14963

mempool, validation: Explain cs_main locking semantics #14963

Uh oh!

Conversation

maflcko commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 17, 2018

Uh oh!

promag commented Dec 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Dec 19, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift Jan 5, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift Jan 5, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jan 5, 2019

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented Dec 14, 2018 •

edited

Loading

DrahtBot commented Dec 15, 2018 •

edited

Loading