chore: reverts "cleanup and address comments after refactor" (#9801)

Reverts #9785

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

@@ -130,7 +130,7 @@ describe('AVM simulator: transpiled Noir contracts', () => {
     const contractClass = makeContractClassPublic(0, publicFn);
     const contractInstance = makeContractInstanceFromClassId(contractClass.id);
 
-    // The values here should match those in getContractInstance test case
+    // The values here should match those in `avm_simulator.test.ts`
     const instanceGet = new SerializableContractInstance({
       version: 1,
       salt: new Fr(0x123),

yarn-project/simulator/src/avm/journal/journal.ts

@@ -62,10 +62,10 @@ export class AvmPersistableStateManager {
   /**
    * Create a new state manager forked from this one
    */
-  public fork() {
+  public fork(incrementSideEffectCounter: boolean = false) {
     return new AvmPersistableStateManager(
       this.worldStateDB,
-      this.trace.fork(),
+      this.trace.fork(incrementSideEffectCounter),
       this.publicStorage.fork(),
       this.nullifiers.fork(),
     );
@@ -242,7 +242,7 @@ export class AvmPersistableStateManager {
   /**
    * Accept nested world state modifications
    */
-  public mergeForkedState(forkedState: AvmPersistableStateManager) {
+  public acceptForkedState(forkedState: AvmPersistableStateManager) {
     this.publicStorage.acceptAndMerge(forkedState.publicStorage);
     this.nullifiers.acceptAndMerge(forkedState.nullifiers);
   }
@@ -298,7 +298,7 @@ export class AvmPersistableStateManager {
     avmCallResults: AvmContractCallResult,
   ) {
     if (!avmCallResults.reverted) {
-      this.mergeForkedState(forkedState);
+      this.acceptForkedState(forkedState);
     }
     const functionName = await getPublicFunctionDebugName(
       this.worldStateDB,
@@ -307,7 +307,7 @@ export class AvmPersistableStateManager {
       nestedEnvironment.calldata,
     );
 
-    this.log.verbose(`[AVM] State manager is processing & tracing results of nested call ${functionName}`);
+    this.log.verbose(`[AVM] Calling nested function ${functionName}`);
 
     this.trace.traceNestedCall(
       forkedState.trace,
@@ -330,7 +330,7 @@ export class AvmPersistableStateManager {
     reverted: boolean,
   ) {
     if (!reverted) {
-      this.mergeForkedState(forkedState);
+      this.acceptForkedState(forkedState);
     }
     const functionName = await getPublicFunctionDebugName(
       this.worldStateDB,
@@ -339,7 +339,7 @@ export class AvmPersistableStateManager {
       calldata,
     );
 
-    this.log.verbose(`[AVM] State manager is processing & tracing results of enqueued call ${functionName}`);
+    this.log.verbose(`[AVM] Encountered enqueued public call starting with function ${functionName}`);
 
     this.trace.traceEnqueuedCall(forkedState.trace, publicCallRequest, calldata, reverted);
   }
@@ -355,10 +355,10 @@ export class AvmPersistableStateManager {
     reverted: boolean,
   ) {
     if (!reverted) {
-      this.mergeForkedState(forkedState);
+      this.acceptForkedState(forkedState);
     }
 
-    this.log.verbose(`[AVM] State manager is processing & tracing results of an execution phase`);
+    this.log.verbose(`[AVM] Encountered app logic phase`);
 
     this.trace.traceExecutionPhase(forkedState.trace, publicCallRequests, calldatas, reverted);
   }

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

@@ -24,20 +24,18 @@ export class DualSideEffectTrace implements PublicSideEffectTraceInterface {
     public readonly enqueuedCallTrace: PublicEnqueuedCallSideEffectTrace,
   ) {}
 
-  public fork() {
-    return new DualSideEffectTrace(this.innerCallTrace.fork(), this.enqueuedCallTrace.fork());
+  public fork(incrementSideEffectCounter: boolean = false) {
+    return new DualSideEffectTrace(
+      this.innerCallTrace.fork(incrementSideEffectCounter),
+      this.enqueuedCallTrace.fork(incrementSideEffectCounter),
+    );
   }
 
   public getCounter() {
     assert(this.innerCallTrace.getCounter() == this.enqueuedCallTrace.getCounter());
     return this.innerCallTrace.getCounter();
   }
 
-  public incrementSideEffectCounter() {
-    this.innerCallTrace.incrementSideEffectCounter();
-    this.enqueuedCallTrace.incrementSideEffectCounter();
-  }
-
   public tracePublicStorageRead(contractAddress: Fr, slot: Fr, value: Fr, exists: boolean, cached: boolean) {
     this.innerCallTrace.tracePublicStorageRead(contractAddress, slot, value, exists, cached);
     this.enqueuedCallTrace.tracePublicStorageRead(contractAddress, slot, value, exists, cached);

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

@@ -127,9 +127,9 @@ export class PublicEnqueuedCallSideEffectTrace implements PublicSideEffectTraceI
     this.avmCircuitHints = AvmExecutionHints.empty();
   }
 
-  public fork() {
+  public fork(incrementSideEffectCounter: boolean = false) {
     return new PublicEnqueuedCallSideEffectTrace(
-      this.sideEffectCounter,
+      incrementSideEffectCounter ? this.sideEffectCounter + 1 : this.sideEffectCounter,
       new PublicValidationRequestArrayLengths(
         this.previousValidationRequestArrayLengths.noteHashReadRequests + this.noteHashReadRequests.length,
         this.previousValidationRequestArrayLengths.nullifierReadRequests + this.nullifierReadRequests.length,
@@ -155,7 +155,7 @@ export class PublicEnqueuedCallSideEffectTrace implements PublicSideEffectTraceI
     return this.sideEffectCounter;
   }
 
-  public incrementSideEffectCounter() {
+  private incrementSideEffectCounter() {
     this.sideEffectCounter++;
   }
 
@@ -449,8 +449,8 @@ export class PublicEnqueuedCallSideEffectTrace implements PublicSideEffectTraceI
   }
 
   /**
-   * Trace an execution phase.
-   * Accept some results from a finished phase's trace into this one.
+   * Trace an enqueued call.
+   * Accept some results from a finished call's trace into this one.
    */
   public traceExecutionPhase(
     /** The trace of the enqueued call. */

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

@@ -202,7 +202,8 @@ export class EnqueuedCallsProcessor {
     );
     // TODO(dbanks12): insert all non-revertible side effects from private here.
 
-    for (const phase of phases) {
+    for (let i = 0; i < phases.length; i++) {
+      const phase = phases[i];
       let stateManagerForPhase: AvmPersistableStateManager;
       if (phase === PublicKernelPhase.SETUP) {
         // don't need to fork for setup since it's non-revertible
@@ -312,10 +313,8 @@ export class EnqueuedCallsProcessor {
       const availableGas = this.getAvailableGas(tx, publicKernelOutput, phase);
       const transactionFee = this.getTransactionFee(tx, publicKernelOutput, phase);
 
-      const enqueuedCallStateManager = txStateManager.fork();
       // each enqueued call starts with an incremented side effect counter
-      enqueuedCallStateManager.trace.incrementSideEffectCounter();
-
+      const enqueuedCallStateManager = txStateManager.fork(/*incrementSideEffectCounter=*/ true);
       const enqueuedCallResult = await this.enqueuedCallSimulator.simulate(
         callRequest,
         executionRequest,
@@ -349,6 +348,10 @@ export class EnqueuedCallsProcessor {
       if (revertReason) {
         // TODO(#6464): Should we allow emitting contracts in the private setup phase?
         // if so, this is removing contracts deployed in private setup
+        // You can't submit contracts in public, so this is only relevant for private-created
+        // side effects
+        // Are we reverting here back to end of non-revertible insertions?
+        // What are we reverting back to?
         await this.worldStateDB.removeNewContracts(tx);
         tx.filterRevertedLogs(publicKernelOutput);
       } else {

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

@@ -43,7 +43,7 @@ export class PublicExecutor {
    */
   public async simulate(
     stateManager: AvmPersistableStateManager,
-    executionRequest: PublicExecutionRequest,
+    executionRequest: PublicExecutionRequest, // TODO(dbanks12): CallRequest instead?
     globalVariables: GlobalVariables,
     allocatedGas: Gas,
     transactionFee: Fr = Fr.ZERO,
@@ -105,8 +105,6 @@ export class PublicExecutor {
    * @param transactionFee - Fee offered for this TX.
    * @param startSideEffectCounter - The start counter to initialize the side effect trace with.
    * @returns The result of execution including side effect vectors.
-   * FIXME: this function is only used by the TXE. Ideally we would not support this as an external interface.
-   * Avoid using this interface as it it shouldn't really exist in the first place.
    */
   public async simulateIsolatedEnqueuedCall(
     executionRequest: PublicExecutionRequest,

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

@@ -88,15 +88,15 @@ export class PublicSideEffectTrace implements PublicSideEffectTraceInterface {
     this.avmCircuitHints = AvmExecutionHints.empty();
   }
 
-  public fork() {
-    return new PublicSideEffectTrace(this.sideEffectCounter);
+  public fork(incrementSideEffectCounter: boolean = false) {
+    return new PublicSideEffectTrace(incrementSideEffectCounter ? this.sideEffectCounter + 1 : this.sideEffectCounter);
   }
 
   public getCounter() {
     return this.sideEffectCounter;
   }
 
-  public incrementSideEffectCounter() {
+  private incrementSideEffectCounter() {
     this.sideEffectCounter++;
   }
 

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

@@ -14,9 +14,8 @@ import { type AvmExecutionEnvironment } from '../avm/avm_execution_environment.j
 import { type EnqueuedPublicCallExecutionResultWithSideEffects, type PublicFunctionCallResult } from './execution.js';
 
 export interface PublicSideEffectTraceInterface {
-  fork(): PublicSideEffectTraceInterface;
+  fork(incrementSideEffectCounter?: boolean): PublicSideEffectTraceInterface;
   getCounter(): number;
-  incrementSideEffectCounter(): void;
   // all "trace*" functions can throw SideEffectLimitReachedError
   tracePublicStorageRead(contractAddress: Fr, slot: Fr, value: Fr, exists: boolean, cached: boolean): void;
   tracePublicStorageWrite(contractAddress: Fr, slot: Fr, value: Fr): void;