From c92e69a13fc1597b5d5261fa8c6343e2152d1b28 Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 27 Aug 2026 16:05:10 -0500 Subject: [PATCH] miner: fix cs_main/mempool.cs lock leak on the isStake error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CreateNewBlock takes cs_main and mempool.cs unconditionally via ENTER_CRITICAL_SECTION, and releases them on every return path. Two of those LEAVE pairs — the notary-pay failure and the TestBlockValidity failure — were wrapped in `if (!isStake) { LEAVE; LEAVE; }` but still `return(0)` afterwards, so when isStake is true the function returned with both locks still held: a lock leak that deadlocks the next cs_main acquirer. The success and timelock return paths already release unconditionally, so the guard was simply wrong. Make both LEAVE pairs unconditional to match the ENTER. Behavior-identical on DragonX (a RandomX chain where staking/LWMAPOS is off, so isStake is always false and the LEAVE already ran), and correct for both isStake values. Verified: a node self-mined thousands of blocks across two mining threads (each block ENTER/LEAVEs the locks in CreateNewBlock) with height rising continuously and verifychain=true — a leak would have deadlocked immediately. The fuller RAII conversion (replace the manual ENTER/LEAVE with a scoped LOCK2 for exception safety) remains a worthwhile follow-up, but is a larger change to cs_main handling best done under its own review. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/miner.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 1a2afd327..b22cc406a 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -677,11 +677,11 @@ CBlockTemplate* CreateNewBlock(CPubKey _pk,const CScript& _scriptPubKeyIn, int32 if ( totalsats == 0 ) { LogPrintf("Could not create notary payment, trying again.\n"); - if ( !isStake ) - { - LEAVE_CRITICAL_SECTION(cs_main); - LEAVE_CRITICAL_SECTION(mempool.cs); - } + // Release unconditionally to match the unconditional ENTER above. The old + // `if(!isStake)` guard leaked cs_main/mempool.cs on the isStake path (this + // still return(0)s), while the success and timelock paths always release. + LEAVE_CRITICAL_SECTION(cs_main); + LEAVE_CRITICAL_SECTION(mempool.cs); return(0); } } else LogPrintf("vout 2 of notarization is not OP_RETURN scriptlen.%i\n", scriptlen); @@ -726,11 +726,11 @@ CBlockTemplate* CreateNewBlock(CPubKey _pk,const CScript& _scriptPubKeyIn, int32 CValidationState state; if ( !TestBlockValidity(state, *pblock, pindexPrev, false, false)) // invokes CC checks { - if ( !isStake ) - { - LEAVE_CRITICAL_SECTION(cs_main); - LEAVE_CRITICAL_SECTION(mempool.cs); - } + // Release unconditionally to match the unconditional ENTER above. The old + // `if(!isStake)` guard leaked cs_main/mempool.cs on the isStake path (this + // still return(0)s), while the success and timelock paths always release. + LEAVE_CRITICAL_SECTION(cs_main); + LEAVE_CRITICAL_SECTION(mempool.cs); LogPrintf("%s: TestBlockValidity failed!\n", __func__); //throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed"); // crashes the node, moved to GetBlockTemplate and issue return. return(0);