fix: validate pool-controlled extranonce2_size and bound its consumers - #90
Open
Schnitzel wants to merge 3 commits into
Open
fix: validate pool-controlled extranonce2_size and bound its consumers#90Schnitzel wants to merge 3 commits into
Schnitzel wants to merge 3 commits into
Conversation
The pool-supplied extranonce2_size was accepted without bounds checking and later narrowed with `as u8`. A size of 0 or >8 made every job fail template conversion (miner connected but idle), and a value like 264 silently wrapped to 8, mining an extranonce2 space the pool never offered so every share would be rejected. Validate the value at subscribe time, accepting only the 1-8 byte range Extranonce2 supports, and fail the subscription otherwise; the source reconnects with the usual backoff.
assign_job_to_threads split the job's extranonce2 range across eligible threads with an expect(). Extranonce2Range::split returns None when the range holds fewer values than there are threads — reachable with a pathological pool-advertised extranonce2 size (e.g. 1 byte = 256 values) and a large thread count — panicking the scheduler task, which nothing supervises: mining silently halted while the API kept serving stale telemetry. Replace the expect with a let-else that logs and skips the job. Adds a regression test driving 257 stub threads against a 1-byte EN2 space.
The thread count parsed from the environment was unbounded, turning a typo into a resource problem and making pathological thread counts easy to reach in testing. Clamp with a warning, far above any real core count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The pool-supplied
extranonce2_size(frommining.subscribe) flowed into job construction and work splitting with two lossy/assuming steps, all reachable by a malicious pool or a MITM (the stratum transport is plaintext):as u8truncation injob_to_template: a size of 0 or >8 made every job fail template conversion — the miner stayed connected but mined nothing; a value like 264 silently wrapped to 8, mining an extranonce2 space the pool never offered (all shares invalid at the pool).expect()onExtranonce2Range::splitin the scheduler:splitreturnsNonewhen the range holds fewer values than there are eligible threads. Pathological size (e.g. 1 byte = 256 values) + 257+ threads → the scheduler task panicked, and since nothing supervises it, mining silently halted while the API kept serving stale telemetry.MUJINA_CPUMINER_THREADSmade such thread counts trivial to reach (CPU backend) and turned typos into resource problems.Found during a source review of pool-facing input handling.
Fix (one commit per change)
fix(stratum_v1): reject out-of-range extranonce2_size at subscribe— accept only the 1–8 byte rangeExtranonce2supports; fail the subscription otherwise (reconnect with the usual backoff). This also makes the downstreamas u8provably lossless.fix(scheduler): skip jobs too small to split across threads— replace theexpectwith a let-else that logs (source, thread count, EN2 space) and skips the job. Regression test included: 257 stub threads vs a 1-byte EN2 space.fix(cpu_miner): clamp MUJINA_CPUMINER_THREADS to 4096— warning on clamp; far above any real core count.Tests
test_subscribe_rejects_out_of_range_extranonce2_size(0, 9, 264 →SubscriptionFailed)test_subscribe_accepts_valid_extranonce2_size(1, 4, 8 → state set)assign_job_skips_when_en2_space_smaller_than_thread_count(no panic; no tasks assigned)test_thread_count_clamped_to_maxcargo fmt,cargo clippy(no new warnings), andcargo test(356 passed) are green; each commit passes on its own (verified individually).