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

Nargo fails to verify the proof (repro attached) #4443

Closed
LogvinovLeon opened this issue Feb 28, 2024 · 7 comments · Fixed by #4951
Closed

Nargo fails to verify the proof (repro attached) #4443

LogvinovLeon opened this issue Feb 28, 2024 · 7 comments · Fixed by #4951
Assignees
Labels
bug Something isn't working

Comments

@LogvinovLeon
Copy link
Contributor

LogvinovLeon commented Feb 28, 2024

Aim

I would like to be able to generate a proof and then verify it

Expected Behavior

Nargo verify succeeds after

Bug

Nargo verfiy fails

To Reproduce

  1. Install nargo 0.24.0. noirup --version 0.24.0
  2. git clone [email protected]:vlayer-xyz/nargo-verify-repro.git
  3. cd nargo-verify-repro
  4. nargo prove
  5. nargo verify - Fails

Project Impact

Blocker

Impact Context

It's a blocker because nargo generates proofs that are invalid.

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

Binary (noirup default)

Nargo Version

noirc version = 0.24.0+df04ce9d72581416665e7e420d188d61d6cdc529

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@LogvinovLeon LogvinovLeon added the bug Something isn't working label Feb 28, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Feb 28, 2024
@TomAFrench
Copy link
Member

I can reproduce this on master

@vezenovm
Copy link
Contributor

vezenovm commented Mar 8, 2024

@LogvinovLeon Would you be able to link the repo in the issue so we don't have to go searching for it?

@LogvinovLeon
Copy link
Contributor Author

It is linked in the repro steps, but here is the link one more time: [email protected]:vlayer-xyz/nargo-verify-repro.git
Or http version: https://github.com/vlayer-xyz/nargo-verify-repro

@LogvinovLeon
Copy link
Contributor Author

Here is it's contents inline:

global number = 14194126;
global hash = 0;

global block_header_partial = BlockHeaderPartial { number, hash};

struct BlockHeaderPartial {
    number: Field,
    hash: Field,
}

fn main(block_no: pub Field) -> pub BlockHeaderPartial {
    let block_header_partial = get_header_unconstrained(block_no);
    assert(block_header_partial.number == block_no);
    block_header_partial
}

unconstrained fn get_header_unconstrained(block_no: Field) -> BlockHeaderPartial {
    block_header_partial
}

@LogvinovLeon
Copy link
Contributor Author

Is there any workaround around this? This is making it impossible for us to enable all assertions we want on our system and therefore - the proofs we're generating do not satisfy soundness. So for us - we can't fix our critical security vulnerability before this is fixed. (although we're not live yet).

Also - the optics on this are pretty bad as it's not an issue in noir-js or solidity but core nargo.

@guipublic
Copy link
Contributor

I have been able to reproduce this with an even smaller example:

fn main(block_no: pub Field) -> pub [Field;2] {
   [block_no, 0]
}

The next step for me is to reproduce it within a Barretenberg test case, which I have not been able to do yet.

@Savio-Sou Savio-Sou moved this from 📋 Backlog to 🏗 In progress in Noir Mar 15, 2024
@vezenovm
Copy link
Contributor

@guipublic The basic reproduction looks like we are returning the same input witness as output, so I tried adding a distinct keyword.

fn main(block_no: pub Field) -> distinct pub [Field;2] {
   [block_no, 0]
}

This successfully proves and verifies for me. Adding distinct to the program output @LogvinovLeon posted above also successfully verified for me. For now this is the workaround @LogvinovLeon.

This verify false is still deceiving though and we should improve this handling of duplicate inputs/outputs. @guipublic This should help for a barretenberg test case as well.

@TomAFrench TomAFrench moved this from 🏗 In progress to 📋 Backlog in Noir Apr 19, 2024
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
bug Something isn't working
Projects
Archived in project
4 participants