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

Endpoint for querying light blocks. #50

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

td202
Copy link
Contributor

@td202 td202 commented Dec 15, 2023

Purpose

Add a GRPC endpoint for querying light client blocks. This returns a stream of proofs that can be used to catch up a light client from one block to another.

Changes

  • Add GetLightBlocks and associated types.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

Comment on lines +3551 to +3559
// If true, indicates that the light client is not aware of the current epoch finalization
// committee for it's highest finalized block.
bool current_committee_unknown = 3;
// If true, indicates that the light client is not aware of the next epoch finalization
// committee for it's highest finalized block.
bool next_committee_unknown = 4;
// If true, the response should include a proof of the next epoch finalization committee for
// the 'to' block.
bool next_committee_required = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: I am not so sure about having these flags, since they will result in the parsed return type having options for this information, but the user specified whether they are present already.
I find that it is usually better for an API to introduce separate functions instead of flags, since it eliminates the error handling of the option.
Also, do we need this many flags? Maybe we could have a separate entry point, just with a proof of current and next committee for a given block or just always provide them as part of this entrypoint.

// Representation of a generic Merkle proof.
// A Merkle proof can be canonically converted to a root hash by converting each of its
// branches to bytes, concatenating them, and then taking the SHA256 hash of the result.
message MerkleProof {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: On top of this, we should probably have new type wrappers for each type of merkle proof that we will expose.


// Branches of a Merkle proof.
// A Merkle branch is either raw data bytes or a sub-Merkle proof.
message MerkleBranch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Consider moving this into MerkleProof as a sub-type.

Comment on lines +3584 to +3592
message LightBlocksItem {
oneof item {
// A finalization entry proving that a block is finalized, and checkable knowing
// the finalization committee for the epoch of the block.
EpochFinalizationEntry finalization_entry = 1;
// A Merkle proof of what the next or current epoch finalization committee is for a block.
MerkleProof committee_proof = 2;
}
}
Copy link
Contributor

@limemloh limemloh Jan 4, 2024

Choose a reason for hiding this comment

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

I have been a while since we talked about this, but could we avoid the oneof by making the committee_proof an optional field?
I think this makes more sense, since the committee_proof is tied to a finalization_entry which has to be emitted right before it.

Suggested change
message LightBlocksItem {
oneof item {
// A finalization entry proving that a block is finalized, and checkable knowing
// the finalization committee for the epoch of the block.
EpochFinalizationEntry finalization_entry = 1;
// A Merkle proof of what the next or current epoch finalization committee is for a block.
MerkleProof committee_proof = 2;
}
}
message LightBlocksItem {
// A finalization entry proving that a block is finalized, and checkable knowing
// the finalization committee for the epoch of the block.
EpochFinalizationEntry finalization_entry = 1;
// A Merkle proof of what the next or current epoch finalization committee is for a block.
optional MerkleProof committee_proof = 2;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants