chore: re-enable some avm proving tests

Author: dbanks12

noir-projects/aztec-nr/aztec/src/context/public_context.nr

@@ -881,22 +881,15 @@ pub unconstrained fn calldata_copy<let N: u32>(cdoffset: u32, copy_size: u32) ->
 }
 
 // `success_copy` is placed immediately after the CALL opcode to get the success value
-//
-// This function is temporarily exposed publicly to be able to test it in AVMTest contract.
-// TODO: Refactor tests to keep this implementation detail private within the crate.
-pub unconstrained fn success_copy() -> bool {
+unconstrained fn success_copy() -> bool {
     success_copy_opcode()
 }
 
-// This function is temporarily exposed publicly to be able to test it in AVMTest contract.
-// TODO: Refactor tests to keep this implementation detail private within the crate.
-pub unconstrained fn returndata_size() -> u32 {
+unconstrained fn returndata_size() -> u32 {
     returndata_size_opcode()
 }
 
-// This function is temporarily exposed publicly to be able to test it in AVMTest contract.
-// TODO: Refactor tests to keep this implementation detail private within the crate.
-pub unconstrained fn returndata_copy(rdoffset: u32, copy_size: u32) -> [Field] {
+unconstrained fn returndata_copy(rdoffset: u32, copy_size: u32) -> [Field] {
     returndata_copy_opcode(rdoffset, copy_size)
 }
 
@@ -908,10 +901,7 @@ pub unconstrained fn avm_return(returndata: [Field]) {
 // to do rethrows, where the revert data is the same as the original revert data.
 // For normal reverts, use Noir's `assert` which, on top of reverting, will also add
 // an error selector to the revert data.
-//
-// This function is temporarily exposed publicly to be able to test it in AVMTest contract.
-// TODO: Refactor tests to keep this implementation detail private within the crate.
-pub unconstrained fn avm_revert(revertdata: [Field]) {
+unconstrained fn avm_revert(revertdata: [Field]) {
     revert_opcode(revertdata)
 }
 

noir-projects/noir-contracts/contracts/test/avm_test_contract/src/fake_public_context.nr

@@ -0,0 +1,53 @@
+use dep::aztec::protocol_types::address::AztecAddress;
+
+pub unconstrained fn call(
+    l2_gas_allocation: u32,
+    da_gas_allocation: u32,
+    address: AztecAddress,
+    args: [Field],
+) {
+    call_opcode(l2_gas_allocation, da_gas_allocation, address, args)
+}
+
+pub unconstrained fn success_copy() -> bool {
+    success_copy_opcode()
+}
+
+pub unconstrained fn returndata_size() -> u32 {
+    returndata_size_opcode()
+}
+
+pub unconstrained fn returndata_copy(rdoffset: u32, copy_size: u32) -> [Field] {
+    returndata_copy_opcode(rdoffset, copy_size)
+}
+
+pub unconstrained fn avm_return(returndata: [Field]) {
+    return_opcode(returndata)
+}
+
+pub unconstrained fn avm_revert(revertdata: [Field]) {
+    revert_opcode(revertdata)
+}
+
+#[oracle(avmOpcodeCall)]
+unconstrained fn call_opcode(
+    l2_gas_allocation: u32,
+    da_gas_allocation: u32,
+    address: AztecAddress,
+    args: [Field],
+) {}
+
+#[oracle(avmOpcodeSuccessCopy)]
+unconstrained fn success_copy_opcode() -> bool {}
+
+#[oracle(avmOpcodeReturndataSize)]
+unconstrained fn returndata_size_opcode() -> u32 {}
+
+#[oracle(avmOpcodeReturndataCopy)]
+unconstrained fn returndata_copy_opcode(rdoffset: u32, copy_size: u32) -> [Field] {}
+
+#[oracle(avmOpcodeReturn)]
+unconstrained fn return_opcode(returndata: [Field]) {}
+
+#[oracle(avmOpcodeRevert)]
+unconstrained fn revert_opcode(revertdata: [Field]) {}

noir-projects/noir-contracts/contracts/test/avm_test_contract/src/main.nr

@@ -1,4 +1,5 @@
 mod note;
+mod fake_public_context;
 use dep::aztec::macros::aztec;
 
 #[aztec]
@@ -11,12 +12,16 @@ pub contract AvmTest {
         0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef;
 
     // Libs
+    use crate::fake_public_context::{
+        avm_return, avm_revert, call, returndata_copy, returndata_size, success_copy,
+    };
     use dep::aztec::context::gas::GasOpts;
     use dep::aztec::macros::{functions::{private, public}, storage::storage};
     use dep::aztec::oracle::get_contract_instance::{
         get_contract_instance_class_id_avm, get_contract_instance_deployer_avm,
         get_contract_instance_initialization_hash_avm,
     };
+    use dep::aztec::protocol_types::abis::function_selector::FunctionSelector;
     use dep::aztec::protocol_types::{
         address::{AztecAddress, EthAddress},
         point::Point,
@@ -26,7 +31,7 @@ pub contract AvmTest {
         constants::{GRUMPKIN_ONE_X, GRUMPKIN_ONE_Y, MAX_PUBLIC_CALLS_TO_UNIQUE_CONTRACT_CLASS_IDS},
         contract_class_id::ContractClassId,
         storage::map::derive_storage_slot_in_map,
-        traits::{Empty, FromField},
+        traits::{Empty, FromField, ToField},
     };
     use dep::aztec::state_vars::Map;
     use dep::aztec::state_vars::PublicMutable;
@@ -267,35 +272,34 @@ pub contract AvmTest {
         let _ = AvmTest::at(context.this_address()).divide_by_zero().call(&mut context);
     }
 
-    // TODO(#16099): Re-enable this test
-    // #[public]
-    // fn external_call_to_divide_by_zero_recovers() {
-    //     // Be sure to allocate ~200k+ gas to this function~
-
-    //     // Get the gas remaining and allocate some smaller amount to nested call.
-    //     // We don't want to allocate too much to the nested call
-    //     // since it will all be consumed on exceptional halt.
-    //     let l2_gas_left = context.l2_gas_left();
-    //     let da_gas_left = context.da_gas_left();
-    //     let selector = FunctionSelector::from_signature("divide_by_zero()");
-
-    //     // Call without capturing a return value since call no longer returns success
-    //     call(
-    //         l2_gas_left - 200_000,
-    //         da_gas_left - 200_000,
-    //         context.this_address(),
-    //         &[selector.to_field()],
-    //     );
-
-    //     // Use SUCCESSCOPY to get the success status
-    //     let success = success_copy();
-
-    //     assert(!success, "Nested CALL instruction should return failure on exceptional halt");
-    //     assert(
-    //         returndata_size() == 0,
-    //         "Returndata should be empty when nested call exceptionally halts",
-    //     );
-    // }
+    #[public]
+    fn external_call_to_divide_by_zero_recovers() {
+        // Be sure to allocate ~200k+ gas to this function~
+
+        // Get the gas remaining and allocate some smaller amount to nested call.
+        // We don't want to allocate too much to the nested call
+        // since it will all be consumed on exceptional halt.
+        let l2_gas_left = context.l2_gas_left();
+        let da_gas_left = context.da_gas_left();
+        let selector = FunctionSelector::from_signature("divide_by_zero()");
+
+        // Call without capturing a return value since call no longer returns success
+        call(
+            l2_gas_left - 200_000,
+            da_gas_left - 200_000,
+            context.this_address(),
+            &[selector.to_field()],
+        );
+
+        // Use SUCCESSCOPY to get the success status
+        let success = success_copy();
+
+        assert(!success, "Nested CALL instruction should return failure on exceptional halt");
+        assert(
+            returndata_size() == 0,
+            "Returndata should be empty when nested call exceptionally halts",
+        );
+    }
 
     #[public]
     fn debug_logging() {
@@ -321,20 +325,20 @@ pub contract AvmTest {
     #[public]
     fn returndata_copy_oracle() {
         let _ = AvmTest::at(context.this_address()).return_oracle().call(&mut context);
-        let returndatasize = dep::aztec::context::public_context::returndata_size();
-        let returndata = dep::aztec::context::public_context::returndata_copy(0, returndatasize);
+        let returndatasize = returndata_size();
+        let returndata = returndata_copy(0, returndatasize);
         assert(returndata == &[1, 2, 3], "Returndata copy failed");
     }
 
     #[public]
     fn return_oracle() -> [Field; 3] {
-        dep::aztec::context::public_context::avm_return([1, 2, 3]);
+        avm_return([1, 2, 3]);
         [4, 5, 6] // Should not get here.
     }
 
     #[public]
     fn revert_oracle() -> [Field; 3] {
-        dep::aztec::context::public_context::avm_revert([1, 2, 3]);
+        avm_revert([1, 2, 3]);
         [4, 5, 6] // Should not get here.
     }
 
@@ -557,17 +561,16 @@ pub contract AvmTest {
         AvmTest::at(garbageAddress).nested_call_to_nothing().call(&mut context)
     }
 
-    // TODO(#16099): Re-enable this test
-    // #[public]
-    // fn nested_call_to_nothing_recovers() {
-    //     let garbageAddress = AztecAddress::from_field(42);
-    //     call(1, 1, garbageAddress, &[]);
-    //     let success = success_copy();
-    //     assert(
-    //         !success,
-    //         "Nested CALL instruction should return failure if target contract does not exist",
-    //     );
-    // }
+    #[public]
+    fn nested_call_to_nothing_recovers() {
+        let garbageAddress = AztecAddress::from_field(42);
+        call(1, 1, garbageAddress, &[]);
+        let success = success_copy();
+        assert(
+            !success,
+            "Nested CALL instruction should return failure if target contract does not exist",
+        );
+    }
 
     #[public]
     fn nested_call_to_add_with_gas(

yarn-project/bb-prover/src/avm_proving_tests/avm_bulk.test.ts

@@ -34,7 +34,8 @@ describe('AVM proven bulk test', () => {
   it(
     'Prove and verify',
     async () => {
-      await bulkTest(tester, logger, AvmTestContractArtifact, (b: boolean) => expect(b).toBe(true));
+      const result = await bulkTest(tester, logger, AvmTestContractArtifact);
+      expect(result.revertCode.isOK()).toBe(true);
     },
     TIMEOUT,
   );

yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit1.test.ts

@@ -14,7 +14,7 @@ import { AvmProvingTester } from './avm_proving_tester.js';
 
 const TIMEOUT = 30_000;
 
-describe.skip('AVM check-circuit – unhappy paths 1', () => {
+describe('AVM check-circuit – unhappy paths 1', () => {
   let avmTestContractInstance: ContractInstanceWithAddress;
   let tester: AvmProvingTester;
 

yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts

@@ -21,7 +21,7 @@ describe('AVM check-circuit – unhappy paths 2', () => {
     );
   });
 
-  it.skip(
+  it(
     'an exceptional halt due to a nested call to non-existent contract is propagated to top-level',
     async () => {
       await tester.simProveVerifyAppLogic(
@@ -32,38 +32,44 @@ describe('AVM check-circuit – unhappy paths 2', () => {
     TIMEOUT,
   );
 
-  // TODO(#16099): Re-enable this test
-  // it(
-  //   'an exceptional halt due to a nested call to non-existent contract is recovered from in caller',
-  //   async () => {
-  //     await tester.simProveVerifyAppLogic(
-  //       { address: avmTestContractInstance.address, fnName: 'nested_call_to_nothing_recovers', args: [] },
-  //       /*expectRevert=*/ false,
-  //     );
-  //   },
-  //   TIMEOUT,
-  // );
+  it(
+    'an exceptional halt due to a nested call to non-existent contract is recovered from in caller',
+    async () => {
+      await tester.simProveVerifyAppLogic(
+        { address: avmTestContractInstance.address, fnName: 'nested_call_to_nothing_recovers', args: [] },
+        /*expectRevert=*/ false,
+      );
+    },
+    TIMEOUT,
+  );
 
-  it.skip('top-level exceptional halts due to a non-existent contract in app-logic and teardown', async () => {
+  it('top-level exceptional halts due to a non-existent contract in app-logic and teardown', async () => {
     // don't insert contracts into trees, and make sure retrieval fails
     const tester = await AvmProvingTester.new(/*checkCircuitOnly=*/ true);
+    // Note: we need to specify the contract artifacts here because we intentionally skip registration,
+    // so the tester can't retrieve them on its own.
     await tester.simProveVerify(
       sender,
       /*setupCalls=*/ [],
       /*appCalls=*/ [
-        { address: avmTestContractInstance.address, fnName: 'add_args_return', args: [new Fr(1), new Fr(2)] },
+        {
+          address: avmTestContractInstance.address,
+          fnName: 'add_args_return',
+          args: [new Fr(1), new Fr(2)],
+          contractArtifact: AvmTestContractArtifact,
+        },
       ],
       /*teardownCall=*/ {
         address: avmTestContractInstance.address,
         fnName: 'add_args_return',
         args: [new Fr(1), new Fr(2)],
+        contractArtifact: AvmTestContractArtifact,
       },
       /*expectRevert=*/ true,
     );
   });
 
-  // TODO: unskip once internalcall constraints are fixed (DEFAULT_PROPAGATE_CALL_ID in particular)
-  it.skip(
+  it(
     'enqueued calls in every phase, with enqueued calls that depend on each other',
     async () => {
       await tester.simProveVerify(
@@ -86,7 +92,7 @@ describe('AVM check-circuit – unhappy paths 2', () => {
     },
     TIMEOUT,
   );
-  it.skip(
+  it(
     'Should prove and verify a TX that reverts in teardown',
     async () => {
       await tester.simProveVerify(

yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit3.test.ts

@@ -7,7 +7,7 @@ import { AvmProvingTester } from './avm_proving_tester.js';
 
 const TIMEOUT = 100_000;
 
-describe.skip('AVM check-circuit – unhappy paths 3', () => {
+describe('AVM check-circuit – unhappy paths 3', () => {
   const sender = AztecAddress.fromNumber(42);
   let avmTestContractInstance: ContractInstanceWithAddress;
   let tester: AvmProvingTester;
@@ -77,15 +77,14 @@ describe.skip('AVM check-circuit – unhappy paths 3', () => {
     TIMEOUT,
   );
 
-  // TODO(#16099): Re-enable this test
-  // it(
-  //   'a nested exceptional halt is recovered from in caller',
-  //   async () => {
-  //     await tester.simProveVerifyAppLogic(
-  //       { address: avmTestContractInstance.address, fnName: 'external_call_to_divide_by_zero_recovers', args: [] },
-  //       /*expectRevert=*/ false,
-  //     );
-  //   },
-  //   TIMEOUT,
-  // );
+  it(
+    'a nested exceptional halt is recovered from in caller',
+    async () => {
+      await tester.simProveVerifyAppLogic(
+        { address: avmTestContractInstance.address, fnName: 'external_call_to_divide_by_zero_recovers', args: [] },
+        /*expectRevert=*/ false,
+      );
+    },
+    TIMEOUT,
+  );
 });

yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit_amm.test.ts

@@ -8,7 +8,7 @@ import path from 'path';
 
 import { AvmProvingTester } from './avm_proving_tester.js';
 
-const TIMEOUT = 300_000;
+const TIMEOUT = 60_000;
 
 describe('AVM proven AMM', () => {
   const logger = createLogger('avm-proven-tests-amm');

yarn-project/bb-prover/src/avm_proving_tests/avm_mega_bulk.test.ts

@@ -35,7 +35,8 @@ describe.skip('AVM proven MEGA bulk test', () => {
   it(
     'Prove and verify mega bulk test',
     async () => {
-      await megaBulkTest(tester, logger, AvmTestContractArtifact, (b: boolean) => expect(b).toBe(true));
+      const result = await megaBulkTest(tester, logger, AvmTestContractArtifact);
+      expect(result.revertCode.isOK()).toBe(true);
     },
     TIMEOUT,
   );