fix!: misc avm - fixes constraints, ts sim, cpp sim, tracegen (#16664)

dbanks12 · ludamad · commit 752017e70c1e · 2025-09-01T18:26:42.000Z
diff --git a/barretenberg/cpp/pil/vm2/alu.pil b/barretenberg/cpp/pil/vm2/alu.pil
@@ -119,7 +119,7 @@ pol EXPECTED_C_TAG = (sel_op_add + sel_op_sub + sel_op_mul + sel_op_div + sel_op
 // Gating with (1 - sel_tag_err) is necessary because when an error occurs, we have to set the tag to 0,
 // which might not be equal to EXPECTED_C_TAG.
 #[C_TAG_CHECK]
-(1 - sel_tag_err) * (EXPECTED_C_TAG - ic_tag) = 0;
+(1 - sel_err) * (EXPECTED_C_TAG - ic_tag) = 0;
 
 pol commit sel_tag_err;
 sel_tag_err * (1 - sel_tag_err) = 0;
diff --git a/barretenberg/cpp/pil/vm2/context.pil b/barretenberg/cpp/pil/vm2/context.pil
@@ -12,6 +12,10 @@ namespace execution;
     #[skippable_if]
     sel = 0;
 
+    // TODO: make sure that all uses of `sel_error` here really should be "error" only
+    // since `sel_error` will not be ` for the REVERT opcode. If we need "error or REVERT",
+    // use `sel_failure`.
+
     // Guaranteed to be boolean because sel_execute_call & sel_execute_static_call are mutually exclusive
     pol commit sel_enter_call;
     // The following selectors will be 0 if there is an error during execution (before the opcode execution step).
diff --git a/barretenberg/cpp/pil/vm2/execution.pil b/barretenberg/cpp/pil/vm2/execution.pil
@@ -147,7 +147,7 @@ sel_bytecode_retrieval_success {
 
 pol commit sel_instruction_fetching_success;
 // If sel = 0, we want sel_instruction_fetching_success = 0. We shouldn't be using it.
-sel_instruction_fetching_success = sel * (1 - sel_instruction_fetching_failure);
+sel_instruction_fetching_success = sel_bytecode_retrieval_success * (1 - sel_instruction_fetching_failure);
 
 #[INSTRUCTION_FETCHING_BODY]
 sel_instruction_fetching_success {
diff --git a/barretenberg/cpp/pil/vm2/tx.pil b/barretenberg/cpp/pil/vm2/tx.pil
@@ -306,7 +306,7 @@ namespace tx;
     execution.enqueued_call_end {
        execution.context_id,
        execution.next_context_id,
-       execution.sel_error,
+       execution.sel_failure,
        execution.discard,
        // Tree State
        execution.note_hash_tree_root,
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/alu_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/alu_impl.hpp
@@ -138,7 +138,7 @@ void aluImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     }
     { // C_TAG_CHECK
         using Accumulator = typename std::tuple_element_t<8, ContainerOverSubrelations>;
-        auto tmp = (FF(1) - in.get(C::alu_sel_tag_err)) * (alu_EXPECTED_C_TAG - in.get(C::alu_ic_tag));
+        auto tmp = (FF(1) - in.get(C::alu_sel_err)) * (alu_EXPECTED_C_TAG - in.get(C::alu_ic_tag));
         tmp *= scaling_factor;
         std::get<8>(evals) += typename Accumulator::View(tmp);
     }
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/execution_impl.hpp
@@ -120,7 +120,8 @@ void executionImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     {
         using Accumulator = typename std::tuple_element_t<8, ContainerOverSubrelations>;
         auto tmp = (in.get(C::execution_sel_instruction_fetching_success) -
-                    in.get(C::execution_sel) * (FF(1) - in.get(C::execution_sel_instruction_fetching_failure)));
+                    in.get(C::execution_sel_bytecode_retrieval_success) *
+                        (FF(1) - in.get(C::execution_sel_instruction_fetching_failure)));
         tmp *= scaling_factor;
         std::get<8>(evals) += typename Accumulator::View(tmp);
     }
diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_tx.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_tx.hpp
@@ -243,7 +243,7 @@ struct lookup_tx_dispatch_exec_end_settings_ {
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> DST_COLUMNS = {
         ColumnAndShifts::execution_context_id,
         ColumnAndShifts::execution_next_context_id,
-        ColumnAndShifts::execution_sel_error,
+        ColumnAndShifts::execution_sel_failure,
         ColumnAndShifts::execution_discard,
         ColumnAndShifts::execution_note_hash_tree_root,
         ColumnAndShifts::execution_note_hash_tree_size,
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/bytecode_manager.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/bytecode_manager.cpp
@@ -18,11 +18,14 @@ BytecodeId TxBytecodeManager::get_bytecode(const AztecAddress& address)
     // This handles nullifier checks, address derivation, and update validation
     auto maybe_instance = contract_instance_manager.get_contract_instance(address);
 
+    auto tree_states = merkle_db.get_tree_state();
     if (!maybe_instance.has_value()) {
         // Contract instance not found - emit error event and throw
         retrieval_events.emit({
             .bytecode_id = FF(0), // Use default ID for error case
             .address = address,
+            .nullifier_root = tree_states.nullifierTree.tree.root,
+            .public_data_tree_root = tree_states.publicDataTree.tree.root,
             .error = true,
         });
         vinfo("Contract ", field_to_string(address), " is not deployed!");
@@ -45,7 +48,6 @@ BytecodeId TxBytecodeManager::get_bytecode(const AztecAddress& address)
     // Check if we've already processed this bytecode. If so, don't do hashing and decomposition again!
     if (bytecodes.contains(bytecode_id)) {
         // Already processed this bytecode - just emit retrieval event and return
-        auto tree_states = merkle_db.get_tree_state();
         retrieval_events.emit({
             .bytecode_id = bytecode_id,
             .address = address,
@@ -69,8 +71,6 @@ BytecodeId TxBytecodeManager::get_bytecode(const AztecAddress& address)
     // We now save the bytecode so that we don't repeat this process.
     bytecodes.emplace(bytecode_id, std::move(shared_bytecode));
 
-    auto tree_states = merkle_db.get_tree_state();
-
     retrieval_events.emit({
         .bytecode_id = bytecode_id,
         .address = address,
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp
@@ -114,6 +114,7 @@ void TxExecution::simulate(const Tx& tx)
                 format("[SETUP] UNRECOVERABLE ERROR! Enqueued call to ", call.request.contractAddress, " failed"));
         }
     }
+    SideEffectStates end_setup_side_effect_states = tx_context.side_effect_states;
 
     // The checkpoint we should go back to if anything from now on reverts.
     merkle_db.create_checkpoint();
@@ -159,6 +160,7 @@ void TxExecution::simulate(const Tx& tx)
         info("Revertible failure while simulating tx ", tx.hash, ": ", e.what());
         // We revert to the post-setup state.
         merkle_db.revert_checkpoint();
+        tx_context.side_effect_states = end_setup_side_effect_states;
         // But we also create a new fork so that the teardown phase can transparently
         // commit or rollback to the end of teardown.
         merkle_db.create_checkpoint();
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/written_public_data_slots_tree_check.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/written_public_data_slots_tree_check.cpp
@@ -139,6 +139,7 @@ void WrittenPublicDataSlotsTreeCheck::create_checkpoint()
 
 void WrittenPublicDataSlotsTreeCheck::commit_checkpoint()
 {
+    // Commit the current top of the stack down one level.
     WrittenPublicDataSlotsTree current_tree = std::move(written_public_data_slots_tree_stack.top());
     written_public_data_slots_tree_stack.pop();
     written_public_data_slots_tree_stack.top() = std::move(current_tree);
diff --git a/barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp b/barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp
@@ -476,7 +476,7 @@ void ExecutionTraceBuilder::process(
          **************************************************************************************************/
 
         // Along this function we need to set the info we get from the EXEC_SPEC_READ lookup.
-        bool should_read_exec_spec = !instruction_fetching_failed;
+        bool should_read_exec_spec = process_instruction_fetching && !instruction_fetching_failed;
         if (should_read_exec_spec) {
             process_execution_spec(ex_event, trace, row);
         }
@@ -557,6 +557,21 @@ void ExecutionTraceBuilder::process(
         // TODO: would is_err here catch any error at the opcode execution step which we dont want to consider?
         bool sel_exit_call = should_execute_return || should_execute_revert || is_err;
 
+        if (sel_exit_call) {
+            // We rollback if we revert or error and we have a parent context.
+            trace.set(row,
+                      { {
+                          // Exit reason - opcode or error
+                          { C::execution_sel_execute_return, should_execute_return ? 1 : 0 },
+                          { C::execution_sel_execute_revert, should_execute_revert ? 1 : 0 },
+                          { C::execution_sel_exit_call, sel_exit_call ? 1 : 0 },
+                          { C::execution_nested_return, should_execute_return && has_parent ? 1 : 0 },
+                          // Enqueued or nested exit dependent on if we are a child context
+                          { C::execution_enqueued_call_end, !has_parent ? 1 : 0 },
+                          { C::execution_nested_exit_call, has_parent ? 1 : 0 },
+                      } });
+        }
+
         bool opcode_execution_failed = ex_event.error == ExecutionError::OPCODE_EXECUTION;
         if (should_execute_opcode) {
             // At this point we can assume instruction fetching succeeded, so this should never fail.
@@ -600,19 +615,6 @@ void ExecutionTraceBuilder::process(
                               { C::execution_call_is_da_gas_allocated_lt_left, is_da_gas_allocated_lt_left },
                               { C::execution_call_allocated_left_da_cmp_diff, allocated_left_da_cmp_diff },
                           } });
-            } else if (sel_exit_call) {
-                // We rollback if we revert or error and we have a parent context.
-                trace.set(row,
-                          { {
-                              // Exit reason - opcode or error
-                              { C::execution_sel_execute_return, should_execute_return ? 1 : 0 },
-                              { C::execution_sel_execute_revert, should_execute_revert ? 1 : 0 },
-                              { C::execution_sel_exit_call, sel_exit_call ? 1 : 0 },
-                              { C::execution_nested_return, should_execute_return && has_parent ? 1 : 0 },
-                              // Enqueued or nested exit dependent on if we are a child context
-                              { C::execution_enqueued_call_end, !has_parent ? 1 : 0 },
-                              { C::execution_nested_exit_call, has_parent ? 1 : 0 },
-                          } });
             }
             // Separate if-statement for opcodes.
             // This cannot be an else-if chained to the above,
diff --git a/yarn-project/simulator/src/public/avm/avm_simulator.test.ts b/yarn-project/simulator/src/public/avm/avm_simulator.test.ts
@@ -149,7 +149,12 @@ describe('AVM simulator: injected bytecode', () => {
 
 describe('AVM simulator: transpiled Noir contracts', () => {
   it('execution of a non-existent contract immediately reverts and consumes all allocated gas', async () => {
-    const context = initContext();
+    const treesDB = mock<PublicTreesDB>();
+    const persistableState = initPersistableStateManager({ treesDB });
+    const address = AztecAddress.fromNumber(1234);
+    const env = initExecutionEnvironment({ address });
+    const context = initContext({ env, persistableState });
+    mockCheckNullifierExists(treesDB, false);
     const results = await new AvmSimulator(context).execute();
 
     expect(results.reverted).toBe(true);
diff --git a/yarn-project/simulator/src/public/avm/opcodes/external_calls.test.ts b/yarn-project/simulator/src/public/avm/opcodes/external_calls.test.ts
@@ -79,6 +79,8 @@ describe('External Calls', () => {
       // Define dst offset for SuccessCopy
       const successDstOffset = 6;
 
+      mockCheckNullifierExists(treesDB, false);
+
       const { l2GasLeft: initialL2Gas, daGasLeft: initialDaGas } = context.machineState;
 
       context.machineState.memory.set(0, new Uint32(l2Gas));
diff --git a/yarn-project/simulator/src/public/state_manager/state_manager.test.ts b/yarn-project/simulator/src/public/state_manager/state_manager.test.ts
@@ -141,6 +141,7 @@ describe('state_manager', () => {
     });
 
     it('Can get undefined contract instance', async () => {
+      mockCheckNullifierExists(treesDB, false);
       await persistableState.getContractInstance(address);
     });
   });
@@ -167,6 +168,7 @@ describe('state_manager', () => {
       );
     });
     it('Can get undefined bytecode', async () => {
+      mockCheckNullifierExists(treesDB, false);
       await persistableState.getBytecode(address);
     });
   });
diff --git a/yarn-project/simulator/src/public/state_manager/state_manager.ts b/yarn-project/simulator/src/public/state_manager/state_manager.ts
@@ -340,18 +340,18 @@ export class PublicPersistableStateManager {
     const exists = instanceWithAddress !== undefined;
 
     const instance = exists ? new SerializableContractInstance(instanceWithAddress) : undefined;
-    if (!exists) {
-      this.log.debug(`Contract instance NOT FOUND (address=${contractAddress})`);
-      return undefined;
-    }
 
-    this.log.trace(`Got contract instance (address=${contractAddress}): instance=${jsonStringify(instance!)}`);
-    // Canonical addresses do not trigger nullifier and update checks.
     if (contractAddressIsCanonical(contractAddress)) {
+      this.log.trace(
+        `Got canonical contract instance (address=${contractAddress}): instance=${jsonStringify(instance!)}`,
+      );
       return instance;
     }
 
-    // This will decide internally whether to check the nullifier tree or not depending on doMerkleOperations.
+    // Nullifier check! We do this regardless of whether or not the instance exists,
+    // as even for non-existence, we need to generate nullifier check hints.
+
+    // This will internally decide whether to check the nullifier tree or not depending on doMerkleOperations.
     const nullifierExistsInTree = await this.checkNullifierExists(
       AztecAddress.fromNumber(CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS),
       contractAddress.toField(),
@@ -361,6 +361,13 @@ export class PublicPersistableStateManager {
       `Contract instance for address ${contractAddress} in DB: ${exists} != nullifier tree: ${nullifierExistsInTree}. This is a bug!`,
     );
 
+    if (!exists) {
+      this.log.debug(`Contract instance NOT FOUND (address=${contractAddress})`);
+      return undefined;
+    }
+
+    this.log.trace(`Got contract instance (address=${contractAddress}): instance=${jsonStringify(instance!)}`);
+
     // All that is left is tocheck that the contract updatability information is correct.
     // That is, that the current and original contract class ids are correct.
     await this.checkContractUpdateInformation(instanceWithAddress);
@@ -455,6 +462,7 @@ export class PublicPersistableStateManager {
       this.log.debug(`Contract instance NOT FOUND (id=${classId})`);
     }
 
+    // TODO(dbanks12): does this need to be moved to before the DB accesses as was done with writeNullifier?
     this.trace.traceGetContractClass(classId, exists);
     return extendedClass;
   }

Original file line number	Diff line number	Diff line change
`@@ -138,7 +138,7 @@ void aluImpl<FF_>::accumulate(ContainerOverSubrelations& evals,`
`138`	`138`	`}`
`139`	`139`	`{ // C_TAG_CHECK`
`140`	`140`	`using Accumulator = typename std::tuple_element_t<8, ContainerOverSubrelations>;`
`141`		`- auto tmp = (FF(1) - in.get(C::alu_sel_tag_err)) * (alu_EXPECTED_C_TAG - in.get(C::alu_ic_tag));`
	`141`	`+ auto tmp = (FF(1) - in.get(C::alu_sel_err)) * (alu_EXPECTED_C_TAG - in.get(C::alu_ic_tag));`
`142`	`142`	`tmp *= scaling_factor;`
`143`	`143`	`std::get<8>(evals) += typename Accumulator::View(tmp);`
`144`	`144`	`}`
Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,8 @@ void executionImpl<FF_>::accumulate(ContainerOverSubrelations& evals,`
`120`	`120`	`{`
`121`	`121`	`using Accumulator = typename std::tuple_element_t<8, ContainerOverSubrelations>;`
`122`	`122`	`auto tmp = (in.get(C::execution_sel_instruction_fetching_success) -`
`123`		`- in.get(C::execution_sel) * (FF(1) - in.get(C::execution_sel_instruction_fetching_failure)));`
	`123`	`+ in.get(C::execution_sel_bytecode_retrieval_success) *`
	`124`	`+ (FF(1) - in.get(C::execution_sel_instruction_fetching_failure)));`
`124`	`125`	`tmp *= scaling_factor;`
`125`	`126`	`std::get<8>(evals) += typename Accumulator::View(tmp);`
`126`	`127`	`}`
Original file line number	Diff line number	Diff line change
`@@ -114,6 +114,7 @@ void TxExecution::simulate(const Tx& tx)`
`114`	`114`	`format("[SETUP] UNRECOVERABLE ERROR! Enqueued call to ", call.request.contractAddress, " failed"));`
`115`	`115`	`}`
`116`	`116`	`}`
	`117`	`+ SideEffectStates end_setup_side_effect_states = tx_context.side_effect_states;`
`117`	`118`
`118`	`119`	`// The checkpoint we should go back to if anything from now on reverts.`
`119`	`120`	`merkle_db.create_checkpoint();`
`@@ -159,6 +160,7 @@ void TxExecution::simulate(const Tx& tx)`
`159`	`160`	`info("Revertible failure while simulating tx ", tx.hash, ": ", e.what());`
`160`	`161`	`// We revert to the post-setup state.`
`161`	`162`	`merkle_db.revert_checkpoint();`
	`163`	`+ tx_context.side_effect_states = end_setup_side_effect_states;`
`162`	`164`	`// But we also create a new fork so that the teardown phase can transparently`
`163`	`165`	`// commit or rollback to the end of teardown.`
`164`	`166`	`merkle_db.create_checkpoint();`
Original file line number	Diff line number	Diff line change
`@@ -139,6 +139,7 @@ void WrittenPublicDataSlotsTreeCheck::create_checkpoint()`
`139`	`139`
`140`	`140`	`void WrittenPublicDataSlotsTreeCheck::commit_checkpoint()`
`141`	`141`	`{`
	`142`	`+ // Commit the current top of the stack down one level.`
`142`	`143`	`WrittenPublicDataSlotsTree current_tree = std::move(written_public_data_slots_tree_stack.top());`
`143`	`144`	`written_public_data_slots_tree_stack.pop();`
`144`	`145`	`written_public_data_slots_tree_stack.top() = std::move(current_tree);`