fix: do not rebind nodes if child ViewDU is not changed (#380)

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) {

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);

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);

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);
+  });
+});