feat: AVM uses native checkpointing trees instead of TS ephemeral forest #11955

dbanks12 · 2025-02-12T18:47:37Z

Note: in a later PR, we need to iron out a better WorldStateDB interface because it's kind of a sham at the moment.

Performance improvements

For execution of 100 public transfers in the public processor we went from ~11.5s down to ~6.9 seconds (~40% improvement).

Before:

After:

Changes

AVM State Manager

Deleted Ephemeral forest
State manager performs all merkle accesses directly on the native world state
Forking the state manager calls db.createCheckpoint(). Reverting calls db.revertCheckpoint. Returning/merging calls db.commitCheckpoint()`.
Added more tree insertion tests to avm_simulator.test.ts

Public processor

No longer re-performs merkle insertions for txs that have public calls.
Calls db.createCheckpoint() before processing of a TX. Calls db.revertCheckpoint() or db.commitCheckpoint() afterwards based on the result of execution.
Since the AVM performs merkle insertions itself, the public processor just trusts the state after AVM execution.
If a TX doesn't have public calls, the public processor still performs all merkle insertions.
Post-processing TX validation can't check for duplicate nullifiers anymore if there are public calls because the nullifiers have already been inserted into the tree by the AVM. Duplicate nullifiers will be caught by the AVM / public tx simulator.

WorldStateDB

Removed unused functions
AvmStateManager directly calls the underlying merkle trees often because it needs to. We probably need to iron out a better WorldStateDB interface since it's not sufficient if we need to directly call the underlying MerkleTreeWriteOperations interface often.

TXE

TXE does db.createCheckpoint() before calling PublicTxSimulator and then always does db.revertCheckpoint() afterwards because right now we execute just a single enqueued call at a time. Later during commitState(), the TXE re-applies all state changes from txEffects.
We will iron out a better way to handle public execution in the TXE in the coming days.

…f TS ephemeral forest

dbanks12 · 2025-02-20T18:49:09Z

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

-
-  /**
-   * Commit the pending public changes to the DB.
-   * @returns Nothing.
-   */
-  commit(): Promise<void> {
-    for (const [k, v] of this.publicCheckpointedWriteCache) {
-      this.publicCommittedWriteCache.set(k, v);
-    }
-    // uncommitted writes take precedence over checkpointed writes
-    // since they are the most recent
-    for (const [k, v] of this.publicUncommittedWriteCache) {
-      this.publicCommittedWriteCache.set(k, v);
-    }
-    return this.rollbackToCommit();
-  }
-
-  /**
-   * Rollback the pending public changes.
-   * @returns Nothing.
-   */
-  async rollbackToCommit(): Promise<void> {
-    await this.rollbackToCheckpoint();
-    this.publicCheckpointedWriteCache = new Map<bigint, Fr>();
-    return Promise.resolve();
-  }
-
-  checkpoint(): Promise<void> {
-    for (const [k, v] of this.publicUncommittedWriteCache) {
-      this.publicCheckpointedWriteCache.set(k, v);
-    }
-    return this.rollbackToCheckpoint();
-  }
-
-  rollbackToCheckpoint(): Promise<void> {
-    this.publicUncommittedWriteCache = new Map<bigint, Fr>();
-    return Promise.resolve();
-  }


No longer need caching in this class. AvmStateManager handles caching when merkle ops are off. When merkle ops are on, checkpointed trees are used which is slower than "caching", but necessary. We can probably turn merkle ops off for block building eventually, but that will require quite a bit of work.

dbanks12 · 2025-02-20T18:49:47Z

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

Test doesn't make sense anymore with caching removed from db

dbanks12 · 2025-02-20T18:50:19Z

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

-    const siloedNullifiers = [new Fr(10000), new Fr(20000), new Fr(30000), new Fr(40000), new Fr(50000)];
+    const siloedNullifiers = [new Fr(0x10000), new Fr(0x20000), new Fr(0x30000), new Fr(0x40000), new Fr(0x50000)];


Just for readability of logs

dbanks12 · 2025-02-20T18:50:53Z

yarn-project/txe/src/util/txe_world_state_db.ts

-  override checkpoint(): Promise<void> {
-    return Promise.resolve();
-  }
-  override rollbackToCheckpoint(): Promise<void> {
-    throw new Error('Cannot rollback');
-  }
-  override commit(): Promise<void> {
-    return Promise.resolve();
-  }
-  override rollbackToCommit(): Promise<void> {
-    throw new Error('Cannot rollback');


txe actually uses the underlying checkpoint functions now so we don't override

dbanks12 · 2025-02-20T18:51:42Z

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

-  public async checkExists(
-    siloedNullifier: Fr,
-  ): Promise<[/*exists=*/ boolean, /*isPending=*/ boolean, /*leafIndex=*/ Fr]> {
+  public async checkExists(siloedNullifier: Fr): Promise<{ exists: boolean; cacheHit: boolean }> {


Object is cleaner than array for return type

dbanks12 · 2025-02-20T18:52:48Z

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

-    it('Can get undefined contract instance', async () => {
+    it('Can get undefined bytecode', async () => {


wrong description

dbanks12 · 2025-02-20T18:54:19Z

yarn-project/p2p/src/msg_validators/tx_validator/double_spend_validator.ts

-    const nullifiers = tx instanceof Tx ? tx.data.getNonEmptyNullifiers() : tx.txEffect.nullifiers;
+    // Don't need to check for duplicate nullifiers if the tx has public calls
+    // because the AVM will perform merkle insertions as it goes and will fail on
+    // duplicate nullifier. In fact we CANNOT check here because the nullifiers
+    // have already been inserted, and so they will exist in nullifierSource.
+    if (!hasPublicCalls(tx)) {
+      const nullifiers = tx instanceof Tx ? tx.data.getNonEmptyNullifiers() : tx.txEffect.nullifiers;

-    // Ditch this tx if it has repeated nullifiers
-    const uniqueNullifiers = new Set(nullifiers);
-    if (uniqueNullifiers.size !== nullifiers.length) {
-      this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for emitting duplicate nullifiers`);
-      return { result: 'invalid', reason: ['Duplicate nullifier in tx'] };
-    }
+      // Ditch this tx if it has repeated nullifiers
+      const uniqueNullifiers = new Set(nullifiers);
+      if (uniqueNullifiers.size !== nullifiers.length) {
+        this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for emitting duplicate nullifiers`);
+        return { result: 'invalid', reason: ['Duplicate nullifier in tx'] };
+      }

-    if ((await this.#nullifierSource.nullifiersExist(nullifiers.map(n => n.toBuffer()))).some(Boolean)) {
-      this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for repeating a nullifier`);
-      return { result: 'invalid', reason: ['Existing nullifier'] };
+      if ((await this.#nullifierSource.nullifiersExist(nullifiers.map(n => n.toBuffer()))).some(Boolean)) {
+        this.#log.warn(`Rejecting tx ${await Tx.getHash(tx)} for repeating a nullifier`);
+        return { result: 'invalid', reason: ['Existing nullifier'] };
+      }
    }
-


If public calls are present, can't check that nullifiers in txEffects don't already exist in the trees because the AVM now inserts them into the trees. Still need these checks for private-only txs.

sklppy88

Checking off on TXe, thanks for the lovely comments !

PhilWindle · 2025-02-20T23:05:38Z

yarn-project/txe/src/oracle/txe_oracle.ts

@@ -909,10 +916,14 @@ export class TXE implements TypedOracle {
    globalVariables.blockNumber = new Fr(this.blockNumber);
    globalVariables.gasFees = new GasFees(1, 1);

+    // Checkpoint here so that we can revert merkle ops after simulation.
+    // See note at revert below.
+    await db.createCheckpoint();


Should we use a try/finally here to ensure the checkpoint is always reverted?

We don't want to put a revert in finally because we don't want to revert it on success, right?

Oh sorry, this is in the TXE. I thought this was in the public processor. Yes, this should be in a try/finally, good call!

Moved into a try/finally!

PhilWindle · 2025-02-20T23:09:55Z

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

+        } else {
+          // Otherwise, perform all tree insertions for side effects from private
+          await this.commitTxState(processedTx);
+        }
        nullifierCache?.addNullifiers(processedTx.txEffect.nullifiers.map(n => n.toBuffer()));


Just wonder if this section should be in a try/catch so we don't hit the revert after having already run either commit or revert.

Good point! Will fix.

This whole block is already in a try/catch. I changed the commitCheckpoint to always happen on success, even for private-only txs. And I moved the commitCheckpoint to the very end of the try block. Does this make sense?
'

Also I renamed the old commitTxState to doTreeInsertionsForPrivateOnlyTx since that's what it actually does now. I thought I should leave that call where it is so that we don't update totalBlockGas/etc before we do the private-only-tx insertions that could theoretically blow up (although i don't think they should since TX post-processing should do duplicate-nullifier checks).

PhilWindle · 2025-02-20T23:11:03Z

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

+          await this.worldStateDB.commitCheckpoint();
+        } else {
+          // Otherwise, perform all tree insertions for side effects from private
+          await this.commitTxState(processedTx);


Does this commitTxState perform a commit or revert?

It doesn't look like it does. Doesn't this mean we never commit/revert the checkpoint in this case?

You're right! We would never commit or revert it in this case and that's a bug.

dbanks12 · 2025-02-21T12:13:02Z

Going to merge. In a follow-up PR, I'm going to add some sort of "fork counting" to the checkpointing to prevent someone from trying to revert a parent/grandparent that still has unresolved children.

dbanks12 requested a review from fcarreiro as a code owner February 12, 2025 18:47

dbanks12 changed the title ~~feat: AVM uses native checkpointing trees instead of TS ephemeral forest~~ [WIP] feat: AVM uses native checkpointing trees instead of TS ephemeral forest Feb 12, 2025

dbanks12 removed the request for review from fcarreiro February 12, 2025 18:47

dbanks12 changed the base branch from master to pw/checkpointing February 12, 2025 18:48

Base automatically changed from pw/checkpointing to master February 12, 2025 19:37

dbanks12 added the e2e-all CI: Deprecated, use ci-full. Enable all master checks. label Feb 13, 2025

dbanks12 added 22 commits February 15, 2025 16:23

chore: avm TS test cleanup and refactors

a0accc9

new test in orchestrator

33cf18b

Merge branch 'master' into db/testing-updates

20397fc

Merge branch 'master' into db/testing-updates

0e00834

fixes

caeec38

merge

504a63a

fixes

e6b5622

fix

b69a11d

more fixes

ba5fa4a

use new sha in benchmarking contract

8e5ac40

Fixes

5f05e9f

fixes & cleanup

4a6972c

Merge branch 'master' into db/testing-updates

4c0c17f

fix noir contract

41c536b

fix contract updates test

bc5806d

revert incorrect type change

6376465

fmt

5b9afbc

Merge branch 'master' into db/testing-updates

e1adf6a

merge

0fe5ec2

[WIP - attempt 2] feat: AVM uses native checkpointing trees instead o…

3347f14

…f TS ephemeral forest

fix feePayer

c1e8993

fix feePayer

d9defc5

dbanks12 force-pushed the db/avm-uses-checkpointing branch from 8460485 to d9defc5 Compare February 19, 2025 17:02

fix

adf62ae

dbanks12 added 2 commits February 19, 2025 21:03

merge

c9d8cd0

fix txe

847f643

dbanks12 changed the title ~~[WIP] feat: AVM uses native checkpointing trees instead of TS ephemeral forest~~ feat: AVM uses native checkpointing trees instead of TS ephemeral forest Feb 20, 2025

dbanks12 added 2 commits February 20, 2025 18:32

merge

bc0b6c3

fmt

b87ba9b

dbanks12 requested a review from IlyasRidhuan February 20, 2025 18:39

dbanks12 commented Feb 20, 2025

View reviewed changes

dbanks12 requested review from PhilWindle and sklppy88 February 20, 2025 19:10

sklppy88 approved these changes Feb 20, 2025

View reviewed changes

PhilWindle reviewed Feb 20, 2025

View reviewed changes

dbanks12 requested a review from PhilWindle February 21, 2025 02:20

address pr feedback - missing commit-checkpoints, add try/finally

0eb8470

IlyasRidhuan approved these changes Feb 21, 2025

View reviewed changes

PhilWindle added 3 commits February 21, 2025 11:27

Checkpoint lifecycle

66abe17

Typo

39e376e

Minor review change

b6fb9f0

move checkpoint file

3d2dfb9

dbanks12 enabled auto-merge (squash) February 21, 2025 12:30

dbanks12 merged commit 1cc5ed1 into master Feb 21, 2025
9 checks passed

dbanks12 deleted the db/avm-uses-checkpointing branch February 21, 2025 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: AVM uses native checkpointing trees instead of TS ephemeral forest #11955

feat: AVM uses native checkpointing trees instead of TS ephemeral forest #11955

dbanks12 commented Feb 12, 2025 •

edited

Loading

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

dbanks12 Feb 20, 2025

sklppy88 left a comment

PhilWindle Feb 20, 2025

dbanks12 Feb 21, 2025

dbanks12 Feb 21, 2025

dbanks12 Feb 21, 2025

PhilWindle Feb 20, 2025

dbanks12 Feb 21, 2025

dbanks12 Feb 21, 2025 •

edited

Loading

PhilWindle Feb 20, 2025

PhilWindle Feb 20, 2025

dbanks12 Feb 21, 2025

dbanks12 commented Feb 21, 2025

		const siloedNullifiers = [new Fr(10000), new Fr(20000), new Fr(30000), new Fr(40000), new Fr(50000)];
		const siloedNullifiers = [new Fr(0x10000), new Fr(0x20000), new Fr(0x30000), new Fr(0x40000), new Fr(0x50000)];

		it('Can get undefined contract instance', async () => {
		it('Can get undefined bytecode', async () => {

feat: AVM uses native checkpointing trees instead of TS ephemeral forest #11955

feat: AVM uses native checkpointing trees instead of TS ephemeral forest #11955

Conversation

dbanks12 commented Feb 12, 2025 • edited Loading

Note: in a later PR, we need to iron out a better WorldStateDB interface because it's kind of a sham at the moment.

Performance improvements

Changes

AVM State Manager

Public processor

WorldStateDB

TXE

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sklppy88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanks12 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanks12 commented Feb 21, 2025

dbanks12 commented Feb 12, 2025 •

edited

Loading

dbanks12 Feb 21, 2025 •

edited

Loading