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

Add /eth/v0/beacon/light_client/instant_update #270

Closed
wants to merge 2 commits into from

Conversation

zsfelfoldi
Copy link

@zsfelfoldi zsfelfoldi commented Nov 28, 2022

This PR adds a /eth/v0/beacon/light_client/instant_update feature that allows optimistic light clients to operate with only a few seconds of delay as opposed to /eth/v1/beacon/light_client/optimistic_update which introduces a full block of delay. Security is not affected by the latest sync committee signature being canonized in the next block and therefore it is possible for the serving node to always listen to the sync committee signature gossip (just like it does when it is on the sync committee) and provide sync aggregates for the current head as soon as signatures are available.
The endpoint receives a block_root parameter and when asked about the current head, assuming that the client already has the header, returns only the current best aggregate. If an older block_root is specified then the most recent head header is also returned. This makes sense because the final best signature for a given block can typically be determined when its child appears, in which case the polling client is notified that the result can be considered final and the instant update for the returned new head should be polled next time.
When implemented as an eventstream, the streaming updates should refer to the block_root of the head known at the time when the last update was sent, meaning that whenever a new block appears the best signature for its parent and the header of the new head should be sent. Until a new block arrives, new updates containing only best_sync_aggregate and signature_slot are sent as soon as more signatures are available. A short (<100ms) delay is acceptable in order to avoid sending up to 512 updates per slot as the individual signatures arrive.

Comment on lines +5 to +13
Requests the best [`LightClientInstantUpdate`](../../../types/altair/light_client.yaml#/Altair/LightClientInstantUpdate) known by the server for the given block header.
Depending on the `Accept` header it can be returned either as JSON or SSZ-serialized bytes.

Servers providing this endpoint SHOULD always listen to the sync committee signature gossip and collect individual signatures. On request the best BLS
signature is aggregated and returned for the specified (recent) block header. When a new block appears, the previously known best sync aggregate for its
parent is compared against the sync aggregate found in the new block and replaced if the canonical one is better. Best sync aggregates are retained and
served for the 16 most recent slots.
Note that since always listening to the signature gossip costs some resources, it is acceptable to only start listening once this endpoint is called
and stop listening if it is not called for an extended period of time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So there's SyncCommitteeMessage and SyncCommitteeContribution.. can we line up what's being asked for here so that its better undesrtood?

It does concern me listening to all gossip on everything running the beacon-api, especially for home stakers etc it's overhead they shouldn't require..
Aggregates will be available before a block is produced anyway, I'm wondering why we'd be pushing to listen to individual messages?

Copy link
Author

Choose a reason for hiding this comment

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

Actually we don't necessarily have to specify here how to obtain the best signatures as it does not affect the way the endpoint is used, from the consumer perspective it just affects the delays. Listening to all individual sync committee sigs is indeed expensive and it was my mistake to specify it as the default option. Listening to the four subnet aggregates is definitely better as a default suggestion. I changed the description accordingly. I see no reason to specifically forbid beacon nodes from listening to all sigs but it should not be a mandatory thing to implement (and even if implemented by a certain consensus client it should probably not be the default option). I'd even be fine with making this whole feature optional to implement, I just want to come up with a spec that makes sense so those who are interested in implementing it can stay compatible with each other.

description: A new `LightClientInstantUpdate` has been generated (the node's current head and/or the best sync aggregate for the previously known head has been updated)
value: |
event: light_client_instant_update
data: {"version":"phase0", "data": {"new_head_header": {"slot":"1", "proposer_index":"1", "parent_root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2", "state_root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2", "body_root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"}, "best_sync_aggregate": {"sync_committee_bits":"0x01", "sync_committee_signature":"0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505"}, "signature_slot":"1"}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

version phase0 isn't valid for sync committee...

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, fixed. It was also wrong in the other update examples, fixed those too.

@@ -100,6 +100,8 @@ paths:
$ref: "./apis/beacon/light_client/finality_update.yaml"
/eth/v1/beacon/light_client/optimistic_update:
$ref: "./apis/beacon/light_client/optimistic_update.yaml"
/eth/v0/beacon/light_client/instant_update/{block_root}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer we keep this as v1 and mark experimental in the tags for the endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines +72 to +80
LightClientInstantUpdate:
type: object
properties:
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probably add the required attrbiutes list here if we're not requiring all fields - sounds like new_head_header is optional...

Also is new_head_header in a spec somewhere? I'm not a huge fan of that name...

Copy link
Author

Choose a reason for hiding this comment

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

Renamed to new_head, the description and type reference should tell the rest. Also added the required attrbiutes list.

@rolfyone
Copy link
Collaborator

I may be missing something but event stream may be problematic on this endpoint if you're expecting it refers to the last block root of a specific clients call, which is what it sounds like... If that's the intent, maybe leave it off the stream because it will be very difficult to correlate, especially if there's multiple LC connecting to one BN.
This sounds like a lot of overhead potentially for a BN. Will continue to watch, but I'm not sure it's really ideal in its current state...
Would be interested if I could get @michaelsproul and @rkapka to take a look and gather some extra thoughts...

@dapplion
Copy link
Collaborator

dapplion commented Dec 5, 2022

I believe this endpoint is necessary since the artificial full slot delay on getLightClientOptimisticUpdate() may not be acceptable for some consumers. Are all clients required to implement all of this APIs in every since configuration they are run? If not, this enpoint could:

  • Error unless a --subscribe-sync-committee-subnets flag is set
  • Return the best update known from any source, since the beacon node may be subscribed to some subnets, else wait for global topic aggregates

@zsfelfoldi
Copy link
Author

zsfelfoldi commented Dec 14, 2022

@rolfyone Thank you for the detailed review and sorry for responding slowly. So I think the main issue is whether it makes sense to subscribe to every individual SyncCommitteeMessage. You raised a valid point that probably it shouldn't be in the spec that this is always required. Getting signatures quicker than a full slot delay is also possible by just listening to the SyncCommitteeContribution of each subnet which is a lot cheaper (though probably a bit slower than the first version). Whether the extra networking cost is justified might depend on the use case. My main use case is the Geth light client (the WIP version 5 of the LES protocol) which can propagate the best sync committee aggregates on its own p2p network. Dedicated LES server setups might be interested in getting the best signatures ASAP, even at a higher cost. So I guess the spec could define that the endpoint provides the best available sync committee aggregate, which can be obtained either way. Consensus clients could implement either, or both, selectable by a command line option. Would you find this better?
We could even allow disabling this feature altogether, though I'm not sure it's necessary if it's only activated when the endpoint is actually being used. Stakers obviously don't need to waste their bandwidth with this, they should definitely not open up their API to the public and therefore no one will call this endpoint on their setups.

@zsfelfoldi
Copy link
Author

About the event stream: maybe the spec wasn't clear enough here, I should fix it. So in the polling version the client tells in the parameter which is the last head it knows about, the BN returns the best known signature for that head and also a new head if it's different from what the client knows about. In case of the event stream the BN pushes a continuous stream of messages and it can always assume that the last block the subscriber (any subscriber) knows about is the new block contained in its own last message that contained a new block. So it can always send the same message to every subscriber.

@arnetheduck
Copy link
Contributor

I think for a general-purpose API, it might be better to keep this focused on the the aggregates instead - ie the individual messages are quite numerous and expensive to maintain a subscription over.

I also think that propagating this over a the LES network is a crutch at best - ie at that point, it would make for a more resilient network if consumers simply subscribed and contributed to the libp2p gossip directly instead of developing a REST API that all CL:s must implement then feeding that into LES - the duplicated LES network over devp2p introduces fragmentation for this data that is already scarcely distributed, and it's problematic because clients participating in that network will have weaker protections than what a libp2p participant can achieve.

@zsfelfoldi
Copy link
Author

@arnetheduck see my comment above regarding the aggregates vs individual messages.
About propagating signatures over LES: this is what I am doing currently in order to come out with an integrated light client solution in Geth ASAP, but feel free to consider it a temporary solution or a "crutch" if you like :) Actually it does not really matter whether we are talking about LES or not, the interesting question is what functionalities we want to make possible. Getting signatures for consensus blocks with sufficient majority ASAP is something pretty basic IMHO. Some use cases might want to rely on the REST API entirely, they could also make good use of it. A future libp2p subnet specifically targeted for light clients could also propagate the best complete sync committee aggregates. If someone on this network is willing to listen to everything and aggregate ASAP then fine, if not then also fine, it's just that the signatures will be globally available a few seconds later.
Right now both the consensus and execution side has a p2p protocol and an http/ws API, and I've heard different ideas about how light clients should access their data and how protocols/APIs should be integrated. At this point I'd prefer having multiple options and seeing what works best in the end. Now I'm channeling consensus light client data through LES but I'd also support doing it the other way around.

@rolfyone
Copy link
Collaborator

I've posted for comments on this PR, will circle back tomorrow and see where we're at. In the meantime, if you have a second to clean up that conflict @zsfelfoldi that'd save me a task :)

@mcdee
Copy link
Contributor

mcdee commented Jan 12, 2023

A couple of thoughts after a quick read-through:

  • if the delay here is a few seconds then it isn't instant. This may seem like semantics, but the next time someone comes along with a method that allows even faster response what would it be called? It may be better to name it according to the update method than any property (incorrect or not) of its performance
  • the conditions for generating update events on the eventstream aren't very well defined. Is there a base % of the sync committee members whose signatures need to be present before sending something? Should incremental updates be based on additional weight in the aggregate, or time based? Is there a point in the aggregate where there is considered sufficient weight, such that further updates don't need to be sent? I think that it makes sense for these to be standard across implementations, so that a light client can connect to any beacon node and have a fair idea of what it is going to receive

@dapplion
Copy link
Collaborator

dapplion commented Jan 14, 2023

@mcdee the conditions for generating update events on the eventstream aren't very well defined

True, I do not believe this "instant update" should be emitted to events.

instant_update could be implemented in the existing optimistic update route, customizable with some flag on the beacon node. Or by passing an extra parameter on the route such as source: block | aggregates | signatures. With the parameter strategy, receiving a request for source: signatures would trigger the beacon node to subscribe to all sync subnets. This capability is already exposed on the API so I don't see a security / performance concern.

@arnetheduck ie the individual messages are quite numerous and expensive to maintain a subscription over.

Above approach would only force the beacon node to do extra work if its consumer requires it. Also 512 sync messages / 12 seconds is not that much compared to attestations.

@zsfelfoldi
Copy link
Author

@mcdee @dapplion I removed the event stream option for now, it's indeed maybe just unnecessary extra complexity. If we figure out later that it's worth having for some reason, I can open a separate PR and also specify it more exactly.

@zsfelfoldi
Copy link
Author

@rolfyone fixed the conflicts :)

@zsfelfoldi
Copy link
Author

@mcdee about the naming: the spec intentionally does not specify how the signature aggregate is obtained (see discussion above), the definition of this endpoint is that it gives the best available update (highest participation signature aggregate) at any moment without waiting for it to become canonical. So it's instant in the sense that the API makes it available as soon as it's available to the underlying CL client. Maybe it's possible to choose a better name though... it could be best_update as it gives the best possible update at any moment. Or non_canonical_update would tell something meaningful about it too (maybe a bit long though). Any other suggestions?

@arnetheduck
Copy link
Contributor

Per out-of-band discussion, similar information is available from https://ethereum.github.io/beacon-APIs/#/Validator/produceSyncCommitteeContribution

@Falehfale Falehfale mentioned this pull request Sep 28, 2023
Closed
@rolfyone
Copy link
Collaborator

Closing as stale, we can re-open if/when required.

@rolfyone rolfyone closed this Jan 22, 2024
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.

6 participants