API for LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events#3954
Merged
michaelsproul merged 25 commits intosigp:unstablefrom Nov 28, 2023
Conversation
dapplion
reviewed
Nov 15, 2023
Collaborator
dapplion
left a comment
There was a problem hiding this comment.
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
22 tasks
LightClientBootstrap, LightClientFinalityUpdate, LightClientOptimisticUpdate and light client events
# 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
dapplion
approved these changes
Nov 16, 2023
d23bba7 to
bfd3fb7
Compare
…tBootstrap` API to return `ForkVersionedResponse`.
7c2b5b6 to
d90df3f
Compare
dapplion
approved these changes
Nov 17, 2023
Collaborator
dapplion
left a comment
There was a problem hiding this comment.
Everything looks good now, excellent!
Surfacing the two CI errors for others, both appear to be unrelated?
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
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
Member
- log cleanup - move http_api config mutation to `config::get_config` for consistency - fix light client API responses
92f7b7e to
75bd2ac
Compare
jimmygchen
approved these changes
Nov 20, 2023
Member
jimmygchen
left a comment
There was a problem hiding this comment.
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.
dapplion
approved these changes
Nov 20, 2023
This was referenced Nov 21, 2023
This was referenced Nov 21, 2023
michaelsproul
approved these changes
Nov 28, 2023
Member
michaelsproul
left a comment
There was a problem hiding this comment.
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!
Member
|
Rad, lets merge once CI passes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Addressed
#4894 and partially #3651
Proposed Changes
This PR implements the following Beacon APIs:
/eth/v1/beacon/light_client/bootstrap/{block_root}/eth/v1/beacon/light_client/optimistic_update/eth/v1/beacon/light_client/finality_updatelight_client_optimistic_updateandlight_client_finality_updatetopics to/eth1/v1/events.Additional Info
When the
--light_client_serveris not enabled a generic404 Not Foundis returned, whereas when the server is enabled but the server has no latest update stored, a404with the message saying that NoLightClientOptimisticUpdateis available.