Skip to content

fix(gpu): review follow-ups on the round-4 residency PR - #937

Merged
ColoCarletti merged 1 commit into
gpu-opt-round4-residencyfrom
review/pr888-followups
Aug 18, 2026
Merged

fix(gpu): review follow-ups on the round-4 residency PR#937
ColoCarletti merged 1 commit into
gpu-opt-round4-residencyfrom
review/pr888-followups

Conversation

@MauroToscano

Copy link
Copy Markdown
Contributor

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 new gpu_force_downgrade recipe 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_LDE promised 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 on host_trace_empty() and take their host arm. Not true for the R4 Merkle-proof gather: try_expand_leaf_and_tree_row_major_keep makes the host tree root-only for every GPU-committed table, independent of retain_host_lde, so a declined gather_proofs_dev has nothing to fall back to and aborts regardless of the host LDE. Since tree residency is gated on gpu_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_only assert's rationale said the host walk "would emit an empty path for position 0 instead of failing". get_proof_by_pos returns None on 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 on main.)

gather_proofs_dev's doc said callers fall back to the host tree on None. 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_THRESHOLD now 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 matched ResidentMainTrace, 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.md is the toolkit's reference and was missing all four new env vars and h2d_histo.py. Added, including a note that LAMBDA_VM_GPU_DEVICE_ONLY_THRESHOLD is the finer instrument for shedding device-only tables — better than reaching for LAMBDA_VM_DISABLE_DEVICE_ONLY=1, which is what the #927 workaround currently uses.

bary_num_chunks gets 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 in h2d_histo.py.

make lint passes 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_MB defaults 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) and BLOCK_DIM (Rust) are still coupled by hand, and #888 adds two more kernels that depend on them matching. The BARY_MAX_K single-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_LDE comment 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 and main.

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_for precondition. 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-tests is merge-queue only. That configuration first runs on real bench or production proves.

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
ColoCarletti merged commit 3c38bdf into gpu-opt-round4-residency Aug 18, 2026
16 checks passed
@ColoCarletti
ColoCarletti deleted the review/pr888-followups branch August 18, 2026 20:02
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