Skip to content
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

Merged
merged 15 commits into from
Jul 23, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: implement is_better_update for light client
  • Loading branch information
rupam-04 committed Jul 6, 2024
commit 382e5c186ad8a41719358db86bddd4d64e7b8574
76 changes: 76 additions & 0 deletions beacon-chain/rpc/eth/light-client/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@ import (
"github.com/prysmaticlabs/prysm/v5/config/params"
"github.com/prysmaticlabs/prysm/v5/consensus-types/interfaces"
v1 "github.com/prysmaticlabs/prysm/v5/proto/eth/v1"
ethpbv2 "github.com/prysmaticlabs/prysm/v5/proto/eth/v2"
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
Copy link
Contributor

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.

)

// 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:

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.

//
Expand Down Expand Up @@ -311,3 +316,74 @@ func newLightClientUpdateToJSON(input *v2.LightClientUpdate) *structs.LightClien
SignatureSlot: strconv.FormatUint(uint64(input.SignatureSlot), 10),
}
}

func isSyncCommitteeUpdate(update *ethpbv2.LightClientUpdate) bool {
nextSyncCommitteeBranch := make([][]byte, fieldparams.NextSyncCommitteeBranchDepth)
return update.NextSyncCommitteeBranch != nextSyncCommitteeBranch
rupam-04 marked this conversation as resolved.
Show resolved Hide resolved
}

func isFinalityUpdate(update *ethpbv2.LightClientUpdate) bool {
finalityBranch := make([][]byte, finalityBranchNumOfLeaves)
return update.FinalityBranch != finalityBranch
rupam-04 marked this conversation as resolved.
Show resolved Hide resolved
}

func isBetterUpdate(newUpdate, oldUpdate *ethpbv2.LightClientUpdate) bool {
maxActiveParticipants := newUpdate.SyncAggregate.SyncCommitteeBits.Len()
newNumActiveParticipants := uint64(0)
for i := uint64(0); i < maxActiveParticipants; i++ {
if newUpdate.SyncAggregate.SyncCommitteeBits.BitAt(i) {
newNumActiveParticipants += 1
}
}
Copy link
Contributor

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
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

newHasSupermajority := newNumActiveParticipants*3 >= maxActiveParticipants*2
oldHasSupermajority := oldNumActiveParticipants*3 >= maxActiveParticipants*2

if newHasSupermajority != oldHasSupermajority {
return newHasSupermajority
}
if !newHasSupermajority && newNumActiveParticipants != oldNumActiveParticipants {
return newNumActiveParticipants > oldNumActiveParticipants
}

// Compare presence of relevant sync committee
newHasRelevantSyncCommittee := isSyncCommitteeUpdate(newUpdate) && (slots.ToEpoch(newUpdate.AttestedHeader.Slot) == slots.ToEpoch(newUpdate.SignatureSlot))
oldHasRelevantSyncCommittee := isSyncCommitteeUpdate(oldUpdate) && (slots.ToEpoch(oldUpdate.AttestedHeader.Slot) == slots.ToEpoch(oldUpdate.SignatureSlot))

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

Copy link
Contributor Author

@rupam-04 rupam-04 Jul 5, 2024

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

Copy link
Contributor

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().


if newHasRelevantSyncCommittee != oldHasRelevantSyncCommittee {
return newHasRelevantSyncCommittee
}

// Compare indication of any finality
newHasFinality := isFinalityUpdate(newUpdate)
oldHasFinality := isFinalityUpdate(oldUpdate)
if newHasFinality != oldHasFinality {
return newHasFinality
}

// Compare sync committee finality
if newHasFinality {
newHasSyncCommitteeFinality := slots.ToEpoch(newUpdate.FinalizedHeader.Slot) == slots.ToEpoch(newUpdate.AttestedHeader.Slot)
oldHasSyncCommitteeFinality := slots.ToEpoch(oldUpdate.FinalizedHeader.Slot) == slots.ToEpoch(oldUpdate.AttestedHeader.Slot)

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_slotas per specs


if newHasSyncCommitteeFinality != oldHasSyncCommitteeFinality {
return newHasSyncCommitteeFinality
}
}

// Tiebreaker 1: Sync committee participation beyond supermajority
if newNumActiveParticipants != oldNumActiveParticipants {
return newNumActiveParticipants > oldNumActiveParticipants
}

// Tiebreaker 2: Prefer older data (fewer changes to best)
if newUpdate.AttestedHeader.Slot != oldUpdate.AttestedHeader.Slot {
return newUpdate.AttestedHeader.Slot < oldUpdate.AttestedHeader.Slot
}
return newUpdate.SignatureSlot < oldUpdate.SignatureSlot
}