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

Make return values distinct by default #4914

Closed
Savio-Sou opened this issue Apr 24, 2024 · 3 comments · Fixed by #4951
Closed

Make return values distinct by default #4914

Savio-Sou opened this issue Apr 24, 2024 · 3 comments · Fixed by #4951
Assignees
Labels
enhancement New feature or request

Comments

@Savio-Sou
Copy link
Collaborator

Problem

Return values of Noir programs currently require opt-in to be made distinct.

Albeit not-distinct could yield less constraint counts for certain programs, the way it optimizes and reorders the witness during compilation isn't necessary intuitive for developers to comprehend versus how public inputs and return values are defined in their Noir programs / Verifier.toml.

This in turn leads to potential confusions and footguns with how public values are generated and serialized, and by extension how to use them with proofs for verification.

For example:

Happy Case

Make the existing distinct behavior the default and remove the keyword.

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

Credit to @PhilWindle for flagging the footgun.

I don't personally know how much of a performance gain does not-distinct give, hence if it's justified to come up with a new keyword for developers to opt into that.

Insights welcome below.

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@Savio-Sou Savio-Sou added the enhancement New feature or request label Apr 24, 2024
@Savio-Sou Savio-Sou added this to Noir Apr 24, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 24, 2024
@Savio-Sou
Copy link
Collaborator Author

@TomAFrench mentioned we already have an abi-decoding tool; a better approach might actually be documenting and bringing awareness to the tool.

@TomAFrench to investigate.

@sirasistant
Copy link
Contributor

sirasistant commented Apr 30, 2024

Using abi encoding and decoding in nargo/JS is fine, but in solidity and recursive circuits this problem is unsolved I think.

In solidity and recursive circuits I think devs want to treat the circuit public inputs as structured data to make some checks on it, and then transform it into the public inputs vector for verification of the circuit. I think we'd need a solidity "noir ABI" encoder and a noir "noir ABI" encoder which adds a lot of complexity vs just serializing the data structure to an array of fields.

@TomAFrench
Copy link
Member

Agreed, I've been doing some thinking as well on the recursion issue and come to the same conclusion. Between that and a couple of other issues related to return witnesses, it's time make this the default.

There's some optimisations we can perform the avoid the constraints just to renumber the witnesses into the correct order but fundamentally we should be discouraging users from excessive returns from their programs where it can be shown that they're unnecessary.

github-merge-queue bot pushed a commit that referenced this issue May 1, 2024
# Description

## Problem\*

Resolves #4914
Resolves #4443

## Summary\*

We're starting to need various workarounds for non-distinct witnesses to
the point where it's more of a hindrance than it's worth. This PR makes
the `distinct` behaviour the default and raises a parser error when
using the `distinct` keyword.

I've gone for a hard error rather than a warning as the only place where
`distinct` can be used is on `main` and so there's no risk of breaking
libraries. Affected users can simply modify their own binary project to
fix the issue.

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] 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](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants