threading: never require logging from sync.h #34793

nit: Strictly speaking, this header should be guarded with #ifdef DEBUG_LOCKCONTENTION as is done in the master branch. Similarly, other includes should be guarded with #ifdef DEBUG_LOCKORDER.

Agreed. I had a second commit which split the includes and functions into 3 chunks: contention, order, both. I decided to leave it out for the sake of simplicity: anyone who needs this .cpp will have all of those headers available.

I can push that commit if you'd like, but I don't think it's worth the complexity. At least for now.

I can push that commit if you'd like, but I don't think it's worth the complexity. At least for now.

No need. This can be done once IWYU begins force warnings for sync.cpp.

There's another option, btw, which is to leave out all of the ifdefs in the .cpp. Because the functions are only used if the inlines in the .h aren't, it doesn't hurt to just always have the full functionality in the .cpp.

This would look like:

diff --git a/src/sync.cpp b/src/sync.cpp
index 812aa3d480e..0c6b9067fe1 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -2,6 +2,14 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

+#ifndef DEBUG_LOCKCONTENTION
+#define DEBUG_LOCKCONTENTION
+#endif
+
+#ifndef DEBUG_LOCKORDER
+#define DEBUG_LOCKORDER
+#endif
+
 #include <sync.h>

 #include <logging/timer.h>
@@ -20,14 +28,11 @@
 #include <utility>
 #include <vector>

-#ifdef DEBUG_LOCKCONTENTION
 void LogContention(const char* pszName, const char* pszFile, int nLine)
 {
     LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK);
 }
-#endif

-#ifdef DEBUG_LOCKORDER
 //
 // Early deadlock detection.
 // Problem being solved:
@@ -325,4 +330,3 @@ bool LockStackEmpty()

 bool g_debug_lockorder_abort = true;

-#endif /* DEBUG_LOCKORDER */

Though, if we were going that route, rather than playing define tricks, I'd prefer to instead create internal/external versions of each function instead. Like:

inline void LogContentionInt(const char* pszName, const char* pszFile, int nLine)
{
#ifdef DEBUG_LOCKCONTENTION
    LogContention(pszName, pszFile, nLine);
#endif
}

I think this breaks the timing component, in that the timer now only measure the time of doing the logging, but not the lock contention?

An alternative approach using a callback: https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2026-03/lock-contention-handler

Using globals is not ideal. But on the upside, it completely removes sync's dependency on logging, which I think is a nice property (and relevant for e.g. kernel).

Hmm, this seems overly complicated.

Pushed a fixed up version to https://github.com/theuni/bitcoin/commit/46de6787b96f8defd04abc5b0045073b0aa1dac5

Is that missing something compared to the callback approach?

Yeah that looks good! I'd probably modernize the strings to string_view but that's a detail.

I was curious to see what it would take to decouple sync from logging entirely, but I think your version is better when there's currently no hard requirement to decouple them.

I see, thanks!

Fwiw, I don't think the two are incompatible. For ex, your global approach would be easier after moving everything out of the header. So I think it's a good thing to do either way. Will open a fresh PR.

But also, kernel could supply a different "kernel_sync.cpp" that implements the necessary methods. For most things I'd consider that a really ugly hack, but for low-level thread debugging, I don't think it would be the end of the world.