fix: light client generating LightClientUpdate with wrong length of branches (#7187)

* initial commit

* Rewrite SyncCommitteeWitnessRepository

* Fix finality branch

* Update unit test

* fix e2e

* Review PR

---------

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

Author: ensi321

packages/beacon-node/src/chain/lightClient/index.ts

@@ -1,61 +1,64 @@
 import {BitArray, CompositeViewDU} from "@chainsafe/ssz";
+import {routes} from "@lodestar/api";
+import {ChainForkConfig} from "@lodestar/config";
+import {
+  LightClientUpdateSummary,
+  isBetterUpdate,
+  toLightClientUpdateSummary,
+  upgradeLightClientHeader,
+} from "@lodestar/light-client/spec";
+import {
+  ForkExecution,
+  ForkLightClient,
+  ForkName,
+  ForkSeq,
+  MIN_SYNC_COMMITTEE_PARTICIPANTS,
+  SYNC_COMMITTEE_SIZE,
+  forkLightClient,
+  highestFork,
+  isForkPostElectra,
+} from "@lodestar/params";
+import {
+  CachedBeaconStateAltair,
+  computeStartSlotAtEpoch,
+  computeSyncPeriodAtEpoch,
+  computeSyncPeriodAtSlot,
+  executionPayloadToPayloadHeader,
+} from "@lodestar/state-transition";
 import {
-  altair,
   BeaconBlock,
   BeaconBlockBody,
   LightClientBootstrap,
   LightClientFinalityUpdate,
   LightClientHeader,
   LightClientOptimisticUpdate,
   LightClientUpdate,
-  phase0,
   Root,
   RootHex,
+  SSZTypesFor,
   Slot,
+  SyncPeriod,
+  altair,
+  electra,
+  phase0,
   ssz,
   sszTypesFor,
-  SSZTypesFor,
-  SyncPeriod,
 } from "@lodestar/types";
-import {ChainForkConfig} from "@lodestar/config";
-import {
-  CachedBeaconStateAltair,
-  computeStartSlotAtEpoch,
-  computeSyncPeriodAtEpoch,
-  computeSyncPeriodAtSlot,
-  executionPayloadToPayloadHeader,
-} from "@lodestar/state-transition";
-import {
-  isBetterUpdate,
-  toLightClientUpdateSummary,
-  LightClientUpdateSummary,
-  upgradeLightClientHeader,
-} from "@lodestar/light-client/spec";
 import {Logger, MapDef, pruneSetToMax, toRootHex} from "@lodestar/utils";
-import {routes} from "@lodestar/api";
-import {
-  MIN_SYNC_COMMITTEE_PARTICIPANTS,
-  SYNC_COMMITTEE_SIZE,
-  ForkName,
-  ForkSeq,
-  ForkExecution,
-  ForkLightClient,
-  highestFork,
-  forkLightClient,
-} from "@lodestar/params";
 
+import {ZERO_HASH} from "../../constants/index.js";
 import {IBeaconDb} from "../../db/index.js";
+import {NUM_WITNESS, NUM_WITNESS_ELECTRA} from "../../db/repositories/lightclientSyncCommitteeWitness.js";
 import {Metrics} from "../../metrics/index.js";
-import {ChainEventEmitter} from "../emitter.js";
 import {byteArrayEquals} from "../../util/bytes.js";
-import {ZERO_HASH} from "../../constants/index.js";
+import {ChainEventEmitter} from "../emitter.js";
 import {LightClientServerError, LightClientServerErrorCode} from "../errors/lightClientError.js";
 import {
+  getBlockBodyExecutionHeaderProof,
+  getCurrentSyncCommitteeBranch,
+  getFinalizedRootProof,
   getNextSyncCommitteeBranch,
   getSyncCommitteesWitness,
-  getFinalizedRootProof,
-  getCurrentSyncCommitteeBranch,
-  getBlockBodyExecutionHeaderProof,
 } from "./proofs.js";
 
 export type LightClientServerOpts = {
@@ -208,7 +211,10 @@ export class LightClientServer {
   private checkpointHeaders = new Map<BlockRooHex, LightClientHeader>();
   private latestHeadUpdate: LightClientOptimisticUpdate | null = null;
 
-  private readonly zero: Pick<altair.LightClientUpdate, "finalityBranch" | "finalizedHeader">;
+  private readonly zero: Pick<
+    altair.LightClientUpdate | electra.LightClientUpdate,
+    "finalityBranch" | "finalizedHeader"
+  >;
   private finalized: LightClientFinalityUpdate | null = null;
 
   constructor(
@@ -225,7 +231,9 @@ export class LightClientServer {
     this.zero = {
       // Assign the hightest fork's default value because it can always be typecasted down to correct fork
       finalizedHeader: sszTypesFor(highestFork(forkLightClient)).LightClientHeader.defaultValue(),
-      finalityBranch: ssz.altair.LightClientUpdate.fields.finalityBranch.defaultValue(),
+      // Electra finalityBranch has fixed length of 5 whereas altair has 4. The fifth element will be ignored
+      // when serializing as altair LightClientUpdate
+      finalityBranch: ssz.electra.LightClientUpdate.fields.finalityBranch.defaultValue(),
     };
 
     if (metrics) {
@@ -388,12 +396,13 @@ export class LightClientServer {
     parentBlockSlot: Slot
   ): Promise<void> {
     const blockSlot = block.slot;
-    const header = blockToLightClientHeader(this.config.getForkName(blockSlot), block);
+    const fork = this.config.getForkName(blockSlot);
+    const header = blockToLightClientHeader(fork, block);
 
     const blockRoot = ssz.phase0.BeaconBlockHeader.hashTreeRoot(header.beacon);
     const blockRootHex = toRootHex(blockRoot);
 
-    const syncCommitteeWitness = getSyncCommitteesWitness(postState);
+    const syncCommitteeWitness = getSyncCommitteesWitness(fork, postState);
 
     // Only store current sync committee once per run
     if (!this.storedCurrentSyncCommittee) {
@@ -621,6 +630,16 @@ export class LightClientServer {
     if (!syncCommitteeWitness) {
       throw Error(`syncCommitteeWitness not available at ${toRootHex(attestedData.blockRoot)}`);
     }
+
+    const attestedFork = this.config.getForkName(attestedHeader.beacon.slot);
+    const numWitness = syncCommitteeWitness.witness.length;
+    if (isForkPostElectra(attestedFork) && numWitness !== NUM_WITNESS_ELECTRA) {
+      throw Error(`Expected ${NUM_WITNESS_ELECTRA} witnesses in post-Electra numWitness=${numWitness}`);
+    }
+    if (!isForkPostElectra(attestedFork) && numWitness !== NUM_WITNESS) {
+      throw Error(`Expected ${NUM_WITNESS} witnesses in pre-Electra numWitness=${numWitness}`);
+    }
+
     const nextSyncCommittee = await this.db.syncCommittee.get(syncCommitteeWitness.nextSyncCommitteeRoot);
     if (!nextSyncCommittee) {
       throw Error("nextSyncCommittee not available");
@@ -641,7 +660,6 @@ export class LightClientServer {
       finalityBranch = attestedData.finalityBranch;
       finalizedHeader = finalizedHeaderAttested;
       // Fork of LightClientUpdate is based off on attested header's fork
-      const attestedFork = this.config.getForkName(attestedHeader.beacon.slot);
       if (this.config.getForkName(finalizedHeader.beacon.slot) !== attestedFork) {
         finalizedHeader = upgradeLightClientHeader(this.config, attestedFork, finalizedHeader);
       }

packages/beacon-node/src/chain/lightClient/proofs.ts

@@ -1,32 +1,58 @@
 import {Tree} from "@chainsafe/persistent-merkle-tree";
-import {BeaconStateAllForks, CachedBeaconStateAllForks} from "@lodestar/state-transition";
 import {
-  FINALIZED_ROOT_GINDEX,
   BLOCK_BODY_EXECUTION_PAYLOAD_GINDEX,
-  ForkExecution,
+  FINALIZED_ROOT_GINDEX,
   FINALIZED_ROOT_GINDEX_ELECTRA,
+  ForkExecution,
+  ForkName,
+  isForkPostElectra,
 } from "@lodestar/params";
+import {BeaconStateAllForks, CachedBeaconStateAllForks} from "@lodestar/state-transition";
 import {BeaconBlockBody, SSZTypesFor, ssz} from "@lodestar/types";
 
 import {SyncCommitteeWitness} from "./types.js";
 
-export function getSyncCommitteesWitness(state: BeaconStateAllForks): SyncCommitteeWitness {
+export function getSyncCommitteesWitness(fork: ForkName, state: BeaconStateAllForks): SyncCommitteeWitness {
   state.commit();
   const n1 = state.node;
-  const n3 = n1.right; // [1]0110
-  const n6 = n3.left; // 1[0]110
-  const n13 = n6.right; // 10[1]10
-  const n27 = n13.right; // 101[1]0
-  const currentSyncCommitteeRoot = n27.left.root; // n54 1011[0]
-  const nextSyncCommitteeRoot = n27.right.root; // n55 1011[1]
+  let witness: Uint8Array[];
+  let currentSyncCommitteeRoot: Uint8Array;
+  let nextSyncCommitteeRoot: Uint8Array;
 
-  // Witness branch is sorted by descending gindex
-  const witness = [
-    n13.left.root, // 26
-    n6.left.root, // 12
-    n3.right.root, // 7
-    n1.left.root, // 2
-  ];
+  if (isForkPostElectra(fork)) {
+    const n2 = n1.left;
+    const n5 = n2.right;
+    const n10 = n5.left;
+    const n21 = n10.right;
+    const n43 = n21.right;
+
+    currentSyncCommitteeRoot = n43.left.root; // n86
+    nextSyncCommitteeRoot = n43.right.root; // n87
+
+    // Witness branch is sorted by descending gindex
+    witness = [
+      n21.left.root, // 42
+      n10.left.root, // 20
+      n5.right.root, // 11
+      n2.left.root, // 4
+      n1.right.root, // 3
+    ];
+  } else {
+    const n3 = n1.right; // [1]0110
+    const n6 = n3.left; // 1[0]110
+    const n13 = n6.right; // 10[1]10
+    const n27 = n13.right; // 101[1]0
+    currentSyncCommitteeRoot = n27.left.root; // n54 1011[0]
+    nextSyncCommitteeRoot = n27.right.root; // n55 1011[1]
+
+    // Witness branch is sorted by descending gindex
+    witness = [
+      n13.left.root, // 26
+      n6.left.root, // 12
+      n3.right.root, // 7
+      n1.left.root, // 2
+    ];
+  }
 
   return {
     witness,

packages/beacon-node/src/chain/lightClient/types.ts

@@ -26,7 +26,7 @@
  * ```
  */
 export type SyncCommitteeWitness = {
-  /** Vector[Bytes32, 4] */
+  /** Vector[Bytes32, 4] or Vector[Bytes32, 5] depending on the fork */
   witness: Uint8Array[];
   currentSyncCommitteeRoot: Uint8Array;
   nextSyncCommitteeRoot: Uint8Array;

packages/beacon-node/src/db/repositories/lightclientSyncCommitteeWitness.ts

@@ -5,6 +5,15 @@ import {ssz} from "@lodestar/types";
 import {SyncCommitteeWitness} from "../../chain/lightClient/types.js";
 import {Bucket, getBucketNameByValue} from "../buckets.js";
 
+// We add a 1-byte prefix where 0 means pre-electra and 1 means post-electra
+enum PrefixByte {
+  PRE_ELECTRA = 0,
+  POST_ELECTRA = 1,
+}
+
+export const NUM_WITNESS = 4;
+export const NUM_WITNESS_ELECTRA = 5;
+
 /**
  * Historical sync committees witness by block root
  *
@@ -13,12 +22,56 @@ import {Bucket, getBucketNameByValue} from "../buckets.js";
 export class SyncCommitteeWitnessRepository extends Repository<Uint8Array, SyncCommitteeWitness> {
   constructor(config: ChainForkConfig, db: DatabaseController<Uint8Array, Uint8Array>) {
     const bucket = Bucket.lightClient_syncCommitteeWitness;
+    // Pick some type but won't be used. Witness can be 4 or 5 so need to handle dynamically
     const type = new ContainerType({
-      witness: new VectorCompositeType(ssz.Root, 4),
+      witness: new VectorCompositeType(ssz.Root, NUM_WITNESS),
       currentSyncCommitteeRoot: ssz.Root,
       nextSyncCommitteeRoot: ssz.Root,
     });
 
     super(config, db, bucket, type, getBucketNameByValue(bucket));
   }
+
+  // Overrides for multi-fork
+  encodeValue(value: SyncCommitteeWitness): Uint8Array {
+    const numWitness = value.witness.length;
+
+    if (numWitness !== NUM_WITNESS && numWitness !== NUM_WITNESS_ELECTRA) {
+      throw Error(`Number of witness can only be 4 pre-electra or 5 post-electra numWitness=${numWitness}`);
+    }
+
+    const type = new ContainerType({
+      witness: new VectorCompositeType(ssz.Root, numWitness),
+      currentSyncCommitteeRoot: ssz.Root,
+      nextSyncCommitteeRoot: ssz.Root,
+    });
+
+    const valueBytes = type.serialize(value);
+
+    // We need to differentiate between post-electra and pre-electra witness
+    // such that we can deserialize correctly
+    const isPostElectra = numWitness === NUM_WITNESS_ELECTRA;
+    const prefixByte = new Uint8Array(1);
+    prefixByte[0] = isPostElectra ? PrefixByte.POST_ELECTRA : PrefixByte.PRE_ELECTRA;
+
+    const prefixedData = new Uint8Array(1 + valueBytes.length);
+    prefixedData.set(prefixByte, 0);
+    prefixedData.set(valueBytes, 1);
+
+    return prefixedData;
+  }
+
+  decodeValue(data: Uint8Array): SyncCommitteeWitness {
+    // First byte is written
+    const prefix = data.subarray(0, 1);
+    const isPostElectra = prefix[0] === PrefixByte.POST_ELECTRA;
+
+    const type = new ContainerType({
+      witness: new VectorCompositeType(ssz.Root, isPostElectra ? NUM_WITNESS_ELECTRA : NUM_WITNESS),
+      currentSyncCommitteeRoot: ssz.Root,
+      nextSyncCommitteeRoot: ssz.Root,
+    });
+
+    return type.deserialize(data.subarray(1));
+  }
 }