Skip to content

fix: don't disallow writing to memory after passing it to brillig#8276

Merged
TomAFrench merged 3 commits intomasterfrom
tf/fix-circuit-simulator
Apr 29, 2025
Merged

fix: don't disallow writing to memory after passing it to brillig#8276
TomAFrench merged 3 commits intomasterfrom
tf/fix-circuit-simulator

Conversation

@TomAFrench
Copy link
Member

@TomAFrench TomAFrench commented Apr 29, 2025

Description

Problem*

Resolves

Summary*

This fixes an issue that @MirandaWood was running into on her batched blobs branch.

@kashbrti and I found the root cause is that if a memory block is passed as an argument to a brillig call then the CircuitSimulator marks it as "used" at which point it will reject any future writes to that memory block. I don't see any reason for why the simulator should reject this so I've replaced this logic with a simple check to ensure that we don't perform any reads/writes from uninitialized memory blocks.

@guipublic I think you originally wrote this logic, can you explain the motivation behind the old behaviour in case I'm missing something?

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

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

@TomAFrench TomAFrench requested a review from a team April 29, 2025 14:50
@TomAFrench TomAFrench enabled auto-merge April 29, 2025 18:56
Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

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

LGTM w/ a few notes:

  1. Should we note this behavior in the BrilligInputs docs?
    pub enum BrilligInputs<F> {
  2. Is the original example too complex for a Noir-level regression test?

@TomAFrench TomAFrench added this pull request to the merge queue Apr 29, 2025
Merged via the queue into master with commit 4bc636a Apr 29, 2025
116 checks passed
@TomAFrench TomAFrench deleted the tf/fix-circuit-simulator branch April 29, 2025 19:43
@TomAFrench
Copy link
Member Author

Should we note this behavior in the BrilligInputs docs?

I'm not clear what behaviour you mean, the ability to pass a memory block as input?

Is the original example too complex for a Noir-level regression test?

We could construct one but I think a more focused test is better.

@michaeljklein
Copy link
Contributor

I'm not clear what behaviour you mean, the ability to pass a memory block as input?

Mainly how the initialization works

github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 1, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: add `--debug-compile-stdin` to read `main.nr` from `STDIN` for
testing (noir-lang/noir#8253)
feat: better error message on unicode whitespace that isn't ascii
whitespace (noir-lang/noir#8295)
chore: update `quicksort` from iterative `noir_sort` version
(noir-lang/noir#7348)
fix: use correct meta attribute names in contract custom attributes
(noir-lang/noir#8273)
feat: `nargo expand` to show code after macro expansions
(noir-lang/noir#7613)
feat: allow specifying fuzz-related dirs when invoking `nargo test`
(noir-lang/noir#8293)
chore: redo typo PR by ciaranightingale
(noir-lang/noir#8292)
chore: Extend the bug list with issues found by the AST fuzzer
(noir-lang/noir#8285)
fix: don't disallow writing to memory after passing it to brillig
(noir-lang/noir#8276)
chore: test against zkpassport rsa lib
(noir-lang/noir#8278)
feat: omit element size array for more array types
(noir-lang/noir#8257)
chore: refactor array handling in ACIRgen
(noir-lang/noir#8256)
chore: document cast (noir-lang/noir#8268)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
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.

2 participants