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

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

Merged
merged 6 commits into from Sep 4, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 1, 2020

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

No change in behavior, the lock is already held at call sites.
No change in behavior, the lock is already held at call sites.
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
No change in behavior, the lock is already held at call sites.
Also `const uint256` refactored to `const uint256&`.
No change in behavior, the lock is already held at call sites.
No change in behavior, the lock is already held at call sites.
@ajtowns
Copy link
Contributor

ajtowns commented Sep 1, 2020

Concept ACK, looks reasonable

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 1, 2020

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

Conflicts

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.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 020f051

@promag
Copy link
Member

promag commented Sep 2, 2020

Core review ACK 020f051.

@jnewbery
Copy link
Contributor

jnewbery commented Sep 4, 2020

Code review ACK 020f051

@laanwj laanwj merged commit 99a8eb6 into bitcoin:master Sep 4, 2020
@hebasto hebasto deleted the 200901-mmx3 branch September 4, 2020 11:25
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 9, 2020
…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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 22, 2021
Summary:
Only trivial thread safety annotations and lock assertions added. No new locks.
No change in behavior, the lock is already held at call sites.

This is a backport of [[bitcoin/bitcoin#19854 | core#19854]]

Test Plan:
With TSAN:
`ninja && ninja check check-functional`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10177
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants