From bd926a52a01dc55180d79c759e6aeaab413059f6 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <knstqq@gmail.com>
Date: Mon, 4 Dec 2023 17:38:47 +0700
Subject: [PATCH] fix: drop useless mutex cs_llmq_vbc to avoid deadlock (#5749)

## Issue being fixed or feature implemented
Missing changes in https://github.com/dashpay/dash/pull/5736
The prior backport of https://github.com/bitcoin/bitcoin/pull/19438 has
been needed to this particular changes: drop the mutex `cs_llmq_vbc`.

This mutex can potentially cause deadlock such as:
```
'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
 (2) 'cs_llmq_vbc' in llmq/utils.cpp:704 (in thread 'main')
 'm_mutex' in versionbits.cpp:253 (in thread 'main')
 (1) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Current lock order is:
 'cs_Shutdown' in init.cpp:220 (TRY) (in thread 'shutoff')
 (1) 'cs_main' in init.cpp:328 (in thread 'shutoff')
 (2) 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff')

Assertion failed: detected inconsistent lock order for 'llmq::cs_llmq_vbc' in llmq/context.cpp:64 (in thread 'shutoff'), details in debug log.
```


## What was done?
Drop `cs_llmq_vbc` mutex from llmq/utils

## How Has This Been Tested?
Re-started app several times -> no other deadlock happens.

## Breaking Changes
N/A

## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

---------

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
---
 src/llmq/context.cpp | 5 +----
 src/llmq/utils.cpp   | 6 ------
 src/llmq/utils.h     | 6 +++---
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/src/llmq/context.cpp b/src/llmq/context.cpp
index 2162f36ed3..3583675561 100644
--- a/src/llmq/context.cpp
+++ b/src/llmq/context.cpp
@@ -60,10 +60,7 @@ LLMQContext::~LLMQContext() {
     llmq::chainLocksHandler.reset();
     llmq::quorumManager.reset();
     llmq::quorumBlockProcessor.reset();
-    {
-        LOCK(llmq::cs_llmq_vbc);
-        llmq::llmq_versionbitscache.Clear();
-    }
+    llmq::llmq_versionbitscache.Clear();
 }
 
 void LLMQContext::Interrupt() {
diff --git a/src/llmq/utils.cpp b/src/llmq/utils.cpp
index 98daba584a..8105ac5520 100644
--- a/src/llmq/utils.cpp
+++ b/src/llmq/utils.cpp
@@ -35,7 +35,6 @@ std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(co
 namespace llmq
 {
 
-Mutex cs_llmq_vbc;
 VersionBitsCache llmq_versionbitscache;
 
 namespace utils
@@ -708,7 +707,6 @@ bool IsV19Active(gsl::not_null<const CBlockIndex*> pindex)
 
 bool IsV20Active(gsl::not_null<const CBlockIndex*> pindex)
 {
-    LOCK(cs_llmq_vbc);
     return llmq_versionbitscache.State(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V20) == ThresholdState::ACTIVE;
 }
 
@@ -716,19 +714,16 @@ bool IsMNRewardReallocationActive(gsl::not_null<const CBlockIndex*> pindex)
 {
     if (!IsV20Active(pindex)) return false;
 
-    LOCK(cs_llmq_vbc);
     return llmq_versionbitscache.State(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_MN_RR) == ThresholdState::ACTIVE;
 }
 
 ThresholdState GetV20State(gsl::not_null<const CBlockIndex*> pindex)
 {
-    LOCK(cs_llmq_vbc);
     return llmq_versionbitscache.State(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V20);
 }
 
 int GetV20Since(gsl::not_null<const CBlockIndex*> pindex)
 {
-    LOCK(cs_llmq_vbc);
     return llmq_versionbitscache.StateSinceHeight(pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V20);
 }
 
@@ -1005,7 +1000,6 @@ bool IsQuorumTypeEnabledInternal(Consensus::LLMQType llmqType, const CQuorumMana
             return true;
 
         case Consensus::LLMQType::LLMQ_TEST_V17: {
-            LOCK(cs_llmq_vbc);
             return llmq_versionbitscache.State(pindex, consensusParams, Consensus::DEPLOYMENT_TESTDUMMY) == ThresholdState::ACTIVE;
         }
         case Consensus::LLMQType::LLMQ_100_67:
diff --git a/src/llmq/utils.h b/src/llmq/utils.h
index 02037feadc..2db8da2725 100644
--- a/src/llmq/utils.h
+++ b/src/llmq/utils.h
@@ -30,10 +30,10 @@ namespace llmq
 class CQuorumManager;
 class CQuorumSnapshot;
 
-// Use a separate cache instance instead of versionbitscache to avoid locking cs_main
+// A separate cache instance instead of versionbitscache has been introduced to avoid locking cs_main
 // and dealing with all kinds of deadlocks.
-extern Mutex cs_llmq_vbc;
-extern VersionBitsCache llmq_versionbitscache GUARDED_BY(cs_llmq_vbc);
+// TODO: drop llmq_versionbitscache completely so far as VersionBitsCache do not uses anymore cs_main
+extern VersionBitsCache llmq_versionbitscache;
 
 static const bool DEFAULT_ENABLE_QUORUM_DATA_RECOVERY = true;