coins: prevent DB resize from invalidating cursors #35744

l0rinc commented at 10:25 PM on July 17, 2026: contributor

Problem: gettxoutsetinfo, scantxoutset, and dumptxoutset retain LevelDB iterators after releasing cs_main. While those iterators are live, AssumeUTXO cache rebalancing can call ResizeCache(), replace m_db, and cause LevelDB to abort. The issue was [reported during review of #35465](https://github.com/bitcoin/bitcoin/pull/35465#discussion_r3428902672).

Fix: Each cursor holds a shared lock on m_db_mutex for its lifetime. Compaction takes the same shared lock, allowing it to run alongside cursors. ResizeCache() takes m_db_mutex exclusively, so it waits for their shared locks to be released before replacing m_db. ResizeCache() holds cs_main while it waits, so a long-running UTXO scan can stall validation during cache rebalancing.

<details> <summary>Historical test failures without the fix</summary>

test/coins_tests.cpp:1090: error: in "coins_tests/coins_db_cursor_resize": check !CCoinsViewDBTestAccess::TryExclusiveLock(db) has failed
test/coins_tests.cpp:1102: error: in "coins_tests/coins_db_cursor_resize": check status == std::future_status::timeout has failed [0 != 1]

</details>

DrahtBot added the label UTXO Db and Indexes on Jul 17, 2026

DrahtBot commented at 10:26 PM on July 17, 2026: 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/35744.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited

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:

#36167 ([RFC] Enable -Wunused by fanquake)
#35713 (Remove boost as a unit test runner by rustaceanrob)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)

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/txdb.h:45 in d3484251b3

  38 | @@ -38,9 +39,12 @@ class CCoinsViewDB final : public CCoinsView
  39 |  protected:
  40 |      DBParams m_db_params;
  41 |      CoinsViewOptions m_options;
  42 | -    //! Prevents CompactFull() from using m_db while ResizeCache() replaces it.
  43 | -    Mutex m_db_mutex;
  44 | +    //! Prevents cursor creation while ResizeCache() replaces m_db.
  45 | +    mutable Mutex m_db_mutex;
  46 | +    //! Live cursors blocking ResizeCache() from replacing m_db.
  47 | +    mutable std::atomic_int m_cursor_count{0};

andrewtoth commented at 2:31 AM on July 18, 2026:

Thanks for tackling this.

I'm not sure about this approach with an atomic counter though. Would it make sense to instead have a shared mutex that would allow multiple consumers to take a read lock along with the cursor, which are then released in the destructor? Then only ResizeCache takes an exclusive lock? Just brainstorming here.

l0rinc commented at 4:45 AM on July 18, 2026:

Thanks, I already investigated this, but the warnings and LLM reviews scared me off because it seems to force cursors to be destroyed on the thread that created them: unlock_shared() requires the calling thread to hold the shared lock. Current callers seem to satisfy that, though, so maybe it is fine. I also found that std::shared_mutex may lose some of Core's lock checking: this Godbolt recreates it, which still warns inside a std::unique_lock<std::shared_mutex>, and my local DEBUG_LOCKORDER probe did not see the lock. What do you think?

andrewtoth commented at 9:41 PM on July 18, 2026:

it seems to force cursors to be destroyed on the thread that created them

Iterators alone are not thread-safe though, so we should not be passing them to other threads. I think in this case it should be fine.

std::shared_mutex may lose some of Core's lock checking

Do we need this here? We do use a shared_mutex already in SignatureCache without any of the lock checking machinery.

l0rinc force-pushed on Jul 25, 2026

l0rinc commented at 5:28 AM on July 25, 2026: contributor

Thanks, took your suggestions, @andrewtoth. I reworked the fix around it and split the series into a characterization test, a commit that makes ResizeCache() wait for live cursors, and a follow-up that lets cursors remain live during compaction. Let me know if you think it's better this way.

in src/txdb.cpp:76 in 1154f96c5f

andrewtoth commented at 6:02 PM on July 26, 2026:

Should we add AssertLockHeld(::cs_main); here?

andrewtoth commented at 6:30 PM on July 26, 2026: contributor

Thanks, I do like this better. What do you think of adding shared locking to our lock checking machinery? Since we now take a shared lock while inside a separate lock which is released while the shared lock continues to be held, it would require us to make a modification to make sure we're popping the correct lock being released. This would let us catch issues with DEBUG_LOCKORDER.

<details> <summary>Something like this?</summary>

diff --git a/src/sync.cpp b/src/sync.cpp
index 6d740866d4..838f420c96 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -14,6 +14,7 @@
 #include <map>
 #include <mutex>
 #include <set>
+#include <shared_mutex>
 #include <system_error>
 #include <thread>
 #include <type_traits>
@@ -31,6 +32,7 @@ void ContendedLock(std::string_view name, std::string_view file, int nLine, Lock
 }
 template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::mutex>& lock);
 template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::recursive_mutex>& lock);
+template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::shared_mutex>& lock);
 
 #endif
 
@@ -75,7 +77,7 @@ private:
     bool fTry;
     std::string mutexName;
     std::string sourceFile;
-    const std::string m_thread_name;
+    std::string m_thread_name;
     int sourceLine;
 };
 
@@ -204,13 +206,20 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
     }
 }
 
-static void pop_lock()
+static void pop_lock(void* cs)
 {
     LockData& lockdata = GetLockData();
     STDLOCK(lockdata.dd_mutex);
 
     LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
-    lock_stack.pop_back();
+    // Erase the most recent entry for this mutex rather than popping the back:
+    // locks that outlive their creating scope may be released out of LIFO order.
+    for (auto it = lock_stack.rbegin(); it != lock_stack.rend(); ++it) {
+        if (it->first == cs) {
+            lock_stack.erase(std::next(it).base());
+            break;
+        }
+    }
     if (lock_stack.empty()) {
         lockdata.m_lock_stacks.erase(std::this_thread::get_id());
     }
@@ -223,6 +232,7 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexTyp
 }
 template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
 template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
+template void EnterCritical(const char*, const char*, int, std::shared_mutex*, bool);
 
 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
 {
@@ -250,9 +260,9 @@ void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, c
     throw std::logic_error(strprintf("%s was not most recent critical section locked", guardname));
 }
 
-void LeaveCritical()
+void LeaveCritical(void* cs)
 {
-    pop_lock();
+    pop_lock(cs);
 }
 
 static std::string LocksHeld()
diff --git a/src/sync.h b/src/sync.h
index 28bc78e911..fcb870ecaf 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -14,6 +14,7 @@
 #include <cassert>
 #include <condition_variable>
 #include <mutex>
+#include <shared_mutex>
 #include <string>
 #include <thread>
 
@@ -47,7 +48,7 @@ TRY_LOCK(mutex, name);
 #ifdef DEBUG_LOCKORDER
 template <typename MutexType>
 void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false);
-void LeaveCritical();
+void LeaveCritical(void* cs);
 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
 template <typename MutexType>
 void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -65,7 +66,7 @@ extern bool g_debug_lockorder_abort;
 #else
 template <typename MutexType>
 inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false) {}
-inline void LeaveCritical() {}
+inline void LeaveCritical(void* cs) {}
 inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
 template <typename MutexType>
 inline void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs) {}
@@ -113,6 +114,7 @@ public:
     }
 
     using unique_lock = std::unique_lock<PARENT>;
+    using shared_lock = std::shared_lock<PARENT>;
 #ifdef __clang__
     //! For negative capabilities in the Clang Thread Safety Analysis.
     //! A negative requirement uses the EXCLUSIVE_LOCKS_REQUIRED attribute, in conjunction
@@ -130,6 +132,9 @@ using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
 /** Wrapped mutex: supports waiting but not recursive locking */
 using Mutex = AnnotatedMixin<std::mutex>;
 
+/** Wrapped shared mutex: supports read locking via SharedLock and exclusive locking via LOCK */
+using SharedMutex = AnnotatedMixin<std::shared_mutex>;
+
 /** Different type to mark Mutex at global scope
  *
  * Thread safety analysis can't handle negative assertions about mutexes
@@ -173,7 +178,7 @@ private:
         if (Base::try_lock()) {
             return true;
         }
-        LeaveCritical();
+        LeaveCritical(Base::mutex());
         return false;
     }
 
@@ -200,7 +205,7 @@ public:
     ~UniqueLock() UNLOCK_FUNCTION()
     {
         if (Base::owns_lock())
-            LeaveCritical();
+            LeaveCritical(Base::mutex());
     }
 
     operator bool()
@@ -224,7 +229,7 @@ public:
 
             CheckLastCritical((void*)lock.mutex(), lockname, _guardname, _file, _line);
             lock.unlock();
-            LeaveCritical();
+            LeaveCritical(lock.mutex());
             lock.swap(templock);
         }
 
@@ -253,6 +258,34 @@ public:
 // the sake of thread-safety analysis, but it is not actually used otherwise.
 #define REVERSE_LOCK(g, cs) typename std::decay<decltype(g)>::type::reverse_lock BITCOIN_UNIQUE_NAME(revlock)(g, cs, #cs, __FILE__, __LINE__)
 
+/**
+ * An RAII shared (read) lock for SharedMutex that registers with the
+ * DEBUG_LOCKORDER checker. Unlike LOCK(), an instance may outlive the scope
+ * that created it, but it must be destroyed on the thread that created it.
+ */
+template <typename MutexType>
+class SharedLock : public MutexType::shared_lock
+{
+    using Base = typename MutexType::shared_lock;
+
+public:
+    SharedLock(MutexType& mutex, const char* name, const char* file, int line)
+        : Base{mutex, std::defer_lock}
+    {
+        EnterCritical(name, file, line, Base::mutex());
+        Base::lock();
+    }
+
+    SharedLock(SharedLock&&) = default;
+
+    ~SharedLock()
+    {
+        if (Base::owns_lock()) LeaveCritical(Base::mutex());
+    }
+};
+
+#define READ_LOCK(cs) SharedLock BITCOIN_UNIQUE_NAME(sharedlock)(cs, #cs, __FILE__, __LINE__)
+
 // When locking a Mutex, require negative capability to ensure the lock
 // is not already held
 inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }

</details>

refactor: extract common lock acquisition

Move lock acquisition and contention logging into `EnterLock()` so exclusive and shared lock wrappers use the same path.
This keeps the shared-locking commit focused on its new behavior.

49d961776e

sync: add shared mutex lock checking

Raw `std::shared_mutex` users bypass `DEBUG_LOCKORDER`, lock-contention logging, and Clang thread-safety analysis.

Add `SharedMutex`, `SharedLock`, and `READ_LOCK` alongside the existing exclusive-lock wrappers.
The test covers exclusive writes, shared reads, and the lock assertions.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

4d25c38ad5

sync: support non-LIFO lock release

Lock tracking always removes the latest entry, but a long-lived shared lock can outlive an outer lock.
Releasing the outer lock would remove the shared lock and corrupt `DEBUG_LOCKORDER` state.

Identify the released mutex and erase its latest entry.
Erasing a middle entry requires `CLockLocation` to be move-assignable, so make `m_thread_name` non-const.
The test releases an outer lock while a shared lock remains live, then verifies that the inverse order is detected and the stack is emptied.

Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

852a9f0901

script: check signature cache shared locking

Use `SharedMutex`, `READ_LOCK`, and `LOCK` so `DEBUG_LOCKORDER`, lock-contention diagnostics, and Clang thread-safety analysis cover signature-cache access.
Mark `setValid` `GUARDED_BY(cs_sigcache)` to require locking at access sites.

264766d580

test: check cuckoo cache shared locking

Use the checked shared-mutex wrappers in the parallel cuckoo-cache test so lock-order and contention diagnostics cover this existing shared-lock user.

e638704c6f

l0rinc force-pushed on Jul 28, 2026

l0rinc commented at 1:27 AM on July 28, 2026: contributor

Thanks, I implemented both suggestions and pushed the reworked series.

The rework is quite heavy and needs a thorough review, but I think I managed to split it into reviewable logical chunks: common lock acquisition, checked shared-lock support, non-LIFO lock tracking, migration of SignatureCache and the parallel cuckoo-cache test, then characterization and fixes for cursor lifetime during resize, destruction, and compaction.

ResizeCache() now asserts cs_main, cursors and compaction hold m_db_mutex shared, while resize takes it exclusively.

Edit: looks like a member mutex cannot guard its own destruction - TSAN should be fixed.

l0rinc marked this as a draft on Jul 28, 2026

DrahtBot added the label CI failed on Jul 28, 2026

DrahtBot commented at 2:19 AM on July 28, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task TSan: https://github.com/bitcoin/bitcoin/actions/runs/30319724201/job/90152871691</sub> <sub>LLM reason (✨ experimental): CI failed because ThreadSanitizer reported a data race (in pthread_cond_destroy) causing the coins_tests CTest to fail.</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>

test: characterize coins DB cursor lifetime

`ResizeCache()` currently returns while a cursor remains live, and full compaction runs concurrently with cursors.

Keep the underlying `CDBWrapper` alive and use a separate path for replacement so the resize behavior can be observed without hitting the LevelDB live-iterator assertion.
Keep a cursor live during compaction to characterize the existing behavior there too.

1678b11ff2

l0rinc force-pushed on Jul 28, 2026

coins: block DB resize during cursor iteration

`gettxoutsetinfo`, `scantxoutset`, and `dumptxoutset` retain LevelDB iterators after releasing `cs_main`.
AssumeUTXO cache rebalancing can then call `ResizeCache()` and replace `m_db` while an iterator is live.
The issue was reported in https://github.com/bitcoin/bitcoin/pull/35465#discussion_r3428902672.

Each cursor holds a shared lock on `m_db_mutex` for its lifetime, while `ResizeCache()` takes the lock exclusively before replacing `m_db`.
Full compaction remains exclusive in this commit, so it waits for every live cursor to be destroyed.

Current callers keep each cursor on its creating thread, as required by `std::shared_mutex` and LevelDB iterators.

Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

05c0e6cb6f

coins: allow DB cursors during compaction

Full compaction must prevent `ResizeCache()` from replacing `m_db`, but it can run alongside cursors.

Take `m_db_mutex` shared during compaction.
`ResizeCache()` still takes it exclusively and waits for both compaction and live cursors before replacing `m_db`.
Annotate `m_compaction` with its existing `cs_main` guard.

The regression test keeps a cursor live and requires compaction to finish before releasing it.

Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>

38b8476960

l0rinc force-pushed on Jul 28, 2026

l0rinc marked this as ready for review on Jul 28, 2026

DrahtBot removed the label CI failed on Jul 28, 2026

sedited commented at 4:25 PM on September 8, 2026: contributor

Concept ACK

sedited requested review from andrewtoth on Sep 8, 2026