dash: a busy tracker must not forfeit the block-winning share (#889) (#903)

add_local_share acquires the exclusive tracker 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 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 a won block that NO
peer -- including our own oracle -- can ever record. A try_to_lock decline
therefore reproduces the FULL #887 symptom (invisible block AND lost weight),
just intermittently and far harder to notice. This is the residual hole PR #888
left open and it must ship with it.

THE FIX. The mint path carries a won_block flag from the stratum classify arm
down to the two tracker acquisitions on it, and takes a BOUNDED WAIT on that
path ONLY. The ordinary share path is byte-for-byte unchanged: Urgency::
Opportunistic is literally the same single try_to_lock, and it is the default
argument everywhere, so nothing can inherit the wait by omission.

BOUND: 2000 ms, chosen from measurement, not taste. Over a 42 h DASH sharechain
soak ([ASYNC-THINK] compute done in Nms, n=6721 post-join think cycles):
p50 125 ms, p90 193 ms, p99 300 ms, p99.9 512 ms, max 1074 ms; >500 ms in 8
cycles (0.119%), >1000 ms in exactly 1 (0.015%). 2000 ms covers 100% of the
observed steady-state distribution with ~1.9x headroom over the single worst
cycle, while capping well below the 6-15 s bootstrap think cycles the same log
shows in the first ~5 min after a cold join (a node in bootstrap has no
sharechain to mint onto and is not winning blocks). An expired budget is a
LOG_ERROR plus a forfeit counter, never a silent drop.

Polled try_lock rather than a blocking lock(): it must be bounded, and
std::shared_mutex has no timed acquire -- switching to shared_timed_mutex would
rewrite ~30 reader-discipline call sites across node.hpp/node.cpp for no gain.
The poll keeps the mutex type and every existing call site identical.

BLOCK SUBMIT ORDERING IS UNCHANGED AND UNCONDITIONAL. The won-block arm
dispatches the block via submit_block_fn_ and only THEN invokes the mint seam
(#887/#888). Whatever the wait costs, the block is already out; an ordering
witness in the new end-to-end KAT asserts submit_called was already true when
the mint ran, so a future reorder goes red.

CALLER-SIDE LOCK TRACE (#878/#881), re-verified against this tree, not taken on
trust from #888: asio handler -> StratumSession::process_message -> handle_submit
-> mining_submit carries no mutex at all (grep for mutex/lock over
stratum_server.cpp:1030-1360 is empty); mining_submit copies mint_share_fn_
under a scoped mint_share_mutex_ and releases before calling it, and
found_block_mutex_ on the block arm is likewise scoped and released first; the
mint lambda's read guard is scoped and RELEASED before add_local_share takes the
exclusive lock -- never nested. Zero locks held on entry, which is what makes a
bounded wait viable rather than a deadlock. is_compute_thread() is the
belt-and-braces guard: the compute thread already owns the lock and never waits.

NO CONSENSUS CHANGE. Share construction, target derivation and serialisation are
untouched; this changes only how hard we try to acquire a lock.

Tests folded into EXISTING allowlisted targets (no new add_executable):
  test_dash_node                  13/13 PASSED (+5, +dash/sharechain link so the
                                  KATs drive the REAL add_local_share against the
                                  REAL m_tracker_mutex held by another thread)
  test_dash_stratum_work_source   50/50 PASSED (+4, incl. an end-to-end
                                  block-target submit through an exclusively
                                  held tracker)
All 9 verified as REGISTERED AND EXECUTED under ctest (#895), with non-zero
durations. Negative control (block path forced back to Opportunistic +
won_block forced false) reddens 4 of them and leaves the ordinary-path controls
green, as designed.

SCOPE: dash only. btc/bch reach the same seam through CreateShareFn, whose
signature carries no solve class; widening it touches four more files plus three
test binders and is not the hotel-deploy path. dgb cannot mint any local share
until #884. Tracked as follow-ups, not silently assumed fixed.

Author: frstrtr

src/c2pool/main_dash.cpp

@@ -1930,12 +1930,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<dash::producer::BuiltShare> 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(
@@ -1949,7 +1981,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; });

src/impl/dash/node.cpp

@@ -1374,20 +1374,65 @@ std::vector<uint256> 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;
             }
         }

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<dash::Config, dash::ShareChain, dash::Pee
     // thread under m_tracker_mutex (mirrors btc::NodeImpl::m_best_share_hash).
     uint256 m_best_share_hash;
 
+    // #889: block-winning mints forfeited because the BOUNDED wait on the
+    // exclusive tracker lock expired. Written from the stratum IO thread only,
+    // read from anywhere (dashboard/KAT) — atomic, relaxed; it is a diagnostic
+    // gauge, not a synchronisation point.
+    std::atomic<uint64_t> 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<dash::Config, dash::ShareChain, dash::Pee
     /// (+ LevelDB persist + attempt_verify), then broadcast + run_think.
     /// Returns the share hash, or ZERO when the tracker lock is busy or the
     /// share is a duplicate (fail-closed: caller declines the mint).
-    uint256 add_local_share(ShareType share);
+    ///
+    /// #889 `block_winning`: true ONLY when this solve already cleared the COIN
+    /// BLOCK target. That share is unrepeatable — there is no next solve — and a
+    /// share that is never minted is also a block no p2pool peer can ever see
+    /// (they detect pool blocks through the sharechain, not through a block
+    /// announcement). It therefore takes a BOUNDED WAIT on the exclusive lock
+    /// instead of the single try. false (the default) is the ordinary share
+    /// path: byte-for-byte today's `try_to_lock` decline, deliberately unchanged.
+    ///
+    /// CALLER CONTRACT (#878/#881): a `block_winning` caller MUST hold no
+    /// tracker lock. The block-arm caller (main_dash.cpp's mint lambda, reached
+    /// from the stratum handler) holds zero locks; the compute thread is guarded
+    /// against explicitly inside.
+    uint256 add_local_share(ShareType share, bool block_winning = false);
+
+    /// #889 observability: block-winning mints lost because the bounded wait on
+    /// the tracker expired. MUST stay 0 in production — every increment is one
+    /// won block that is invisible to the whole p2pool network. Each one also
+    /// emits a LOG_ERROR; this counter is what makes it aggregatable instead of
+    /// a line someone has to notice.
+    uint64_t block_share_lock_forfeits() const {
+        return m_block_share_lock_forfeits.load(std::memory_order_relaxed);
+    }
 
     /// Register the current template's txs in m_known_txs so send_shares can
     /// serve remember_tx for a minted share's new_transaction_hashes. The
@@ -1017,16 +1046,29 @@ class NodeImpl : public pool::BaseNode<dash::Config, dash::ShareChain, dash::Pee
     /// RAII guard for IO-thread tracker reads.
     /// - IO thread:      acquires shared_lock(try_to_lock); falsy if busy.
     /// - Compute thread: skips locking (exclusive already held); always truthy.
+    ///
+    /// #889: an optional `urgency` selects a BOUNDED WAIT instead of the single
+    /// try. It defaults to Opportunistic, so every existing call site keeps
+    /// today's exact non-blocking behaviour; only the block-winning mint (which
+    /// has no "next solve" to fall back on) passes BlockWinning.
     class TrackerReadGuard {
         std::shared_lock<std::shared_mutex> 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<dash::Config, dash::ShareChain, dash::Pee
     /// Preferred tracker accessor for IO-thread callbacks. On the IO thread it
     /// acquires shared_lock(try_to_lock) (check `if (!guard)`); on the compute
     /// thread it skips locking (exclusive already held).
-    TrackerReadGuard read_tracker() {
-        return TrackerReadGuard(m_tracker_mutex, m_tracker, is_compute_thread());
+    ///
+    /// #889: pass tracker_acquire::Urgency::BlockWinning ONLY from a solve that
+    /// already cleared the BLOCK target, and ONLY from a caller holding zero
+    /// locks (#878/#881). Everything else keeps the default, i.e. no change.
+    TrackerReadGuard read_tracker(tracker_acquire::Urgency urgency =
+                                      tracker_acquire::Urgency::Opportunistic) {
+        return TrackerReadGuard(m_tracker_mutex, m_tracker, is_compute_thread(),
+                                urgency);
     }
 
     /// Acquire shared (reader) lock on the tracker mutex — BLOCKING. Only for

src/impl/dash/stratum/work_source.cpp

@@ -859,6 +859,12 @@ nlohmann::json DASHWorkSource::mining_submit(
             in.merkle_branches = std::move(branch_hashes);
             in.payout_script   = core::address_to_script(username);
             in.pow_hash        = pow_hash;
+            // #889: tell the mint binding this solve is a WON BLOCK, so it
+            // takes a bounded wait on the tracker rather than the ordinary
+            // try-and-decline. The block has already been dispatched above —
+            // this flag can only affect how hard we try to MINT, never the
+            // submit. On the share arm it stays false: unchanged behaviour.
+            in.won_block       = won_block;
             // ref_hash: the coinb1/coinb2 split sits immediately after the
             // 32-byte OP_RETURN ref_hash (before the 8B nonce64 slot), so the
             // commitment is the coinb1 tail. Raw LE-internal bytes — embedded

src/impl/dash/stratum/work_source.hpp

@@ -124,6 +124,16 @@ class DASHWorkSource : public core::stratum::IWorkSource
         /// Template tx hex frozen with the job (shared, may be null) — the
         /// run-loop registers these for share relay (remember_tx serving).
         std::shared_ptr<const std::vector<std::string>> 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).