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

[frstrtr]

Addresses the dash lane of #889.

⚠️ #889 MUST NOT BE AUTO-CLOSED BY THIS MERGE. There is deliberately no Fixes/Closes keyword above: this PR fixes dash only. btc and bch remain exposed to the identical defect and dgb inherits it the moment #884 lands. See Scope. Please close #889 by hand only once those lanes are done, or split them into follow-up issues first.

Rebased onto master (22e8c692) now that #888 has squash-merged as 3e8a88c9. This branch previously carried #888's original commits; those were dropped with git rebase --onto origin/master 3866a460, which replayed cleanly with no conflicts — the two files the merge route conflicted in (src/impl/dash/stratum/work_source.cpp, test/test_dash_stratum_work_source.cpp) were the same content arriving twice, which a rebase simply does not have to reconcile. The diff is now only the #889 work: the Urgency enum, tracker_acquire.hpp, the 2000 ms bound, the on_compute_thread guard, the forfeit counter, and the tests.

Post-rebase verification (all five coordinator checks, re-run on the rebased tree)

1. Block-arm ordering still block-dispatch-FIRST. Unchanged and confirmed by line order in the rebased work_source.cpp: submit_block_fn_(block_bytes, ...) at :1013 → dashboard fb_fn(height, ...) at :1037 → mint_solved_share(/*won_block=*/true) at :1053 → bump(true) / return at :1055-1056. My entire change to that file is a 6-line addition inside the already-downstream helper — git diff origin/master...HEAD -- src/impl/dash/stratum/work_source.cpp is one hunk, in.won_block = won_block; plus its comment. Nothing was moved or reordered.

2. The submit_called-before-mint ordering KATs pass.

[ OK ] DashStratumWorkSource.MiningSubmitWonBlockAlsoMintsTheShare                        (0 ms)
[ OK ] DashStratumWorkSource.MiningSubmitWonBlockDispatchesBeforeAndDespiteAThrowingMint  (0 ms)
[ OK ] DashStratumWorkSource.WonBlockShareMintsThroughAnExclusivelyHeldTracker          (251 ms)

The third is mine and carries its own ordering witness: it reads rig.fx.submit_called at the instant the mint runs, before it waits, and asserts it was already true.

3. Urgency::Opportunistic is still what the ordinary share path gets. Audited by grep rather than by eye, because the coordinator is right that this would not necessarily fail a test:

• add_local_share(ShareType share, bool block_winning = false) — default false.
• read_tracker(Urgency = Urgency::Opportunistic) — default Opportunistic.
• Every read_tracker( call site in src/ is bare read_tracker() except exactly one: main_dash.cpp:1933, the mint lambda, which passes the urgency derived from in.won_block.
• add_local_share has exactly one production caller — main_dash.cpp:1967 — which passes in.won_block.
• Urgency::Opportunistic short-circuits before the wait loop in both tracker_acquire::exclusive and ::shared, so it is literally one try_to_lock.

Behaviourally pinned too: OrdinaryShareStillDeclinesWhileTrackerHeld and DefaultArgumentIsTheOpportunisticPath both drive a held tracker and assert the decline still happens.

4. mint_solved_share's single-call contract survives. The two std::moves (branch_hashes at :857, coinbase at :883) are inside the helper, and the helper has exactly two call sites — :1053 (block arm) and :1064 (share arm) — in mutually exclusive classify arms, the block arm returning at :1056 before the share arm is reachable. At most one invocation per mining_submit, unchanged from #888.

5. Negative control still shows the same pairing — see Negative control below, re-run on the rebased tree with identical results: 4 block-path tests red, all 4 ordinary-path controls green.

Full suites re-run post-rebase: test_dash_node 13/13, test_dash_stratum_work_source 50/50, c2pool-dash links clean. CI drift-guard now reports 220 per-coin test target(s) across 6 coin lane(s) (widened by #893 on master) — still OK.

The residual hole

add_local_share takes the exclusive tracker with std::try_to_lock and declines if 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 a momentarily busy tracker permanently forfeits the highest-work share the node will ever produce.

Why this is more than share-weight loss

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). So a block-winning share that never enters the sharechain is a won block no peer — including our own oracle — can ever record. Established live today: 2511241 (64 conf) and 2511303, both dashd-accepted and correctly paid, and no legacy node registered either.

A try_to_lock decline therefore reproduces the full #887 symptom — invisible block and lost weight — just intermittently and far harder to notice.

Measured cost of the share-weight half alone: ~135 shares/hour minted and ~10.9 blocks/day won ⇒ each lost block-share is ~0.2% of daily PPLNS weight. The visibility half is total for that block.

Approach chosen: bounded blocking, not deferred retry

The issue offered both. I took the bounded blocking acquire (its primary suggestion), for three reasons:

1. The deferred alternative is much larger here. The mint's first tracker acquisition is a read, used to REBUILD the share (mint_from_inputs walks the chain). If it declines there is no share object to queue at all — a deferral would have to queue the raw MintShareInputs plus its frozen-job handle and re-run the whole mint from a think-completion hook: a new queue, new lifetime rules, and a new re-entrancy surface on a live reward path.
2. Bounded blocking is measurably cheap here (numbers below): the wait is entered only on a won block that lands inside a think window, and the expected wait is one think tail.
3. A deferral still cannot bound latency — it inherits whatever the next cycle does. The bound is the point.

The ordinary share path is byte-for-byte unchanged: Urgency::Opportunistic is today's single try_to_lock, and it is the default argument at every call site, so nothing can inherit the wait by omission.

Why polled try_lock and not a blocking lock()

It must be bounded — an unbounded acquire on the stratum IO thread would stall other miners' submissions with no ceiling and no diagnostic. std::shared_mutex has no timed acquire (that is std::shared_timed_mutex); switching the type would rewrite ~30 explicit std::shared_lock<std::shared_mutex> reader-discipline call sites across node.hpp/node.cpp — a far larger and riskier diff than the defect warrants. The poll keeps the mutex type and every existing call site identical, and gives an exact, testable deadline.

Fairness is stated honestly in the header: polled try_lock is not fair, and could in principle be starved by an unbroken stream of shared readers. Every shared reader on this mutex is itself an IO-thread try_to_lock holding for microseconds, so unbroken 2 s coverage is not a realistic state — and if it happened the bound turns it into a logged, counted forfeit instead of a wedged IO thread. Strictly better than today, where the same situation is a silent drop.

★ The bound: 2000 ms, and where the number comes from

Measured, not guessed. From a 42 h DASH sharechain soak — [ASYNC-THINK] compute done in Nms, n = 6721 post-join think cycles:

p50	p90	p99	p99.9	max
125 ms	193 ms	300 ms	512 ms	1074 ms

>250 ms: 242 (3.60%) · >500 ms: 8 (0.119%) · >750 ms: 3 (0.045%) · >1000 ms: 1 (0.015%)

2000 ms covers 100% of the observed steady-state distribution, with ~1.9x headroom over the single worst cycle.

• Not tighter, because the exclusive hold also covers publish_snapshot() + flush_verified_to_leveldb() (slightly beyond the logged think_ms), and clean_tracker() takes the same exclusive lock for a think + stale-head eat + tail drop + LevelDB prune which this build does not time at all. Against an unmeasured second holder the safe direction is more headroom — the failure mode of a too-tight budget is exactly the loss being fixed.
• Not looser, because the same log shows 6–15 s think cycles in the first ~5 minutes after a cold join (the bootstrap full-verify pass over an empty verified set). A budget covering those would stall the stratum IO thread for 15 s. A node in bootstrap has no sharechain to mint onto and is not winning blocks, so covering that state is worthless — capping below it is the whole point of having a bound.

Expected cost in production. Total exclusive-lock duty cycle over the soak is ~0.7%, so of ~10.9 block solves/day roughly one every two weeks enters the wait at all, and its expected wait is one think tail (~125 ms). Worst case is 2 s per acquisition. Both acquisitions on the path use the same budget, so the theoretical ceiling is 4 s — but the second is near-free whenever the first succeeded, because succeeding means the holder had already released. Nothing is lost during a stall: miner submissions sit in the socket buffer, jobs are not invalidated, and vardiff credit is not affected.

On expiry: LOG_ERROR + m_block_share_lock_forfeits, a counter exposed via block_share_lock_forfeits(). It must stay 0 in production; every increment is one won block invisible to the whole network. A silent drop is the defect — an accounted one is the fix's failure mode.

★ Block submit stays FIRST and UNCONDITIONAL

Not regressed, and verified two ways.

Statically. The won-block arm in src/impl/dash/stratum/work_source.cpp runs the payee guard, dispatches through submit_block_fn_, records the dashboard entry, and only then calls mint_solved_share(true). My change adds one field assignment (in.won_block = won_block) inside that already-downstream helper. Nothing was moved. If the bounded wait blocks, the block has already been submitted.

Dynamically. The new end-to-end KAT carries an ordering witness: the bound mint records rig.fx.submit_called at the instant it runs, before it waits, and the test asserts it was already true. Any future reorder of the mint ahead of the submit turns that assertion red.

★ Caller-side lock trace — re-verified against this tree

The #878/#881 hazard is that a caller holding the exclusive tracker lock across a call into a self-locking callee makes the callee dead code — and making it blocking in that situation is a deadlock, not a no-op. #888's trace was not taken on trust; I re-ran it on the tree this builds on.

Caller side — zero locks held.
asio async_read_until handler → StratumSession::process_message (core/stratum_server.cpp:505) → handle_submit (:1036) → mining_interface_->mining_submit(...) (:1332 pool-share arm, :1347 pseudoshare-is-a-block arm). A scan of stratum_server.cpp:1030-1360 for mutex|lock_guard|unique_lock|shared_lock|lock( returns zero hits — handle_submit holds nothing, and it deliberately copies the job snapshot rather than holding a reference.

Inside mining_submit — zero locks across the seam, on both arms.
mint_share_mutex_ is taken in a scoped block that copies mint_share_fn_ and releases before the call; found_block_mutex_ on the block arm is likewise scoped and released before the mint; workers_mutex_ is only touched afterwards in the bump lambda. Both classify arms sit in the same function at the same lock depth (zero) under the same caller.

Callee side. main_dash.cpp's mint lambda takes its read guard inside a scoped block, releases it, and only then calls add_local_share, which takes the exclusive lock. Sequential, never nested — so making both bounded cannot self-deadlock.

Belt and braces. tracker_acquire takes an on_compute_thread flag and never waits when set; add_local_share passes is_compute_thread(). The compute thread already owns the exclusive lock, so the one path that would be a self-deadlock degrades to a single try. Pinned by AcquirePrimitiveContract.

Conclusion: the caller holds zero locks, so the bounded blocking acquire is viable — the deferred-retry alternative was not required.

Test evidence

The regression is "hold the tracker exclusively, drive a block-target submit, assert the share still lands" — and nothing existing covered it: every prior mint KAT runs against a free tracker, where try_to_lock and a bounded wait are indistinguishable.

Folded into existing allowlisted targets; no new add_executable. tools/ci/check_test_target_allowlist.py → CI drift-guard OK: 120 per-coin test target(s) across 5 coin lane(s) all present. .github/workflows/ untouched.

test_dash_node gains dash + sharechain on its link line so the KATs drive the real NodeImpl::add_local_share against the real m_tracker_mutex, held exclusively by another thread. That closes #888's own stated honest limit ("the KATs bind their own callback") for the part #889 is about. dash's deps were already on that line or come in transitively; nothing outside src/impl/dash is added.

target	result
test_dash_node (+5)	13/13 PASSED
test_dash_stratum_work_source (+4)	50/50 PASSED

Build: Release, gcc 13.3. c2pool-dash links clean.

[ RUN      ] DashBlockWinningMint.LandsWhileTrackerHeldExclusively
[       OK ] DashBlockWinningMint.LandsWhileTrackerHeldExclusively (401 ms)
[ RUN      ] DashBlockWinningMint.OrdinaryShareStillDeclinesWhileTrackerHeld
[warning] [MINT] tracker busy — local share 890000c200000000 declined (retry on next solve)
[       OK ] DashBlockWinningMint.OrdinaryShareStillDeclinesWhileTrackerHeld (400 ms)
[ RUN      ] DashBlockWinningMint.BudgetExpiryIsACountedForfeitNotASilentDrop
[error]   [MINT-BLOCK] FORFEIT — tracker still busy after 2000ms; BLOCK-WINNING share
          890000e400000000 NOT minted. The block itself was already submitted, but no
          p2pool peer will see it and its PPLNS weight is lost. forfeits=1
[       OK ] DashBlockWinningMint.BudgetExpiryIsACountedForfeitNotASilentDrop (2301 ms)
[       OK ] DashBlockWinningMint.DefaultArgumentIsTheOpportunisticPath (300 ms)
[       OK ] DashBlockWinningMint.AcquirePrimitiveContract (650 ms)

[       OK ] DashStratumWorkSource.MiningSubmitWonBlockFlagsTheMintAsBlockWinning (0 ms)
[       OK ] DashStratumWorkSource.MiningSubmitOrdinaryShareIsNotFlaggedBlockWinning (0 ms)
[       OK ] DashStratumWorkSource.WonBlockShareMintsThroughAnExclusivelyHeldTracker (251 ms)
[       OK ] DashStratumWorkSource.OrdinaryShareStillDeclinesThroughAHeldTracker (1 ms)

LandsWhileTrackerHeldExclusively also asserts the call waited ≥ 350 ms — i.e. it genuinely rode out the holder rather than finding a free mutex, which is what makes it a real exercise of the bounded acquire and not an accidental pass on an idle lock.

#895 / #898 checks

Every new case is a plain TEST, not #ifdef-guarded, so gtest_add_tests(AUTO) cannot register something unrunnable. Verified as actually EXECUTED under ctest, with real durations — not "Not Run", not a phantom pass:

803: DashBlockWinningMint.LandsWhileTrackerHeldExclusively ........... Passed  0.41 sec
804: DashBlockWinningMint.OrdinaryShareStillDeclinesWhileTrackerHeld . Passed  0.41 sec
805: DashBlockWinningMint.DefaultArgumentIsTheOpportunisticPath ...... Passed  0.31 sec
806: DashBlockWinningMint.BudgetExpiryIsACountedForfeitNotASilentDrop  Passed  2.31 sec
807: DashBlockWinningMint.AcquirePrimitiveContract ................... Passed  0.66 sec
100% tests passed, 0 tests failed out of 5

1150: ...MiningSubmitWonBlockFlagsTheMintAsBlockWinning ............... Passed  0.01 sec
1151: ...MiningSubmitOrdinaryShareIsNotFlaggedBlockWinning ............ Passed  0.01 sec
1152: ...WonBlockShareMintsThroughAnExclusivelyHeldTracker ............ Passed  0.26 sec
1153: ...OrdinaryShareStillDeclinesThroughAHeldTracker ................ Passed  0.01 sec
100% tests passed, 0 tests failed out of 4

test_dash_node and test_dash_stratum_work_source use gtest_add_tests(... AUTO) (source parsing), not gtest_discover_tests(... PRE_TEST), so the #898 5 s discovery timeout does not apply to them.

Negative control

Fix removed in two places — add_local_share forced back to Urgency::Opportunistic, and in.won_block forced false — then rebuilt and re-run:

[  FAILED  ] DashBlockWinningMint.LandsWhileTrackerHeldExclusively           (share forfeited)
[  FAILED  ] DashBlockWinningMint.BudgetExpiryIsACountedForfeitNotASilentDrop (declined in ~0ms, not after the budget)
[  FAILED  ] DashStratumWorkSource.MiningSubmitWonBlockFlagsTheMintAsBlockWinning
[  FAILED  ] DashStratumWorkSource.WonBlockShareMintsThroughAnExclusivelyHeldTracker

and the ordinary-path controls stayed green by design — OrdinaryShareStillDeclinesWhileTrackerHeld, DefaultArgumentIsTheOpportunisticPath, MiningSubmitOrdinaryShareIsNotFlaggedBlockWinning, OrdinaryShareStillDeclinesThroughAHeldTracker. That pairing is deliberate: if the block-winning tests ever passed merely because the mutex happened to be free, these would fail alongside them. Restored, rebuilt, re-ran → 13/13 and 50/50 green.

No consensus change

Share construction, target derivation and serialisation are entirely inside the untouched mint / create_local_share seams. This changes only how hard we try to acquire a lock. git diff touches no share struct, no target math, no serializer.

Scope

dash only, deliberately — so #889 stays OPEN after this merges.

• btc / bch reach the same seam through CreateShareFn, whose signature carries no solve class. Widening it touches work_source.hpp/.cpp for both coins plus main_btc.cpp and pool_entrypoint.hpp, and breaks three existing test binders. Neither is the hotel-deploy path this must ship with, and both would raise conflict probability against sibling PRs. Not silently assumed fixed — the same defect is live there and should be a follow-up.
• dgb cannot mint any local share until dgb: set_mint_share_fn is never bound in main_dgb.cpp — DGB cannot mint local shares #884 binds set_mint_share_fn, so there is nothing to make blocking.

Untouched by design: src/core/socket.cpp, src/core/web_server.cpp, src/impl/*/protocol_legacy.cpp, src/impl/dash/coin/checkpoints/**, mn_checkpoint* (#899), and main_dash.cpp's arm-resolution block (#891 — its main_dash.cpp hunks are at 1148-1349 and 2403+; mine are at 1871-1928, no overlap).

Not verified / follow-ups

• No live-node soak. This is a unit-level change verified by KAT plus negative control. The lock-timing evidence is from soak logs, not a soak of this build.
• clean_tracker()'s exclusive hold is not instrumented in this build, so the 2000 ms budget is sized against think() alone plus judgement. If clean cycles routinely exceed 2 s the counter will say so — which is precisely why the counter exists. Adding a [CLEAN] done in Nms line is a cheap follow-up.
• The hotel node was not queried directly (no SSH credentials available from this environment); the distribution above comes from a local 42 h DASH sharechain soak log, not from the hotel itself. Hotel hardware may differ.
• Production-contention behaviour of the real bound lambda still cannot be fully exercised from a unit test: test_dash_node drives the real add_local_share through the real mutex, but main_dash.cpp's lambda body is in the executable TU. Its read guard now uses the same helper under the same urgency, and the work-source KAT exercises that discipline end to end.
• btc / bch remain exposed to the identical defect (see Scope).

[frstrtr]

Integrator review — reward-path PR, so this got a caller-side lock trace rather than a CI glance.

Verified independently (not taken from the PR text):

1. Lock depth zero at the BlockWinning call site. Grepped every .cpp in src/impl/dash/stratum/ for m_tracker_mutex / unique_lock / shared_lock — zero acquisitions anywhere in the stratum layer. The chain asio handler -> process_message -> handle_submit -> mining_submit -> mint seam therefore enters at depth 0, which is what makes a bounded wait viable instead of the ltc: broadcast_share() is unreachable — caller holds the exclusive tracker lock its shared try-lock needs #878/ltc: notify_local_share() also unreachable — same held-exclusive-lock defect as #878 #881 deadlock.

2. Guard scope closes before the write. The read_tracker(urgency) guard is in its own block; the brace closes and only then does add_local_share take the exclusive lock. Never nested — confirmed by reading the brace structure, not the comment.

3. dash/btc/bch/dgb: block-winning share is never minted into the sharechain (LTC is the only correct lane) #887 ordering is NOT reintroduced. Block arm reads submit_block_fn_ (in its own try/catch) -> dashboard fb_fn -> mint_solved_share(true) -> bump(true) -> return. Submit strictly precedes mint, so a mint failure cannot cost the block. mint_solved_share also contains its own exceptions.

4. Opportunistic default preserved. read_tracker(Urgency = Opportunistic) — the sole non-bare call site is the mint lambda. Ordinary shares keep exactly today one-shot try_to_lock.

5. The bound is real: budget 2000 ms, poll 200 us, plus the on_compute_thread guard so the compute thread never waits on a lock it already owns.

Two design choices I want to endorse explicitly, because both are the harder-but-correct call:

• Bounded poll instead of shared_timed_mutex. Swapping the mutex type would have rewritten ~30 reader-discipline call sites on a live reward path. The poll keeps every existing call site byte-identical and still yields an exact, testable deadline.
• Forfeit is logged and counted, not silent. Even the worst case is now a LOG_ERROR + counter rather than a share vanishing — which is precisely the observability gap that made dash/btc/bch: a busy tracker still forfeits the block-winning share — try_to_lock decline has no 'next solve' for a block #889 hard to see in the first place.

Scope is correct: dash only, no closing keyword, #889 stays open for btc/bch (CreateShareFn carries no solve class) and for dgb once #884 lands.

Holding merge on the CI rollup — 3 pass / 25 pending at review time.