Ensure only the same validators sub tree is being mutated at once

packages/beacon-state-transition/src/allForks/block/initiateValidatorExit.ts

@@ -1,14 +1,32 @@
 import {FAR_FUTURE_EPOCH} from "@chainsafe/lodestar-params";
-import {allForks, ValidatorIndex} from "@chainsafe/lodestar-types";
+import {allForks, phase0, ValidatorIndex} from "@chainsafe/lodestar-types";
 import {CachedBeaconState} from "../util";
 
 /**
  * Initiate the exit of the validator with index ``index``.
+ *
+ * NOTE: This function takes a `validator` as argument instead of the validator index.
+ * SSZ TreeBacked have a dangerous edge case that may break the code here in a non-obvious way.
+ * When running `state.validators[i]` you get a SubTree of that validator with a hook to the state.
+ * Then, when a property of `validator` is set it propagates the changes upwards to the parent tree up to the state.
+ * This means that `validator` will propagate its new state along with the current state of its parent tree up to
+ * the state, potentially overwriting changes done in other SubTrees before.
+ * ```ts
+ * // default state.validators, all zeroes
+ * const validatorsA = state.validators
+ * const validatorsB = state.validators
+ * validatorsA[0].exitEpoch = 9
+ * validatorsB[0].exitEpoch = 9 // Setting a value in validatorsB will overwrite all changes from validatorsA
+ * // validatorsA[0].exitEpoch is 0
+ * // validatorsB[0].exitEpoch is 9
+ * ```
+ * Forcing consumers to pass the SubTree of `validator` directly mitigates this issue.
  */
-export function initiateValidatorExit(state: CachedBeaconState<allForks.BeaconState>, index: ValidatorIndex): void {
-  const {config, validators, epochCtx} = state;
-
-  const validator = validators[index];
+export function initiateValidatorExit(
+  state: CachedBeaconState<allForks.BeaconState>,
+  validator: phase0.Validator
+): void {
+  const {config, epochCtx} = state;
 
   // return if validator already initiated exit
   if (validator.exitEpoch !== FAR_FUTURE_EPOCH) {

packages/beacon-state-transition/src/allForks/block/processAttesterSlashing.ts

@@ -33,7 +33,7 @@ export function processAttesterSlashing(
   }
 
   let slashedAny = false;
-  const validators = state.validators;
+  const validators = state.validators; // Get the validators sub tree once for all indices
   // TODO: Why do we need to sort()? If it necessary add a comment with why
   for (const index of indices.sort((a, b) => a - b)) {
     if (isSlashableValidator(validators[index], state.epochCtx.currentShuffling.epoch)) {

packages/beacon-state-transition/src/allForks/block/processVoluntaryExit.ts

@@ -18,7 +18,8 @@ export function processVoluntaryExitAllForks(
   verifySignature = true
 ): void {
   assertValidVoluntaryExit(state as CachedBeaconState<allForks.BeaconState>, signedVoluntaryExit, verifySignature);
-  initiateValidatorExit(state as CachedBeaconState<allForks.BeaconState>, signedVoluntaryExit.message.validatorIndex);
+  const validator = state.validators[signedVoluntaryExit.message.validatorIndex];
+  initiateValidatorExit(state as CachedBeaconState<allForks.BeaconState>, validator);
 }
 
 export function assertValidVoluntaryExit(

packages/beacon-state-transition/src/allForks/block/slashValidator.ts

@@ -22,13 +22,12 @@ export function slashValidatorAllForks(
   blockProcess: BlockProcess,
   whistleblowerIndex?: ValidatorIndex
 ): void {
-  const {validators, epochCtx} = state;
+  const {epochCtx} = state;
   const epoch = epochCtx.currentShuffling.epoch;
+  const validator = state.validators[slashedIndex];
 
   // TODO: Merge initiateValidatorExit validators.update() with the one below
-  initiateValidatorExit(state as CachedBeaconState<allForks.BeaconState>, slashedIndex);
-
-  const validator = validators[slashedIndex];
+  initiateValidatorExit(state as CachedBeaconState<allForks.BeaconState>, validator);
 
   validator.slashed = true;
   validator.withdrawableEpoch = Math.max(validator.withdrawableEpoch, epoch + EPOCHS_PER_SLASHINGS_VECTOR);

packages/beacon-state-transition/src/allForks/epoch/processEffectiveBalanceUpdates.ts

@@ -23,12 +23,15 @@ export function processEffectiveBalanceUpdates(
   state: CachedBeaconState<allForks.BeaconState>,
   epochProcess: IEpochProcess
 ): void {
-  const {validators, epochCtx} = state;
+  const {epochCtx} = state;
   const {effectiveBalances} = epochCtx;
   const HYSTERESIS_INCREMENT = EFFECTIVE_BALANCE_INCREMENT / BigInt(HYSTERESIS_QUOTIENT);
   const DOWNWARD_THRESHOLD = HYSTERESIS_INCREMENT * BigInt(HYSTERESIS_DOWNWARD_MULTIPLIER);
   const UPWARD_THRESHOLD = HYSTERESIS_INCREMENT * BigInt(HYSTERESIS_UPWARD_MULTIPLIER);
 
+  // Get the validators sub tree once for all the loop
+  const validators = state.validators;
+
   for (let i = 0, len = epochProcess.balancesFlat.length; i < len; i++) {
     const balance = epochProcess.balancesFlat[i];
     // PERF: It's faster to access to get() every single element (4ms) than to convert to regular array then loop (9ms)

packages/beacon-state-transition/src/allForks/epoch/processRegistryUpdates.ts

@@ -21,14 +21,17 @@ export function processRegistryUpdates(
   state: CachedBeaconState<allForks.BeaconState>,
   epochProcess: IEpochProcess
 ): void {
-  const {validators, epochCtx} = state;
+  const {epochCtx} = state;
+
+  // Get the validators sub tree once for all the loop
+  const validators = state.validators;
 
   // TODO: Batch set this properties in the tree at once with setMany() or setNodes()
   // process ejections
   for (const index of epochProcess.indicesToEject) {
     // set validator exit epoch and withdrawable epoch
     // TODO: Figure out a way to quickly set properties on the validators tree
-    initiateValidatorExit(state, index);
+    initiateValidatorExit(state, validators[index]);
   }
 
   // set new activation eligibilities
@@ -40,7 +43,7 @@ export function processRegistryUpdates(
   for (const index of epochProcess.indicesToEject) {
     // set validator exit epoch and withdrawable epoch
     // TODO: Figure out a way to quickly set properties on the validators tree
-    initiateValidatorExit(state, index);
+    initiateValidatorExit(state, validators[index]);
   }
 
   const finalityEpoch = state.finalizedCheckpoint.epoch;

packages/beacon-state-transition/src/allForks/util/epochContext.ts

@@ -232,9 +232,13 @@ export function syncPubkeys(
   if (currentCount !== index2pubkey.length) {
     throw new Error(`Pubkey indices have fallen out of sync: ${currentCount} != ${index2pubkey.length}`);
   }
+
+  // Get the validators sub tree once for all the loop
+  const validators = state.validators;
+
   const newCount = state.validators.length;
   for (let i = currentCount; i < newCount; i++) {
-    const pubkey = state.validators[i].pubkey.valueOf() as Uint8Array;
+    const pubkey = validators[i].pubkey.valueOf() as Uint8Array;
     pubkey2index.set(pubkey, i);
     // Pubkeys must be checked for group + inf. This must be done only once when the validator deposit is processed.
     // Afterwards any public key is the state consider validated.

packages/beacon-state-transition/src/altair/block/processAttestation.ts

@@ -31,6 +31,9 @@ export function processAttestations(
   const stateSlot = state.slot;
   const rootCache = new RootCache(state);
 
+  // Get the validators sub tree once for all the loop
+  const validators = state.validators;
+
   // Process all attestations first and then increase the balance of the proposer once
   let proposerReward = BigInt(0);
   for (const attestation of attestations) {
@@ -91,10 +94,11 @@ export function processAttestations(
       if (totalWeight > 0) {
         // TODO: Cache effectiveBalance in a separate array
         // TODO: Consider using number instead of bigint for faster math
-        totalBalancesWithWeight += state.validators[index].effectiveBalance * totalWeight;
+        totalBalancesWithWeight += validators[index].effectiveBalance * totalWeight;
       }
     }
 
+    // Do the discrete math inside the loop to ensure a deterministic result
     const totalIncrements = totalBalancesWithWeight / EFFECTIVE_BALANCE_INCREMENT;
     const proposerRewardNumerator = totalIncrements * state.baseRewardPerIncrement;
     proposerReward += proposerRewardNumerator / PROPOSER_REWARD_DOMINATOR;

packages/beacon-state-transition/src/altair/util/syncCommittee.ts

@@ -24,6 +24,8 @@ export function getNextSyncCommitteeIndices(
 ): ValidatorIndex[] {
   const epoch = computeEpochAtSlot(state.slot) + 1;
 
+  const validators = state.validators; // Get the validators sub tree once for all the loop
+
   const activeValidatorCount = activeValidatorIndices.length;
   const seed = getSeed(state, epoch, DOMAIN_SYNC_COMMITTEE);
   let i = 0;
@@ -32,7 +34,8 @@ export function getNextSyncCommitteeIndices(
     const shuffledIndex = computeShuffledIndex(i % activeValidatorCount, activeValidatorCount, seed);
     const candidateIndex = activeValidatorIndices[shuffledIndex];
     const randomByte = hash(Buffer.concat([seed, intToBytes(intDiv(i, 32), 8, "le")]))[i % 32];
-    const effectiveBalance = state.validators[candidateIndex].effectiveBalance;
+    // TODO: Use a fast cache to get the effective balance 🐢
+    const effectiveBalance = validators[candidateIndex].effectiveBalance;
     if (effectiveBalance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * BigInt(randomByte)) {
       syncCommitteeIndices.push(candidateIndex);
     }
@@ -51,7 +54,9 @@ export function getNextSyncCommittee(
   activeValidatorIndices: ValidatorIndex[]
 ): altair.SyncCommittee {
   const indices = getNextSyncCommitteeIndices(state, activeValidatorIndices);
-  const pubkeys = indices.map((index) => state.validators[index].pubkey);
+  const validators = state.validators; // Get the validators sub tree once for all the loop
+  // TODO: Use the index2pubkey cache here! 🐢
+  const pubkeys = indices.map((index) => validators[index].pubkey);
   return {
     pubkeys,
     aggregatePubkey: aggregatePublicKeys(pubkeys.map((pubkey) => pubkey.valueOf() as Uint8Array)),

packages/beacon-state-transition/src/util/balance.ts

@@ -18,6 +18,7 @@ export function getTotalBalance(state: allForks.BeaconState, indices: ValidatorI
   return bigIntMax(
     EFFECTIVE_BALANCE_INCREMENT,
     indices.reduce(
+      // TODO: Use a fast cache to get the effective balance 🐢
       (total: Gwei, index: ValidatorIndex): Gwei => total + state.validators[index].effectiveBalance,
       BigInt(0)
     )

packages/beacon-state-transition/src/util/genesis.ts

@@ -157,6 +157,7 @@ export function applyDeposits(
   const activeValidatorIndices: ValidatorIndex[] = [];
   // Process activations
   // validators are edited, so we're not iterating (read-only) through the validators
+  // TODO: Use readonly SSZ methods for fast iteration 🐢
   const validatorLength = state.validators.length;
   for (let index = 0; index < validatorLength; index++) {
     const validator = state.validators[index];

packages/beacon-state-transition/src/util/proposer.ts

@@ -29,6 +29,7 @@ export function computeProposerIndex(
   while (true) {
     const candidateIndex = indices[computeShuffledIndex(i % indices.length, indices.length, seed)];
     const randByte = hash(Buffer.concat([seed, intToBytes(intDiv(i, 32), 8)]))[i % 32];
+    // TODO: Use a fast cache to get the effective balance 🐢
     const effectiveBalance = state.validators[candidateIndex].effectiveBalance;
     if (effectiveBalance * MAX_RANDOM_BYTE >= MAX_EFFECTIVE_BALANCE * BigInt(randByte)) {
       return candidateIndex;

packages/lodestar/src/api/impl/beacon/state/index.ts

@@ -46,13 +46,14 @@ export function getBeaconStateApi({chain, config, db}: Pick<ApiModules, "chain"
     async getStateValidators(stateId, filters) {
       const state = await resolveStateId(config, chain, db, stateId);
       const currentEpoch = getCurrentEpoch(state);
+      const validators = state.validators; // Get the validators sub tree once for all the loop
 
-      const validators: routes.beacon.ValidatorResponse[] = [];
+      const validatorResponses: routes.beacon.ValidatorResponse[] = [];
       if (filters?.indices) {
         for (const id of filters.indices) {
           const validatorIndex = getStateValidatorIndex(id, state, chain);
           if (validatorIndex != null) {
-            const validator = state.validators[validatorIndex];
+            const validator = validators[validatorIndex];
             if (filters.statuses && !filters.statuses.includes(getValidatorStatus(validator, currentEpoch))) {
               continue;
             }
@@ -62,10 +63,10 @@ export function getBeaconStateApi({chain, config, db}: Pick<ApiModules, "chain"
               state.balances[validatorIndex],
               currentEpoch
             );
-            validators.push(validatorResponse);
+            validatorResponses.push(validatorResponse);
           }
         }
-        return {data: validators};
+        return {data: validatorResponses};
       } else if (filters?.statuses) {
         const validatorsByStatus = filterStateValidatorsByStatuses(filters.statuses, state, chain, currentEpoch);
         return {data: validatorsByStatus};

packages/lodestar/src/api/impl/validator/index.ts

@@ -192,7 +192,7 @@ export function getValidatorApi({
 
       // Note: Using a MutableVector is the fastest way of getting compressed pubkeys.
       //       See benchmark -> packages/lodestar/test/perf/api/impl/validator/attester.test.ts
-      const validators = state.validators;
+      const validators = state.validators; // Get the validators sub tree once for all the loop
       const duties: routes.validator.ProposerDuty[] = [];
 
       for (let slot = startSlot; slot < startSlot + SLOTS_PER_EPOCH; slot++) {

packages/lodestar/src/api/impl/validator/utils.ts

@@ -50,7 +50,7 @@ export function getPubkeysForIndices(
   state: CachedBeaconState<allForks.BeaconState>,
   validatorIndices: ValidatorIndex[]
 ): (validatorIndex: ValidatorIndex) => BLSPubkey {
-  const validators = state.validators;
+  const validators = state.validators; // Get the validators sub tree once for all the loop
   const pubkeyMap = new Map(
     validatorIndices.map((validatorIndex) => {
       const validator = validators[validatorIndex];