fix: on nullifier collision during insertion of revertibles from private, app logic should fail gracefully and proceed to teardown #12422

dbanks12 · 2025-03-03T16:50:19Z

No description provided.

…ed earlier

…ck (#12390)

…ate, app logic should fail gracefully and proceed to tairdown

…rivate

dbanks12 · 2025-03-03T21:30:17Z

yarn-project/end-to-end/src/e2e_private_voting_contract.test.ts

      await expect(
        votingContract.methods.cast_vote(candidate).send({ skipPublicSimulation: true }).wait(),
-      ).rejects.toThrow('Reason: Tx dropped by P2P node.');
+      ).rejects.toThrow(TxStatus.APP_LOGIC_REVERTED);
    });


TxReceipt doesn't have an applogic-revert error message and it doesn't seem trivial to add it unfortunately.

dbanks12 · 2025-03-03T21:30:58Z

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

      const revertStart = process.hrtime.bigint();
-      // FIXME(#12375): TX shouldn't die if revertible insertions fail. Should just revert to snapshot.
-      await this.insertRevertiblesFromPrivate(context);
-      // add new contracts to the contracts db so that their functions may be found and called
-      await this.worldStateDB.addNewRevertibleContracts(tx);
-      const revertEnd = process.hrtime.bigint();
-      this.metrics.recordPrivateEffectsInsertion(Number(revertEnd - revertStart) / 1_000, 'revertible');
-
-      if (context.hasPhase(TxExecutionPhase.APP_LOGIC)) {
-        const appLogicResult: ProcessedPhase = await this.simulateAppLogicPhase(context);
-        processedPhases.push(appLogicResult);
+      const success = await this.insertRevertiblesFromPrivate(context);
+      if (success) {
+        // add new contracts to the contracts db so that their functions may be found and called
+        await this.worldStateDB.addNewRevertibleContracts(tx);
+        const revertEnd = process.hrtime.bigint();
+        this.metrics.recordPrivateEffectsInsertion(Number(revertEnd - revertStart) / 1_000, 'revertible');
+
+        // Only proceed with app logic if there was no revert during revertible insertion
+        if (context.hasPhase(TxExecutionPhase.APP_LOGIC)) {
+          const appLogicResult: ProcessedPhase = await this.simulateAppLogicPhase(context);
+          processedPhases.push(appLogicResult);
+        }
+      } else {
+        this.log.debug(`Revertible insertions failed. Skipping app logic.`);
      }


Only proceed to app logic if inserting revertibles succeeds.

dbanks12 · 2025-03-03T21:31:47Z

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

-  public async insertRevertiblesFromPrivate(context: PublicTxContext) {
+  public async insertRevertiblesFromPrivate(context: PublicTxContext): /*success=*/ Promise<boolean> {
    // Fork the state manager so we can rollback to end of setup if app logic reverts.
    await context.state.fork();
    const stateManager = context.state.getActiveStateManager();
    try {
      await stateManager.writeSiloedNullifiersFromPrivate(context.revertibleAccumulatedDataFromPrivate.nullifiers);
    } catch (e) {
      if (e instanceof NullifierCollisionError) {
-        // FIXME(#12375): simulator should be able to recover from this and just revert to snapshot.
-        throw new NullifierCollisionError(
-          `Nullifier collision encountered when inserting revertible nullifiers from private. Details:\n${e.message}\n.Stack:${e.stack}`,
+        // Instead of throwing, revert the app_logic phase
+        context.revert(
+          TxExecutionPhase.APP_LOGIC,
+          new SimulationError(
+            `Nullifier collision encountered when inserting revertible nullifiers from private\nDetails: ${e.message}\nError stack: ${e.stack}`,
+            [],
+          ),
+          /*culprit=*/ 'insertRevertiblesFromPrivate',
        );
+        return /*success=*/ false;
+      } else {
+        throw e;
      }
    }


Only rethrow if the error is NOT the known nullifier collision. Otherwise just revert app logic with the error and return success=false.

fcarreiro · 2025-03-03T21:54:48Z

yarn-project/simulator/src/public/public_tx_simulator/public_tx_simulator.test.ts

+    });
+
+    // Mock the writeSiloedNullifiersFromPrivate method to throw a NullifierCollisionError
+    (AvmPersistableStateManager.prototype as any).writeSiloedNullifiersFromPrivate = jest


Ugh, can't we mock it in a... saner way? 🙃

😅 I think maybe it shouldn't be mocked. I could craft a real-ish test of this instead.

Issue with mocking this function here is that it isn't an arg to PublicTxSimulator but is crafted inside. (this is probably something that could be solved by better dependency injection?)

fcarreiro · 2025-03-03T21:55:38Z

yarn-project/simulator/src/public/public_tx_simulator/public_tx_simulator.test.ts

+    expect(txResult.processedPhases.length).toBe(2);
+    expect(txResult.processedPhases[0].phase).toBe(TxExecutionPhase.SETUP);
+    expect(txResult.processedPhases[1].phase).toBe(TxExecutionPhase.TEARDOWN);
+


Why not expect(txResult.?processedPhases).toBe([TxExecutionPhase.SETUP, TxExecutionPhase.TEARDOWN]) ? (or toequals)

I think I copied other tests, but I agree this is nicer!

dbanks12 and others added 16 commits February 25, 2025 19:28

feat: ensure that later txs in block cannot wipe out contracts deploy…

80912f1

…ed earlier

merge & finish

e6bb7d8

cleanup

be0eaff

done

889c144

merge

3abe736

fix imports

9267a90

fix: ensure that contracts can be called after deployment in same blo…

90bebdd

…ck (#12390)

only add non-revertible contract info before setup

b274e10

unskip test

5840332

fix: on nullifier collision during insertion of revertibles from priv…

0e7516b

…ate, app logic should fail gracefully and proceed to tairdown

fix

5501966

patch test for now

583a77e

Merge branch 'db/tx-cache' into db/fail-gracefully-revertibles-from-p…

3a8fe96

…rivate

fix failing test

67beb43

sleep

bdacb35

Merge branch 'db/tx-cache' into db/fail-gracefully-revertibles-from-p…

1d5dc51

…rivate

Base automatically changed from db/tx-cache to master March 3, 2025 20:39

merge

a930138

dbanks12 requested review from sirasistant and fcarreiro and removed request for sirasistant March 3, 2025 21:28

dbanks12 commented Mar 3, 2025

View reviewed changes

fcarreiro approved these changes Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: on nullifier collision during insertion of revertibles from private, app logic should fail gracefully and proceed to teardown #12422

fix: on nullifier collision during insertion of revertibles from private, app logic should fail gracefully and proceed to teardown #12422

dbanks12 commented Mar 3, 2025

dbanks12 Mar 3, 2025

dbanks12 Mar 3, 2025

dbanks12 Mar 3, 2025

fcarreiro Mar 3, 2025

dbanks12 Mar 3, 2025

fcarreiro Mar 3, 2025

dbanks12 Mar 3, 2025

fix: on nullifier collision during insertion of revertibles from private, app logic should fail gracefully and proceed to teardown #12422

Are you sure you want to change the base?

fix: on nullifier collision during insertion of revertibles from private, app logic should fail gracefully and proceed to teardown #12422

Conversation

dbanks12 commented Mar 3, 2025

dbanks12 Mar 3, 2025

Choose a reason for hiding this comment

dbanks12 Mar 3, 2025

Choose a reason for hiding this comment

dbanks12 Mar 3, 2025

Choose a reason for hiding this comment

fcarreiro Mar 3, 2025

Choose a reason for hiding this comment

dbanks12 Mar 3, 2025

Choose a reason for hiding this comment

fcarreiro Mar 3, 2025

Choose a reason for hiding this comment

dbanks12 Mar 3, 2025

Choose a reason for hiding this comment