noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
874 stars 189 forks source link

Content of a struct gets "destroyed" #5771

Open benesjan opened 1 month ago

benesjan commented 1 month ago

Aim

I was just working on an arbitrary task in aztec-nr and my code started to fail in an e2e test because a value in a struct was suddently zero.

Expected Behavior

Struct should not get reset to "zero".

Bug

A content of a struct gets reset to zero. A bug will be obvious when running the reproduction

To Reproduce (updated on 30.9.2024)

  1. git clone https://github.com/AztecProtocol/aztec-packages.git
  2. cd aztec-packages
  3. git fetch origin janb/context-bug-reproduction && git checkout janb/context-bug-reproduction
  4. cd noir-projects/noir-contracts/contracts/test_contract
  5. nargo execute --package test_contract --silence-warnings

Once you run these commands you should get the following error:

image

To Reproduce (original occurrence)

  1. git clone https://github.com/AztecProtocol/aztec-packages.git
  2. cd aztec-packages
  3. git fetch origin dereferencing-bug && git checkout dereferencing-bug
  4. cd noir-projects/noir-contracts/contracts/token_with_refunds_contract
  5. nargo execute --package token_with_refunds_contract --silence-warnings

Once you run these commands you should get the following error:

image

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

Blocker

Blocker Context

It prevents merging of this PR

Nargo Version

nargo version = 0.33.0 noirc version = 0.33.0+26b87408b9360b55a59ed1589a0275dfc5ef9a02 (git version hash: 26b87408b9360b55a59ed1589a0275dfc5ef9a02, is dirty: false)

NoirJS Version

No response

Proving Backend Tooling & Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

TomAFrench commented 1 month ago

This branch is very out of date with the current master branch of aztec-packages so migrating it across to this repository is then difficult. Can you update this so that it pulls aztec-nr from a fixed commit from https://github.com/AztecProtocol/aztec-nr?

benesjan commented 1 month ago

@TomAFrench pointing it to a specific version in aztec-nr repo is not possible because I've modified aztec-nr to not do oracle calls. But I've just merged the branch with master. Is that enough?

jfecher commented 1 month ago

When I tested this I just had to replace each std::unsafe::zeroed path with std::mem::zeroed

jfecher commented 4 weeks ago

We had some breaking changes recently with to_be_bytes getting its argument removed. I fixed up the uses there and pushed an update to the dereferencing-bug branch but now with a more recent version of the compiler I'm getting a different error.

Originally after I made the changes I was getting a Could not resolve some references to the array. All references must be resolved at compile time error, although after changing some of the code I'm no longer getting this and can't replicate it. Now I'm getting a panic:

The application panicked (crashed).
Message:  internal error: entered unreachable code: No size for slice v701 = Instruction { instruction: Id(990), position: 0, typ: Reference(Reference(Array([Numeric(NativeField), Numeric(Unsigned { bit_size: 32 }), Numeric(NativeField)], 16))) }
Location: compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs:158

Seems to occur with or without --show-ssa so it is probably not a result of the new normalize values pass.

benesjan commented 3 weeks ago

@vezenovm @jfecher I have bad news. Rebased my PR on master which has Noir with Maxim's commit and the issue just occurs in a different place. Do you want me to create another reproduction?

jfecher commented 3 weeks ago

@benesjan Ah, yes I was worried about that :sweat_smile:. That'd be great if you could, thank you

benesjan commented 5 days ago

@jfecher finally got to this. I updated the reproduction description above (janb/context-bug-reproduction branch).

The behavior was a bit different this time. Now it didn't nuke the whole context but instead it just reset the side_effects_counter to 0. This should never happen as the counter is always just incremented. I would say it's the same bug because it occurred in a similar situation.