fix: do not change hashTreeRoot()

packages/ssz/src/branchNodeStruct.ts

@@ -9,18 +9,15 @@ import {hashObjectToUint8Array, Node} from "@chainsafe/persistent-merkle-tree";
  * expensive because the tree has to be recreated every time.
  */
 export class BranchNodeStruct<T> extends Node {
-  constructor(
-    private readonly valueToNode: (value: T) => Node,
-    private readonly hashTreeRootInto: (value: T, node: Node) => void,
-    readonly value: T
-  ) {
+  constructor(private readonly valueToNode: (value: T) => Node, readonly value: T) {
     // First null value is to save an extra variable to check if a node has a root or not
     super(null as unknown as number, 0, 0, 0, 0, 0, 0, 0);
   }
 
   get rootHashObject(): HashObject {
     if (this.h0 === null) {
-      this.hashTreeRootInto(this.value, this);
+      const node = this.valueToNode(this.value);
+      super.applyHash(node.rootHashObject);
     }
     return this;
   }

packages/ssz/src/type/containerNodeStruct.ts

@@ -1,5 +1,4 @@
 import {Node} from "@chainsafe/persistent-merkle-tree";
-import {byteArrayIntoHashObject} from "@chainsafe/as-sha256";
 import {Type, ByteViews} from "./abstract";
 import {isCompositeType} from "./composite";
 import {ContainerType, ContainerOptions, renderContainerTypeName} from "./container";
@@ -74,7 +73,7 @@ export class ContainerNodeStructType<Fields extends Record<string, Type<unknown>
 
   tree_deserializeFromBytes(data: ByteViews, start: number, end: number): Node {
     const value = this.value_deserializeFromBytes(data, start, end);
-    return new BranchNodeStruct(this.valueToTree.bind(this), this.computeRootInto.bind(this), value);
+    return new BranchNodeStruct(this.valueToTree.bind(this), value);
   }
 
   // Proofs
@@ -95,7 +94,7 @@ export class ContainerNodeStructType<Fields extends Record<string, Type<unknown>
     super.tree_serializeToBytes({uint8Array, dataView}, 0, node);
     const value = this.value_deserializeFromBytes({uint8Array, dataView}, 0, uint8Array.length);
     return {
-      node: new BranchNodeStruct(this.valueToTree.bind(this), this.computeRootInto.bind(this), value),
+      node: new BranchNodeStruct(this.valueToTree.bind(this), value),
       done: true,
     };
   }
@@ -107,7 +106,7 @@ export class ContainerNodeStructType<Fields extends Record<string, Type<unknown>
   }
 
   value_toTree(value: ValueOfFields<Fields>): Node {
-    return new BranchNodeStruct(this.valueToTree.bind(this), this.computeRootInto.bind(this), value);
+    return new BranchNodeStruct(this.valueToTree.bind(this), value);
   }
 
   private valueToTree(value: ValueOfFields<Fields>): Node {
@@ -116,13 +115,4 @@ export class ContainerNodeStructType<Fields extends Record<string, Type<unknown>
     this.value_serializeToBytes({uint8Array, dataView}, 0, value);
     return super.tree_deserializeFromBytes({uint8Array, dataView}, 0, uint8Array.length);
   }
-
-  private computeRootInto(value: ValueOfFields<Fields>, node: Node): void {
-    if (node.h0 !== null) {
-      return;
-    }
-
-    this.hashTreeRootInto(value, this.temporaryRoot, 0);
-    byteArrayIntoHashObject(this.temporaryRoot, 0, node);
-  }
 }

packages/ssz/src/view/containerNodeStruct.ts

@@ -60,7 +60,7 @@ export function getContainerTreeViewClass<Fields extends Record<string, Type<unk
 
           // TODO: Should this check for valid field name? Benchmark the cost
           newNodeValue[fieldName] = value as ValueOf<Fields[keyof Fields]>;
-          this.tree.rootNode = new BranchNodeStruct(node["valueToNode"], node["hashTreeRootInto"], newNodeValue);
+          this.tree.rootNode = new BranchNodeStruct(node["valueToNode"], newNodeValue);
         },
       });
     }
@@ -86,7 +86,7 @@ export function getContainerTreeViewClass<Fields extends Record<string, Type<unk
 
           // TODO: Should this check for valid field name? Benchmark the cost
           newNodeValue[fieldName] = fieldType.toValueFromView(view) as ValueOf<Fields[keyof Fields]>;
-          this.tree.rootNode = new BranchNodeStruct(node["valueToNode"], node["hashTreeRootInto"], newNodeValue);
+          this.tree.rootNode = new BranchNodeStruct(node["valueToNode"], newNodeValue);
         },
       });
     }

packages/ssz/test/lodestarTypes/phase0/viewDU/listValidator.ts

@@ -38,6 +38,7 @@ const validatorRoots: Uint8Array[] = [];
 for (let i = 0; i < PARALLEL_FACTOR; i++) {
   validatorRoots.push(batchLevel3Bytes.subarray(i * 32, (i + 1) * 32));
 }
+const validatorRoot = new Uint8Array(32);
 
 export class ListValidatorTreeViewDU extends ListCompositeTreeViewDU<ValidatorNodeStructType> {
   constructor(
@@ -49,6 +50,11 @@ export class ListValidatorTreeViewDU extends ListCompositeTreeViewDU<ValidatorNo
   }
 
   commit(hcOffset = 0, hcByLevel: HashComputationLevel[] | null = null): void {
+    if (hcByLevel === null) {
+      // this is not from batchHashTreeRoot() call, go with regular flow
+      return super.commit();
+    }
+
     const isOldRootHashed = this._rootNode.h0 !== null;
     if (this.viewsChanged.size === 0) {
       if (!isOldRootHashed && hcByLevel !== null) {
@@ -112,8 +118,11 @@ export class ListValidatorTreeViewDU extends ListCompositeTreeViewDU<ValidatorNo
       const viewIndex = indicesChanged[i];
       const viewChanged = viewsChanged.get(viewIndex);
       if (viewChanged) {
-        // commit and hash
+        // commit
         viewChanged.commit();
+        // compute root for each validator
+        viewChanged.type.hashTreeRootInto(viewChanged.value, validatorRoot, 0);
+        byteArrayIntoHashObject(validatorRoot, 0, viewChanged.node);
         nodesChanged.push({index: viewIndex, node: viewChanged.node});
         // Set new node in nodes array to ensure data represented in the tree and fast nodes access is equal
         this.nodes[viewIndex] = viewChanged.node;

packages/ssz/test/perf/eth2/beaconState.test.ts

@@ -6,9 +6,9 @@ import {preset} from "../../lodestarTypes/params";
 const {SLOTS_PER_HISTORICAL_ROOT, EPOCHS_PER_ETH1_VOTING_PERIOD, SLOTS_PER_EPOCH} = preset;
 
 const vc = 200_000;
-const numModified = vc / 20;
+const numModified = vc / 2;
 // every we increase vc, need to change this value from "recursive hash" test
-const expectedRoot = "0x759d635af161ac1e4f4af11aa7721fd4996253af50f8a81e5003bbb4cbcaae42";
+const expectedRoot = "0xb0780ec0d44bff1ae8a351e98e37a9d8c3e28edb38c9d5a6312656e0cba915d9";
 
 /**
  * This simulates a BeaconState being modified after an epoch transition in lodestar
@@ -22,71 +22,73 @@ describe(`BeaconState ViewDU partially modified tree vc=${vc} numModified=${numM
     minMs: 20_000,
   });
 
+  const hc = new HashComputationGroup();
   itBench({
-    id: `BeaconState ViewDU hashTreeRoot() vc=${vc}`,
+    id: `BeaconState ViewDU batchHashTreeRoot vc=${vc}`,
     beforeEach: () => createPartiallyModifiedDenebState(),
     fn: (state: CompositeViewDU<typeof BeaconState>) => {
-      state.hashTreeRoot();
-      if (toHexString(state.node.root) !== expectedRoot) {
-        throw new Error("hashTreeRoot does not match expectedRoot");
+      // commit() step is inside hashTreeRoot(), reuse HashComputationGroup
+      if (toHexString(state.batchHashTreeRoot(hc)) !== expectedRoot) {
+        throw new Error(
+          `batchHashTreeRoot ${toHexString(state.batchHashTreeRoot(hc))} does not match expectedRoot ${expectedRoot}`
+        );
       }
+      state.batchHashTreeRoot(hc);
     },
   });
 
   itBench({
-    id: `BeaconState ViewDU recursive hash - commit step vc=${vc}`,
+    id: `BeaconState ViewDU batchHashTreeRoot - commit step vc=${vc}`,
     beforeEach: () => createPartiallyModifiedDenebState(),
     fn: (state: CompositeViewDU<typeof BeaconState>) => {
-      state.commit();
+      state.commit(0, []);
     },
   });
 
   itBench({
-    id: `BeaconState ViewDU validator tree creation vc=${numModified}`,
+    id: `BeaconState ViewDU batchHashTreeRoot - hash step vc=${vc}`,
     beforeEach: () => {
       const state = createPartiallyModifiedDenebState();
-      state.commit();
-      return state;
+      const hcByLevel: HashComputationLevel[] = [];
+      state.commit(0, hcByLevel);
+      return hcByLevel;
     },
-    fn: (state: CompositeViewDU<typeof BeaconState>) => {
-      const validators = state.validators;
-      for (let i = 0; i < numModified; i++) {
-        validators.getReadonly(i).node.left;
-      }
+    fn: (hcByLevel) => {
+      executeHashComputations(hcByLevel);
     },
   });
 
-  const hc = new HashComputationGroup();
   itBench({
-    id: `BeaconState ViewDU batchHashTreeRoot vc=${vc}`,
+    id: `BeaconState ViewDU hashTreeRoot() vc=${vc}`,
     beforeEach: () => createPartiallyModifiedDenebState(),
     fn: (state: CompositeViewDU<typeof BeaconState>) => {
-      // commit() step is inside hashTreeRoot(), reuse HashComputationGroup
-      if (toHexString(state.batchHashTreeRoot(hc)) !== expectedRoot) {
-        throw new Error("batchHashTreeRoot does not match expectedRoot");
+      state.hashTreeRoot();
+      if (toHexString(state.node.root) !== expectedRoot) {
+        throw new Error(`hashTreeRoot ${toHexString(state.node.root)} does not match expectedRoot ${expectedRoot}`);
       }
-      state.batchHashTreeRoot(hc);
     },
   });
 
   itBench({
     id: `BeaconState ViewDU hashTreeRoot - commit step vc=${vc}`,
     beforeEach: () => createPartiallyModifiedDenebState(),
     fn: (state: CompositeViewDU<typeof BeaconState>) => {
-      state.commit(0, []);
+      state.commit();
     },
   });
 
   itBench({
-    id: `BeaconState ViewDU hashTreeRoot - hash step vc=${vc}`,
+    id: `BeaconState ViewDU hashTreeRoot - validator tree creation vc=${numModified}`,
     beforeEach: () => {
       const state = createPartiallyModifiedDenebState();
-      const hcByLevel: HashComputationLevel[] = [];
-      state.commit(0, hcByLevel);
-      return hcByLevel;
+      state.commit();
+      return state;
     },
-    fn: (hcByLevel) => {
-      executeHashComputations(hcByLevel);
+    fn: (state: CompositeViewDU<typeof BeaconState>) => {
+      const validators = state.validators;
+      for (let i = 0; i < numModified; i++) {
+        validators.getReadonly(i).node.left;
+      }
     },
   });
 });