mempool: Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…) #11689

practicalswift · 2017-11-14T22:37:18Z

Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins):

reading variable mapTx requires holding mutex cs
reading variable mapNextTx requires holding mutex cs
reading variable nCheckFrequency requires holding mutex cs

Fix missing locking in CTxMemPool::setSanityCheck(double dFrequency):

writing variable nCheckFrequency requires holding mutex cs

promag · 2017-11-15T09:37:22Z

ACK 2abe610.

Unrelated, but should read nCheckFrequency with the lock too?

practicalswift · 2017-11-15T09:44:15Z

@promag Yes, nCheckFrequency should probably be guarded by the mutex cs too. Good catch!

I'll ping in @MarcoFalke and @TheBlueMatt for a comment. What do you say? :-)

$ git blame src/txmempool.h | grep nCheckFrequency
fada0c42 (MarcoFalke               2016-04-03 11:49:36 +0200 415)     uint32_t nCheckFrequency; //!< Value n means that n times in 2^32 we check.
53a6590f (Matt Corallo             2017-09-11 15:43:49 -0400 510)     void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = static_cast<uint32_t>(dFrequency * 4294967295.0); }
$ git grep -E '(nCheckFrequency|setSanityCheck)'
src/init.cpp:        mempool.setSanityCheck(1.0 / ratio);
src/qt/test/rpcnestedtests.cpp:    //mempool.setSanityCheck(1.0);
src/test/test_bitcoin.cpp:        mempool.setSanityCheck(1.0);
src/txmempool.cpp:    nCheckFrequency = 0;
src/txmempool.cpp:                if (nCheckFrequency != 0) assert(!coin.IsSpent());
src/txmempool.cpp:    if (nCheckFrequency == 0)
src/txmempool.cpp:    if (GetRand(std::numeric_limits<uint32_t>::max()) >= nCheckFrequency)
src/txmempool.h:    uint32_t nCheckFrequency; //!< Value n means that n times in 2^32 we check.
src/txmempool.h:    void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = static_cast<uint32_t>(dFrequency * 4294967295.0); }

promag · 2017-11-15T09:54:30Z

Actually nCheckFrequency doesn't change, only set in

src/init.cpp:        mempool.setSanityCheck(1.0 / ratio);

TheBlueMatt · 2017-11-15T22:47:59Z

Technically there is no issue here - mempool (should only be) modified with cs_main and mempool.check is only called with cs_main. Indeed, probably just move the lock to the top of the function. Unless you want to go to the trouble of making nCheckFrequency a const, best to fix it for the static analysis just like the rest.

practicalswift · 2017-11-16T09:10:05Z

@TheBlueMatt @promag Thanks for reviewing! An updated version has now been pushed taking into account the formal locking requirements of nCheckFrequency too. Please re-review :-)

practicalswift · 2017-11-23T06:46:45Z

Added a commit with the Clang thread safety analysis annotations to facilitate reviewing. @promag, would you mind re-reviewing? :-)

practicalswift · 2017-11-29T23:06:36Z

Updated with annotations moved over to the header files.

The EXCLUSIVE_LOCKS_REQUIRED(mempool.cs) annotations for BlockAssembler::SkipMapTxEntry and BlockAssembler::addPackageTxs should be placed in miner.h rather than miner.cpp, but the existence of mempool (and hence mempool.cs) is not currently not known in miner.h. Would extern CTxMemPool mempool; be acceptable in miner.h?

TheBlueMatt · 2017-12-04T17:46:46Z

As mentioned before, please do not place EXCLUSIVE_LOCKS_REQUIRED annotations on class member function declarations instead of their definitions. Placing them on declarations makes the annotations incredibly brittle and, thus, largely useless. Better to leave them off than to add them in a place where the order of functions in a file matters.

practicalswift · 2018-03-12T23:16:59Z

@TheBlueMatt Sorry, I had missed moving the annotations for SkipMapTxEntry, addPackageTxs, UpdateChildrenForRemoval and CalculateDescendants. Now fixed! Please re-review :-)

sipa · 2018-03-17T00:19:40Z

utACK 661db1d63afcea4016ccb7f97fda6cde7f719826

practicalswift · 2018-04-03T11:35:54Z

Rebased!

* reading variable 'mapTx' requires holding mutex 'cs' * reading variable 'mapNextTx' requires holding mutex 'cs' * reading variable 'nCheckFrequency' requires holding mutex 'cs'

* writing variable 'nCheckFrequency' requires holding mutex 'cs'

practicalswift · 2018-05-14T13:26:15Z

@promag @sipa Would you mind re-reviewing?

@MarcoFalke Would you mind reviewing? :-)

maflcko · 2018-05-14T14:16:44Z

utACK 47782b4

…d CTxMemPool::setSanityCheck(…) 47782b4 Add Clang thread safety analysis annotations (practicalswift) 0e2dfa8 Fix missing locking in CTxMemPool::setSanityCheck(double dFrequency) (practicalswift) 6bc5b71 Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins) (practicalswift) Pull request description: Fix missing locking in `CTxMemPool::check(const CCoinsViewCache *pcoins)`: * reading variable `mapTx` requires holding mutex `cs` * reading variable `mapNextTx` requires holding mutex `cs` * reading variable `nCheckFrequency` requires holding mutex `cs` Fix missing locking in `CTxMemPool::setSanityCheck(double dFrequency)`: * writing variable `nCheckFrequency` requires holding mutex `cs` Tree-SHA512: ce7c365ac89225223fb06e6f469451b121acaa499f35b21ad8a6d2a266c91194639b3703c5428871be033d4f5f7be790cc297bd8c25b2e0c59345ef09c3693d0

…k(…) and CTxMemPool::setSanityCheck(…) 47782b4 Add Clang thread safety analysis annotations (practicalswift) 0e2dfa8 Fix missing locking in CTxMemPool::setSanityCheck(double dFrequency) (practicalswift) 6bc5b71 Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins) (practicalswift) Pull request description: Fix missing locking in `CTxMemPool::check(const CCoinsViewCache *pcoins)`: * reading variable `mapTx` requires holding mutex `cs` * reading variable `mapNextTx` requires holding mutex `cs` * reading variable `nCheckFrequency` requires holding mutex `cs` Fix missing locking in `CTxMemPool::setSanityCheck(double dFrequency)`: * writing variable `nCheckFrequency` requires holding mutex `cs` Tree-SHA512: ce7c365ac89225223fb06e6f469451b121acaa499f35b21ad8a6d2a266c91194639b3703c5428871be033d4f5f7be790cc297bd8c25b2e0c59345ef09c3693d0

* reading variable 'mapTx' requires holding mutex 'cs' * reading variable 'mapNextTx' requires holding mutex 'cs' * reading variable 'nCheckFrequency' requires holding mutex 'cs' zcash: cherry picked from commit 6bc5b71 zcash: bitcoin/bitcoin#11689

* writing variable 'nCheckFrequency' requires holding mutex 'cs' zcash: cherry picked from commit 0e2dfa8 zcash: bitcoin/bitcoin#11689

zcash: cherry picked from commit 47782b4 zcash: bitcoin/bitcoin#11689

* reading variable 'mapTx' requires holding mutex 'cs' * reading variable 'mapNextTx' requires holding mutex 'cs' * reading variable 'nCheckFrequency' requires holding mutex 'cs' zcash: cherry picked from commit 6bc5b71 zcash: bitcoin/bitcoin#11689

* writing variable 'nCheckFrequency' requires holding mutex 'cs' zcash: cherry picked from commit 0e2dfa8 zcash: bitcoin/bitcoin#11689

zcash: cherry picked from commit 47782b4 zcash: bitcoin/bitcoin#11689

…k(…) and CTxMemPool::setSanityCheck(…) 47782b4 Add Clang thread safety analysis annotations (practicalswift) 0e2dfa8 Fix missing locking in CTxMemPool::setSanityCheck(double dFrequency) (practicalswift) 6bc5b71 Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins) (practicalswift) Pull request description: Fix missing locking in `CTxMemPool::check(const CCoinsViewCache *pcoins)`: * reading variable `mapTx` requires holding mutex `cs` * reading variable `mapNextTx` requires holding mutex `cs` * reading variable `nCheckFrequency` requires holding mutex `cs` Fix missing locking in `CTxMemPool::setSanityCheck(double dFrequency)`: * writing variable `nCheckFrequency` requires holding mutex `cs` Tree-SHA512: ce7c365ac89225223fb06e6f469451b121acaa499f35b21ad8a6d2a266c91194639b3703c5428871be033d4f5f7be790cc297bd8c25b2e0c59345ef09c3693d0

fanquake added the Mempool label Nov 14, 2017

practicalswift changed the title ~~Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins)~~ mempool: Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins) Nov 14, 2017

practicalswift force-pushed the CTxMemPool-check branch from 2abe610 to fd13497 Compare November 16, 2017 08:55

practicalswift changed the title ~~mempool: Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins)~~ mempool: Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…) Nov 16, 2017

practicalswift force-pushed the CTxMemPool-check branch from 24ce4cf to 0431003 Compare November 29, 2017 22:52

practicalswift mentioned this pull request Mar 12, 2018

[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

Closed

practicalswift force-pushed the CTxMemPool-check branch 2 times, most recently from 93bde20 to 661db1d Compare March 12, 2018 23:07

practicalswift force-pushed the CTxMemPool-check branch from 661db1d to 36441af Compare April 3, 2018 11:35

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

practicalswift force-pushed the CTxMemPool-check branch from 36441af to 682778d Compare May 5, 2018 06:19

practicalswift added 3 commits May 5, 2018 16:50

Fix missing locking in CTxMemPool::check(const CCoinsViewCache *pcoins)

6bc5b71

* reading variable 'mapTx' requires holding mutex 'cs' * reading variable 'mapNextTx' requires holding mutex 'cs' * reading variable 'nCheckFrequency' requires holding mutex 'cs'

Fix missing locking in CTxMemPool::setSanityCheck(double dFrequency)

0e2dfa8

* writing variable 'nCheckFrequency' requires holding mutex 'cs'

Add Clang thread safety analysis annotations

47782b4

practicalswift force-pushed the CTxMemPool-check branch from 682778d to 47782b4 Compare May 5, 2018 14:51

maflcko merged commit 47782b4 into bitcoin:master May 14, 2018

Bushstar mentioned this pull request May 16, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#332

Merged

LarryRuane mentioned this pull request Mar 23, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

practicalswift deleted the CTxMemPool-check branch April 10, 2021 19:34

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Add Clang thread safety analysis annotations

9c4a259

zcash: cherry picked from commit 47782b4 zcash: bitcoin/bitcoin#11689

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Add Clang thread safety analysis annotations

6e6f49d

zcash: cherry picked from commit 47782b4 zcash: bitcoin/bitcoin#11689

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

mempool: Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…) #11689

mempool: Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…) #11689

Uh oh!

Conversation

practicalswift commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 15, 2017

Uh oh!

practicalswift commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 15, 2017

Uh oh!

TheBlueMatt commented Nov 15, 2017

Uh oh!

practicalswift commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 23, 2017

Uh oh!

practicalswift commented Nov 29, 2017

Uh oh!

TheBlueMatt commented Dec 4, 2017

Uh oh!

practicalswift commented Mar 12, 2018

Uh oh!

sipa commented Mar 17, 2018

Uh oh!

practicalswift commented Apr 3, 2018

Uh oh!

practicalswift commented May 14, 2018

Uh oh!

maflcko commented May 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Nov 14, 2017 •

edited

Loading

practicalswift commented Nov 15, 2017 •

edited

Loading

practicalswift commented Nov 16, 2017 •

edited

Loading