dash(node): notify on tip changes absorbed by the CLEAN cycle (closes #854) - #856
Merged
Conversation
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.
frstrtr
marked this pull request as ready for review
July 25, 2026 21:46
frstrtr
added a commit
that referenced
this pull request
Jul 25, 2026
…target (#858) Wires the canonical 1.67% pool-share cap (work.py:311) into the DASH producer target. We were minting at the chain difficulty floor — a share roughly every 4s at ~43 TH/s against SHARE_PERIOD 20s — which maximised the collision window for the stale-work defect fixed in #853/#856. Canonical p2pool applies this cap; we ported the helper (work_target.hpp) but never called it on DASH. Wiring: build_producer_job gains a trailing local_hash_rate; pin.desired_target becomes desired_share_target(params, rate), calling the existing modulate_desired_share_target (Cap 1 only) and flooring with params.max_target so the no-signal path is bit-identical. Rate comes from get_local_addr_rates() keyed on the MINER's P2PKH hash160 (not the fee-substituted identity, so --fee cannot misattribute hashrate). Mirrors main_ltc.cpp:4504-4550. Retarget dynamics analysed and simulated: the retarget uses min_work = ata(share.max_target), so every chain share counts at the pool target regardless of its actual bits. Our emitted target is pinned at a fixed ratio to the quantity being retargeted, so the loop gain is unchanged from stock p2pool — the cap adds no new feedback edge. Converges monotonically after a single damped overshoot to pool diff ~34,150, chain interval 20s, our share interval ~102s. PPLNS weight is ata(actual bits) x (65535-donation), so payout expectation stays exactly hashrate-proportional despite producing fewer shares. Effect on other operators is net positive: the uncapped trajectory drives pool difficulty toward ~228,000, pushing the smaller operators from 29s to 166s share intervals and shrinking the PPLNS window from ~24h to ~4.8h. Capped, they settle at -15% difficulty with a 25s interval and the window returns to ~24h. Also fixes a latent UB: average_attempts_to_target cast a double above 2^64 to uint64_t, yielding 0 and throwing "Division by zero" out of the producer path, which degrades to the non-producer coinbase — i.e. minting silently stops. Now saturates. The same bug in LTC's inlined copy is filed as issue #859. Consensus-visible surface is exactly one line (pin.desired_target). Band clip is applied last, so a stale rate can only shift position inside the band, never outside it. Canonical check() re-derives from our emitted target rather than range-checking, so the KATs pin the fixed-point property. 15 new KATs; ctest -R Dash 718/718; selftest unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #854.
What #853 left uncovered
#853 fixed the callback: the sharechain best-share-changed binding now runs
the full fan-out (
bump_work_generation→notify_all→ dashboard) viadash::stratum::fire_share_tip_refresh. What it could not fix from that seamis whether the callback runs at all. Two pre-existing paths in
src/impl/dash/node.cppswallow a real sharechain tip change before it everreaches the binding:
(A) The CLEAN cycle absorbs the election in Step 1.
clean_tracker()runsthink()twice and both passes assignm_best_share_hash:The flag was computed inside Step 4 against the value Step 1 had already
written, so an election that materialised in Step 1 compared equal,
clean_best_changedstayedfalse, and the IO-phase pushed nothing. It alsolived inside Step 4's
IsNull()guard, so a Step-4 think that electednothing (its scorable heads were just pruned) dropped a valid Step-1 election
on the floor too.
(B) The skipped-think race.
run_think()bailed out whenm_think_runningwas set and dropped the request (node.cpp:490). It isreachable on the hot path:
add_local_share()(node.cpp:1366— the mint) anddrain_peer_best_adverts()both callrun_think()from the IO thread, and athink/clean cycle's IO-phase runs on that same thread with the flag still
true (the tracker lock is already released, so the mint has genuinely landed in
the chain). The re-election that would promote the fresh share to the tip never
ran. The acknowledged
TODO(dash-async)at:813-819documented this foradverts; the mint had the same hole and no TODO.
Both then fall through to the 25 s keepalive, which is non-clean — it keys
clean_jobson the COIN prevhash (src/core/stratum_server.cpp:1711), which asharechain-only tip change does not move — so the ASIC drains its queued stale
work first: a rare bounded sibling burst. #853 turned the systematic ~76% storm
into this occasional blip; this closes the remainder.
The fix
New header
src/impl/dash/think_gate.hpp— the whole protocol in one place, sonode.cppand the KATs exercise literally the same code:acquire_think_slot(running, pending)acquire_clean_slot(running)release_think_slot(running, pending)clean_cycle_best_changed(entry, exit)const uint256 best_at_entry = m_best_share_hash;is captured under the exclusive lock before Step 1, and
clean_best_changedis computed once after Step 4 — outside theIsNull()guard, so a Step-1 election survives an empty Step 4.
m_rethink_pending. Arun_think()that loses the slot is remembered;the cycle that owns the slot re-posts
run_think()when it releases (bothin
run_think()'s IO-phase and inclean_tracker()'s). N dropped requestscollapse into one re-think —
think()is a full re-election, so one passsubsumes any number of them.
phases still call
m_on_best_share_changed(), which is the bindingdash(stratum): push new work on sharechain tip change (fixes ~76% sibling/orphan rate) #853 landed at
main_dash.cpp:1886→dash::stratum::fire_share_tip_refresh(ws, stratum_server, web).Behaviour is identical on every leg by construction.
Two
TODO(dash-async, follow-up)comments are now resolved rather thanrestated: the advert-drain no-op (B) and the
load()-then-store()check-then-act in
clean_tracker()'s prologue, which becomes acompare_exchange_strong— symmetric with them_clean_running.exchange()above it.
Threading
main_dashruns exactly oneioc.run()thread, a 1-thread RPC pool, and the1-thread compute pool.
m_think_running/m_rethink_pendingare written and read ONLY on the IOthread. Every entry point is on it: the mint hook (
add_local_share, fromthe stratum submit path), the reception path (
add_verified_shares, whichalready
post()s itself back to*m_contextafter the verify pool), the 2 sadvert drain, the periodic clean timer, and both cycles' IO-phase epilogues
(
boost::asio::post(*m_context, ...)handlers). The compute thread nevertouches either flag — it only runs the tracker under the exclusive lock and
then hops back.
run_think()now carriesassert(!is_compute_thread() && ...), the same posture asregister_template_txs'sm_known_txsconfinement assert.std::atomicso the release/acquire pairing across thecompute-thread hop stays well defined and the pattern survives a future
second IO thread.
best_at_entryis a compute-thread local, read under the exclusivem_tracker_mutexbefore eitherthink(), alongside every otherm_best_share_hashaccess in that cycle. No new lock, no new lock ordering.runningis cleared beforependingisconsumed. A request that interleaves there sees
running == false, wins theslot outright and runs the think itself — so the re-think happens exactly
once and is never lost. The reverse order could drop it.
drain_pending_adds()→add_verified_shares()→run_think(), which fires only whennew_count > 0.m_pending_addsis filled solely while a think holds thelock and is cleared on drain, so the re-post chain ends as soon as no new
shares arrive.
REWARD-SAFETY / consensus neutrality
This PR changes only when an election re-runs and when miners are told.
re-signs a share.
git difftouches noshare_producer,share.hpp,gentx, coinbase, payee,
ref_hash, merkle, PPLNS weight or difficulty code.bytes (
share_producer.hpp:589→compute_ref_hash→ coinbaseOP_RETURN→
coinb1→ merkle root → the solved 80-byte header), so re-stamping atmint would invalidate the PoW. Nothing in this diff writes a timestamp.
think(),attempt_verify, the acceptfloor and
apply_min_protocol_ratchet()are called in the same places, thesame number of times, with the same arguments. The only edit inside a
think()block is the removal of theclean_best_changedassignment fromStep 4 and its recomputation two lines later —
m_best_share_hashand theratchet call are byte-for-byte unchanged.
work_source.cpp mining_submit(block-target check + submit), before anysharechain-side bookkeeping. Pushing work earlier can only change which
template a rig works on next, never how a solved header is scored or
submitted.
MINIMUM_PROTOCOL_VERSIONbehaviour, noStaleInfostamping.src/impl/dash/(plus the DASHtest).
src/core/,src/sharechain/and the LTC/DGB/BCH/BTC lanes areuntouched, so no other coin's behaviour moves.
mining.notifythat shouldalways have been sent, is sent".
notify_all()already pushes withsend_notify_work(true, &frozen_best)(clean_jobs=true) — unchanged.Tests
Folded into the EXISTING allowlisted target
test_dash_stratum_notify_roundtrip— no newadd_executable;tools/ci/check_test_target_allowlist.pypasses (118 targets, all present).Each KAT runs the real landed helper and carries the pre-#854 formula
verbatim alongside, so the divergence is pinned rather than asserted. Every
pre854expectation below fails against master:DashCleanCycleTipRefresh.TipChangeAbsorbedByStep1PushesNotify— the dash(node): CLEAN-cycle Step-1 election absorbed without clean_best_changed — residual missed stratum notify #854pin: entry
A, Step 1 electsB, Step 4 re-electsB→ must report achange and push exactly one notify. Master's formula on the same inputs:
false, zero notifies.DashCleanCycleTipRefresh.Step1ElectionSurvivesEmptyStep4— Step 4 electsnothing; Step 1's election is still a tip change. Master:
false.DashCleanCycleTipRefresh.Step4OnlyTipChangeStillPushesNotify— noregression on the path master did handle (both formulas agree).
DashCleanCycleTipRefresh.UnchangedTipDoesNotPush— a quiet clean cycle mustNOT push (a false positive would throw away every rig's queued work on a
periodic timer), and a node that has never elected anything is not a tip
change.
DashThinkGate.SkippedRunThinkIsQueuedAndServed— the skip-race pin: thedropped request is remembered, served once, and the flag is consumed.
DashThinkGate.ManySkipsCollapseToOneRethink— 16 dropped requests → onere-think (no think storm).
DashThinkGate.QuietCycleOwesNoRethink— no spurious re-post.DashThinkGate.CleanSlotIsAtomicAndQueuesNothing— CLEAN takes the slot byCAS; a lost CLEAN queues nothing, a lost think does; clean's release serves
it.
DashThinkGate.MintDuringThinkIoPhaseStillReachesTheMiners— end to end: ashare minted during a think IO-phase → queued re-think → new tip → the SAME
fan-out, in order (
bump→notify→web).test_dash_stratum_notify_roundtripalso keeps all 14 of #853's KATs green.Also in this PR
src/impl/dash/local_mint_ledger.hpp— lock-safety note onclassify_local_mint. It calls the non-constget_acc_height()andget_nth_parent_via_skip()under a shared tracker lock (the binding atmain_dash.cpp:1897usesread_tracker()), so two of them can runconcurrently while both mutate lazily-built caches. That is safe today only
because each cache carries its own internal leaf mutex —
TrackerView::m_cache_mutex(src/sharechain/tracker_view.hpp:118-124) andDistanceSkipList::m_skips_mutex(src/sharechain/skip_list.hpp:76-80,explicitly documented as a leaf). Strip either in a future "faster cache"
refactor and this call becomes one thread rehashing an
unordered_mapwhileanother iterates it: the exact freed-memory dereference of the #828 GP-fault
class. The comment says so, and says what to do instead (keep the leaf mutexes,
or promote the call site to the exclusive lock). Comment only — no code change.
Item 5 (keepalive
clean_jobs): proposed and deliberately NOT doneThe issue suggests folding the sharechain prev into
last_prevhash_so the25 s keepalive safety net is itself
clean. I judged it not worth the riskand left it. Reasons:
last_prevhash_is a member of the sharedcore::StratumSession(
src/core/stratum_server.hpp:182), on the single comparison every coin'sclean_jobsgoes through (stratum_server.cpp:1711). DASH-only semanticscannot be injected there without either changing
clean_jobsfor LTC, DGB,BCH and BTC or adding yet another per-coin knob to a subsystem already
slated for the versioned pub/sub redesign — and that redesign's whole point
is no more band-aids on this seam (cf. the dash(s8): live-wire MnStateMachine::apply_block via on_block_connected #675 regression).
few seconds, so the sharechain prev moves between nearly every keepalive
tick;
clean_jobs=trueon a 25 s timer means rigs discard queued work onessentially every tick, for measurable ASIC dead time.
tip change now pushes through
notify_all(), which already forcesclean_jobs=true(send_notify_work(true, &frozen_best)). The remainingvalue of making the net clean is the residual window between a tip change
and the notify — which this PR is what closes.
If it is wanted anyway, the safe shape is a per-coin
StratumConfigfield carrying an extra opaque "work identity" component foldedinto the comparison key only (never onto the wire), landed with the stratum
redesign rather than ahead of it. Happy to open that as a separate issue.
Build / test
Local,
build_ci(Release, gcc-13, Conanci/conan/linux-gcc13.profile,-DCOIN_DGB=ON— the CI Linux leg's exact configure):c2pool-dashbuilds clean.--selftestOK (X11 KATs mainnet-genesis + testnet3 #1497944, CoinParamsSSOT, subsidy h=2459985, masternode 3/4 payment).
ctest -R Dash— 712/712 passed, 0 failed.test_dash_stratum_notify_roundtrip— 23/23: dash(stratum): push new work on sharechain tip change (fixes ~76% sibling/orphan rate) #853's 14 KATs plus the 9new ones.
tools/ci/check_test_target_allowlist.py— OK, 118 per-coin targets across 5coin lanes, all present in
build.yml --target.