feat: rvr support for rv64 deferral extention#2913
Conversation
|
@claude provide review of this PR. make sure that all deferral related constants are took into account and updated accordingly, make sure that changes that were done are correct |
|
Claude finished @mansur20478's task in 5m 2s —— View job PR Review: RVR support for RV64 deferral extension
SummaryThis PR fixes the deferral extension for the RV64 migration by correcting Correctness:
|
| Constant | Formula | Value | Status |
|---|---|---|---|
DEFERRAL_COMMIT_NUM_BYTES |
8 × 4 |
32 | ✅ Fixed |
DEFERRAL_OUTPUT_KEY_BYTES |
32 + 8 |
40 | ✅ Correct |
COMMIT_WORDS |
32 / 8 |
4 (u64s) | ✅ Correct |
OUTPUT_KEY_WORDS |
40 / 8 |
5 (u64s) | ✅ Correct |
DIGEST_MEMORY_OPS |
8 / 8 |
1 |
Correctness: u32 → u64 migration in C
All changes are consistent:
commit_words,key_words,row_wordsarrays correctly changed fromuint32_t[]touint64_t[]- All
rd_mem_u32_range_traced/wr_mem_u32_range_traced/trace_mem_access_u32_rangecalls correctly changed to their_u64variants - The
output_lenextraction simplification (key_words[COMMIT_WORDS]instead of manual two-u32 assembly) is correct — the u64 at index 4 is exactly the 8 bytes at byte offset 32, matching the output key layout (commit ++ u64 length)
Correctness: CI change
Removing the TODO comment and uncommenting the deferral RVR test entry is appropriate — this PR lands the support it was waiting for.
Pre-existing issue worth noting
DIGEST_MEMORY_OPS = DEFERRAL_DIGEST_SIZE / WORD_SIZE = 1 — This formula mixes units (field elements ÷ bytes). It produces the correct page-tracking behavior only because pages are large (512 elements at PAGE_BITS=6, PTRS_PER_LEAF=8) and one digest (8 elements starting at a 16-aligned address) always fits within a single page. However, the formula is semantically fragile. If page geometry ever tightened, this would under-report touched pages.
This is not introduced by this PR (it was 8/4=2 in RV32, now 8/8=1 in RV64 — same formula, different numeric result), and there is already a TODO on the deferral address space semantics at rvr_ext_deferral.c:58-60. Just flagging it for awareness.
u32s_as_u8s in rvr-openvm-ffi-common/src/lib.rs:173 uses WORD_SIZE (= 8) instead of size_of::<u32>() (= 4) to compute the output slice length, which would produce a slice 2× too long. This is not part of this PR's diff — pre-existing issue.
Verdict
LGTM. The changes are correct and fix a real bug (wrong commit byte count causing buffer overflows in FFI callbacks). The u32→u64 migration is clean and consistent. No issues introduced by this PR.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights. Commit: 5c137cf |
rvr support for deferral extension. The PR required changes in how the rvr deferral extension reads and write to the memory, from 4 byte granularity to 8 byte. This also meant that for `output length`, the u64 LE stored right after the commit, formula got changed to accommodate 8 byte granularity. Also, `DEFERRAL_COMMIT_NUM_BYTES` had to be updated to `DEFERRAL_DIGEST_SIZE * F_NUM_BYTE` from `DEFERRAL_DIGEST_SIZE * WORD_SIZE`. This is because `DEFERRAL_DIGEST_SIZE` is digest size in field elements, not in `WORDS`. resolves INT-8113
rvr support for deferral extension. The PR required changes in how the rvr deferral extension reads and write to the memory, from 4 byte granularity to 8 byte. This also meant that for `output length`, the u64 LE stored right after the commit, formula got changed to accommodate 8 byte granularity. Also, `DEFERRAL_COMMIT_NUM_BYTES` had to be updated to `DEFERRAL_DIGEST_SIZE * F_NUM_BYTE` from `DEFERRAL_DIGEST_SIZE * WORD_SIZE`. This is because `DEFERRAL_DIGEST_SIZE` is digest size in field elements, not in `WORDS`. resolves INT-8113
rvr support for deferral extension.
The PR required changes in how the rvr deferral extension reads and write to the memory, from 4 byte granularity to 8 byte. This also meant that for
output length, the u64 LE stored right after the commit, formula got changed to accommodate 8 byte granularity.Also,
DEFERRAL_COMMIT_NUM_BYTEShad to be updated toDEFERRAL_DIGEST_SIZE * F_NUM_BYTEfromDEFERRAL_DIGEST_SIZE * WORD_SIZE. This is becauseDEFERRAL_DIGEST_SIZEis digest size in field elements, not inWORDS.resolves INT-8113