Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mempool, validation: Explain cs_main locking semantics #14963

Merged
merged 4 commits into from Jan 15, 2019

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented Dec 14, 2018

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

@MarcoFalke MarcoFalke added the Docs label Dec 14, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-docValLocks branch 3 times, most recently Dec 14, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

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.

Show resolved Hide resolved src/validation.cpp Outdated
Show resolved Hide resolved src/validation.cpp
@promag
Copy link
Member

left a comment

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

Show resolved Hide resolved src/txmempool.h Outdated
Show resolved Hide resolved src/txmempool.h Outdated
Show resolved Hide resolved src/validation.cpp

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-docValLocks branch Dec 17, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

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

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

@MarcoFalke MarcoFalke added Validation Mempool and removed Docs labels Dec 17, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK fa64bb42e97b6c6c58e83badca88d386f99dbcd2

Show resolved Hide resolved src/bench/mempool_eviction.cpp
Show resolved Hide resolved src/validation.cpp Outdated
Show resolved Hide resolved src/txmempool.h Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-docValLocks branch 2 times, most recently Dec 17, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

ACK

@ryanofsky
Copy link
Contributor

left a comment

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

Show resolved Hide resolved src/txmempool.h
Show resolved Hide resolved src/validation.cpp Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-docValLocks branch 3 times, most recently Dec 19, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-docValLocks branch to fa5e373 Dec 22, 2018

@ryanofsky
Copy link
Contributor

left a comment

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.

@@ -541,8 +577,8 @@ class CTxMemPool
// Note that addUnchecked is ONLY called from ATMP outside of tests
// and any other callers may break wallet's in-mempool tracking (due to
// 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);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 5, 2019

Member

Changer order of cs and cs_main here to make locking order consistent?

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);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 5, 2019

Member

Changer order of cs and cs_main here to make locking order consistent?

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

utACK fa5e373 modulo nits

@sdaftuar
Copy link
Member

left a comment

utACK fa5e373

* 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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jan 15, 2019

Member

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

@MarcoFalke MarcoFalke merged commit fa5e373 into bitcoin:master Jan 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jan 15, 2019

Merge #14963: mempool, validation: Explain cs_main locking semantics
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

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1812-docValLocks branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.