fix!: ecc add predicate completeness bug #19471
Merged
Conversation
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
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dbanks12
requested review from
IlyasRidhuan,
fcarreiro and
jeanmon
as code owners
dbanks12
changed the title
dbanks12
requested review from
MirandaWood
and removed request for
IlyasRidhuan,
fcarreiro and
jeanmon
MirandaWood
approved these changes
Jan 12, 2026
Contributor
MirandaWood
left a comment
MirandaWood
left a comment
There was a problem hiding this comment.
Nice! I also found this here: #19462
But mine incorrectly assumes that we cannot have real coordinates with !x_match && y_match, so this PR should go in and I'll edit mine accordingly 🙌
MirandaWood
added a commit
that referenced
this pull request
Jan 12, 2026
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 12, 2026
BEGIN_COMMIT_OVERRIDE feat(avm security): add static check for isolated/unused columns (#19489) feat(avm): use noop calldata hasher in fast sim (#19495) chore(avm): rename indirect -> addressing mode (#19491) chore(avm): small cursor optimizations chore(avm):! rename indirect -> addressing mode (PIL) (#19493) fix(avm): constraint when unwinding empty call stack (#19485) feat(avm): Fuzz debug log and refactor env getter (#19494) fix!: ecc add predicate completeness bug (#19471) chore(avm): callstackmetadatacollector clarifications (#19490) chore: sanity assert in execution for bytecode id (#19486) fix!: sstore allowed injection of malicious write rows (#19470) fix!: defensive ghost row constraints in bc_hashing pil (#19481) fix(avm): fix execution::mov for mac? (#19507) chore(avm)!: resolve execution TODOs (#19501) fix!: multiple traces had ghost row injection vulnerabilities (#19480) fix(avm): defensively copy MemoryValues (#19512) feat: align TS and BB log levels (#19518) END_COMMIT_OVERRIDE
MirandaWood
added a commit
that referenced
this pull request
Jan 14, 2026
### ECC Pre-Audit - Normalise infinities Closes [AVM-193](https://linear.app/aztec-labs/issue/AVM-193/normalise-input-infinity-points-to-ecc-add) NOTE: The issue of no operation being set is now addressed in #19471 => this PR only normalises `inf`s so they always have `(0, 0)` coordinates in `ecc.pil`. Though the circuit no longer fails, it does incorrectly set the predicates (i.e. `double = 0` when doubling `inf`), which could be a footgun. In pre-audit it was found that our C++ elliptic curve point class `StandardAffinePoint` accepts different representations of the infinity (`O`) point - this makes sense as in noir we use `(0, 0)` and in BB we use `((P+1)/2, 0)`. This would be fine if any ECC calculations in the AVM _first_ checked `is_inf` and overrided any subsequent coordinate-based operations. But the `ecc.pil` trace sets whether we have a `double` or `add` operation based on coordinates without gating by any `is_inf` checks: ``` bool x_match = p.x() == q.x(); bool y_match = p.y() == q.y(); bool double_predicate = (x_match && y_match); bool add_predicate = (!x_match && !y_match); // If x match but the y's don't, the result is the infinity point when adding; bool infinity_predicate = (x_match && !y_match); ``` Assuming (understandably!) that two infinity points would share the same coordinates, the above works fine, but we can input a BB-standard `O` as `p` and a Noir-standard `O` as `q` from as early as the simulation call without any errors/checks. ~This results in `!x_match && y_match` which means none of `double_predicate`, `add_predicate`, or `infinity_predicate` are true and the circuit falls over. See c9da1ea for repro of this.~ ~Though there is no current flow that throws here, I think it's worth addressing as we can make the circuit fail with what should be valid inputs.~ ### Fix The current approach is to normalise any _input_ infinity points (we already normalise _resulting_ infinity points in `ecc` and `scalar_mul`) at the simulation stage, so events with any infs always have `(0, 0)` coordinates. This means: - No changes required to `ecc.pil` or the `execution.pil` dispatch - No changes to memory reads/writes - The ecc simulator sets `x = 0, y = 0` if the point `is_inf` for standard add and scalar mul events - `ecc_mem.pil` now constrains that `ecc.pil` has `x = 0, y = 0` if the point `is_inf` via the existing lookup (requires 4 more columns) We could alternatively normalise the points in tracegen, but this means we don't use the values emitted in the events, which feels a bit gross. We could also add many gating constraints to `ecc.pil` to handle `inf` edge cases, but this would be expensive and defeats the point a bit of the trace assuming that the input points are on the curve and validated. So this approach felt the smoothest!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
add_predicatebug for points with same y-coordinate but different x-coordinateDescription
The ECC tracegen had a bug where
add_predicatewas set to(!x_match && !y_match), requiring both coordinates to differ. However, the PIL constraintsel = double_op + add_op + INFINITY_PREDrequiresadd_op = 1whenever x-coordinates differ (regardless of y-coordinates).This caused constraint failures when adding two points with different x but same y:
x_match = 0,y_match = 1double_op = 0(not doubling)INFINITY_PRED = x_match * (1 - y_match) = 0add_op = sel - double_op - INFINITY_PRED = 1add_op = 0→ constraintsel = 0 + 0 + 0 = 0 ≠ 1failsWhy this edge case exists
On Grumpkin (y² = x³ - 17), two distinct points can share the same y-coordinate due to cube roots of unity in BN254 Fr. If
(x, y)is on the curve, then(ω·x, y)is also valid sinceω³ = 1.Fix
Test plan
🤖 Generated with https://claude.ai/code
Co-Authored-By: Claude noreply@anthropic.com