Repository navigation
fix(circuits): encode foreign point identity flag as a separate public input - #15
Conversation
jtcoolen
left a comment
jtcoolen
left a comment
There was a problem hiding this comment.
Review summary
Taste rating: 🟢 Good taste
The fix is correct and token-identical to upstream midnight-zk#398. The first public-input word packs several limbs of x, so adding 2^LOG2_BASE for the flag was the same as adding 1 to limb 1. An identity point, whose x/y are unconstrained, could therefore share public inputs with a non-identity point; a dedicated flag element closes that. The inline comments cover the two places that still assume the old layout.
General notes
q: This branch is stacked on #14 (benchmark_256_trees), so merging it into main also merges the 255-level map change, which still has changes requested. Could you retarget the base to benchmark_256_trees, or rebase onto main so the identity fix can land separately? The fix doesn't depend on the map change.
Risk assessment
Verdict: ✅ Worth merging once the five zk_stdlib VKs are regenerated and the Solidity accumulator decoder is updated (or tracked in a linked follow-up).
ae29f6f to
0f4cc9e
Compare
| // The circuit constrains the flag as a bit. Anything else is a | ||
| // malformed public input rather than a non-identity point. | ||
| ok := lt(flag, 2) | ||
| is_id := eq(flag, 1) |
There was a problem hiding this comment.
P2 (addressed): proof-backed identity coverage. The original successful IVC proof carried is_id = 0 for both accumulator points. Flipping a flag only checks rejection of altered public inputs; it does not show that a valid identity point passes this decoder. I added fix commit 7358054 with a valid identity-accumulator proof accepted by the generated EVM verifier, and 61c525e with recursive neutral/all-zero rejection cases. Focused tests and the full IVC-to-EVM test pass. No exploitable decoder flaw was confirmed. This cryptographic path remains high-risk and merits human review; the separate Merkle-map changes were outside this review.
|
cb80706 checks identity encoding and acceptance. The tests.rs:1490 checks the exact 12-word accumulator layout, including each point’s trailing identity flag and scalar. The test.rs:286 then generates real proofs for an identity pair with both canonical coordinates and deliberately out-of-range coordinate bits. Both must verify: when the identity flag is set, those coordinate words have no point meaning, though the proof still binds them. 9a4a07b checks that an accumulator cannot be substituted. In the ivc_keccak_solidity.rs:1439, the neutral identity pair satisfies the accumulator pairing on its own. Replacing the genuine non-neutral public accumulator with it must nevertheless fail because the original proof binds different public inputs. Trying to reprove the unchanged leaf witness against that neutral accumulator must also fail synthesis or verification. 84e7c56 checks where rejection happens. It changes a leaf opening to another valid G1 point, then proves an outer statement whose PLONK proof verifies but whose carried accumulator is invalid. The ivc_keccak_solidity.rs:1349 confirms the Solidity verifier reaches final pairing, receives false, and reverts. A new evm.rs:275 also distinguishes bad encodings rejected before G1MSM, off-curve points rejected by G1MSM, and changed public inputs rejected at final pairing. |
539fe26 to
e5bcfdb
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Foreign point public inputs now carry is_id as a dedicated word after the x and y limbs instead of packing it into x's first limb. Match that layout in the accumulator decoder: * count 5 words per accumulator point (12 for the fully collapsed accumulator, 10 for the Moonlight point pair); * read the trailing flag word, require it to be 0 or 1, and never decode x/y when it is set, since the circuit leaves them unconstrained; * drop the packed-flag p - 1 constants and is_acc_encoded_identity. Add a unit test pinning the word counts to the circuit's as_public_input lengths, derive the IVC e2e accumulator offsets from the circuit encoding, and add non-boolean and flipped identity flag negatives there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e5bcfdb to
9645c9b
Compare
Check that foreign-point identity flags and carried scalars occupy the expected public-input words. Prove an identity accumulator with both canonical and unconstrained coordinate words, showing that the Solidity decoder ignores coordinates when is_id is set.
Neutral satisfies its pairing equation, but unchanged leaves compute a non-neutral accumulator. Reusing their outer proof with four neutral or zero-word public-input encodings must be rejected: flagged identities change the outer KZG transcript, unflagged identity coordinates decode to forbidden (0,0), and unflagged raw zeros decode to off-curve (1,1). A neutral claim with unchanged leaves also cannot yield a valid outer proof because the circuit binds its public accumulator to the value it computes.
An incorrect but well-formed inner KZG opening is public inner proof data carried as outer witness. The partial verifier prepares its pairing equation without checking it; binding the computed accumulator as outer public input yields a valid outer proof whose carried pairing is false. Record 0x0c/0x0f precompile calls even on revert to prove the bad opening reaches final pairing, while the four altered accumulator inputs stop at their actual validation stages: final outer KZG pairing, canonical (0,0) decode, or the first G1MSM rejecting off-curve (1,1). No production verifier logic changes.
3 participants
Overview
This cherry-picks the matching fix from
midnight. Before, a foreign point'sis_idflag was added into the first public-input element. Because that element packs several limbs ofx, an identity point could produce the same public inputs as a non-identity point. The flag is now its own public-input element after thexandylimbs.