Fork version used to verify sync aggregate in LC update

[nflaig]

From reviewing the spec, I noticed that is states that sync_aggregate fork might be different and is based on signature_slot

specs/altair/light-client/p2p-interface.md?plain=1#L245

A ForkDigest-context based on compute_fork_version(compute_epoch_at_slot(optimistic_update.attested_header.beacon.slot)) is used to select the fork namespace of the Response type. Note that this fork_version may be different from the one used to verify the optimistic_update.sync_aggregate, which is based on optimistic_update.signature_slot.

I don't think we are following the spec here, but based on our current design of spec types it is difficult to use a different fork type for nested properties.

This seems to be mostly relevant during a hard fork and only if the SyncAggregate changes between those forks.

@wemeetagain @g11tech any thoughts?

Originally posted by @nflaig in https://github.com/ChainSafe/lodestar/issues/6309#issuecomment-1899215202

[g11tech]

I have had discussions on this in past if I remember, yes the fork version depends on attested header, but it doesnt matter if you keep or transmit an updated version of it

let me know some concrete example thats bugging you because that can help in identifying what should be the treatment

[nflaig]

let me know some concrete example thats bugging you because that can help in identifying what should be the treatment

As I understand it, the signature_slot is the slot of the block while attested_header.beacon.slot is the slot of the previous block. This means if there is a fork transition, we might have to different fork types to serialize / deserialize the data.

e.g use capella.LightClientOptimisticUpdate for the update itself but the sync_aggregate should use deneb.SyncAggregate

Our current ssz types do not support this as far as I can see as we don't allow to pick a fork type for individual properties, only the whole container.

As is it right now, it does not seem to be relevant because sync_aggregate didn't change since altair https://github.com/ChainSafe/lodestar/blob/01d47b9a0901c9f03a01ef68c819f4e592cbfd8b/packages/types/src/capella/sszTypes.ts#L241

But it is something we should keep in mind in case it changes in some future fork

[g11tech]

which particular method you are talking about? req/resp? api? gossip?

[g11tech]

.g use capella.LightClientOptimisticUpdate for the update itself but the sync_aggregate should use deneb.SyncAggregate

the update is "upgraded" to signature slot's , so the entire update is/SHOULD be one one particular fork, if this is not happening then its a bug to be fixed

[nflaig]

which particular method you are talking about? req/resp? api? gossip?

any place where we serialize / derserialize / validate light client updates

e.g. eventstream api https://github.com/ChainSafe/lodestar/blob/347c95fb0da219b5a4d551b7ce014f2ceda7ae48/packages/api/src/beacon/routes/events.ts#L209-L210

[nflaig]

the update is "upgraded" to signature slot's , so the entire update is/SHOULD be one one particular fork, if this is not happening then its a bug to be fixed

It's not what we do in the code but what the spec states in several places, so we might wanna bring this up there if it is incorrect

[g11tech]

whwre don't we do that, can you point out?

[g11tech]

which particular method you are talking about? req/resp? api? gossip?

any place where we serialize / derserialize / validate light client updates

e.g. eventstream api

https://github.com/ChainSafe/lodestar/blob/347c95fb0da219b5a4d551b7ce014f2ceda7ae48/packages/api/src/beacon/routes/events.ts#L209-L210

yes so it serializes deserializes with the version with whihc its being emitted, can you check if the data is not being matched/upgraded at the time for the emit for the version its being emitted at?

[nflaig]

can you check if the data is not being matched/upgraded at the time for the emit for the version its being emitted at?

We use the attested header's fork to set the version https://github.com/ChainSafe/lodestar/blob/347c95fb0da219b5a4d551b7ce014f2ceda7ae48/packages/beacon-node/src/chain/lightClient/index.ts#L497-L498

This is correct as per spec but the spec also states that the sync_aggregate property should not use the attested header's fork but rather the fork of the signature_slot while we also use the attested header's fork for that.