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

Light client consensus types #14512

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Oct 7, 2024

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR creates a foundation for changing how we interact with light client types in the codebase. Currently we use protobuf-generated objects everywhere, but this has some problems:

  • LightClientHeader is defined using a oneof, which makes it impossible to generate SSZ for this type or any type referencing the header. Without this, we can't return SSZ-serialized objects from the Beacon API.
  • Since Prysm wants to ultimately move away from protocol buffers, it's undesirable to have them used everywhere for light client work. This will make a future switch much harder.

This PR's design is largely influenced by https://github.com/prysmaticlabs/prysm/blob/develop/consensus-types/blocks/execution.go. Main components:

  • New protobuf types declared in v1alpha1 with one type per fork instead of using a oneof. The reason for having them in v1alpha1 is that it is the default directory for protobuf definitions. The eth/v1 and eth/v2 directories are an artifact of some previous work and should be removed ultimately.
  • Interfaces for all spec objects with each of them implementing the ssz.Marshaler interface:
    • LightClientHeader
    • LightClientBootstrap
    • LightClientUpdate
    • LightClientFinalityUpdate
    • LightClientOptimisticUpdate
  • Wrappers for each protobuf type that do not expose the light client protobuf. All SSZ methods are simply delegated to the underlying protobuf.

Which issues(s) does this PR fix?

Part of #12991

Acknowledgements

  • I have read CONTRIBUTING.md.
  • I have made an appropriate entry to CHANGELOG.md.
  • I have added a description to this PR with sufficient context for reviewers to understand this PR.

@rkapka rkapka requested a review from a team as a code owner October 7, 2024 15:00
Comment on lines +33 to +34
header interfaces.LightClientHeader
currentSyncCommitteeBranch interfaces.LightClientSyncCommitteeBranch
Copy link
Contributor Author

@rkapka rkapka Oct 7, 2024

Choose a reason for hiding this comment

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

I added fields for object that would have to be re-created every time the getter is called. That's one pointer to a wrapper with one pointer field for each header, and at most 6*32 bytes for branches.

Comment on lines +47 to +60
if len(p.CurrentSyncCommitteeBranch) != interfaces.SyncCommitteeBranchNumOfLeaves {
return nil, fmt.Errorf(
"sync committee branch has %d leaves instead of expected %d",
len(p.CurrentSyncCommitteeBranch),
interfaces.SyncCommitteeBranchNumOfLeaves,
)
}
branch := interfaces.LightClientSyncCommitteeBranch{}
for i, leaf := range p.CurrentSyncCommitteeBranch {
if len(leaf) != fieldparams.RootLength {
return nil, fmt.Errorf("sync committee branch leaf at index %d has length %d instead of expected %d", i, len(leaf), fieldparams.RootLength)
}
branch[i] = bytesutil.ToBytes32(leaf)
}
Copy link
Contributor Author

@rkapka rkapka Oct 7, 2024

Choose a reason for hiding this comment

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

This pattern appears many times, I will de-duplicate it in a future commit

Comment on lines +11 to +13
ExecutionBranchNumOfLeaves = 4
SyncCommitteeBranchNumOfLeaves = 5
FinalityBranchNumOfLeaves = 6
Copy link
Contributor Author

@rkapka rkapka Oct 7, 2024

Choose a reason for hiding this comment

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

I should probably move this to fieldparams, rename NumOfLeaves to Depth and rename SyncCommitteeBranch to NextSyncCommitteeBranch for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant