Merge branch 'next' into merge-train/barretenberg

Author: AztecBot

barretenberg/cpp/src/barretenberg/vm2/tracegen/execution_trace.cpp

@@ -605,7 +605,12 @@ void ExecutionTraceBuilder::process(
                               { C::execution_enqueued_call_end, !has_parent ? 1 : 0 },
                               { C::execution_nested_exit_call, has_parent ? 1 : 0 },
                           } });
-            } else if (exec_opcode == ExecutionOpCode::GETENVVAR) {
+            }
+            // Separate if-statement for opcodes.
+            // This cannot be an else-if chained to the above,
+            // because `sel_exit_call` can happen on any opcode
+            // and we still need to tracegen the opcode-specific logic.
+            if (exec_opcode == ExecutionOpCode::GETENVVAR) {
                 assert(ex_event.addressing_event.resolution_info.size() == 2 &&
                        "GETENVVAR should have exactly two resolved operands (envvar enum and output)");
                 // rop[1] is the envvar enum

barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.cpp

@@ -529,6 +529,7 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface<simulation:
 
     // This is the tree state we will use during the "skipped" phases
     TxContextEvent propagated_state = startup_event_data.state;
+    TxContextEvent end_setup_snapshot = startup_event_data.state;
     // Used to track the gas limit for the "padded" phases.
     Gas current_gas_limit = startup_event_data.gas_limit;
     Gas teardown_gas_limit = startup_event_data.teardown_gas_limit;
@@ -565,6 +566,10 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface<simulation:
             if (row == 1) {
                 trace.set(row, handle_first_row());
             }
+            if (phase == TransactionPhase::SETUP) {
+                // If setup is empty, the end-setup-snapshot should just be current/propagated state
+                end_setup_snapshot = propagated_state;
+            }
             row++;
             continue;
         }
@@ -590,7 +595,6 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface<simulation:
                     { C::tx_start_phase, phase_counter == 0 ? 1 : 0 },
                     { C::tx_sel_read_phase_length, phase_counter == 0 && !is_one_shot_phase(tx_phase_event->phase) },
                     { C::tx_is_revertible, is_revertible(tx_phase_event->phase) ? 1 : 0 },
-
                     { C::tx_end_phase, phase_counter == phase_events.size() - 1 ? 1 : 0 },
                 } });
             trace.set(row, handle_prev_gas_used(gas_used));
@@ -654,6 +658,22 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface<simulation:
         }
         // In case we encounter another skip row
         propagated_state = phase_events.back()->state_after;
+
+        if (phase == TransactionPhase::SETUP) {
+            // Store off the state at the end of setup to rollback to later on revert
+            end_setup_snapshot = phase_events.back()->state_after;
+        }
+        if (phase_events.back()->reverted) {
+            // On revert, roll back to end-setup-snapshot
+            // Even though tx-execution events should already do this,
+            // we need to update propagated state here so that any padded rows
+            // get the correct rolled-back state rather then the pre-rollback state.
+            propagated_state.tree_states = end_setup_snapshot.tree_states;
+            propagated_state.written_public_data_slots_tree_snapshot =
+                end_setup_snapshot.written_public_data_slots_tree_snapshot;
+            propagated_state.side_effect_states = end_setup_snapshot.side_effect_states;
+            // Note: we only rollback tree/side-effect states, not gas used or next_context_id.
+        }
     }
 }
 

yarn-project/simulator/src/public/avm/avm_simulator.test.ts

@@ -710,9 +710,10 @@ describe('AVM simulator: transpiled Noir contracts', () => {
         expect(results.reverted).toBe(true);
         expect(results.revertReason?.message).toMatch(/Attempted to emit duplicate nullifier/);
 
-        // Nullifier should be traced exactly once
-        expect(trace.traceNewNullifier).toHaveBeenCalledTimes(1);
-        expect(trace.traceNewNullifier).toHaveBeenCalledWith(siloedNullifier0);
+        // Nullifier should still be traced twice. Fails after second trace.
+        expect(trace.traceNewNullifier).toHaveBeenCalledTimes(2);
+        expect(trace.traceNewNullifier).toHaveBeenNthCalledWith(1, siloedNullifier0);
+        expect(trace.traceNewNullifier).toHaveBeenNthCalledWith(2, siloedNullifier0);
       });
     });
 

yarn-project/simulator/src/public/avm/opcodes/accrued_substate.test.ts

@@ -204,8 +204,9 @@ describe('Accrued Substate', () => {
           `Attempted to emit duplicate nullifier ${value0} (contract address: ${address}).`,
         ),
       );
-      expect(trace.traceNewNullifier).toHaveBeenCalledTimes(1);
-      expect(trace.traceNewNullifier).toHaveBeenCalledWith(siloedNullifier0);
+      expect(trace.traceNewNullifier).toHaveBeenCalledTimes(2);
+      expect(trace.traceNewNullifier).toHaveBeenNthCalledWith(1, siloedNullifier0);
+      expect(trace.traceNewNullifier).toHaveBeenNthCalledWith(2, siloedNullifier0);
     });
 
     it('Nullifier collision reverts (nullifier exists in host state)', async () => {
@@ -216,7 +217,8 @@ describe('Accrued Substate', () => {
           `Attempted to emit duplicate nullifier ${value0} (contract address: ${address}).`,
         ),
       );
-      expect(trace.traceNewNullifier).toHaveBeenCalledTimes(0); // the only attempt should fail before tracing
+      expect(trace.traceNewNullifier).toHaveBeenCalledTimes(1); // fails, but after tracing
+      expect(trace.traceNewNullifier).toHaveBeenCalledWith(siloedNullifier0);
     });
   });
 

yarn-project/simulator/src/public/state_manager/state_manager.ts

@@ -138,15 +138,15 @@ export class PublicPersistableStateManager {
   public async writeStorage(contractAddress: AztecAddress, slot: Fr, value: Fr, protocolWrite = false): Promise<void> {
     this.log.trace(`Storage write (address=${contractAddress}, slot=${slot}): value=${value}`);
 
+    await this.trace.tracePublicStorageWrite(contractAddress, slot, value, protocolWrite);
+
     if (this.doMerkleOperations) {
       // write to native merkle trees
       await this.treesDB.storageWrite(contractAddress, slot, value);
     } else {
       // Cache storage writes for later reference/reads
       this.publicStorage.write(contractAddress, slot, value);
     }
-
-    await this.trace.tracePublicStorageWrite(contractAddress, slot, value, protocolWrite);
   }
 
   public isStorageCold(contractAddress: AztecAddress, slot: Fr): boolean {
@@ -217,10 +217,10 @@ export class PublicPersistableStateManager {
    */
   public async writeUniqueNoteHash(uniqueNoteHash: Fr): Promise<void> {
     this.log.trace(`noteHashes += @${uniqueNoteHash}.`);
+    this.trace.traceNewNoteHash(uniqueNoteHash);
     if (this.doMerkleOperations) {
       await this.treesDB.writeNoteHash(uniqueNoteHash);
     }
-    this.trace.traceNewNoteHash(uniqueNoteHash);
   }
 
   /**
@@ -262,6 +262,8 @@ export class PublicPersistableStateManager {
   public async writeSiloedNullifier(siloedNullifier: Fr) {
     this.log.trace(`Inserting siloed nullifier=${siloedNullifier}`);
 
+    this.trace.traceNewNullifier(siloedNullifier);
+
     if (this.doMerkleOperations) {
       const exists = await this.treesDB.checkNullifierExists(siloedNullifier);
 
@@ -277,8 +279,6 @@ export class PublicPersistableStateManager {
       // Cache pending nullifiers for later access
       await this.nullifiers.append(siloedNullifier);
     }
-
-    this.trace.traceNewNullifier(siloedNullifier);
   }
 
   /**