[DRAFT] refactor(verifier): collapse to a single archived proof representation#824
Draft
MauroToscano wants to merge 19 commits into
Draft
[DRAFT] refactor(verifier): collapse to a single archived proof representation#824MauroToscano wants to merge 19 commits into
MauroToscano wants to merge 19 commits into
Conversation
crypto/stark: StarkProof, MultiProof, FriDecommitment, BusPublicInputs, Table, PolynomialOpenings, and DeepPolynomialOpening dual-derive rkyv alongside serde. The verifier operates over a StarkProofView (owned or an rkyv-archived buffer read in place); multi_verify and multi_verify_archived both build the matching view and share one verification implementation, so neither the owned nor the archived path pays a serialization cost. prover: the recursion guest verifies its inner proof straight from the mmapped private-input region via a 12-byte aligning magic/version prefix, with no deserialization pass. Continuation and CLI verification go through the same proof-view path. executor/syscalls: get_private_input_slice() borrows the private input in place instead of copying it into a Vec. bin/cli: proof file persistence uses rkyv instead of bincode. tooling/ethrex-tests: ethrex's guest tests move into their own detached workspace, since ethrex pulls in rkyv's "unaligned" feature which cannot coexist with the root workspace's "aligned" feature in one resolved dependency graph.
CI failed to compile lambda-vm-prover under the disk-spill feature since these tests were never updated during the bincode->rkyv proof migration.
…align CLI proof reads verify_recursion_blob trusted access_unchecked on a prover-supplied archive (real UB on any non-guest target); switch to checked rkyv::access, which measured at ~0.26% of guest cycles on the multiquery profile — not worth the risk for that saving. multi_verify_views only rejected inconsistent/empty OOD dimensions, not a height that isn't a multiple of the AIR's step_size, letting a malformed archive panic in into_frame instead of failing verification cleanly. The CLI verify commands read proof files into a plain Vec<u8> before handing them to rkyv, which requires alignment the allocator only happens to provide; read directly into an AlignedVec instead.
read_aligned_file created a &mut [u8] over uninitialized memory before the OS write, which is UB regardless of pread's actual behavior. Zero the buffer via resize instead of forming the reference unsafely.
as_native/slice_as_native reinterpreted an archived base type as its native form based only on F::BaseType: Archive, which arbitrary IsField impls can satisfy without matching size/align/layout. Restrict the cast to a sealed NativeArchived trait implemented for u32, u64, and types built from them (FieldElement<F>, [T; N]), and propagate the bound through the stark crate's zero-copy proof views.
After the rkyv migration nothing under prover/ references bincode; the dev-dependency was dead. Other crates that still use bincode keep their own declarations.
The continuation bundle derives rkyv (not serde) and round-trips through rkyv; bin/cli's VM-proof format is now rkyv, so examples_cli no longer mirrors it. Correct both statements.
rkyv is the authoritative wire format; the serde derives survive solely for examples/examples_cli.rs and the serde_cbor round-trip tests. Record that so nobody adds a production serde dependency on these types.
Both functions had byte-identical volatile-length + from_raw_parts logic. Delegate the owned-Vec path to the borrowing one so the memory layout and its single unsafe block live in exactly one place.
multi_verify only wraps owned proofs into StarkProofView::Owned and delegates to multi_verify_views. Call multi_verify_views directly with a borrowed Owned view instead of deep-cloning the proof into a throwaway single-element MultiProof.
… into_frame Give owned Table a dimensions_consistent method (reading length through row_major_data() so it stays correct under disk-spill) and have both StarkTableView arms delegate to it. Collapse the two verbatim into_frame copies into a single StarkTableView::into_frame written over the uniform get_row/height accessors, deleting the duplicated owned and archived bodies. No behavior change.
The ethrex tests were relocated to the detached tooling/ethrex-tests workspace, but no CI job or Makefile target ran it, and the old `cargo test -p executor test_ethrex` step matched zero tests and passed vacuously — so the ethrex guest/host rkyv ProgramInput cross-check ran nowhere and the detached crate wasn't even compile-checked. Replace the vacuous step with one that runs the detached workspace (`cd tooling/ethrex-tests && cargo test --release -- --include-ignored`, using its isolated Cargo.lock so the rkyv unaligned/aligned feature conflict stays contained), and add a matching `make test-ethrex` target.
The verifier reads proof data only through the StarkProofView family, but nothing links a struct field to a view accessor: adding a field to StarkProof (or PolynomialOpenings / DeepPolynomialOpening / FriDecommitment) compiles with no accessor, and the verifier silently ignores it — a soundness gap. Add never-run functions that exhaustively destructure each backing struct without `..`, so a newly-added field becomes a compile error (E0027) pointing at the guard, forcing the author to wire an accessor. Enforces accessor presence, not arm symmetry (a wrong-but-same-typed field in an arm still needs a behavioral test).
get_private_input_slice read a prover-controlled u32 length prefix and built a slice of that length with no upper bound. Clamp it to 64 MiB (the same cap the host enforces at store time). An honest length is always within bound, so this never changes behavior for real inputs; it only bounds the slice when a malformed/forged prefix claims more. Defense-in-depth only: on 64-bit the u32 length can't overflow the pointer range, and no writable region overlaps the oversized span today, so this is a documented-invariant / robustness guard, not a fix for a reachable bug.
ci: actually run the ethrex host-reference tests
fix(stark): compile-guard proof-view field coverage
harden(syscalls): clamp private-input length to MAX_PRIVATE_INPUT_SIZE
The verifier previously read proofs through five Owned/Archived view enums (~64 match arms) plus ten hand-written Clone/Copy impls, so it could run over either an owned StarkProof or an rkyv-archived buffer. Collapse this to one representation: the rkyv archive. - Delete the view enums entirely. Each accessor becomes an inherent method on the corresponding Archived type (ArchivedStarkProof / ArchivedTable / ArchivedFriDecommitment / ArchivedDeepPolynomialOpening / ArchivedPolynomialOpenings). The verifier now operates on &ArchivedStarkProof and its nested Archived* refs; a &Archived_ is Copy for free, so all ten Clone/Copy blocks are gone. view.rs drops from 471 to 168 lines. - Owned public API is preserved via serialize->access shims: Verifier::verify and Verifier::multi_verify (and the prover-level verify / verify_with_options / verify_prepared / continuation::verify_continuation) keep their owned signatures but rkyv::to_bytes the proof into an aligned buffer, rkyv::access it (bytecheck), and run the archived verifier. No test/bench/example changed. - CLI verify is now zero-copy: the proof file is already read into a 16-aligned buffer, so replace the full rkyv::from_bytes::<VmProof> deserialize with rkyv::access + a new prover::verify_archived_with_options entry point that materializes only the tiny metadata and reads the STARK proof in place. - The recursion guest feeds the already-accessed archived sub-proofs straight into the verifier, dropping the StarkProofView::Archived wrapper Vec (one small allocation removed; still zero-copy, no proof-sized copy/deserialize). - multi_verify_archived is now the single verification core (formerly multi_verify_views); the old thin multi_verify_archived wrapper is removed. No verification logic changed: only how proof fields are accessed (view accessor -> archived accessor / direct field). Every soundness check, transcript absorb, and Merkle/FRI/DEEP computation is byte-identical, and the guest path stays byte-identical and zero-copy.
MauroToscano
force-pushed
the
perf/rkyv-archived-only
branch
from
15e2dbc to
d906803
Compare
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
2 participants
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.
Draft / design exploration — not for merge as-is. This shows what PR #769 looks like if the
Owned/Archivedview duality collapses to a single archived representation, per the design review. Opening it so the actual diff and the tradeoff are concrete.What changes
Today
view.rs(471 lines) carries 5Owned/Archivedview enums, ~33 accessors × 2 match arms (64 arms), and 10 hand-writtenClone/Copyimpls (to dodge spuriousF: Clonebounds). The verifier is written once over these views, reached by an owned wrapper (multi_verify) and an archived wrapper (multi_verify_archived).This deletes the duality:
Archived*type, with a body that is the verbatim oldArchivedmatch arm (e.g.query(i)→&self.query_list.as_slice()[i]). The verifier body is unchanged exceptStarkProofView<'_,…>→&ArchivedStarkProof<…>and dropping three now-pointlesslet proof = *proof;copies.Verifier::verify/multi_verify/ proververify_preparedrkyv::to_bytesthe proof into an aligned buffer,rkyv::access(bytecheck) it, and run the archived verifier. So no test/bench/example source changes.verifybecomes zero-copy: the proof file is already read into a 16-aligned buffer; the fullrkyv::from_bytes::<VmProof>is replaced withrkyv::access+ a newverify_archived_with_optionsthat reads the STARK proof in place (materializing only the tiny metadata). A real speedup on the host verify path.Vec<StarkProofView::Archived>wrapper); the big proof stays in the mapped buffer.Numbers
view.rs: 471 → 168 lines (−303). Overall net −208 (257 ins / 465 del) across 5 files.cargo checkclean on stark (default +disk-spill --tests), crypto, prover, cli.cargo test -p stark --lib→ 190 passed (including the soundness / tamper / serialized-roundtrip equivalence tests). Fullmake lintclean.Guarantees
Tradeoffs (why it's a draft, for discussion)
prove_and_verify, tests, benches, per-epochverify_continuation) now pay a one-shot hostto_bytes+access. All host-only, one-shot, never in a Merkle/FRI loop — but it does undo the PR's "neither path serializes" goal for those callers. Verify-benchmarks that time an in-memory owned proof would fold a serialize into the timed region unless pre-serialized.verify/multi_verifytrait methods (for<'a> Serialize<…>/Portable + CheckBytes<…>), satisfied at all current call sites but a wider signature.cmd_verify_continuationwas left onfrom_bytes(making it zero-copy needs a separate archivedContinuationProofentry point — out of scope here).If adopted, the review fixes in #815 (OOD width guard, concat allocs) and #821 (view-coverage guard) would rebase on top; note #821's field-coverage guard would move from the deleted view enums onto the archived accessors.