Skip to content

Conversation

@jialinli98
Copy link
Contributor

@jialinli98 jialinli98 commented Jul 7, 2025

Description

remove validation with transcript for add and sub if program is unconstrained

Problem*

Resolve #79

Summary*

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@github-project-automation github-project-automation bot moved this to 👀 To Triage in Noir Libraries Jul 7, 2025
@jialinli98 jialinli98 changed the title chore: simply add and sub chore: simplify add and sub Jul 7, 2025
@jialinli98 jialinli98 requested review from TomAFrench and kashbrti July 9, 2025 01:21
Comment on lines 321 to 327
let new_transcript =
JTranscript { x3: p2.x, y3: p2.y, z3: B::one(), lambda_numerator: B::zero() };
result = (p2, new_transcript);
} else if (rhs_infinity & !lhs_infinity) {
result = (self, JTranscript::new());
let new_transcript =
JTranscript { x3: self.x, y3: self.y, z3: B::one(), lambda_numerator: B::zero() };
result = (self, new_transcript);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks down if the z coordinate of the inputs are not 1.

@TomAFrench
Copy link
Member

I'm surprised that there isn't a diff on the brillig circuit sizes being shown on this PR now. We should be removing a significant number of brillig opcodes.

@TomAFrench
Copy link
Member

Looking at this PR vs https://noir-lang.github.io/noir_bigcurve/dev/bench/, we're cutting the number of brillig opcodes for an add by ~10% but this isn't getting a comment being posted on this PR.

@TomAFrench
Copy link
Member

10% is pretty chunky though! We should follow this up with mul once this is merged.

@jialinli98
Copy link
Contributor Author

Looking at this PR vs https://noir-lang.github.io/noir_bigcurve/dev/bench/, we're cutting the number of brillig opcodes for an add by ~10% but this isn't getting a comment being posted on this PR.

We set alert-threshold to be 101%, so a comment will only get posted if performance regresses :(

@TomAFrench
Copy link
Member

Ahh, ok. I saw

- name: Add brillig diff to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
# delete the comment in case changes no longer impact circuit sizes
delete: ${{ !steps.brillig_diff.outputs.markdown }}
and was expecting the old style benchmark comments. We should remove that step as it's not doing anything.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TomAFrench TomAFrench merged commit 1ac3233 into main Jul 16, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 To Triage to ✅ Done in Noir Libraries Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

Avoid creating transcript when performing unconstrained point addition

4 participants