Skip to content

fix(prover): deterministic blob layout tag + continuation tamper coverage - #851

Open
diegokingston wants to merge 3 commits into
mainfrom
fix/continuation-rkyv-review
Open

fix(prover): deterministic blob layout tag + continuation tamper coverage#851
diegokingston wants to merge 3 commits into
mainfrom
fix/continuation-rkyv-review

Conversation

@diegokingston

Copy link
Copy Markdown
Collaborator

Summary

Stacked on #845 — fixes for the findings from its review:

  1. Deterministic layout discrimination. The first reserved prefix byte is now a layout kind (RECURSION_INPUT_KIND_MONOLITHIC = 0, RECURSION_INPUT_KIND_CONTINUATION = 1). Both blob verifiers check it at the prefix stage via the new recursion_archive_bytes_for_kind, so a blob of the wrong layout is rejected deterministically instead of relying on the rkyv bytecheck happening to fail on a differently-shaped archive. Backwards compatible: blobs encoded before this change have zeros there, which is exactly KIND_MONOLITHIC.
  2. Tamper coverage for the continuation blob path.
    • test_recursion_continuation_rejects_corrupted_commitment — the continuation analog of the monolithic corrupted-commitment guard: honest bundle + corrupted DECODE root → Ok(None).
    • test_recursion_blob_kind_byte_discriminates_layouts — continuation blob rejected by the monolithic verifier and vice versa at the prefix stage, plus kind-byte tamper rejection.
  3. KEEP IN SYNC notes between verify_continuation_with_roots and verify_continuation_archived (the two must apply the same validation in the same order), and a doc note that owned verify_continuation_and_attest is the host-side symmetric counterpart of the _blob entry point.
  4. Stale doc: docs/continuations_design.md now references compute_expected_commit_bus_balance_view (the owned variant was deleted in perf(prover): verify continuation proofs in place via rkyv #845).

Test plan

  • cargo test -p lambda-vm-prover --lib recursion_smoke — 5 passed, 0 failed (includes both new tests; pre-existing monolithic + continuation blob tests unaffected)
  • cargo clippy -p lambda-vm-prover --all-targets -- -D warnings — clean
  • rustfmt clean

Oppen and others added 3 commits July 16, 2026 18:09
…e input for continuation verify

Mirrors #782's monolithic mechanism for the continuation path: a caller
(the recursion guest) can supply the DECODE preprocessed root and each
touched data page's genesis root instead of the verifier recomputing
them from the ELF, skipping the in-VM FFT + Merkle build. Supplied
roots are used verbatim; the binding shifts to the consumer's
recompute-and-compare of the folded identity, exactly like the
monolithic path.

verify_global's genesis-page classification (ELF-backed vs zero-init)
is derived from ELF segment address ranges instead of materializing a
full byte-level image when roots are supplied, since the real bytes are
never read once a root covers that page. Page lookups are keyed by
page number (page_base >> log2(page_size)) rather than the raw
page-aligned address, since the low bits are always zero.

verify_continuation keeps its existing signature (trustless recompute);
verify_continuation_with_roots is the new supplied-roots entry point,
and continuation_precomputed_commitments lets a caller derive the roots
to supply for a given bundle.
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.
…ion tamper coverage

Stacked on #845, addressing review findings:

- Reuse the first reserved prefix byte as a layout kind
  (RECURSION_INPUT_KIND_MONOLITHIC=0, KIND_CONTINUATION=1). Both blob
  verifiers now check it at the prefix stage via the new
  recursion_archive_bytes_for_kind, so a blob of the wrong layout is
  rejected deterministically instead of relying on the rkyv bytecheck
  happening to fail on a differently-shaped archive. Backwards
  compatible: pre-kind blobs have zeros there (= KIND_MONOLITHIC).
- New tests: test_recursion_continuation_rejects_corrupted_commitment
  (continuation analog of the monolithic corrupted-commitment guard)
  and test_recursion_blob_kind_byte_discriminates_layouts (both
  directions + kind-byte tamper).
- KEEP IN SYNC notes between verify_continuation_with_roots and
  verify_continuation_archived; note that owned
  verify_continuation_and_attest is the host-side symmetric counterpart.
- docs/continuations_design.md: update the deleted
  compute_expected_commit_bus_balance reference to the _view variant.
@Oppen
Oppen force-pushed the perf/continuation-rkyv-zero-copy branch 3 times, most recently from 9e9f790 to 995f971 Compare July 17, 2026 22:17
Base automatically changed from perf/continuation-rkyv-zero-copy to main July 17, 2026 23:07

@MauroToscano MauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI-generated review. Produced by an automated agent and posted from this account — not written by hand. Findings below are leads to verify, not confirmed conclusions.

This PR adds a kind byte to the recursion input prefix to discriminate archive layouts, plus two smoke tests and a docs one-liner. The new work is sound — deterministic rejection at the prefix stage instead of relying on bytecheck to happen to trip on a differently-shaped archive, and zeros in old blobs decoding as KIND_MONOLITHIC keeps it backwards compatible. The blocker is packaging.

The branch's merge base is 18f3b8f, before #843#846 landed, so the diff re-states the pre-review snapshots of #844/#845 (~700 of the 843 added lines) around the ~160 lines of genuinely new work in 221e78c. GitHub reports it CONFLICTING/DIRTY with conflicts in all five prover files, and those conflicts fall exactly on hunks where merged main is already the better version. It cannot land as-is. Rebase and re-cut to 221e78c's content only — the RECURSION_INPUT_KIND_* constants, recursion_archive_bytes_for_kind, the two encoder changes, the kind check wired into main's access_recursion_archive/verify_continuation_and_attest, the two recursion_smoke_test.rs tests, and the docs fix — taking main's side on everything below.

prover/src/continuation.rs:802 — a missing supplied page-genesis root is a debug_assert! here; merged main hard-rejects it (return false at continuation.rs:946, under the "Reject explicitly instead" comment). Resolving this hunk head-side compiles the check out of release entirely and falls through to global_memory_air(opts, config, None) on a classify-only config whose init_values is Some(Vec::new()), so the AIR is built against the all-zero-page commitment rather than the real ELF-data genesis. This is defense-in-depth, not a soundness hole — the same supplied list feeds program_id (recursion.rs:353) and the consumer's id != expected_program_id check still rejects — but main's version should survive the rebase. The comment is wrong either way: the fallback isn't "recompute (slow)", it's a full page FFT + Merkle producing the wrong root.

prover/src/tables/page.rs:59page_number and its debug_assert_eq!(page_base % DEFAULT_PAGE_SIZE as u64, 0) were dropped during #844's review; main keys the supplied map by raw page_base to match the monolithic page_commitments lookup and rejects a non-page-aligned page_genesis_commitments entry with an explicit Err. That matters because verify_continuation_and_attest_blob deserializes page_commitments straight out of the untrusted blob and hands them to verify_global, which calls page_number on every entry (continuation.rs:797/816) — and this revision only alignment-checks touched_page_bases (continuation.rs:1273-1279), not the supplied commitments. On this branch a malformed blob therefore panics the verifier in any debug-assertions build instead of returning a rejectable result. Deployment impact is nil (the guest ELF is --release); exposure is dev-profile host builds and the non---release make test-profile-recursion-* targets. Main already fixes all three points — don't revert them. page_number/PAGE_SIZE_LOG2 have no other caller at this head and can be removed from page.rs entirely.

prover/src/continuation.rs:1036, 1176 — the two KEEP IN SYNC notes are the whole continuation.rs contribution of 221e78c, and they are dead on main: #845's review collapsed verify_continuation_with_roots and verify_continuation_archived into a single verify_continuation_view (continuation.rs:1221) over EpochProofView/ContinuationProofView, which also shrank verify_epoch from the 13-parameter form here to a view plus seven. Landing the duplicated pair plus drift-warning comments would resurrect the drift. Drop the notes and re-express the continuation tamper additions against the view API.

prover/src/recursion.rs:284 — this branch still carries the pre-review split: a 5-arg owned verify_continuation_and_attest plus verify_continuation_and_attest_blob at :313. Merged main collapsed those into one blob-taking verify_continuation_and_attest(blob, &ProofOptions) (recursion.rs:306) and deleted the owned variant, and the continuation guest calls that name with two args at bench_vs/lambda/recursion/src/main.rs:97 under #[cfg(feature = "continuation")]. The conflict lands in these two functions, so the rebase must pick a naming — if the resolution keeps _blob, the guest call site needs updating in the same commit. The stated test plan cannot catch this: bench_vs/lambda/recursion declares its own [workspace] and is excluded from the root members, so cargo test/clippy -p lambda-vm-prover never builds it. CI will (make compile-recursion-elfs runs in test-prover, test-prover-comprehensive and seed-elf-cache, and the cache key hashes prover/src/**, so this PR forces a real rebuild of all three recursion-cont-{min,blowup2,blowup4} ELFs), so a bad resolution costs a red round trip rather than a silently broken verifier binary.

prover/src/continuation.rs (tests)test_verify_continuation_with_supplied_roots collides by name with main's version at continuation.rs:1489, which is strictly stronger: main uses the data_page_touch fixture and covers tampered-page-root and all-zero-page-root, where this one is stack-only all_loadstore_32 and tampers DECODE only. Same for recursion_smoke_test.rs, which predates #846's blowup2/blowup4 cycle tests. Keep main's and add the new cases on top.

Nits:

  • Summary item 3 describes the owned verify_continuation_and_attest as the host-side counterpart of _blob; that variant no longer exists on main, so the PR body needs a pass after the rebase.
  • The docs fix still applies post-rebase — docs/continuations_design.md:644 on main still names compute_expected_commit_bus_balance while lib.rs:954 is _view.

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.

3 participants