feat: implement is_better_update for light client
#14186
Conversation
| // createLightClientBootstrap - implements https://github.com/ethereum/consensus-specs/blob/3d235740e5f1e641d3b160c8688f26e7dc5a1894/specs/altair/light-client/full-node.md#create_light_client_bootstrap | ||
| // def create_light_client_bootstrap(state: BeaconState) -> LightClientBootstrap: |
There was a problem hiding this comment.
These specs are needed for all the four functions.
| newHasRelevantSyncCommittee := isSyncCommitteeUpdate(newUpdate) && (slots.ToEpoch(newUpdate.AttestedHeader.Slot) == slots.ToEpoch(newUpdate.SignatureSlot)) | ||
| oldHasRelevantSyncCommittee := isSyncCommitteeUpdate(oldUpdate) && (slots.ToEpoch(oldUpdate.AttestedHeader.Slot) == slots.ToEpoch(oldUpdate.SignatureSlot)) |
There was a problem hiding this comment.
slots.ToEpoch probably needs to be implemented in a different function compute_sync_committee_period_at_slot as per specs
There was a problem hiding this comment.
From what I saw the compute_sync_committee_period_at_slot is used in some other places of the codebase too and instead of implementing the function, slots.ToEpoch() has been used
There was a problem hiding this comment.
Comparing epochs like this won't work because the two compared epochs don't have to be equal. It is enough that they are from the same sync committee period. You should use slots.SyncCommitteePeriod().
| newHasSyncCommitteeFinality := slots.ToEpoch(newUpdate.FinalizedHeader.Slot) == slots.ToEpoch(newUpdate.AttestedHeader.Slot) | ||
| oldHasSyncCommitteeFinality := slots.ToEpoch(oldUpdate.FinalizedHeader.Slot) == slots.ToEpoch(oldUpdate.AttestedHeader.Slot) |
There was a problem hiding this comment.
Same as above slots.ToEpoch probably needs to be implemented in compute_sync_committee_period_at_slotas per specs
| v2 "github.com/prysmaticlabs/prysm/v5/proto/eth/v2" | ||
| "github.com/prysmaticlabs/prysm/v5/proto/migration" | ||
| "github.com/prysmaticlabs/prysm/v5/time/slots" | ||
| ) | ||
|
|
||
| const ( | ||
| finalityBranchNumOfLeaves = 6 |
There was a problem hiding this comment.
The same constant is defined in beacon-chain/blockchain/lightclient.go. We should not duplicate constants because then we have to maintain the correct value in more than one place. The correct way is to export the existing one by making it upper-case, and using it in this package.
| newNumActiveParticipants := uint64(0) | ||
| for i := uint64(0); i < maxActiveParticipants; i++ { | ||
| if newUpdate.SyncAggregate.SyncCommitteeBits.BitAt(i) { | ||
| newNumActiveParticipants += 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
SyncCommitteeBits.Count() will give you the number of bits set
| oldNumActiveParticipants := uint64(0) | ||
| for i := uint64(0); i < oldUpdate.SyncAggregate.SyncCommitteeBits.Len(); i++ { | ||
| if oldUpdate.SyncAggregate.SyncCommitteeBits.BitAt(i) { | ||
| oldNumActiveParticipants += 1 | ||
| } | ||
| } |
| newHasRelevantSyncCommittee := isSyncCommitteeUpdate(newUpdate) && (slots.ToEpoch(newUpdate.AttestedHeader.Slot) == slots.ToEpoch(newUpdate.SignatureSlot)) | ||
| oldHasRelevantSyncCommittee := isSyncCommitteeUpdate(oldUpdate) && (slots.ToEpoch(oldUpdate.AttestedHeader.Slot) == slots.ToEpoch(oldUpdate.SignatureSlot)) |
There was a problem hiding this comment.
Comparing epochs like this won't work because the two compared epochs don't have to be equal. It is enough that they are from the same sync committee period. You should use slots.SyncCommitteePeriod().
rupam-04
force-pushed
the
is_better_update-implementation
branch
from
1eaf8ad
to
1829feb
Compare
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Implements the
is_better_updatefunction for light clients from CL specsWhich issues(s) does this PR fix?
Fixes #14185
Other notes for review
Facing an error in 2 other helper functions(
isSyncCommitteeUpdateandisFinalityUpdate) required for theisBetterUpdatefunction. It says:and
respectively. Would appreciate some pointers