Fix LightClientUpdate headers (#3656)

* Fix LightClientUpdate headers

* Update light-client client accordingly

* Fix lightclient unit test

packages/light-client/src/index.ts

@@ -15,7 +15,7 @@ import {isValidMerkleBranch} from "./utils/verifyMerkleBranch";
 import {SyncCommitteeFast} from "./types";
 import {chunkifyInclusiveRange} from "./utils/chunkify";
 import {LightclientEmitter, LightclientEvent} from "./events";
-import {assertValidSignedHeader, assertValidLightClientUpdate} from "./validation";
+import {assertValidSignedHeader, assertValidLightClientUpdate, activeHeader} from "./validation";
 import {GenesisData} from "./networks";
 import {getLcLoggerConsole, ILcLogger} from "./utils/logger";
 import {computeSyncPeriodAtEpoch, computeSyncPeriodAtSlot, computeEpochAtSlot} from "./utils/clock";
@@ -438,8 +438,7 @@ export class Lightclient {
    */
   private processSyncCommitteeUpdate(update: altair.LightClientUpdate): void {
     // Prevent registering updates for slots too far in the future
-    const isFinalized = !isEmptyHeader(update.finalizedHeader);
-    const updateSlot = isFinalized ? update.finalizedHeader.slot : update.attestedHeader.slot;
+    const updateSlot = activeHeader(update).slot;
     if (updateSlot > slotWithFutureTolerance(this.config, this.genesisTime, MAX_CLOCK_DISPARITY_SEC)) {
       throw Error(`updateSlot ${updateSlot} is too far in the future, currentSlot ${this.currentSlot}`);
     }
@@ -464,7 +463,7 @@ export class Lightclient {
     const nextPeriod = updatePeriod + 1;
     const existingNextSyncCommittee = this.syncCommitteeByPeriod.get(nextPeriod);
     const newNextSyncCommitteeStats: LightclientUpdateStats = {
-      isFinalized,
+      isFinalized: !isEmptyHeader(update.finalizedHeader),
       participation: sumBits(update.syncCommitteeAggregate.syncCommitteeBits),
       slot: updateSlot,
     };

packages/light-client/src/validation.ts

@@ -1,4 +1,4 @@
-import {altair, Root, Slot, ssz} from "@chainsafe/lodestar-types";
+import {altair, phase0, Root, Slot, ssz} from "@chainsafe/lodestar-types";
 import {PublicKey, Signature} from "@chainsafe/bls";
 import {
   FINALIZED_ROOT_INDEX,
@@ -34,7 +34,6 @@ export function assertValidLightClientUpdate(
 
   // Verify update header root is the finalized root of the finality header, if specified
   const isFinalized = !isEmptyHeader(update.finalizedHeader);
-  const signedHeader = isFinalized ? update.finalizedHeader : update.attestedHeader;
   if (isFinalized) {
     assertValidFinalityProof(update);
   } else {
@@ -46,8 +45,9 @@ export function assertValidLightClientUpdate(
   // An update may not increase the period but still be stored in validUpdates and be used latter
   assertValidSyncCommitteeProof(update);
 
-  const headerBlockRoot = ssz.phase0.BeaconBlockHeader.hashTreeRoot(signedHeader);
-  assertValidSignedHeader(config, syncCommittee, update.syncCommitteeAggregate, headerBlockRoot, signedHeader.slot);
+  const {attestedHeader} = update;
+  const headerBlockRoot = ssz.phase0.BeaconBlockHeader.hashTreeRoot(attestedHeader);
+  assertValidSignedHeader(config, syncCommittee, update.syncCommitteeAggregate, headerBlockRoot, attestedHeader.slot);
 }
 
 /**
@@ -65,11 +65,11 @@ export function assertValidLightClientUpdate(
 export function assertValidFinalityProof(update: altair.LightClientUpdate): void {
   if (
     !isValidMerkleBranch(
-      ssz.phase0.BeaconBlockHeader.hashTreeRoot(update.attestedHeader),
+      ssz.phase0.BeaconBlockHeader.hashTreeRoot(update.finalizedHeader),
       Array.from(update.finalityBranch).map((i) => i.valueOf() as Uint8Array),
       FINALIZED_ROOT_DEPTH,
       FINALIZED_ROOT_INDEX,
-      update.finalizedHeader.stateRoot.valueOf() as Uint8Array
+      update.attestedHeader.stateRoot.valueOf() as Uint8Array
     )
   ) {
     throw Error("Invalid finality header merkle branch");
@@ -99,13 +99,27 @@ export function assertValidSyncCommitteeProof(update: altair.LightClientUpdate):
       Array.from(update.nextSyncCommitteeBranch).map((i) => i.valueOf() as Uint8Array),
       NEXT_SYNC_COMMITTEE_DEPTH,
       NEXT_SYNC_COMMITTEE_INDEX,
-      update.attestedHeader.stateRoot.valueOf() as Uint8Array
+      activeHeader(update).stateRoot.valueOf() as Uint8Array
     )
   ) {
     throw Error("Invalid next sync committee merkle branch");
   }
 }
 
+/**
+ * The "active header" is the header that the update is trying to convince us
+ * to accept. If a finalized header is present, it's the finalized header,
+ * otherwise it's the attested header
+ * @param update
+ */
+export function activeHeader(update: altair.LightClientUpdate): phase0.BeaconBlockHeader {
+  if (!isEmptyHeader(update.finalizedHeader)) {
+    return update.finalizedHeader;
+  }
+
+  return update.attestedHeader;
+}
+
 /**
  * Assert valid signature for `signedHeader` with provided `syncCommittee`.
  *

packages/light-client/test/prepareUpdateNaive.ts

@@ -102,10 +102,10 @@ export async function prepareUpdateNaive(
   const nextSyncCommitteeBranch = finalizedCheckpointState.tree.getSingleProof(BigInt(NEXT_SYNC_COMMITTEE_GINDEX));
 
   return {
-    attestedHeader: finalizedCheckpointBlockHeader,
+    attestedHeader: syncAttestedBlockHeader,
     nextSyncCommittee: finalizedCheckpointState.nextSyncCommittee,
     nextSyncCommitteeBranch: nextSyncCommitteeBranch,
-    finalizedHeader: syncAttestedBlockHeader,
+    finalizedHeader: finalizedCheckpointBlockHeader,
     finalityBranch: finalityBranch,
     syncCommitteeAggregate: syncAggregate,
     forkVersion: syncAttestedForkVersion,

packages/light-client/test/unit/validation.test.ts

@@ -50,14 +50,14 @@ describe("validateLightClientUpdate", () => {
     const nextSyncCommitteeBranch = finalizedCheckpointState.tree.getSingleProof(BigInt(NEXT_SYNC_COMMITTEE_GINDEX));
 
     // update.header must have stateRoot to finalizedCheckpointState
-    const header = defaultBeaconBlockHeader(updateHeaderSlot);
-    header.stateRoot = ssz.altair.BeaconState.hashTreeRoot(finalizedCheckpointState);
+    const finalizedHeader = defaultBeaconBlockHeader(updateHeaderSlot);
+    finalizedHeader.stateRoot = ssz.altair.BeaconState.hashTreeRoot(finalizedCheckpointState);
 
     // syncAttestedState must have `header` as finalizedCheckpoint
     const syncAttestedState = ssz.altair.BeaconState.defaultTreeBacked();
     syncAttestedState.finalizedCheckpoint = {
       epoch: 0,
-      root: ssz.phase0.BeaconBlockHeader.hashTreeRoot(header),
+      root: ssz.phase0.BeaconBlockHeader.hashTreeRoot(finalizedHeader),
     };
     // Prove it
     const finalityBranch = syncAttestedState.tree.getSingleProof(BigInt(FINALIZED_ROOT_GINDEX));
@@ -76,10 +76,10 @@ describe("validateLightClientUpdate", () => {
     };
 
     update = {
-      attestedHeader: header,
+      attestedHeader: syncAttestedBlockHeader,
       nextSyncCommittee: nextSyncCommittee,
       nextSyncCommitteeBranch: nextSyncCommitteeBranch,
-      finalizedHeader: syncAttestedBlockHeader,
+      finalizedHeader: finalizedHeader,
       finalityBranch: finalityBranch,
       syncCommitteeAggregate: syncAggregate,
       forkVersion,

packages/light-client/test/utils.ts

@@ -126,36 +126,38 @@ export function computeLightclientUpdate(config: IBeaconConfig, period: SyncPeri
     NEXT_SYNC_COMMITTEE_INDEX
   );
 
-  const header: phase0.BeaconBlockHeader = {
+  // finalized header's state root is used to to validate sync committee branch
+  const finalizedHeader: phase0.BeaconBlockHeader = {
     slot: updateSlot,
     proposerIndex: 0,
     parentRoot: SOME_HASH,
     stateRoot: headerStateRoot,
     bodyRoot: SOME_HASH,
   };
 
-  const {root: finalityHeaderStateRoot, proof: finalityBranch} = computeMerkleBranch(
-    ssz.phase0.BeaconBlockHeader.hashTreeRoot(header),
+  const {root: stateRoot, proof: finalityBranch} = computeMerkleBranch(
+    ssz.phase0.BeaconBlockHeader.hashTreeRoot(finalizedHeader),
     FINALIZED_ROOT_DEPTH,
     FINALIZED_ROOT_INDEX
   );
 
-  const finalityHeader: phase0.BeaconBlockHeader = {
+  // attested header's state root is used to validate finality branch
+  const attestedHeader: phase0.BeaconBlockHeader = {
     slot: updateSlot,
     proposerIndex: 0,
     parentRoot: SOME_HASH,
-    stateRoot: finalityHeaderStateRoot,
+    stateRoot: stateRoot,
     bodyRoot: SOME_HASH,
   };
 
   const forkVersion = config.getForkVersion(updateSlot);
-  const syncAggregate = committee.signHeader(config, finalityHeader);
+  const syncAggregate = committee.signHeader(config, attestedHeader);
 
   return {
-    attestedHeader: header,
+    attestedHeader,
     nextSyncCommittee,
     nextSyncCommitteeBranch,
-    finalizedHeader: finalityHeader,
+    finalizedHeader,
     finalityBranch,
     syncCommitteeAggregate: syncAggregate,
     forkVersion,
@@ -254,7 +256,7 @@ export function committeeUpdateToHeadUpdate(
   committeeUpdate: altair.LightClientUpdate
 ): routes.lightclient.LightclientHeaderUpdate {
   return {
-    attestedHeader: committeeUpdate.finalizedHeader,
+    attestedHeader: committeeUpdate.attestedHeader,
     syncAggregate: {
       syncCommitteeBits: committeeUpdate.syncCommitteeAggregate.syncCommitteeBits,
       syncCommitteeSignature: committeeUpdate.syncCommitteeAggregate.syncCommitteeSignature,

packages/lodestar/src/chain/lightClient/index.ts

@@ -277,10 +277,10 @@ export class LightClientServer {
 
     if (partialUpdate.isFinalized) {
       return {
-        attestedHeader: partialUpdate.finalizedHeader,
+        attestedHeader: partialUpdate.attestedHeader,
         nextSyncCommittee: nextSyncCommittee,
         nextSyncCommitteeBranch: getNextSyncCommitteeBranch(syncCommitteeWitness),
-        finalizedHeader: partialUpdate.attestedHeader,
+        finalizedHeader: partialUpdate.finalizedHeader,
         finalityBranch: partialUpdate.finalityBranch,
         syncCommitteeAggregate: partialUpdate.syncCommitteeAggregate,
         forkVersion: this.config.getForkVersion(partialUpdate.attestedHeader.slot),