From deecd52f9f1e94c85c8d9e8c7293800cc2f4f7a6 Mon Sep 17 00:00:00 2001
From: twoeths <vutuyen2636@gmail.com>
Date: Thu, 11 Jul 2024 10:07:48 +0700
Subject: [PATCH] fix: do not rebind nodes if child ViewDU is not changed
 (#380)

---
 packages/ssz/src/viewDU/abstract.ts           |  2 +-
 packages/ssz/src/viewDU/arrayComposite.ts     |  9 ++++--
 packages/ssz/src/viewDU/container.ts          |  9 ++++--
 .../ssz/test/unit/unchangedViewDUs.test.ts    | 29 +++++++++++++++++++
 4 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100644 packages/ssz/test/unit/unchangedViewDUs.test.ts

diff --git a/packages/ssz/src/viewDU/abstract.ts b/packages/ssz/src/viewDU/abstract.ts
index 1398fb20..90190b0a 100644
--- a/packages/ssz/src/viewDU/abstract.ts
+++ b/packages/ssz/src/viewDU/abstract.ts
@@ -67,7 +67,7 @@ export abstract class TreeViewDU<T extends CompositeType<unknown, unknown, unkno
     // remember not to do a commit() before calling this function
     // in ethereum consensus, the only type goes with TVDU is BeaconState and it's really more efficient to hash the tree in batch
     // if consumers don't want to batch hash, just go with `this.node.root` similar to what View.hashTreeRoot() does
-
+    // there should not be another ViewDU.hashTreeRoot() during this flow so it's safe to reuse nextHashComps
     const hashComps = nextHashComps;
     this.commit(hashComps);
     if (nextHashComps.byLevel.length > 0 || nextHashComps.offset !== 0) {
diff --git a/packages/ssz/src/viewDU/arrayComposite.ts b/packages/ssz/src/viewDU/arrayComposite.ts
index a257bcbe..6666a3a8 100644
--- a/packages/ssz/src/viewDU/arrayComposite.ts
+++ b/packages/ssz/src/viewDU/arrayComposite.ts
@@ -197,9 +197,12 @@ export class ArrayCompositeTreeViewDU<
 
     for (const [index, view] of this.viewsChanged) {
       const node = this.type.elementType.commitViewDU(view, hashCompsView);
-      // Set new node in nodes array to ensure data represented in the tree and fast nodes access is equal
-      this.nodes[index] = node;
-      nodesChanged.push({index, node});
+      // there's a chance the view is not changed, no need to rebind nodes in that case
+      if (this.nodes[index] !== node) {
+        // Set new node in nodes array to ensure data represented in the tree and fast nodes access is equal
+        this.nodes[index] = node;
+        nodesChanged.push({index, node});
+      }
 
       // Cache the view's caches to preserve it's data after 'this.viewsChanged.clear()'
       const cache = this.type.elementType.cacheOfViewDU(view);
diff --git a/packages/ssz/src/viewDU/container.ts b/packages/ssz/src/viewDU/container.ts
index ed01b958..a5eb515a 100644
--- a/packages/ssz/src/viewDU/container.ts
+++ b/packages/ssz/src/viewDU/container.ts
@@ -100,9 +100,12 @@ class ContainerTreeViewDU<Fields extends Record<string, Type<unknown>>> extends
     for (const [index, view] of this.viewsChanged) {
       const fieldType = this.type.fieldsEntries[index].fieldType as unknown as CompositeTypeAny;
       const node = fieldType.commitViewDU(view, hashCompsView);
-      // Set new node in nodes array to ensure data represented in the tree and fast nodes access is equal
-      this.nodes[index] = node;
-      nodesChanged.push({index, node});
+      // there's a chance the view is not changed, no need to rebind nodes in that case
+      if (this.nodes[index] !== node) {
+        // Set new node in nodes array to ensure data represented in the tree and fast nodes access is equal
+        this.nodes[index] = node;
+        nodesChanged.push({index, node});
+      }
 
       // Cache the view's caches to preserve it's data after 'this.viewsChanged.clear()'
       const cache = fieldType.cacheOfViewDU(view);
diff --git a/packages/ssz/test/unit/unchangedViewDUs.test.ts b/packages/ssz/test/unit/unchangedViewDUs.test.ts
new file mode 100644
index 00000000..98e696c9
--- /dev/null
+++ b/packages/ssz/test/unit/unchangedViewDUs.test.ts
@@ -0,0 +1,29 @@
+import {expect} from "chai";
+import * as sszAltair from "../lodestarTypes/altair/sszTypes";
+import {getRandomState} from "../utils/generateEth2Objs";
+
+describe("Unchanged ViewDUs", () => {
+  const state = sszAltair.BeaconState.toViewDU(getRandomState(100));
+
+  it("should not recompute hashTreeRoot() when no fields is changed", () => {
+    const root = state.hashTreeRoot();
+    // this causes viewsChanged inside BeaconState container
+    state.validators.length;
+    state.balances.length;
+    // but we should not recompute root, should get from cache instead
+    const root2 = state.hashTreeRoot();
+    expect(root2).to.equal(root, "should not recompute hashTreeRoot() when no fields are changed");
+  });
+
+  it("handle childViewDU.hashTreeRoot()", () => {
+    const state2 = state.clone();
+    state2.latestBlockHeader.stateRoot = Buffer.alloc(32, 3);
+    const root2 = state2.hashTreeRoot();
+    const state3 = state.clone();
+    state3.latestBlockHeader.stateRoot = Buffer.alloc(32, 3);
+    // hashTreeRoot() also does the commit()
+    state3.latestBlockHeader.commit();
+    const root3 = state3.hashTreeRoot();
+    expect(root3).to.be.deep.equal(root2);
+  });
+});