-
Notifications
You must be signed in to change notification settings - Fork 980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement is_better_update
for light client
#14186
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above slots.ToEpoch
probably needs to be implemented in compute_sync_committee_period_at_slot
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
1eaf8ad
to
1829feb
Compare
@rkapka Since in helpers.go only |
Co-authored-by: Radosław Kapka <[email protected]>
Co-authored-by: Radosław Kapka <[email protected]>
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Implements the
is_better_update
function for light clients from CL specsWhich issues(s) does this PR fix?
Fixes #14185
Part of #12991
Other notes for review
Facing an error in 2 other helper functions(
isSyncCommitteeUpdate
andisFinalityUpdate
) required for theisBetterUpdate
function. It says:and
respectively. Would appreciate some pointers