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

dbanks12 · web-flow · commit 331ad6c86099 · 2025-09-01T18:26:42.000Z
Fixes to alu, context, execution, tx pil. Fixes to tx execution and tx
tracegen. Trace side-effect operations before they fail in TS state
manager. Always perform nullifier non-membership check on non-existence
in AVM TS. Side-effects rollbacks in tx execution.

1. ALU PIL should disable C's tag check on `sel_err` (any error) rather
than `sel_tag_err`. It at least needs to be disabled on div-by-0 as
well.
2. `sel_instruction_fetching_success` should be forced to 0 on bytecode
retrieval faliure.
3. TX's dispatch "end" interaction to execution's should use execution's
`sel_failure` to capture "error or revert opcode".
4. Bytecode retrieval failures should emit an event that includes tree
roots so that non-membership can be performed.
5. When bytecode is not found for a contract call, the execution trace's
empty row should have opcode 0.
6. TX trace should snapshot and restore end-setup's side effect states.
7. Execution tracegen should handle "exit call scenarios" in a separate
if-statement to individual opcodes, because if an opcode errors, we need
to set all of the selectors for "exit", but also need to tracegen the
failing opcode.
8. In the TS avm simulator, contract instance retrieval should perform
the nullifier [non]membership check even if the instance does not exist.
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);`