Skip to content

Commit

Permalink
fix: drop useless mutex cs_llmq_vbc to avoid deadlock (dashpay#5749)
Browse files Browse the repository at this point in the history
## Issue being fixed or feature implemented
Missing changes in dashpay#5736
The prior backport of bitcoin#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 <[email protected]>
  • Loading branch information
2 people authored and ogabrielides committed Dec 4, 2023
1 parent bab3aa1 commit bd926a5
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 13 deletions.
5 changes: 1 addition & 4 deletions src/llmq/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
6 changes: 0 additions & 6 deletions src/llmq/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(co
namespace llmq
{

Mutex cs_llmq_vbc;
VersionBitsCache llmq_versionbitscache;

namespace utils
Expand Down Expand Up @@ -708,27 +707,23 @@ 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;
}

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);
}

Expand Down Expand Up @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions src/llmq/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit bd926a5

Please sign in to comment.