channels_sv2: bound job-storage retention (future templates, past jobs, replaced group jobs) - #2290
Conversation
plebhash
changed the title
channels_sv2: improve bound job-storage retention (future templates, past jobs, replaced group jobs)channels_sv2: bound job-storage retention (future templates, past jobs, replaced group jobs)
6211eb6 to
456fb81
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
456fb81 to
97bf704
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Second pass of clanker review: Solid PR overall — I audited the map/deque bookkeeping on every mutation path and it holds up, evicted-ID handling is correct at all call sites, and 🔴 Retired extranonce prefix released while still referenced
🟠 Tip-transition eviction drops one job from the stale setAt every tip transition the displaced active job is retired through the capped past path right before past drains into stale — so with past at cap, the oldest past job lands in neither set and its late share is rejected 🟡 Doc wording
Nits
|
addressed |
847cc70 to
0ea211c
Compare
gimballock
left a comment
gimballock
left a comment
There was a problem hiding this comment.
This began as scale testing, not as a review of this PR. We have been running distributed load tests against a vendored pin of this stack, and several defects we hit independently turn out to be the ones this PR and its two review passes already address. For a security fix that convergence is worth reporting: two very different methods — an LLM-assisted audit reading the source, and a fleet of miners hammering a pool on EC2 — landed on the same eviction bugs and the same bounds.
Where we ended up matching you:
| What we hit under load | Finding here |
|---|---|
past_jobs overshoot bounded at exactly MAX_PAST_JOBS + 1 |
retire_active_to_past_uncapped and its test assert that bound |
Evicting by smallest job_id rather than insertion order |
insertion-ordered past_job_order replaces it |
| Share-acceptance loss when the cap shrinks | pass 2 🟡, Accepted → InvalidJobId |
job_id_to_target growing unbounded alongside past jobs |
pass 1 🔴 |
What we can add that the thread does not have yet: a measurement of the cap trade-off. At fixed N (~110k connections) moving the cap from 300 to 50 doubled the share-acceptance deficit, while the memory saved below 50 is small (~0.07 MiB/conn). We have no data at 16, and two points establish a direction rather than an optimum. Framing the cap as a retention window rather than a round number, our earlier analysis puts the reachable submit depth at ~1–2 past jobs, which leaves 16 with roughly 8× margin on the eviction path — so nothing we have contradicts it. The open question is whether eviction is the mechanism that matters. Details are inline on the constant.
One small confirmation while we are here: we re-checked the assumption behind dropping the past_job_order dedup, and it holds at this revision — every job_store insertion in group.rs, standard.rs, and extended.rs is minted by that channel's own job_factory, including the custom-job path, so IDs are monotonic per store.
Context so the numbers are interpretable: pool isolated on its own EC2 box, a fleet of client load-generator boxes, every figure scraped pool-side from local /metrics. Our ladder ran 7.5k → 300,012 connections, the last held to RSS (resident set size) convergence across a 3,596 s plateau. We vendor this tree at pin 7fa5aa1 with one local patch making MAX_PAST_JOBS configurable, so these are not stock-upstream figures — but the cap was the only variable between arms.
We are downstream users rather than maintainers, so tell us which of this is useful and which is noise.
Contributions. We measured the memory saving from bounding past jobs at 36.5%, quantified the yield cost of a smaller cap at +2.7 points of share-acceptance deficit at fixed N, and confirmed the per-store job-ID monotonicity this revision relies on. If a continuously-hashing A/B across 16 / 50 / 300 would help pick the constant, that is something we could look into.
| /// and credited — a bounded loss of creditable work, the price of bounding memory against a | ||
| /// malicious template-distribution peer streaming non-future templates while withholding | ||
| /// `SetNewPrevHash`. Size the cap with that trade-off in mind. | ||
| pub(crate) const MAX_PAST_JOBS: usize = 16; |
There was a problem hiding this comment.
We support the bound — any cap beats unbounded retention, and that question looks settled. Our data speaks to a narrower one: where to sit on the capped spectrum. Two measured points give a direction rather than an optimum.
Same hardware (r7i.8xlarge), ~110k connections, held to RSS convergence, cap the only variable:
| cap=300 | cap=50 | |
|---|---|---|
| per-conn RSS | 1.429 MiB | 0.907 MiB (−36.5%) |
| share-acceptance deficit | 3,453 (3.16%) | 6,514 (5.86%) |
stale-share |
3,365 | 6,308 |
invalid-job-id |
4 | 0 |
| peak active channels | 105,701 | 104,697 (noise) |
Lowering the cap trades memory against credited work: 300 → 50 saved 36.5% of per-conn RSS and cost +2.7 points of share-acceptance deficit. Both directions carry a cost, so an optimum exists somewhere — but two points locate a gradient, not a minimum. The cap moved density rather than capacity: 105,701 against 104,697 active channels is noise.
A way to size it that does not rely on round numbers. The cap is really a retention window — MAX_PAST_JOBS ÷ template rate is the late-share tolerance it buys, so its meaning scales with your template cadence rather than ours. Two bounds apply, and they differ sharply. The latency bound is generous: at our ~1 template/s, a cap of 16 tolerates roughly 16 s of in-flight share latency. The structural bound is far tighter than either cap under discussion — a miner stops hashing a job as soon as the next arrives, and the share hop is a zero-delay channel, so reachable submit depth is ~1–2 past jobs. Against that, 16 already carries about 8× margin, and our own analysis put the smallest defensible cap on this rig at 20; we picked 50 for headroom, not necessity. On the eviction path we would therefore expect 16 to be fine.
What we cannot yet answer is whether the eviction path is the one that matters. The yield delta we measured probably is not it: invalid-job-id stayed at 0–4 in both arms while the deficit tracked stale-share, which points at the stale-set coupling rather than at evicted jobs — and no margin argument covers that mechanism. Our runs also could not test eviction at all, because each simulated miner stopped hashing once its first share was accepted, so every channel submitted exactly one share within seconds of connecting and none was ever validated against an evicted job. That makes our invalid-job-id zero vacuous rather than reassuring.
Closing that gap would take continuous hashing at fixed N across a sweep bracketing the structural floor — something like 8 / 16 / 25 / 50, chosen to straddle the ~20 estimate rather than for roundness. That is something we could look into if it would be useful; nothing we have contradicts 16 in the meantime.
| /// Maximum number of past jobs a server channel retains under the current chain tip. | ||
| /// | ||
| /// Past jobs exist for late-share validation, so the cap must stay nonzero. A share against an | ||
| /// evicted job is rejected as `InvalidJobId` even though it would otherwise have been accepted |
There was a problem hiding this comment.
Our data supports the conclusion in the second pass's 🟡 note — this is lost creditable work, not a benign reclassification — but complicates the label. In our runs the loss did not surface as InvalidJobId. The invalid-job-id rejection counter, labelled by error code, stayed between 0 and 4 in both cap arms, while the share-acceptance deficit doubled and tracked stale-share.
So the observable symptom of a too-small cap was shares rejected as stale and channels never credited. That matters for testing: a regression test asserting on InvalidJobId counts passes whether or not this effect exists. Reproducing it takes continuous share submission across a prev-hash rotation under load.
| pub fn mark_past_jobs_as_stale(&mut self) { | ||
| // Transfer past jobs to stale jobs collection and reset past jobs to empty | ||
| self.stale_jobs = std::mem::take(&mut self.past_jobs); | ||
| self.past_job_order.clear(); |
There was a problem hiding this comment.
This couples the two bounds, and it is our leading candidate for the yield loss we measured on MAX_PAST_JOBS: because stale_jobs = std::mem::take(&mut self.past_jobs), the cap transitively bounds the stale set as well as the past set.
A smaller cap therefore shrinks the stale window too, so shares in flight across a tip transition can land outside both sets. We could not isolate this from our data — the rejections came back as stale-share rather than invalid-job-id — so treat it as a hypothesis rather than a finding.
| /// misclassify its late shares as `InvalidJobId` instead of `Stale`), and no prune runs | ||
| /// while an in-flight future job is outside every collection (which would release its | ||
| /// retired extranonce prefix while the job goes on to accept shares under it). | ||
| fn retire_active_to_past_uncapped(&mut self) { |
There was a problem hiding this comment.
Independent corroboration: we hit this same overshoot in our own cap patch before this PR existed. The bypass paths let past_jobs transiently reach MAX_PAST_JOBS + 1, and we recorded the bound as exactly MAX + 1.
This method and its test land on the same number deliberately. Nothing to change — just a note that the invariant holds under sustained load, not only in the unit test.
0ea211c to
cecbc63
Compare
|
thanks @gimballock but I'm having difficulty on how to digest the comments above on a superficial level I interpreted these as more informational comments that corroborate the PR, rather than a actual requests for change, but the following bits got me slightly confused:
can you please clarify what you were aiming to communicate with the comments above? (ideally human generated response please) |
|
I was offering to rerun the scale test with a different sized job buffers to sweep for a best value. This buffer holds the outstanding jobs (MAX_PAST_JOBS): The size should be bigger than the rate of jobs submitted divided by the rate jobs are completed to not lose inflight work, but we want it as small as possible to minimize memory footprint. The current value you have (16) was picked arbitrarily, we tested some other arbitrary values, 300 and 50, and 50 was better. More data points are needed to say what the best setting might be. (not a blocking issue) |
|
thanks @gimballock I have a clear understanding now also thank you for this analysis, it's very valuable! I'm switching a valuable follow-up could consist of:
although I must say that would be placed as relatively low priority on SRI review pipeline, since we have a lot of work stacking up from audits in other areas of the codebase |
JobStore::add_future_job retained every future template's full job keyed by the peer-controlled template_id. A malicious or compromised Template Distribution peer could stream future templates while withholding SetNewPrevHash, growing future_jobs and future_template_to_job_id without limit, with each entry carrying large wire-controlled buffers. Track template IDs in receipt order and evict the oldest future job beyond MAX_FUTURE_JOBS (16), the same pattern used for client channels. One change covers all three server channel types (group, standard, extended), which all route future templates through add_future_job. Eviction deliberately does not prune retired extranonce prefixes: a prefix referenced only by an evicted job is held until the next tip transition, which is the safe direction (slot reserved longer, never released early).
Group channels never validate shares (the module docs explicitly say they don't track past or stale jobs), yet they retained job history at two sites: for every non-future template, GroupChannel::on_new_template routed through JobStore::add_active_job, which moves the previous active job into the unbounded past_jobs map, and future-job activation retired the displaced active job the same way, leaving one stale entry per tip transition. The former was pure unbounded retention: a malicious Template Distribution peer could establish a chain tip and then stream immediately-active templates while withholding SetNewPrevHash, retaining one full job per message. Add JobStore::replace_active_job and JobStore::activate_future_job_replacing_active, which drop the displaced active job instead of retaining it, and use them in the group channel so it keeps no past or stale job history at all.
Standard and extended server channels grow past_jobs through JobStore::add_active_job (non-future templates and SetCustomMiningJob), cleared only on activation. A malicious Template Distribution peer can stream non-future templates while withholding SetNewPrevHash, retaining one full job per message without limit. Unlike group channels, these channels validate shares against past jobs, so the history is bounded instead of dropped: track job IDs in retirement order and evict the oldest past job beyond MAX_PAST_JOBS (50). A share against an evicted job degrades to InvalidJobId instead of Stale, acceptable within a single tip window. stale_jobs is transitively bounded, since it is only ever a snapshot of past_jobs at tip transitions.
Client channels retain past jobs for late-share validation, but the job stream is upstream-controlled, so the collection must be bounded to prevent memory exhaustion. Introduce a dedicated cap, separate from MAX_FUTURE_JOBS, to be enforced by the standard and extended client channels.
A malicious upstream can stream immediately-active jobs (min_ntime present) on one channel and force one retained past job per message, no proof of work required, since past_jobs was only cleared on a successful SetNewPrevHash the attacker can withhold. Track past job IDs in retirement order and evict the oldest beyond MAX_PAST_JOBS, mirroring the future-job bound. A share against an evicted job degrades to InvalidJobId instead of Stale, acceptable within a single tip window. stale_jobs is transitively bounded, since it is only ever a snapshot of past_jobs at tip transitions.
Same defect as the standard client channel, at three sites: a malicious upstream can grow past_jobs without limit through immediately-active NewExtendedMiningJob messages, SetCustomMiningJobSuccess, or the job displaced on chain tip transitions, since the map was only cleared on a successful SetNewPrevHash (or chain tip update) the attacker can withhold. Route all four insertion sites through a shared helper that tracks past job IDs in retirement order and evicts the oldest beyond MAX_PAST_JOBS. A share against an evicted job degrades to InvalidJobId instead of Stale, acceptable within a single tip window.
Bounding past-job storage left the server channels' job_id_to_target maps unbounded: entries were inserted per non-future job but only cleared on a chain-tip transition, which a malicious template distribution peer can withhold, so eviction stranded one target entry per flooded template. JobStore::retire_active_to_past now reports the evicted job's ID through add_active_job and deactivate_job, and the standard and extended channels drop the matching target mapping. Shares against an evicted job are rejected as InvalidJobId before the target lookup, so removing the entry does not change validation behavior. The past-job flood tests now also assert that target metadata is bounded to the active job plus MAX_PAST_JOBS.
Retired extranonce prefixes were only pruned when past jobs went stale on a chain transition. The bounded-retention eviction paths (replaced future job under a reused template ID, evicted-oldest future job, evicted-oldest past job) dropped jobs without pruning, so a peer withholding SetNewPrevHash could keep a rotated-out prefix's allocator slot reserved indefinitely after its last referencing job was evicted. JobStore now prunes retired prefixes in add_future_job and after a past-job eviction in retire_active_to_past. Regression tests cover both paths: a store-level test for the reused template ID drop, and a channel-level test asserting the allocator slot is released once eviction removes the last job created under a rotated-out prefix.
Retiring the displaced active job through the capped past path at a tip transition caused two defects: 1. activate_future_job pops the future job from storage before the retirement, so an eviction-triggered prune of retired extranonce prefixes ran while the in-flight job was in no collection. A retired prefix whose only remaining reference was that job had its allocator slot freed while the job went on to accept shares under those prefix bytes, allowing the same extranonce space to be handed to a second live channel. 2. With past jobs at the MAX_PAST_JOBS cap, the eviction pushed the oldest past job out of the stale set right before past drained into stale, misclassifying its late shares as InvalidJobId instead of Stale. The eviction bought nothing: past jobs are emptied on the next statement. Tip transitions now retire the displaced job uncapped (stale_jobs stays bounded at MAX_PAST_JOBS + 1) and without pruning; the prune runs in mark_past_jobs_as_stale, after the activated job is back in active_job. Affected paths: activate_future_job, deactivate_job (which loses its never-consumed evicted-ID return), and the extended channel's SetCustomMiningJob chain-tip change, which now retires the displaced job before marking past jobs stale rather than after. Also rewords the eviction docs: mid-tip, a share against an evicted past job is rejected as InvalidJobId even though it would otherwise have been accepted and credited - a bounded loss of creditable work, not a Stale-to-InvalidJobId downgrade.
…PrevHash Retiring the displaced active job through the capped past path right before past jobs drained into stale meant that, with past jobs at the MAX_PAST_JOBS cap, the oldest past job was evicted from the stale set: its late share was rejected as InvalidJobId instead of Stale, the exact misclassification the existing regression tests guard against. The eviction bought nothing, as past jobs are cleared on the next statement. The displaced job is now inserted directly into the stale set after past jobs are rotated into it, bypassing the cap; stale_jobs stays bounded at MAX_PAST_JOBS + 1. Also rewords the past-job eviction docs: mid-tip, a share against an evicted past job is rejected as InvalidJobId even though it would otherwise have been accepted and propagated - a bounded loss of creditable work, not a Stale-to-InvalidJobId downgrade.
…tip transitions Same defect as the client standard channel, at two sites: on_set_new_prev_hash and on_chain_tip_update. Retiring the displaced active job through the capped past path right before past jobs drained into stale meant that, with past jobs at the MAX_PAST_JOBS cap, the oldest past job was evicted from the stale set: its late share was rejected as InvalidJobId instead of Stale. The eviction bought nothing, as past jobs are cleared on the next statement. The displaced job is now inserted directly into the stale set after past jobs are rotated into it, bypassing the cap; stale_jobs stays bounded at MAX_PAST_JOBS + 1. Also rewords the past-job eviction doc: mid-tip, a share against an evicted past job is rejected as InvalidJobId even though it would otherwise have been accepted and propagated - a bounded loss of creditable work, not a Stale-to-InvalidJobId downgrade.
Behavior-preserving cleanups from review: - add_future_job prunes retired extranonce prefixes only when a job was actually dropped (replaced template ID or evicted-oldest); on the no-drop path pruning can never free anything. - retire_active_to_past no longer purges the retiring ID from the eviction order before pushing it: server job IDs are minted by the job factory, strictly monotonic per channel, so the ID can never already be present. The client channels keep their purge - client job IDs are upstream-controlled and can repeat. - activate_future_job_replacing_active delegates to activate_future_job first and clears the stale set on success, instead of duplicating the delegate's future-job lookup as a check-then-act. activate_future_job mutates no job collection on its not-found path, so a failed activation still leaves channel state untouched.
cecbc63 to
2fb0240
Compare
|
I can create the followup PR (unless you would prefer to), feel free to prioritize as needed |
sure go ahead ideally there should be a new that means a companion PR against moreover, I wouldn't necessarily completely throw away then we always do for now, merging this as is |
3 participants
close #2289
close https://github.com/project-loupe/audit-stratum/issues/101
close https://github.com/project-loupe/audit-stratum/issues/100
close https://github.com/project-loupe/audit-stratum/issues/97
close https://github.com/project-loupe/audit-stratum/issues/127
close https://github.com/project-loupe/audit-stratum/issues/134
close https://github.com/project-loupe/audit-stratum/issues/100
close https://github.com/project-loupe/audit-stratum/issues/117
close https://github.com/project-loupe/audit-stratum/issues/101
close https://github.com/project-loupe/audit-stratum/issues/126
close https://github.com/project-loupe/audit-stratum/issues/136
close https://github.com/project-loupe/audit-stratum/issues/129