-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[2/5] light client http api #12984
[2/5] light client http api #12984
Conversation
1dc44a0
to
e154a1e
Compare
Hi @nicopernas , can you please open one more PR and move
|
That's no problem at all. If it's not an urgent change, can we do it after the rest of the PRs are merged?? It would save me from rebasing once or twice :) |
45e25ff
to
9efb69e
Compare
OK, but in that case I will open an issue for it, just to make sure this is not forgotten. |
Thanks for that! |
) | ||
|
||
// GetLightClientBootstrap - implements https://github.com/ethereum/beacon-APIs/blob/263f4ed6c263c967f13279c7a9f5629b51c5fc55/apis/beacon/light_client/bootstrap.yaml | ||
func (bs *Server) GetLightClientBootstrap(w http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All HTTP receivers are named s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
// Get the block | ||
blockRootParam, err := hexutil.Decode(mux.Vars(req)["block_root"]) | ||
if err != nil { | ||
http2.HandleError(w, "invalid block root "+err.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http2.HandleError(w, "invalid block root "+err.Error(), http.StatusBadRequest) | |
http2.HandleError(w, "invalid block root: "+err.Error(), http.StatusBadRequest) |
similarly in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
var blockRoot [32]byte | ||
copy(blockRoot[:], blockRootParam) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a bytesutil.ToBytes32
function that you can use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
} | ||
|
||
response := &LightClientBootstrapResponse{ | ||
Version: ethpbv2.Version(blk.Version()).String(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better to use the String
function defined in runtime/version/fork.go
. We want to avoid depending on ethpbv1
/ethpbv2
because we plan to remove these packages in the near future. Another advantage is that it returns a lower-case version name, which is aligned with the API spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
func CreateLightClientBootstrap(ctx context.Context, state state.BeaconState) (*LightClientBootstrap, error) { | ||
// assert compute_epoch_at_slot(state.slot) >= ALTAIR_FORK_EPOCH | ||
if slots.ToEpoch(state.Slot()) < params.BeaconConfig().AltairForkEpoch { | ||
return nil, fmt.Errorf("invalid state slot %d", state.Slot()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better to be more informative here and say something like "light client bootstrap is not supported before Altair"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
) | ||
|
||
const ( | ||
nextSyncCommitteeBranchNumOfLeaves = 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably only correct for minimal config, where the sync committee size is 32. For mainnet config the sync committee size is 512 and therefore the proof will be larger. In order to make this work for both, you should define a minimal and mainnet version of this const in the fieldparams
package (the name of the package is a little outdated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This const
may be wrongly named. It basically states the number of branches needed for a proof.
This is only relevant the position of the field in the trie leaves. (Where currentSyncCommittee and nextSyncCommittee are next to each other).
So this isn't related to sync committee
size (no matter it is 32 or 512), but related to the total number of leaves in the trie.
Yes making this configurable is future proof though. So let's make it configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
) | ||
|
||
type Server struct { | ||
BeaconDB db.ReadOnlyDatabase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lookup.Blocker
interface that's probably better suited to use for handlers. You can pass the block root into the Block
function and get the same result as you currently do, but unit testing should be much easier as you won't need to save anything to the db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Thanks.
Patched.
resp := &LightClientBootstrapResponse{} | ||
require.NoError(t, json.Unmarshal(writer.Body.Bytes(), resp)) | ||
require.Equal(t, "CAPELLA", resp.Version) | ||
require.Equal(t, hexutil.Encode(header.Header.BodyRoot), resp.Data.Header.BodyRoot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have the helper function that prepares the whole bootstrap object, I think it's fine to check only the version here and maybe verify that resp.Data
is not nil. But I would still check values of several bootstrap fields by writing a unit test of the helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment, let's just check resp.Data
not nil, and existing very basic field checking.
Feel free to create a following task, so we can add more solid unit tests.
|
||
// Return result | ||
result := &LightClientBootstrap{ | ||
Header: &apimiddleware.BeaconBlockHeaderJson{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly comment as for apimiddleware.SyncCommitteeJson
, but this time please put the new type inside /eth/shared/structs_blocks.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
Header *apimiddleware.BeaconBlockHeaderJson `json:"header"` | ||
CurrentSyncCommittee *apimiddleware.SyncCommitteeJson `json:"current_sync_committee"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two fields should be of the newly created types that I mentioned in other comments, not the ones in apimiddleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
I reviewed the bootstrap function and will likely continue tomorrow. |
I found something missing in your previous PR. In your proto definitions you use Looking at these variables, will Speaking about SSZ generation, API spec requires SSZ support:
This would mean that SSZ generation for |
var nextSyncCommitteeBranch [][]byte | ||
|
||
// update_signature_period = compute_sync_committee_period(compute_epoch_at_slot(block.message.slot)) | ||
updateSignaturePeriod := uint64(block.Block().Slot()) / slotsPerPeriod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the slots.ToEpoch
helper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
updateSignaturePeriod := uint64(block.Block().Slot()) / slotsPerPeriod | ||
|
||
// update_attested_period = compute_sync_committee_period(compute_epoch_at_slot(attested_header.slot)) | ||
updateAttestedPeriod := uint64(result.AttestedHeader.Slot) / slotsPerPeriod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
countParam := req.URL.Query().Get("count") | ||
count, err := strconv.ParseUint(countParam, 10, 64) | ||
if err != nil { | ||
http2.HandleError(w, fmt.Sprintf("got invalid 'count' query variable '%s', err %v", countParam, err), | ||
http.StatusInternalServerError) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a shared.UintFromQuery
helper that you can use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
startPeriodParam := req.URL.Query().Get("start_period") | ||
startPeriod, err := strconv.ParseUint(startPeriodParam, 10, 64) | ||
if err != nil { | ||
http2.HandleError(w, fmt.Sprintf("got invalid 'start_period' query variable '%s', err %v", startPeriodParam, err), | ||
http.StatusInternalServerError) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
return | ||
} | ||
|
||
lHeadSlot := uint64(headState.Slot()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be renamed to headSlot
? We don't use such prefixes anywhere in the codebase. Similarly for other variables in this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, patched.
Hey @rkapka thanks a lot for all the good feedback! We are making internal changes to our team structure and I will be looking into something else from now own. Someone else from the team will take over to make sure this and future PRs make it to |
} | ||
|
||
// Get the block | ||
latestBlockHeader := *state.LatestBlockHeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
attestedState, | ||
finalizedBlock, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
// sync_aggregate=block.message.body.sync_aggregate, | ||
// signature_slot=block.message.slot, | ||
// ) | ||
func CreateLightClientUpdate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered this part of the spec? I don't see this implemented.
Full nodes SHOULD provide the best derivable LightClientUpdate (according to is_better_update) for each sync committee period covering any epochs in range [max(ALTAIR_FORK_EPOCH, current_epoch - MIN_EPOCHS_FOR_BLOCK_REQUESTS), current_epoch] where current_epoch is defined by the current wall-clock time. Full nodes MAY also provide LightClientUpdate for other sync committee periods.
- LightClientUpdate are assigned to sync committee periods based on their attested_header.beacon.slot
- LightClientUpdate are only considered if compute_sync_committee_period_at_slot(update.attested_header.beacon.slot) == compute_sync_committee_period_at_slot(update.signature_slot)
- Only LightClientUpdate with next_sync_committee as selected by fork choice are provided, regardless of ranking by is_better_update. To uniquely identify a non-finalized sync committee fork, all of period, current_sync_committee and next_sync_committee need to be incorporated, as sync committees may reappear over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. There is no implementation for above mentioned text section.
We only implemented the python pseudo spec.
Can we create a separate task to patch this? As this function is individual function, not used by others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helpers don't need to be exported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct.
Patched, only NewLightClientBootstrapFromJSON
is exported, and this is used in light client side.
require.Equal(t, hexutil.Encode(header.Header.BodyRoot), resp.Data.Header.BodyRoot) | ||
} | ||
|
||
func TestLightClientHandler_GetLightClientUpdatesByRange(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This endpoint has quite a lot of logic in it - can we have additional test cases?
count > config.MaxRequestLightClientUpdates
startPeriodEndSlot < uint64(config.AltairForkEpoch)*uint64(config.SlotsPerEpoch)
headPeriod < endPeriod
lLastSlotInPeriod > lHeadSlot
len(updates) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
if block == nil { | ||
return nil, fmt.Errorf("latest block is nil") | ||
} | ||
// Loop through the blocks until we find a block that has super majority of sync committee signatures (2/3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this comment is correct? Isn't supermajority required only when calling from GetLightClientFinalityUpdate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, the comment isn't correct.
Patched.
} | ||
|
||
// getLightClientEventBlock - returns the block that should be used for light client events, which satisfies the minimum number of signatures from sync committee | ||
func (bs *Server) getLightClientEventBlock(ctx context.Context, minSignaturesRequired uint64) (interfaces.ReadOnlySignedBeaconBlock, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some unit tests, two at a minimum:
- happy case
- number of signatures too low, need to fetch parent block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched.
} | ||
|
||
// getLightClientEventBlock - returns the block that should be used for light client events, which satisfies the minimum number of signatures from sync committee | ||
func (bs *Server) getLightClientEventBlock(ctx context.Context, minSignaturesRequired uint64) (interfaces.ReadOnlySignedBeaconBlock, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered caching? Fetching block from the database in a loop is expensive. If your current implementation does not make use of caches, then we should consider adding it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't consider the cache. Please add a task, so we can improve this later.
Depending on the configuration, typically, satisfying the loop condition isn't hard. (But yes, in extreme error conditions, loop will be triggered severely, and causing performance issue).
BTW, since we switch to use lookup.Blocker
, does this component provide cache automatically? If so, then we don't need to add cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't. I will add a task.
@rkapka This is I will address your comments and submit revisions. This will take some time. Thanks! |
Hi @qinlz2 , are you able to say how when you might have some time to pick this up? We would like to give light client support higher priority and implement this in the next few months (by "this" I mean not this one PR, but the whole thing). |
Yes, commit hours for this now. Was deviate from this PR (and the rest of PRs) a little bit.
|
@rkapka In our initial PRs, we only implements JSON support. Please create task for later SSZ support. Thanks! |
Between mainnet and min, there is no difference. We can hardcode (and we actually didn't use these at all in the code I think). So I will remove them for now from proto definitions. |
@rkapka I pushed new revision based on your valuable comments. |
Hey @qinlz2 , everything looks great! My only concern is #12984 (comment) - test names don't align with the test scenarios that I listed. Can you confirm that all test cases are covered? Maybe test names are incorrect? |
I updated #12991 to serve as a general tracking issue for future light client improvements. |
if sc == nil { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this? The reason is twofold:
- we can safely assume structs coming from core code are not nil
- even if they are nil, it's better to panic as soon as possible, rather than returning nil,and then panicking somewhere else in code; it will make debugging easier
If you look at other FromConsensus
functions, there is no clear pattern. We sometimes check for nil and return errors, sometimes not. We intend to unify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, patched.
Yes, all of them are covered and tested. (Due to slight code sequence re-order, the naming for test case won't be exactly the same as your original comment, but all the important |
@rkapka I update slightly based on your recent comment. |
ce21485
to
48cfc79
Compare
Thanks, @rkapka It should be good to go now. |
Hey @qinlz2 , please run |
Also
|
@rkapka Thanks, patched both. |
@qinlz2 Merged! Thanks a lot for this wonderful work! Looking forward to the next PR 😅 |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Implements the Altair spec logic defined in this version:
https://github.com/ethereum/consensus-specs/tree/208da34ac4e75337baf79adebf036ab595e39f15/specs/altair/light-client
It also implements the relevant endpoints specified at https://github.com/ethereum/beacon-APIs
#11815
Which issues(s) does this PR fix?
Partially addresses #11571
Other notes for review
This PR is a follow up to #11815 which was too big to be reviewed. This is the second PR in a series of 5 that I will be submitting sequentially.
The heavy lifting here was done by @qinlz2 and @7AC, so kudos to them.
Full series:
#12854 is the last PR in the series that contains all changes so you can go back and see the full picture while reviewing this one.