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

feat(acir_gen): Use predicates with MemoryOps #2400

Closed
wants to merge 12 commits into from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Aug 22, 2023

Description

Problem*

Resolves #2133

Summary*

The ACVM was missing a predicate as part of MemoryOps, thus we were pushing MemoryOps in ACIR gen without their predicate. This caused memory operations under dynamic predicates to be executed no matter what. This is a bug as we can error out with an index out of bounds for valid execution. The linked issue has a full example of this and I have expanded the dynamic_index_failure test:

fn main(mut x: [u32; 5], z: Field) {
    let idx = z + 10;
    
    if z == 20 {
        x[0] = x[4];
    } else {
        // We should not hit out of bounds here as we have a predicate
        // that should not be hit
        if idx as u32 < 3 {
            x[idx] = 10;
        }
    }
    // Error should occur here as an index out of bounds read occurs 
    assert(x[idx] != 0);
}

Patches:
Backend branch: noir-lang/acvm-backend-barretenberg#247
ACVM: noir-lang/acvm#503

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@jfecher
Copy link
Contributor

jfecher commented Aug 22, 2023

Waiting to review this until the other PR on mv/mem-op-predicate is merged

Base automatically changed from mv/dyn-array-index to master August 22, 2023 18:58
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

This is currently using a patched ACVM and should not be merged until we've included the changes in a release.

@kevaundray
Copy link
Contributor

What is the status of this PR?

@vezenovm
Copy link
Contributor Author

vezenovm commented Sep 5, 2023

This PR was originally created to avoid the execution error in ACVM with mem ops and predicates. This should be superseded by #2493. As per #2493 (comment) we should be able to then remove the inclusion of a predicate on the MemoryOp ACIR opcode as well.

@vezenovm
Copy link
Contributor Author

vezenovm commented Sep 5, 2023

Superseded by #2553

@vezenovm vezenovm closed this Sep 5, 2023
@TomAFrench TomAFrench deleted the mv/mem-op-predicate branch September 5, 2023 22:46
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.

Noir program attempts to read uninitialised memory
4 participants