refactor(verifier-ray): BuildPcsSystem not to take in a runtime - #3752
refactor(verifier-ray): BuildPcsSystem not to take in a runtime#3752Tabaie wants to merge 2 commits into
BuildPcsSystem not to take in a runtime#3752Conversation
Signed-off-by: Arya Tabaie <arya.pourtabatabaie@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors verifier-ray PCS codegen so that BuildPcsSystem no longer depends on a proving/verifying-time wiop.Runtime. Instead, compile-time PCS invariants are derived solely from wiop.System, and proof-specific entry_claims are extracted per-proof via a new helper.
Changes:
- Refactors
BuildPcsSystemto be runtime-independent and moves proof-specific claimed evaluations intoExtractPcsOpening. - Updates fixture generation to bake one
PcsSystemper case while extracting per-proofentry_claimsfor honest/invalid/alt proofs. - Adds utilities/tests around dynamic-module sizing and canonical PCS entry ordering.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| verifier-ray/testdata/generate/pcs_emit.go | Emits PCS opening fixtures from explicit entryClaims instead of reading them from PcsSystem. |
| verifier-ray/testdata/generate/main.go | Bakes one compile-time PCS system per case and extracts per-proof openings via ExtractPcsOpening. |
| verifier-ray/codegen/pcs.go | Refactors PCS system extraction to depend only on wiop.System; introduces shift-slot derivation from sys.LagrangeEvals. |
| verifier-ray/codegen/pcs_zig_test.go | Updates tests to reflect the split between system extraction and proof opening extraction. |
| verifier-ray/codegen/pcs_opening.go | New: per-proof extraction of PCS entry_claims in verifier canonical entry order. |
| verifier-ray/codegen/pcs_opening_test.go | New: unit test for canonical PCS entry ordering logic. |
| verifier-ray/codegen/coin_routing.go | Adds DynamicModuleSizes helper shared by codegen and fixture generation. |
| verifier-ray/codegen/actions.go | Updates action mapping documentation to include ExtractPcsOpening. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func pcsShiftFor(cv *wiop.ColumnView) (isDynamic bool, shift int) { | ||
| if cv.Column.Module.IsDynamic() { | ||
| return true, cv.ShiftingOffset | ||
| } | ||
| size := 1 << loc.SizeID | ||
| size := cv.Column.Module.Size() | ||
| return false, ((cv.ShiftingOffset % size) + size) % size | ||
| } |
| // Static column: size_log2 is the padded, fixed module size. SetSize | ||
| // already rounds it up to a power of two, so log2 is exact. | ||
| if !col.Module.IsSized() { | ||
| return PcsSystem{}, fmt.Errorf("codegen: BuildPcsSystem: static module %q of committed column %q has no size set", | ||
| col.Module.Context.Path(), col.Context.Path()) | ||
| } | ||
| desc.SizeLog2 = bits.Len(uint(col.Module.Size())) - 1 | ||
| } |
| colDeclByID := pcsColumnDeclIndex(sys) | ||
| if len(colDeclByID) != len(pcs.Columns) { | ||
| return nil, fmt.Errorf("codegen: ExtractPcsOpening: sys has %d committed columns, pcs has %d; built from a different sys?", | ||
| len(colDeclByID), len(pcs.Columns)) | ||
| } |
| size := moduleSizes[col.DynamicIndex] | ||
| sizeLog2 := bits.Len(uint(size)) - 1 | ||
| if size <= 0 || 1<<sizeLog2 != size { | ||
| return nil, fmt.Errorf("codegen: ExtractPcsOpening: dynamic column %d proved at non-power-of-two size %d", c, size) | ||
| } |
YaoJGalteland
left a comment
YaoJGalteland
left a comment
There was a problem hiding this comment.
Thanks for removing the runtime from BuildPcsSystem. I just have a few restructuring suggestions to make the full pipeline a bit clearer.
| if len(vanishingSystem.Modules) == 0 { | ||
| return nil | ||
| } | ||
| honestRt := runProver(sys, honest) |
There was a problem hiding this comment.
A small restructure: build all sys-derived compile-time systems (routing, vanishingSystem, logDeriv, pcs) are built first, then each proof's runProver + ExtractPcsOpening + view-extraction happens as one contiguous block (honest, then invalid) — matching addMultiSize's existing shape.
There was a problem hiding this comment.
how about extract the shared runProver → ExtractPcsOpening → extractVanishingProofView block into a new buildProofFixture(sys, pcs, assign, source, name, label) helper (returning a proofFixture{view, entryClaims, openingProof}), and rewrote add and addMultiSize to call it once per proof (honest/invalid/alt) instead of inlining the block three times?
| honestProof := extractVanishingProofView(sys, honestRt) | ||
|
|
||
| tc := fixtureCase{name: name, honest: honestProof} | ||
| pcs, err := codegen.BuildPcsSystem(sys, routing) |
There was a problem hiding this comment.
I think it would be nice to have a single BuildAllSystems function in verifier-ray/codegen/system.go, that builds all the compile-time systems together (coin routing, vanishing, log-derivative, and PCS).
Currently,
BuildPcsSystemrequires a dummy runtime from which to extract column sizes and shift schedules via callingGetLayouton the runtime. This design is conceptually incorrect, as a runtime is by definition a proving/verifying time object. This PR instead extracts that information fromwiop.System.