From 343db5ffb533b98c692e23ec1f33e4702ab88871 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 23 Oct 2023 22:33:47 +0700 Subject: [PATCH] fix: double lock of deterministicMNManager->cs (#5637) ## Issue being fixed or feature implemented fix: double lock of deterministicMNManager->cs Logs: ``` node1 2023-10-21T16:46:03.990302Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] DOUBLE LOCK DETECTED node1 2023-10-21T16:46:03.990322Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] Lock order: node1 2023-10-21T16:46:03.990339Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] 'cs_main' in miner.cpp:129 (in thread 'httpworker.1') node1 2023-10-21T16:46:03.990353Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] 'm_mempool.cs' in miner.cpp:129 (in thread 'httpworker.1') node1 2023-10-21T16:46:03.990366Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] (*) 'deterministicMNManager->cs' in evo/cbtx.cpp:114 (in thread 'httpworker.1') node1 2023-10-21T16:46:03.990439Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] (*) 'cs' in ./evo/deterministicmns.h:614 (in thread 'httpworker.1') node1 2023-10-21T16:46:04.003619Z (mocktime: 2014-12-04T17:33:19Z) [httpworker.1] Posix Signal: Aborted No debug information available for stacktrace. You should add debug information and then run: dashd -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaacwiyltnbsbkudponuxqictnftw4ylmhiqecytpoj2gkzaphcbzaaaaaaaaayeurhimekiaaav6ldwqyiuqaafwsoe5bqrjaaahz6eh2dbcsaaakhhzaaaaaaaaanreseaaaaaaacgguliaaaaaaadyauwqaaaaaaacp3daaaaaaaaamxigcaaaaaaablpulyaaaaaaadovy3yaaaaaaahj7vbaaaaaaaadnbsdaaaaaaaaaa====== ``` Part of backtrace: ``` #9 UniqueLock, std::unique_lock >::UniqueLock (fTry=false, nLine=615, pszFile=0x55a9f71a3710 "./evo/deterministicmns.h", pszName=0x55a9f719caff "cs", mutexIn=..., this=0x7f7e1e71b250) at ./sync.h:164 #10 CDeterministicMNManager::GetListForBlock (this=0x55a9f84d06b0, pindex=0x7f7db03621c0) at ./evo/deterministicmns.h:615 #11 0x000055a9f6612258 in llmq::utils::ComputeQuorumMembersByQuarterRotation (pCycleQuorumBaseBlockIndex=0x7f7db03a6930, llmqParams=...) at /usr/include/c++/12/bits/unique_ptr.h:191 #12 llmq::utils::GetAllQuorumMembers (llmqType=, pQuorumBaseBlockIndex=0x7f7db0359bc0, reset_cache=reset_cache@entry=false) at llmq/utils.cpp:150 #13 0x000055a9f694d957 in CDeterministicMNManager::HandleQuorumCommitment (qc=..., pQuorumBaseBlockIndex=, mnList=..., debugLogs=debugLogs@entry=false) at evo/deterministicmns.cpp:989 #14 0x000055a9f695c455 in CDeterministicMNManager::BuildNewListFromBlock (this=, block=..., pindexPrev=pindexPrev@entry=0x7f7db03c1ac0, state=..., view=..., mnListRet=..., debugLogs=false) at evo/deterministicmns.cpp:918 #15 0x000055a9f692e7cd in CalcCbTxMerkleRootMNList (block=..., pindexPrev=pindexPrev@entry=0x7f7db03c1ac0, merkleRootRet=..., state=..., view=...) at /usr/include/c++/12/bits/unique_ptr.h:191 #16 0x000055a9f6a352ed in BlockAssembler::CreateNewBlock (this=this@entry=0x7f7e1e71f0b0, scriptPubKeyIn=...) at ./validation.h:649 #17 0x000055a9f6771c49 in generateBlocks (chainman=..., mempool=..., evodb=..., llmq_ctx=..., coinbase_script=..., nGenerate=10, nMaxTries=) at rpc/mining.cpp:167 #18 0x000055a9f677a496 in generatetoaddress (request=...) at /usr/include/c++/12/bits/unique_ptr.h:191 #19 0x000055a9f671ea02 in CRPCCommand::CRPCCommand(char const*, char const*, UniValue (*)(JSONRPCRequest const&), std::initializer_list)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const (__closure=, result=..., request=...) at ./rpc/server.h:120 ``` ## What was done? `CDeterministicMNManager::BuildNewListFromBlock` doesn't require `cs` lock anymore ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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 Co-authored-by: pasta --- src/evo/cbtx.cpp | 16 ++++++++-------- src/evo/deterministicmns.cpp | 8 +++----- src/evo/deterministicmns.h | 6 ++---- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 6e44a0dcaa..de68e1a18e 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -111,12 +111,10 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, const bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, BlockValidationState& state, const CCoinsViewCache& view) { - LOCK(deterministicMNManager->cs); - try { - static int64_t nTimeDMN = 0; - static int64_t nTimeSMNL = 0; - static int64_t nTimeMerkle = 0; + static std::atomic nTimeDMN = 0; + static std::atomic nTimeSMNL = 0; + static std::atomic nTimeMerkle = 0; int64_t nTime1 = GetTimeMicros(); @@ -134,10 +132,12 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev int64_t nTime3 = GetTimeMicros(); nTimeSMNL += nTime3 - nTime2; LogPrint(BCLog::BENCHMARK, " - CSimplifiedMNList: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeSMNL * 0.000001); - static CSimplifiedMNList smlCached; - static uint256 merkleRootCached; - static bool mutatedCached{false}; + static Mutex cached_mutex; + static CSimplifiedMNList smlCached GUARDED_BY(cached_mutex); + static uint256 merkleRootCached GUARDED_BY(cached_mutex); + static bool mutatedCached GUARDED_BY(cached_mutex) {false}; + LOCK(cached_mutex); if (sml == smlCached) { merkleRootRet = merkleRootCached; if (mutatedCached) { diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 12baa3cfce..9b86418bce 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -613,8 +613,6 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde int nHeight = pindex->nHeight; try { - LOCK(cs); - if (!BuildNewListFromBlock(block, pindex->pprev, state, view, newList, true)) { // pass the state returned by the function above return false; @@ -626,6 +624,8 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde newList.SetBlockHash(pindex->GetBlockHash()); + LOCK(cs); + oldList = GetListForBlockInternal(pindex->pprev); diff = oldList.BuildDiff(newList); @@ -708,11 +708,9 @@ void CDeterministicMNManager::UpdatedBlockTip(const CBlockIndex* pindex) bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, BlockValidationState& state, const CCoinsViewCache& view, CDeterministicMNList& mnListRet, bool debugLogs) { - AssertLockHeld(cs); - int nHeight = pindexPrev->nHeight + 1; - CDeterministicMNList oldList = GetListForBlockInternal(pindexPrev); + CDeterministicMNList oldList = GetListForBlock(pindexPrev); CDeterministicMNList newList = oldList; newList.SetBlockHash(uint256()); // we can't know the final block hash, so better not return a (invalid) block hash newList.SetHeight(nHeight); diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index 8af688c822..7d56e3d103 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -574,10 +574,8 @@ class CDeterministicMNManager static constexpr int DISK_SNAPSHOTS = llmq_max_blocks() / DISK_SNAPSHOT_PERIOD + 1; static constexpr int LIST_DIFFS_CACHE_SIZE = DISK_SNAPSHOT_PERIOD * DISK_SNAPSHOTS; -public: - Mutex cs; - private: + Mutex cs; Mutex cs_cleanup; // We have performed CleanupCache() on this height. int did_cleanup GUARDED_BY(cs_cleanup) {0}; @@ -607,7 +605,7 @@ public: // the returned list will not contain the correct block hash (we can't know it yet as the coinbase TX is not updated yet) bool BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, BlockValidationState& state, const CCoinsViewCache& view, - CDeterministicMNList& mnListRet, bool debugLogs) EXCLUSIVE_LOCKS_REQUIRED(cs); + CDeterministicMNList& mnListRet, bool debugLogs) LOCKS_EXCLUDED(cs); static void HandleQuorumCommitment(const llmq::CFinalCommitment& qc, const CBlockIndex* pQuorumBaseBlockIndex, CDeterministicMNList& mnList, bool debugLogs); CDeterministicMNList GetListForBlock(const CBlockIndex* pindex) LOCKS_EXCLUDED(cs) {