Add Capella & Deneb light client support

[eserilev]

Issue Addressed

#4896

Proposed Changes

conform to capella/deneb light client specs

• Capella light client spec
• Deneb light client spec

Additional Info

https://github.com/sigp/lighthouse/pull/3954 is a blocker

[eserilev]

Here's a quick update on what I've got so far:

• I explored superstruct usage within LightClientHeader but I believe using Option and handling the different execution payload types (i.e. capella, deneb) to be the better solution here. If we were to use a superstruct I think we'd need to make some changes to additional light client related structs which feels bad.
• I'm a bit iffy on the code I wrote that creates the merkle proof. This is the main thing I'm trying to figure out at the moment
• I'm wondering what kind of tests we should write here.
• I need to get SSZ encode and decode to play nice with Option<ExecutionPayloadHeader>

[eserilev]

I opened the PR, but theres a few TODOs ill be taking care of tomorrow

• add tests for beacon block body merkle proof
• add tests for new light client header helper functions

also a question:

the new helper functions are considered dead code at the moment which causes linting errors. I think I can tell clippy to ignore the dead code, but should these helper functions be called anywhere? (outside of tests)

[jimmygchen]

Re the unused helper functions, I guess you meant LightClientHeader::new (equivalent to the spec's block_to_light_client_header function, may be worth adding a comment on top of the function). This will be used when deriving light client data, see deriving light client data in altair specs.

@dapplion is currently working on computing and caching the light client data for Altair (#4926) , this function would be used when producing the light client data, I think your function implementation would replace this: https://github.com/sigp/lighthouse/compare/unstable...dapplion:lighthouse:lc-prod-recent-updates?expand=1#diff-19cf8a8a12dc28ae7de4622c52ec606add9dbe2abef58b59209cccf372975234R219-R224

[jimmygchen]

FYI I created a PR that runs light client SSZ test vectors from consensus-spec-tests - if it's useful for testing your SSZ type changes, feel free to merge it into your branch. Otherwise we can review that one after your PR is merged. https://github.com/sigp/lighthouse/pull/4938

[eserilev]

I pushed up my changes regarding superstruct. Theres still a small issue or two that needs to be resolved. To summarize:

Superstruct Changes

• LightClientHeader is now a superstruct with three variants, Altair, Capella and Deneb. Each variant has its own implementation for block_to_light_client_header, get_lc_execution_root, is_valid_light_client_header

LightClientBootstrap::from_beacon_state

• Added ChainSpec as one of the function arguments so we can make a fork consistency check here.
• The function checks what fork we're on (altair, capella or deneb) and chooses to construct the correct LightClientHeader variant based on the fork check. There might be a better way to do this.

LightClientFinalityUpdate::new

• added an additional function argument attested_block: SignedBeaconBlock<T> which is required to create the attested_header
• updated finalized_block arg to &SignedBeaconBlock<T> (from SignedBlindedBeaconBlock), beacause we need the execution payload in order to construct the LightClientHeader

LightClientOptimisticUpdate::new

• added an additional function argument attested_block: SignedBeaconBlock<T> which is required to create the attested_header
• The function checks what fork we're on (altair, capella or deneb) and chooses to construct the correct LightClientHeader variant based on the fork check. There might be a better way to do this.

LightClientUpdate::new

• added attested_block: SignedBeaconBlock<T> as an argument to new, I believe this is required per the spec
• The function checks what fork we're on (altair, capella or deneb) and chooses to construct the correct LightClientHeader variant based on the fork check. There might be a better way to do this.

TestRandom

• TestRandom cannot be derived for enum types. With the superstruct changes to LightClientHeader deriving TestRandom for LightClientFinalityUpdate, LightClientOptimisticUpdate, LightClientUpdate and LightClientBootstrap causes compilation issues. I'll need to dig in here to resolve

I also left a comment regarding a small change in beacon_node/lighthouse_network/src/rpc/methods.rs thats causing an issue

Fetching the full beacon block

In Altair, we only needed the block header to construct a LightClientHeader. In Capella and Deneb we need the execution payload as well. In places where we were fetching a SignedBlindedBeaconBlock we are now required to fetch the full beacon block. Fetching the full beacon block is done by calling the async function get_block. Since the LightClient code is sync, I created a runtime object (via tokio::runtime) to allow for executing async code within a sync function. I'm not sure what the overhead is for creating these runtimes, or if theres a better/more standard way of doing this.

[jimmygchen]

Awesome, I'll take a look later today or tomorrow!

[eserilev]

I've pushed some significant changes/fixes to resolve some issues that I caught after writing my latest update. I'll edit my post to reflect those changes

[ec2]

Hey friends! Very excited for this PR to land. Currently we need to maintain our own structs and would like to instead use Lighthouse's fork/version aware stuff. https://github.com/ralexstokes/ethereum-consensus/ also similarly has not implemented these types yet and also currently doesn't support merkle proofs.

Light client server functionality aside, it would be dope if just the struct def'n land so that we can make API calls to Lodestar to get the data we need.

Is this PR active? Or will we need continue to maintain our own structs for the foreseeable future?

[jimmygchen]

Hi @ec2, Yes this PR is still active! We're currently reviewing #4969, and once that's merged we'll revisit this PR, hopefully won't be too long!

[ec2]

Thanks for the update @jimmygchen !!

[jimmygchen]

@eserilev #4969 has been merged 🎉 so it should unblock this PR! There are now some conflicts that needs to be resolved.

[eserilev]

I've resolved the merge conflicts. Need to take another look through the code to make sure its ready for a review. Will ping here as soon as I think its ready.

On a separate note: We decided to change LightClientHeader to a superstruct. The struct used to derive TestRandom, I believe for compatibility with the ssz_test! macro. With the changes to superstruct, LightClientHeader is now an enum. TestRandom doesnt work with enums. I've commented out the calls to ssz_test! for now. If anyone has any better ideas here let me know

[jimmygchen]

Sounds good!

You can have TestRandom added to the superstruct variant attributes, so each generated struct derives it, like this: https://github.com/sigp/lighthouse/blob/31044402ee180ff937027ec842513bef90d7eec8/consensus/types/src/beacon_block_body.rs#L25-L55

[eserilev]

Sorry I should have been more clear. Yes I was able to add TestRandom to the superstruct itself within the variant attributes like you mentioned. However, this causes issues downstream e.g. LightClientBootstrap. Since one of the fields in LightClientBootstrap is a LightClientHeader I am unable to derive TestRandom for LightClientBootstrap. The issue I guess is that the variants of LightClientHeader derive TestRandom, but LIghtClientHeader itself does not.

[eserilev]

I think this PR is in a good spot for another review

[eserilev]

@jimmygchen

commit 06da24a0f1b6f5a66dffe5da8611583a56f166a6 is my attempt at a workaround for TestRandom on structs that include a LightClientHeader field. My reasoning here is that since LightClientHeaderDeneb is a superset of all other variants, it makes most sense to use it to implement TestRandom on LightClientHeader. Since TestRandom is strictly for unit tests, it felt ok for me to do this. The other option here is to convert LightClientBootstrap, LightClientUpdate, LightClientFinalityUpdate and LightClientOptimisticUpdate to superstructs. I'm hoping to avoid that if possible. Let me know what you think.

[dapplion]

Would like to see if it's simpler or worse to make LightClientOptimisticUpdate a superstruct instead of rolling our own decoding logic. Otherwise looks good, but spec tests for lightclient data structures are breaking

[eserilev]

had to make some small tweaks to the ef_test framework to get things working.

@dapplion @jimmygchen I have a PR here that uses superstruct for all the LightClient types

https://github.com/eserilev/lighthouse/pull/2/files

superstruct is more compatible with the ef_test framework, and removes the need for manual encode/decode implementations. It does make certain parts of the code a bit more verbose, and there might be some additional maintenance work needed for new forks. Lmk what you guys think!

[jimmygchen]

@eserilev I've reviewed your superstruct PR and it looks good. I've added some small comments, but we're really close!

[jimmygchen]

btw @dapplion the optimistic updates & finality caching seems to be working great, response times were pretty much instant 🥳

[realbigsean]

@Mergifyio queue

[mergify[bot]]

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI. Then, re-embark the pull request into the merge queue by posting the comment @mergifyio refresh on the pull request.

[eserilev]

resolved a beta compiler issue

[realbigsean]

@Mergifyio requeue

[mergify[bot]]

requeue

❌ This pull request head commit has not been previously disembarked from queue.

[realbigsean]

@Mergifyio queue

[mergify[bot]]

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI. Then, re-embark the pull request into the merge queue by posting the comment @mergifyio refresh on the pull request.

[jimmygchen]

@mergify queue

[mergify[bot]]

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI. Then, re-embark the pull request into the merge queue by posting the comment @mergifyio refresh on the pull request.

[jimmygchen]

@mergify requeue