Add NonIdentityPoint::new_from_constant#887
Conversation
This PR adds `NonIdentityPoint::new_from_constant` to enable creating non-identity points that are properly pinned to constants, which is required by the Orchard ZSA circuit ([Orchard PR](QED-it/orchard#246)). More precisely, in the Orchard ZSA circuit, `q_init_zec` / `q_init_zsa` must be constrained to fixed constants, as they define the initial point `Q` of the Sinsemilla hash. If constructed via `NonIdentityPoint::new`, they remain unconstrained witnesses, allowing a prover to inject an arbitrary on-curve point and break commitment soundness.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
==========================================
+ Coverage 83.31% 83.36% +0.05%
==========================================
Files 105 105
Lines 13054 13048 -6
==========================================
+ Hits 10876 10878 +2
+ Misses 2178 2170 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…#57) Add tests for `NonIdentityPoint::new_from_constant` and `Point::new_from_constant`.
| }, | ||
| fixed_commitments: [ | ||
| (0x05f5862cad2888855bc3c1843a9eff57b11b592d9eb0e13354256661387f5231, 0x32236b14df85bf5f532a930232cb23a5c56ef7d67aaeed8bcb8fc10ea132cbd6), | ||
| (0x0b70f6f89e88131bb5ab28571ca837a4160b51998c6ced04328991a5c3bdcae9, 0x2cee3ef6523c2d36e01981c4794271d53fb47e9c5480b982a2ff3a869144d648), |
There was a problem hiding this comment.
This changes because of the call to super::chip::witness_point::tests::test_witness_non_id in MyEccCircuit::synthesize. But we need to ensure circuit stability, so actually MyEccCircuit should have a test_zsa_additions flag that is set to false in order to check stability of the existing fixtures (both _insecure and _fixed after halo2_gadgets 0.5.0), and to true for the circuit variant that tests non_constant_point_id as well (there we only need the "fixed" variant).
There was a problem hiding this comment.
Blocking requests:
- rebase on top of
halo2_gadgets 0.5.0when that merges; - add a flag to
MyEccCircuitso that the existing stability checks (test_ecc_chip_*_against_stored_circuit) continue to pass without changing the*.bin,*.rdatafixture files; and add new stability checks with new fixture files (only thecircuit_version = AnchoredBasevariant needed).
Otherwise this looks good, we'll merge it and release as halo2_gadgets 0.5.1.
…hecks (#58) This PR introduces a circuit version flag in MyEccCircuit to preserve backward compatibility with the existing serialized circuit fixtures.
Merge upstream into add_non_identity_constant_point
Adds a regression test for the ECC chip with ZSA additions enabled, verifying the proof and verification key against a stored reference.
This PR adds
NonIdentityPoint::new_from_constantto enable creating non-identity points that are properly pinned to constants, which is required by the Orchard ZSA circuit (Orchard PR).More precisely, in the Orchard ZSA circuit,
q_init_zec/q_init_zsamust be constrained to fixed constants, as they define the initial pointQof the Sinsemilla hash. If constructed viaNonIdentityPoint::new, they remain unconstrained witnesses, allowing a prover to inject an arbitrary on-curve point and break commitment soundness.