Miscellaneous fixes#6
Open
jtcoolen wants to merge 72 commits into
Open
Conversation
What: Change the memory lifetime of the four PCS fixed windows (rot_points, x1_powers, q_com, q_eval_set) in VerifierMemoryLayout::new from MemoryLifetime::Phase(MemoryPhase::PcsFixed) to MemoryLifetime::Permanent, and remove the now-unused MemoryPhase::PcsFixed enum variant. Why: These windows are not transient scratch. They are written during PCS preparation and then read across several *later* phases: x1_powers feeds both the rolled q_eval fold (PcsQEvalSourceTable) and the fused final MSM (PcsFinalMsm), while rot_points/q_eval_set feed the f_eval interpolation. Because MemoryLifetime::intersects treats two distinct Phase values as never co-live, tagging these long-lived windows with their own phase made MemoryMap::validate() blind to any overlap between them and the PCS scratch that consumes them. The planner's central non-overlap invariant therefore silently did not cover these windows. They were safe in practice only because their fixed theta-relative addresses (words 52-201) happen to sit below the commitment region while all consuming scratch is allocated above it, a property of the hard-coded address map rather than something the validator guaranteed. How: Nothing ever reuses these byte ranges, so the correct lifetime is Permanent, which forces validate() to reject any future overlap against them. With the four registrations switched over, MemoryPhase::PcsFixed has no remaining users and is deleted. An explanatory comment records the lifetime invariant at the allocation site. No address, size, or rendered output changes; all 28 layout unit tests and the full crate build pass. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: add an inclusive phase-span memory lifetime and tag selector_accumulators as live from QuotientVm through PcsFinalMsm. Why: selector accumulators are written during quotient evaluation and read by later PCS phases, so a single PcsFinalMsm phase let validation miss unsafe overlaps. How: teach MemoryLifetime::intersects about phase spans, apply the span to selector_accumulators, and cover the behavior with memory-layout regression tests.
What: size the accumulator pairing batch region with PAIRING_BATCH_HASH_BYTES. Why: the old expression only matched the hash frame because current G2 and G1 encodings have a coincidental ratio. How: import the named accumulator hash-frame constant and keep the existing memory-region alias pointed at it.
What: include rot_points, x1_powers, q_com, q_eval_set, and q_eval_cptr ends when placing trace_u256_log_word. Why: the trace scratch bound should stay correct if theta-window caps move beyond today's g1_identity guard. How: add each theta-window end to the max-bound list and cover the placement with an oversized-window regression test.
What: reserve constant slots for product-add fused opcodes before emitting the base expression. Why: emitting the base can add enough constants to push the product scalar past the u8 slot checked earlier, causing codegen to panic. How: insert the fused product scalar with const_slot before base emission and add a regression with 256 base constants followed by a fused mem*mem*const add.
What: reserve limb-shape coefficient slots before emitting the extracted residue expression. Why: residue emission can add enough constants to invalidate the earlier u8-slot preflight and panic in emit_limb_shape. How: call const_slot for each recognized limb coefficient before residue emission and add a regression with a LIN7 subshape plus 256 residue constants.
What: add regression coverage that the Solidity generator rejects rotated non-committed instance queries, and clarify the code comments around the single local public-instance evaluation. How: keep the existing constructor-time Rotation::cur guard, document why non-committed instance_eval is a single word, add a debug assertion in quotient lowering, and replace two trace/layout magic literals with named constants. Why: the lowering path only reconstructs the non-committed public-input polynomial at the current rotation, so rotated instance queries must fail at codegen time instead of producing a verifier that disagrees with native Midfall.
Size the pre-first-squeeze transcript run from every advice phase up to and including the first challenge-bearing phase, not just phase 0, so the valid "advice in an early phase, challenge in a later phase" shape no longer under-sizes the buffer and overruns VK_MPTR. In the quotient VM limb-decomposition and product-add peepholes, re-check that fused opcode coefficients still fit their one-byte constant slots after intervening residue/base emission, falling back to generic ops instead of panicking in the u8::try_from(...).expect(...). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The generated Lagrange block writes num_instances + num_neg_lagranges + 1 denominator words in place starting at X_N_MPTR (theta word 26) and batch-inverts them, but the memory planner modeled only the prefix-product scratch half of that call: the input run was never registered as a region or bounded, batch_invert_scratch_bytes sized only the modexp scratch, and validate() capped just the PCS fixed windows. Nothing caps num_instances, so for large enough circuits the run silently overwrote state that is already live at Lagrange time: Q_EVAL_CPTR_MPTR (theta word 201, stored by the proof parser and read by the PCS q_eval fold) at num_instances + num_neg_lagranges >= 175, the virgin-zero G1_IDENTITY_MPTR (word 209, relied on as the committed-instance MSM base) at >= 183, and the decoded proof evaluations (word 220) at >= 194. Codegen succeeded but the deployed verifier rejected every honest proof, a silent memory-safety/completeness break. Extract batch_invert_input_words as the shared source of truth for the run size, record it on VerifierMemoryLayout, and make validate() reject any layout whose run reaches min(Q_EVAL_CPTR_WORD, G1_IDENTITY_WORD, REVERSED_EVALS_WORD), failing codegen with a clear error before any Solidity is rendered. A capacity check is used instead of an arena region because the run intentionally overlays the theta band and PCS fixed windows that are dead at Lagrange time but modeled Permanent, so registering it would produce fake overlaps; the existing benign spill into ROT_POINTS stays allowed. Add a boundary regression test covering the exact pass/fail instance counts. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
g1_to_u256s and g2_to_u256s returned the all-zero EIP-2537 word array whenever coordinates() yielded None. In midnight-curves, coordinates() returns None not only for the point at infinity but for any off-curve point, and the all-zero words are exactly the EIP-2537 encoding of the identity. An off-curve/corrupted point (e.g. an s_g2 from an unchecked deserialization path) was therefore silently baked into the generated verifier as the identity: NEG_S_G2 collapsing to infinity degenerates the e(w, -s*G2) KZG pairing term, and a malformed VK commitment becomes an identity commitment (a different circuit), both with no build error. Distinguish the genuine identity (is_identity()) from an invalid point and assert on the latter, so codegen fails loudly instead of emitting a soundness-broken constant. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
generate_base_vk emits G1Affine::generator() as the VK header G1_BASE while taking g2/s_g2 from the deployer-supplied params, with nothing asserting params.g[0] == G. midnight-proofs' ParamsKZG accepts arbitrary g vectors, so a params file whose SRS base is c*G (c != 1) built without error. The generated verifier then computes the -v*G term of the KZG combination from G while every proof/VK commitment is over c*G, so the on-chain pairing check enforces a different (scaled) statement than the native Rust verifier: honest proofs are rejected, and accept/reject can diverge from the reference verifier, all silently at build time. g[0] is crate-private in midnight-proofs, but the commitment of the constant-1 polynomial equals g[0], and in the Lagrange basis that commitment is sum(g_lagrange). Assert sum(g_lagrange) == generator at codegen time so a mismatched SRS fails the build instead of producing a verifier that silently diverges from the reference verifier. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
record_yul_const_assignment recorded 'let name := literal' bindings for limb7 coefficient recognition but ignored non-let reassignments, because yul_let_assignment returns None for 'x := ...'. A stale literal therefore survived a later runtime reassignment, so a block that binds a variable to a limb7 coefficient literal and then reassigns it to a runtime value could be falsely matched: specialize_limb7_chains would replace the chain with a q_limb7 call baking in the literal while the original code multiplied by the reassigned runtime value, silently miscompiling the quotient identity (no codegen or Yul error). This is unreachable today only because gate-identity lines are SSA, but nothing asserts that and the Evaluator already emits non-let reassignment lines on other paths. Track constants via yul_assignment (let or not) and forget any variable reassigned to a non-constant value, so a stale coefficient can never be matched. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The VK payload embeds the quotient const table and packed bytecode compiled inside generate_vk, but LoweringPlan recompiles the program from scratch and renders the interpreter (program length, opcode gating, native-callback ordering, stack sizing) from that second build. The only cross-checks were length/reservation inequalities; nothing compared the bytes actually embedded in the VK payload with the second build. Parity rested solely on compile determinism over HashMap/HashSet-backed structures, so an ordering-dependent compile could ship a pinned VK whose bytecode disagrees with the rendered interpreter, permanently rejecting every valid proof (fail-closed on-chain DoS) invisibly at codegen time. Compare vk.constants[quotient_const_offset..] and [quotient_program_offset..] word-for-word against the plan build's consts and PackedProgramCodec::encode_words(bytes) in validate_generator_invariants, failing codegen on any divergence. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
validate_quotient_program proved stack effects, instruction lengths, and mem-token bytes, but never checked that decoded const-table indices fall inside the emitted table. An encoder/planner regression emitting an out-of-range const slot (this class already produced one real bug, the quotient VM const-table overflow) would ship a verifier that loads an arbitrary trailing VK word as a gate coefficient, deterministically flipping accept/reject with nothing failing at build or runtime. Add validate_quotient_const_slots, which walks the finalized byte stream and bounds-checks every constant slot across all const-bearing opcodes (push/add/mul const and const_u8, the add-mul const_u8 forms and their runs, LIN7/BILIN7 rows and pairwise coeffs, and the dynamic AFFINE_SUM and MODARITH7 layouts). It runs after validate_quotient_program in QuotientProgramBuilder::finish, whose length validation guarantees the walked layout is in bounds. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: make kzg::memory_requirements and kzg::computations panic instead of
returning a default/empty result when the intermediate-set construction yields
zero point sets.
How: replace the two `if n_sets == 0 { return ... }` early returns with an
assert!(n_sets != 0, ...) carrying the soundness rationale.
Why: with zero point sets, computations() emits no Block 6, so
PAIRING_LHS_MPTR/PAIRING_RHS_MPTR are never written. Zero-initialized EVM
memory is the EIP-2537 encoding of the BLS12-381 point at infinity, so
FinalPairing.yul's ec_pairing computes e(inf, G2) * e(inf, -sG2) = 1 and the
verifier accepts ANY transcript-parseable proof with no cryptographic checking
of the openings. This is unreachable today only because ProtocolPlan::validate
requires the schedule to end with the Linearization query (n_sets >= 1); the
KZG emitter is the last line of defense for the pairing check and must fail
closed rather than silently emit an accept-all verifier.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: assert the rotation-point walk in kzg::computations does not unroll more than MAX_ROTATION_WALK_STEPS (4096) mulmod steps. How: compute the walk length as |max_rot| + |min_rot| and assert it against a generous cap before emitting the forward/backward omega walks. Why: the block emits one `mulmod` line per unit step across the whole rotation span, so the emitted Yul grows with rotation magnitude, not with the number of distinct rotations (which is separately capped at ROT_POINTS_CAP_WORDS=28). A legitimate circuit using a large rotation (e.g. Rotation(50_000)) would pass every existing capacity check yet emit enough code to exceed the EIP-170 24KB runtime-size limit, producing an undeployable verifier with no diagnostic. The cap fails closed with a clear message well above any realistic circuit's rotation range; the documented remedy is to roll the walk into a Yul loop. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: rewrite the misleading soundness note above the Montgomery batch inverse in kzg::computations to state the real lbasis_j != 0 invariant and the actual failure mode. How: comment-only. Document that lbasis_j is non-zero only because distinct rotation *values* map to distinct rotation *points* (which holds because the domain order n=2^k exceeds the rotation span for every supported circuit), and that a degenerate tiny-domain circuit aliasing two rotations fails closed (a zero lbasis product makes scalar_inv revert) rather than producing a wrong f_eval. Add a TODO to thread the domain order in for a codegen-time assert. Why: the previous comment claimed lbasis_j is non-zero simply because construct_intermediate_sets de-duplicates rotations, but that de-dups i32 rotation values, not the points x*omega^rot; two distinct rotations alias when rot_i == rot_j (mod n). The comment also contradicted a sibling finding on scalar_inv: the batch path reverts on a zero product, so the true behavior is fail-closed, not a silent wrong evaluation. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: correct the comment claiming PAIRING_LHS = pi is 'paired against G2_BASE' in kzg::computations Block 6. How: comment-only. pi (PAIRING_LHS) is actually paired against NEG_S_G2_BASE: FinalPairing.yul calls ec_pairing(success, PAIRING_RHS_MPTR, PAIRING_LHS_MPTR) with the slots swapped, and ec_pairing pairs arg0 against G2_BASE and arg1 against NEG_S_G2_BASE. The comment now states this and ties it to the KZG identity e(final_com - v*G + x3*pi, [1]_2) = e(pi, [s]_2). Why: the executed code is correct, but the contradictory comment invited a maintainer to 'un-swap' the ec_pairing call (or swap the mcopy destinations) to make code match comment, which would flip the equation to e(pi,[1]) = e(RHS,[s]) and reject all honest proofs (fail-closed completeness break). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: add a TODO(structural) comment in construct_intermediate_sets_impl explaining that KZG query grouping and the duplicate-query eval-consistency assert compare EcPoint/Word handles by memory pointer, not by runtime value. How: comment-only. Records that this diverges from the midnight-proofs prover (groups by polynomial identity) and verifier (groups by commitment value), that it is safe today only because SolidityGenerator supports a single committed-instance column and all consumers depend only on first-appearance order, and that with >= 2 committed-instance columns sharing G1_IDENTITY_MPTR at one rotation the assert would panic at codegen. A value-based grouping is the real fix but is structural and out of scope here. Why: the auditor flagged both a codegen DoS (assert panics for >= 2 shared committed-instance columns) and a prover/verifier grouping divergence rooted in pointer identity; the fix is structural, so the invariant is documented to prevent a future refactor from silently dropping the eval-consistency check. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: promote the debug_assert_eq!(rotation, 0) in DataQuotientExpressionEnv::instance to a hard assert_eq!. How: replace debug_assert_eq! with assert_eq! and document why it must fail closed even in release builds. Why: the non-committed public-instance column is only interpolated at Rotation::cur(), so a rotated query must never reach this arm. debug_assert compiles out in release, so a rotated query slipping past the far-away constructor guard (builder/api.rs) would silently substitute instance(x) for instance(x*omega^k), generating a verifier whose quotient identity differs from the circuit (it could reject valid proofs and accept ones violating the intended constraint). A hard assert matches the fail-closed style of word_to_quotient_expr and ptr_to_quotient_mem in this file. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: replace the two `offset as u32` casts in ptr_to_quotient_mem with u32::try_from(...).expect(...). How: convert the isize Value offset via u32::try_from, which rejects both negative and > u32::MAX offsets, dropping the now-redundant assert!(offset >= 0) guards. Why: Value::Integer/Identifier carry isize offsets, so `offset as u32` silently wraps a planner offset above u32::MAX, and the generated VM bytecode would mload a wrapped, unrelated address, producing a verifier that computes the quotient numerator from the wrong memory word with no build-time diagnostic. Every other narrowing in this file already uses a checked conversion; this was the one silent cast. Practically unreachable today (EVM gas keeps frames far below 4GiB) but now fails loudly at generation time. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: assert every quotient memory pointer/token-offset is 32-byte word aligned in ptr_to_quotient_mem. How: at the single QuotientMem construction choke point, assert offset % WORD_BYTES == 0 for both the absolute Literal address and the TokenOffset offset. Why: the offline bytecode safety pass proves structural/stack safety and now bounds-checks const-table slots (validate_quotient_const_slots), but nothing validated the embedded memory operands. Every address the quotient VM reads is a word slot, so a non-word-aligned pointer indicates a truncated/mis-encoded address that would make the interpreter mload a straddling window and fold the wrong value into the numerator, flipping accept/reject with no build-time diagnostic. Guarding at ptr_to_quotient_mem covers all memory pointers in one place; all 151 lib tests pass, confirming the invariant. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: fail closed on a rotated non-committed instance query in instance_eval_at and expression_memory_ptr. How: hard assert_eq!(rotation, 0) in instance_eval_at's non-committed branch (the value resolver), and return None for rotation != 0 in expression_memory_ptr's Instance arm (which only detects consecutive direct memory pointers, so declining is safe). Why: both helpers resolve ANY rotation of a non-committed instance column to the single Rotation::cur() INSTANCE_EVAL word. Commit 5d7afe9 added the guard to the parallel-lookup path in quotient_numerator/vm/mod.rs but not to this file, so the only protection was the constructor-time guard in builder/api.rs. Any future entry point or refactor that lowers a circuit querying the public instance column at rotation +/-1 without going through SolidityGenerator::try_new would silently evaluate instance[rot] as instance[cur], generating a verifier that checks a different polynomial identity than the native Midfall verifier. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: replace rotation.abs() with rotation.unsigned_abs() in column_eval_var. How: one-line change; unsigned_abs returns u32 and is total over all i32. Why: rotation.abs() overflows for rotation == i32::MIN, panicking in debug and wrapping to a colliding/garbage variable name in release. This is only reachable if the constraint system supplied to codegen contains a query at Rotation(i32::MIN), so it is a build-time DoS on codegen inputs, not an on-chain issue, but unsigned_abs makes the conversion total. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: change the k == 0 empty-input-chunk branch of lookup_computations to emit the helper eval (h_eval) as the identity value instead of a hard-coded 0. How: replace the `let zero := 0` stub with out.push((lines, h_eval.to_string())). Why: the reference verifier (plonk/logup.rs) computes an empty chunk's helper constraint as helper_eval * (empty product = 1) - (empty sum = 0) = helper_eval, which enforces h == 0. Emitting 0 drops that binding while the accumulator constraint still folds the chunk's h_eval into sum_h, so a prover could set the unconstrained h freely and shift selector*sum_h to forge lookup balance. The branch is dead today (chunks are never empty), but emitting the reference-faithful value fails safe if a future chunking refactor ever reaches it. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: make lookup_computations' accumulator block tolerate an empty h_evals vector instead of indexing h_evals[0]. How: replace `let sum_h := h_evals[0]` + slice with a split_first(), emitting `let sum_h := 0` when there are no helper evals. Why: a LogUp argument with no input expressions produces zero helper chunks and an empty h_evals vector; the native verifier folds sum_helpers over the empty set to F::ZERO, but this emitter panicked with an index-out-of-bounds during code generation. This makes the Solidity generator match the native handling (and the structured native path in quotient.rs) instead of crashing at build time on a degenerate but legal circuit. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: make quotient_stack_words_for_build return at least one word. How: add .max(1) to the max_stack/native-scratch computation and document why. Why: every inline/native direct_quotient_block writes one eval-scratch word at eval_scratch_slot == quotient_stack_mptr, but the function could return 0 for a degenerate-but-valid VK whose gates all fit the inline prefix with no permutation sets, lookups, or VM items. That write is only in-bounds today because layout/memory.rs independently sizes the region as max(quotient_stack_len, MODEXP_FRAME_BYTES) for modexp-frame sharing, which is documented as a modexp concern, not as covering the quotient eval scratch. If that clamp is ever refactored away, the inline write would land one word past the registered region. Flooring here keeps the invariant with the code that emits the write; the modexp clamp still dominates real layouts, so this changes no current output (151 lib tests still pass). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: change the k == 0 empty-input-chunk branch of structured_lookup_loop_block to fold h_eval instead of 0 into q_lookup_eval. How: replace `let q_lookup_eval := 0` with `let q_lookup_eval := <h_eval>`, mirroring the Yul emitter fix. Why: same latent constraint-drop as in quotient_numerator/yul_emit.rs. The native verifier computes helper_eval * 1 - 0 = helper_eval for an empty chunk, enforcing h == 0; emitting 0 leaves h unconstrained while the accumulator still folds this h_eval into sum_h, which would let a prover forge lookup balance if a future chunking refactor ever produced an empty chunk. Dead code today but now fails safe in both lookup emission paths. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: add assert_eq!(input_chunks.len(), h_evals.len()) before the chunk/helper-eval zip in both structured_lookup_loop_block (quotient.rs) and lookup_computations (quotient_numerator/yul_emit.rs). How: assert the two counts match immediately before zipping them. Why: both paths fold one helper identity per element of input_expression_chunks().zip(h_evals). If data.lookup_evals ever carried fewer helper evals than chunk_by_degree produces chunks (e.g. an EvalRead-schedule or protocol drift after a midnight_proofs update), zip would silently drop the excess chunks, removing helper constraints from the y-batched numerator and producing a verifier that accepts proofs with unconstrained h values (forged lookup membership). This is only guarded transitively today via the protocol lookup count check; a direct assert at each zip site fails loudly at codegen. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
What: add a doc comment explaining why native_identity_estimate_block uses a hard-coded selector_gap of Some(1) that differs from the real selector_fold.gap_for(identity) used at emission. How: documentation only; no behavior change. Why: an auditor flagged the gap mismatch between the estimate (Some(1)) and the real native emission in compact_quotient_computation_blocks (gap_for). The estimate feeds only the native-vs-VM gate selection heuristic and runs during native_gate_candidates, before the selector_fold plan exists (the plan is derived from the selection outcome), so the real gap is genuinely unavailable here and the proxy is intentional. The divergence can only shift which gates are promoted to native callbacks, never the correctness of the emitted verifier. Documenting this prevents a future maintainer from mistaking it for a bug or wiring in gap_for where the plan does not yet exist. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
`eval_run_bytes` is a bound on transcript absorb bytes, but it summed `proof.quotient_limbs.byte_len` -- a *calldata* length -- while its two sibling bounds, initial_run_bytes and total_run_bytes, correctly use `layout::transcript::G1_ABSORB_BYTES`. The mixed units are invisible only because G1_ABSORB_BYTES == G1_BYTES == 128 right now. The term is also provably dead: total_run_bytes - eval_run_bytes reduces to word_absorb + (total_g1_count - num_quotients) * 128 + squeeze_cushion, which is non-negative, so eval_run_bytes can never be the max that sets `words`. That makes the wrong units silent -- no test can currently distinguish them. These two facts compound badly. If the transcript G1 absorb encoding ever diverges from the calldata encoding again -- as it did before the fix that stopped absorbing compressed 48/49-byte G1s against padded calldata -- then any refactor that starts trusting eval_run_bytes (using it for a per-run assertion, or dropping the conservative total_run_bytes) inherits a silently undersized keccak buffer, and the absorb loop overruns VK_MPTR, corrupting K_MPTR/OMEGA_MPTR. Count the limbs as `item_count * g1_absorb` so the bound is expressed in the units it claims. Value is unchanged today; the term stays as the per-run documentation of the eval phase. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Companion to the existing fixed-commitment assert. `permutation_comms` is built by zipping `meta.permutation_columns` against `EcPoint::range(permutation_comm_mptr)` with `izip!`, which silently truncates to the shorter side. Nothing checked that the VK actually carries one commitment per permutation column. Both counts derive from the same VerifyingKey and are equal by halo2 construction, so this is not reachable through the public API. But midnight-proofs' `VerifyingKey::read` takes column counts from file-controlled u32s without cross-checking the constraint system, so a crafted or corrupted VK could under-report them. The truncating zip would then quietly drop permutation columns from the generated verifier's permutation argument -- a verifier enforcing a weaker statement than the circuit -- instead of failing at codegen. Assert equality next to the fixed-commitment check that guards the same region's base pointer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`Value::Integer` holds a signed byte offset, and `Display` rendered a
negative one as `sub(0, N)`. The doc comment justified this by claiming
the result "only ever appears as `ptr_end` in `lt(ptr_end, ptr)` style
loops where any value strictly less than the smallest visited address is
acceptable".
That justification is backwards. `sub(0, N)` in EVM arithmetic is
`2^256 - N`, and `lt` is unsigned -- so the value is *larger* than every
representable address, not smaller. A descending loop written exactly as
the comment describes,
for { ptr := start } lt(ptr_end, ptr) { ptr := sub(ptr, 0x20) } { ... }
would evaluate `lt(2^256 - N, start)` as false on the first check and run
zero iterations, silently skipping whatever proof reads, absorption, or
validation the body performs. That is the classic shape of a fail-open
bug, and the comment invited a future emitter to rely on it.
No emitter produces a negative offset today (no rendered template
contains `sub(0,`), so this is latent. Make it stay that way: panic on a
negative concrete offset at render time and correct the comment to say
why no rendering is possible.
The `Sub` impls stay -- they are not dead as the report assumed. The
Askama template Constants.sol uses `proof_cptr - 1`, which is exactly the
kind of caller the panic now protects.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`repack_proof` validated G1 commitments (rejecting anything that fails subgroup-checked decompression with InvalidCompressedG1) but passed every eval and q_eval word straight through `scalar_le_to_be_word`, which only reverses byte order and never checks the value is < r. A caller feeding a corrupted or hostile native proof whose LE scalar bytes decode to a value >= r therefore got `Ok(calldata)` back from the shim -- calldata that the generated verifier is guaranteed to reject, since TranscriptProofParser.yul reverts on any eval or q_eval that is not a canonical Fr element. The caller pays gas for a doomed transaction and gets an opaque on-chain revert instead of a descriptive host-side error. This is fail-closed, not a soundness gap: the on-chain verifier range-checks every scalar and instance itself, so no forged acceptance was possible through the shim. It is purely about reporting the failure where the diagnostic is useful. Validate each scalar with `Fq::from_repr` -- the same canonicality test the native verifier applies -- and add a RepackError::NonCanonicalScalar variant carrying the byte offset and the offending word, mirroring the existing G1 error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The constant was documented as ASCII "pairing-batch-acc-kzg" right-padded
to one EVM word. It is not. The literal is 29 bytes -- 21 ASCII bytes plus
8 trailing zeros -- and FinalPairing.yul writes it with `mstore`, which
left-pads numeric literals. The word actually hashed into alpha is
00 00 00 || "pairing-batch-acc-kzg" || 00 * 8
Domain separation is unaffected, since the value is still a fixed unique
constant, and no code reads the tag by reconstructing it from the ASCII.
But the documented form and the implemented form differ, so any external
tool, differential fixture, or future reimplementation that derives alpha
from the right-padded spec would compute a different challenge than the
deployed verifier and diverge on accept/reject when reproducing a
verification.
Document the actual hashed word rather than changing the constant --
changing it would alter every derived alpha for no benefit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The accumulator pairing-batch hash frame occupies [PAIRING_BATCH_PTR, PAIRING_BATCH_PTR + PAIRING_BATCH_HASH_BYTES) = [0x100, 0x320), while final_pairing_scratch started at FINAL_PAIRING_SCRATCH_START = PAIRING_TWO_PAIR_BYTES = 0x300. The two regions therefore shared the word [0x300, 0x320) -- the last word of the ACC_LHS copy inside the alpha keccak preimage was physically the same memory as the first word of ec_pairing's input frame. Nothing caught this. The regions carry different MemoryPhases (AccumulatorPairingBatch vs FinalPairing), and MemoryLifetime::intersects returns false for any two distinct phases, so the arena's overlap validation is structurally incapable of reporting it. It was correct only by execution order within a single straight-line file: FinalPairing.yul computes the batch keccak, and makes its last read of the frame, before ec_pairing writes its scratch. Any future change that writes the pairing scratch earlier -- pre-staging the G2 pairs to save an mcopy, or emitting a debug re-hash after the pairing -- would corrupt the y_lo word of ACC_LHS inside the alpha preimage while validate() still passed. A partially-unbound alpha weakens the Fiat-Shamir binding of the randomized batching. Derive FINAL_PAIRING_SCRATCH_START from the batch frame's end so the two cannot overlap by construction (0x300 -> 0x320), and assert disjointness in fixed_low_memory_regions_are_planner_registered, since that assertion is now the only check that can catch a regression. Verified end-to-end: rendered scratch moves to 0x0320 and the fixture verifiers still compile under solc and verify real proofs on revm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MemoryLifetime::intersects decides whether two memory regions may share
bytes by comparing MemoryPhase values with <=, so the enum's declaration
order is trusted as the runtime execution order. Two variants were in the
wrong place:
* AccumulatorMsm was declared after PcsPairing, but its only user --
validate_public_accumulator, the sole consumer of acc_msm_scratch --
is called from VkLoading.yul immediately after the VK payload loads,
before the transcript has started.
* ScalarInv was declared between Transcript and LagrangeBatchInvert,
but scalar_inv is called from the PCS f_eval interpolation. In the
rendered verifier its call sites sit after the q_eval source-table
fold and before the fused final MSM, nowhere near transcript
absorption.
No current region pair turns this into corruption -- the arena validates
to the correct verdict, but for the wrong reason. The hazard is a future
region: a PhaseSpan { start: QuotientVm, end: PcsFinalMsm }, exactly the
span the selector accumulators already use, overlapping scalar_inv's
scratch at [vk_mptr - 0x100, vk_mptr - 0x40) would have been accepted by
validate() (ScalarInv sorted below QuotientVm) while scalar_inv's modexp
frames were in fact written in the middle of that span -- corrupting the
region and flipping the verification result with no codegen-time error.
Move both variants to their true positions and document that this order
is load-bearing and must track the include order in Halo2Verifier.sol.
The full EVM gate still passes, which confirms the reorder does not
expose a real overlap; it only makes the validator ask the right
question.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In trace renders, QuotientAndLinearization.yul expands the linearization terms into a G1MSM frame at `add(SELECTOR_ACC_MPTR, selector_len)` and writes (num_quotients + num_simple_selectors) * 0xa0 bytes there. That byte range was never registered in the MemoryArena: lin_trace_len was computed only to size the constructor's G1MSM smoke maximum, which is allocated at a different base. The write band was therefore in bounds by accident. Its safety rested on an unvalidated coincidence -- that the PCS scratch trio allocated from the same base (quotient_tmp_base) happens to be at least as long, because final_msm.input_bytes covers (num_quotients + nsel + 1) * 0xa0 -- and on the trace log word being placed past the PCS scratch ends. Neither memory.validate() nor validate_layout() could see the region at all, so a future layout change that shrank or relocated the PCS band (moving pcs_final_msm_scratch to a dedicated base, or a scheme change reducing final-MSM terms) would let the trace MSM silently overwrite whatever followed -- first casualty trace_u256_log_word, then unregistered memory -- with no codegen-time diagnostic. Register it as "linearization_trace_msm" under a new LinearizationTrace phase (it runs after the quotient VM and before the PCS blocks reuse the band), feed its end into the trace-log-word placement, and assert its base equals the address the template computes so the two cannot drift. Trace builds are not the production verifier, so there was no path to accepting a bad proof; this closes a blind spot in the planner rather than an exploitable bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
No production fixture enables `with_accumulator`, so the entire
`{%- if self.expected_has_accumulator %}` branch of
AccumulatorHelpers.yul -- the packed-limb coordinate decoder, the
pre-transcript public-accumulator MSM, and the fixed-base scalar tail --
was never handed to solc by the default test gate. The only coverage was
the opt-in ivc_keccak_solidity bench, which needs k = 20, a release
build, and external SRS assets, so in practice a Yul syntax error or a
solc stack-depth regression in that branch could reach a release
unnoticed. Rendered-template assertions in lowering/tests.rs pin the
source text but never compile it.
Render three accumulator shapes against the existing Poseidon VK --
fully collapsed, partially collapsed with a fixed-base scalar tail sized
to the VK's own commitments (-G + fixed + permutation), and the
point-pair form with implicit unit scalars -- and compile each verifier
and VK under the pinned solc. Also pin the accumulator scalar
canonicality checks, and assert the point-pair form reads no explicit
scalar at all.
Verified the coverage is real: injecting a Yul syntax error into the
accumulator-only branch fails this test, and it passes again once
reverted.
This still does not execute the accumulator logic against a real proof.
That needs a decider circuit carrying a genuine accumulator in its public
inputs, which remains the ivc_keccak_solidity bench's job.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The committed fixture dumps under target/*-fixture-dump/ no longer
matched what codegen produces. Two independent causes:
* They were already stale before this branch's verifier changes. The
quotient constant table in the VK dumps is ordered differently
because of "Reserve fused add-product constants early" and "Reserve
limb decomposition constants early", which legitimately change the
order constants are interned -- but the artifacts were not
regenerated at the time. Confirmed by regenerating at the parent
commit and diffing against the committed file.
* This branch's verifier-side hardening (Fr inversion range checks,
batch_invert early leave, the ec_pairing/TraceReturn fail-closed
guards, the quotient VM stack-balance check, the y^0 selector slot,
and the final-pairing scratch move to 0x320) changes the rendered
Yul.
Codegen itself is deterministic -- regenerating twice on the same tree
produces byte-identical output, and that output matches a regeneration
at the parent commit -- so this is purely a refresh, not a fix for
nondeterminism.
Note: target/ivc-keccak-solidity-dump/ is also tracked and is the only
dump covering the accumulator render path, but regenerating it requires
the opt-in ivc_keccak_solidity bench (k = 20, release build, external SRS
assets), so it is left untouched here and remains stale.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The accumulator's fixed-base scalar tail is derived from the *instance*
vector: fixed_scalar_count = num_instances - (offset + fixed_payload).
The bases those scalars multiply are generated from the *verifying key*,
as fixed_comm_mptr + i * 0x80 for i in 0..num_fixed_bases, where
num_fixed_bases = fixed_scalar_count - (1 + num_permutation_comms)
Nothing bounded num_fixed_bases against the VK's actual fixed-commitment
count. A tail longer than the VK can supply bases for rendered cleanly
and emitted base pointers past the end of the fixed-commitment region,
aliasing permutation commitments -- and past those, arbitrary VK payload
words -- as accumulator G1 bases. The generated verifier then checks a
different accumulator equation than the native verifier, so honest
proofs fail on-chain and the drift is discovered only at proof time.
The existing assert did not catch this: `bases.len() == fixed_scalar_count`
is a tautology, since `bases` is built as
`1 + num_fixed_bases + num_perm_bases` with num_fixed_bases defined by
subtracting those same terms from fixed_scalar_count. It can never fire.
(An earlier commit promoted it from debug_assert to assert, which changed
when it runs but not what it can detect.)
Reproduced against the Poseidon VK: num_instances chosen for a 32-scalar
tail against a VK with 19 fixed and 8 permutation commitments rendered
successfully with 23 fixed bases, i.e. 4 pointers into the permutation
region.
Validate in SolidityGenerator::try_new, where both the encoding and the
VK are in scope and a typed error is already the contract. Two tail
lengths are rejected: longer than `-G` + permutations + every fixed
commitment (the out-of-region case above), and shorter than `-G` +
permutations (which underflows the base count in the artifact emitter).
A tail inside that range is accepted: it covers a prefix of the fixed
commitments and keeps every pointer in region. Nothing documents whether
a prefix is meaningful, but it is memory-safe, and rejecting it would
invent a stricter contract than the emitter implies.
Add GeneratorError::AccumulatorFixedBaseTailMismatch carrying the
supported range and both commitment counts, so a misconfigured
num_instances reports the values it should have had, and replace the
tautological assert in artifacts.rs with a bound that actually
constrains num_fixed_bases.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Setting HALO2_SOLIDITY_RUN_EVM_TESTS=1 without the test SRS or the
pinned solc used to print a "skipping ..." line and return, so the run
reported every test green while compiling no Solidity and executing no
proof. That is the worst possible outcome: the operator explicitly asked
for the gate and got a silent no-op that is indistinguishable from a
pass.
This is not hypothetical. It is how the rendered fixture artifacts under
target/*-fixture-dump/ drifted out of date across several commits with no
test noticing, and how a round of verifier-template changes in this
branch was initially reported as "validated by the full suite" when in
fact 28 of the 34 end-to-end tests had returned early.
Split the two cases that were conflated:
* Not opting in stays a quiet skip. A fresh checkout without the 96 MB
SRS still runs `cargo test` cleanly, which is why the skips exist.
* Opting in and lacking a prerequisite is now a panic, with the fetch
command for the SRS and the SOLC / allow-unpinned escape hatches for
the compiler, so the message says how to fix it.
Applied to both the in-crate gate helpers and the four fixture
integration tests, which each carried their own copy of the skip logic.
Verified all four paths: opted out passes quietly (189 tests, no EVM
work); opted in with SRS and solc present passes for real; opted in with
SRS_DIR pointing nowhere fails with fetch instructions; opted in with a
bogus SOLC fails naming the pinned version.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`ec_pairing` folded the precompile result into its success flag with `ret := and(ret, mload(scratch))`. Because `ret` is already reduced to 0 or 1 by the preceding `returndatasize` check, the bitwise `and` against the full 32-byte result word only tests that word's low bit -- so any odd return value was accepted as a successful pairing, not just the canonical 1. A conformant EIP-2537 pairing precompile returns exactly 0 or 1, so this is hardening rather than a live break. It is worth closing anyway: the threat model explicitly warns that deployers on L2s and alt-EVMs must run their own conformance tests, and the codebase was already inconsistent with itself -- the constructor smoke test in PrecompileSmoke.sol uses the strict `eq(mload(scratch), 1)` form. Compare against 1 in the helper too, and update the template assertion in `production_verifier_documents_revert_or_true_policy`, which pinned the old expression verbatim. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`load_acc_coord_shifted` stripped the identity flag from the first
packed word under the guard:
if and(iszero(div(i, limbs_per_word)), first_adjust)
Yul's `and` is bitwise, not logical. `iszero(...)` yields 0 or 1, while
`first_adjust` is either 0 or a radix base -- 2^56 for the current
BLS12-381 self-emulation parameters. `1 & 2^56` is 0, so the guard was
false on every call and `packed := sub(packed, first_adjust)` never
executed. The parameter was entirely inert.
The visible consequence is that the identity probe in `load_acc_coord`
never removed the flag before testing for p-1, so `is_id` was always 0
and the `if is_id` branch in `load_acc_point` was dead. Infinity was
still accepted, via the separate canonical constant comparison in
`is_acc_encoded_identity`, and a flag-carrying x that did not match that
quadruple decoded to (p-1) + 2^56 and failed the base-field range check.
So the decoder was fail-closed throughout and no forgery was reachable.
What was broken is the canonicality barrier the comment above
`is_acc_encoded_identity` claims to enforce -- a barrier that exists so
the on-chain verifier need not rely on circuit-side reasoning it cannot
itself check.
Gate on the word index alone. Subtracting `first_adjust` is already a
no-op when it is zero, so the second conjunct was never needed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`SolidityGenerator::try_new` documents itself as "returning a typed error
when the supplied constraint system is outside the currently supported
Midfall verifier shape", and exists precisely so callers can avoid
`new`'s panic. It did not hold up its end.
After its own checks, try_new called `ConstraintSystemMeta::new`, which
reaches `ProtocolPlan::from_constraint_system` and ends in:
plan.validate().unwrap_or_else(|err| panic!("invalid protocol plan: {err}"));
`ProtocolPlan::validate` has roughly twenty `return Err(...)` arms, every
one of which became a panic on the supposedly panic-free path. The most
reachable is the absorbed-but-unopened advice check: `advice_indices` has
an entry per declared advice column while `advice_queries` holds only the
queried ones, so a circuit that calls `cs.advice_column()` without ever
querying it -- an ordinary authoring mistake -- panicked inside try_new.
The exposure is the opposite way round from what one might assume:
`SolidityGenerator::new` has no non-test callers, so its deliberate panic
is not on a production path, while try_new is.
Split each layer into a fallible variant and a panicking wrapper:
`ProtocolPlan::try_from_constraint_system`, `ConstraintSystemMeta::try_new`.
try_new now maps the failure onto the existing `GeneratorError::Planning`
variant with stage "constraint system"; `new` and the protocol-level tests
keep the old panicking entry points, so no caller changes.
Adds `generator_reports_unopened_advice_column_as_typed_error`, which
builds a VK from a circuit declaring an advice column it never queries and
asserts a typed error rather than a panic.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Lagrange block writes its batch-inversion input run in place from X_N_MPTR, and the run is num_instances + abs(rotation_last) + 1 words long. Nothing in `GeneratorConfig` or `SolidityGenerator::try_new` placed any upper bound on `config.num_instances`, so an oversized count was carried all the way into layout planning. The overrun itself was already caught -- `VerifierMemoryLayout::validate` has a dedicated cap check for this run against the first memory that is live when Lagrange executes (the q_eval calldata cursor, the G1 identity slot, and the decoded evaluations). So this was never a memory-safety hole. What was wrong is where the failure surfaced: a caller passing too many public inputs got an error phrased in theta-word offsets from deep inside layout planning, rather than a typed rejection from the constructor whose job is to validate the requested shape. The asymmetry made the gap easy to miss: `transcript_buffer_layout_for_meta` explicitly sizes the transcript buffer for arbitrary `num_instances`, so large public-input counts look supported, while the Lagrange window silently is not. Reject the count in `try_new` with a new `GeneratorError::TooManyInstances` carrying the requested count, the maximum, and the rotation contribution. The check must sit after `meta` is built, since `rotation_last` is only known then. To keep the early bound and the layout check from drifting apart, both now derive from one source: `batch_invert_input_words` for the run length and a new `theta_window::LAGRANGE_RUN_CAP_WORDS` for the cap. The existing layout test asserts the constant equals the window-derived value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Lagrange batch-inversion input run is written in place from X_N_MPTR and is longer than the 26 words between X_N_MPTR and ROT_POINTS_MPTR -- in the shipped moonlight-wrap render it spans [0x6cc0, 0x7080), four words past ROT_POINTS_MPTR. That is intentional, not an overrun: the rot_points, x1_powers and q_eval_set windows are written only in later phases, so `VerifierMemoryLayout::validate` caps the run against the first memory that is genuinely live at Lagrange time -- the q_eval calldata cursor, the G1 identity slot, and the decoded evaluations. The run is deliberately not registered as a `MemoryRegion`: doing so would make the arena's overlap loop report overlaps that are correct by phase ordering. The cost is that nothing mechanically enforces the ordering the cap relies on. It rested entirely on a source comment, so moving a rot_points access earlier would silently either clobber the denominators mid-flight or read their leftovers -- with no codegen error and no failing test. Add `rot_points_window_is_written_after_the_lagrange_denominator_run`, which renders verifier source and asserts the first ROT_POINTS_MPTR access -- read or write -- occurs after the Lagrange batch_invert call. State the invariant explicitly at the cap check and point it at the test. No behaviour change; this closes the enforcement gap, not a live defect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every EIP-2537 probe in `require_eip2537_precompiles` used the all-zero point-at-infinity encoding: G1ADD(identity, identity), a worst-case G1MSM with all-zero terms, and PAIRING_CHECK over identity pairs. The identity is the one input a precompile can answer correctly without performing any curve arithmetic, so an implementation that simply returns zeros satisfies the G1ADD and G1MSM probes. The soundness-relevant part is narrower and worse: the identity is also the one input on which an implementation that omits the EIP-2537 subgroup check still returns the right answer, and the production verifier leans on G1MSM precisely as its subgroup validator for absorbed commitments. A chain whose G1MSM is present and correctly sized but subgroup-check-less would deploy without complaint and then accept proofs carrying off-subgroup points. Add a G1ADD(G, G) == 2G probe, which no stub can satisfy by echoing its input or returning a constant. The vectors are computed from the curve library at render time rather than hardcoded, and emitted as literals so the deploy cost stays a handful of mstores. Note this narrows but does not close the gap: the pairing and MSM probes are still identity-only, so a subgroup-check-less G1MSM would still pass. Closing that needs an off-subgroup negative vector, which is a larger change. Verified against revm's real EIP-2537 implementation: the constructor deploys, and `verifier_constructor_rejects_missing_or_mismatched_eip2537_precompiles` still rejects misrouted precompiles. A full `--features evm` run shows no test regressing relative to 2c77051. `constructor_known_answer_vector_is_the_generator_and_its_double` pins the constants against the curve library, including that the expected output differs from the probe input -- a known-answer test whose answer equals its question tests nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The verifier body is wrapped in `assembly ("memory-safe")` while writing
absolute addresses from 0x80 upward and never allocating through the
free-memory pointer. The annotation is therefore false -- and it is also
what enables solc's via-IR stack-to-memory mover, which reserves spill
slots upward from 0x80 and records the top in the runtime's
`mstore(0x40, ...)` prologue.
Both parties then claimed the same bytes. The deployed moonlight-wrap
runtime begins `6108e0604052` = mstore(0x40, 0x08e0): solc reserved
[0x80, 0x8e0) for itself, while the generated layout put TRANSCRIPT_MPTR
and RETURN_MPTR at 0x80. The optimized IR shows a real spill in that
window -- `mstore(0x300, mload(0x6a00))` stores the y challenge and reads
it back ~600 lines later.
This has not misbehaved so far only because the spill placements and the
verifier's writes happen not to collide destructively. Nothing enforced
that. The reservation is a function of stack pressure, not a constant:
0x80 (ivc-keccak, none), 0xe0 (rsa), 0x3c0 (poseidon), 0x8e0
(moonlight-wrap). A different circuit, solc release, or optimizer
schedule can place a live spill slot across a verifier write, corrupting
a challenge or pairing input with no source change, no compile error, and
no crash -- potentially a false accept produced by recompilation alone.
Removing the annotation was tried first and does not work: the block then
fails with `Cannot swap Variable usr$f_4 ... too deep in the stack by 1
slots`, and solc itself suggests re-adding it. It is load-bearing.
So move the generated layout above the reservation instead. Rooting
LOW_MEMORY_SCRATCH_START at 0x1000 makes solc's spill region and the
verifier's memory disjoint in space, so their liveness stops mattering.
VK_CONSTRUCTOR_PAYLOAD_START is decoupled and stays at 0x80: it belongs
to Halo2VerifyingKey, whose assembly is not annotated memory-safe, so no
reservation exists there.
Being explicit about scope: this removes the consequence, not the false
annotation. Making the annotation honest needs runtime mload(0x40)-based
re-basing, which costs an ADD per access.
Add `compiled_memoryguard_does_not_overlap_generated_layout`, which
compiles each rendered variant and reads the free-memory-pointer prologue
back out of real bytecode, failing if the reservation ever reaches the
layout base. Verified to have teeth: with the base returned to 0x80 it
fails with `solc reserved [0x80, 0x3c0) ... overlaps the generated
verifier layout based at 0x80`.
Size impact is immaterial -- the largest known verifier runtime is 21161
bytes against EIP-170's 24576, and the change only widens some PUSH1
address literals to PUSH2.
Full gate green: 194 passed with HALO2_SOLIDITY_RUN_EVM_TESTS=1 and
features evm,rust-verifier-trace, including the end-to-end proof
verification tests under revm.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`Halo2VerifyingKey` is size-checked at render time by `validate_payload_layout`, because a data contract's runtime length is known before compilation. The verifier's is not -- it only exists once solc has run -- so nothing bounded it. That gap was invisible rather than latent: the revm harness sets `cfg.limit_contract_code_size = Some(usize::MAX)` (evm.rs), deliberately, so that in-process tests can run oversized contracts. The consequence is that a verifier exceeding 24576 bytes would pass the entire suite, including every end-to-end proof verification, and then fail to deploy on mainnet or any other EIP-170 chain. The comment justifying the cap lift is also stale: it cites a ~25 kB verifier from before the inline `decompress_g1` calls were removed. Add `compiled_verifier_runtime_fits_the_eip170_limit`, which compiles the embedded, separate, quotient and VK renders and asserts each runtime is within the limit. Checking the compiled artifact is the only option here, since the size is a property of solc's output rather than of the plan. Current headroom is comfortable -- the embedded poseidon render is 16037 bytes and the largest known verifier, the deployed moonlight-wrap one, is 21161 -- but the layout rebase in the previous commit widens some address literals from PUSH1 to PUSH2, so the bound is now worth asserting rather than assuming. Verified the assertion measures real bytecode by temporarily inverting it and reading back the reported sizes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`midnight-aggregation` did not compile. `LightAggregator::verify`
delegates to `plonk::prepare`, which gained the bound
<T::Hash as TranscriptHash>::Input: TranscriptInputBytes
in db26827 ("test(solidity): add native differential trace hooks"). The
bound was never propagated to this caller, so the crate has been broken
at HEAD -- and because it is a workspace member, `cargo check --workspace`
fails before reaching anything else.
Add the bound to `verify`'s where clause, matching how the other
`prepare` callers declare it in `proofs/tests/plonk_api.rs:477,523`. This
is a propagation gap rather than a design problem: `TranscriptInputBytes`
is implemented for both real transcript input types, `Vec<u8>` and
`Vec<Fq>`, so every concrete transcript already satisfies it and no call
site needs to change.
`cargo check --workspace --lib --tests` is clean and the crate's two lib
tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The public-accumulator decode path in AccumulatorHelpers.yul had no
executing test coverage at all. The tests that appeared to cover it --
`accumulator_decoder_rejects_noncanonical_infinity` and neighbours in
src/lowering/tests.rs -- are `verifier_template.contains("...")` string
greps over the raw template: they assert the guard *text* is present and
never render, compile, or run it. That is exactly how the always-false
`and` guard fixed in a951f04 survived. Those greps passed for as long as
the identity branch was dead code, and a regression that re-deadened it
would pass them again.
The only executing accumulator tests are in tests/ivc_keccak_solidity.rs,
which proves a k=20 decider from scratch. It needs ~300 MB of SRS and
three minutes, so it is gated behind HALO2_SOLIDITY_RUN_IVC_BENCH=1 and
never runs in CI.
Replace the orphaned fixtures/ivc contents with pre-rendered artifacts
and replay those instead. The previous fixtures (vk.bin, proof.bin,
instance.bin, two trace dumps) were referenced by no test, script, or
example, had no writer anywhere in the repo, and were not even loadable:
`VerifyingKey` serialization starts with VERSION = 0x03 while vk.bin
starts with 0x12. Reading one would also require the decider circuit type
from midnight-aggregation, and rendering a verifier from a VK needs the
full SRS because `SolidityGenerator` consumes `params.g_lagrange()` --
which is what makes a VK-based replay unusable in CI no matter how fresh
the fixture is.
Shipping the rendered contracts plus matching calldata sidesteps all of
that: the replay needs only solc and revm. It runs in ~2 s.
The test verifies the fixture proof is accepted first, then asserts three
classes of accumulator mutation are rejected: non-canonical limb packing,
a malformed coordinate with a zeroed scalar, and substitution of the
canonical encoded point at infinity. The accept baseline is what makes
the rejections meaningful -- without it they could pass because the
verifier rejects everything.
Two things guard against the test passing vacuously:
- The offset arithmetic is checked structurally. Four 56-bit limbs
occupy the low 224 bits of a packed word, so every accumulator
coordinate word must have four zero high bytes; random proof bytes
would not. A miscomputed offset would otherwise still revert, for the
wrong reason.
- Each mutation class was individually neutered and confirmed to make
the verifier accept, so all three assertions are load-bearing.
The fixture describes itself: accumulator offset, limb count and the
has_accumulator flag are parsed back out of the rendered VK payload, and
the infinity encoding out of the verifier's own constants, so this test
carries no duplicate constants that could drift from the artifact.
Fixtures were regenerated at this branch's codegen and confirmed to carry
the rebased memory layout (TRANSCRIPT_MPTR = 0x1000); the underlying bench
verified the proof on-chain in 1,285,274 gas. fixtures/ivc/README.md
records provenance and the regeneration command.
Known limitation: a rendered fixture is a codegen snapshot. It stays
self-consistent and keeps passing after a codegen change, so it silently
stops testing current output. Detecting that needs re-rendering, which
needs the SRS again, so it is tracked by a commit stamp rather than an
assertion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two gaps remained after the IVC replay landed. The `point_pair` accumulator encoding had no executing coverage of any kind. `accumulator_verifier_variants_compile_with_pinned_solc` renders it against the poseidon VK and checks it compiles, but the template's `expected_acc_has_carried_scalars = false` arms were never run against a proof -- and that is the encoding the deployed Moonlight wrap verifier uses. It cannot be synthesized from the IVC fixture either: point_pair occupies 8 public-input words against point-and-scalar's 10, so dropping the scalars changes the instance count, hence the Lagrange evaluation and the transcript. Public inputs are bound into the proof. So generate a real one. `fixtures/moonlight-wrap` holds the rendered wrap decider and its calldata, produced by Moonlight's wrap bench on `origin/codex/wrap-bench-cherry-picks` against this branch's codegen. That run also confirms the layout rebase against a second accumulator shape: proof accepted on-chain in 1,277,823 gas with 244 native/Solidity trace points matched. The replay is now parameterized over both fixtures and recovers the encoding kind from the payload width rather than being told, so a fixture still describes itself. Split renders deploy a quotient evaluator; single-contract renders like the wrap one deploy against the verifying key alone. Second gap: the ivc replay only attacked the accumulator words. Port the calldata-level dimensions that poseidon covers and a static fixture can carry -- framing (trailing bytes, truncation, selector, ABI heads, proof length, instance count), every proof commitment against off-curve, base-modulus and non-canonical-padding G1 vectors, and every evaluation scalar and non-accumulator public input set to the Fr modulus. That is 32 commitments x 3 vectors, 115 eval words and 14 instances for the IVC fixture, ~230 EVM calls in about 2 s. The poseidon suite is left intact. Much of it cannot move to a static fixture -- wrong-VK and VK-mutation cases need a second verifying key, cross-wiring needs four circuits on one SRS, the forced-domain-root case needs prover-side control, and the pbt_* family draws fresh proptest seeds per run. Deleting those would have removed dimensions no fixture replay can reproduce, so this is additive: the accumulator configs gain the portable attacks, poseidon keeps the rest. The commitment run length is discovered from the EIP-2537 padding signature rather than hardcoded, so a regenerated fixture with a different commitment count stays covered. Every mutation class was individually neutered and confirmed to make the verifier accept, including on the point_pair fixture -- so its accumulator is genuinely consumed rather than ignored, and no assertion passes vacuously. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Sepolia deployment record documented what was deployed but not how to check it, nor that the generator has since diverged from it. Add both. Deliberately does not regenerate the artifacts. This directory is a record of a live deployment -- addresses, transaction hashes and the runtime codehashes of contracts that exist on Sepolia. Re-rendering Halo2Verifier.sol at current codegen while leaving those fields would assert that 0x5CfEd44D16F994fc17f681A83FEdFEB9c3348c17 has source it does not have. The sources and bytecode stay exactly as deployed. Verification. The tracked source does reproduce the on-chain runtime: compiled with the documented flags at optimizer-runs 200 it yields 21,161 bytes, matching the recorded runtimeBytes. Its codehash (0x79432a36...) is *not* the recorded runtimeCodeHash, and should not be: a byte-level comparison against the fetched on-chain code shows exactly 40 differing bytes, in two 20-byte runs at offsets 0x51 and 0x125. Those are the placeholder slots for `address public immutable AUTHORIZED_VK`, which the constructor fills at deployment. Every other byte matches. Both the command and the figures were re-run from inside the deployment directory before being written down. Divergence. Re-rendering the same circuit at e5300d4 gives a 212,419-byte source and a 21,203-byte runtime against the deployed 206,619 / 21,161, with TRANSCRIPT_MPTR moving from 0x80 to 0x1000. The verifying key differs too, in its quotient_program section. The circuit is unchanged (acc_offset = 11, 19 public inputs, point_pair). The record now names the fixes the deployed code does not carry: the memory layout rebase (its layout still sits inside solc's via-IR spill window -- AUDIT.md TA-5), the always-false accumulator identity guard, the pairing check accepting any odd result word, and the identity-only EIP-2537 smoke vectors. None is a demonstrated forgery path, but a reader of this directory should not have to reconstruct that list from the git log. Redeployment needs a funded keystore and an RPC endpoint and is left to a deliberate operator action. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rendered artifacts under proofs/solidity-verifier/target are committed
and gated in CI, which fails the build when they drift from the generator.
Refresh them for the codegen changes on this branch.
Every difference is accounted for:
- TRANSCRIPT_MPTR / RETURN_MPTR move from 0x80 to 0x1000, the layout
rebase above solc's via-IR spill reservation.
- ec_pairing compares the precompile result against 1 rather than
folding it with a bitwise and.
- The accumulator limb decoder's identity guard is gated on the word
index alone, replacing the always-false bitwise and.
- The constructor smoke test gains the G1ADD(G, G) == 2G known-answer
vector.
- Halo2VerifyingKey's quotient_program changes because the VM encodes
absolute memory pointers: each shifts by exactly 0xf80, which is
0x1000 - 0x80, matching the layout move (0x4740 -> 0x56c0,
0x48c0 -> 0x5840, 0x47e0 -> 0x5760).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The quotient lowering runs a peephole optimizer whose shape recognizers
and run-compaction pass rewrite seven-limb foreign-field expressions
into superinstructions. Both are pure encoding choices that must
preserve the evaluated polynomial exactly, but their correctness was
only checked indirectly by fixture-circuit trace differentials in CI.
A recognizer bug on a previously unseen gate shape could therefore ship
a wrong verifier without any generator-time failure.
Add a two-part certification pass that runs on every LoweringPlan:
* vm/reference.rs is an independent interpreter for finalized
quotient bytecode, deliberately written against the ABI without
sharing execution code with the emitter.
* vm/certify.rs re-executes the emitted program with that reference
interpreter and compares each identity against direct evaluation of
the QuotientExpr tree it was lowered from, then repeats the check
against a second build produced with the limb superinstructions
disabled. Any disagreement fails the render before the bytecode can
be pinned into a verifying key.
Wire this in from LoweringPlan::new and validate_generator_invariants
so the check runs unconditionally, not just in tests. Expose
QuotientProgramBuilder::with_limb_vm_ops outside cfg(test) so the
generator can build the baseline program, and add
build_quotient_program_items_with_limb_ops to lower the same item
stream under an explicit opcode policy.
Add QuotientVmTestCircuit and lowering_plan_certifies_emitted_quotient_bytecode
so the fast test suite actually reaches the VM path (the existing
LoweringPlanTestCircuit has only one gate and fits entirely in the
inline prefix). The test also asserts that the emitted program uses
LIN7, so it fails loudly if the planner ever stops routing these
identities through the recognizer.
Update QUOTIENT_EVALUATOR_9KB_BYTECODE.md to fix the stale module path
and document the new certification gate.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Re-ran Moonlight's wrap Solidity bench (wrap_circuit_composes_two_fold_children_from_four_dummy_fold_proofs with MOONLIGHT_RUN_WRAP_SOLIDITY_BENCH=1) against current codegen using the sibling-worktree pattern documented in the fixture README. Halo2Verifier.sol and Halo2VerifyingKey.sol come out byte-identical, so only calldata.bin needs refreshing to match the freshly serialized transcript. The bench reports 1,277,811 gas under revm Prague and 244 matched trace points; the CI replay wrap_point_pair_decoder_rejects_malformed_public_accumulator continues to pass against the updated calldata. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ndow The accumulator/KZG pairing-batch hash frame (0x100) and the ec_pairing two-pair input frame (0x320) sat inside the [0x80, reserved_end) window solc's via-IR stack-to-memory mover uses for spill slots (observed up to 0x8e0), so the alpha Fiat-Shamir preimage and the final pairing inputs were protected only by an unenforced liveness coincidence. Root PAIRING_BATCH_PTR at LOW_MEMORY_SCRATCH_START like every other low-memory base, make VerifierMemoryLayout::validate() reject any generated region below that base, add the batch-frame end to the VK_MPTR floor, and extend the memoryguard test to also compile the accumulator-bearing variants whose FinalPairing block was never handed to solc by that guard. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The single-element fast path fails closed on any word >= r, but the multi-element path folded raw words straight through mulmod, so a non-canonical scalar was silently reduced and its slot filled with inverse(x mod r): accept/reject semantics depended on batch length. Unreachable from the one production call site (the Lagrange block feeds addmod/mulmod outputs), but a latent divergence inside a shared primitive whose other inversion helper, scalar_inv, already rejects x >= r. Guard every element of the forward pass like the singleton path, pin the guard text in the template greps, and add a revm harness test that executes the rendered helper on adversarial words in both paths (it reproduces the acceptance on the unfixed template) and checks canonical batches against native inverses. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
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
1 participant
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.
No description provided.