From 78d6ed2c211937213e846036d1b21484cc4653a9 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Sun, 26 Jul 2026 00:20:14 +0400 Subject: [PATCH] dash(node): notify on tip changes absorbed by the CLEAN cycle Residual of the #853 missed-notify defect. #853 fixed the callback (it now pushes mining.notify instead of only invalidating the cached payload); it did not fix whether the callback runs at all. Two pre-existing paths in src/impl/dash/node.cpp swallowed a real sharechain tip change first. (A) CLEAN Step 1 absorbed the election. clean_tracker() runs think() twice and both passes assign m_best_share_hash, but clean_best_changed was computed inside Step 4 against the value Step 1 had already written, so B != B was false and nothing was pushed. It also sat inside Step 4's IsNull() guard, so a Step-4 think that elected nothing discarded a valid Step-1 election too. (B) run_think() dropped a request outright when m_think_running was set. That is reachable on the hot path: add_local_share() (the mint) and drain_peer_best_adverts() both call it from the IO thread, and a think or clean cycle's IO-phase runs on that same thread with the flag still set while the tracker lock is already released, so the share has genuinely landed. The re-election that would promote it to the tip never ran. Both fell through to the 25s keepalive, which is non-clean (clean_jobs keys on the COIN prevhash, core/stratum_server.cpp), so the ASIC drained its queued stale work first: a rare bounded sibling burst. Fix, in one new header (src/impl/dash/think_gate.hpp) so node.cpp and the KATs run the same code: * capture the tip at CLEAN entry and decide clean_best_changed across BOTH thinks, outside Step 4's IsNull() guard; * m_rethink_pending: a run_think() that loses the slot is remembered and the owning cycle re-posts it on release. N dropped requests collapse into one re-think; * acquire_clean_slot() is a compare_exchange, closing the load-then-store check-then-act window clean_tracker()'s prologue carried as a TODO; * the resulting tip change goes through the SAME m_on_best_share_changed binding, i.e. dash::stratum::fire_share_tip_refresh. No second notify call-site. Threading: m_think_running and m_rethink_pending are IO-thread-only (single ioc.run() thread in main_dash) - the mint hook, the reception path (which posts itself back to m_context after the verify pool), the advert drain, the clean timer, and both IO-phase epilogues. The compute thread never touches them. Enforced with assert(!is_compute_thread()) in run_think(), matching the register_template_txs precedent. best_at_entry is a compute-thread local read under the exclusive tracker lock. Release clears the running flag before consuming the pending flag, so an interleaving request wins the slot outright rather than being lost. Consensus-neutral: no share bytes, no timestamp re-stamping, no gentx or ref_hash, no coinbase or payee, no PPLNS math, no share validation, no wire message, no change to the won-block submit path. Every changed file is under src/impl/dash/ plus the DASH test, so no other coin lane moves. Also: document in local_mint_ledger.hpp that classify_local_mint calls the non-const get_acc_height() and get_nth_parent_via_skip() under a SHARED tracker lock, and that this is safe only because TrackerView and DistanceSkipList carry internal leaf mutexes - so a future cache refactor does not silently reintroduce the #828 GP-fault crash class. Tests fold into the existing allowlisted target test_dash_stratum_notify_roundtrip (no new add_executable): four DashCleanCycleTipRefresh KATs and five DashThinkGate KATs, each carrying the pre-fix formula verbatim so the divergence is pinned. 23/23 pass. Closes #854. --- src/impl/dash/local_mint_ledger.hpp | 22 ++ src/impl/dash/node.cpp | 115 ++++++--- src/impl/dash/node.hpp | 12 + src/impl/dash/think_gate.hpp | 125 ++++++++++ test/test_dash_stratum_notify_roundtrip.cpp | 254 ++++++++++++++++++++ 5 files changed, 500 insertions(+), 28 deletions(-) create mode 100644 src/impl/dash/think_gate.hpp diff --git a/src/impl/dash/local_mint_ledger.hpp b/src/impl/dash/local_mint_ledger.hpp index b4527b3ee..cc7e9f814 100644 --- a/src/impl/dash/local_mint_ledger.hpp +++ b/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 inline MintVerdict classify_local_mint(ChainT& chain, const uint256& best_share, diff --git a/src/impl/dash/node.cpp b/src/impl/dash/node.cpp index 453903e1f..4388eddf5 100644 --- a/src/impl/dash/node.cpp +++ b/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 @@ -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([](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; diff --git a/src/impl/dash/node.hpp b/src/impl/dash/node.hpp index 88956df03..3ba2ef88e 100644 --- a/src/impl/dash/node.hpp +++ b/src/impl/dash/node.hpp @@ -146,6 +146,18 @@ class NodeImpl : public pool::BaseNode m_think_running{false}; std::atomic 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 m_rethink_pending{false}; mutable std::shared_mutex m_tracker_mutex; // ── Lock-free stats snapshot ───────────────────────────────────────── diff --git a/src/impl/dash/think_gate.hpp b/src/impl/dash/think_gate.hpp new file mode 100644 index 000000000..c3927e7a2 --- /dev/null +++ b/src/impl/dash/think_gate.hpp @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +#pragma once + +// ============================================================================ +// think_gate.hpp — the DASH async-think serialization protocol, in ONE place. +// +// The DASH node runs at most one tracker.think() at a time: the compute pool +// has a single thread and think() holds m_tracker_mutex exclusively. That is +// enforced by a `running` flag which run_think() exchanges on entry and the +// IO-phase clears on exit. +// +// The bug that flag alone creates (#854): a run_think() request that arrives +// while a cycle is in flight is DROPPED, not queued. It is reachable on the +// hot path — add_local_share() (node.cpp, mint) and drain_peer_best_adverts() +// both call run_think() from the IO thread, and a think cycle's IO-phase runs +// on that same thread with `running` still true (the tracker lock is already +// released, so the mint that triggered the request has really landed in the +// chain). The election that would have promoted the freshly minted share to +// the tip never runs, so the sharechain best-share-changed callback never +// fires, so no mining.notify is pushed and rigs keep hashing the previous +// prev_share_hash until the 25 s keepalive — which is itself non-clean +// (clean_jobs is keyed on the COIN prevhash, core/stratum_server.cpp), i.e. a +// bounded sibling burst. That is the residual of the #853 storm. +// +// The fix is a second flag: a request that loses the race is REMEMBERED, and +// the owner of the cycle services it when it releases. These two free +// functions are the whole protocol, so node.cpp and the KATs +// (test_dash_stratum_notify_roundtrip) exercise literally the same code. +// +// ── THREADING ─────────────────────────────────────────────────────────────── +// Both flags are touched ONLY on the IO thread in main_dash (one ioc.run() +// thread): run_think()'s prologue, clean_tracker()'s prologue (the periodic +// timer callback), and both cycles' IO-phase epilogues, which are +// boost::asio::post(*m_context, ...) handlers. The compute thread NEVER reads +// or writes either flag. They are std::atomic anyway so the release/acquire +// pairing across the compute-thread hop is well defined, and so the pattern +// stays correct if the DASH lane ever grows a second IO thread. +// +// Release ordering is deliberate: `running` is cleared BEFORE the pending flag +// is consumed. A request that interleaves at that point sees running == false, +// wins the slot outright and runs the think itself — so the re-think is served +// exactly once and never lost. The reverse order could drop it. +// +// Consensus-neutral: this file decides only WHEN an election re-runs and when +// miners are told about it. It touches no share bytes, no timestamp, no +// gentx/ref_hash, no PPLNS weight, and no part of the won-block submit path. +// ============================================================================ + +#include + +#include + +namespace dash::think { + +/// Try to take the single think slot for a run_think() cycle. +/// +/// Returns true if the caller now owns the cycle and must eventually call +/// release_think_slot(). Returns false if a cycle is already in flight — in +/// which case the request is recorded, and the owner of the running cycle will +/// re-post run_think() when it releases. +inline bool acquire_think_slot(std::atomic& running, + std::atomic& rethink_pending) +{ + if (running.exchange(true)) { + rethink_pending.store(true); // #854: remember the dropped request + return false; + } + return true; +} + +/// Try to take the single think slot for the CLEAN maintenance cycle. +/// +/// Unlike acquire_think_slot() a failure here records NOTHING: clean_tracker() +/// is periodic maintenance driven by its own timer, and the think cycle that +/// beat it to the slot already performs the election clean's Step 1 would have +/// performed. Queuing a re-think for a skipped CLEAN would just add a +/// redundant think. +/// +/// compare_exchange (not load-then-store) closes the check-then-act window the +/// pre-#854 clean_tracker() prologue carried as a TODO: the guard is now +/// symmetric with the m_clean_running exchange() above it. +inline bool acquire_clean_slot(std::atomic& running) +{ + bool expected = false; + return running.compare_exchange_strong(expected, true); +} + +/// Release the think slot at the end of a cycle's IO-phase. +/// +/// Returns true if at least one run_think() request was dropped while the +/// cycle held the slot, i.e. the caller MUST re-post run_think(). Multiple +/// dropped requests collapse into one re-think — think() is a full re-election +/// over the current chain, so one pass subsumes any number of requests. +inline bool release_think_slot(std::atomic& running, + std::atomic& rethink_pending) +{ + running.store(false); // clear FIRST — see the ordering note + return rethink_pending.exchange(false); +} + +/// Did the CLEAN cycle move the sharechain tip? +/// +/// #854: clean_tracker() runs think() TWICE — Step 1 (score the chain as +/// found) and Step 4 (re-score after stale-head eating / tail dropping) — and +/// BOTH assign m_best_share_hash. The pre-#854 code computed the "did the tip +/// move" flag inside Step 4 only, against the value Step 1 had ALREADY +/// updated: +/// +/// Step 1: m_best_share_hash = result.best; // A -> B +/// Step 4: changed = (m_best_share_hash != result.best); // B != B -> false +/// +/// so an election absorbed by Step 1 — exactly the case a mint landing during +/// a think IO-phase produces — evaluated false and pushed no work to the rigs. +/// +/// Comparing the tip at cycle ENTRY against the tip at cycle EXIT detects the +/// change wherever inside the cycle it materialised. `best_at_exit` is +/// m_best_share_hash after both thinks; it is null only on a node that has +/// never elected anything, which is not a tip change. +inline bool clean_cycle_best_changed(const uint256& best_at_entry, + const uint256& best_at_exit) +{ + return !best_at_exit.IsNull() && best_at_exit != best_at_entry; +} + +} // namespace dash::think diff --git a/test/test_dash_stratum_notify_roundtrip.cpp b/test/test_dash_stratum_notify_roundtrip.cpp index cdd444752..a3959f194 100644 --- a/test/test_dash_stratum_notify_roundtrip.cpp +++ b/test/test_dash_stratum_notify_roundtrip.cpp @@ -39,6 +39,7 @@ #include +#include #include #include #include @@ -49,6 +50,7 @@ #include // merkle_branches_raw, sha256d, EXTRANONCE2_SIZE #include // fire_share_tip_refresh (tip-change fan-out) #include // LocalMintLedger / classify_local_mint +#include // acquire/release_think_slot, clean_cycle_best_changed (#854) #include // HexStr #include @@ -465,3 +467,255 @@ TEST(DashLocalMintLedger, PendingSetIsBounded) { EXPECT_EQ(s.dropped, 10u); EXPECT_DOUBLE_EQ(s.orphan_rate, 0.0); // nothing settled -> no rate invented } + +// ═════════════════════════════════════════════════════════════════════════════ +// #854 — the RESIDUAL missed-notify paths #853 did not cover. +// +// #853 fixed the callback (it now pushes mining.notify instead of only +// invalidating the cached payload). What it could not fix from that seam is +// WHEN the callback runs at all. Two pre-existing holes in src/impl/dash/ +// node.cpp kept a real sharechain tip change from ever reaching it: +// +// (A) CLEAN Step 1 absorbs the election. clean_tracker() runs think() twice +// and BOTH passes assign m_best_share_hash; the pre-#854 code decided +// "did the tip move" inside Step 4 only, against the value Step 1 had +// ALREADY written — so an election that materialised in Step 1 compared +// equal, clean_best_changed stayed false and nothing was pushed. +// +// (B) The skipped-think race. run_think() bailed out when m_think_running +// was set and DROPPED the request. A local mint (add_local_share) or a +// peer best-advert lands on the IO thread during a think/clean IO-phase +// — tracker lock already released, flag still true — so the re-election +// that would promote the just-minted share to the tip never ran. +// +// Both then fall through to the 25 s keepalive, which is NON-clean +// (clean_jobs is keyed on the COIN prevhash, core/stratum_server.cpp:1711), so +// the ASIC drains its queued stale work first: a rare bounded sibling burst. +// +// These KATs run the REAL landed helpers (src/impl/dash/think_gate.hpp) and +// the REAL fan-out (fire_share_tip_refresh), and each one carries the +// pre-#854 formula verbatim alongside so the divergence is pinned, not +// asserted. Every pre-#854 expectation below FAILS against master. +// +// Consensus-neutral by construction: nothing here (or in the code it pins) +// reads or writes a share, a timestamp, a gentx, a ref_hash, a PPLNS weight, +// or the won-block path. It decides only when an election re-runs and when +// miners are told. +// ═════════════════════════════════════════════════════════════════════════════ + +namespace { + +// Mirrors NodeImpl::clean_tracker()'s compute phase: `best` stands in for +// m_best_share_hash, and the two arguments are what each think() elected +// (null = think returned no best, e.g. everything scorable was just pruned). +struct CleanCycleSim { + uint256 best; + + // The LANDED (#854) decision: entry tip vs exit tip, across both thinks. + bool run(const uint256& step1_best, const uint256& step4_best) { + const uint256 best_at_entry = best; + if (!step1_best.IsNull()) best = step1_best; // Step 1 + if (!step4_best.IsNull()) best = step4_best; // Step 4 + return dash::think::clean_cycle_best_changed(best_at_entry, best); + } + + // The pre-#854 decision, verbatim from master: computed inside Step 4's + // IsNull() guard, against the value Step 1 already overwrote. + bool run_pre854(const uint256& step1_best, const uint256& step4_best) { + bool clean_best_changed = false; + if (!step1_best.IsNull()) best = step1_best; + if (!step4_best.IsNull()) { + clean_best_changed = (best != step4_best); + best = step4_best; + } + return clean_best_changed; + } +}; + +// What the rigs actually see: the clean IO-phase fires the shared fan-out. +int notifies_for(bool clean_best_changed) { + SpyLog log; + SpyWorkSource ws{&log}; + SpyStratumServer ss{&log}; + SpyWebServer web{&log}; + if (clean_best_changed) + dash::stratum::fire_share_tip_refresh(&ws, &ss, &web); + return ss.notifies; +} + +} // namespace + +// (15) THE #854 REGRESSION PIN: a tip change absorbed by CLEAN Step 1 must +// still push work. Step 4 re-elects the SAME share Step 1 just installed, +// which is exactly what happens when the election has already converged +// by the time the prune finishes. +TEST(DashCleanCycleTipRefresh, TipChangeAbsorbedByStep1PushesNotify) { + const uint256 A = fill_hash(0xA1); // tip as the clean cycle found it + const uint256 B = fill_hash(0xB2); // the new tip (e.g. our fresh mint) + + CleanCycleSim sim{A}; + EXPECT_TRUE(sim.run(/*step1=*/B, /*step4=*/B)) + << "tip moved A->B inside the clean cycle but the cycle reported no change"; + EXPECT_EQ(sim.best, B); + EXPECT_EQ(notifies_for(true), 1); + + // Master's formula on the same inputs: B != B -> false -> zero notifies. + CleanCycleSim pre{A}; + EXPECT_FALSE(pre.run_pre854(B, B)); + EXPECT_EQ(notifies_for(false), 0); +} + +// (16) Step 1 elects, Step 4's think returns nothing (its scorable heads were +// pruned). Step 1's election still stands in m_best_share_hash and IS the +// tip the rigs must be moved to — master skipped the flag entirely here, +// because it lived inside Step 4's IsNull() guard. +TEST(DashCleanCycleTipRefresh, Step1ElectionSurvivesEmptyStep4) { + const uint256 A = fill_hash(0xA1), B = fill_hash(0xB2); + + CleanCycleSim sim{A}; + EXPECT_TRUE(sim.run(/*step1=*/B, /*step4=*/uint256())); + EXPECT_EQ(sim.best, B); + + CleanCycleSim pre{A}; + EXPECT_FALSE(pre.run_pre854(B, uint256())); +} + +// (17) No regression on the path master DID handle: the tip moves only in +// Step 4 (the prune changed the scoring). Both formulas agree. +TEST(DashCleanCycleTipRefresh, Step4OnlyTipChangeStillPushesNotify) { + const uint256 A = fill_hash(0xA1), C = fill_hash(0xC3); + + CleanCycleSim sim{A}; + EXPECT_TRUE(sim.run(/*step1=*/A, /*step4=*/C)); + CleanCycleSim pre{A}; + EXPECT_TRUE(pre.run_pre854(A, C)); + EXPECT_EQ(notifies_for(true), 1); +} + +// (18) A quiet clean cycle must NOT push: no tip change, no notify. Clean runs +// on a periodic timer, so a false positive here would spam every rig with +// clean_jobs=true and throw away queued work for nothing. +TEST(DashCleanCycleTipRefresh, UnchangedTipDoesNotPush) { + const uint256 A = fill_hash(0xA1); + + CleanCycleSim sim{A}; + EXPECT_FALSE(sim.run(A, A)); + EXPECT_FALSE(sim.run(uint256(), uint256())); // neither think elected + EXPECT_EQ(sim.best, A); + EXPECT_EQ(notifies_for(false), 0); + + // And a node that has never elected anything is not a tip change. + CleanCycleSim fresh{uint256()}; + EXPECT_FALSE(fresh.run(uint256(), uint256())); +} + +// (19) THE SKIP-RACE PIN: a run_think() request that loses the slot is QUEUED, +// and the owning cycle serves it on release. Pre-#854 it was dropped. +TEST(DashThinkGate, SkippedRunThinkIsQueuedAndServed) { + std::atomic running{false}, pending{false}; + + // Cycle #1 takes the slot. + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + + // IO-phase of cycle #1: a local mint lands and calls run_think(). + EXPECT_FALSE(dash::think::acquire_think_slot(running, pending)) + << "a second cycle must not start while one is in flight"; + EXPECT_TRUE(pending.load()) << "the dropped request was not remembered"; + + // Cycle #1 releases: it is told a re-think is owed, and the flag is + // consumed so the re-think happens exactly once. + EXPECT_TRUE(dash::think::release_think_slot(running, pending)); + EXPECT_FALSE(pending.load()); + EXPECT_FALSE(running.load()); + + // The owed re-think runs and finds nothing further queued. + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + EXPECT_FALSE(dash::think::release_think_slot(running, pending)); +} + +// (20) N dropped requests collapse into ONE re-think — think() is a full +// re-election over the current chain, so one pass subsumes them all. This +// is what keeps the fix from turning a burst into a think storm. +TEST(DashThinkGate, ManySkipsCollapseToOneRethink) { + std::atomic running{false}, pending{false}; + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + for (int i = 0; i < 16; ++i) + EXPECT_FALSE(dash::think::acquire_think_slot(running, pending)); + EXPECT_TRUE(dash::think::release_think_slot(running, pending)); + EXPECT_FALSE(dash::think::release_think_slot(running, pending)); +} + +// (21) An idle cycle owes nothing: no spurious re-post, no think loop. +TEST(DashThinkGate, QuietCycleOwesNoRethink) { + std::atomic running{false}, pending{false}; + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + EXPECT_FALSE(dash::think::release_think_slot(running, pending)); + EXPECT_FALSE(running.load()); +} + +// (22) The CLEAN slot is a compare_exchange (closing the pre-#854 +// load()-then-store() check-then-act window) and a lost CLEAN queues +// NOTHING — the think that beat it already performs that election, and +// clean's own periodic timer retries the prune. +TEST(DashThinkGate, CleanSlotIsAtomicAndQueuesNothing) { + std::atomic running{false}, pending{false}; + + ASSERT_TRUE(dash::think::acquire_clean_slot(running)); + EXPECT_TRUE(running.load()); + + // A think starting now must lose, and IS queued (it is a real election). + EXPECT_FALSE(dash::think::acquire_think_slot(running, pending)); + EXPECT_TRUE(pending.load()); + + // A second clean must lose, and must NOT leave the flag set on its own. + std::atomic running2{true}, pending2{false}; + EXPECT_FALSE(dash::think::acquire_clean_slot(running2)); + EXPECT_FALSE(pending2.load()); + + // Clean's IO-phase release serves the think that was queued during it. + EXPECT_TRUE(dash::think::release_think_slot(running, pending)); +} + +// (23) End to end, the production sequence: a share is minted while a think +// cycle is in its IO-phase; the queued re-think elects it as the new tip; +// the tip change fires the SAME fan-out every other leg uses, so the rigs +// are pushed a clean job. Pre-#854 the request was dropped and the rigs +// saw nothing until the (non-clean) 25 s keepalive. +TEST(DashThinkGate, MintDuringThinkIoPhaseStillReachesTheMiners) { + const uint256 OLD_TIP = fill_hash(0x51); + const uint256 MINTED = fill_hash(0x52); + + SpyLog log; + SpyWorkSource ws{&log}; + SpyStratumServer ss{&log}; + SpyWebServer web{&log}; + + std::atomic running{false}, pending{false}; + uint256 best = OLD_TIP; + + // Cycle #1 runs and elects nothing new. + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + // ... IO-phase (lock released, flag still set): add_local_share() lands + // MINTED in the chain and calls run_think(). + const bool started_now = dash::think::acquire_think_slot(running, pending); + EXPECT_FALSE(started_now); + // Cycle #1 releases and is told to serve the queued re-think. + ASSERT_TRUE(dash::think::release_think_slot(running, pending)); + + // The owed re-think: the election now sees MINTED as the best head. + ASSERT_TRUE(dash::think::acquire_think_slot(running, pending)); + const uint256 best_at_entry = best; + best = MINTED; + const bool best_changed = (best != best_at_entry); + EXPECT_FALSE(dash::think::release_think_slot(running, pending)); + + ASSERT_TRUE(best_changed); + if (best_changed) + dash::stratum::fire_share_tip_refresh(&ws, &ss, &web); + + EXPECT_EQ(ss.notifies, 1) << "the freshly minted tip never reached the rigs"; + ASSERT_EQ(log.calls.size(), 3u); + EXPECT_EQ(log.calls[0], "bump"); + EXPECT_EQ(log.calls[1], "notify"); + EXPECT_EQ(log.calls[2], "web"); +}