channels_sv2: address review on the configurable past-jobs cap

- 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>

Author: Eric Price

sv2/channels-sv2/src/client/extended.rs

@@ -4,7 +4,7 @@
 //! **Extended Channel** within a mining client.
 
 extern crate alloc;
-use super::{HashMap, MAX_FUTURE_JOBS, MAX_PAST_JOBS};
+use super::{resolve_max_past_jobs, HashMap, MAX_FUTURE_JOBS};
 use crate::{
     bip141::try_strip_bip141,
     chain_tip::ChainTip,
@@ -62,7 +62,7 @@ pub type ExtendedJob = (NewExtendedMiningJobOwned, Vec<u8>, Target);
 ///   [`SetNewPrevHash`](SetNewPrevHashMp) message.
 /// - The currently active job.
 /// - Past jobs (previously active under the current chain tip, indexed by `job_id`, capped at
-///   [`MAX_PAST_JOBS`]).
+///   [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS)).
 /// - Stale jobs (previously active and past jobs under the previous chain tip, indexed by
 ///   `job_id`).
 /// - Share accounting for the channel (as tracked by the client).
@@ -100,7 +100,7 @@ impl ExtendedChannel {
     /// Constructs a new [`ExtendedChannel`].
     ///
     /// `max_past_jobs` caps the past jobs retained under the current chain tip; `None` uses
-    /// [`MAX_PAST_JOBS`].
+    /// [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS).
     #[allow(clippy::too_many_arguments)]
     pub fn new(
         channel_id: u32,
@@ -126,9 +126,7 @@ impl ExtendedChannel {
             past_jobs: HashMap::new(),
             past_job_order: VecDeque::new(),
             stale_jobs: HashMap::new(),
-            max_past_jobs: max_past_jobs
-                .map(NonZeroUsize::get)
-                .unwrap_or(MAX_PAST_JOBS),
+            max_past_jobs: resolve_max_past_jobs(max_past_jobs),
             share_accounting: ShareAccounting::new(),
             chain_tip: None,
         }
@@ -260,7 +258,7 @@ impl ExtendedChannel {
 
     /// Returns an iterator over all past jobs for this channel.
     ///
-    /// At most [`MAX_PAST_JOBS`] jobs are kept (oldest evicted first).
+    /// At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) jobs are kept (oldest evicted first).
     pub fn get_past_jobs(&self) -> impl Iterator<Item = (&u32, &ExtendedJob)> + '_ {
         self.past_jobs.iter()
     }
@@ -272,7 +270,7 @@ impl ExtendedChannel {
 
     /// Returns the number of past jobs tracked by this channel.
     ///
-    /// At most [`MAX_PAST_JOBS`] jobs are kept (oldest evicted first).
+    /// At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) jobs are kept (oldest evicted first).
     pub fn get_past_jobs_count(&self) -> usize {
         self.past_jobs.len()
     }
@@ -323,7 +321,7 @@ impl ExtendedChannel {
     ///   At most [`MAX_FUTURE_JOBS`] future jobs are kept: storing a new one beyond that limit
     ///   evicts the oldest.
     /// - Otherwise, the job is activated and previous active job moves to the past jobs list.
-    ///   At most [`MAX_PAST_JOBS`] past jobs are kept: retiring one beyond that limit evicts the
+    ///   At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) past jobs are kept: retiring one beyond that limit evicts the
     ///   oldest.
     pub fn on_new_extended_mining_job(
         &mut self,
@@ -390,7 +388,7 @@ impl ExtendedChannel {
     /// Handles a `SetCustomMiningJobSuccess` message from upstream.
     /// Requires the corresponding `SetCustomMiningJob`.
     ///
-    /// The previous active job (if any) moves to the past jobs list. At most [`MAX_PAST_JOBS`]
+    /// The previous active job (if any) moves to the past jobs list. At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS)
     /// past jobs are kept: retiring one beyond that limit evicts the oldest.
     ///
     /// To be used by a Sv2 Job Declarator Client
@@ -1112,7 +1110,7 @@ mod tests {
     }
 
     #[test]
-    fn test_past_jobs_honour_constructor_override() {
+    fn test_past_jobs_respect_constructor_override() {
         // Some(cap) must override MAX_PAST_JOBS all the way through to the eviction path.
         let custom_cap = NonZeroUsize::new(3).unwrap();
         assert!(custom_cap.get() < MAX_PAST_JOBS);

sv2/channels-sv2/src/client/mod.rs

@@ -39,6 +39,16 @@ pub const MAX_FUTURE_JOBS: usize = 16;
 /// `max_past_jobs: Option<NonZeroUsize>` and fall back to this value when passed `None`.
 pub const MAX_PAST_JOBS: usize = 50;
 
+/// Resolves a caller-supplied past-jobs cap against [`MAX_PAST_JOBS`].
+///
+/// Keeps the default referenced in one place, so changing it does not mean touching every
+/// channel constructor.
+pub(crate) fn resolve_max_past_jobs(max_past_jobs: Option<core::num::NonZeroUsize>) -> usize {
+    max_past_jobs
+        .map(core::num::NonZeroUsize::get)
+        .unwrap_or(MAX_PAST_JOBS)
+}
+
 // Type aliases that switch between `std::collections` and `hashbrown`
 // depending on whether the `no_std` feature is enabled.
 #[cfg(not(feature = "no_std"))]

sv2/channels-sv2/src/client/standard.rs

@@ -5,7 +5,7 @@
 //! and chain tip state, enabling share validation and mining job lifecycle management.
 
 extern crate alloc;
-use super::{HashMap, MAX_FUTURE_JOBS, MAX_PAST_JOBS};
+use super::{resolve_max_past_jobs, HashMap, MAX_FUTURE_JOBS};
 use crate::{
     chain_tip::ChainTip,
     client::{
@@ -48,7 +48,7 @@ pub type StandardJob = (NewMiningJobOwned, Target);
 /// - future mining jobs (indexed by job_id, activated upon [`NewMiningJob`](mining_sv2::NewMiningJob) receipt, capped at [`MAX_FUTURE_JOBS`])
 /// - active mining job
 /// - past jobs (active jobs under current chain tip, indexed by job_id, capped at
-///   [`MAX_PAST_JOBS`])
+///   [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS))
 /// - stale jobs (jobs from previous chain tip, indexed by job_id)
 /// - share accounting state
 /// - chain tip state
@@ -80,7 +80,7 @@ impl StandardChannel {
     /// Creates a new [`StandardChannel`] instance with provided channel parameters.
     ///
     /// `max_past_jobs` caps the past jobs retained under the current chain tip; `None` uses
-    /// [`MAX_PAST_JOBS`].
+    /// [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS).
     pub fn new(
         channel_id: u32,
         user_identity: String,
@@ -101,9 +101,7 @@ impl StandardChannel {
             past_jobs: HashMap::new(),
             past_job_order: VecDeque::new(),
             stale_jobs: HashMap::new(),
-            max_past_jobs: max_past_jobs
-                .map(NonZeroUsize::get)
-                .unwrap_or(MAX_PAST_JOBS),
+            max_past_jobs: resolve_max_past_jobs(max_past_jobs),
             share_accounting: ShareAccounting::new(),
             chain_tip: None,
         }
@@ -213,7 +211,7 @@ impl StandardChannel {
 
     /// Returns an iterator over all past jobs for the channel (active jobs under current chain tip).
     ///
-    /// At most [`MAX_PAST_JOBS`] jobs are kept (oldest evicted first).
+    /// At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) jobs are kept (oldest evicted first).
     pub fn get_past_jobs(&self) -> impl Iterator<Item = (&u32, &StandardJob)> + '_ {
         self.past_jobs.iter()
     }
@@ -225,7 +223,7 @@ impl StandardChannel {
 
     /// Returns the number of past jobs tracked by this channel.
     ///
-    /// At most [`MAX_PAST_JOBS`] jobs are kept (oldest evicted first).
+    /// At most [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) jobs are kept (oldest evicted first).
     pub fn get_past_jobs_count(&self) -> usize {
         self.past_jobs.len()
     }
@@ -307,7 +305,7 @@ impl StandardChannel {
     /// - If `min_ntime` is empty, the job is added to future jobs. At most [`MAX_FUTURE_JOBS`]
     ///   future jobs are kept: storing a new one beyond that limit evicts the oldest.
     /// - If an active job exists, it is moved to past jobs on activation. At most
-    ///   [`MAX_PAST_JOBS`] past jobs are kept: retiring one beyond that limit evicts the oldest.
+    ///   [`MAX_PAST_JOBS`](super::MAX_PAST_JOBS) past jobs are kept: retiring one beyond that limit evicts the oldest.
     pub fn on_new_mining_job(&mut self, new_mining_job: NewMiningJobOwned) {
         self.store_new_mining_job(new_mining_job);
     }
@@ -568,6 +566,7 @@ mod tests {
     };
     use binary_sv2::Sv2OptionOwned as Sv2Option;
     use bitcoin::Target;
+    use core::num::NonZeroUsize;
     use mining_sv2::{
         NewExtendedMiningJobOwned as NewExtendedMiningJob, NewMiningJobOwned as NewMiningJob,
         SetNewPrevHashOwned as SetNewPrevHashMp, SubmitSharesStandardOwned,
@@ -749,6 +748,60 @@ mod tests {
         channel.on_set_new_prev_hash(set_new_prev_hash).unwrap();
     }
 
+    #[test]
+    fn test_past_jobs_respect_constructor_override() {
+        // Some(cap) must override MAX_PAST_JOBS on the client standard channel's own eviction
+        // path, which keeps its past jobs directly rather than delegating to a JobStore.
+        let custom_cap = NonZeroUsize::new(3).unwrap();
+        assert!(custom_cap.get() < MAX_PAST_JOBS);
+
+        let channel_id = 1;
+        let extranonce_prefix = [
+            83, 116, 114, 97, 116, 117, 109, 32, 86, 50, 32, 83, 82, 73, 32, 80, 111, 111, 108, 0,
+            0, 0, 0, 0, 0, 0, 1,
+        ]
+        .to_vec();
+
+        let mut channel = StandardChannel::new(
+            channel_id,
+            "user_identity".to_string(),
+            ExtranoncePrefix::from_wire(extranonce_prefix).unwrap(),
+            Target::from_le_bytes([0xff; 32]),
+            1.0,
+            Some(custom_cap),
+        );
+
+        let active_job = NewMiningJob {
+            channel_id,
+            job_id: 0,
+            merkle_root: [
+                189, 200, 25, 246, 119, 73, 34, 42, 209, 112, 237, 50, 169, 71, 163, 192, 24, 84,
+                56, 86, 147, 71, 243, 44, 18, 107, 167, 169, 169, 66, 186, 98,
+            ]
+            .into(),
+            version: 536870912,
+            min_ntime: Sv2Option::new(Some(1746839905)),
+        };
+
+        let job_count = 20u32;
+        for job_id in 0..job_count {
+            let mut job = active_job.clone();
+            job.job_id = job_id;
+            channel.on_new_mining_job(job);
+        }
+
+        // bounded by the override, not by MAX_PAST_JOBS
+        assert_eq!(channel.get_past_jobs_count(), custom_cap.get());
+
+        // the last job is active; only the newest `custom_cap` retired jobs survive
+        for job_id in 0..job_count - 1 - custom_cap.get() as u32 {
+            assert!(channel.get_past_job(job_id).is_none());
+        }
+        for job_id in job_count - 1 - custom_cap.get() as u32..job_count - 1 {
+            assert!(channel.get_past_job(job_id).is_some());
+        }
+    }
+
     #[test]
     fn test_past_jobs_are_bounded() {
         let channel_id = 1;

sv2/channels-sv2/src/server/extended.rs

@@ -49,7 +49,7 @@ use crate::{
         jobs::{
             extended::ExtendedJob,
             factory::JobFactory,
-            job_store::{JobStore, MAX_PAST_JOBS},
+            job_store::{resolve_max_past_jobs, JobStore},
             JobOrigin,
         },
         share_accounting::{ShareAccounting, ShareValidationError, ShareValidationResult},
@@ -263,11 +263,7 @@ impl ExtendedChannel {
             job_id_to_target: HashMap::new(),
             nominal_hashrate,
             stable_hashrate: false,
-            job_store: JobStore::new(
-                max_past_jobs
-                    .map(NonZeroUsize::get)
-                    .unwrap_or(MAX_PAST_JOBS),
-            ),
+            job_store: JobStore::new(resolve_max_past_jobs(max_past_jobs)),
             job_factory,
             share_accounting: ShareAccounting::new(share_batch_size),
             expected_share_per_minute,

sv2/channels-sv2/src/server/jobs/job_store.rs

@@ -10,7 +10,10 @@
 //! - **Retired Extranonce Prefixes**: Holds on to extranonce prefixes that were rotated out of the
 //!   channel while jobs created under them can still accept shares, so that their allocator slots
 //!   are not handed to another channel too early.
-use std::collections::{HashMap, VecDeque};
+use std::{
+    collections::{HashMap, VecDeque},
+    num::NonZeroUsize,
+};
 
 use super::Job;
 use crate::extranonce_manager::ExtranoncePrefix;
@@ -41,6 +44,16 @@ pub(crate) const MAX_FUTURE_JOBS: usize = 16;
 /// override and fall back to this value when none is given.
 pub(crate) const MAX_PAST_JOBS: usize = 50;
 
+/// Resolves a caller-supplied past-jobs cap against `MAX_PAST_JOBS`.
+///
+/// Keeps the default referenced in one place, so changing it does not mean touching every
+/// channel constructor.
+pub(crate) fn resolve_max_past_jobs(max_past_jobs: Option<NonZeroUsize>) -> usize {
+    max_past_jobs
+        .map(NonZeroUsize::get)
+        .unwrap_or(MAX_PAST_JOBS)
+}
+
 /// Internal implementation for tracking mining job states in SV2 server channels.
 ///
 /// Maintains collections for future, active, past, and stale jobs, and tracks template-to-job ID
@@ -74,6 +87,10 @@ impl<T: Job> JobStore<T> {
     /// Creates a new empty job store retaining at most `max_past_jobs` past jobs under the
     /// current chain tip.
     pub fn new(max_past_jobs: usize) -> Self {
+        // callers resolve an `Option<NonZeroUsize>` via `resolve_max_past_jobs`, so a zero cap
+        // cannot arrive here; a zero cap would evict the just-retired job immediately and reject
+        // the most common late share as `InvalidJobId`
+        debug_assert!(max_past_jobs > 0, "max_past_jobs must be nonzero");
         Self {
             future_template_to_job_id: HashMap::new(),
             future_template_order: VecDeque::new(),
@@ -411,7 +428,7 @@ mod tests {
     }
 
     #[test]
-    fn past_jobs_honour_a_custom_cap() {
+    fn past_jobs_respect_a_custom_cap() {
         // a store built with a cap below the default must evict against that cap, not the default
         let custom_cap = 2;
         assert!(custom_cap < MAX_PAST_JOBS);

sv2/channels-sv2/src/server/standard.rs

@@ -41,7 +41,7 @@ use crate::{
         jobs::{
             extended::ExtendedJob,
             factory::JobFactory,
-            job_store::{JobStore, MAX_PAST_JOBS},
+            job_store::{resolve_max_past_jobs, JobStore},
             standard::StandardJob,
         },
         share_accounting::{ShareAccounting, ShareValidationError, ShareValidationResult},
@@ -246,11 +246,7 @@ impl StandardChannel {
             stable_hashrate: false,
             share_accounting: ShareAccounting::new(share_batch_size),
             expected_share_per_minute,
-            job_store: JobStore::new(
-                max_past_jobs
-                    .map(NonZeroUsize::get)
-                    .unwrap_or(MAX_PAST_JOBS),
-            ),
+            job_store: JobStore::new(resolve_max_past_jobs(max_past_jobs)),
             job_factory,
             chain_tip: None,
         })