fix(gpu): review follow-ups on the GPU grinding PR - #945
Merged
Conversation
Route the GPU dispatch and its tests through one inner-hash-to-lanes conversion. The tests built their own copy, so the line the prover actually runs was executed by nothing: swapping it to from_be_bytes would have kept every test green while is_valid_nonce rejected every device nonce at runtime and the search sat on the CPU fallback forever. stark::grinding:: inner_hash_lanes is now the single entry point, which also lets get_inner_hash go back to private. Report that fallback on stderr instead of log::warn. The CLI initialises env_logger with no default filter, so a warn-level line never prints unless RUST_LOG is set — and it is the only signal that the kernel has started returning garbage. The other device-decline paths already use eprintln with a [gpu] prefix. Wrap test-math-cuda in GPU_TEST_TIMEOUT. It was the only one of the five GPU targets without it, and it is Group 1 of gpu_test.sh, so a hang there costs Groups 2-5 as well and a job timeout yields `cancelled`, which skips the run-summary step and leaves no readable output. Document LAMBDA_VM_NO_GPU_GRIND in the profiling README's knob list. Drop the "Parity" framing from the test module: there is nothing to be at parity with, since any valid nonce is acceptable and the CPU's find_any does not agree with itself between runs. What is pinned is validity, plus the search completeness that minimality stands in for — noted as a probe rather than a contract, so a future kernel that deliberately returns any valid nonce relaxes the assertion instead of being treated as broken. Same for the doc on generate_nonce_maybe_gpu, which claimed "smallest" for both arms.
ColoCarletti
approved these changes
Aug 21, 2026
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
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.
Follow-ups from a review of #936, targeting its branch so they land as part of it. No behaviour changes to the grind itself.
The review found no correctness or security problem. The kernel's Keccak was traced against the host predicate twice independently — lane layout,
bswap64(nonce), the 0x01/0x80 padding for a 40-byte message,bswap64(st[0])versusfrom_be_bytes(digest[..8]), andlimitacross the admitted factor range — and they agree. More importantly the design is safe by construction:generate_nonce_maybe_gpure-checks every device nonce withis_valid_nonce, the same function the verifier calls, in release as well as debug. A broken kernel can cost speed. It cannot cost proof-of-work.The one that motivated this PR
The tests never exercised the conversion the prover runs.
tests/grinding.rsbuilt its owninner_hash→[u64; 4]lanes rather than calling the production path ingenerate_nonce_maybe_gpu. The two were byte-identical, which is exactly the hazard: flip the production line tofrom_be_bytes— it reads the same at a glance — and all three tests stay green, while at runtimeis_valid_noncerejects every device nonce and the feature silently reverts to the CPU search. The whole speedup, gone, with nothing red anywhere.It would also have been invisible in the logs. The only signal was
log::warn!, andbin/cli/src/main.rscalls bareenv_logger::init()with no default filter, so at default level that line never prints. So both halves are fixed together: onestark::grinding::inner_hash_lanescalled by the prover and the tests, and the fallback message moved toeprintln!("[gpu] …"), matching what the other device-decline paths already do.get_inner_hashgoes back to private as a result.CI
test-math-cudanow carries$(GPU_TEST_TIMEOUT). It was the only one of the five GPU targets without it. It is also Group 1 ingpu_test.shand the groups run sequentially, so a hang there costs Groups 2-5 too — and because a job timeout resolves ascancelledrather thanfailure, the run-summary step is skipped and you get no readable output at all. Pre-existing gap; worth closing now that the target runs a search loop rather than only fixed-size launches.Naming
Dropped the "Parity" framing from the test module. There is nothing here to be at parity with: any nonce satisfying
is_valid_nonceis as good as any other, and the CPU'sfind_anydoes not agree with itself between two runs. What the tests actually pin is validity, plus the search completeness that minimality stands in for.That distinction is worth writing down because it has a consequence. The minimality assertion is not a contract — it holds because
atomicMinover ascending blocks is an order-independent reduction, which is also why it is deterministic despite 262,144 threads. If someone later dropsatomicMinfor a cheaper first-writer-wins, the kernel is still correct and that test goes red. The comment now says to relax it to validity in that case rather than treat it as a defect. (Worth noting that swap probably buys very little: the grid-stride loop means all threads sweep the nonce space as one wavefront, so the existing early exit already collapses the grid within a single iteration of the first hit — minimality is close to free here.)Same correction to
generate_nonce_maybe_gpu's doc, which promised "the smallest valid one" for both arms when only the GPU arm does that.LAMBDA_VM_NO_GPU_GRINDis now in the profiling README's knob list, alongside theLAMBDA_VM_NO_GPU_LOGUPit is modelled on.For the description, since I can't edit it
Two numbers are quoted as if comparable: the body's "18.89s -> 13.10s = -30.6%" is 100tx e20, while the ABBA bot on this PR reports −9.55% (95% CI [−10.02%, −9.08%]) end-to-end on a real block. Both can be true — they measure different things — but they should be labeled. Worth adding in the same breath that
prover/src/recursion.rsand the default test options grind at factor 1, below the factor-12 gate, so wrap and recursion proves never take this path.Considered and dropped
An earlier draft wanted an iteration cap on the retry loop in
generate_nonce_gpu, on the grounds that a kernel which runs cleanly but never matches would spin instead of falling back. Dropped: the CPU path is(0..u64::MAX).find_any(…)and is exactly as unbounded, so this is not a regression in kind, and the trigger requires a kernel defect that does not exist. TheGPU_TEST_TIMEOUTabove closes the concrete half of that exposure regardless of the cause.Verified
make lintpasses all four clippy passes including the cuda one; the editedmath-cudatests type-check. Nothing was executed against a GPU — there is none locally, andgpu-testsruns only onmerge_group, so these tests first execute in the queue either way.