dash(node): notify on tip changes absorbed by the CLEAN cycle (closes #854)

Closes issue #854 — the residual missed-notify path left after #853.

CLEAN-cycle Step 1 assigned m_best_share_hash without setting clean_best_changed, so Step 4 compared against the already-updated value and skipped the notify. A second latent case was found during the fix: the decision also sat inside Step 4's IsNull() guard, so a Step-1 election died on an empty Step 4.

- New think_gate.hpp holds the slot protocol so node.cpp and the KATs run the same code.
- CLEAN captures best-at-entry under the exclusive lock; the decision is computed after Step 4 and outside the IsNull() guard.
- m_rethink_pending records a think request dropped by the m_think_running skip race and services it on release, closing the mint-during-IO-phase window at all three run_think producer call sites.
- clean_tracker()'s prologue load-then-store becomes compare_exchange_strong.
- No new notify call-site: both IO phases still fire m_on_best_share_changed into the fire_share_tip_refresh helper added by #853.

Reward/consensus inert: only flag protocol and decision placement change. think() is invoked with identical arguments in identical positions; both apply_min_protocol_ratchet() calls stay byte-identical inside the same guards. No share construction, timestamp, gentx/ref_hash, coinbase, payee, PPLNS, validation, wire-message or won-block-submit code is touched. DASH-only.

Reviewed: no stall, spin, double-entry or lost wakeup under the actual thread topology (all flag touches verified IO-thread-confined); the regression pin was checked against master's real pre-image and genuinely diverges rather than passing either way. ctest -R Dash 712/712 reproduced independently; roundtrip 23/23; selftest OK.

Follow-up nits recorded separately: an RAII slot guard would eliminate the pre-existing "post() throws while slot held" stall class (master has the identical shape).

Author: frstrtr

src/impl/dash/local_mint_ledger.hpp

@@ -50,6 +50,28 @@ enum class MintVerdict {
 /// height and get_nth_parent_via_skip() is the O(log n) Bitcoin-Core skip-list
 /// walk — the same two primitives the producer walks already use. The caller
 /// holds the tracker read guard.
+///
+/// ── LOCK-SAFETY NOTE (do not delete — #828 GP-fault crash class) ────────────
+/// Both primitives are NON-CONST and mutate lazily-built caches, yet the only
+/// caller (main_dash.cpp's best-share-changed binding) holds a SHARED tracker
+/// lock, so two of them can run concurrently. That is safe TODAY, and ONLY
+/// because each cache carries its own internal LEAF mutex:
+///
+///   * get_acc_height() -> TrackerView::get_delta() populates m_deltas /
+///     m_reverse_deltas / m_delta_refs / m_reverse_delta_refs, all serialized
+///     by TrackerView::m_cache_mutex (src/sharechain/tracker_view.hpp:118-124);
+///   * get_nth_parent_via_skip() -> DistanceSkipList builds m_skips lazily,
+///     serialized by DistanceSkipList::m_skips_mutex (src/sharechain/
+///     skip_list.hpp:76-80), documented there as a leaf — taken around each
+///     individual map operation, with m_previous_fn invoked outside it, so it
+///     never nests with the tracker lock.
+///
+/// Strip either leaf mutex — e.g. a "faster" unsynchronized height/skip cache
+/// refactor — and this call becomes one thread rehashing an unordered_map
+/// while another iterates it: the exact freed-memory dereference of the #828
+/// GP-fault class. If you touch those caches, either keep the leaf mutexes or
+/// promote this call site to the EXCLUSIVE tracker lock. Do NOT reason "the
+/// tracker lock protects it" — the tracker lock is SHARED here, by design.
 template <typename ChainT>
 inline MintVerdict classify_local_mint(ChainT& chain,
                                        const uint256& best_share,

src/impl/dash/node.cpp

@@ -3,6 +3,7 @@
 #include "share.hpp"
 #include "mint_runloop.hpp"   // dash::mint::elect_best_share (election policy SSOT)
 #include "known_txs_retention.hpp"  // dash::retain_template_txs / all_txs_backable (F1/F3)
+#include "think_gate.hpp"     // dash::think:: think-slot protocol + clean-cycle tip decision (#854)
 
 #include <cassert>
 
@@ -488,16 +489,37 @@ void NodeImpl::apply_min_protocol_ratchet()
 
 void NodeImpl::run_think()
 {
+    // THREAD-CONFINEMENT (enforced, not assumed — register_template_txs
+    // precedent): the think-slot flags (m_think_running / m_rethink_pending)
+    // are IO-thread-only. Every caller is on the single ioc.run() thread: the
+    // mint hook (add_local_share), the reception path (add_verified_shares,
+    // which posts itself back to m_context after the verify pool), the 2 s
+    // advert drain, and both cycles' IO-phase epilogues. The compute thread
+    // NEVER touches them — it only runs the tracker under the exclusive lock.
+    // A compute-thread caller would break the "release then re-post" handshake
+    // below (it would re-enter the slot it still owns), so trip loudly in
+    // debug builds rather than let it be discovered in production.
+    assert(!is_compute_thread() &&
+           "run_think must not be called from the compute thread — the think "
+           "slot (m_think_running/m_rethink_pending) is IO-thread-confined");
+
     // Serialize: only one think() in flight (compute pool has 1 thread).
-    if (m_think_running.exchange(true)) {
+    // #854: a skipped request is no longer DROPPED — acquire_think_slot()
+    // records it in m_rethink_pending and the cycle that owns the slot
+    // re-posts run_think() when it releases. Without that, a local mint or a
+    // peer best-advert landing during a think/clean IO-phase (tracker lock
+    // already released, flag still true) silently loses its re-election, so
+    // the tip change never reaches the rigs until the 25 s keepalive.
+    if (!dash::think::acquire_think_slot(m_think_running, m_rethink_pending)) {
         static int skip_log = 0;
         if (skip_log++ % 20 == 0)
-            LOG_INFO << "[ASYNC-THINK] skipped — compute thread busy";
+            LOG_INFO << "[ASYNC-THINK] skipped — compute thread busy (re-think queued)";
         return;
     }
     if (!m_context) {
-        // Rig-free (KAT/standalone) node: no IO context to hop back to.
-        m_think_running.store(false);
+        // Rig-free (KAT/standalone) node: no IO context to hop back to, so
+        // there is nowhere to post a queued re-think either — drop it.
+        (void)dash::think::release_think_slot(m_think_running, m_rethink_pending);
         return;
     }
 
@@ -611,16 +633,24 @@ void NodeImpl::run_think()
 
             drain_pending_adds();
 
-            m_think_running.store(false);
+            // #854: release_think_slot() reports whether a run_think() request
+            // was dropped while this cycle held the slot — e.g. the mint that
+            // drain_pending_adds()/add_local_share() just landed. Serve it, or
+            // that share's election (and the miner notify it drives) is lost
+            // until the next timer tick.
+            const bool rethink_owed =
+                dash::think::release_think_slot(m_think_running, m_rethink_pending);
 
-            if (needs_continue)
+            if (needs_continue || rethink_owed)
                 boost::asio::post(*m_context, [this]() { run_think(); });
           } catch (const std::exception& e) {
             LOG_ERROR << "run_think() IO phase failed: " << e.what();
-            m_think_running.store(false);
+            if (dash::think::release_think_slot(m_think_running, m_rethink_pending))
+                boost::asio::post(*m_context, [this]() { run_think(); });
           } catch (...) {
             LOG_ERROR << "run_think() IO phase failed: unknown error";
-            m_think_running.store(false);
+            if (dash::think::release_think_slot(m_think_running, m_rethink_pending))
+                boost::asio::post(*m_context, [this]() { run_think(); });
           }
         });
     });
@@ -812,11 +842,12 @@ void NodeImpl::drain_peer_best_adverts()
             to_download.emplace_back(weak_peer, best);
         }
     }
-    // TODO(dash-async, follow-up): when drain_peer_best_adverts runs FROM the
-    // think() IO-phase, m_think_running is still true, so this run_think() is a
-    // no-op (skipped). A m_rethink_pending flag checked at the end of the think
-    // cycle would guarantee the re-election happens. Benign today: the 2s
-    // advert timer calls drain again shortly, and by then think() has finished.
+    // #854 (was a TODO here): when drain_peer_best_adverts runs FROM the
+    // think() IO-phase, m_think_running is still true, so this run_think()
+    // cannot start a cycle. It is no longer a no-op — acquire_think_slot()
+    // records the request in m_rethink_pending and the owning cycle re-posts
+    // run_think() when it releases, so the re-election is guaranteed rather
+    // than left to the next 2 s advert-timer tick.
     if (rethink)
         run_think();
     for (auto& [weak_peer, best] : to_download)
@@ -913,26 +944,25 @@ void NodeImpl::clean_tracker()
     if (m_clean_running.exchange(true))
         return;
 
-    // Skip if think() is already in flight — the periodic timer will retry.
-    // TODO(dash-async, follow-up): this load()-then-store() is check-then-act;
-    // a run_think() could win the m_think_running flag between the load and the
-    // store below. Benign today because both only ever run on the IO thread
-    // (the timer callbacks are serialized on the single io_context), so there
-    // is no true concurrency — but a compare_exchange on m_think_running would
-    // make the guard symmetric with the exchange() above and future-proof.
-    if (m_think_running.load()) {
-        m_clean_running.store(false);
-        return;
-    }
     if (!m_context) {
         m_clean_running.store(false);
         return;   // rig-free (KAT/standalone) node
     }
 
+    // Take the think slot, which also blocks run_think() re-entry for the
+    // duration of the clean. #854: acquire_clean_slot() is a compare_exchange,
+    // closing the load()-then-store() check-then-act window this prologue used
+    // to carry as a TODO — the guard is now symmetric with the m_clean_running
+    // exchange() above. A failure records NO pending re-think: the think cycle
+    // that won the slot performs the same election clean's Step 1 would, and
+    // the periodic timer retries the prune shortly.
+    if (!dash::think::acquire_clean_slot(m_think_running)) {
+        m_clean_running.store(false);
+        return;
+    }
+
     // Post the entire body to the compute thread: chain modifications happen
     // under the exclusive lock, never concurrent with think() or IO reads.
-    m_think_running.store(true);  // block think() re-entry during clean
-
     boost::asio::post(m_think_pool, [this]() {
       m_compute_thread_id.store(std::this_thread::get_id(), std::memory_order_relaxed);
 
@@ -941,6 +971,14 @@ void NodeImpl::clean_tracker()
       try {
         std::unique_lock lock(m_tracker_mutex);  // exclusive
 
+        // #854: the tip AS THE CLEAN CYCLE FOUND IT. clean_best_changed is
+        // decided against this, not against the value Step 1 has already
+        // written — otherwise an election absorbed by Step 1 (the case a mint
+        // landing during a think IO-phase produces) makes Step 4's comparison
+        // trivially false and no work is pushed to the rigs. Read under the
+        // exclusive lock, on the compute thread, before either think() runs.
+        const uint256 best_at_entry = m_best_share_hash;
+
         auto block_rel_height = m_block_rel_height_fn
             ? m_block_rel_height_fn
             : std::function<int32_t(uint256)>([](uint256) -> int32_t { return 0; });
@@ -1072,10 +1110,16 @@ void NodeImpl::clean_tracker()
                                           /*bits=*/0, bootstrap);
             m_last_top5_heads = std::move(result.top5_heads);
             if (!result.best.IsNull()) {
-                clean_best_changed = (m_best_share_hash != result.best);
                 m_best_share_hash = result.best;
                 apply_min_protocol_ratchet();  // v36 accept-floor ratchet (dgb node.cpp:2077 parallel)
             }
+            // #854: decide across BOTH thinks — entry tip vs exit tip. Placed
+            // outside the IsNull() guard on purpose: if Step 1 elected a new
+            // tip and Step 4's think returns nothing (e.g. everything it would
+            // have scored was just pruned), m_best_share_hash still carries
+            // Step 1's election and that IS a tip change the rigs must see.
+            clean_best_changed =
+                dash::think::clean_cycle_best_changed(best_at_entry, m_best_share_hash);
             publish_snapshot();
             flush_verified_to_leveldb();
         }
@@ -1100,6 +1144,10 @@ void NodeImpl::clean_tracker()
         try {
             if (clean_best_changed) {
                 LOG_INFO << "[CLEAN] IO-phase: work refresh (best changed)";
+                // Same single fan-out every other tip-change leg uses: the
+                // binding in main_dash.cpp calls dash::stratum::
+                // fire_share_tip_refresh (bump -> notify_all -> dashboard).
+                // No second notify call-site with its own logic.
                 if (m_on_best_share_changed)
                     m_on_best_share_changed();
                 broadcast_share(m_best_share_hash);  // re-announce new head
@@ -1110,8 +1158,14 @@ void NodeImpl::clean_tracker()
         } catch (...) {
             LOG_ERROR << "[CLEAN] IO phase failed: unknown error";
         }
-        m_think_running.store(false);
+        // #854: the clean cycle holds the think slot too, so run_think()
+        // requests that arrived during it (a mint, a peer best-advert) were
+        // queued rather than dropped — serve them now.
+        const bool rethink_owed =
+            dash::think::release_think_slot(m_think_running, m_rethink_pending);
         m_clean_running.store(false);
+        if (rethink_owed)
+            boost::asio::post(*m_context, [this]() { run_think(); });
       });
     });
 }
@@ -1379,6 +1433,11 @@ uint256 NodeImpl::add_local_share(ShareType share)
     }
 
     // Lock released — relay + rescore (both take their own locks / post).
+    // #854: this run_think() is the ONLY thing that promotes the share we just
+    // minted to the tip and thereby pushes fresh work to the rigs. It used to
+    // be dropped outright whenever it landed during a think/clean IO-phase
+    // (IO thread, tracker lock already released, m_think_running still true) —
+    // the residual missed-notify path. acquire_think_slot() now queues it.
     broadcast_share(hash);
     run_think();
     return hash;

src/impl/dash/node.hpp

@@ -146,6 +146,18 @@ class NodeImpl : public pool::BaseNode<dash::Config, dash::ShareChain, dash::Pee
     boost::asio::thread_pool m_think_pool{1};
     std::atomic<bool> m_think_running{false};
     std::atomic<bool> m_clean_running{false};
+    // #854: a run_think() request that arrives while a cycle is already in
+    // flight used to be DROPPED. It is reachable on the hot path — a local
+    // mint (add_local_share) or a peer best-advert drain landing during a
+    // think/clean IO-phase, when the tracker lock is already released but
+    // m_think_running is still true — and the dropped election is the tip
+    // change that never reaches the rigs. Set by dash::think::
+    // acquire_think_slot() when the slot is busy, consumed by
+    // dash::think::release_think_slot() at the end of the running cycle,
+    // which then re-posts run_think(). Both flags are IO-thread-only (single
+    // ioc.run() thread in main_dash); the compute thread never touches them.
+    // Protocol + rationale: src/impl/dash/think_gate.hpp.
+    std::atomic<bool> m_rethink_pending{false};
     mutable std::shared_mutex m_tracker_mutex;
 
     // ── Lock-free stats snapshot ─────────────────────────────────────────