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 updates by range rpc and add LightClientHeader #3886

Closed
wants to merge 24 commits into from

Conversation

GeemoCandama
Copy link
Contributor

@GeemoCandama GeemoCandama commented Jan 15, 2023

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

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
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.

};

/// Maximum number of blocks in a single request.
pub type MaxRequestBlocks = U1024;
pub const MAX_REQUEST_BLOCKS: u64 = 1024;

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

Choose a reason for hiding this comment

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

Is this used?

@jimmygchen
Copy link
Member

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants