Skip to content

Reject non-finite input in hash_rate_to_target - #2221

Open
gimballock wants to merge 2 commits into
stratum-mining:mainfrom
marafoundation:fix/hash-rate-to-target-reject-non-finite
Open

Reject non-finite input in hash_rate_to_target#2221
gimballock wants to merge 2 commits into
stratum-mining:mainfrom
marafoundation:fix/hash-rate-to-target-reject-non-finite

Conversation

@gimballock

Copy link
Copy Markdown

Reject non-finite input in hash_rate_to_target

Summary

channels_sv2::target::hash_rate_to_target accepts non-finite arguments
(NaN, ±infinity) and silently produces a garbage target instead of
returning an error. For a +inf hashrate the garbage target is the hardest
the function can emit
— it drives difficulty to the ceiling. This PR rejects
non-finite input up front with a dedicated error variant. It is a pure
input-validation fix: no behavioral change to any caller's control path.

The bug, and why +inf is the dangerous one

The function guards only three cases today:

if share_per_min == 0.0 { return Err(DivisionByZero); }
if share_per_min.is_sign_negative() { return Err(NegativeInput); }
if hashrate.is_sign_negative() { return Err(NegativeInput); }

None of these screen for finiteness, and the intermediate cast saturates
silently:

let h_times_s = hashrate * shares_occurrency_frequence;
let h_times_s = h_times_s as u128;   // NaN -> 0, +inf -> u128::MAX

So a non-finite argument slips past every guard and reaches the target
arithmetic. The three non-finite hashrate inputs resolve as:

Input as u128 Resulting target Difficulty
0.0 (already accepted) 0 ≈ max target easiest
NaN 0 ≈ max target easiest
+inf u128::MAX 0 hardest

The easy cases (NaN, 0.0) are self-correcting — an over-easy target makes
the miner over-produce shares and the controller tightens back up. The +inf
case is not.
It silently emits the maximally over-difficult target — the
over-difficulty direction — from a converter whose contract is to reject bad
input. A miner whose telemetry reports +inf hashrate (a divide-by-zero in a
hashrate estimate, an overflow, an uninitialized sensor) would be handed the
tightest possible difficulty with no error raised anywhere. That is the failure
worth closing at the source: a single non-finite input driving difficulty to the
ceiling.

The fix

Reject non-finite hashrate and share_per_min before any arithmetic, with
a dedicated variant:

if !hashrate.is_finite() || !share_per_min.is_finite() {
    return Err(HashRateToTargetError::NonFiniteInput);
}

Two details that matter:

  • Both operands are screened. A non-finite share_per_min reaches the same
    saturating cast path (60.0 / share_per_min then into the work term), so
    screening only the hashrate would leave the symmetric hole open.
  • The check is ordered first, and the order is load-bearing. A NaN
    compares false to 0.0 and is sign-positive, so it would fall through the
    existing == 0.0 and is_sign_negative() checks to the cast; a -inf is
    sign-negative and would be mis-reported as NegativeInput. Putting the finite
    screen ahead of the others is what makes the rejection correct and the error
    variant accurate. A test pins this ordering.

A new HashRateToTargetError::NonFiniteInput variant (rather than overloading
NegativeInput) keeps the fault honest for callers: "you passed NaN" is a
different error than "you passed a negative number."

Tests

6 unit tests in target.rs (the function had no test module previously):

  • non_finite_hashrate_is_rejectedNaN/+inf/-inf hashrate → NonFiniteInput
  • non_finite_share_per_min_is_rejected — same for share_per_min (both operands)
  • neg_infinity_is_non_finite_not_negative — the load-bearing ordering: -inf
    and a NaN share_per_min are reported NonFiniteInput, not NegativeInput
    / DivisionByZero
  • positive_infinity_hashrate_does_not_yield_a_target — the headline case: a
    +inf hashrate no longer produces the hardest-difficulty target
  • finite_input_still_converts / preexisting_finite_guards_unchanged
    regression guards: ordinary finite conversion still works, zero hashrate is
    still accepted, and the pre-existing NegativeInput / DivisionByZero guards
    are unchanged

Scope

  • One file: sv2/channels-sv2/src/target.rs (+~20 lines of fix, the rest
    tests). No other crate consumes HashRateToTargetError by exhaustive match,
    so the added variant is non-breaking.
  • Input validation only — no behavioral/policy change. This does not alter
    how any controller responds to a valid hint; it rejects inputs that were
    never valid. (Hint-handling policy — how a consumer should react to a
    difficulty revision — is a separate concern and deliberately out of scope here.)

gimballock pushed a commit to marafoundation/stratum that referenced this pull request Jul 1, 2026
…ning#2221 class)

Records the bounded result of a parallel source-audit of channels_sv2's
public input surface for the stratum-mining#2221 bug class (functions accepting
non-finite/out-of-range numeric input that produce garbage/dangerous
control values). Confirmed independent findings: A (vardiff new_with_min
NaN-disables-clamp) + B (try_vardiff divisor finiteness), both fixed on
branch fix/vardiff-reject-non-finite-hashrate; C (server set_nominal_hashrate
unvalidated store) as a defense-in-depth note. Six garbage-target caller
sites are the blast radius of stratum-mining#2221 (closed on its merge), not new findings.
Cleared the false suspects (hash_rate_from_target, client setters,
share-accounting sums) — the cleared set outnumbers the confirmed.

Sequencing decided: let stratum-mining#2221 merge first, prepare A+B ready-but-unopened,
then let maintainers' convention choose the tracking construct.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@par1ram par1ram left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one remaining correctness gap in the new validation. The guard at target.rs:93 validates only the operands, not the derived work value. Both inputs can be finite while 60.0 / share_per_min or the subsequent multiplication is non-finite or larger than u128::MAX. On the current merge with main, hash_rate_to_target(f64::from(f32::MAX), 1.0)—within the domain of the existing f32 channel callers—passes the guard, saturates to u128::MAX, and panics at target.rs:119 on h_times_s + 1 in debug builds; release builds continue with a garbage target. Please validate the computed floating-point work for finiteness and representability, with room for the + 1, before casting, return an error, and add this case as a regression test. I reproduced this with a catch_unwind regression test. The existing 83 crate tests, cargo clippy -p channels_sv2 --lib -- -D warnings, and the no_std check otherwise pass.

gimballock pushed a commit to marafoundation/stratum that referenced this pull request Jul 28, 2026
The finiteness screen guarded the two arguments but not the value derived
from them. Both can be finite while `hashrate * (60 / share_per_min)` is
non-finite or too large for u128: `f64::from(f32::MAX)` is inside the domain
of the f32 channel callers (`nominal_hashrate: f32` in
server/{standard,extended}.rs, widened via `.into()`), and at 1 share/min it
yields h*s ~= 2.04e40 -- 60x above u128::MAX.

`as u128` saturates rather than wrapping, so that became u128::MAX and then
overflowed on `h_times_s + 1`: a panic in debug builds, and in release a
silent garbage target (the `max()` intended to absorb it never runs).

Validate the product for finiteness and representability before the cast,
with headroom for the `+ 1`, and return a dedicated `WorkOutOfRange`
variant -- `NonFiniteInput` would misname the representable-but-too-large
case.

Three regression tests: the f32::MAX path, a non-finite product from finite
operands, and a just-under-the-limit case pinning the boundary against
over-rejection. The first two panic without the guard.

Reported by @par1ram in review of stratum-mining#2221.

Co-Authored-By: Claude <noreply@anthropic.com>
@gimballock

Copy link
Copy Markdown
Author

Confirmed and fixed in e9209f29. Thanks — this was a real gap, and I reproduced your case before fixing it:

thread '...' panicked at sv2/channels-sv2/src/target.rs:119:45:
attempt to add with overflow

Exactly as you described: both operands are finite so they pass the guard, f32::MAX × 60 ≈ 2.04e40 sits ~60× above u128::MAX, the as u128 cast saturates, and h_times_s + 1 overflows before max() can absorb it. The max(h_times_s, h_times_s + 1) was clearly meant as the saturation guard, but it only works in release.

The fix validates the derived product before the cast, with headroom for the + 1:

if !h_times_s.is_finite() || h_times_s >= u128::MAX as f64 {
    return Err(HashRateToTargetError::WorkOutOfRange);
}

I added a dedicated WorkOutOfRange variant rather than reusing NonFiniteInput — the headline case is representable-but-too-large, not non-finite, and reusing the existing name would misdescribe it. The enum is new in this PR, so no breaking change.

Three regression tests:

  • work_exceeding_u128_is_rejected_not_saturated — your f32::MAX case
  • non_finite_derived_work_is_rejected — a second route into the same gap: finite operands whose product overflows to +inf (f64::MAX at 1e-300 spm). Worth covering separately since it reaches the bad cast via +inf → u128::MAX rather than via saturation.
  • work_just_below_the_limit_still_converts — guards against over-rejection and pins the boundary, so a future tightening is a visible change

I verified the first two are genuine regression guards rather than assuming it: with the new guard replaced by if false, both fail with the overflow panic; with it restored, all pass.

Gates (on 1.89, matching ci.yaml): 86 lib tests pass (83 + 3 new), clippy -p channels_sv2 --lib -- -D warnings clean, --no-default-features --features no_std clean, cargo fmt --check clean.

One note on severity, not to argue the fix but to calibrate it: overflow needs hashrate > ~5.67e36 H/s at 1 spm, roughly 22 orders of magnitude above a real S21 (~2e14 H/s). No honest miner reaches it — it takes a malformed or hostile OpenMiningChannel. So this is hostile-input hardening rather than a live bug. That said, I think the release-build behavior is the more concerning half: a silently garbage target is worse than a panic, and it's in the over-difficulty direction. Glad you caught it.

Eric Price and others added 2 commits July 28, 2026 13:11
hash_rate_to_target guards only against zero/negative share_per_min and
negative hashrate. It does NOT screen for non-finite values, and the
intermediate `h * s as u128` cast saturates silently: `NaN as u128` is 0
and `f64::INFINITY as u128` is u128::MAX. So a NaN or infinite argument
slips past every existing guard and yields a garbage target with no error.

The +inf case is the dangerous one. A +inf hashrate casts to the maximum
possible work, which collapses the target toward zero — the HARDEST
difficulty the function can emit. So a single non-finite hashrate silently
produces a maximally over-difficult target (the over-difficulty direction),
from a converter whose contract is to reject bad input. NaN and 0.0 produce
the easiest target instead, but the asymmetry is the point: one non-finite
input drives difficulty to the ceiling.

Reject non-finite hashrate AND share_per_min up front, with a dedicated
HashRateToTargetError::NonFiniteInput variant. The check is ordered FIRST,
ahead of the zero/negative checks: a NaN compares false to 0.0 and is
sign-positive, and -inf is sign-negative, so without the leading finite
screen they would fall through to DivisionByZero / NegativeInput / the cast.
The ordering is the soundness property and is pinned by a test.

Pure robustness fix — input validation only, no behavioral change to any
caller's control path. 6 unit tests: each non-finite value on each operand,
the load-bearing ordering (-inf and NaN screened as NonFiniteInput, not
Negative/DivisionByZero), the +inf-yields-no-target headline, and that the
pre-existing finite guards and valid conversions are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The finiteness screen guarded the two arguments but not the value derived
from them. Both can be finite while `hashrate * (60 / share_per_min)` is
non-finite or too large for u128: `f64::from(f32::MAX)` is inside the domain
of the f32 channel callers (`nominal_hashrate: f32` in
server/{standard,extended}.rs, widened via `.into()`), and at 1 share/min it
yields h*s ~= 2.04e40 -- 60x above u128::MAX.

`as u128` saturates rather than wrapping, so that became u128::MAX and then
overflowed on `h_times_s + 1`: a panic in debug builds, and in release a
silent garbage target (the `max()` intended to absorb it never runs).

Validate the product for finiteness and representability before the cast,
with headroom for the `+ 1`, and return a dedicated `WorkOutOfRange`
variant -- `NonFiniteInput` would misname the representable-but-too-large
case.

Three regression tests: the f32::MAX path, a non-finite product from finite
operands, and a just-under-the-limit case pinning the boundary against
over-rejection. The first two panic without the guard.

Reported by @par1ram in review of stratum-mining#2221.

Co-Authored-By: Claude <noreply@anthropic.com>
@gimballock
gimballock force-pushed the fix/hash-rate-to-target-reject-non-finite branch from e9209f2 to 58f6baf Compare July 28, 2026 17:28
@gimballock
gimballock requested a review from par1ram July 28, 2026 17:29
@par1ram

par1ram commented Jul 28, 2026

Copy link
Copy Markdown

The derived-work validation in 58f6baf fixes the correctness issue I reported. I rechecked the exact f64 boundary, protocol-facing f32 edge cases, and both debug and release paths, and found no remaining numerical issue.

There is one separate blocking API-compatibility issue: HashRateToTargetError already exists as a public exhaustive enum in channels_sv2 7.0.0. Adding NonFiniteInput and WorkOutOfRange is therefore a SemVer-major change for downstream exhaustive matches, even if no current workspace caller matches it. This is the enum_variant_added failure reported by semver-check, so the claim that the enum is new/non-breaking is incorrect.

Please resolve this according to the project's release policy—either bump the major version or preserve the existing public enum surface—and update the PR's compatibility claim. Once that is resolved and CI is green, the numerical fix looks good to me.

@gimballock

Copy link
Copy Markdown
Author

You're right, and my compatibility claim was wrong — thanks for catching it. HashRateToTargetError is already public in the released 7.0.0 with DivisionByZero/NegativeInput, so both variants I added (NonFiniteInput in the first commit, WorkOutOfRange in the second) are enum_variant_added breaks for downstream exhaustive matches. I conflated "these variants are new in this PR" with "the enum is new," which isn't the same thing. semver-check is correctly failing; the other 14 jobs pass.

Also noting the SHA drifted: the fix is 58f6bafc after rebasing onto c1a79913, not the e9209f29 I cited earlier.

@plebhash @GitGab19 — this needs a release-policy call I don't want to make unilaterally. Three ways to resolve, and I'd rather implement your preference than guess:

  1. Major-bump channels_sv2 to 8.0.0 in this PR. There's clear precedent (ae3741dd bump to 5.0.0, 89e5841d to 4.0.0, fd67587a major-bump mining_sv2), so this looks like the normal path — but a major bump for a hostile-input hardening fix seems like your call, not mine.
  2. Add #[non_exhaustive] to the enum as well as the bump. Still breaking once, but it makes every future variant addition non-breaking. Worth doing now if you'd otherwise hit this again, though I'd note the enum is small and stable so the payoff is speculative.
  3. Keep the existing public surface — drop both variants and route these rejections through DivisionByZero/NegativeInput. No bump, but a NaN hashrate reported as DivisionByZero is actively misleading, and the diagnostic clarity was most of the point of the PR. I'd rather not, but it's a legitimate choice if you want to avoid a major release right now.

My preference is 1, or 1+2 if you want the future-proofing. Happy to push whichever you pick.

On severity, for whatever it's worth to the decision: the overflow needs a declared hashrate above ~5.67e36 H/s (roughly 22 orders of magnitude past a real S21), so it's only reachable via a malformed or hostile OpenMiningChannel — not a live bug. The release-build behavior is the more interesting half, since it silently yields a garbage target in the over-difficulty direction rather than panicking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants