Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat(acir)!: Add predicate to MemoryOp #503

Merged
merged 10 commits into from
Aug 30, 2023
Merged

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

Resolves noir-lang/noir#2133

Summary*

We are executing memory operations even under a zero predicate. This leads to situations where the VM will error with index out of bounds even if the memory operation is under a zero predicate.

Additional Context

PR Checklist*

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

acvm/src/pwg/memory_op.rs Outdated Show resolved Hide resolved
@sirasistant
Copy link
Contributor

I think we might need to update this file https://github.com/noir-lang/acvm/blob/master/acir/tests/test_program_serialization.rs to also test serialization of memory ops. I think this PR needs to update the test cases for JS as well, but since the test doesn't use memory ops, it doesn't alert the dev

Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

We still need to encode the predicate in the circuit which could be done on the bb side. For now, its just a serialization change and no logic will be required in the cpp to be changed (ie it ignores the predicate) and we will open up an issue about predicate encoding -- It could also be done on the Noir side, so it requires a bit more discussion

@vezenovm
Copy link
Contributor Author

I think we might need to update this file https://github.com/noir-lang/acvm/blob/master/acir/tests/test_program_serialization.rs to also test serialization of memory ops.

I don't see MemoryOps being used anywhere in this test?

@TomAFrench
Copy link
Member

Alvaro is saying that we should add a test which does use MemoryOps to warn if their serialization changes.

@vezenovm
Copy link
Contributor Author

Alvaro is saying that we should add a test which does use MemoryOps to warn if their serialization changes.

Will add a test

@vezenovm
Copy link
Contributor Author

We still need to encode the predicate in the circuit which could be done on the bb side. For now, its just a serialization change and no logic will be required in the cpp to be changed (ie it ignores the predicate) and we will open up an issue about predicate encoding -- It could also be done on the Noir side, so it requires a bit more discussion

noir-lang/noir#2493

@kevaundray kevaundray added this pull request to the merge queue Aug 30, 2023
Merged via the queue into master with commit ca9eebe Aug 30, 2023
13 checks passed
@TomAFrench TomAFrench deleted the mv/mem-op-predicate branch August 30, 2023 21:32
TomAFrench added a commit that referenced this pull request Aug 30, 2023
* master:
  feat(acir)!: Add predicate to MemoryOp (#503)
  chore(acvm)!: Remove unused arguments from `Backend` trait (#511)
  feat!: Assertion messages embedded in the circuit (#484)
@kevaundray kevaundray mentioned this pull request Aug 30, 2023
Sakapoi pushed a commit to Sakapoi/acvm_fork that referenced this pull request Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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