feat(avm): ephemeral avm tree by IlyasRidhuan · Pull Request #9798 · AztecProtocol/aztec-packages

IlyasRidhuan · 2024-11-07T11:51:46Z

This adds forkable trees so that we can use them in the avm to handle context changes.

The existing native trees are missing the ability to arbitrarily fork from uncommitted state. This is a requirement for our one-tx processing as we need to be able to fork-rollback-fork when encountering a change in the avm context (e.g. in a nested call).

These trees store the difference between the initial tx state (stored in the native world state db) and the current ephmeral state during transaction processing. Queries are either handled by the ephemeral tree or proxied to the db.

Currently there are definitely some room for performance improvements. Namely:

Storage of a sparse, value-sorted data structure for the indexed tree leaf updates.
- This would allow for fast low nullifier lookup (which will be the bottleneck for indexed tree operations)
Copy-on-write when forking
- Some nested calls may not perform any operations on the trees (or perhaps a subset of tree). Forking all the information may unnecessarily consume memory for deeper nested contexts.

IlyasRidhuan · 2024-11-07T11:52:00Z

feat(avm): integrate ephemeral trees #9917
feat(avm): more efficient low leaf search #9870
test(avm): minor benchmarking #9869
feat(avm): ephemeral avm tree #9798 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

dbanks12

Nice! Lots of nits and some feedback on naming.

I don't have an easy time following some of this. In particular the frontier stuff. And I wish we didn't have to do so much of this from scratch and could reuse one of the existing merkle implementations that exists in the codebase.

I would get Alex's or Phil's eyes on this for approval since they have merkle trees fresher in mind.

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

dbanks12 · 2024-11-07T17:41:51Z

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

+    switch (tree.tag) {
+      case TreeType.BRANCH: {
+        return insertPath.pop() === 0
+          ? Branch(this._insertLeaf(value, insertPath, depth - 1, tree.leftTree, appendMode), tree.rightTree)
+          : Branch(tree.leftTree, this._insertLeaf(value, insertPath, depth - 1, tree.rightTree, appendMode));
+      }
+      case TreeType.LEAF:
+      case TreeType.EMPTY: {
+        const zeroLeaf = appendMode ? Leaf(this.zeroHashes[depth - 1]) : Empty();
+        return insertPath.pop() === 0
+          ? Branch(this._insertLeaf(value, insertPath, depth - 1, zeroLeaf, appendMode), zeroLeaf)
+          : Branch(zeroLeaf, this._insertLeaf(value, insertPath, depth - 1, zeroLeaf, appendMode));


I'd break these ternary operators up into ifs and assign the results of each _insertLeaf to a meaningfully named variables like updatedLeftTree, etc

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

dbanks12 · 2024-11-07T17:45:57Z

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

+   * Recursively hashes the subtree
+   * @param tree - The tree to be hashed
+   * @param depth - The depth of the tree
+   */
+  public hashTree(tree: Tree, depth: number): Fr {
+    switch (tree.tag) {


consider renaming hashSubtree

fcarreiro

Lots of stuff! I cannot guarantee that I understand all of it, but tests seem reasonable.

Does it work? We'll know soon :D

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

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

fcarreiro · 2024-11-08T16:28:50Z

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

+    const computedRoot = treeContainer.treeMap.get(treeId)!.getRoot();
+    expect(computedRoot.toBuffer()).toEqual(wsRoot);
+
+    const wsSiblingPath = await worldStateTrees.getSiblingPath(treeId, getSiblingIndex, true);


Since there are not many, would it make sense to compare every single sibling path?

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

IlyasRidhuan · 2024-11-11T09:41:04Z

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

+  rightTree: Tree;
+};
+
+type Tree = Leaf | Branch;


@dbanks12 removed EMPTY

dbanks12

I think that once we're happy with this implementation it MIGHT make sense to move it into a merkle library and not call it "Avm*". But for now this feels right. Nice work!

dbanks12 · 2024-11-14T02:56:11Z

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

+type PreimageWitness<T extends IndexedTreeLeafPreimage> = {
+  preimage: T;
+  index: bigint;
+  update: boolean;
+};


I still think this name isn't great. Elsewhere we use "witness" in the context of merkle trees to be leaf index & sibling path. I don't know what a better name is, but PreimageWitness doesn't feel right

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

* master: (67 commits) chore: Fix bad merge on AztecLMDBStore initializer feat: add persisted database of proving jobs (#9942) chore: Clean up data configuration (#9973) chore: remove public kernels (#10027) chore: misc cleanup, docs and renaming (#9968) feat: IPA Accumulator in Builder (#9846) chore(docs): Updates to token contract (#9954) test(avm): minor benchmarking (#9869) chore(ci): run `l1-contracts` CI in parallel with `build` step (#10024) chore: build acir test programs in parallel to e2e build step (#9988) chore: pull out `array_set` pass changes (#9993) feat(avm): ephemeral avm tree (#9798) fix: don't take down runners with faulty runner check (#10019) feat(docs): add transaction profiler docs (#9932) chore: hotfix runner wait (#10018) refactor: remove EnqueuedCallSimulator (#10015) refactor: stop calling public kernels (#9971) git subrepo push --branch=master noir-projects/aztec-nr git_subrepo.sh: Fix parent in .gitrepo file. [skip ci] chore: replace relative paths to noir-protocol-circuits ...

IlyasRidhuan marked this pull request as ready for review November 7, 2024 11:58

IlyasRidhuan requested review from dbanks12 and fcarreiro as code owners November 7, 2024 11:58

IlyasRidhuan changed the title ~~feat(avm): ephmeral avm tree~~ feat(avm): ephemeral avm tree Nov 7, 2024

IlyasRidhuan force-pushed the ir/11-03-feat_avm_ephmeral_avm_tree branch from 5ac16a3 to 07f1316 Compare November 7, 2024 15:03

dbanks12 requested changes Nov 7, 2024

View reviewed changes

fcarreiro reviewed Nov 8, 2024

View reviewed changes

IlyasRidhuan force-pushed the ir/11-03-feat_avm_ephmeral_avm_tree branch 5 times, most recently from df8067d to 4c99c3d Compare November 10, 2024 18:12

This was referenced Nov 10, 2024

test(avm): minor benchmarking #9869

Merged

feat(avm): more efficient low leaf search #9870

Merged

IlyasRidhuan commented Nov 11, 2024

View reviewed changes

IlyasRidhuan force-pushed the ir/11-03-feat_avm_ephmeral_avm_tree branch 4 times, most recently from da0ecb5 to 13e10c1 Compare November 13, 2024 00:57

IlyasRidhuan mentioned this pull request Nov 13, 2024

feat(avm): integrate ephemeral trees #9917

Merged

IlyasRidhuan force-pushed the ir/11-03-feat_avm_ephmeral_avm_tree branch from 13e10c1 to d9eecce Compare November 13, 2024 15:49

IlyasRidhuan requested review from dbanks12 and fcarreiro November 13, 2024 19:04

dbanks12 approved these changes Nov 14, 2024

View reviewed changes

dbanks12 reviewed Nov 14, 2024

View reviewed changes

yarn-project/simulator/src/avm/avm_tree.ts Outdated Show resolved Hide resolved

dbanks12 reviewed Nov 14, 2024

View reviewed changes

yarn-project/simulator/src/avm/avm_tree.ts Outdated Show resolved Hide resolved

IlyasRidhuan added 2 commits November 14, 2024 21:02

feat(avm): ephmeral avm tree

2c7c457

feat(avm): indexed tree shenanigans

42178fc

IlyasRidhuan added 7 commits November 14, 2024 21:02

feat(avm): indexed low nullifier updates

97311a8

refactor(avm): better generics, less readable

634abf6

test(avm): some decent tests and changes

3666901

feat(avm): add forking

59cb74b

doc: clean up and comment

2021808

doc: clean up and comment

3acd6b4

fix: address comments

f152568

IlyasRidhuan force-pushed the ir/11-03-feat_avm_ephmeral_avm_tree branch from d9eecce to f152568 Compare November 14, 2024 21:29

IlyasRidhuan merged commit 41743d0 into master Nov 18, 2024

IlyasRidhuan deleted the ir/11-03-feat_avm_ephmeral_avm_tree branch November 18, 2024 17:56

AztecBot mentioned this pull request Nov 18, 2024

chore(master): Release 0.63.0 #9651

Merged

Conversation

IlyasRidhuan commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IlyasRidhuan commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbanks12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbanks12 Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbanks12 Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

fcarreiro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fcarreiro Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IlyasRidhuan Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

dbanks12 left a comment

Choose a reason for hiding this comment

Uh oh!

dbanks12 Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IlyasRidhuan commented Nov 7, 2024 •

edited

Loading

IlyasRidhuan commented Nov 7, 2024 •

edited

Loading