split up function selector and name in TS sim, cpp-ts sim comparison skips error message comparisons

dbanks12 · dbanks12 · commit 26f55a8b3eea · 2025-11-26T16:19:37.000Z
diff --git a/barretenberg/cpp/src/barretenberg/nodejs_module/avm_simulate/avm_simulate_napi.cpp b/barretenberg/cpp/src/barretenberg/nodejs_module/avm_simulate/avm_simulate_napi.cpp
@@ -101,7 +101,7 @@ struct ContractCallbacks {
 Napi::Value AvmSimulateNapi::simulate(const Napi::CallbackInfo& cb_info)
 {
     // TODO(dbanks12): configurable verbosity (maybe based on TS log level)
-    verbose_logging = true;
+    // verbose_logging = true;
     // debug_logging = true;
 
     Napi::Env env = cb_info.Env();
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/call_stack_metadata_collector.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/call_stack_metadata_collector.cpp
@@ -112,23 +112,24 @@ ReturnDataProvider make_return_data_provider(const ContextInterface& context, ui
         try {
             const auto& memory = context.get_memory();
             std::vector<FF> data;
+            info("Collecting returndata rd_offset:", rd_offset, " rd_size:", rd_size, " max_size:", max_size, ")");
             data.reserve(std::min(max_size, rd_size));
             for (uint32_t i = 0; i < std::min(max_size, rd_size); i++) {
                 data.push_back(memory.get(rd_offset + i).as_ff());
             }
             return data;
         } catch (...) {
-            vinfo("Failed to collect returndata (to:",
-                  context.get_address(),
-                  " pc:",
-                  context.get_pc(),
-                  " rd_offset:",
-                  rd_offset,
-                  " rd_size:",
-                  rd_size,
-                  " max_size:",
-                  max_size,
-                  ")");
+            info("Failed to collect returndata (to:",
+                 context.get_address(),
+                 " pc:",
+                 context.get_pc(),
+                 " rd_offset:",
+                 rd_offset,
+                 " rd_size:",
+                 rd_size,
+                 " max_size:",
+                 max_size,
+                 ")");
             return {};
         }
     };
diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/call_stack_metadata_collector.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/lib/call_stack_metadata_collector.test.cpp
@@ -422,15 +422,22 @@ TEST_F(MakeProviderTest, MakeReturnDataProviderSuccess)
         MemoryValue::from<FF>(FF(0x2222)),
         MemoryValue::from<FF>(FF(0x3333)),
     };
+    auto zero = MemoryValue::from<FF>(FF(0));
 
     auto provider = make_return_data_provider(mock_context, rd_addr, rd_size);
 
     // This is called when invoking the provider
     StrictMock<MockMemory> memory;
     EXPECT_CALL(::testing::Const(mock_context), get_memory()).WillOnce(ReturnRef(memory));
-    ON_CALL(memory, get).WillByDefault([&return_data_values](uint32_t i) -> const MemoryValue& {
-        return return_data_values[i];
-    });
+    ON_CALL(memory, get)
+        .WillByDefault([&return_data_values, rd_addr, rd_size, &zero](uint32_t i) -> const MemoryValue& {
+            // if offset is not in range [rd_addr, rd_addr + rd_size], return 0
+            if (i < rd_addr || i >= rd_addr + rd_size) {
+                return zero;
+            }
+            const auto offset_into_rd = i - rd_addr;
+            return return_data_values[offset_into_rd];
+        });
     EXPECT_CALL(memory, get).Times(static_cast<int>(rd_size));
 
     auto result = provider(1024);
diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/minimal_tx.testdata.bin b/barretenberg/cpp/src/barretenberg/vm2/testing/minimal_tx.testdata.bin
diff --git a/barretenberg/cpp/src/barretenberg/vm2/testing/tx_result_0x02440a894870f8078ce6065f76c2f598e2f3df9719f1a78038057c20eb096516.testdata.bin b/barretenberg/cpp/src/barretenberg/vm2/testing/tx_result_0x02440a894870f8078ce6065f76c2f598e2f3df9719f1a78038057c20eb096516.testdata.bin
diff --git a/yarn-project/simulator/artifacts/avm_minimal_inputs.json b/yarn-project/simulator/artifacts/avm_minimal_inputs.json
@@ -91,13 +91,7 @@
         "commitment": "0x253955340ab6819b564797927db2433e18decc4b3f37cbd27dcf3b4d6222a694"
       }
     ],
-    "debugFunctionNames": [
-      {
-        "address": "0x03deefb26b3b88ef515e0ce5bd1402ea3a7c12569597cddb19ce6260444d625f",
-        "selector": "0x000000000000000000000000000000000000000000000000000000000d6ab3ae",
-        "name": "0x0d6ab3ae"
-      }
-    ],
+    "debugFunctionNames": [],
     "contractDbCreateCheckpointHints": [{ "actionCounter": 0, "oldCheckpointId": 0, "newCheckpointId": 1 }],
     "contractDbCommitCheckpointHints": [{ "actionCounter": 1, "oldCheckpointId": 1, "newCheckpointId": 0 }],
     "contractDbRevertCheckpointHints": [],
diff --git a/yarn-project/simulator/src/public/avm/avm_simulator.ts b/yarn-project/simulator/src/public/avm/avm_simulator.ts
@@ -177,7 +177,7 @@ export class AvmSimulator implements AvmSimulatorInterface {
 
         if (machineState.pc >= bytecode.length) {
           this.log.warn('Passed end of program');
-          throw new InvalidProgramCounterError(machineState.pc, /*max=*/ bytecode.length);
+          throw new InvalidProgramCounterError(machineState.pc, /*max=*/ bytecode.length - 1);
         }
       }
 
@@ -235,12 +235,15 @@ export class AvmSimulator implements AvmSimulatorInterface {
 
   private async handleFailureToRetrieveBytecode(message: string): Promise<AvmContractCallResult> {
     // revert, consuming all gas
-    const fnName = await this.context.persistableState.getPublicFunctionDebugName(this.context.environment);
+    const { functionSelector, functionName } = await this.context.persistableState.getPublicFunctionSelectorAndName(
+      this.context.environment,
+    );
     const revertReason = new AvmRevertReason(
       message,
       /*failingFunction=*/ {
         contractAddress: this.context.environment.address,
-        functionName: fnName,
+        functionSelector,
+        functionName,
       },
       /*noirCallStack=*/ [],
     );
diff --git a/yarn-project/simulator/src/public/avm/revert_reason.ts b/yarn-project/simulator/src/public/avm/revert_reason.ts
@@ -18,13 +18,16 @@ async function createRevertReason(message: string, revertData: Fr[], context: Av
     message = context.machineState.collectedRevertInfo.recursiveRevertReason.message;
   }
 
-  const fnName = await context.persistableState.getPublicFunctionDebugName(context.environment);
+  const { functionSelector, functionName } = await context.persistableState.getPublicFunctionSelectorAndName(
+    context.environment,
+  );
 
   return new AvmRevertReason(
     message,
     /*failingFunction=*/ {
       contractAddress: context.environment.address,
-      functionName: fnName,
+      functionSelector,
+      functionName,
     },
     /*noirCallStack=*/ [...internalCallStack, context.machineState.pc].map(pc => `0.${pc}`),
     /*options=*/ { cause: nestedError },
diff --git a/yarn-project/simulator/src/public/debug_fn_name.ts b/yarn-project/simulator/src/public/debug_fn_name.ts
@@ -16,3 +16,28 @@ export async function getPublicFunctionDebugName(
   const selector = FunctionSelector.fromField(calldata[0]);
   return (await db.getDebugFunctionName(contractAddress, selector)) ?? selector.toString();
 }
+
+/**
+ * Get the function selector and optional debug name for a public function.
+ * Returns the selector and name separately, with name only populated if a debug name is available.
+ * @param db - The contracts database
+ * @param contractAddress - The contract address
+ * @param calldata - The calldata (selector is in calldata[0])
+ * @returns An object with functionSelector (always if calldata[0] exists) and functionName (only if debug name found)
+ */
+export async function getPublicFunctionSelectorAndName(
+  db: PublicContractsDBInterface,
+  contractAddress: AztecAddress,
+  calldata: Fr[],
+): Promise<{ functionSelector?: FunctionSelector; functionName?: string }> {
+  // Public function is dispatched and therefore the target function is passed in the first argument.
+  if (!calldata[0]) {
+    return {};
+  }
+  const selector = FunctionSelector.fromField(calldata[0]);
+  const debugName = await db.getDebugFunctionName(contractAddress, selector);
+  return {
+    functionSelector: selector,
+    functionName: debugName ?? undefined,
+  };
+}
diff --git a/yarn-project/simulator/src/public/fixtures/simple_contract_data_source.ts b/yarn-project/simulator/src/public/fixtures/simple_contract_data_source.ts
@@ -90,19 +90,17 @@ export class SimpleContractDataSource implements ContractDataSource {
     return this.contractArtifacts.get(contractInstance!.currentContractClassId.toString());
   }
 
-  async getDebugFunctionName(address: AztecAddress, selector: FunctionSelector): Promise<string> {
+  async getDebugFunctionName(address: AztecAddress, selector: FunctionSelector): Promise<string | undefined> {
     const contractInstance = await this.getContract(address);
     if (!contractInstance) {
-      this.logger.warn(
-        `Couldn't get fn name for debugging. Contract not in tester's ContractDataSource. Using selector:${selector} instead...`,
-      );
-      return `selector:${selector.toString()}`;
+      this.logger.warn(`Couldn't get fn name for debugging. Contract not in tester's ContractDataSource.`);
+      return undefined;
     }
     const key = `${contractInstance.currentContractClassId.toString()}:${selector.toString()}`;
     const fnName = this.debugFunctionName.get(key);
     if (!fnName) {
-      this.logger.warn(`Couldn't get fn name for debugging. Using selector:${selector} instead...`);
-      return selector.toString();
+      this.logger.warn(`Couldn't get fn name for debugging...`);
+      return undefined;
     }
     return fnName;
   }
diff --git a/yarn-project/simulator/src/public/public_processor/public_processor.ts b/yarn-project/simulator/src/public/public_processor/public_processor.ts
@@ -12,7 +12,6 @@ import {
   AvmCircuitPublicInputs,
   AvmExecutionHints,
   type AvmProvingRequest,
-  CallStackMetadata,
   PublicDataWrite,
   PublicSimulatorConfig,
 } from '@aztec/stdlib/avm';
@@ -35,7 +34,6 @@ import {
   type ProcessedTx,
   StateReference,
   Tx,
-  TxExecutionPhase,
   makeProcessedTxFromPrivateOnlyTx,
   makeProcessedTxFromTxWithPublicCalls,
 } from '@aztec/stdlib/tx';
@@ -524,7 +522,7 @@ export class PublicProcessor implements Traceable {
     const timer = new Timer();
 
     const result = await this.publicTxSimulator.simulate(tx);
-    const { hints, publicInputs, gasUsed, revertCode, callStackMetadata } = result;
+    const { hints, publicInputs, gasUsed, revertCode /*callStackMetadata*/ } = result;
 
     if (!hints) {
       this.metrics.recordFailedTx();
diff --git a/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/avm_minimal.test.ts b/yarn-project/simulator/src/public/public_tx_simulator/apps_tests/avm_minimal.test.ts
@@ -1,6 +1,6 @@
 import { jsonParseWithSchema, jsonStringify } from '@aztec/foundation/json-rpc';
 import { readTestData, writeTestData } from '@aztec/foundation/testing/files';
-import { AvmCircuitInputs } from '@aztec/stdlib/avm';
+import { AvmCircuitInputs, PublicSimulatorConfig } from '@aztec/stdlib/avm';
 import { NativeWorldStateService } from '@aztec/world-state/native';
 
 import { executeAvmMinimalPublicTx, readAvmMinimalPublicTxInputsFromFile } from '../../fixtures/minimal_public_tx.js';
@@ -12,6 +12,14 @@ describe.each([
 ])('Public TX simulator apps tests: AvmMinimalTestContract ($simulatorName)', ({ useCppSimulator }) => {
   let worldStateService: NativeWorldStateService;
   let tester: PublicTxSimulationTester;
+  // Make sure we collect hints
+  const config: PublicSimulatorConfig = PublicSimulatorConfig.from({
+    skipFeeEnforcement: false,
+    collectCallMetadata: true,
+    collectDebugLogs: false,
+    collectHints: true,
+    collectStatistics: false,
+  });
 
   beforeEach(async () => {
     worldStateService = await NativeWorldStateService.tmp();
@@ -20,6 +28,7 @@ describe.each([
       /*globals=*/ undefined,
       /*metrics=*/ undefined,
       useCppSimulator,
+      config,
     );
   });
 
diff --git a/yarn-project/simulator/src/public/public_tx_simulator/cpp_public_tx_simulator.ts b/yarn-project/simulator/src/public/public_tx_simulator/cpp_public_tx_simulator.ts
@@ -147,22 +147,53 @@ export class CppPublicTxSimulator extends PublicTxSimulator implements PublicTxS
     assert(cppResult.gasUsed.teardownGas.equals(tsResult.gasUsed.teardownGas));
     assert(cppResult.gasUsed.billedGas.equals(tsResult.gasUsed.billedGas));
     assert(cppResult.publicInputs.toBuffer().equals(tsResult.publicInputs.toBuffer()));
-    // FIXME(https://github.com/AztecProtocol/aztec-packages/issues/18441): a few but not all keccaks fail!
-    // assert(cppResult.getAppLogicReturnValues().length === tsResult.getAppLogicReturnValues().length);
-    // for (let i = 0; i < cppResult.getAppLogicReturnValues().length; i++) {
-    //   assert(cppResult.getAppLogicReturnValues()[i].equals(tsResult.getAppLogicReturnValues()[i]));
-    // }
+    if (this.config?.collectCallMetadata) {
+      assert(cppResult.getAppLogicReturnValues().length === tsResult.getAppLogicReturnValues().length);
+      for (let i = 0; i < cppResult.getAppLogicReturnValues().length; i++) {
+        assert(cppResult.getAppLogicReturnValues()[i].equals(tsResult.getAppLogicReturnValues()[i]));
+      }
+    }
     // Messages are still not ok for exceptional halts (they are not plumbed in C++).
     const cppRevertReason = cppResult.findRevertReason() || {};
     const tsRevertReason = tsResult.findRevertReason() || {};
     const cppRevertReasonAsObject = JSON.parse(JSON.stringify(cppRevertReason));
     const tsRevertReasonAsObject = JSON.parse(JSON.stringify(tsRevertReason));
     if (JSON.stringify(cppRevertReasonAsObject) !== JSON.stringify(tsRevertReasonAsObject)) {
-      console.log('cppResult.findRevertReason()', cppRevertReasonAsObject);
-      console.log('tsResult.findRevertReason()', tsRevertReasonAsObject);
+      this.log.debug('cppResult.findRevertReason()', cppRevertReasonAsObject);
+      this.log.debug('tsResult.findRevertReason()', tsRevertReasonAsObject);
     }
     // TODO: dont compare the strings since this is not deterministic.
-    assert(JSON.stringify(cppRevertReasonAsObject) === JSON.stringify(tsRevertReasonAsObject));
+
+    // Sometimes error messages are different between C++ and TS, so we omit in the default comparison
+    const cppRevertReasonWithoutMessage = { ...cppRevertReasonAsObject, originalMessage: undefined };
+    const tsRevertReasonWithoutMessage = { ...tsRevertReasonAsObject, originalMessage: undefined };
+    assert(JSON.stringify(cppRevertReasonWithoutMessage) === JSON.stringify(tsRevertReasonWithoutMessage));
+
+    const cppHasRevertMessage =
+      cppRevertReasonAsObject.originalMessage && cppRevertReasonAsObject.originalMessage.length > 0;
+    const tsHasRevertMessage =
+      tsRevertReasonAsObject.originalMessage && tsRevertReasonAsObject.originalMessage.length > 0;
+    // assert that if one of the error messages is non-empty, the other is
+    assert(
+      cppHasRevertMessage === tsHasRevertMessage,
+      'One of the AVM simulators (C++ or TS) produced a revert message, but the other did not',
+    );
+    // Ideally, we'd love to be able to compare full error messages, but without a lot of work
+    // the two simulators will always be able to produce some differing errors.
+    // Commenting out the code below will enforce that the error messages are at least
+    // similar (one contains the other). Even this is not something we can guarantee.
+    //if (cppHasRevertMessage) {
+    //  const cppRevertMessageContainsTs = cppRevertReasonAsObject.originalMessage.includes(
+    //    tsRevertReasonAsObject.originalMessage,
+    //  );
+    //  const tsRevertMessageContainsCpp = tsRevertReasonAsObject.originalMessage.includes(
+    //    cppRevertReasonAsObject.originalMessage,
+    //  );
+    //  assert(
+    //    cppRevertMessageContainsTs || tsRevertMessageContainsCpp,
+    //    'The AVM simulators (C++ and TS) produced different revert messages (neither was a substring of the other)',
+    //  );
+    //}
 
     // Confirm that tree roots match
     const cppStateRef = await this.merkleTree.getStateReference();
@@ -178,7 +209,6 @@ export class CppPublicTxSimulator extends PublicTxSimulator implements PublicTxS
       cppGasUsed: tsResult.gasUsed.totalGas.l2Gas,
     });
 
-    // TODO(dbanks12): C++ should return PublicTxResult (or something similar)
     return tsResult;
   }
 }
@@ -293,7 +323,8 @@ export class CppPublicTxSimulatorHintedDbs extends PublicTxSimulator implements
  */
 export class MeasuredCppPublicTxSimulatorHintedDbs
   extends CppPublicTxSimulatorHintedDbs
-  implements MeasuredPublicTxSimulatorInterface {
+  implements MeasuredPublicTxSimulatorInterface
+{
   constructor(
     merkleTree: MerkleTreeWriteOperations,
     contractsDB: PublicContractsDB,
diff --git a/yarn-project/simulator/src/public/state_manager/state_manager.ts b/yarn-project/simulator/src/public/state_manager/state_manager.ts
@@ -11,6 +11,7 @@ import { Fr } from '@aztec/foundation/fields';
 import { jsonStringify } from '@aztec/foundation/json-rpc';
 import { type LogLevel, createLogger } from '@aztec/foundation/log';
 import { ProtocolContractAddress } from '@aztec/protocol-contracts';
+import { FunctionSelector } from '@aztec/stdlib/abi';
 import { AztecAddress } from '@aztec/stdlib/aztec-address';
 import type { ContractClassPublicWithCommitment, ContractInstanceWithAddress } from '@aztec/stdlib/contract';
 import { SerializableContractInstance } from '@aztec/stdlib/contract';
@@ -26,7 +27,7 @@ import { strict as assert } from 'assert';
 
 import type { AvmExecutionEnvironment } from '../avm/avm_execution_environment.js';
 import type { PublicContractsDBInterface } from '../db_interfaces.js';
-import { getPublicFunctionDebugName } from '../debug_fn_name.js';
+import { getPublicFunctionDebugName, getPublicFunctionSelectorAndName } from '../debug_fn_name.js';
 import type { PublicTreesDB } from '../public_db_sources.js';
 import {
   L1ToL2MessageIndexOutOfRangeError,
@@ -532,6 +533,12 @@ export class PublicPersistableStateManager {
     return await getPublicFunctionDebugName(this.contractsDB, avmEnvironment.address, avmEnvironment.calldata);
   }
 
+  public async getPublicFunctionSelectorAndName(
+    avmEnvironment: AvmExecutionEnvironment,
+  ): Promise<{ functionSelector?: FunctionSelector; functionName?: string }> {
+    return await getPublicFunctionSelectorAndName(this.contractsDB, avmEnvironment.address, avmEnvironment.calldata);
+  }
+
   public async padTree(treeId: MerkleTreeId, leavesToInsert: number): Promise<void> {
     await this.treesDB.padTree(treeId, leavesToInsert);
   }

Original file line number	Diff line number	Diff line change
`@@ -101,7 +101,7 @@ struct ContractCallbacks {`
`101`	`101`	`Napi::Value AvmSimulateNapi::simulate(const Napi::CallbackInfo& cb_info)`
`102`	`102`	`{`
`103`	`103`	`// TODO(dbanks12): configurable verbosity (maybe based on TS log level)`
`104`		`- verbose_logging = true;`
	`104`	`+ // verbose_logging = true;`
`105`	`105`	`// debug_logging = true;`
`106`	`106`
`107`	`107`	`Napi::Env env = cb_info.Env();`
Original file line number	Diff line number	Diff line change
`@@ -90,19 +90,17 @@ export class SimpleContractDataSource implements ContractDataSource {`
`90`	`90`	`return this.contractArtifacts.get(contractInstance!.currentContractClassId.toString());`
`91`	`91`	`}`
`92`	`92`
`93`		`- async getDebugFunctionName(address: AztecAddress, selector: FunctionSelector): Promise<string> {`
	`93`	`+ async getDebugFunctionName(address: AztecAddress, selector: FunctionSelector): Promise<string \| undefined> {`
`94`	`94`	`const contractInstance = await this.getContract(address);`
`95`	`95`	`if (!contractInstance) {`
`96`		`- this.logger.warn(`
`97`		- `Couldn't get fn name for debugging. Contract not in tester's ContractDataSource. Using selector:${selector} instead...`,
`98`		`- );`
`99`		- return `selector:${selector.toString()}`;
	`96`	+ this.logger.warn(`Couldn't get fn name for debugging. Contract not in tester's ContractDataSource.`);
	`97`	`+ return undefined;`
`100`	`98`	`}`
`101`	`99`	const key = `${contractInstance.currentContractClassId.toString()}:${selector.toString()}`;
`102`	`100`	`const fnName = this.debugFunctionName.get(key);`
`103`	`101`	`if (!fnName) {`
`104`		- this.logger.warn(`Couldn't get fn name for debugging. Using selector:${selector} instead...`);
`105`		`- return selector.toString();`
	`102`	+ this.logger.warn(`Couldn't get fn name for debugging...`);
	`103`	`+ return undefined;`
`106`	`104`	`}`
`107`	`105`	`return fnName;`
`108`	`106`	`}`