Release `LockData::dd_mutex` before calling `*

hebasto commented at 3:38 PM on December 31, 2022: member

Both double_lock_detected() and potential_deadlock_detected() functions call LogPrintf() which in turn implies locking of the Logger::m_cs mutex. To avoid a deadlock, the latter must not have the Mutex type (see #16112).

With this PR the mentioned restriction has been lifted, and it is possible now to use our regular Mutex type for the Logger::m_cs mutex instead of a dedicated StdMutex type (not introducing that change here, as its diff is much bigger than a few lines, and the currently proposed diff seems valuable by itself).

Note for reviewers: Make sure the code is configured and built with CPPFLAGS=-DDEBUG_LOCKORDER.

DrahtBot commented at 3:38 PM on December 31, 2022: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26781.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	ryanofsky, vasild

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33192 (refactor: unify container presence checks by l0rinc)

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.

in src/sync.cpp:189 in 6fe13650f0 outdated

 184 | @@ -184,6 +185,7 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
 185 |          if (lockdata.lockorders.count(p2)) {
 186 |              auto lock_stack_copy = lock_stack;
 187 |              lock_stack.pop_back();
 188 | +            lock.unlock();
 189 |              potential_deadlock_detected(p1, lockdata.lockorders[p2], lock_stack_copy);

vasild commented at 5:03 PM on January 4, 2023:

Here lockdata is accessed after unlocking the mutex which is supposed to protect it. Would it be better to create a copy of lockdata.lockorders[p2] and p1 before unlocking?

vasild commented at 4:03 PM on January 5, 2023:

Looking more carefully, p1 is just a pair of void* that are never dereferenced, so no need to create a copy of that.

This can be marked as resolved.

hebasto force-pushed on Jan 5, 2023

vasild approved

vasild commented at 4:05 PM on January 5, 2023: contributor

ACK 20647b8b805e96f18e6d122ada23fef363a96619

achow101 requested review from ryanofsky on Apr 25, 2023

hebasto commented at 7:16 PM on August 8, 2023: member

cc @ryanofsky

DrahtBot added the label CI failed on Oct 21, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

DrahtBot added the label CI failed on Dec 19, 2023

DrahtBot removed the label CI failed on Dec 24, 2023

DrahtBot added the label CI failed on Jan 13, 2024

hebasto force-pushed on Feb 12, 2024

hebasto commented at 2:36 PM on February 12, 2024: member

Rebased to refresh the CI.

vasild approved

vasild commented at 4:20 PM on February 12, 2024: contributor

ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc

DrahtBot removed the label CI failed on Feb 12, 2024

hebasto commented at 10:55 AM on March 8, 2024: member

@maflcko @ryanofsky

Do you mind having a look into this PR please?

maflcko commented at 1:04 PM on March 11, 2024: member

Sure, seems fine, but I don't see the benefit, if the logger is kept as-is?

in src/sync.cpp:185 in 08cfe273dd outdated

 171 | @@ -172,6 +172,7 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
 172 |              // same thread as that results in an undefined behavior.
 173 |              auto lock_stack_copy = lock_stack;
 174 |              lock_stack.pop_back();
 175 | +            lock.unlock();

ryanofsky commented at 2:17 PM on March 11, 2024:

In commit "Release LockData::dd_mutex before calling *_detected functions" (08cfe273ddf9bd7640456865926ec9d94b363520)

Would be good to have a comment saying why this is necessary. If I understand correctly it could say something like // Release dd_mutex before calling double_lock_detected to avoid deadlocks, so no other lock is ever acquired after dd_mutex, and lock order is consistent.

in src/sync.cpp:196 in 08cfe273dd outdated

 182 | @@ -182,9 +183,11 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
 183 |  
 184 |          const LockPair p2 = std::make_pair(c, i.first);
 185 |          if (lockdata.lockorders.count(p2)) {
 186 | -            auto lock_stack_copy = lock_stack;
 187 | +            auto lock_stack_current = lock_stack;

ryanofsky commented at 2:20 PM on March 11, 2024:

In commit "Release LockData::dd_mutex before calling *_detected functions" (08cfe273ddf9bd7640456865926ec9d94b363520)

Previous name lock_stack_copy seems like it explains the purpose for this variable better than lock_stack_current. Also lock_stack_copy is the name used above and it would be nicer if double_lock_detected and potential_deadlock_detected cases were handled consistently I think, unless I am missing something.

ryanofsky commented at 2:33 PM on March 11, 2024: contributor

Code review ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc. This seems like a good change, but I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

achow101 commented at 9:22 PM on June 17, 2024: member

@hebasto Are you planning on addressing review feedback here?

I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

I agree. The PR description sounded like this was going to be the ultimate change, but it does not appear to have actually been done.

vasild commented at 8:44 AM on June 21, 2024: contributor

I re-reviewed this again and retain my ACK because IMO it is an improvement to master even without further changes because it reduces the scope where the mutex is held. I don't see the fact that it can go further as a blocker for this.

I would be happy to review changing StdMutex m_cs to Mutex m_cs here or in another PR.

The PR description sounded like this was going to be the ultimate change

well, fwiw, the description contains "not introducing that change here".

DrahtBot added the label CI failed on Sep 12, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

DrahtBot added the label CI failed on Sep 28, 2024

DrahtBot removed the label CI failed on Sep 30, 2024

hebasto marked this as a draft on Oct 11, 2024

DrahtBot added the label CI failed on Oct 25, 2024

DrahtBot removed the label CI failed on Oct 29, 2024

DrahtBot added the label CI failed on Nov 5, 2024

DrahtBot removed the label CI failed on Nov 9, 2024

DrahtBot added the label CI failed on Nov 17, 2024

DrahtBot commented at 1:48 AM on February 15, 2025: contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

vasild approved

vasild commented at 1:20 PM on February 21, 2025: contributor

ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc

(repeat my earlier comment, DrahtBot was confused)

DrahtBot commented at 12:46 AM on May 22, 2025: contributor

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

maflcko removed the label CI failed on Sep 26, 2025

maflcko commented at 11:55 AM on September 26, 2025: member

+gha ci

maflcko closed this on Sep 26, 2025

maflcko reopened this on Sep 26, 2025

DrahtBot added the label Needs rebase on Dec 17, 2025

fanquake commented at 11:06 AM on January 20, 2026: member

What is the status of this? Seems like 3/4 of the reviewers agree with:

I think it would be better if it had another commit getting rid of StdMutex/StdLockGuard. Otherwise nothing is really taking advantage of the change.

sync: Factor out `inconsistent_lock_order_detected` function

This simplifies enforcing the rule of not holding `LockData::dd_mutex`
across all `*_detected` functions in subsequent commits.

Additionally, the `[[noreturn]]` attribute has been added to the
`potential_deadlock_detected` and `double_lock_detected` functions.

d8a627ab73

sync: Release `LockData::dd_mutex` before calling `*_detected` functions

All `*_detected()` functions call `LogError()`, which requires acquiring
the `Logger::m_cs` mutex. To avoid a potential lock deadlock, the latter
must not have the `Mutex` type.

This change lifts the restriction on the `Logger::m_cs` type, allowing
it to be transitioned from `StdMutex` to our standard `Mutex` type in
subsequent commits.

f27c00fbc5

logging: Migrate `Logger::m_cs` type from `StdMutex` to `Mutex`

There is no longer a need for the `StdMutex` type, which was
specifically implemented for use in the `Logger` class.

a1bc76d7ad

logging: Migrate `LogRateLimiter::m_mutex` type to `Mutex` 94b46cfb2f

refactor: Remove unused `StdMutex` and `StdLockGuard` 9f40a2b28b

hebasto force-pushed on Jan 25, 2026

hebasto marked this as ready for review on Jan 25, 2026

hebasto marked this as a draft on Jan 25, 2026

DrahtBot removed the label Needs rebase on Jan 25, 2026

DrahtBot added the label CI failed on Jan 25, 2026

DrahtBot commented at 6:07 PM on January 25, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task test max 6 ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/21336422429/job/61409015334</sub> <sub>LLM reason (✨ experimental): Compilation failed due to multiple errors in src/logging/timer.h (string/formatting API mismatches and missing symbols).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

hebasto commented at 8:13 PM on January 25, 2026: member

With this PR the mentioned restriction has been lifted, and it is possible now to use our regular Mutex type for the Logger::m_cs mutex instead of a dedicated StdMutex type...

On second thought, I realize this is likely infeasible due to the current implementation of DEBUG_LOCKCONTENTION.

Closing.

hebasto closed this on Jan 25, 2026

Release `LockData::dd_mutex` before calling `*_detected` functions #26781

Code Coverage & Benchmarks

Reviews

Conflicts