Skip to content

Light client updates by range rpc and add LightClientHeader#3886

Closed
GeemoCandama wants to merge 24 commits intosigp:unstablefrom
GeemoCandama:lcupdatesbyrange
Closed

Light client updates by range rpc and add LightClientHeader#3886
GeemoCandama wants to merge 24 commits intosigp:unstablefrom
GeemoCandama:lcupdatesbyrange

Conversation

@GeemoCandama
Copy link
Contributor

@GeemoCandama GeemoCandama commented Jan 15, 2023 •
edited
Loading

Issue Addressed

#3651

Proposed Changes

  • Add LightClientHeader and use it in place of BeaconBlockHeader in the other light client structs.
  • Make the LightClient update constructors reflect the spec more because the current implementation avoids some checks outlined in the spec.
  • Add is_better_update function for LightClientUpdates
  • Outline LightClientUpdates rpc
  • Add LightClientUpdates DBColumn to the db indexed by sync committee period and add migration file
  • add get_light_client_update and put_light_light_client_update and use them to complete the rpc
  • add is_better_update tests (I think there is a standard set of tests)
  • add tests for LightClientUpdate rpc
  • use proper db iterator in request handler

Additional Info

The database stuff is confusing for me but I think I'll have time to finish this PR by the end of the week.
EDIT: Would this be better as two separate PRs?

@GeemoCandama
Copy link
Contributor Author

GeemoCandama commented Jan 23, 2023 •
edited
Loading

The test I did for the update_ranking ef_test is a bit awkward because it doesn't follow the pattern of many of the other tests. I am comparing each update to eachother twice but with the updates in opposite order. I used a wrapper struct around bool to use the sweet sweet procedural macro for CompareFields. Let me know if you want this changed.
EDIT: I'm setting this to ready to review because to make the tests work for the rpcs will require a lot of code. I'll make another PR if you want testing. Also, I'd appreciate a bit of assistance using the appropriate db iterator.

@GeemoCandama GeemoCandama marked this pull request as ready for review February 14, 2023 04:23
dapplion
dapplion reviewed Nov 15, 2023
View reviewed changes
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.

This PR implements the wiring for ReqResp method light client updates, but does not produce the necessary messages. The wiring part is correct, but merging as-is would add a route that will always return resource not available.

The store methods make some assumptions on how updates are produced, i.e. using the split to store in either hot / cold. That may not be necessary in a progressive lightclient server (compute updates eagerly).

I would put this PR on ice, implement the message producing logic, and then wire the network based of these commits.

beacon_node/lighthouse_network/src/rpc/methods.rs
pub const MAX_REQUEST_BLOCKS: u64 = 1024;

/// Maximum number of light client updates in a single request.
pub type MaxRequestLightClientUpdates = U128;
Copy link
Collaborator

@dapplion dapplion Nov 15, 2023

Choose a reason for hiding this comment

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

Is this used?

@jimmygchen jimmygchen mentioned this pull request Nov 15, 2023
Open
22 tasks
This was referenced Nov 21, 2023
Open
Closed
@jimmygchen
Copy link
Member

jimmygchen commented Jun 19, 2024

This branch is now quite outdated, and we've made quite a bit of progress on light client server, and it might be easier to close this one and start from scratch, what do you think @eserilev ?

@dapplion
Copy link
Collaborator

dapplion commented Jun 27, 2024

Most of the outstanding changes of this PR have been covered in other light-client PRs, closing. Thank you so much @GeemoCandama for initially driving this effort forward! It's on us to take too long to dedicate bandwidth to this.

@dapplion dapplion closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dapplion dapplion dapplion left review comments

Assignees

No one assigned

Labels

blocked light-client

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GeemoCandama @jimmygchen @dapplion