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

update validator subnet subscriptions and validator flow doc #68

Merged
merged 4 commits into from
Aug 18, 2020

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Aug 3, 2020

  • change subscribeToBeaconCommitteeSubnet to prepareBeaconCommitteeSubnet. This is called regardless of is_aggregator to ensure that the BN has a signal to find peers of that particular subnet
    • Note, many BN implementations attempt to just pro-actively have a diverse set of attnet peers, but having the ability to signal particular subscriptions provides flexibility in BN implementation and allows for BN to be extra certain to have the proper subscriptions.
  • update and cleanup validator flow doc

@djrtwo
Copy link
Contributor Author

djrtwo commented Aug 3, 2020

cc: @AgeManning for feedback

@mpetrunic
Copy link
Collaborator

cc @rolfyone for feedback if you are still implementing api 🙂

@djrtwo
Copy link
Contributor Author

djrtwo commented Aug 3, 2020

I think this is generally in line with what lighthouse currently does except that use a bool is_aggregator instead of sending the slot_signature for the BN to verifiy and decide what to do.

Pros: simple, especially when multiple vals in same committee
Cons: maybe misses some built in access control, but I'm not certain there is much value here. The BN+VC relationship in most instances is highly trusted. If it were a public endpoint, they'd probably have their own access control outside of the base API regardless

validator-flow.md Outdated Show resolved Hide resolved
validator-flow.md Outdated Show resolved Hide resolved
validator-flow.md Outdated Show resolved Hide resolved
@mcdee
Copy link
Contributor

mcdee commented Aug 4, 2020

Regarding is_aggregator Vs. slot_signature: having is_aggregator is simple for trusting beacon nodes, but assuming trust doesn't seem to be in the spirit of what is being built here. So why not send both? If a BN wants to trust the validator client it takes the value of is_aggregator without checking, if it doesn't want to trust the validator client it takes steps to confirm.

@mpetrunic
Copy link
Collaborator

Regarding is_aggregator Vs. slot_signature: having is_aggregator is simple for trusting beacon nodes, but assuming trust doesn't seem to be in the spirit of what is being built here. So why not send both? If a BN wants to trust the validator client it takes the value of is_aggregator without checking, if it doesn't want to trust the validator client it takes steps to confirm.

Just to confirm you are also in favour of sending slot_signature and letting BN decide if it should verify ir or not?

@mcdee
Copy link
Contributor

mcdee commented Aug 4, 2020

Just to confirm you are also in favour of sending slot_signature and letting BN decide if it should verify ir or not?

Sorry, yes (too early in the morning to string a coherent sentence together), those "steps" would be verify the signature, then do the math itself to see if the validator is an aggregator.

It would still be beholden on the validator client to send the "best" signature for a given (slot, committee) tuple, best being defined as aggregators being preferred over non-aggregators.

@AgeManning
Copy link
Contributor

Hey guys, sorry a bit late to this, have not been across the new APIs.

Just wanted to add some thoughts we had when implementing our first version of this API.

When getting the attester duties, we send the modulo used in the is_aggregator() function. The reason being, is that this value is dependant on TARGET_AGGREGATORS_PER_COMMITTEE. We didn't want the possibility that a VC gets initialised with different spec constants than the BN. If it did, the VC and BN would likely disagree on whether the validator is an aggregator or not. If we send the modulo they both will then always agree. I guess this is a point in favour of double-checking the aggregator condition on the VC and the BN side. However it may be nice to not have to run into this issue at all.

The reason we went with is_aggregator in the subscriptions is for simplicity and to avoid extra sig verification. A subscription from an aggregator, makes us subscribe to the subnet and collect and aggregate attestations on that subnet. If a VC lies to us, no harm really is done, we just throw away the attestations. I guess having the option is more general.

It would still be beholden on the validator client to send the "best" signature for a given (slot, committee) tuple, best being defined as aggregators being preferred over non-aggregators.

I'm not sure I understand this point. For us, all validators send subscriptions whether aggregators or not. They are of equal importance. Firstly, they give us a measure of how many active validators we have connected to the BN, so we know how many random subnets we should subscribe to (and add to our ENR) to form the backbone structure of the subnets. Secondly, non-aggregators still need to publish to a subnet, so the BN needs to know in advance to find peers for that subnet or ensure it has enough peers on that subnet to publish.

@mpetrunic
Copy link
Collaborator

mpetrunic commented Aug 5, 2020

We didn't want the possibility that a VC gets initialised with different spec constants than the BN

Since new api has spec endpoint, it's probably best for VC to query spec when initializing to ensure they are on the same page as BN.

@djrtwo
Copy link
Contributor Author

djrtwo commented Aug 10, 2020

Firstly, they give us a measure of how many active validators we have connected to the BN

Do vals from the VC that are in the same exact committee (slot/index) send multiple subscriptions for that same slot?

@djrtwo
Copy link
Contributor Author

djrtwo commented Aug 10, 2020

So why not send both?

In a non-trust relationship BN + many-VCs, I suspect (1) that there will be access control in a layer right outside of the API and (2) I suspect that such a BN is a super-full node and is going to be following all subnets and performing aggregation opportunistically anyway.

In such a case, the prepareBeaconCommitteeSubnet is a no-op on the BN regardless of is_aggregator or not.

@AgeManning
Copy link
Contributor

Firstly, they give us a measure of how many active validators we have connected to the BN

Do vals from the VC that are in the same exact committee (slot/index) send multiple subscriptions for that same slot?

For us yes. They could be in the same slot/index but one could be an aggregator and one not.
Even if they are both the same type of aggregator, we are currently using the subscriptions as an indicator of active validators connected to the beacon node in order to tell how many random subnets we should be subscribed to.

@mcdee
Copy link
Contributor

mcdee commented Aug 11, 2020

Firstly, they give us a measure of how many active validators we have connected to the BN, so we know how many random subnets we should subscribe to (and add to our ENR) to form the backbone structure of the subnets

This sounds like a proxy for sending an actual count of validators. At worst you'd end up receiving a list something like:

(slot 1, committee 0, is not an aggregator)
(slot 1, committee 0, is not an aggregator)
(slot 1, committee 0, is not an aggregator)
(slot 1, committee 0, is not an aggregator)
...

which is not only a waste of resources (especially if there is signature verification for each) but is counter-intuitive that this would have a different effect than sending a single (slot 1, committee 0, is not an aggregator).

I'm also curious as to how this would act if you had two separate validator clients attached to the same beacon node. Would the beacon node assume the sum of the number of subscriptions? And if so, how does it handle a re-org where each validator client would send a new list?

If knowing the number of validators attached to a beacon node is important we should look at codifying this directly, rather than it being a side-effect of another call.

Secondly, non-aggregators still need to publish to a subnet, so the BN needs to know in advance to find peers for that subnet or ensure it has enough peers on that subnet to publish.

An aggregator is a superset of the functionality of a non-aggregator, is it not? If the beacon node receives (slot 1, committee 0, is an aggregator) won't it need to do all of the things that it would do for slot 1, committee 0, is not an aggregator) plus its aggregation duties?

@AgeManning
Copy link
Contributor

AgeManning commented Aug 11, 2020

Yep I agree with most of the points here.

Our initial implementation was trying to use what was currently available in eth2-api's repo so that we wouldn't have too much trouble migrating to the proper standard. For this reason we didn't go off and implement new endpoints (such as a validator count) which would be handy, as you've pointed out.

Also we have not considered multiple validator clients per beacon node (in great detail) or multiple beacon nodes per validator. I was hoping these cases would be thought out more thoroughly in the next iteration of the APIs. But as it stands, we would use the summation of the subscriptions.

An aggregator is a superset of the functionality of a non-aggregator, is it not? If the beacon node receives (slot 1, committee 0, is an aggregator) won't it need to do all of the things that it would do for slot 1, committee 0, is not an aggregator) plus its aggregation duties?

Yes I think this is true. I may have to double check the implementation for any subtleties here, but I agree in principle we could just have a count for validators and then only send a subscription which favours aggregators over non-aggregators.

EDIT: After looking over the implementation

We track the validator index for each subscription. So we know which validators are connected/active.
If we opt for a validator count API to prevent possible duplicates subscriptions I'd imagine we'd need to send it once per epoch to ensure the validators are still alive.

As a side note, doing this decouples the validator subscriptions from the long-lived gossipsub subscriptions. In our current implementation, as a validator if you want your attestations counted you need to subscribe, in doing so, the BN will subscribe to a long-lived subnet helping stabilise the network. If we shift to a count, in principle users could filter the count requests or not send them at all, to get a leaner BN that doesn't subscribe to long-lived subnets and still perform the standard attestation duties. (This may not be a realistic concern, rather a side thought).

@mcdee
Copy link
Contributor

mcdee commented Aug 11, 2020

Great stuff. So we should consider what such an endpoint would look like. It sounds like some sort of "validator check-in" endpoint, which would be run on startup of the validator client and on epoch change, would be what we're looking for. A count of the number of validators in each state would probably be useful, do we need anything beyond that? Does sending the index of each validator in a relevant state buy us anything? Should we identify the validator client sending the request somehow?

@djrtwo
Copy link
Contributor Author

djrtwo commented Aug 17, 2020

updated to use is_aggregator. seeking final review @mpetrunic

@mpetrunic mpetrunic merged commit 6c227ec into master Aug 18, 2020
@mpetrunic mpetrunic deleted the prepare-subnets branch August 18, 2020 08:18
@AgeManning
Copy link
Contributor

Sorry, I was late to reply to this. Seems like its sorted. :)

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.

4 participants