noir-lang / noir

Noir is a domain specific language for zero knowledge proofs

https://noir-lang.org

Apache License 2.0

871 stars 188 forks source link

fix: fix scalar mul in stdlib/ec #5279

Closed guipublic closed 2 months ago

guipublic commented 3 months ago

Description

Problem*

Resolves #5259

Summary*

refactor the square and multiply

Additional Context

Documentation*

Check one:

[X] No documentation needed.
[ ] Documentation included in this PR.
[ ] [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

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

TomAFrench commented 3 months ago

We have this same code repeated in the other ec submodules so we should make those changes there as well.

guipublic commented 3 months ago

I fixed the code also in tecurve.rs, I did not find another instance.

github-actions[bot] commented 3 months ago

Changes to circuit sizes

Generated at commit: 18c1d3f31a324e87d526d0156050a83ed24fe091, compared to commit: e1000176a31140b2abd79c47653cbc4bb1a6808a

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
eddsa	+2,980 ❌	+4.23%	+2,980 ❌	+4.15%
bench_eddsa_poseidon	+745 ❌	+4.16%	+745 ❌	+3.59%

Full diff report 👇

| Program | ACIR opcodes (+/-) | % | Circuit size (+/-) | % | |:-|-:|-:|-:|-:| | **eddsa** | 73,439 (+2,980) | **+4.23%** | 74,812 (+2,980) | **+4.15%** | | **bench_eddsa_poseidon** | 18,667 (+745) | **+4.16%** | 21,509 (+745) | **+3.59%** |

TomAFrench commented 3 months ago

I think we can leave this until Zac's improved implemention is ready. This also causes a performance degradation it seems.

Savio-Sou commented 2 months ago

Zac's improved implemention

Is that on a branch / PR?

TomAFrench commented 2 months ago

That's in his separate repository that he's been posting in slack.

Savio-Sou commented 2 months ago

Oh https://github.com/zac-williamson/noir_bigcurve? Thanks