channels_sv2: make the past-jobs cap configurable - #2307
Conversation
`MAX_PAST_JOBS` is a retention window -- `cap / template rate` -- and the template rate is a deployment property this crate cannot observe. The same constant buys 50s of late-share tolerance at a 1s template cadence and over 16 minutes at 20s, so no single value fits every deployment. Add `max_past_jobs: Option<NonZeroUsize>` to the server and client `ExtendedChannel` and `StandardChannel` constructors, resolved as `max_past_jobs.map(NonZeroUsize::get).unwrap_or(MAX_PAST_JOBS)`. Passing `None` preserves current behavior, so the constant stays the documented default rather than being replaced. `NonZeroUsize` rather than `usize`: a zero cap evicts the job that just retired, so the most common late share is rejected as `InvalidJobId`, with no startup error to warn the operator. Validating at runtime would mean adding `Result` to the client constructors, which return `Self` today. Group channels take no parameter -- client `GroupChannel` keeps no past jobs, and server group channels replace the active job rather than retiring it, so the cap is inert there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DO NOT MERGE. Temporary commit so CI can build against the companion stratum branch before it lands on stratum-mining/stratum. Repoints the stratum-core git dependency from stratum-mining/stratum#main to marafoundation/stratum#feat/configurable-max-past-jobs (which carries the max_past_jobs constructor parameter these changes depend on) in stratum-apps and bitcoin-core-sv2, and regenerates the committed lockfiles so the --locked CI builds resolve the fork. Revert this commit once stratum-mining/stratum#2307 is merged and the stratum-core pin is bumped to the merged rev. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Test the client `StandardChannel` override. That channel keeps its own `past_jobs`/`past_job_order` and eviction path rather than delegating to `JobStore`, so it was the one place `max_past_jobs` could regress unnoticed: the override was covered for the client extended channel and for the server via `JobStore`, but not here. - Factor the resolver. `max_past_jobs.map(NonZeroUsize::get).unwrap_or(...)` was copy-pasted into four constructors; each side now routes through a `resolve_max_past_jobs` helper colocated with its own default, so changing a default touches one line rather than four. - `debug_assert!(max_past_jobs > 0)` in `JobStore::new`. The nonzero guarantee lived only in the callers; this documents and enforces it internally without changing the signature. - Rename the new tests off "honour" — the crate uses American spellings throughout. - Make the client `MAX_PAST_JOBS` doc links explicit (`super::MAX_PAST_JOBS`) so they no longer depend on an import that the resolver made otherwise unused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Now that the cap ( Two losses, opposite fixes. An uncredited share is either an eviction ( Why our data can't decideThe costs are the ones already in #2290. On the memory side, at fixed connection count, cap 300 → 50 cut per-connection memory 36.5% (1.429 → 0.907 MiB, ~4.5 kB per retained job per channel). Both arms carried the same connection count within noise, so the cap moves density, not capacity — it buys headroom on the box rather than raising the connection ceiling. One caveat on that figure: per-connection memory isn't comparable across differing hardware, connection counts or trial lengths, and we published a 45.5% version of it that was wrong for exactly that reason. 36.5% is the corrected same-conditions result. On the yield side, that same change cost 2.7 points on a channel-level metric we track — uncredited channels: those that opened successfully but never had a single share credited, 3.16% → 5.86%. Those channels stayed connected for the whole trial, so they were paying full per-connection memory while producing nothing creditable — a yield loss, not a connection loss. Both figures come from a gauge we've seen undercount under scrape pressure, so the 2.7-point delta is sturdier than either absolute. Theory says the eviction path should be nearly empty. The required depth — how many past jobs the cap has to retain — is The experiment that decidesThat uncredited-channel metric can't be reused: it only means anything because each channel submitted exactly once, and once miners resubmit after a rejection every channel eventually lands an accepted share, so the count collapses toward zero however much work is being lost. So this experiment needs to measure an actual stream of shares — rejected ÷ submitted, split by error code — under continuous hashing at fixed connection count. Those two counters are the discriminator, and previous runs simply never put load on them.
Six runs. Without rotations, That subtraction assumes eviction is equal across arms, which is close but not exact: each rotation drains past into stale, so the ring briefly can't evict while it refills, and the rotating arm suppresses eviction slightly. The bias runs against attributing loss to the past/stale coupling, so a signal that survives it is the stronger result. The ladder is deliberately low. Eviction can only fire when a share arrives more than Predictions, registered before the run:
The third row is a real outcome, named in advance so it isn't treated as a failed run: rejection rates flat across the whole ladder would mean the earlier 2.7 points came from the one-shot workload rather than from the cap, and that nothing on the yield side argues against a small default. Recommendation: keep the default at 50 until this runs. It's the safer side of a trade-off whose downside is unquantified below it. We wouldn't argue against a smaller cap on the eviction path; we can't vouch for it on the cross-tip path. When it does run, the knee is a floor rather than a default. It's measured against one latency distribution and one cadence, so a shipped default wants headroom above it — and since the knee moves with both, the parameter this PR adds probably matters more than whichever number we end up recommending. Contributions:
|
Replaces `Option<NonZeroUsize>` with `Option<usize>` and drops the `resolve_max_past_jobs` helpers, resolving inline in each constructor instead, as requested in review. `None` and `Some(0)` now both mean "no opinion" and select `MAX_PAST_JOBS`. That keeps a zero cap unreachable — it would evict the job that just retired and reject the most common late share as `InvalidJobId` — without the `NonZeroUsize` ceremony at every call site, and it reads as the usual "0 means unset" config convention. Tests: `Some(0)` is asserted equivalent to `None` on the server extended channel and on both client channels. The server test arrives via a small `retained_past_jobs` helper, which also closes a gap from the last round — the override was previously only covered on the server side at the `JobStore` level, never through a channel constructor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MAX_PAST_JOBSis really a retention window —cap ÷ template rate— and the template rate is a deployment property the library can't see. The same constant buys 50s of late-share tolerance at a 1s template cadence and over 16 minutes at 20s, so I don't think one number can be right for everyone.Adds
max_past_jobs: Option<usize>to the server and clientExtendedChannel/StandardChannelconstructors.NoneandSome(0)both selectMAX_PAST_JOBS, so nothing changes for existing callers.Treating
Some(0)as "no opinion" rather than as a literal zero keeps a zero cap unreachable: it would evict the job that just retired, so the most common late share would come backInvalidJobIdwith no startup error to warn you.Group channels take no parameter — client
GroupChannelhas nopast_jobs, and server group channels usereplace_active_job.A second commit will set the default once we've measured it; the analysis is coming in a comment.
Follow-up to #2290.
companion stratum-mining/sv2-apps#735