perf(prover): verify continuation proofs in place via rkyv#845
Conversation
|
/ai-review |
Codex Code Review
|
Review:
|
a227d09 to
9a1239c
Compare
9a1239c to
9e9f790
Compare
Adapts continuation verification to the same in-place rkyv pattern verify_recursion_blob already uses for the monolithic path (#769): verify_epoch/verify_global take a StarkProofView slice (owned or archived) instead of an owned MultiProof/EpochProof, so the new guest entry point (verify_continuation_and_attest_blob, via verify_continuation_archived) reads every per-epoch/global STARK proof straight out of the archive. Only small per-epoch metadata (table counts, reg_fini, l2g_root, public output) is materialized - the (large) per-epoch/global proof data is never copied into an owned MultiProof just to verify it. Adds the continuation guest's wire format (ContinuationGuestInput, encode_continuation_guest_input) mirroring GuestInput's magic-prefixed rkyv layout, and verify_continuation_and_attest[_blob] mirroring verify_and_attest_blob's program_id fold. replay_transcript_phase_a/compute_expected_commit_bus_balance (owned) lose their last production caller to this refactor; deleted rather than kept as compatibility wrappers now that every caller (production and test) goes through the _view variants already introduced by #769.
Adds a negative test for the archived continuation path: encode a bundle with a tampered epoch l2g_root, assert the blob verify rejects it. Drops verify_continuation_and_attest_blob's now-unused owned-bundle predecessor and renames the surviving function to verify_continuation_and_attest, since the _blob suffix only existed to disambiguate it from that owned variant. Also fixes a stale intra-doc link left over from the deleted compute_expected_commit_bus_balance.
…wed views verify_epoch/verify_global/verify_proof_parts had ballooned into long parameter lists (proof_views, table_counts, runtime_page_ranges, reg_fini, l2g_root, public_output, ...) built ad hoc at every call site via `.iter().map(StarkProofView::Owned/Archived).collect()`, a leftover from keeping owned and archived verify paths side by side. Adds MultiProofView (crypto/stark) mirroring StarkProofView, and EpochProofView/ContinuationProofView (prover) mirroring it one level up, so callers pass one view instead of exploding a bundle into loose fields. multi_verify_views, compute_expected_commit_bus_balance_view, and replay_transcript_phase_a_view are now generic over a ProofViewSource trait (impl'd for slices, Vecs, and MultiProofView) and iterate in place - no Vec materialization anywhere in the path. Collapses verify_continuation_with_roots/verify_continuation_archived (two ~130-line near-duplicates) into one verify_continuation_view, and unifies verify_l2g_commitment_binding/_views into one view-based fn.
…not per element MultiProofView::iter() built each StarkProofView via get(i): a match on Owned/Archived plus a bounds-checked index, redone for every element on every one of multi_verify_views' several passes over the same proof set, even though a given MultiProofView is homogeneously one variant for its whole lifetime. MultiProofViewIter now matches once, in iter() itself, and then drives a plain slice::Iter for whichever representation was chosen - no per-step bounds check, no re-matching against the source enum.
…ration, not per element" This reverts commit f0cd8abb8d5b05e656072d462efae5f80e92c9a4.
Closes the ~0.04% guest-cycle regression the MultiProofView refactor introduced (measured via scripts/bench_recursion_scaling.sh, blowup4/txs=4): len/get/last/iter and the ProofViewSource impls' view_len/view_iter cross a crate boundary (stark -> prover) and apparently weren't getting inlined into multi_verify_views' hot loops. An alternative fix - matching Owned/Archived once per MultiProofView instead of once per element, via a hand-rolled slice::Iter-backed enum iterator - was tried and measured WORSE (+2.3% cycles): it defeats LLVM's specialized Range<usize>::map optimization the closure-based iter() benefits from (see the previous two commits). #[inline(always)] alone, keeping the closure-based iter(), measured at parity with (very slightly better than) the pre-refactor baseline.
…inuation verify - Add tests that splice a different run's global proof onto valid epochs (same shape, different L2G data) so verify_l2g_commitment_binding_view's own reject branch actually executes, both for the owned and archived verify paths. The existing tampered-root tests were caught earlier by verify_epoch's per-epoch root check and never reached this binding. - Thread entry_point out of verify_continuation_archived so verify_continuation_and_attest can fold program_id via program_id_from_digest instead of re-parsing the ELF with program_id_from_elf. - Extract access_recursion_archive, sharing the aligned-fallback + rkyv::access boilerplate between verify_recursion_blob and verify_continuation_and_attest.
9e9f790 to
995f971
Compare
|
/ai-review |
Codex Code Review
|
|
Review — perf(prover): verify continuation proofs in place via rkyv Reviewed the diff focused on safety/soundness, bugs, performance, and readability. This is a clean, well-scoped refactor that extends the existing owned/archived StarkProofView pattern up to the MultiProof level and to continuation verification. Verification semantics look preserved throughout:
Findings: Low (readability / doc accuracy) — verify_continuation_and_attest reimplements the prefix + alignment + rkyv::access logic that the PR own new access_recursion_archive helper exists to share; the helper doc even names this function as a caller, but it is not one. See inline comment — use the helper or fix its doc. Nothing blocking. Nice work. |
|
New feedback will be addressed in a follow-up unless blocking. |
AI ReviewPR #845 · 9 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: MultiProofView::get panics on out-of-bounds index
Claim The public get() method uses as_slice()[i] which panics if i >= len(). While internal callers (iter, last) check bounds first, the method is part of the public API and external callers could trigger a panic. Evidence Line 470: Suggested fix Either return Option<StarkProofView> (like Vec::get) or add a debug_assert/panic with a clear message. Since this is a view type for internal use, adding AI-003: verify_epoch return type change propagates deserialization errors as Err
Claim verify_epoch now returns Result<bool, Error> instead of bool. Deserialization errors from archived proofs now propagate as Err rather than returning false. This is a behavior change for callers. Evidence Function signature changed from Suggested fix This appears intentional (better error handling), but document the semantics: Err = malformed bundle (deserialization failed), Ok(false) = well-formed proof that failed verification, Ok(true) = success. Ensure all callers handle the three cases correctly. AI-004: verify_continuation_view returns entry_point that only the archived caller uses
Claim
Evidence continuation.rs:1189 discards the entry_point in the owned wrapper. continuation.rs:1331 returns Suggested fix Keep as-is unless AI-005: verify_continuation_view materializes each epoch's reg_fini twice (verify_epoch + the loop)
Claim Per epoch, Evidence continuation.rs:762 ( Suggested fix Have AI-006: access_recursion_archive doc claims it is shared with verify_continuation_and_attest, but that caller duplicates the logic
Claim The doc comment on Evidence The PR adds access_recursion_archive (pub(crate)) with that doc, and verify_recursion_blob calls it. But verify_continuation_and_attest (recursion.rs lines 283-305) re-implements Suggested fix Either (a) drop the "Shared by ... verify_continuation_and_attest" line from the helper's doc, or (b) call the helper from AI-009: Test helpers still allocate the Vec<StarkProofView> boilerplate MultiProofView was meant to eliminate
Claim The PR introduces MultiProofView::Owned(&multi_proof.proof) specifically to replace the Evidence prove_elfs_tests.rs shows Suggested fix Replace the Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Discarded candidates (1) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
* perf(prover): verify continuation proofs in place via rkyv Adapts continuation verification to the same in-place rkyv pattern verify_recursion_blob already uses for the monolithic path (#769): verify_epoch/verify_global take a StarkProofView slice (owned or archived) instead of an owned MultiProof/EpochProof, so the new guest entry point (verify_continuation_and_attest_blob, via verify_continuation_archived) reads every per-epoch/global STARK proof straight out of the archive. Only small per-epoch metadata (table counts, reg_fini, l2g_root, public output) is materialized - the (large) per-epoch/global proof data is never copied into an owned MultiProof just to verify it. Adds the continuation guest's wire format (ContinuationGuestInput, encode_continuation_guest_input) mirroring GuestInput's magic-prefixed rkyv layout, and verify_continuation_and_attest[_blob] mirroring verify_and_attest_blob's program_id fold. replay_transcript_phase_a/compute_expected_commit_bus_balance (owned) lose their last production caller to this refactor; deleted rather than kept as compatibility wrappers now that every caller (production and test) goes through the _view variants already introduced by #769. * test(prover): cover continuation blob tamper path, drop dead API Adds a negative test for the archived continuation path: encode a bundle with a tampered epoch l2g_root, assert the blob verify rejects it. Drops verify_continuation_and_attest_blob's now-unused owned-bundle predecessor and renames the surviving function to verify_continuation_and_attest, since the _blob suffix only existed to disambiguate it from that owned variant. Also fixes a stale intra-doc link left over from the deleted compute_expected_commit_bus_balance. * fmt * refactor(prover,stark): replace MultiProof field-explosion with borrowed views verify_epoch/verify_global/verify_proof_parts had ballooned into long parameter lists (proof_views, table_counts, runtime_page_ranges, reg_fini, l2g_root, public_output, ...) built ad hoc at every call site via `.iter().map(StarkProofView::Owned/Archived).collect()`, a leftover from keeping owned and archived verify paths side by side. Adds MultiProofView (crypto/stark) mirroring StarkProofView, and EpochProofView/ContinuationProofView (prover) mirroring it one level up, so callers pass one view instead of exploding a bundle into loose fields. multi_verify_views, compute_expected_commit_bus_balance_view, and replay_transcript_phase_a_view are now generic over a ProofViewSource trait (impl'd for slices, Vecs, and MultiProofView) and iterate in place - no Vec materialization anywhere in the path. Collapses verify_continuation_with_roots/verify_continuation_archived (two ~130-line near-duplicates) into one verify_continuation_view, and unifies verify_l2g_commitment_binding/_views into one view-based fn. * perf(stark): match Owned/Archived once per MultiProofView iteration, not per element MultiProofView::iter() built each StarkProofView via get(i): a match on Owned/Archived plus a bounds-checked index, redone for every element on every one of multi_verify_views' several passes over the same proof set, even though a given MultiProofView is homogeneously one variant for its whole lifetime. MultiProofViewIter now matches once, in iter() itself, and then drives a plain slice::Iter for whichever representation was chosen - no per-step bounds check, no re-matching against the source enum. * Revert "perf(stark): match Owned/Archived once per MultiProofView iteration, not per element" This reverts commit f0cd8abb8d5b05e656072d462efae5f80e92c9a4. * perf(stark): force-inline MultiProofView's per-element accessors Closes the ~0.04% guest-cycle regression the MultiProofView refactor introduced (measured via scripts/bench_recursion_scaling.sh, blowup4/txs=4): len/get/last/iter and the ProofViewSource impls' view_len/view_iter cross a crate boundary (stark -> prover) and apparently weren't getting inlined into multi_verify_views' hot loops. An alternative fix - matching Owned/Archived once per MultiProofView instead of once per element, via a hand-rolled slice::Iter-backed enum iterator - was tried and measured WORSE (+2.3% cycles): it defeats LLVM's specialized Range<usize>::map optimization the closure-based iter() benefits from (see the previous two commits). #[inline(always)] alone, keeping the closure-based iter(), measured at parity with (very slightly better than) the pre-refactor baseline. * fix(prover): cover L2G binding rejection, dedupe ELF re-parse in continuation verify - Add tests that splice a different run's global proof onto valid epochs (same shape, different L2G data) so verify_l2g_commitment_binding_view's own reject branch actually executes, both for the owned and archived verify paths. The existing tampered-root tests were caught earlier by verify_epoch's per-epoch root check and never reached this binding. - Thread entry_point out of verify_continuation_archived so verify_continuation_and_attest can fold program_id via program_id_from_digest instead of re-parsing the ELF with program_id_from_elf. - Extract access_recursion_archive, sharing the aligned-fallback + rkyv::access boilerplate between verify_recursion_blob and verify_continuation_and_attest. * fmt
Summary
Stacked on #844. Adapts continuation verification to the same in-place rkyv pattern
verify_recursion_blobalready uses for the monolithic path (#769):verify_epoch/verify_globaltake aStarkProofViewslice (owned or archived) instead of an ownedMultiProof/EpochProof, so the new guest entry point (verify_continuation_and_attest_blob, viaverify_continuation_archived) reads every per-epoch/global STARK proof straight out of the archive. Only small per-epoch metadata (table counts, reg_fini, l2g_root, public output) is materialized — the (large) per-epoch/global proof data is never copied into an ownedMultiProofjust to verify it.ContinuationGuestInput,encode_continuation_guest_input) mirroringGuestInput's magic-prefixed rkyv layout, andverify_continuation_and_attest[_blob]mirroringverify_and_attest_blob'sprogram_idfold.replay_transcript_phase_a/compute_expected_commit_bus_balance(owned) lose their last production caller to this refactor; deleted rather than kept as compatibility wrappers now that every caller (production and test) goes through the_viewvariants perf(verifier): verify STARK proofs in place via rkyv #769 already introduced.Test plan
cargo test -p lambda-vm-prover --lib(516 passed)test_recursion_continuation_blob_decodes_and_verifies_on_host— encode/decode/verify/attest contract for the continuation guest blobcargo clippy --all-targets -- -D warnings/cargo fmt --check