coins: prevent DB resize from invalidating cursors #35744

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, while ResizeCache() takes the lock exclusively before replacing m_db. ResizeCache() waits for active compaction before taking the exclusive lock, allowing cursors to remain live during compaction. ResizeCache() holds cs_main while it waits, so a long-running UTXO scan can stall validation during cache rebalancing.

<details> <summary>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>

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

<!--5faf32d7da4f0f540f40219e4f7537a3-->

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.

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>

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 and destruction take it exclusively.

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

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task TSan: https://github.com/bitcoin/bitcoin/actions/runs/30319724201/job/90152871691} _{LLM reason (✨ experimental): CI failed because ThreadSanitizer reported a data race (in pthread_cond_destroy) causing the coins_tests CTest to fail.}

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