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

API for LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events #3954

Merged
merged 25 commits into from
Nov 28, 2023

Conversation

GeemoCandama
Copy link
Contributor

@GeemoCandama GeemoCandama commented Feb 8, 2023

Issue Addressed

#4894 and partially #3651

Proposed Changes

This PR implements the following Beacon APIs:

Additional Info

When the --light_client_server is not enabled a generic 404 Not Found is returned, whereas when the server is enabled but the server has no latest update stored, a 404 with the message saying that No LightClientOptimisticUpdate is available.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

The routes added by this PR follow the spec at a time. It is not compliant with current spec as the types have not been updated to use the new lightclient header type.

Since all server routes are behind a flag this PR is technically a strict improvement over unstable. The types can be fixed in a follow-up PR. Left a note on the eth2 lib, which will expose a broken client available by default.

I would suggest to fix conflicts and merge as-is, and optionally delete the http client routes

beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved
beacon_node/http_api/src/lib.rs Show resolved Hide resolved
common/eth2/src/lib.rs Outdated Show resolved Hide resolved
@jimmygchen jimmygchen changed the title beacon api for LightClientFinalityUpdate and OptimisticUpdate API for LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events Nov 15, 2023
@jimmygchen jimmygchen changed the title API for LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events API for LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events Nov 15, 2023
@jimmygchen jimmygchen self-assigned this Nov 15, 2023
# Conflicts:
#	beacon_node/beacon_chain/src/events.rs
#	beacon_node/http_api/src/lib.rs
#	beacon_node/http_api/src/test_utils.rs
#	beacon_node/http_api/tests/main.rs
#	beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs
#	beacon_node/lighthouse_network/src/rpc/methods.rs
#	beacon_node/lighthouse_network/src/service/api_types.rs
#	beacon_node/network/src/beacon_processor/worker/rpc_methods.rs
#	beacon_node/tests/test.rs
#	common/eth2/src/types.rs
#	lighthouse/src/main.rs
@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress HTTP-API labels Nov 16, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 16, 2023
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Impl is correct and good except two nits!

consensus/types/src/light_client_bootstrap.rs Outdated Show resolved Hide resolved
beacon_node/http_api/src/lib.rs Outdated Show resolved Hide resolved
…tBootstrap` API to return `ForkVersionedResponse`.
@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Nov 17, 2023
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Everything looks good now, excellent!

Surfacing the two CI errors for others, both appear to be unrelated?

compile-with-beta-compiler

 Compiling slot_clock v0.2.0 (/home/runner/work/lighthouse/lighthouse/common/slot_clock)
error: unused import: `std::time::SystemTimeError`
 --> common/slot_clock/src/system_time_slot_clock.rs:5:9
  |
5 | pub use std::time::SystemTimeError;
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: could not compile `slot_clock` (lib) due to previous error

check-code

error: unnecessary map_err on constructor Err(_)
   --> beacon_node/lighthouse_network/src/rpc/codec/ssz_snappy.rs:380:14
    |
380 |         _ => Err(err).map_err(RPCError::from),
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Err(RPCError::from(err))`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_on_constructor
    = note: `-D clippy::unnecessary-map-on-constructor` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::unnecessary_map_on_constructor)]`

   Compiling ethers-contract-derive v1.0.2
error: could not compile `lighthouse_network` (lib) due to previous error

@jimmygchen
Copy link
Member

jimmygchen commented Nov 17, 2023

Yeah these are related to the new Rust release (fix in #4931) and new lint warnings from the new beta compiler version (fix in #4932)

- log cleanup
- move http_api config mutation to `config::get_config` for consistency
- fix light client API responses
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 17, 2023
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

I've made a couple of changes to bring this PR up to date with unstable. I think it looks good to me now - would be good to have this merged first, as there are a few parallel work streams on light client right now (caching optimisation, capella types etc) that may depend on some of the fixes here.

@jimmygchen jimmygchen linked an issue Nov 20, 2023 that may be closed by this pull request
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks great! I had one very minor perf suggestion, but sounds like we'll be re-working and optimising this anyway so happy to merge without it!

beacon_node/beacon_chain/src/beacon_chain.rs Outdated Show resolved Hide resolved
@michaelsproul
Copy link
Member

Rad, lets merge once CI passes.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 28, 2023
@michaelsproul michaelsproul merged commit 8a599ec into sigp:unstable Nov 28, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add beacon API support for light client bootstrap
4 participants