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

registerValidator to use array of entries #10

Merged
merged 1 commit into from
May 23, 2022

Conversation

metachris
Copy link
Contributor

@metachris metachris commented May 12, 2022

Update registerValidator to accept an array of entries (in line with the beacon-API: https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/register_validator.yaml)

Using an array would be particularly useful if there's a lot of validators using the same CL/mev-boost instance. Since mev-boost regularly resends the validator registration to the relays/builders, it would only need one call per relay instead of potentially thousands (one per validator).

@metachris metachris changed the title registerValidator send array of entries registerValidator to use array of entries May 12, 2022
$ref: "../../builder-oapi.yaml#/components/schemas/SignedValidatorRegistration"
type: array
items:
$ref: "../../builder-oapi.yaml#/components/schemas/SignedValidatorRegistration"
responses:
"200":
description: Success response.
Copy link
Member

Choose a reason for hiding this comment

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

think we need to clarify the error semantics here, see #8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's true, and I think that's a different PR, independent of the payload this call is using. (Whether an item or an array is sent wouldn't impact the error)

@djrtwo
Copy link
Collaborator

djrtwo commented May 12, 2022

I'm not sure I see the value in the array implementation here.

In the event that some builder endpoints end up being gossiped, I have a hard time seeing that an array is the proper choice. Both from a privacy (simply tying these together as one) as well as load and other complexities.

E.g. you'll need anti-dos mechanisms for such a gossip path and putting N of these messages together means you have to say things like "none of the validators in the list of registrations has submit a registration in the past T seconds".

EDIT: I think it's nice to have these methods be very mappable to future structures. The ability to map 1:1 to gossip is a nice-to-have but in the event that we add some sort of privacy preserving scheme, there's not way we'll be bundling these registrations in a single message regardless of the transport

@metachris
Copy link
Contributor Author

metachris commented May 12, 2022

I don't think the array here is a must-have, just thought it would be more elegant in case of thousands of validators, where all those requests could be just a single API call to the relay. Also mev-boost regularly resends all known recent registrations, that would also be nice to do in one call, rather than one per validator.

If this negatively impacts a future p2p design then let's not do it, I didn't think the REST API semantics would have an effect on that.

@metachris
Copy link
Contributor Author

An additional benefit is that the BN can just proxy the request from the validator (which already comes as an array), instead of needing to split it up into multiple individual calls.

@djrtwo
Copy link
Collaborator

djrtwo commented May 20, 2022

I'm fine with his as long as this design does not creep into future gossip designs (if/when gossip replaces some relay functionality).

Additionally, I would encourage relays to not blindly forward this request but instead to allow for queries that do not explicitly tie validators togthr as they wre sent to the relay

@ralexstokes
Copy link
Member

Yeah I think we can move ahead with this PR and possibly add a note that it is highly encouraged for relays to turn a batch of N into N batches of 1 to help w/ validator privacy

@metachris
Copy link
Contributor Author

Yeah I think we can move ahead with this PR and possibly add a note that it is highly encouraged for relays to turn a batch of N into N batches of 1 to help w/ validator privacy

Makes sense. If a validator cares very much about this privacy, it could also already send the registrations as single items to the beacon node? 🤔

@ralexstokes
Copy link
Member

ralexstokes commented May 22, 2022

The privacy concern here is a relay/builder learning about IPs of validating nodes. For the purposes of the security risk, the beacon node and validator client are essentially one entity as the beacon node will be the entrypoint for the validator client onto the p2p network.

If I use the builder network from the same IP I send proposals from, then any agent (under the current model, the relays) can make a mapping of validator identity to this IP (c.f. registrations have the pubkey in them). With this mapping, I learn which nodes I need to DoS to add a liveness failure to the chain. Moreover, I can be incentivized to pull off an attack of this sort given a block with very high MEV (where my cost to execute a re-org is subsidized by the MEV profit in the block).

Leaking this information to relays is bad not only because the relays could be malicious (I think we can assume they won't be), but they themselves could accidentally leak the information ("honeypot" scenario) or become a target for attack. Mercenary relays could also simply sell the data to the highest bidder.

The immediate stop-gap solution is to run your beacon node behind a VPN or similar setup where you can periodically rotate your IP. But even this solution has higher complexity than just "staking out of the box" so harms decentralization. Ideally we can evolve the builder specs in a direction where it is much harder to gather this information in the first place.

So in this context, my comment above helps but is probably not the lowest hanging fruit as I imagine most relay <-> builder architectures will involve a hop where the IP linkage becomes broken and in that case it matters less that relays break up responses (unless we want to start talking about network-wide timing attacks etc. :) )

If we want to improve the situation beyond demanding validators use a VPN, then we can consider a gossip layer for registrations which functions as a mixnet in this scenario. I don't think we will be able to design/build/test this by the time the Merge drops, but it is something worth keeping in mind for sure.

@metachris
Copy link
Contributor Author

Thanks for the explanation. Definitely something important to think about and plan for.

Do you see an impact on this whether the registerValidator call sends single items vs an array?

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

This has had some time to be reviewed by various parties and it seems that there is consensus around this change for the builder API, but this batching mechanism should not creep into future gossip based designs, nor be passed naively to builders from relays. Use of this method is delicate, because at best, it is makes it easy to tie all validators in a batch together. At worse, it ties all the validators together to a single IP.

@lightclient lightclient merged commit e602513 into ethereum:main May 23, 2022
@ralexstokes
Copy link
Member

Do you see an impact on this whether the registerValidator call sends single items vs an array?

I think we are fine for now as far as the API goes, if we hit a blocker we can always update it

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