From e8abf28d6d37b671f774387890bb24d15871c697 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Tue, 16 Jun 2026 17:24:17 +0000 Subject: [PATCH] =?UTF-8?q?btc(v36):=20complete=20livelock=20mirror=20?= =?UTF-8?q?=E2=80=94=20hold=20tracker=20lock=20across=20IO-thread=20share?= =?UTF-8?q?=20mutations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors LTC f445db8e onto the BTC twin. PR #97 landed only the watchdog half; the lock-yield half was inert (test-then-release). Two IO-thread sites read/mutated m_tracker.chain racing the compute-thread clean_tracker() exclusive prune (drop_tails), freeing chain nodes mid-access -> SIGSEGV (kr1z1s LTC/DGB shape): - processing_shares_phase2(): held the try_to_lock only to TEST availability, released it, then ran m_tracker.add() lock-free. The unique_lock is now held across the whole mutation body and released just before the async run_think() trigger. BTC-only backpressure cap (MAX_PENDING_ADDS) preserved. - notify_local_share(): bare m_tracker.chain.contains() in the early-return ran outside the lock. Moved under the existing try_to_lock guard (owns_lock() && contains()); on contention the inline verify is skipped and run_think() scores the share next cycle. BTC-only (src/impl/btc/node.cpp). Same non-blocking IO-thread contract (busy -> defer/queue, never block). --- src/impl/btc/node.cpp | 71 +++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/src/impl/btc/node.cpp b/src/impl/btc/node.cpp index 56ea7e5bc..997cf8444 100644 --- a/src/impl/btc/node.cpp +++ b/src/impl/btc/node.cpp @@ -393,34 +393,38 @@ void NodeImpl::processing_shares_phase2(HandleSharesData& data, NetService addr) // Non-blocking mutex: if think() holds the exclusive lock on the compute // thread, queue this batch for processing after think() releases. The IO // thread never blocks — keepalive timers and network I/O continue. - { - std::unique_lock lock(m_tracker_mutex, std::try_to_lock); - if (!lock.owns_lock()) { - // ── Backpressure (V36 livelock defense-in-depth) ────────────── - // If think() is wedged/slow the deferred queue could grow without - // bound and blow memory. Cap it: over MAX_PENDING_ADDS we DROP the - // new batch (peers re-advertise their best share, so dropped shares - // are re-requested later) and warn instead of growing unbounded. - if (m_pending_adds.size() >= MAX_PENDING_ADDS) { - LOG_WARNING << "[ASYNC-DEFER] BACKPRESSURE: pending_adds at cap (" - << m_pending_adds.size() << "/" << MAX_PENDING_ADDS - << "), dropping batch of " << data.m_items.size() - << " shares from " << addr.to_string() - << " — think() may be wedged"; - return; - } - LOG_INFO << "[ASYNC-DEFER] processing_shares_phase2: mutex busy, queuing " - << data.m_items.size() << " shares from " << addr.to_string() - << " (pending=" << m_pending_adds.size() + 1 << ")"; - m_pending_adds.push_back(PendingShareBatch{ - std::make_unique(std::move(data)), addr}); + // + // HOLD this lock across the entire mutation body below (mirrors LTC f445db8e). + // try_to_lock keeps the IO thread non-blocking — busy => queue + return. Once + // acquired we must NOT release until all m_tracker.chain mutations are done: + // the prior code released here and ran the body lock-free, letting the + // compute-thread clean_tracker() exclusive prune (drop_tails) free chain nodes + // mid-mutation -> SIGSEGV (kr1z1s LTC/DGB). Released just before run_think(). + std::unique_lock lock(m_tracker_mutex, std::try_to_lock); + if (!lock.owns_lock()) { + // ── Backpressure (V36 livelock defense-in-depth) ────────────── + // If think() is wedged/slow the deferred queue could grow without + // bound and blow memory. Cap it: over MAX_PENDING_ADDS we DROP the + // new batch (peers re-advertise their best share, so dropped shares + // are re-requested later) and warn instead of growing unbounded. + if (m_pending_adds.size() >= MAX_PENDING_ADDS) { + LOG_WARNING << "[ASYNC-DEFER] BACKPRESSURE: pending_adds at cap (" + << m_pending_adds.size() << "/" << MAX_PENDING_ADDS + << "), dropping batch of " << data.m_items.size() + << " shares from " << addr.to_string() + << " — think() may be wedged"; return; } + LOG_INFO << "[ASYNC-DEFER] processing_shares_phase2: mutex busy, queuing " + << data.m_items.size() << " shares from " << addr.to_string() + << " (pending=" << m_pending_adds.size() + 1 << ")"; + m_pending_adds.push_back(PendingShareBatch{ + std::make_unique(std::move(data)), addr}); + return; } - // Lock released — proceed with normal processing. - // No lock needed for the rest: we only enter here when think() is NOT - // running (m_tracker_mutex was available), and ASIO single-thread - // guarantees no other IO handler overlaps. + // Lock acquired and HELD across the mutation body below; released just before + // the async run_think() trigger so the compute thread can take the exclusive + // lock. ASIO single-thread still guarantees no overlapping IO handler. // Step 1: collect verified shares (skip any that failed verification, hash still null) std::vector valid_shares; @@ -581,6 +585,11 @@ void NodeImpl::processing_shares_phase2(HandleSharesData& data, NetService addr) << " chain=" << m_tracker.chain.size() << ")"; } + // Release the tracker lock before triggering think(): run_think() posts the + // think+prune job to the compute thread, which needs the exclusive lock. All + // chain mutations above are complete at this point. + lock.unlock(); + // Trigger think() after every share batch (p2pool: set_best_share after handle_shares). // p2pool calls set_best_share() after EVERY batch with new_count > 0 — no size gate. // think() scores heads and updates best_share + desired set for download_shares. @@ -805,14 +814,18 @@ void NodeImpl::notify_local_share(const uint256& share_hash) { // p2pool: set_best_share() → think() synchronously on the reactor thread. // Use think() for ALL best_share decisions, matching p2pool exactly. - if (share_hash.IsNull() || !m_tracker.chain.contains(share_hash)) + if (share_hash.IsNull()) return; - // Try inline verify — if think() holds the mutex, defer to next think() cycle. - // The share is already in the chain; think() will verify + score it. + // Both the chain.contains() read AND attempt_verify() run UNDER the tracker + // lock (mirrors LTC f445db8e). A bare m_tracker.chain.contains() here (the + // prior code) raced the compute-thread clean_tracker() exclusive prune freeing + // chain nodes -> SIGSEGV (kr1z1s LTC/DGB). try_to_lock keeps the IO thread + // non-blocking; if think()/clean holds the lock we skip the inline verify — + // the share is already in-chain and run_think() below will score it next cycle. { std::unique_lock lock(m_tracker_mutex, std::try_to_lock); - if (lock.owns_lock()) + if (lock.owns_lock() && m_tracker.chain.contains(share_hash)) m_tracker.attempt_verify(share_hash); }