Add thread safety annotations to CTxMemPool methods #19647

hebasto · 2020-08-02T22:42:29Z

Split out from #19306.

Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change.

This is a step to make CTxMemPool::cs an instance of Mutex rather RecursiveMutex.

hebasto · 2020-08-02T22:45:26Z

@ajtowns @jnewbery @vasild @JeremyRubin

Mind reviewing?

JeremyRubin · 2020-08-02T22:58:14Z

this looks correct, concept and like cr ack.

DrahtBot · 2020-08-03T01:50:18Z

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

Conflicts

No conflicts as of last run.

jnewbery · 2020-08-03T10:37:27Z

Concept ACK. Annotating the existing locking logic is always useful.

maflcko · 2020-08-03T11:36:19Z

src/txmempool.cpp

-TxMempoolInfo CTxMemPool::info(const uint256& txid) const { return info(GenTxid{false, txid}); }
+TxMempoolInfo CTxMemPool::info(const uint256& txid) const
+{
+    AssertLockNotHeld(cs);


Excluding locks in getters makes it impossible to get more than one consistent mempool entry, as the state of the second might change after the first one has been returned.

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

We've got infoAll for a consistent look at all mempool entries (used by DumpMempool), and apparently don't use info for consistent views otherwise, so excluding the lock seems okay to me.

To get consistent values the external locking of CTxMemPool::cs should be used.

Anyway, this PR does not change locking behavior.

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

When a thread safety annotation is a part of function member declaration, I found it very convenient having a corresponding AssertLock{Not}Held() statement in its definition.

Also this assertion would be really helpful when CTxMemPool::cs will actually transit from RecursiveMutex to Mutex.

ajtowns

Concept ACK. Shouldn't this PR also add GUARDED_BY(cs) to all the other non-static, non-atomic data members?

src/txmempool.cpp

ajtowns · 2020-08-03T12:26:32Z

src/txmempool.cpp

-TxMempoolInfo CTxMemPool::info(const uint256& txid) const { return info(GenTxid{false, txid}); }
+TxMempoolInfo CTxMemPool::info(const uint256& txid) const
+{
+    AssertLockNotHeld(cs);


AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

We've got infoAll for a consistent look at all mempool entries (used by DumpMempool), and apparently don't use info for consistent views otherwise, so excluding the lock seems okay to me.

hebasto · 2020-08-03T13:01:38Z

@ajtowns

Concept ACK. Shouldn't this PR also add GUARDED_BY(cs) to all the other non-static, non-atomic data members?

This PR split out from #19306 to make it small and easy to review.
Making other non-static, non-atomic data members GUARDED_BY(cs) is a design-change decision that I'd leave out of this PR scope.

practicalswift · 2020-08-03T15:48:36Z

Concept ACK: thanks for adding thread-safety annotations!

hebasto · 2020-08-03T16:06:38Z

Updated f084aac -> 343b93b (pr19647.01 -> pr19647.02, diff):

addressed @ajtowns' comment:

AssertLockHeld is annotated with ASSERT_EXCLUSIVE_LOCK which disables clang's compile time checks that the lock was acquired beforehand. Wouldn't it be better just to rely on the compile time checks?

vasild

Looks good except the suggestion below.

It was tedious to check the callers of e.g. ApplyDelta() and their callers and their callers etc until finding either LOCK(cs) or AssertLockHeld(cs) to ensure that the mutex is indeed held in all places where ApplyDelta() is called.

vasild · 2020-08-11T09:18:55Z

src/txmempool.cpp


 void CTxMemPool::ApplyDelta(const uint256 hash, CAmount &nFeeDelta) const
 {
-    LOCK(cs);


Here and in CTxMemPool::ClearPrioritisation(), CTxMemPool::GetTotalTxSize() and CTxMemPool::IsUnbroadcastTx() I would strongly advise to add AssertLockHeld(cs) where LOCK(cs) is removed. And in general in every function that is annotated EXCLUSIVE_LOCKS_REQUIRED(). It would serve as documentation to the reader and also ensure that the code does not get executed without holding the mutex.

The clang compile time checks are good, but I would say not to be the sole protection - they get silenced in an unexpected way if EXCLUSIVE_LOCKS_REQUIRED() and ASSERT_EXCLUSIVE_LOCK() are both used, do not work with gcc and produce only compile warnings with clang (could be missed if -Werror is not used). I would say they are an addition, but not replacement of run-time asserts.

6dbd57f

they get silenced in an unexpected way

@vasild do you have a code sample?

I would say they are an addition

Agree.

they get silenced in an unexpected way

@vasild do you have a code sample?

Yes, see #19668 (comment). There are two unexpected things:

The warnings differ depending on the order of the attributes (if more than one is specified):

void f() EXCLUSIVE_LOCKS_REQUIRED(cs) ASSERT_EXCLUSIVE_LOCK(cs); // vs void f() ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(cs);

The presence of ASSERT_EXCLUSIVE_LOCK() alters the behavior of EXCLUSIVE_LOCKS_REQUIRED().

Annotating both ASSERT_EXCLUSIVE_LOCK and EXCLUSIVE_LOCKS_REQUIRED on the same function makes no sense. The only reason to do the ASSERT_EXCLUSIVE_LOCK is if you're relying on the test having succeeded for execution to continue as far as later compile-time checks are concerned, but if you've satisfied EXCLUSIVE_LOCKS_REQUIRED already, then there's no need to hint the later compile-time checks, they're already satisfied.

It was tedious to check the callers of e.g. ApplyDelta() and their callers and their callers etc until finding either LOCK(cs) or AssertLockHeld(cs) to ensure that the mutex is indeed held in all places where ApplyDelta() is called.

The whole point of the compile time annotations is to have the compiler do the work of checking that the callers have a LOCK(cs). Note that an AssertLockHeld(cs) isn't sufficient -- any time that's called without having the lock already held would also be a bug; and until the ASSERT_EXCLUSIVE_LOCK annotation is removed from AssertLockHeld, AssertLockHeld may in fact prevent the compiler from doing adequate checking...

promag

Code review ACK 343b93b.

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

When a thread safety annotation is a part of function member declaration, I found it very convenient having a corresponding AssertLock{Not}Held() statement in its definition.

Also this assertion would be really helpful when CTxMemPool::cs will actually transit from RecursiveMutex to Mutex.

Let's not make a wave of pulls adding negative thread safety annotations everywhere.

promag · 2020-08-11T10:06:37Z

src/txmempool.cpp


 void CTxMemPool::ApplyDelta(const uint256 hash, CAmount &nFeeDelta) const
 {
-    LOCK(cs);


6dbd57f

they get silenced in an unexpected way

@vasild do you have a code sample?

I would say they are an addition

Agree.

promag · 2020-08-11T10:08:54Z

src/txmempool.h

     */
    std::map<uint256, uint256> m_unbroadcast_txids GUARDED_BY(cs);

+    void ClearPrioritisation(const uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs);


3c7d4cb

nit, could refactor to const uint256&?

hebasto · 2020-08-13T10:39:28Z

Fellow reviewers! It seems there is a consensus that adding thread safety annotations would be much safer and more convenient after AssertLockHeldInternal() improving. Therefore, begging for #19668 review at first :)

hebasto · 2020-09-01T09:50:03Z

Closed in favor of #19854.

020f051 refactor: CTxMemPool::IsUnbroadcastTx() requires CTxMemPool::cs lock (Hennadii Stepanov) 7c4bd03 refactor: CTxMemPool::GetTotalTxSize() requires CTxMemPool::cs lock (Hennadii Stepanov) fa5fcb0 refactor: CTxMemPool::ClearPrioritisation() requires CTxMemPool::cs lock (Hennadii Stepanov) 7140b31 refactor: CTxMemPool::ApplyDelta() requires CTxMemPool::cs lock (Hennadii Stepanov) 66e47e5 refactor: CTxMemPool::UpdateChild() requires CTxMemPool::cs lock (Hennadii Stepanov) 9398077 refactor: CTxMemPool::UpdateParent() requires CTxMemPool::cs lock (Hennadii Stepanov) Pull request description: This is another step to transit `CTxMemPool::cs` from `RecursiveMutex` to `Mutex`. Split out from #19306. Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change. Refactoring `const uint256` to `const uint256&` was [requested](#19647 (comment)) by **promag**. Please note that now, since #19668 has been merged, it is safe to apply `AssertLockHeld()` macros as they do not swallow compile time Thread Safety Analysis warnings. ACKs for top commit: promag: Core review ACK 020f051. jnewbery: Code review ACK 020f051 vasild: ACK 020f051 Tree-SHA512: a31e389142d5a19b25fef0aaf1072a337278564528b5cc9209df88ae548a31440e1b8dd9bae0169fd7aa59ea06e22fe5e0413955386512b83ef1f3e7d941e890

…le cases 020f051 refactor: CTxMemPool::IsUnbroadcastTx() requires CTxMemPool::cs lock (Hennadii Stepanov) 7c4bd03 refactor: CTxMemPool::GetTotalTxSize() requires CTxMemPool::cs lock (Hennadii Stepanov) fa5fcb0 refactor: CTxMemPool::ClearPrioritisation() requires CTxMemPool::cs lock (Hennadii Stepanov) 7140b31 refactor: CTxMemPool::ApplyDelta() requires CTxMemPool::cs lock (Hennadii Stepanov) 66e47e5 refactor: CTxMemPool::UpdateChild() requires CTxMemPool::cs lock (Hennadii Stepanov) 9398077 refactor: CTxMemPool::UpdateParent() requires CTxMemPool::cs lock (Hennadii Stepanov) Pull request description: This is another step to transit `CTxMemPool::cs` from `RecursiveMutex` to `Mutex`. Split out from bitcoin#19306. Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change. Refactoring `const uint256` to `const uint256&` was [requested](bitcoin#19647 (comment)) by **promag**. Please note that now, since bitcoin#19668 has been merged, it is safe to apply `AssertLockHeld()` macros as they do not swallow compile time Thread Safety Analysis warnings. ACKs for top commit: promag: Core review ACK 020f051. jnewbery: Code review ACK 020f051 vasild: ACK 020f051 Tree-SHA512: a31e389142d5a19b25fef0aaf1072a337278564528b5cc9209df88ae548a31440e1b8dd9bae0169fd7aa59ea06e22fe5e0413955386512b83ef1f3e7d941e890

hebasto mentioned this pull request Aug 2, 2020

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

35 tasks

hebasto mentioned this pull request Aug 2, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

DrahtBot added the Mempool label Aug 2, 2020

DrahtBot mentioned this pull request Aug 3, 2020

Remove CTxMempool::mapLinks data structure member #19478

Merged

maflcko reviewed Aug 3, 2020

View reviewed changes

ajtowns reviewed Aug 3, 2020

View reviewed changes

hebasto added 3 commits August 3, 2020 18:49

refactor: Add more thread safety annotations to CTxMemPool

6dbd57f

refactor: Make CTxMemPool::ClearPrioritisation() private

3c7d4cb

refactor: Add negative thread safety annotations to CTxMemPool

343b93b

hebasto force-pushed the 200802-mmx-1 branch from f084aac to 343b93b Compare August 3, 2020 16:02

vasild suggested changes Aug 11, 2020

View reviewed changes

promag reviewed Aug 11, 2020

View reviewed changes

hebasto mentioned this pull request Sep 1, 2020

Avoid locking CTxMemPool::cs recursively in simple cases #19854

Merged

hebasto closed this Sep 1, 2020

vasild mentioned this pull request Sep 7, 2020

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Add thread safety annotations to CTxMemPool methods #19647

Add thread safety annotations to CTxMemPool methods #19647

Uh oh!

Conversation

hebasto commented Aug 2, 2020

Uh oh!

hebasto commented Aug 2, 2020

Uh oh!

JeremyRubin commented Aug 2, 2020

Uh oh!

DrahtBot commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Aug 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 3, 2020

Uh oh!

practicalswift commented Aug 3, 2020

Uh oh!

hebasto commented Aug 3, 2020

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 13, 2020

Uh oh!

hebasto commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Aug 3, 2020 •

edited

Loading

hebasto Aug 3, 2020 •

edited

Loading

vasild left a comment •

edited

Loading

promag Aug 11, 2020 •

edited

Loading

promag Aug 11, 2020 •

edited

Loading