diff --git a/src/c2pool/main_dash.cpp b/src/c2pool/main_dash.cpp index f8e4db1af..bb8c83318 100644 --- a/src/c2pool/main_dash.cpp +++ b/src/c2pool/main_dash.cpp @@ -1907,12 +1907,44 @@ int run_node(bool testnet, const std::string& rpc_endpoint, return uint256(); } + // #889: a WON BLOCK is unrepeatable — there is no next solve to + // mint instead, and an unminted block-winning share is a block + // no p2pool peer can ever see (they detect pool blocks by + // watching the sharechain for a share meeting the block target, + // p2pool/node.py:145-147). So the block arm takes a BOUNDED WAIT + // for the tracker; the ordinary share arm keeps today's single + // try-and-decline, which is the right trade when the next solve + // mints. THE BLOCK IS ALREADY SUBMITTED at this point — the + // won-block arm in work_source.cpp dispatches it before invoking + // this seam — so no wait here can delay or endanger it. + // + // #878/#881: this lambda holds ZERO locks on entry (stratum + // asio handler -> process_message -> handle_submit -> + // mining_submit -> mint seam; verified lock-free end to end), + // which is what makes a bounded wait viable instead of a + // deadlock. The guard below is scoped and RELEASED before + // add_local_share takes the exclusive lock — never nested. + const auto urgency = in.won_block + ? dash::tracker_acquire::Urgency::BlockWinning + : dash::tracker_acquire::Urgency::Opportunistic; + std::optional built; { - auto guard = node_ptr->read_tracker(); + auto guard = node_ptr->read_tracker(urgency); if (!guard) { - LOG_WARNING << "[MINT] tracker busy — solve declined " - "(retry on next share)"; + if (in.won_block) { + LOG_ERROR << "[MINT-BLOCK] FORFEIT — tracker still " + "busy after " + << dash::tracker_acquire:: + BLOCK_SHARE_LOCK_BUDGET.count() + << "ms; cannot even REBUILD the " + "block-winning share. The block was " + "already submitted, but no p2pool peer " + "will see it."; + } else { + LOG_WARNING << "[MINT] tracker busy — solve declined " + "(retry on next share)"; + } return uint256(); } built = dash::mint::mint_from_inputs( @@ -1926,7 +1958,13 @@ int run_node(bool testnet, const std::string& rpc_endpoint, dash::ShareType share; share = new dash::DashShare(std::move(built->share)); - const uint256 minted = node_ptr->add_local_share(share); + // #889: same urgency for the tracker WRITE. add_local_share + // still returns ZERO if the bounded wait expires — the decline + // is preserved, it is just no longer silent (LOG_ERROR + a + // forfeit counter) and no longer triggered by a merely + // momentary think(). + const uint256 minted = + node_ptr->add_local_share(share, in.won_block); if (minted.IsNull()) { // Not inserted (busy/duplicate) — reclaim the allocation. share.invoke([](auto* obj) { delete obj; }); diff --git a/src/impl/dash/node.cpp b/src/impl/dash/node.cpp index 4388eddf5..17cfd4def 100644 --- a/src/impl/dash/node.cpp +++ b/src/impl/dash/node.cpp @@ -1374,20 +1374,65 @@ std::vector NodeImpl::send_shares(peer_ptr peer, return sent; } -uint256 NodeImpl::add_local_share(ShareType share) +uint256 NodeImpl::add_local_share(ShareType share, bool block_winning) { const uint256 hash = share.hash(); if (hash.IsNull()) return uint256::ZERO; { - // Exclusive tracker lock, non-blocking (architectural rule): if the - // compute thread is mid-think, DECLINE the mint (fail-closed) — the - // miner keeps the pseudoshare credit, the next solve mints. - std::unique_lock lock(m_tracker_mutex, std::try_to_lock); + // Exclusive tracker lock. + // + // ORDINARY SHARE (block_winning == false) — UNCHANGED. Non-blocking + // (architectural rule, node.hpp): if the compute thread is mid-think, + // DECLINE the mint (fail-closed) — the miner keeps the pseudoshare + // credit, the next solve mints. That trade is correct and stays exactly + // as it was; tracker_acquire::exclusive() with Urgency::Opportunistic is + // literally the same single `try_to_lock`. + // + // BLOCK-WINNING SHARE (#889) — BOUNDED WAIT. There is no next solve: the + // block is found once, so a decline here permanently forfeits the + // highest-work share this node will ever produce. Worse, it is not only + // OUR share weight. p2pool nodes detect a pool block by watching for a + // SHARE with pow_hash <= header bits target (p2pool/node.py:145-147), so + // a block-winning share that never enters the sharechain is a block NO + // peer — not even our own oracle — can ever record. A momentarily busy + // tracker must not be allowed to do that. + // + // ⚠️ THE BLOCK IS ALREADY SUBMITTED BEFORE WE GET HERE. The won-block + // arm in stratum/work_source.cpp dispatches the block via + // submit_block_fn_ and only THEN calls the mint seam (#887/#888 + // ordering, with an ordering-witness KAT pinning it). So whatever this + // wait costs, it can never delay, gate or endanger the block submit. + // + // #878/#881 GUARD: the compute thread already owns this mutex + // exclusively, so it must never wait on it. is_compute_thread() degrades + // the bounded path to a single try there. The live block-winning caller + // (main_dash.cpp's mint lambda, reached from the stratum handler via + // process_message -> handle_submit -> mining_submit) holds zero locks. + const auto urgency = block_winning + ? tracker_acquire::Urgency::BlockWinning + : tracker_acquire::Urgency::Opportunistic; + auto lock = tracker_acquire::exclusive(m_tracker_mutex, urgency, + is_compute_thread()); if (!lock.owns_lock()) { - LOG_WARNING << "[MINT] tracker busy — local share " - << hash.GetHex().substr(0, 16) << " declined (retry on next solve)"; + if (block_winning) { + // LOUD + COUNTED. A silent drop is precisely the defect; this + // is the only remaining way a won block can go unseen, and it + // must be impossible to miss in a log or a dashboard. + m_block_share_lock_forfeits.fetch_add(1, std::memory_order_relaxed); + LOG_ERROR << "[MINT-BLOCK] FORFEIT — tracker still busy after " + << tracker_acquire::BLOCK_SHARE_LOCK_BUDGET.count() + << "ms; BLOCK-WINNING share " + << hash.GetHex().substr(0, 16) + << " NOT minted. The block itself was already " + "submitted, but no p2pool peer will see it and its " + "PPLNS weight is lost. forfeits=" + << m_block_share_lock_forfeits.load(std::memory_order_relaxed); + } else { + LOG_WARNING << "[MINT] tracker busy — local share " + << hash.GetHex().substr(0, 16) << " declined (retry on next solve)"; + } return uint256::ZERO; } if (m_chain->contains(hash)) { @@ -1412,10 +1457,26 @@ uint256 NodeImpl::add_local_share(ShareType share) m_known_txs); }); if (!backable) { - LOG_WARNING << "[MINT] local share " << hash.GetHex().substr(0, 16) - << " references template tx(s) no longer retained — " - "declined (stale job past retention window; retry " - "on next solve)"; + // #889 scope note: this decline is NOT the lock hazard and is + // deliberately NOT bypassed for a block-winning share — minting + // a share whose txs we cannot serve would produce a share + // send_shares must withhold, i.e. the same invisibility by a + // different route. It is escalated to LOG_ERROR on the block + // path only so the loss is never quiet. + if (block_winning) { + LOG_ERROR << "[MINT-BLOCK] BLOCK-WINNING share " + << hash.GetHex().substr(0, 16) + << " references template tx(s) no longer retained " + "— NOT minted (stale job past the retention " + "window). The block itself was already " + "submitted; this is a separate, non-lock " + "forfeit — see register_template_txs retention."; + } else { + LOG_WARNING << "[MINT] local share " << hash.GetHex().substr(0, 16) + << " references template tx(s) no longer retained — " + "declined (stale job past retention window; retry " + "on next solve)"; + } return uint256::ZERO; } } diff --git a/src/impl/dash/node.hpp b/src/impl/dash/node.hpp index 3ba2ef88e..1257a08ac 100644 --- a/src/impl/dash/node.hpp +++ b/src/impl/dash/node.hpp @@ -22,6 +22,7 @@ #include "share_tracker.hpp" #include "peer.hpp" #include "min_protocol_gate.hpp" +#include "tracker_acquire.hpp" // #889: bounded acquisition — BLOCK-WINNING mint path only #include "auto_ratchet.hpp" // dash::apply_min_protocol_ratchet_decision (v36 accept-floor ratchet) #include "version_negotiation.hpp" // dash::version_negotiation::get_desired_version_weights (ratchet window) #include "messages.hpp" @@ -244,6 +245,12 @@ class NodeImpl : public pool::BaseNode m_block_share_lock_forfeits{0}; + // ── v36 min-protocol accept-floor ratchet (#643/#646, mirrors dgb) ────────── // Runtime P2P accept-floor, seeded from the COLD config floor (1700, accept-all) // and lifted to NEW_MINIMUM_PROTOCOL_VERSION (3600) by apply_min_protocol_ratchet() @@ -829,7 +836,29 @@ class NodeImpl : public pool::BaseNode lock_; ShareTracker& tracker_; bool ok_; public: - TrackerReadGuard(std::shared_mutex& mtx, ShareTracker& t, bool on_compute) + TrackerReadGuard(std::shared_mutex& mtx, ShareTracker& t, bool on_compute, + tracker_acquire::Urgency urgency = + tracker_acquire::Urgency::Opportunistic) : lock_(mtx, std::defer_lock), tracker_(t) { - if (on_compute) ok_ = true; // exclusive lock already held - else ok_ = lock_.try_lock(); + if (on_compute) { + ok_ = true; // exclusive lock already held + } else if (urgency == tracker_acquire::Urgency::Opportunistic) { + ok_ = lock_.try_lock(); // unchanged: single try, skip if busy + } else { + lock_ = tracker_acquire::shared(mtx, urgency, /*on_compute=*/false); + ok_ = lock_.owns_lock(); + } } TrackerReadGuard(TrackerReadGuard&&) = default; TrackerReadGuard(const TrackerReadGuard&) = delete; @@ -1046,8 +1088,14 @@ class NodeImpl : public pool::BaseNode> tx_data; + /// #889: this solve already cleared the COIN BLOCK target — the block + /// has ALREADY been dispatched by the time the mint seam is invoked + /// (#887/#888 ordering). It matters because the share is unrepeatable: + /// there is no next solve to mint instead, and p2pool peers detect a + /// pool block ONLY by seeing a share that meets the block target + /// (p2pool/node.py:145-147), so failing to mint it also makes the won + /// block invisible to the entire network. The mint binding uses this to + /// take a BOUNDED WAIT on the tracker instead of the ordinary + /// try-and-decline. false on the ordinary share arm — unchanged path. + bool won_block{false}; }; /// Returns the minted share hash, or null-uint256 when the mint was /// declined/deferred (reason logged by the callee). diff --git a/src/impl/dash/tracker_acquire.hpp b/src/impl/dash/tracker_acquire.hpp new file mode 100644 index 000000000..650b7d20b --- /dev/null +++ b/src/impl/dash/tracker_acquire.hpp @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +#pragma once + +// ═══════════════════════════════════════════════════════════════════════════ +// Bounded tracker acquisition for the BLOCK-WINNING mint path (#889) +// ═══════════════════════════════════════════════════════════════════════════ +// +// THE PROBLEM +// ----------- +// Every IO-thread tracker acquisition in this node is `try_to_lock` and +// DECLINES when the compute thread is mid-think() (node.hpp's synchronisation +// contract: "IO thread reads: shared_lock(try_to_lock) — non-blocking, skip if +// busy"). For an ordinary share that trade is sound — the decline costs one +// share and the NEXT solve mints instead. +// +// A block-winning share has no next solve. The block is found once, so a +// momentarily busy tracker permanently forfeits the highest-work share the node +// will ever produce. And because p2pool nodes detect pool blocks THROUGH the +// sharechain (p2pool/node.py:145-147 fires only for a share with +// `pow_hash <= header['bits'].target`), a share that is never minted is a block +// that no peer — including our own oracle — can ever see. The decline is +// therefore not merely share-weight loss: it reproduces the FULL #887 symptom, +// just intermittently. +// +// THE SHAPE OF THE FIX +// -------------------- +// Make the acquisition BOUNDED-BLOCKING on the block-winning path ONLY. The +// ordinary share path keeps `Urgency::Opportunistic`, i.e. exactly today's +// single `try_lock` with no behavioural change whatsoever. +// +// WHY POLLED try_lock AND NOT A BLOCKING lock() +// --------------------------------------------- +// * It must be BOUNDED. An unbounded `lock()` on the stratum IO thread would +// stall every other miner's submissions for as long as the compute thread +// wanted, with no ceiling and no diagnostic. +// * `std::shared_mutex` has NO timed acquire — that is `std::shared_timed_mutex`. +// Switching the type would rewrite ~30 explicit `std::shared_lock` +// reader-discipline call sites across node.hpp/node.cpp, i.e. a far larger and +// riskier diff than the defect warrants on a live reward path. +// * A poll loop keeps the mutex type, the reader discipline and every existing +// call site byte-identical, and gives an exact, testable deadline. +// +// COST OF THE POLL: at BLOCK_SHARE_LOCK_POLL = 200 us the worst case is 10 000 +// wakeups over the full 2 s budget; the EXPECTED case is one think() tail +// (measured p50 125 ms => ~625 wakeups). Both are noise against a solved block. +// +// FAIRNESS, HONESTLY STATED: polled try_lock is not fair. A writer poll can in +// principle be starved by an unbroken stream of shared readers. Every shared +// reader on this mutex is itself an IO-thread `try_to_lock` that holds for +// microseconds (an advert walk, a stats read), so unbroken coverage for 2 s is +// not a realistic state — and if it ever happened the bound converts it into a +// LOGGED, COUNTED forfeit rather than a wedged IO thread. That is strictly +// better than today, where the same situation is a silent drop. +// +// ⚠️ #878/#881 DEFECT CLASS — READ BEFORE ADDING A CALLER +// ------------------------------------------------------- +// A caller that already holds the tracker lock and then calls into a +// self-locking callee makes the callee dead code; making that callee BLOCKING +// turns dead code into a DEADLOCK. Every `Urgency::BlockWinning` call site must +// therefore hold ZERO locks on entry. The two live ones (main_dash.cpp's mint +// lambda and NodeImpl::add_local_share) are reached from the stratum handler +// at lock depth zero — see the trace in the PR. `on_compute_thread` below is +// the belt-and-braces guard: the compute thread already owns the exclusive +// lock, so it NEVER waits here. + +#include +#include +#include +#include +#include + +namespace dash { +namespace tracker_acquire { + +/// How badly the caller needs the lock. +enum class Urgency { + /// Today's behaviour, unchanged: one `try_lock`, decline if busy. + /// Correct for ordinary shares — the next solve mints. + Opportunistic, + /// Bounded wait. ONLY for a solve that already cleared the BLOCK target: + /// there is no next solve, so a decline is permanent and also hides the + /// block from every p2pool peer. + BlockWinning, +}; + +/// Wait budget for a block-winning acquisition. +/// +/// RATIONALE — measured, not guessed. From a 42 h DASH sharechain soak +/// (`[ASYNC-THINK] compute done in Nms`, n = 6721 think cycles, post-join +/// steady state): +/// +/// p50 = 125 ms p90 = 193 ms p99 = 300 ms p99.9 = 512 ms max = 1074 ms +/// > 500 ms: 8 cycles (0.119%) > 750 ms: 3 (0.045%) > 1000 ms: 1 (0.015%) +/// +/// 2000 ms covers 100% of the observed steady-state distribution with ~1.9x +/// headroom over the single worst cycle. It is deliberately NOT tighter: the +/// exclusive hold also covers publish_snapshot() + flush_verified_to_leveldb() +/// (a little beyond the logged think_ms), and clean_tracker() takes the same +/// exclusive lock for a think + stale-head eat + tail drop + LevelDB prune that +/// this build does not time at all. Against an unmeasured second holder, the +/// safe direction is more headroom, because the failure mode of a too-tight +/// budget is exactly the loss we are fixing. +/// +/// It is also deliberately NOT looser. During the first ~5 minutes after a +/// cold join the same log shows think cycles of 6–15 s (the bootstrap +/// full-verify pass over an empty verified set). A budget that covered THOSE +/// would stall the stratum IO thread for 15 s. A node in bootstrap has no +/// sharechain to mint onto and is not winning blocks, so covering that state is +/// worthless — capping below it is the point of having a bound at all. +inline constexpr std::chrono::milliseconds BLOCK_SHARE_LOCK_BUDGET{2000}; + +/// Re-poll interval while waiting. 200 us is ~0.16% of the p50 think tail, so +/// the added latency over a true blocking acquire is immaterial, and 10 000 +/// wakeups is the worst-case ceiling. +inline constexpr std::chrono::microseconds BLOCK_SHARE_LOCK_POLL{200}; + +/// Bounded EXCLUSIVE acquisition. +/// +/// Returns an owning `unique_lock` on success, or a NON-owning one on expiry — +/// callers MUST check `owns_lock()` exactly as they check it today, so an +/// expired budget degrades to precisely the pre-#889 decline (never a silent +/// or unchecked proceed). +/// +/// `on_compute_thread` is the #878/#881 guard: the compute thread already holds +/// this mutex exclusively, so it degrades to a single try and never waits. +template +std::unique_lock exclusive( + Mutex& m, + Urgency urgency, + bool on_compute_thread = false, + std::chrono::milliseconds budget = BLOCK_SHARE_LOCK_BUDGET, + std::chrono::microseconds poll = BLOCK_SHARE_LOCK_POLL) +{ + std::unique_lock lock(m, std::try_to_lock); + if (lock.owns_lock() || urgency == Urgency::Opportunistic || on_compute_thread + || budget <= std::chrono::milliseconds::zero()) + return lock; + + const auto deadline = std::chrono::steady_clock::now() + budget; + while (std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(poll); + if (lock.try_lock()) + return lock; + } + return lock; // non-owning — budget expired, caller declines + counts it +} + +/// Bounded SHARED acquisition. Same contract as `exclusive` above. +template +std::shared_lock shared( + Mutex& m, + Urgency urgency, + bool on_compute_thread = false, + std::chrono::milliseconds budget = BLOCK_SHARE_LOCK_BUDGET, + std::chrono::microseconds poll = BLOCK_SHARE_LOCK_POLL) +{ + std::shared_lock lock(m, std::try_to_lock); + if (lock.owns_lock() || urgency == Urgency::Opportunistic || on_compute_thread + || budget <= std::chrono::milliseconds::zero()) + return lock; + + const auto deadline = std::chrono::steady_clock::now() + budget; + while (std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(poll); + if (lock.try_lock()) + return lock; + } + return lock; +} + +} // namespace tracker_acquire +} // namespace dash diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b610250dc..010409564 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -307,6 +307,14 @@ if (BUILD_TESTING AND GTest_FOUND) ${Boost_LIBRARIES} ) target_link_libraries(test_dash_node PRIVATE c2pool_payout c2pool_hashrate c2pool_merged_mining c2pool_storage pool) # OBJECT-lib SCC direct-naming (#22/#39) + # #889: the block-winning-mint KATs drive the REAL NodeImpl::add_local_share + # against the REAL m_tracker_mutex held exclusively by another thread, so + # this target now needs the `dash` OBJECT lib (node.cpp) it previously only + # type-checked against. No new add_executable — folded into the existing + # allowlisted target. `dash`'s own deps (core pool sharechain c2pool_storage + # dash_x11 btclibs json boost secp256k1) are already on this line or come in + # transitively; nothing outside src/impl/dash is added. + target_link_libraries(test_dash_node PRIVATE dash sharechain) gtest_add_tests(test_dash_node "" AUTO) # DASH S8 per-miner work-target MODULATION KAT (work.py:308-326 oracle pin): diff --git a/test/test_dash_node.cpp b/test/test_dash_node.cpp index c59672b86..3b1ef56ec 100644 --- a/test/test_dash_node.cpp +++ b/test/test_dash_node.cpp @@ -206,4 +206,253 @@ TEST(DashNodeDispatch, NodeBridgeAliasBindsLegacyActual) static_assert(std::is_base_of_v, "Actual must derive from NodeImpl"); SUCCEED(); -} \ No newline at end of file +} + +// ═══════════════════════════════════════════════════════════════════════════ +// #889 — the BLOCK-WINNING mint must not be forfeited by a momentarily busy +// tracker. +// +// THE DEFECT. add_local_share() acquires the exclusive tracker lock with +// std::try_to_lock and DECLINES when the compute thread is mid-think(). For an +// ordinary share that is a sound trade: the decline costs one share and the +// NEXT solve mints instead. A block-winning share has no next solve — the block +// is found once — so the same decline permanently forfeits the highest-work +// share the node will ever produce. +// +// AND IT IS NOT ONLY OUR SHARE WEIGHT. p2pool nodes do not learn about a pool +// block from a block announcement; they detect it THROUGH THE SHARECHAIN, by +// watching for a share with pow_hash <= header['bits'].target +// (p2pool/node.py:145-147). A block-winning share that never enters the +// sharechain is therefore a won block that NO peer — including our own oracle — +// can ever record. The try_to_lock decline reproduces the full #887 symptom, +// just intermittently and far harder to notice. +// +// WHAT THESE TESTS PIN. Nothing existing covers this: every prior mint KAT runs +// against a FREE tracker, where try_to_lock and a bounded wait are +// indistinguishable. These hold the real m_tracker_mutex EXCLUSIVELY from +// another thread — exactly the state think() puts it in — and drive the mint. +// +// 1. block-winning + tracker held -> the share STILL LANDS (this is the fix) +// 2. ordinary share + tracker held -> still declines (the trade we must NOT +// change; also the negative control that proves test 1 is not passing +// merely because the lock happened to be free) +// 3. budget expiry -> declines LOUDLY and COUNTED, never silently +// +// NOTE (#895): these are plain TESTs, not #ifdef-guarded, so gtest_add_tests +// (AUTO) registers cases that genuinely execute. + +#include + +#include +#include +#include + +namespace { + +// Minimal locally-minted share. add_local_share() only needs a non-null +// identity hash and (for the F1-sub backability gate) an empty +// m_new_transaction_hashes — everything the real producer builds on top of that +// is share CONSTRUCTION, which #889 does not touch. Heap-allocated because +// dash::ShareType is a non-owning variant handle: the tracker takes ownership +// on a successful add, and the caller reclaims on a decline (main_dash.cpp). +dash::ShareType make_local_share(unsigned char tag) +{ + auto* s = new dash::DashShare(); + // NB uint256::begin() is a uint32_t* over 8 WORDS (core/uint256.hpp), not a + // byte pointer. Word 7 is the most significant, so it renders first in + // GetHex() -- the share is then identifiable in the very log lines these + // tests pin the behaviour of. + s->m_hash.begin()[7] = 0x89000000u | static_cast(tag); + return dash::ShareType(s); +} + +void discard(dash::ShareType& share) +{ + share.invoke([](auto* obj) { delete obj; }); +} + +// Holds the node's REAL exclusive tracker lock for `hold`, i.e. reproduces a +// think() cycle in flight. Signals once the lock is actually held so the test +// body can never race ahead of it. +class ExclusiveTrackerHolder { +public: + ExclusiveTrackerHolder(std::shared_mutex& mtx, std::chrono::milliseconds hold) + { + thread_ = std::thread([this, &mtx, hold] { + std::unique_lock lk(mtx); + held_.store(true); + std::this_thread::sleep_for(hold); + held_.store(false); + }); + // Spin until the lock is genuinely held — no sleep-and-hope. + while (!held_.load()) + std::this_thread::sleep_for(std::chrono::microseconds(100)); + } + ~ExclusiveTrackerHolder() { if (thread_.joinable()) thread_.join(); } +private: + std::atomic held_{false}; + std::thread thread_; +}; + +} // namespace + +// 9. ★ THE REGRESSION. Tracker held EXCLUSIVELY; a block-winning mint still +// lands. Pre-#889 this returned ZERO and the share — and with it every +// peer's only way of seeing the block — was gone for good. +TEST(DashBlockWinningMint, LandsWhileTrackerHeldExclusively) +{ + TestNode node; + ASSERT_EQ(node.tracker().chain.size(), 0u); + + constexpr auto kHold = std::chrono::milliseconds(400); + const auto t0 = std::chrono::steady_clock::now(); + + uint256 minted; + { + ExclusiveTrackerHolder holder(node.tracker_mutex(), kHold); + auto share = make_local_share(0xB1); + minted = node.add_local_share(share, /*block_winning=*/true); + if (minted.IsNull()) + discard(share); + } + const auto waited = std::chrono::steady_clock::now() - t0; + + // The share landed. + EXPECT_FALSE(minted.IsNull()) + << "block-winning share forfeited by a busy tracker (#889)"; + EXPECT_EQ(node.tracker().chain.size(), 1u); + // No forfeit was recorded — the wait succeeded, it did not merely give up. + EXPECT_EQ(node.block_share_lock_forfeits(), 0u); + + // It genuinely WAITED for the holder rather than finding a free lock: the + // call could not have returned before the holder released. This is what + // makes the test a real exercise of the bounded acquire and not an + // accidental pass on an idle mutex. + EXPECT_GE(waited, kHold - std::chrono::milliseconds(50)); +} + +// 10. THE TRADE WE MUST NOT CHANGE (and the in-tree negative control for #9): +// an ORDINARY share under the identical held lock still declines, exactly +// as before. If #9 ever passed because the lock was free, this would fail +// alongside it. +TEST(DashBlockWinningMint, OrdinaryShareStillDeclinesWhileTrackerHeld) +{ + TestNode node; + + uint256 minted; + { + ExclusiveTrackerHolder holder(node.tracker_mutex(), + std::chrono::milliseconds(400)); + auto share = make_local_share(0xC2); + minted = node.add_local_share(share, /*block_winning=*/false); + if (minted.IsNull()) + discard(share); + } + + EXPECT_TRUE(minted.IsNull()) + << "the ordinary share path must keep today's try_to_lock decline"; + EXPECT_EQ(node.tracker().chain.size(), 0u); + EXPECT_EQ(node.block_share_lock_forfeits(), 0u); +} + +// 11. Default argument keeps every existing caller on the ordinary path — the +// fix cannot leak into share traffic by omission. +TEST(DashBlockWinningMint, DefaultArgumentIsTheOpportunisticPath) +{ + TestNode node; + + uint256 minted; + { + ExclusiveTrackerHolder holder(node.tracker_mutex(), + std::chrono::milliseconds(300)); + auto share = make_local_share(0xD3); + minted = node.add_local_share(share); // no second argument + if (minted.IsNull()) + discard(share); + } + EXPECT_TRUE(minted.IsNull()); +} + +// 12. THE BOUND IS REAL AND THE FORFEIT IS LOUD. Hold the tracker past the +// whole budget: the mint declines (fail-closed, unchanged end state) but +// now increments a counter and logs at ERROR. A silent drop is the defect; +// an accounted one is the fix's failure mode. +// +// Runtime is BLOCK_SHARE_LOCK_BUDGET + margin by construction — that is the +// property under test, not incidental slowness. +TEST(DashBlockWinningMint, BudgetExpiryIsACountedForfeitNotASilentDrop) +{ + TestNode node; + ASSERT_EQ(node.block_share_lock_forfeits(), 0u); + + const auto over_budget = + dash::tracker_acquire::BLOCK_SHARE_LOCK_BUDGET + + std::chrono::milliseconds(300); + + uint256 minted; + std::chrono::steady_clock::duration waited{}; + { + ExclusiveTrackerHolder holder(node.tracker_mutex(), over_budget); + auto share = make_local_share(0xE4); + const auto t0 = std::chrono::steady_clock::now(); + minted = node.add_local_share(share, /*block_winning=*/true); + waited = std::chrono::steady_clock::now() - t0; + if (minted.IsNull()) + discard(share); + } + + EXPECT_TRUE(minted.IsNull()); + // It spent the WHOLE budget before giving up — not an instant try_to_lock + // decline wearing a counter. (This is also what keeps the test a fix + // detector: without the bounded wait it returns in ~0 ms.) + EXPECT_GE(waited, dash::tracker_acquire::BLOCK_SHARE_LOCK_BUDGET + - std::chrono::milliseconds(100)); + EXPECT_EQ(node.tracker().chain.size(), 0u); + EXPECT_EQ(node.block_share_lock_forfeits(), 1u) + << "an expired budget must be COUNTED, not dropped silently"; +} + +// 13. The acquisition primitive itself: Opportunistic is one try (today's +// behaviour, bit-for-bit), BlockWinning waits, and the compute-thread guard +// never waits — the #878/#881 deadlock hazard, closed at the source. +TEST(DashBlockWinningMint, AcquirePrimitiveContract) +{ + using namespace dash::tracker_acquire; + std::shared_mutex mtx; + + { // free mutex: both urgencies succeed immediately + auto a = exclusive(mtx, Urgency::Opportunistic); + EXPECT_TRUE(a.owns_lock()); + } + { + auto a = exclusive(mtx, Urgency::BlockWinning); + EXPECT_TRUE(a.owns_lock()); + } + + { // held mutex: Opportunistic gives up at once, BlockWinning waits it out + ExclusiveTrackerHolder holder(mtx, std::chrono::milliseconds(250)); + { + auto a = exclusive(mtx, Urgency::Opportunistic); + EXPECT_FALSE(a.owns_lock()); + } + { + // Compute-thread guard: already owns the exclusive lock, so it must + // NEVER wait here — waiting would be the #878/#881 self-deadlock. + auto a = exclusive(mtx, Urgency::BlockWinning, + /*on_compute_thread=*/true); + EXPECT_FALSE(a.owns_lock()); + } + { + auto a = exclusive(mtx, Urgency::BlockWinning); + EXPECT_TRUE(a.owns_lock()); + } + } + + { // budget expiry returns a NON-OWNING lock, so callers decline exactly + // as they do today rather than proceeding unlocked + ExclusiveTrackerHolder holder(mtx, std::chrono::milliseconds(400)); + auto a = exclusive(mtx, Urgency::BlockWinning, /*on_compute_thread=*/false, + std::chrono::milliseconds(50)); + EXPECT_FALSE(a.owns_lock()); + } +} diff --git a/test/test_dash_stratum_work_source.cpp b/test/test_dash_stratum_work_source.cpp index 29733c307..81336a216 100644 --- a/test/test_dash_stratum_work_source.cpp +++ b/test/test_dash_stratum_work_source.cpp @@ -23,6 +23,7 @@ #include // dash::coin::TipHashDedup, zmq_hashblock_frame_to_hex (ZMQ hashblock instant tip-notify) #include #include +#include // #889 bounded block-winning acquisition #include // CSimplifiedMNListEntry (embedded SML seed) #include // parse_cbtx (read served creditPool) #include // encode_cbtx (GBT-xcheck fallback fixture) @@ -38,11 +39,13 @@ #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -727,6 +730,230 @@ TEST(DashStratumWorkSource, MiningSubmitWonBlockDispatchesBeforeAndDespiteAThrow EXPECT_TRUE(result.get()); } +// ═════════════════════════════════════════════════════════════════════════════ +// #889 -- a BUSY TRACKER must not forfeit the block-winning share. +// +// #888 made the block arm REACH the mint seam. This is about that mint then +// being DECLINED: every tracker acquisition on the mint path is try_to_lock and +// gives up if the compute thread is mid-think(). For an ordinary share that is +// right -- the next solve mints. A block-winning share has no next solve, so +// the same decline is permanent, and because p2pool peers detect a pool block +// by watching the sharechain for a share meeting the block target +// (p2pool/node.py:145-147), the won block also becomes invisible to every node +// on the network. A momentary lock contention reproduces the whole #887 +// symptom. +// +// The seam carries `won_block` so the mint binding can take a BOUNDED WAIT on +// that path ONLY. These pin (a) the flag actually reaches the mint, set on the +// block arm and clear on the share arm, and (b) end to end through +// mining_submit: with the tracker HELD EXCLUSIVELY by another thread, a +// block-target submit still lands its share while an ordinary one still +// declines. +// ═════════════════════════════════════════════════════════════════════════════ + +// The block arm marks the solve as block-winning; nothing else about the mint +// inputs changes. +TEST(DashStratumWorkSource, MiningSubmitWonBlockFlagsTheMintAsBlockWinning) +{ + SubmitRig rig; + rig.job.share_bits = 0x207fffffu; + rig.job.block_nbits = "207fffff"; + const uint256 block_target = dash::coin::target_from_nbits(0x207fffffu); + const uint32_t nonce = rig.find_nonce([&](const uint256& pow) { + return pow <= block_target; + }); + + bool mint_called = false; + bool seen_won_block = false; + rig.ws->set_mint_share_fn( + [&](const dash::stratum::DASHWorkSource::MintShareInputs& in) -> uint256 { + mint_called = true; + seen_won_block = in.won_block; + uint256 h; + h.SetHex("6666666666666666666666666666666666666666666666666666666666666666"); + return h; + }); + + auto result = rig.submit(nonce); + ASSERT_TRUE(result.is_boolean()); + EXPECT_TRUE(result.get()); + ASSERT_TRUE(rig.fx.submit_called); // block out first, unchanged + ASSERT_TRUE(mint_called); + EXPECT_TRUE(seen_won_block) + << "the mint seam must be told this solve already won a block -- " + "without it the binding cannot know a decline is permanent (#889)"; +} + +// ...and the ordinary share arm does NOT. This is the guard that the bounded +// wait can never leak onto ordinary share traffic, where try-and-decline is +// the correct trade and must stay exactly as it is. +TEST(DashStratumWorkSource, MiningSubmitOrdinaryShareIsNotFlaggedBlockWinning) +{ + SubmitRig rig; + // Easy share target, impossible block target -> deterministic share arm. + rig.job.share_bits = 0x207fffffu; + rig.job.block_nbits = "1d00ffff"; + const uint256 share_target = dash::coin::target_from_nbits(0x207fffffu); + const uint256 block_target = dash::coin::target_from_nbits(0x1d00ffffu); + const uint32_t nonce = rig.find_nonce([&](const uint256& pow) { + return pow <= share_target && pow > block_target; + }); + + bool mint_called = false; + bool seen_won_block = true; // must be overwritten with false + rig.ws->set_mint_share_fn( + [&](const dash::stratum::DASHWorkSource::MintShareInputs& in) -> uint256 { + mint_called = true; + seen_won_block = in.won_block; + uint256 h; + h.SetHex("7777777777777777777777777777777777777777777777777777777777777777"); + return h; + }); + + auto result = rig.submit(nonce); + ASSERT_TRUE(result.is_boolean()); + EXPECT_TRUE(result.get()); + EXPECT_FALSE(rig.fx.submit_called); // not a block + ASSERT_TRUE(mint_called); + EXPECT_FALSE(seen_won_block); +} + +// ★ THE REGRESSION, end to end through mining_submit: the tracker is HELD +// EXCLUSIVELY by another thread for the whole call -- exactly the state +// think() puts it in -- and a block-target submit STILL lands its share. +// +// The bound mint here mirrors the production binding's lock discipline +// (main_dash.cpp): it derives urgency from in.won_block and acquires through +// the SAME dash::tracker_acquire helper the live path uses, so what is under +// test is the shipped mechanism, not a stand-in for it. +// +// The paired ordinary-share case is the negative control: identical held +// lock, identical helper, and it declines -- which is what proves the block +// case is not passing merely because the mutex happened to be free. +TEST(DashStratumWorkSource, WonBlockShareMintsThroughAnExclusivelyHeldTracker) +{ + std::shared_mutex tracker_mutex; // stands in for NodeImpl::m_tracker_mutex + + // Holder thread: takes the exclusive lock and keeps it, like a think() + // cycle in flight. Spin-wait until it is genuinely held so the submit can + // never race ahead of it. + std::atomic held{false}; + std::atomic stop{false}; + std::thread holder([&] { + std::unique_lock lk(tracker_mutex); + held.store(true); + while (!stop.load()) + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + }); + while (!held.load()) + std::this_thread::sleep_for(std::chrono::microseconds(100)); + + // Release the tracker shortly after the submit begins -- a think() cycle + // ends, it does not last forever. A try_to_lock mint gives up instantly and + // loses the share; a bounded one rides it out. + std::thread releaser([&] { + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + stop.store(true); + }); + + auto mint_under_lock = + [&](const dash::stratum::DASHWorkSource::MintShareInputs& in) -> uint256 { + const auto urgency = in.won_block + ? dash::tracker_acquire::Urgency::BlockWinning + : dash::tracker_acquire::Urgency::Opportunistic; + auto lk = dash::tracker_acquire::exclusive(tracker_mutex, urgency); + if (!lk.owns_lock()) + return uint256(); // declined -- share lost + uint256 h; + h.SetHex("8888888888888888888888888888888888888888888888888888888888888888"); + return h; // minted + }; + + // --- the block arm --- + SubmitRig rig; + rig.job.share_bits = 0x207fffffu; + rig.job.block_nbits = "207fffff"; + const uint256 block_target = dash::coin::target_from_nbits(0x207fffffu); + const uint32_t nonce = rig.find_nonce([&](const uint256& pow) { + return pow <= block_target; + }); + + bool minted = false; + bool block_was_out_before_the_wait = false; + rig.ws->set_mint_share_fn( + [&](const dash::stratum::DASHWorkSource::MintShareInputs& in) -> uint256 { + // ORDERING WITNESS: whatever the wait below costs, the block has + // ALREADY been dispatched. This is the invariant #888 established + // and #889 must not regress -- if a bounded wait ever moved ahead + // of the submit, this flips false. + block_was_out_before_the_wait = rig.fx.submit_called; + const uint256 h = mint_under_lock(in); + minted = !h.IsNull(); + return h; + }); + + auto result = rig.submit(nonce); + + holder.join(); + releaser.join(); + + ASSERT_TRUE(result.is_boolean()); + EXPECT_TRUE(result.get()); + ASSERT_TRUE(rig.fx.submit_called); + EXPECT_TRUE(block_was_out_before_the_wait) + << "the block submit must remain strictly first and unconditional"; + EXPECT_TRUE(minted) + << "a busy tracker forfeited the block-winning share -- the block is " + "then invisible to every p2pool peer (#889)"; +} + +// Negative control for the test above: same helper, same permanently-held +// tracker, ORDINARY share -> declines, unchanged from today. +TEST(DashStratumWorkSource, OrdinaryShareStillDeclinesThroughAHeldTracker) +{ + std::shared_mutex tracker_mutex; + std::atomic held{false}; + std::atomic stop{false}; + std::thread holder([&] { + std::unique_lock lk(tracker_mutex); + held.store(true); + while (!stop.load()) + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + }); + while (!held.load()) + std::this_thread::sleep_for(std::chrono::microseconds(100)); + + SubmitRig rig; + rig.job.share_bits = 0x207fffffu; + rig.job.block_nbits = "1d00ffff"; + const uint256 share_target = dash::coin::target_from_nbits(0x207fffffu); + const uint256 block_target = dash::coin::target_from_nbits(0x1d00ffffu); + const uint32_t nonce = rig.find_nonce([&](const uint256& pow) { + return pow <= share_target && pow > block_target; + }); + + bool minted = true; + rig.ws->set_mint_share_fn( + [&](const dash::stratum::DASHWorkSource::MintShareInputs& in) -> uint256 { + const auto urgency = in.won_block + ? dash::tracker_acquire::Urgency::BlockWinning + : dash::tracker_acquire::Urgency::Opportunistic; + auto lk = dash::tracker_acquire::exclusive(tracker_mutex, urgency); + minted = lk.owns_lock(); + return uint256(); + }); + + auto result = rig.submit(nonce); + stop.store(true); + holder.join(); + + ASSERT_TRUE(result.is_boolean()); + EXPECT_TRUE(result.get()); + EXPECT_FALSE(rig.fx.submit_called); + EXPECT_FALSE(minted) + << "ordinary shares must keep the try-and-decline trade (#889 scope)"; +} + // ═════════════════════════════════════════════════════════════════════════════ // Stale-payee fix KATs (bad-cb-payee root cause; hex-confirmed @h1517420). // DASH rotates the masternode payee EVERY block: a job must be ONE frozen