channels_sv2: enforce share nTime bounds, bound seen_shares, and fix job-ID/chain-tip/coinbase validation gaps (plus template_distribution_sv2 docs fix))

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

@@ -15,7 +15,7 @@ use crate::{
     extranonce_manager::ExtranoncePrefix,
     merkle_root::merkle_root_from_path,
     target::{bytes_to_hex, u256_to_block_hash},
-    MAX_EXTRANONCE_LEN, VERSION_ROLLING_MASK,
+    MAX_EXTRANONCE_LEN, MAX_FUTURE_BLOCK_TIME, VERSION_ROLLING_MASK,
 };
 use alloc::{collections::VecDeque, format, string::String, vec, vec::Vec};
 use binary_sv2::Sv2OptionOwned;
@@ -608,7 +608,12 @@ impl ExtendedChannel {
     ///
     /// Updates channel state with the share validation result:
     /// - Prevents propagation of stale, duplicate, low-difficulty, or low-ntime shares
-    ///   (shares whose `ntime` is below the chain tip's `min_ntime`).
+    ///   (shares whose `ntime` is below the chain tip's `min_ntime` or the job's own
+    ///   `min_ntime` — an immediately-active job may carry a `min_ntime` later than the chain
+    ///   tip's minimum, so the effective lower bound is the larger of the two — as well as
+    ///   shares whose `ntime` exceeds the chain tip's `min_ntime + MAX_FUTURE_BLOCK_TIME`, see
+    ///   [`MAX_FUTURE_BLOCK_TIME`] for how this clockless upper bound relates to the spec's
+    ///   elapsed-time window).
     /// - Indicates whether a block was found from the share.
     /// - Maintains local share accounting for later reconciliation with upstream acknowledgements.
     pub fn validate_share(
@@ -685,6 +690,25 @@ impl ExtendedChannel {
             ));
         }
 
+        // consensus caps block timestamps at ~2h in the future; the allowance is anchored at
+        // chain-tip receipt, since this crate has no clock (see MAX_FUTURE_BLOCK_TIME)
+        if share.ntime > chain_tip.min_ntime().saturating_add(MAX_FUTURE_BLOCK_TIME) {
+            return Err(ShareValidationError::Invalid(
+                ERROR_CODE_SUBMIT_SHARES_INVALID_SHARE,
+            ));
+        }
+
+        // an immediately-active job carries its own min_ntime, which may be later than the
+        // chain tip's minimum; jobs activated from the future queue have it overwritten with
+        // the SetNewPrevHash timestamp, making this check redundant there (and harmless)
+        if let Some(job_min_ntime) = job.0.min_ntime.clone().into_inner() {
+            if share.ntime < job_min_ntime {
+                return Err(ShareValidationError::Invalid(
+                    ERROR_CODE_SUBMIT_SHARES_INVALID_SHARE,
+                ));
+            }
+        }
+
         // Only BIP323 general-purpose bits may differ from the job's advertised version.
         // When version rolling is not allowed, the share version must match the job version exactly.
         let version_rolling_mask = if job.0.version_rolling_allowed {
@@ -1316,7 +1340,7 @@ mod tests {
                 0, 0, 0, 0, 0, 38, 106, 36, 170, 33, 169, 237, 226, 246, 28, 63, 113, 209, 222,
                 253, 63, 169, 153, 223, 163, 105, 83, 117, 92, 105, 6, 137, 121, 153, 98, 180, 139,
                 235, 216, 54, 151, 78, 140, 249, 1, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
             ]
             .try_into()
             .unwrap(),
@@ -2336,4 +2360,184 @@ mod tests {
         assert_eq!(channel.get_past_jobs_count(), 0);
         assert!(channel.get_active_job().is_none());
     }
+
+    #[test]
+    fn test_share_validation_ntime_below_job_min_ntime() {
+        // Regression test: an immediately-active job carries its own min_ntime, which may be
+        // later than the chain tip's minimum. A share in the gap
+        // (chain_tip.min_ntime <= ntime < job.min_ntime) must be rejected.
+        let channel_id = 1;
+        let extranonce_prefix = [
+            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1,
+        ]
+        .to_vec();
+
+        let mut channel = ExtendedChannel::new(
+            channel_id,
+            "user_identity".to_string(),
+            ExtranoncePrefix::from_wire(extranonce_prefix).unwrap(),
+            Target::from_le_bytes([0xff; 32]),
+            1.0,
+            true,
+            8u16,
+        );
+
+        let job = |job_id: u32, min_ntime: Option<u32>| NewExtendedMiningJob {
+            channel_id,
+            job_id,
+            min_ntime: Sv2Option::new(min_ntime),
+            version: 536870912,
+            version_rolling_allowed: true,
+            coinbase_tx_prefix: vec![
+                2, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 34, 82, 0,
+            ]
+            .try_into()
+            .unwrap(),
+            coinbase_tx_suffix: vec![
+                255, 255, 255, 255, 2, 0, 242, 5, 42, 1, 0, 0, 0, 22, 0, 20, 235, 225, 183, 220,
+                194, 147, 204, 170, 14, 231, 67, 168, 111, 137, 223, 130, 88, 194, 8, 252, 0, 0, 0,
+                0, 0, 0, 0, 0, 38, 106, 36, 170, 33, 169, 237, 226, 246, 28, 63, 113, 209, 222,
+                253, 63, 169, 153, 223, 163, 105, 83, 117, 92, 105, 6, 137, 121, 153, 98, 180, 139,
+                235, 216, 54, 151, 78, 140, 249, 1, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+            ]
+            .try_into()
+            .unwrap(),
+            merkle_path: vec![].try_into().unwrap(),
+        };
+
+        let share = |sequence_number: u32, job_id: u32, ntime: u32| SubmitSharesExtended {
+            channel_id,
+            sequence_number,
+            job_id,
+            nonce: 741057,
+            ntime,
+            version: 536870912,
+            extranonce: vec![1, 0, 0, 0, 0, 0, 0, 0].try_into().unwrap(),
+        };
+
+        // activate a chain tip at nTime t via a future job
+        let tip_ntime: u32 = 1745596930;
+        channel.on_new_extended_mining_job(job(1, None)).unwrap();
+        // network target: 000000000000d7c0... (hard, so no accidental BlockFound)
+        channel
+            .on_set_new_prev_hash(SetNewPrevHashMp {
+                channel_id,
+                job_id: 1,
+                prev_hash: [
+                    200, 53, 253, 129, 214, 31, 43, 84, 179, 58, 58, 76, 128, 213, 24, 53, 38, 144,
+                    205, 88, 172, 20, 251, 22, 217, 141, 21, 221, 21, 0, 0, 0,
+                ]
+                .into(),
+                nbits: 453040064,
+                min_ntime: tip_ntime,
+            })
+            .unwrap();
+
+        // re-confirm the chain-tip lower bound still holds
+        let res = channel.validate_share(share(0, 1, tip_ntime - 1));
+        assert!(matches!(res.unwrap_err(), ShareValidationError::Invalid(_)));
+
+        // install an immediately-active job whose own min_ntime is later than the tip's
+        let job_min_ntime = tip_ntime + 3;
+        channel
+            .on_new_extended_mining_job(job(2, Some(job_min_ntime)))
+            .unwrap();
+
+        // a share in the gap passes the chain-tip bound but not the job's own bound
+        let res = channel.validate_share(share(1, 2, job_min_ntime - 1));
+        assert!(matches!(res.unwrap_err(), ShareValidationError::Invalid(_)));
+
+        let res = channel.validate_share(share(2, 2, job_min_ntime));
+        assert!(matches!(res, Ok(ShareValidationResult::Valid(_))));
+    }
+
+    #[test]
+    fn test_share_validation_ntime_above_max_future_block_time() {
+        // Regression test: a share ntime beyond the chain tip's min_ntime +
+        // MAX_FUTURE_BLOCK_TIME would put a consensus-invalid timestamp in the block header,
+        // so it must be rejected; ntime exactly on the bound is still accepted.
+        let channel_id = 1;
+        let extranonce_prefix = [
+            0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1,
+        ]
+        .to_vec();
+
+        let mut channel = ExtendedChannel::new(
+            channel_id,
+            "user_identity".to_string(),
+            ExtranoncePrefix::from_wire(extranonce_prefix).unwrap(),
+            Target::from_le_bytes([0xff; 32]),
+            1.0,
+            true,
+            8u16,
+        );
+
+        channel
+            .on_new_extended_mining_job(NewExtendedMiningJob {
+                channel_id,
+                job_id: 1,
+                min_ntime: Sv2Option::new(None),
+                version: 536870912,
+                version_rolling_allowed: true,
+                coinbase_tx_prefix: vec![
+                    2, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 34, 82, 0,
+                ]
+                .try_into()
+                .unwrap(),
+                coinbase_tx_suffix: vec![
+                    255, 255, 255, 255, 2, 0, 242, 5, 42, 1, 0, 0, 0, 22, 0, 20, 235, 225, 183,
+                    220, 194, 147, 204, 170, 14, 231, 67, 168, 111, 137, 223, 130, 88, 194, 8, 252,
+                    0, 0, 0, 0, 0, 0, 0, 0, 38, 106, 36, 170, 33, 169, 237, 226, 246, 28, 63, 113,
+                    209, 222, 253, 63, 169, 153, 223, 163, 105, 83, 117, 92, 105, 6, 137, 121, 153,
+                    98, 180, 139, 235, 216, 54, 151, 78, 140, 249, 1, 32, 0, 0, 0, 0, 0, 0, 0, 0,
+                    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                    0, 0,
+                ]
+                .try_into()
+                .unwrap(),
+                merkle_path: vec![].try_into().unwrap(),
+            })
+            .unwrap();
+
+        let tip_ntime: u32 = 1745596930;
+        // network target: 000000000000d7c0... (hard, so no accidental BlockFound)
+        channel
+            .on_set_new_prev_hash(SetNewPrevHashMp {
+                channel_id,
+                job_id: 1,
+                prev_hash: [
+                    200, 53, 253, 129, 214, 31, 43, 84, 179, 58, 58, 76, 128, 213, 24, 53, 38, 144,
+                    205, 88, 172, 20, 251, 22, 217, 141, 21, 221, 21, 0, 0, 0,
+                ]
+                .into(),
+                nbits: 453040064,
+                min_ntime: tip_ntime,
+            })
+            .unwrap();
+
+        let share = |sequence_number: u32, ntime: u32| SubmitSharesExtended {
+            channel_id,
+            sequence_number,
+            job_id: 1,
+            nonce: 741057,
+            ntime,
+            version: 536870912,
+            extranonce: vec![1, 0, 0, 0, 0, 0, 0, 0].try_into().unwrap(),
+        };
+
+        // one second above the bound: rejected before any PoW evaluation
+        let res = channel.validate_share(share(0, tip_ntime + crate::MAX_FUTURE_BLOCK_TIME + 1));
+        assert!(matches!(res.unwrap_err(), ShareValidationError::Invalid(_)));
+
+        // u32::MAX is likewise rejected (the bound saturates instead of wrapping)
+        let res = channel.validate_share(share(1, u32::MAX));
+        assert!(matches!(res.unwrap_err(), ShareValidationError::Invalid(_)));
+
+        // exactly on the bound the share is accepted (channel target is permissive)
+        let res = channel.validate_share(share(2, tip_ntime + crate::MAX_FUTURE_BLOCK_TIME));
+        assert!(matches!(res, Ok(ShareValidationResult::Valid(_))));
+    }
 }

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

@@ -35,6 +35,20 @@ pub const MAX_FUTURE_JOBS: usize = 16;
 /// small measured memory cost — see the load-test data in PR #2290.
 pub const MAX_PAST_JOBS: usize = 50;
 
+/// Maximum number of accepted-share hashes a client channel retains for duplicate detection.
+///
+/// 4 096 hashes is one 128 KB allocation, which keeps the `no_std`/embedded use case viable:
+/// the bound has to be affordable on the smallest supported device, since an adversarial
+/// upstream advertising a trivial target can drive the cache to it at message speed.
+///
+/// A client cache does not need to hold a whole chain tip's worth of shares. It exists to catch
+/// a share source re-submitting work it already sent — a retransmit or a buggy loop, which
+/// arrives within seconds — not to reconcile a tip. 4 096 covers ~11 hours of history for a
+/// typical 6 shares/min channel and ~7 minutes for a very busy 600 shares/min proxy channel,
+/// far beyond any realistic duplicate window in both cases. Overflow evicts oldest-first, and
+/// an evicted-then-replayed hash costs one double-counted local statistic.
+pub const MAX_SEEN_SHARES: usize = 4_096;
+
 // 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/share_accounting.rs

@@ -5,8 +5,8 @@
 //! are intended for use in Mining Clients.
 
 extern crate alloc;
-use super::{HashMap, HashSet};
-use alloc::string::String;
+use super::{HashMap, MAX_SEEN_SHARES};
+use alloc::{collections::VecDeque, string::String};
 use bitcoin::hashes::sha256d::Hash;
 use mining_sv2::{
     ERROR_CODE_SUBMIT_SHARES_BAD_EXTRANONCE_SIZE, ERROR_CODE_SUBMIT_SHARES_DIFFICULTY_TOO_LOW,
@@ -109,7 +109,11 @@ pub struct ShareAccounting {
     validated_shares: u32,
     validated_work_sum: f64,
     rejected_shares: HashMap<String, u32>, // <error_code, count>
-    seen_shares: HashSet<Hash>,
+    // Accepted share hashes, oldest at the front; bounded by `MAX_SEEN_SHARES`.
+    // A flat `VecDeque` rather than a set plus a companion order queue: at this size a linear
+    // scan costs less than a hash lookup would (random hashes diverge in the first byte), and
+    // storing each hash once halves the footprint that matters on embedded targets.
+    seen_shares: VecDeque<Hash>,
     best_diff: f64,
     blocks_found: u32,
 }
@@ -131,7 +135,7 @@ impl ShareAccounting {
             validated_work_sum: 0.0,
 
             rejected_shares: HashMap::new(),
-            seen_shares: HashSet::new(),
+            seen_shares: VecDeque::new(),
             best_diff: 0.0,
             blocks_found: 0,
         }
@@ -190,6 +194,17 @@ impl ShareAccounting {
     /// called when the upstream server confirms via [`SubmitSharesSuccess`](mining_sv2::SubmitSharesSuccess).
     ///
     /// `validated_shares` saturates at `u32::MAX`.
+    ///
+    /// At most [`MAX_SEEN_SHARES`] hashes are retained for duplicate detection; beyond that the
+    /// oldest hash is evicted.
+    ///
+    /// Unlike the server side, overflow evicts rather than failing: a replay of an evicted hash
+    /// only double-counts one local statistic — nothing is paid out, and nothing is forwarded as
+    /// newly-validated that the upstream won't independently dedup. That is also why the bound
+    /// is a flat constant here instead of being derived from the channel's target and hashrate:
+    /// the target is upstream-controlled, so a derived bound could not constrain a hostile
+    /// upstream anyway, and it would have to be clamped to something affordable on the smallest
+    /// supported device regardless — which is exactly what [`MAX_SEEN_SHARES`] already is.
     pub fn track_validated_share(
         &mut self,
         share_sequence_number: u32,
@@ -199,13 +214,21 @@ impl ShareAccounting {
         self.last_share_sequence_number = share_sequence_number;
         self.validated_shares = self.validated_shares.saturating_add(1);
         self.validated_work_sum += share_work;
-        self.seen_shares.insert(share_hash);
+        if !self.seen_shares.contains(&share_hash) {
+            // evict before inserting, so the queue never exceeds the bound even transiently and
+            // its backing allocation settles at exactly `MAX_SEEN_SHARES` entries
+            if self.seen_shares.len() == MAX_SEEN_SHARES {
+                self.seen_shares.pop_front();
+            }
+            self.seen_shares.push_back(share_hash);
+        }
     }
 
     /// Clears the set of seen share hashes.
     ///
-    /// Should be called on every chain tip update
-    /// to prevent unbounded memory growth.
+    /// Should be called on every chain tip update to allow new shares for the new tip. This is
+    /// also what makes the seen-shares cap per-tip: the set only ever holds one chain tip's
+    /// worth of validated shares.
     pub fn flush_seen_shares(&mut self) {
         self.seen_shares.clear();
     }
@@ -266,6 +289,9 @@ impl ShareAccounting {
     }
 
     /// Checks if the given share hash has already been seen (duplicate detection).
+    ///
+    /// The underlying queue holds at most [`MAX_SEEN_SHARES`] hashes (oldest evicted first) and
+    /// is flushed on every chain-tip transition.
     pub fn is_share_seen(&self, share_hash: Hash) -> bool {
         self.seen_shares.contains(&share_hash)
     }
@@ -297,7 +323,7 @@ impl ShareAccounting {
 
 #[cfg(test)]
 mod tests {
-    use super::{alloc::format, ShareAccounting, UNKNOWN_ERROR_CODE};
+    use super::{alloc::format, ShareAccounting, MAX_SEEN_SHARES, UNKNOWN_ERROR_CODE};
     use bitcoin::hashes::Hash as _;
 
     #[test]
@@ -371,4 +397,36 @@ mod tests {
         );
         assert_eq!(accounting.get_rejected_shares_count(), 10_002);
     }
+
+    #[test]
+    fn seen_shares_are_bounded_by_fifo_eviction() {
+        fn hash(i: u32) -> bitcoin::hashes::sha256d::Hash {
+            let mut bytes = [0u8; 32];
+            bytes[..4].copy_from_slice(&i.to_le_bytes());
+            <bitcoin::hashes::sha256d::Hash as bitcoin::hashes::Hash>::from_slice(&bytes).unwrap()
+        }
+
+        let cap = MAX_SEEN_SHARES as u32;
+        let overflow = 100;
+        let mut accounting = ShareAccounting::new();
+
+        // flood past the bound with unique hashes, as an adversarial upstream advertising a
+        // trivial target would
+        for i in 0..cap + overflow {
+            accounting.track_validated_share(i, hash(i), 1.0);
+        }
+
+        // retention is bounded, and it is the oldest hashes that were dropped
+        assert_eq!(accounting.seen_shares.len(), cap as usize);
+        for i in 0..overflow {
+            assert!(!accounting.is_share_seen(hash(i)));
+        }
+        for i in overflow..cap + overflow {
+            assert!(accounting.is_share_seen(hash(i)));
+        }
+
+        // a chain-tip transition clears both the set and the eviction order
+        accounting.flush_seen_shares();
+        assert_eq!(accounting.seen_shares.len(), 0);
+    }
 }