ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
335 stars 169 forks source link

Deprecate prepareBeaconProposer, consolidate on registerValidator. #435

Open jshufro opened 7 months ago

jshufro commented 7 months ago

I truly believe prepareBeaconProposer is strictly more cumbersome than registerValidator.

1) It's vulnerable to attack 2) Validator Clients have inherent knowledge of the private keys of their attached Validators, but not the indices, so the first thing many of them do is request, by public key, the index of each associated validator. This can lead to problems at worst, but even with the kinks get ironed out, it represents unnecessary overhead during validator client startup.

Now that builder_boost_factor is present, control over whether to use a builder block or a paired node block is satisfied at the time produceBlockV3 is called, and consolidating prepareBeaconProposer and registerValidator will not leave the BN wondering which block to return to each attached validator client.

ajsutton commented 7 months ago

As the author of that blog post about the attack, it's not something I would be worried about and if anything I'd consider it a feature. A validator should not be using a public beacon node to perform its duties as that means it isn't actually validating the chain (it's just trusting the public node provider).

Otherwise I haven't thought enough about the proposed change to say if it's a good idea or not - just wanted to clarify that this isn't required to fix an actual security issue.

mcdee commented 7 months ago

Registering validators registers all validators known by the validator client and is a pass-through to relays, whereas prepare proposer is meant to be limited to just those validators that are scheduled to propose within the next epoch and more focused on local block production.

There is an argument for using public key rather than index in the registration, although index is used in a number of other places in the API so it isn't a unique requirement for this endpoint. But I think some sort of fast-path notification to the beacon node that the validator client has a validator that is expecting to request a block in the near future is still useful, as it avoids the beacon node having to wade through potentially tens of thousands of registrations to see if one of them is scheduled to propose (although client teams can weigh in here if they think that the overhead isn't relevant).

Now that https://github.com/ethereum/beacon-APIs/pull/386 is present, control over whether to use a builder block or a paired node block is satisfied at the time produceBlockV3 is called, and consolidating prepareBeaconProposer and registerValidator will not leave the BN wondering which block to return to each attached validator client.

Note that local payload generation is usually started ahead of the proposing slot, to allow the execution node a chance to build a good payload and provide same when requested with minimal delay. And the underlying code will usually (outside of specific values of the boost factor) talk to both relays and execution node for payloads simultaneously, so I don't think that this point is relevant for what is being discussed.

jshufro commented 7 months ago

As the author of that blog post about the attack, it's not something I would be worried about and if anything I'd consider it a feature. A validator should not be using a public beacon node to perform its duties as that means it isn't actually validating the chain (it's just trusting the public node provider).

Hi Adrian, I have been wondering when we'd run into each other. I reference your blog post often.

Evaluating it as a feature, it also fails the sniff test in my opinion. The trust assumption with a public beacon node with prepareBeaconProposer is that not only are the maintainers benevolent but that so is every single other participant- but this can easily be reduced by the maintainers to eliminate the need to trust all the other participants. I helped build such a public beacon node.. Further, most "public beacon nodes" are public by accident- node operators spin up a VPS or baremetal and use docker to manage their clients and ufw to secure the node, but the iptables entries for each bypass one another. A quick search on shodan shows thousands of nodes with unsecured API ports.

So, it doesn't accomplish the prevention of public beacon nodes, and it also hurts less-than-professional node operators. Requiring a signature from the validator would at least protect the latter group.

Proof of custody, if well implemented, will resolve the public beacon node problem. prepareBeaconProposer is largely irrelevant at this point.


Registering validators registers all validators known by the validator client and is a pass-through to relays, whereas prepare proposer is meant to be limited to just those validators that are scheduled to propose within the next epoch and more focused on local block production.

Interesting, if this is the original intent. I have observed every single client send prepareBeaconProposer and registerValidator with the same vector sizes at the same cadence. I have graphs, if needed, to back this up.

Since both are called with every attached validator, every epoch (generally), it doesn't make sense to me that they can't serve the same purpose.

I don't think that this point is relevant for what is being discussed.

It is perhaps unrelated. I only mean that in the past, eligibility to request a blinded block was predicated on a validator's presence in registerValidators, and eligibility to request a block built by the paired execution client was predicated on a validator's presence in prepareBeaconProposer, but that need not be true. Both can be prepared by the beacon node based on registerValidator alone, called unconditionally by the validator client, and the validator client can control which one is signed and published.


Ultimately this suggestion serves two purposes:

  1. Improve the security of node operators who fail to secure their beacon node API port
  2. Simplify the clients

I'd like to hear from the client teams as to whether they think it accomplishes 2. I can tell you that 1 is a given, though.

nflaig commented 7 months ago

send prepareBeaconProposer and registerValidator with the same vector sizes at the same cadence. I have graphs, if needed, to back this up.

This is correct, the validator client doesn't know the proposers beforehand so it has to send them all and the beacon node will prepare a payload if there is an scheduled proposal.

I'd like to hear from the client teams

Although we still need to get the validator status at startup for various other reasons, I think consolidating those two APIs is a strict improvement in terms of security and also avoids overhead of calling two different APIs. The only overhead is that we would have to verify signatures but since this API is not that time sensitive I don't think it's an issue.

Just a note that we can't just deprecate prepareBeaconProposers and use the current registerValidator API. This requires to implement a registerValidatorV2 API which consolidates the two APIs (similar to block v3), otherwise pairing two different clients might result in proposers not being prepared at all.

Maybe something to enable at Electra fork epoch?

jshufro commented 7 months ago

I spoke out-of-band with @tersec on the nimbus discord to get his perspective, and hopefully this summary is representative:

I'm of a mind to day that proxied beacon nodes bear the responsibility to broadcast prepareBeaconProposer requests to all upstreams, and that the VCs should not be sending prepareBeaconProposer once per slot. On the other hand, I can see why they do- a node operator who is not savvy enough to secure their beacon api port certainly is not savvy enough to handle a complicated proxying setup.

tersec commented 7 months ago

Thank you for summarizing our discussion here; yes, that's accurate from my perspective.

michaelsproul commented 7 months ago

Speaking as a Lighthouse dev, I am in favour of combining them. I think it's a good simplification. I suspect that the current per-slot schedule for prepareBeaconProposer is excessive. It serves to inform the BN of the fee recipient, which would only be unknown if the BN had just restarted and lost all its state.

We could start experimenting prior to a coordinated release (e.g. Electra) by updating BNs to always draw fee recipients from registerValidator calls, and adding a flag to VCs to stop sending prepareBeaconProposer. Then the fork would just be a matter of 1) Knowing that the BN is consuming the fee recipient data and 2) Enabling the flag to turn off prepareBeaconProposer. We could also bump the version number on registerValidator for good measure, but we would not need to.

jshufro commented 7 months ago

It serves to inform the BN of the fee recipient, which would only be unknown if the BN had just restarted and lost all its state.

This can also be mitigated by holding a tcp connection open (or similar), and sending an adhoc request each time it's reestablished. Nimbus does something along these lines, from what I can tell through observation.