Clone states only when necessary (#4279)

* Clone states only when necessary * Use validators.getReadonly when not need mutable reference
ChainSafe · Jul 18, 2022 · 7db4e4a · 7db4e4a
1 parent 8ccbcb8
commit 7db4e4a
Show file tree

Hide file tree

Showing 14 changed files with 30 additions and 33 deletions.
diff --git a/packages/beacon-node/src/api/impl/beacon/state/index.ts b/packages/beacon-node/src/api/impl/beacon/state/index.ts
@@ -57,7 +57,7 @@ export function getBeaconStateApi({
         for (const id of filters.id) {
           const validatorIndex = getStateValidatorIndex(id, state, pubkey2index);
           if (validatorIndex != null) {
-            const validator = validators.get(validatorIndex);
+            const validator = validators.getReadonly(validatorIndex);
             if (filters.statuses && !filters.statuses.includes(getValidatorStatus(validator, currentEpoch))) {
               continue;
             }
@@ -99,7 +99,7 @@ export function getBeaconStateApi({
       return {
         data: toValidatorResponse(
           validatorIndex,
-          state.validators.get(validatorIndex),
+          state.validators.getReadonly(validatorIndex),
           state.balances.get(validatorIndex),
           getCurrentEpoch(state)
         ),

diff --git a/packages/beacon-node/src/chain/blocks/importBlock.ts b/packages/beacon-node/src/chain/blocks/importBlock.ts
@@ -177,7 +177,7 @@ export async function importBlock(
   // - Write block and state to snapshot_cache
   if (block.message.slot % SLOTS_PER_EPOCH === 0) {
     // Cache state to preserve epoch transition work
-    const checkpointState = postState.clone();
+    const checkpointState = postState;
     const cp = getCheckpointFromState(checkpointState);
     chain.checkpointStateCache.add(cp, checkpointState);
     pendingEvents.push(ChainEvent.checkpoint, cp, checkpointState);

diff --git a/packages/beacon-node/src/chain/opPools/opPool.ts b/packages/beacon-node/src/chain/opPools/opPool.ts
@@ -139,7 +139,7 @@ export class OpPool {
 
     for (const proposerSlashing of this.proposerSlashings.values()) {
       const index = proposerSlashing.signedHeader1.message.proposerIndex;
-      const validator = state.validators.get(index);
+      const validator = state.validators.getReadonly(index);
       if (!validator.slashed && validator.activationEpoch <= stateEpoch && stateEpoch < validator.withdrawableEpoch) {
         proposerSlashings.push(proposerSlashing);
         // Set of validators to be slashed, so we don't attempt to construct invalid attester slashings.
@@ -156,7 +156,7 @@ export class OpPool {
       const slashableIndices = new Set<ValidatorIndex>();
       for (let i = 0; i < attesterSlashing.intersectingIndices.length; i++) {
         const index = attesterSlashing.intersectingIndices[i];
-        const validator = state.validators.get(index);
+        const validator = state.validators.getReadonly(index);
 
         // If we already have a slashing for this index, we can continue on to the next slashing
         if (toBeSlashedIndices.has(index)) {
@@ -232,7 +232,7 @@ export class OpPool {
         //
         // We cannot check the `slashed` field since the `head` is not finalized and
         // a fork could un-slash someone.
-        if (headState.validators.get(index).exitEpoch > finalizedEpoch) {
+        if (headState.validators.getReadonly(index).exitEpoch > finalizedEpoch) {
           continue attesterSlashing;
         }
       }
@@ -249,7 +249,7 @@ export class OpPool {
     const finalizedEpoch = headState.finalizedCheckpoint.epoch;
     for (const [key, proposerSlashing] of this.proposerSlashings.entries()) {
       const index = proposerSlashing.signedHeader1.message.proposerIndex;
-      if (headState.validators.get(index).exitEpoch <= finalizedEpoch) {
+      if (headState.validators.getReadonly(index).exitEpoch <= finalizedEpoch) {
         this.proposerSlashings.delete(key);
       }
     }

diff --git a/packages/beacon-node/src/chain/regen/regen.ts b/packages/beacon-node/src/chain/regen/regen.ts
@@ -242,18 +242,19 @@ async function processSlotsToNearestCheckpoint(
   const preSlot = preState.slot;
   const postSlot = slot;
   const preEpoch = computeEpochAtSlot(preSlot);
-  let postState = preState.clone();
+  let postState = preState;
   const {checkpointStateCache, emitter, metrics} = modules;
 
   for (
     let nextEpochSlot = computeStartSlotAtEpoch(preEpoch + 1);
     nextEpochSlot <= postSlot;
     nextEpochSlot += SLOTS_PER_EPOCH
   ) {
+    // processSlots calls .clone() before mutating
     postState = processSlots(postState, nextEpochSlot, {}, metrics);
 
     // Cache state to preserve epoch transition work
-    const checkpointState = postState.clone();
+    const checkpointState = postState;
     const cp = getCheckpointFromState(checkpointState);
     checkpointStateCache.add(cp, checkpointState);
     emitter.emit(ChainEvent.checkpoint, cp, checkpointState);

diff --git a/packages/beacon-node/src/chain/stateCache/stateContextCache.ts b/packages/beacon-node/src/chain/stateCache/stateContextCache.ts
@@ -41,21 +41,12 @@ export class StateContextCache {
     }
 
     this.metrics?.hits.inc();
-    // clonedCount + 1 as there's a .clone() below
-    this.metrics?.stateClonedCount.observe(item.clonedCount + 1);
-    if (!stateInternalCachePopulated(item)) {
-      this.metrics?.stateInternalCacheMiss.inc();
-    }
-
-    // Clone first to account for metrics below
-    const itemCloned = item.clone();
-
     this.metrics?.stateClonedCount.observe(item.clonedCount);
     if (!stateInternalCachePopulated(item)) {
       this.metrics?.stateInternalCacheMiss.inc();
     }
 
-    return itemCloned;
+    return item;
   }
 
   add(item: CachedBeaconStateAllForks): void {
@@ -64,7 +55,7 @@ export class StateContextCache {
       return;
     }
     this.metrics?.adds.inc();
-    this.cache.set(key, item.clone());
+    this.cache.set(key, item);
     const epoch = item.epochCtx.epoch;
     const blockRoots = this.epochIndex.get(epoch);
     if (blockRoots) {

diff --git a/packages/beacon-node/src/chain/stateCache/stateContextCheckpointsCache.ts b/packages/beacon-node/src/chain/stateCache/stateContextCheckpointsCache.ts
@@ -47,15 +47,12 @@ export class CheckpointStateCache {
       this.preComputedCheckpointHits = (this.preComputedCheckpointHits ?? 0) + 1;
     }
 
-    // Clone first to account for metrics below
-    const itemCloned = item.clone();
-
     this.metrics?.stateClonedCount.observe(item.clonedCount);
     if (!stateInternalCachePopulated(item)) {
       this.metrics?.stateInternalCacheMiss.inc();
     }
 
-    return itemCloned;
+    return item;
   }
 
   add(cp: phase0.Checkpoint, item: CachedBeaconStateAllForks): void {
@@ -65,7 +62,7 @@ export class CheckpointStateCache {
       return;
     }
     this.metrics?.adds.inc();
-    this.cache.set(key, item.clone());
+    this.cache.set(key, item);
     this.epochIndex.getOrDefault(cp.epoch).add(cpHex.rootHex);
   }
 

diff --git a/packages/state-transition/src/block/initiateValidatorExit.ts b/packages/state-transition/src/block/initiateValidatorExit.ts
@@ -1,5 +1,6 @@
 import {FAR_FUTURE_EPOCH} from "@lodestar/params";
-import {phase0} from "@lodestar/types";
+import {CompositeViewDU} from "@chainsafe/ssz";
+import {ssz} from "@lodestar/types";
 import {CachedBeaconStateAllForks} from "../types.js";
 
 /**
@@ -22,7 +23,10 @@ import {CachedBeaconStateAllForks} from "../types.js";
  * ```
  * Forcing consumers to pass the SubTree of `validator` directly mitigates this issue.
  */
-export function initiateValidatorExit(state: CachedBeaconStateAllForks, validator: phase0.Validator): void {
+export function initiateValidatorExit(
+  state: CachedBeaconStateAllForks,
+  validator: CompositeViewDU<typeof ssz.phase0.Validator>
+): void {
   const {config, epochCtx} = state;
 
   // return if validator already initiated exit

diff --git a/packages/state-transition/src/block/processAttesterSlashing.ts b/packages/state-transition/src/block/processAttesterSlashing.ts
@@ -26,7 +26,7 @@ export function processAttesterSlashing(
   const validators = state.validators; // Get the validators sub tree once for all indices
   // Spec requires to sort indexes beforehand
   for (const index of intersectingIndices.sort((a, b) => a - b)) {
-    if (isSlashableValidator(validators.get(index), state.epochCtx.epoch)) {
+    if (isSlashableValidator(validators.getReadonly(index), state.epochCtx.epoch)) {
       slashValidator(fork, state, index);
       slashedAny = true;
     }

diff --git a/packages/state-transition/src/block/processBlockHeader.ts b/packages/state-transition/src/block/processBlockHeader.ts
@@ -52,7 +52,7 @@ export function processBlockHeader(state: CachedBeaconStateAllForks, block: allF
   });
 
   // verify proposer is not slashed. Only once per block, may use the slower read from tree
-  if (state.validators.get(proposerIndex).slashed) {
+  if (state.validators.getReadonly(proposerIndex).slashed) {
     throw new Error("Block proposer is slashed");
   }
 }
diff --git a/packages/state-transition/src/block/processProposerSlashing.ts b/packages/state-transition/src/block/processProposerSlashing.ts
@@ -49,7 +49,7 @@ export function assertValidProposerSlashing(
   }
 
   // verify the proposer is slashable
-  const proposer = state.validators.get(header1.proposerIndex);
+  const proposer = state.validators.getReadonly(header1.proposerIndex);
   if (!isSlashableValidator(proposer, state.epochCtx.epoch)) {
     throw new Error("ProposerSlashing proposer is not slashable");
   }

diff --git a/packages/state-transition/src/epoch/processSyncCommitteeUpdates.ts b/packages/state-transition/src/epoch/processSyncCommitteeUpdates.ts
@@ -24,7 +24,9 @@ export function processSyncCommitteeUpdates(state: CachedBeaconStateAltair): voi
     );
 
     // Using the index2pubkey cache is slower because it needs the serialized pubkey.
-    const nextSyncCommitteePubkeys = nextSyncCommitteeIndices.map((index) => state.validators.get(index).pubkey);
+    const nextSyncCommitteePubkeys = nextSyncCommitteeIndices.map(
+      (index) => state.validators.getReadonly(index).pubkey
+    );
 
     // Rotate syncCommittee in state
     state.currentSyncCommittee = state.nextSyncCommittee;

diff --git a/packages/state-transition/src/stateTransition.ts b/packages/state-transition/src/stateTransition.ts
@@ -33,6 +33,7 @@ export function stateTransition(
   const block = signedBlock.message;
   const blockSlot = block.slot;
 
+  // .clone() before mutating state in state transition
   let postState = state.clone();
 
   // State is already a ViewDU, which won't commit changes. Equivalent to .setStateCachesAsTransient()
@@ -88,6 +89,7 @@ export function processSlots(
   epochProcessOpts?: EpochProcessOpts,
   metrics?: IBeaconStateTransitionMetrics | null
 ): CachedBeaconStateAllForks {
+  // .clone() before mutating state in state transition
   let postState = state.clone();
 
   // State is already a ViewDU, which won't commit changes. Equivalent to .setStateCachesAsTransient()

diff --git a/packages/state-transition/src/util/syncCommittee.ts b/packages/state-transition/src/util/syncCommittee.ts
@@ -26,7 +26,7 @@ export function getNextSyncCommittee(
   const indices = getNextSyncCommitteeIndices(state, activeValidatorIndices, effectiveBalanceIncrements);
 
   // Using the index2pubkey cache is slower because it needs the serialized pubkey.
-  const pubkeys = indices.map((index) => state.validators.get(index).pubkey);
+  const pubkeys = indices.map((index) => state.validators.getReadonly(index).pubkey);
 
   return {
     indices,

diff --git a/packages/state-transition/test/perf/analyzeEpochs.ts b/packages/state-transition/test/perf/analyzeEpochs.ts
@@ -104,7 +104,7 @@ async function analyzeEpochs(network: NetworkName, fromEpoch?: number): Promise<
     const validatorKeys = Object.keys(validatorChangesCountZero) as (keyof typeof validatorChangesCountZero)[];
     for (let i = 0; i < validatorCount; i++) {
       const validatorPrev = state.validators[i];
-      const validatorNext = postState.validators.get(i);
+      const validatorNext = postState.validators.getReadonly(i);
       for (const key of validatorKeys) {
         const valuePrev = validatorPrev[key];
         const valueNext = validatorNext[key];