Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ultrahonk finally #100

Merged
merged 12 commits into from
Dec 19, 2024
Merged

ultrahonk finally #100

merged 12 commits into from
Dec 19, 2024

Conversation

signorecello
Copy link
Collaborator

This is a working version for Noir 1.0.0-beta.0 using Barretenberg v0.63.0

Uses ultrahonk which is currently slightly broken on the verifier contract generation on the bb side. Commenting out the test until we figure this out (interface won't change)

@signorecello
Copy link
Collaborator Author

cc @olehmisar thanks for all your work with the hardhat plugin!

Copy link
Collaborator

@critesjosh critesjosh left a comment

Choose a reason for hiding this comment

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

When going through the vite-hardhat README I am seeing this when running bunx hardhat node . when I resolve this i get other errors about missing packages

The README needs to updated

Screenshot from 2024-12-18 10-37-51

@critesjosh
Copy link
Collaborator

Should we note the tradeoffs between ultraplonk and ultrahonk in the README? Or are we planning to drop UP altogether so should just forget about it?

@signorecello
Copy link
Collaborator Author

We're looking into dropping UP but let's see, depends if we're gonna optimize UH verifier contract or not... still up for debate I guess

Copy link
Collaborator

@critesjosh critesjosh left a comment

Choose a reason for hiding this comment

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

Would be good to make it more obvious that UH on-chain verification isn't working, but otherwise looks good!

@Savio-Sou
Copy link
Member

Re UltraPlonk and UltraHonk:

@Savio-Sou
Copy link
Member

This PR resolves #97

@critesjosh
Copy link
Collaborator

Makes sense, thanks @Savio-Sou. We were discussing also switching back to UP because the UH verifier is broken, so we could have a fully functioning example.

What do you think, should we merge it with UH, make a note that the UH verifier is being worked on and will be updated soon?

@Savio-Sou
Copy link
Member

Savio-Sou commented Dec 18, 2024

Merging with UH + note sounds reasonable.

Which / Do we have an Issue for UH's broken verifier? We should prioritize fixing it asap in parallel.

nit
@signorecello
Copy link
Collaborator Author

Just because I was already working on it, I'm pushing a refactor that allows for both UltraPlonk and UltraHonk. It also tests both, but skips the verifier contract for UltraHonk.

@signorecello
Copy link
Collaborator Author

Which / Do we have an Issue for UH's broken verifier? We should prioritize fixing it asap in parallel.

We don't, I have a pretty good idea of what could be missing so I'm looking into it so I can provide more context to the team

ci
nit
@signorecello signorecello merged commit a96c17d into main Dec 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants