fix(gpu): review follow-ups on the round-4 residency PR - #937
Merged
Conversation
Wrap the new gpu_force_downgrade target in GPU_TEST_TIMEOUT. That variable exists because a device-only cliff panic leaves the prover hung rather than aborting, holding the rented merge-queue box until the workflow timeout, and this target is the one that deliberately drives every device-only table through the decline path. Correct three comments that overstate or misdescribe what the code does: - DEFAULT_DEVICE_ONLY_MIN_LDE promises mid tables "degrade to CPU instead of aborting". That holds for the sites that read the LDE, which all gate on host_trace_empty(), but not for the R4 Merkle-proof gather: the host tree is root-only for every GPU-committed table whatever retain_host_lde says, so a declined gather has nothing to fall back to. Lowering the commit threshold widens that one abort site even though the device-only envelope is unmoved. - The new is_root_only assert claims the host walk would emit an empty path for position 0. get_proof_by_pos refuses root-only trees, so it panics instead — the assert's value is naming the cause, not preventing a bad proof. - gather_proofs_dev says callers fall back to the host tree on None. All three call sites .expect() and abort. Note that DEFAULT_GPU_LDE_THRESHOLD gates the whole dispatch layer, not just the commit, so moving it moves R2/R3/R4/FRI together. Document the four new env vars and h2d_histo.py in the profiling README, which is the toolkit's reference. Pin bary_num_chunks' three branches with unit tests, and cover the 64-chunk cap in the kernel parity tests — every existing case is rows-bound at 1-2 chunks, including the one annotated as exercising the occupancy branch.
ColoCarletti
approved these changes
Aug 18, 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 #888, targeting its branch so they merge as part of it. One CI wiring fix; the rest are comment/doc accuracy and test coverage. No behaviour changes.
The review found no correctness or soundness defect in #888. The verifier is untouched by that diff and every device-derived value is bound by a Merkle root the verifier re-hashes, so the worst outcome anywhere in it is a rejected or dead prove, never an accepted bad one.
The one that matters
Makefile— the newgpu_force_downgraderecipe line was missing$(GPU_TEST_TIMEOUT). The line above it has the wrapper, and the variable's own comment explains why: a panic on a device-only cliff assert leaves the prover hung rather than aborting, holding the rented merge-queue box until the workflow timeout. This target is the one that deliberately drives every device-only table through the decline path, so it is the most exposed in the suite. Without the wrapper a hang burns the full 240-minute workflow timeout instead of failing at 45 minutes with parseable output.Comments corrected
DEFAULT_DEVICE_ONLY_MIN_LDEpromised that mid tables "keep a host copy so a dispatch decline degrades to CPU instead of aborting". True for every site that reads the LDE — they all gate onhost_trace_empty()and take their host arm. Not true for the R4 Merkle-proof gather:try_expand_leaf_and_tree_row_major_keepmakes the host tree root-only for every GPU-committed table, independent ofretain_host_lde, so a declinedgather_proofs_devhas nothing to fall back to and aborts regardless of the host LDE. Since tree residency is gated ongpu_lde_threshold(), the 2^19 → 2^14 move widens that one abort site by 32× even though it leaves the device-only envelope alone. Availability only, and a proof gather is a ~70 KB allocation versus multi-GB for an LDE, so the marginal probability is low — but the comment as written would mislead whoever picks up #927.The new
is_root_onlyassert's rationale said the host walk "would emit an empty path for position 0 instead of failing".get_proof_by_posreturnsNoneon a root-only tree, so the fall-through panics at.expect("FRI query index in bounds"). The assert is worth keeping — it names the real cause instead of misdirecting at query indexing — but for that reason, not for preventing a bad proof. (The wording was inherited from an equally stale comment onmain.)gather_proofs_dev's doc said callers fall back to the host tree onNone. All three call sites.expect()and abort. This one is pre-existing, but it is exactly what made a "silent unverifiable proof" reading of this code look plausible to two independent reviewers, so it seemed worth closing while nearby.DEFAULT_GPU_LDE_THRESHOLDnow records that it gates the whole dispatch layer — R2 decompose, the R3 contexts, R4 DEEP, the FRI fold — not just the commit, so moving it moves 17 floors together. The sweep measured the aggregate, so the value is fine; the description was narrower than the constant.Plus two small ones:
PreUploadedMainTrace's doc claimed its equality matchedResidentMainTrace, which compares row counts; and the split-tree test's "(2^19 LDE)" parenthetical read as if it were quoting the threshold.Docs and tests
scripts/profiling/README.mdis the toolkit's reference and was missing all four new env vars andh2d_histo.py. Added, including a note thatLAMBDA_VM_GPU_DEVICE_ONLY_THRESHOLDis the finer instrument for shedding device-only tables — better than reaching forLAMBDA_VM_DISABLE_DEVICE_ONLY=1, which is what the #927 workaround currently uses.bary_num_chunksgets unit tests pinning which of its three terms binds per regime, plus degenerate inputs. This matters because every existing parity case is rows-bound at 1–2 chunks, including the one annotated as exercising the occupancy branch: at 100 columns and n=2^14 the rows term gives 2, well under occupancy's 20. Also added one parity case per kernel at the 64-chunk cap — the most chunks any shape can request — so the kernels are checked at both ends of the chunk range rather than only the bottom. The added cases cost about 67 MB and 50 MB of device memory. Also dropped an unused parameter inh2d_histo.py.make lintpasses all four clippy passes including the cuda one, and the new unit test runs without a GPU.For you to apply, since I can't edit #888's description
The body attributes the 187 GB → 70 GB PCIe reduction partly to the R1 trace pre-upload, but that bullet ships disabled (
LAMBDA_VM_TRACE_PREUPLOAD_MBdefaults to 0), and the code comment records that at 2^22 epochs the riding-ahead buffers pushed the prove past the card's headroom. One sentence saying it ships off would stop a reader attributing that figure to the default configuration.Deliberately not done
BARY_BLOCK_DIM(CUDA) andBLOCK_DIM(Rust) are still coupled by hand, and #888 adds two more kernels that depend on them matching. TheBARY_MAX_Ksingle-sourcing this PR introduced is exactly the right pattern for it, but it is pre-existing, and I have no local CUDA to compile a build-script change against — worth doing as its own PR.Also not attempted: recovery for a declined R4 proof gather. Rebuilding or downloading the tree at that point is real work, not a review fix. If the abort rate under pressure turns out to matter, that is the fix, and it would make the
DEFAULT_DEVICE_ONLY_MIN_LDEcomment true as originally written.Notes from the review, no action needed here
Codex's posted "mixed host/device state" finding was real when posted and you fixed it 30 minutes later in
f42659dc; that guard is now on both this branch andmain.Preprocessed tables genuinely do enter the device-only envelope now — BITWISE is 2^20 rows, so its 2^21 LDE clears the unchanged 2^19 floor, and it passes every
device_only_forprecondition. That is the feature, and it adds one or two large tables per epoch to the population with no host fallback at R3/R4 while #927 is open. Worth a release note so operators know the new threshold knob exists.The ABBA benchmark was taken 11 days and 6 commits ago against a pre-#914
main. The intervening commits touch only recovery paths, an env read and a test, so the −15.5% still looks representative, but it is not a measurement of the current head.At default thresholds no CI job exercises the preprocessed device-only happy path — the new forced-downgrade test covers the complement, and
gpu-testsis merge-queue only. That configuration first runs on real bench or production proves.