gekko: add --gekko-ticket-diff so Hathor tx jobs can be answered - #6
gekko: add --gekko-ticket-diff so Hathor tx jobs can be answered#6luislhl wants to merge 5 commits into
Conversation
set_ticket() was only ever called with 0.0, meaning "set the highest valid ticket the chip supports" - diff 16 on a GSF/Compac F. The call sites justify this with "pool will be above this anyway", which is true for Bitcoin pools and false for Hathor tx mining: tx jobs arrive at weight 17-32, i.e. difficulties far below 1. A chip at ticket 16 will not report any nonce below weight 36, so it can never answer a tx job at all, regardless of how fast it hashes. Measured against production tx-mining-service over 48h (133 txs): a 75.8 GH/s Compac F at ticket 16 needs 16 * 2^32 / 75.8e9 = ~906 ms to report anything, against a median tx lifetime of 54 ms, and solved 0 txs. A 40 GH/s NewPac - forced to ticket 1 by the BM1387 branch of the hashrate formula - answers in ~107 ms and wins the txs the Compac F cannot. At ticket 1 the Compac F would report in ~57 ms, making it the fastest device in the fleet. Replace the 0.0 arguments with an explicit HTR_TICKET_DIFF constant, including in the ticket-validation retry and give-up paths, which would otherwise silently revert the device to the maximum ticket after a single validation hiccup. Note that MAX_TICKET_CHECK is already documented as "checks allowed before forced to diff=1" and one give-up branch still logs "just set it to 1.0", so diff 1 appears to have been the original intent. The diff-1 row of ticket_1397[] validates cleanly: hi_limit 0.0 means no spurious "ticket too low" failures, and low_limit 1.9 is reached well within the 50-nonce count. Refs: HathorNetwork/ops-tools#1415, HathorNetwork/ops-tools#1426
Replaces the hardcoded HTR_TICKET_DIFF constant from the previous commit with a real cgminer option, so this fork stays usable against Bitcoin pools and the Hathor default is explicit rather than baked in. --gekko-ticket-diff <1-64>, default 1, or 0 for the chip's highest valid ticket. The value is passed straight through to set_ticket(), so 0 reproduces the stock upstream behaviour exactly. The setter rejects values in (0,1): set_ticket() floors its argument and the lowest ticket_1397[] entry is diff 1, so 0.5 would match no entry, fall through the loop, and silently leave the ticket unset. Documents the option and its Hathor rationale in README, along with the -fcommon requirement on GCC 10+ (the unmodified upstream tree fails to link without it too, so this is not specific to the fork). Verified in a clean Ubuntu 22.04 container: - builds with ./autogen.sh --enable-gekko CFLAGS="-O2 -fcommon" - --help lists the option with default 1.0 - 0, 1, 16, 64 accepted; 0.5, 100 and -1 rejected with the range message Refs: HathorNetwork/ops-tools#1415, HathorNetwork/ops-tools#1426 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTPQE8KnWcyAvBwbDeBufb
Review:
|
| device | chips | default freq | hashrate | resulting difficulty |
|---|---|---|---|---|
| Compac / Terminus (GSC/GSE, 1 chip) | 1 | 150 MHz | 8.25 GH/s | 1 ✅ |
| 2Pac (GSD, 2 chips) | 2 | 100 MHz | 11.0 GH/s | 2 ❌ |
| 2Pac tuned | 2 | 200 MHz | 22.0 GH/s | 4 ❌ |
The claim holds for BM1387 (hardcoded to 0) and for single-chip BM1384 at stock clocks. It does not hold for the 2Pac, and any BM1384 above ~156 MHz per chip crosses the threshold. A tuned 2Pac sits at difficulty 4 = weight 34, which is the same class of problem this PR fixes for the BM1397 — and there is no knob for it. The README currently tells those users they're fine.
8. Help text and comments promise a range the code does not honour; the applied value is invisible
(confidence: 9/10) — cgminer.c:2076, driver-gekko.c:2301/2343, driver-gekko.c:1059
cgminer.c:2076says"ticket difficulty 1-64", but the reachable set is{1,2,4,8,16,32,64}, further clamped bycclimit.--gekko-ticket-diff 40silently yields 32;--gekko-ticket-diff 64on a Compac F (cc = 672) silently yields 16. I confirmed3is accepted and floors to 2. There is no API field exposing the requested value —:6395exposes only the resultinginfo->difficulty.driver-gekko.c:2301// give up - just set it to maxand:2343// give up - just set it to 1.0now both describe the same call, and neither matches what it does. Theapplogtext"failed too many times setting to max"(:2303,:2345, and siblings) will actively mislead whoever debugs finding 4 on real hardware. Either fix the call per finding 4 or fix the strings.driver-gekko.c:1059:// with the lowest nonce_count of 150 below for diff 2, TICKET_BLOW_LIM 4 will always be exceeded if incorrectly set to diff 1— the lowestnonce_countin the table is now 50 (the diff-1 row), because that row was unreachable upstream and is now the default. The reasoning in this comment no longer describes the code.driver-gekko.c:1173-1175:if (!got) return;is the sole failure mode for finding 5 and for anycc == 0case, and it logs nothing. Anapplog(LOG_ERR, ...)there would make it self-diagnosing.
9. -Wunused-function in non-gekko builds
(confidence: 10/10 — observed in the build) — cgminer.c:1362
cgminer.c:1362:14: warning: 'set_float_ticket_diff' defined but not used [-Wunused-function]
The build itself succeeds with gekko disabled — the ifdef guards are correct (see below). But set_float_ticket_diff is defined outside #ifdef USE_GEKKO while its only caller is inside it. Note this is not the same as set_float_0_to_500, which does not warn because it also serves --flow-step-freq at cgminer.c:1978 (USE_FLOW). The P0 fix in finding 1 removes this warning as a side effect, since the write_config reference is a second use.
Verified correct
Things I checked because the PR description asserts them or because they looked risky, and which hold up:
- The three duplicated validation blocks are byte-identical. I extracted
driver-gekko.c:2280-2372,2535-2627,2800-2892and diffed them pairwise: zero differences. No copy-paste divergence, all six give-up/retry sites got the same treatment. - The diff-1 row analysis in the PR is right.
hi_limitis0.0and the enclosing guard isif (!info->ticket_ok && diff > 0)(:2282), sodiff < ticket_1397[i].hi_limitis unreachable — no spurious "ticket too low" failures, as claimed. AndP(share diff < 1.9) = 1 - 1/1.9 ≈ 47%per nonce, soticket_got_lowwithin the 50-nonce count is a near-certainty ((1/1.9)^50 ≈ 1.5e-12, consistent with the row'sErlang=1.5x10-7annotation). Side effect:below_noncesis now permanently 0, so the API'sTicketBelowfield (:6398) is dead — cosmetic only. - No unbounded retry loop, and
MAX_TICKET_CHECKstill works.set_ticket()resetsticket_work,ticket_nonces,below_nonces,ticket_ok,ticket_got_low— but deliberately notticket_failures(:1156-1165), which is monotonic between confirms and cleared only at:2331. Both branches still gate onticket_failures > MAX_TICKET_CHECK, and the give-up setsticket_ok = trueafterset_ticket()clears it, so give-up is terminal. Upstream's retry also re-sent the same value (init and retry both passed0.0), so the retry structure is genuinely unchanged — only the consequence differs, per finding 4. - Locking is correct at all 14 sites. The two init sites (
:1542,:1608) are reached fromcompac_scanworkwith noinfo->lockheld →locked=falseis right; all twelve validation sites sit inside theinfo->lockregion →locked=trueis right.set_ticket's callees use different mutexes (gekko_usleep→slock,gh_offset→ghlock,job_offset→joblock,compac_send2→none), so no self-deadlock, and the PR doesn't increase the frequency of the pre-existinginfo->lock-held-across-a-20ms-sleep pattern. - The ifdef guards line up.
float opt_gekko_ticket_diff = 1.0;atcgminer.c:355is inside#ifdef USE_GEKKO(314-…);externatminer.h:1098is inside#ifdef USE_GEKKO(1067-…); theOPT_WITH_ARGentry atcgminer.c:2074is inside#ifdef USE_GEKKO(1984-2083)../autogen.sh --enable-icarus(gekko off) builds and links cleanly, one cosmetic warning aside (finding 9). Thewrite_configfix in finding 1 is the one place that needs its own#ifdef. - The option is parsed before it's needed.
opt_parse/load_default_configrun long beforeusb_initialiseand device detect, and nothing in the API path mutatesopt_gekko_ticket_diffat runtime, so there's no mid-validation change hazard. - No software/hardware floor mismatch. I chased whether cgminer would reject the sub-diff-1 shares this is meant to enable.
submit_nonce→test_nonce(cgminer.c:8501,8464) hard-requires the top 32 bits of the hash to be zero, i.e. Bitcoin diff ≥ 1, beforesubmit_tested_workrunsfulltestagainst the Hathor weight target fromweight_to_target. That floor matches the chip's own mask-0x00baseline exactly, so ticket 1 is coherent end to end. It also means ticket 1 is genuinely the lowest achievable — worth stating in the README (see finding 6). - Hashrate accounting stays unbiased when the ticket actually takes: nonce rate scales up exactly as
info->difficultyscales down, soinfo->hashes,gh->diffsumandinc_hw_errors_n(thr, info->difficulty)keep the same expectation (variance improves). Only the mismatched case in finding 4 breaks it. - Range validation is otherwise solid —
(0,1), negatives,>64,infand non-numeric input are all rejected with a clear message, tested empirically. Onlynangets through (finding 5).
Also confirmed for context: the diff-1 (and diff-2/4/8) rows of ticket_1397[] were unreachable in upstream cgminer — every live call site passed 0.0, and cc for a 1-chip BM1397 is 672, which always resolves to the diff-16 row. This PR makes a never-before-exercised table row the default path. That's not an objection, but it's worth knowing that "it validates cleanly" has no field history behind it, only the reasoning (which I checked and agree with).
Open questions
- Is
1the right default, or should it be0with Hathor deployments opting in? Finding 3 is the crux: the escape hatch only helps someone who knows to reach for it, and it doesn't help a large Hathor rig at all. What's the deployment shape — single Compac Fs, or anything multi-chip? - Has anything been run against a device where the ticket write is dropped? Finding 4's chain (ticket mismatch → 16x hashrate under-report → 16x-tight
nonce_limit→ PT_NONONCE reset → frequency ratchet) is the failure mode I'd most want to see excluded on hardware before this lands, and it's the one the PR description's confidence doesn't cover. - Was config save ever exercised? Finding 1 suggests not — which is fair, since nothing else in this change touches it. Worth adding
saveto whatever manual smoke test you run, since it's a one-command check.
Out of scope, noticed in passing: gen_stratum_work reads pool->weight at cgminer.c:8076, but cg_runlock(&pool->data_lock) is released at :8067 — the read races the stratum thread's write at util.c:2347. Pre-existing (came in with #5), not this branch's problem, but flagging it since I was in there.
Reviewed with the /review workflow: full-diff read, targeted reading outside the diff (set_ticket, ticket_1397, the plateau/watchdog path, write_config, submit_nonce), an independent adversarial pass, and empirical verification in Docker (gekko and non-gekko builds, option-parsing matrix, crash reproduction, master bisect, fix validation). Nothing was committed, pushed, or left in the working tree.
write_config() decides how to serialise an option by comparing its callback pointer against a hardcoded list of setters. set_float_ticket_diff was not in the float list, so --gekko-ticket-diff fell through to the generic string branch, which does *(char **)&opt_gekko_ticket_diff: an 8-byte read of a 4-byte float, punned to a pointer and handed to strlen(). This crashed on every config save, with default settings, on any gekko build, whether or not the option was ever passed. Reproduced in a container: the API "save" command exits 139 (SIGSEGV) having written 0 bytes; the same build with this fix saves 1040 bytes containing "gekko-ticket-diff" : "1.0". Also in this commit: - Reject NaN in the setter. Every comparison against NaN is false, so it slipped past the range check and reached set_ticket(), where floor(NaN) matches no ticket_1397[] entry and would leave the device's ticket unset. - Guard the setter with USE_GEKKO so builds without gekko support don't warn about an unused static function. Verified with an --enable-icarus build, which compiles clean. All three found by review of PR #6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTPQE8KnWcyAvBwbDeBufb
|
Reviewed all 9 findings. The P0 is real — I reproduced it independently before fixing it, and it's a bug I could not have found by reading my own diff. Fixed in 27c01dd. P0 — confirmed and fixed
Same test on the fixed build: Worth stating plainly how bad this was: it fired on default settings, on every gekko build, whether or not anyone passed the flag — so it would have bricked config-save for every existing user of this fork, including the two miners who never wanted the new option. The finding is exactly right that pointer-identity registration is an invisible coupling; nothing in the option table hints that adding a setter obliges you to edit a function 4,500 lines away. Also fixed in the same commit:
P1 (watchdog) — mechanism confirmed, but with a counterweight from productionI verified the arithmetic: Two things that temper it:
So I'm treating this as the specific thing to watch during hardware validation rather than a blocker: grep the trial logs for P1 (cclimit) and P2 (give-up path) — real, and they need a human decisionBoth are genuine and I'm not resolving them unilaterally:
Verified-correct itemsThanks for chasing the Two README claims you flagged as wrong (BM1384 difficulty on the 2Pac, and "however fast it hashes") — both fair, fixing in the next push. |
Both flagged in review of PR #6. "can never answer a tx job, however fast it hashes" was wrong. A chip at ticket 16 reports only nonces of weight >= 36, and such a nonce over-satisfies a weight-17 tx - so it can answer one, just not before the tx is gone. The argument is latency, not impossibility, and the doc contradicted its own ticket * 2^32 / hashrate formula two lines later. The BM1384 claim that its hashrate-derived ticket "already lands at 1 in practice" only holds for slow sticks. bound(pow(2, ceil(log2(hashrate / 2^33))) - 1) gives difficulty 1 at ~5 GH/s, but 2 at ~10 GH/s and 4 at ~20 GH/s, so a well-tuned 2Pac carries a reporting floor of its own that this option does not address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTPQE8KnWcyAvBwbDeBufb
The three operators running this fork are on Raspberry Pis, and asking each of them to install a toolchain and build from source is the main friction in getting the ticket-diff fix onto the hardware. Build the binaries centrally instead and attach them to a GitHub release. Adds a tag-triggered workflow that builds linux-arm64 (Pi OS 64-bit), linux-armhf (Pi OS 32-bit) and linux-amd64. arm64 builds natively on GitHub's ARM runners; armhf needs QEMU because those runners are Cobalt/Ampere cores with no 32-bit support. One binary per architecture covers every GekkoScience model: they share driver-gekko.c and are selected at runtime by USB id, and the --gekko-*-detect allowlists all default to off. The build runs on Debian Bullseye rather than the runner's own Ubuntu 24.04. glibc is forward- but not backward-compatible, so a binary linked against glibc 2.39 would fail to start on Pi OS Bookworm. Bullseye also lets us skip libjansson-dev, which makes configure fall back to the bundled copy and link it statically - one less runtime dependency on the miner host. The shipped binary needs only libcurl4, libusb-1.0-0 and libncurses6. Every binary is smoke-tested before it ships. Besides --version and option validation, the tests drive a config save through the API and assert the process survives it. That is a regression test for the SIGSEGV found in review on this branch: write_config() dispatches on the option's callback pointer, so an unregistered setter gets type-punned into strlen(). It crashed on default settings regardless of whether the new flag was used, and no amount of reading the diff would have caught it. Also records zlib1g-dev as a build dependency - without it the link fails with "cannot find -lz". It was missing from the README alongside the -fcommon note. Verified locally for all three architectures under QEMU: smoke tests pass, file(1) confirms aarch64 / ARM EABI5 / x86-64, and the glibc floor is 2.29.
Problem
set_ticket()was only ever called withdiff = 0.0— "set the highest valid ticket the chip supports", which is diff 16 on a GSF/Compac F. The call sites state the assumption:That holds for Bitcoin pools. It does not hold for Hathor tx mining, where jobs arrive at weight 17-32 — difficulties far below 1.
The ticket mask is the minimum difficulty below which the chip will not report a nonce at all. At ticket 16 a device cannot report anything below weight 36, so it can never answer a tx job, regardless of how fast it hashes.
Evidence from production
Measured against mainnet
tx-mining-serviceover 48 h (133 txs), with all three miners' chips confirmed from their own device dumps:set_ticket(0.0)→ table maxdriver-gekko.c:1801Production tx lifetimes are bimodal: 81 % at weight ~17 with a median lifetime of 54 ms, 19 % at weight 31-32 with a median of 559 ms.
The Compac F is the fastest silicon in that fleet and the slowest to respond, purely because of a 16× ticket penalty the weaker devices don't pay. Its own log shows it:
At ticket 1 the same device would report in
2^32 / 75.8e9= ~57 ms, making it the fastest device in the fleet and competitive even on the light txs.Change
New option, replacing what started as a hardcoded constant:
The value is passed straight through to
set_ticket()at all 14 call sites, so0reproduces stock upstream behaviour exactly and the fork stays usable against Bitcoin pools. Default is 1 because this fork exists to mine Hathor.Crucially this covers the ticket-validation retry and give-up paths, not just the two init sites. Those also called
set_ticket(compac, 0.0, ...), so leaving them alone would silently revert the device to the maximum ticket after a single validation hiccup and quietly undo the fix.The setter rejects values in
(0,1):set_ticket()floors its argument and the lowestticket_1397[]entry is diff 1, so0.5would match no entry, fall through the loop, and leave the ticket unset.Two hints that diff 1 was the original intent upstream:
MAX_TICKET_CHECKis documented as "ticket restart checks allowed before forced to diff=1".0.0, and every one of those sites carries a commented-out//set_ticket(compac, 1.0, true, true);directly above it.The diff-1 row of
ticket_1397[]validates cleanly, so this does not trip the ticket checker:hi_limit = 0.0⇒ thediff < hi_limit"ticket too low" test can never fire.low_limit = 1.9⇒ticket_got_lowis set almost immediately, well within the 50-noncenonce_count.Build verification
Built and exercised in a clean
ubuntu:22.04container:cgminer --version→cgminer 4.13.1--helplists the option with(default: 1.0)0,1,16,64accepted;0.5,100,-1rejected withValue out of range - use 0 for the chip maximum, or 1 to 64-fcommonis required on GCC 10+ and is not specific to this PR — the unmodified upstream tree fails identically without it (multiple definition of 'bab_drv'etc., because the codebase predates-fno-commonbecoming the default). Verified by buildingorigin/masterthe same way. Documented in README.Still to do before this leaves draft
set ticket to 0x00/1instead of0xf0/16, and the device's tx solves should go from zero to competitive. This is the real test, and it doubles as the isolation run in ops-tools#1412.ticket_1397[]comment notes the max-16 default exists "to ensure enough nonces are coming back to identify status changes/issues" — the concern runs the opposite direction here, but worth confirming the USB path andgh/jobaccounting keep up.Shipping binaries to the miner operators — done
All three operators run Raspberry Pis, so requiring a local toolchain build was the main friction in getting this onto hardware. This PR now also adds
.github/workflows/build-release.yml, which builds and publishes prebuilt binaries.One binary per architecture covers every model. All GekkoScience devices share
driver-gekko.cand are selected at runtime by USB vendor/product id; the--gekko-*-detectoptions are opt-in allowlists that all default to off, so an unrestricted build detects the 2Pac, NewPac and Compac F alike. Frequency, core voltage and ticket difficulty are all runtime config. No per-model builds needed.Targets.
linux-arm64(Pi OS 64-bit,uname -m= aarch64),linux-armhf(Pi OS 32-bit, armv7l),linux-amd64(maintainer reproduction). Shipping both ARM variants means we don't have to collectuname -mfrom anyone first.Two deliberate build choices:
debian:bullseye, not the runner's Ubuntu 24.04. glibc is forward- but not backward-compatible; a binary linked against glibc 2.39 will not start on Pi OS Bookworm. Measured glibc floor of the produced binaries is 2.29, so they run on Bullseye and everything newer.libjansson-devdeliberately not installed, soconfigurefalls back to the bundledcompat/jansson-2.9and links it statically — one less runtime dependency on the miner host. Shipped binaries need onlylibcurl4,libusb-1.0-0,libncurses6, all normally present on Pi OS.Every binary is smoke-tested before it ships (
.github/docker/smoke-test.sh):--version, option registration and default, argument validation, and — most importantly — driving a configsavethrough the API and asserting the process survives. That last one is a regression test for the SIGSEGV found in review on this branch.write_config()dispatches on the option's callback pointer, so an unregistered setter gets type-punned intostrlen(); it crashed on default settings whether or not the new flag was used, and reading the diff would never have caught it.CI results (run
32774428750, all green): arm64 45s native onubuntu-24.04-arm, armhf 7m26s under QEMU (GitHub's Cobalt/Ampere cores have no 32-bit support, so emulation is unavoidable there), amd64 59s. Smoke tests pass on all three;file(1)confirmsARM aarch64/ARM EABI5/x86-64.Also fixed a gap in the documented build deps:
zlib1g-devis required — without it the link fails withcannot find -lz. It was missing from the README alongside the-fcommonnote.Each tarball carries an
INSTALL.txtwith pick-your-arch, swap-and-restart, verify and rollback steps, plusRUNTIME-DEPS.txt. Operators replace the binary; their systemd unit and udev/USB group setup are untouched.Note:
ubuntu-24.04-armis free because this repo is public. If it is ever made private, that job starts billing — the fallback is one matrix line to build arm64 under QEMU like armhf.Refs