channels_sv2: make the past-jobs cap configurable w/ a 'good' default value - #2307
channels_sv2: make the past-jobs cap configurable w/ a 'good' default value#2307gimballock wants to merge 3 commits into
channels_sv2: make the past-jobs cap configurable w/ a 'good' default value#2307Conversation
`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>
|
This work is progressing, I have a solid theory of what to expect from the test. I have a physical mining test running now to validate against expectation. In short I think our setting of 50 is way too high and the better value is closer to 2 (as a conservative default value). A lot of the analysis yesterday went into deciding if the stale share buffer and the past jobs buffer should be split into two and sized independently but eventually I realized that at realistic rates on mainnet the two buffers should be small and in approximately equal proportion. I'll post more today once the hw results are in. and i'm trying to get just enough of the analysis together to be informative w/o overwhelming. |
channels_sv2: make the past-jobs cap configurablechannels_sv2: make the past-jobs cap configurable w/ a 'good' default value
Hardware A/B for the default value — now runningFollowing up on the plan above: the run that decides the default just started on real The questionHow many past jobs must a pool retain per channel? The The designTwo pools on one host, sharing one Bitcoin node, identical in every setting except the
Each arm: one real SV1 miner → its own translator → its own pool. Same chain tip, same Network: testnet4, templates driven by the node over IPC with the fee threshold at 0 and a The mechanism under testCap 2 exactly covers a 3-job cache, with zero margin. That's the substance of the This is why real miners matter: a simulated miner that only ever submits against the What we measurePool-side
Reference point: the same units' long-run stale-share rate against their normal production What each outcome means
What it cannot tell usOne firmware build, one template cadence, testnet4, two miners. It does not establish Two known asymmetries, neither affecting job-cache behaviour: the miners sit in different 🤖 Generated with Claude Code |
|
There are many details left out of that relating to why i think 2 is a good value but I can explain all of that if the test affirms that hypothesis, otherwise you probably don't care about my false logic. But part of the estimate is based on our belief of how many jobs a miner can be working on in parallel. |
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