-
Notifications
You must be signed in to change notification settings - Fork 176
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
Validator api - spec 0.9.2 #12
Comments
I would suggest following schema:
We could keep an old one as well if it would exist:
|
How about GET `/eth/validator/wire_attestations/{epoch}/{commiteeIndex} ?
Is there a reason you would like to keep that one? Is it suppose to subscribe to committee attestation topics? Seems like it's way too early to keep backward compatibility 😄 |
Totally agreed! Hence, no need to keep this old fashioned duties request.
I think it's better to move them both into a query string:
|
@mkalinin about that wire attestations, what was your intention with epoch param? Wouldn't it make more sense to replace epoch with attestationDataRoot(hashTreeRoot(attestation.data)? |
The intention was not to mix attestations produced by committees with same index in a different epochs. Validators of the same committee may produce attestations with different data, e.g. different view to a fork, and aggregator have to produce an aggregate for each of them. |
The attestation_subnets have a validation condition that only unaggregated attestations are sent. Similarly, when doing aggregation by the v-client, I would expect |
otherwise, these look like good/valid additions to the api |
We've had problems here where we quickly run out of space in the URL when there are many pubkeys (definitely fails when there's 100 keys). To resolve this, we made a
I'm a little concerned a mixing the concerns of "I want to know duties" with "I want to aggregate attestations". For example, I suspect that block explorers and other UIs might want to read this data without causing an affect in the node. Additionally, this is a I'm also unsure of how the beacon node knows if it needs to subscribe to the topic, because it doesn't have the I would suggest the following endpoint:
This would cause the beacon node to start listening on the aggregation pubsub topics:
I would have thought we can keep more logic inside the beacon node (and out of the validator client) with the following endpoint (it's basically your
This way the beacon node can perform the aggregation itself and then publish the proof. Note: the beacon node should already know the |
I was thinking about putting some sane limit here like 10 pubkeys, I don't think arbitrary number of pubkeys should be allowed (allows you to circumvent api rate limiting etc)
That's a good point, I agree.
You would need slot, to verify signature? Maybe something like:
That was my initial idea, but under assumptions that validator can subscribe to more than one BN, validator won't be able to aggregate all attestations from all BNs unless first BN aggregated like first N/2 and second BN aggregated last N/2 attestations. @paulhauner ^^ |
A good explanation on URL length limit is given on Stackoverflow: https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers Browsers does not support URLs longer than ~2000 characters in their address bar. While servers are not restricted in supporting URLs longer than that, this parameter should be configurable. Bypassing this limitation by using
Agreed.
Passing IMO, subscription should be automatically expired by the node at the end of the slot related to the committee with given index and epoch. And all resources should be released on the expiration procedure. In more sophisticated setups when a VC is connected to several BNs, how this aggregate command will look like? I assume that these BNs have strong connections with each other and quickly shares all the bits of information they have with their neighbors, thus there will be no difference from VC point of view which node is used as a source of an aggregate. However, VC will have to send subscription requests to all of them. |
Seems reasonable, but I feel this should be done on a client-by-client basis. I'd be tempted to wait until it's clear that too many pubkeys in one request is a problem for public APIs before we start applying restrictions.
This is a good point @mkalinin. Perhaps we should keep the
Agreed, I think @mpetrunic's suggestion to add the
I totally agree.
It seems to me that if we include
My thoughts were the the VC should only really try to elect one BN as the aggregator and try to use that BN as the publisher. If it does happen to trigger a couple of BNs (perhaps the first one goes offline for a little bit), then it's just a little more traffic on the pubsub network with no significant cost. |
I would still opt to allow VC to collect attestations from multiple BN and publish aggregate on whichever node he wan't. It would allow us to implement more robust and performant VC. We could have optional endpoint for lightweight VC, where BN would aggregate and publish on behalf of validator. |
Agree, this would work if a beacon node is responsible for aggregation and further propagation of computed aggregate.
Don't you think it's an overhead for VC instance? |
That's why I'm proposing two endpoints, one for more robust and resistant VC and other for people who would like to run VC on RPI or whatever. There is also a issue of evil BN because you are giving it signature so it can publish invalid aggregation without your knowledge. There won't be thousands of attestations to aggregate and you aren't doing it often so I don't think this would be issue even for weak VC. |
Presently we do not store un-aggregated attestations at all; we aggregate and then drop the original. In order to implement this API we'd need to make data-structure changes for the sole purpose of supporting this API that we do not have an immediate need for. I think this scenario is valid but too perhaps too advanced to be placed in the API at this stage. I think we should be targeting "all clients have implemented a minimal API" instead of "all clients have implemented different parts of a wide-reaching API". If several clients do intend to implement the multi-BN aggregation scheme immediately then I'm happy to be wrong here :) |
I would propose new validator API as current API is causing troubles in this aggregation strategy.
Problems are:
duties
for current epoch to check if we are proposer and for future epoch so we can subscribe to attestations (call for proposer checking will subscribe us (beacon node) to unnecessary topics)I would propose following:
/eth/validator/duties/proposer/{epoch}
- returns{slot: publicKey}
mapping for every slot in given epoch/eth/validator/duties/attester/{epoch}?publicKeys=public1, public2
- returns[{publicKey, slot, committeeIndex}]
for given publicKeys and subscribes to topics related to returned commiteeIndices/eth/validator/attestations/{commiteeIndex}
- returns all attestations collected from that committee index topic/eth/validator/aggregate
Other apis will remain the same (attestation type needs change).
After we have agreed api, I'll open PR on repo.
The text was updated successfully, but these errors were encountered: