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!: Disable mocks in execute #6869

Merged
merged 12 commits into from
Jan 10, 2025
Merged

feat!: Disable mocks in execute #6869

merged 12 commits into from
Jan 10, 2025

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Dec 19, 2024

Description

Problem*

Follow up for #6857

Summary*

Disables the MockForeignCallExecutor in nargo execute by returning a new ForeignCallError::Disabled. Also disables it in the ACVM execution.

Additional Context

It looks like std::test::OracleMock is part of the Noir standard library regardless of whether we called nargo execute or nargo test, and as such we can create mocks in any program, e.g. the ones under test_programs/execution_success. AFAIU the intended use is for them to be used in Noir #[test] functions, to facilitate oracle testing in aztec-packages, and it wouldn't make sense for them to be used in production code. But if they were, I don't see anything that would prevent them from hijacking real oracle calls, which sounds to me like a security hole.

I thought about adding a CLI argument to ExecuteCmd to force-enable them, but I can't see under what conditions one wants to test these where they can't write a #[test]. Let me know if that is a use case.

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.

Copy link
Contributor

github-actions bot commented Dec 19, 2024

Peak Memory Sample

Program Peak Memory %
keccak256 78.15M 0%
workspace 122.67M -1%
regression_4709 423.26M 0%
ram_blowup_regression 1.58G 0%
private-kernel-tail 206.71M -1%
private-kernel-reset 720.29M 0%
private-kernel-inner 291.91M -1%
parity-root 171.71M -1%

@aakoshh aakoshh changed the base branch from master to 6742-jsonrpsee December 19, 2024 15:12
Copy link
Contributor

github-actions bot commented Dec 19, 2024

Execution Report

Program Execution Time %
sha256_regression 0.054s -2%
regression_4709 0.001s 0%
ram_blowup_regression 0.582s 1%
rollup-base-public 1.248s 1%
rollup-base-private 0.469s 0%
private-kernel-tail 0.022s -9%

Copy link
Contributor

github-actions bot commented Dec 19, 2024

Compilation Report

Program Compilation Time %
sha256_regression 1.272s -3%
regression_4709 0.775s -5%
ram_blowup_regression 18.060s -2%
rollup-base-public 40.280s 2%
rollup-base-private 12.820s 2%
private-kernel-tail 1.006s 2%

@aakoshh aakoshh marked this pull request as ready for review December 19, 2024 16:05
@aakoshh aakoshh requested a review from a team December 19, 2024 16:06
Base automatically changed from 6742-jsonrpsee to master January 2, 2025 16:40
@aakoshh aakoshh force-pushed the 6742-optional-mock branch from 1ef9c75 to 941415a Compare January 6, 2025 09:57
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Compilation Memory Report

Program Peak Memory
keccak256 78.12M
workspace 123.88M
regression_4709 422.98M
ram_blowup_regression 1.58G
rollup-base-public 4.85G
rollup-base-private 1.26G
private-kernel-tail 207.09M
private-kernel-reset 730.51M
private-kernel-inner 294.32M

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Execution Memory Report

Program Peak Memory
keccak256 74.67M
workspace 123.47M
regression_4709 316.00M
ram_blowup_regression 512.42M
rollup-base-public 482.47M
rollup-base-private 328.59M
private-kernel-tail 180.94M
private-kernel-reset 249.61M
private-kernel-inner 209.78M

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 with small comment r.e. unused with_mocks

tooling/nargo/src/foreign_calls/default.rs Show resolved Hide resolved
@aakoshh aakoshh enabled auto-merge January 10, 2025 16:09
@aakoshh aakoshh added this pull request to the merge queue Jan 10, 2025
Merged via the queue into master with commit e71fcdf Jan 10, 2025
83 of 87 checks passed
@aakoshh aakoshh deleted the 6742-optional-mock branch January 10, 2025 16:31
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 10, 2025
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 10, 2025
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
TomAFrench added a commit that referenced this pull request Jan 10, 2025
* master:
  chore: simplify a couple of enum variants (#7025)
  chore: disallow inserting ACIR-only instructions into brillig functions (#7017)
  feat: unchecked math operations in SSA (#7011)
  chore: delete docs for versions which aren't used (#7020)
  feat!: Disable mocks in `execute` (#6869)
  chore(docs): backport 1.0.0-beta.0 doc fixes (#7014)
  feat(cli): Add CLI option to filter by contract function name (#7018)
  chore: Add more Field use info (#7019)
  fix: let static_assert fail with the provided message (#7005)
  chore: mark `aztec-nr` as expected to compile (#7015)
  chore: clarity fix in docs (#7016)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 11, 2025
…ang/noir#6985)

feat!: disallow calling unconstrained functions outside of `unsafe` blocks and passing unconstrained functions in place of constrained functions (noir-lang/noir#6938)
chore: simplify a couple of enum variants (noir-lang/noir#7025)
chore: disallow inserting ACIR-only instructions into brillig functions (noir-lang/noir#7017)
feat: unchecked math operations in SSA (noir-lang/noir#7011)
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 11, 2025
feat!: disallow calling unconstrained functions outside of `unsafe` blocks and passing unconstrained functions in place of constrained functions (noir-lang/noir#6938)
chore: simplify a couple of enum variants (noir-lang/noir#7025)
chore: disallow inserting ACIR-only instructions into brillig functions (noir-lang/noir#7017)
feat: unchecked math operations in SSA (noir-lang/noir#7011)
chore: delete docs for versions which aren't used (noir-lang/noir#7020)
feat!: Disable mocks in `execute` (noir-lang/noir#6869)
chore(docs): backport 1.0.0-beta.0 doc fixes (noir-lang/noir#7014)
feat(cli): Add CLI option to filter by contract function name (noir-lang/noir#7018)
chore: Add more Field use info (noir-lang/noir#7019)
fix: let static_assert fail with the provided message (noir-lang/noir#7005)
chore: mark `aztec-nr` as expected to compile (noir-lang/noir#7015)
chore: clarity fix in docs (noir-lang/noir#7016)
fix: require generic trait impls to be in scope to call them (noir-lang/noir#6913)
feat!: require trait primitive functions/calls to have their trait in scope (noir-lang/noir#6901)
feat(lsp): use trait method docs for trait impl method docs on hover (noir-lang/noir#7003)
chore: turn on averaging for protocol circuits metrics in CI (noir-lang/noir#6999)
feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (noir-lang/noir#7008)
chore: Add short circuit in ssa-gen for known if conditions (noir-lang/noir#7007)
chore: Only resolved globals monomorphization (noir-lang/noir#7006)
chore: Remove resolve_is_unconstrained pass (noir-lang/noir#7004)
chore: require safety doc comment for unsafe instead of `//@safety` (noir-lang/noir#6992)
fix: Reproduce and fix bytecode blowup (noir-lang/noir#6972)
chore: mark casts as able to be deduplicated (noir-lang/noir#6996)
fix: return trait impl method as FuncId if there's only one (noir-lang/noir#6989)
chore(ci): fail properly in `external-repo-checks` (noir-lang/noir#6988)
fix: allow multiple trait impls for the same trait as long as one is in scope (noir-lang/noir#6987)
chore: Use DFG in SSA printer (noir-lang/noir#6986)
chore!: Reserve `enum` and `match` keywords (noir-lang/noir#6961)
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.

3 participants