Remove fee_recipient_file functionality

[ethDreamer]

The original purpose of the fee_recipient_file was to allow the user to change a validator's fee_recipient on the fly (that is, without restarting the validator client).

But there is now a new validator keymanager API endpoint for doing this. See:

• Implement feerecipient API for keymanager (VC) #3173
• [Merged by Bors] - Implement feerecipient API for keymanager #3213

In order to implement this API endpoint, we must persist changes to the fee_recipient made via the API in the validator_definitions.yml file. This will automatically override the settings in the fee_recipient_file.

So this endpoint not only makes the fee_recipient_file unnecessary, it makes it useless if the API endpoint is used.

Furthermore, in the future, there will be additional configuration options that can be set on a per-validator basis (like the gas_limit). There's no point in having the fee_recipient be the only option that is set via a file, nor do we want to implement a new file type for each setting as it would become tedious to manage, especially when there are these API endpoints to change them.

We should remove the --fee_recipient_file functionality and manage these changes entirely via API endpoints. For options that don't yet have an official endpoint we should utilize a PATCH endpoint.

We should remove this before the merge as most people will not have added it to their workflows yet.

[ricardolyn]

Instead of having an API that external actors need to push data to it, why isn't implemented an argument that allows passing an endpoint to Lighthouse where it can pull the whole data when necessary? Similar to what Teku does with this argument.

[michaelsproul]

@ricardolyn That's a good design too, but the "push-based" API is what is being standardised across clients (incl Teku): https://ethereum.github.io/keymanager-APIs/

[ricardolyn]

We should remove this before the merge as most people will not have added it to their workflows yet.

We have it implemented in our system already.

"push-based" API is what is being standardised across clients (incl Teku)

Yes. my main point is around the deprecation of file support. With the file, we can change fee recipients in bulk. It will be good to keep the functionality while there is no alternative like Teku.

When using the key manager API, if we have to update several validators simultaneously (imagine 1000), will this raise any issues on the lighthouse?

thank you 👍

[michaelsproul]

When using the key manager API, if we have to update several validators simultaneously (imagine 1000), will this raise any issues on the lighthouse?

@ricardolyn I imagine this wouldn't create any issues. Each request would take just 10-100 milliseconds to run and the whole operation should complete in around a minute

Do you envisage updating 1000s of fee recipients at a time to different values? Or would you likely be updating them all to the same value? Would you be well-served by a method to update the default fee recipient, as @realbigsean describes here: #3213 (comment) ?

[mrabino1]

From my lens, I very much wish there was a default_fee_recipient in the validator_definitions.yaml ... would make life easier for sure.. /mr

[ricardolyn]

@michaelsproul, we provide a service to customers, and multiple customer stakes' are allocated through our validators.

We could have 1000 fee recipients updated at the same time to different values or the same values. It all depends on how our customers update their records (for example, they can update their account to 1000 validators at once to the same address).

The default fee recipient is not used to manage the customer's fee recipient. Validators are shared, so the default one is a fallback.

Regarding PR #3213, we already have the default_fee_recipient in the fee recipients file. I don't see any difference moving to the validator_definitions.yaml when there is an argument for it. I only see an advantage to someone that only use one fee recipient for all keys in the validator, which is not our case. Moreover, we don't often change the default one (it's a configured value, not changed dynamically like the mapping public_key->fee_recipient_address.

[michaelsproul]

Thanks for the feedback @ricardolyn. In the interest of making progress with our fee recipient API we're going to continue with removing the fee recipient file support. We'll revisit the performance of the API if it proves troublesome in practice, and may add a pull-based API like Teku in future. If you could please migrate your tools to use the HTTP API before upgrading to our next release that would be greatly appreciated 🙏 Thanks!

(slightly more context here: #3213 (review))

[mrabino1]

for sanity, support for the fee recipient embedded in validator_definitions.yml is still part of the plan.. yes?

[michaelsproul]

Yep, it just won't be part of that PR we're merging now. For now the default can be set with the CLI flag --suggested-fee-recipient on the VC.

[ricardolyn]

@michaelsproul why do you need to remove the file support to make progress with the API?

[michaelsproul]

@ricardolyn The PR implementing the API removed the file support for the reasons described in the OP, i.e. to keep things simple.

We won't add it back, but we may add a pull-based API like Teku's if the performance of the standard API is proved to be inadequate. If you'd like to expedite the pull-based API you could help us benchmark the current API 🙏

[ricardolyn]

@michaelsproul thanks. It would be best if you looked to flashbots/mev-boost#154, which will be implemented on top of the Teku push-based approach. Will lighthouse support the per account relay config in that issue?

[realbigsean]

We only support a single builder/relay connection. To use multiple, you can connect to mev-boost, and there you can configure the relay-validator mapping

[michaelsproul]

@ricardolyn How quickly do you need your updates applied? I noticed that with a pull-based approach we'd need to either reload a large (multi MB) file every time we propose or space the updates out (which seems to be what Teku does). I've raised this on the mev-boost issue too: flashbots/mev-boost#154 (comment)

[ricardolyn]

@michaelsproul we need the updates before the block proposers are selected. I believe Teku spaces the updates out and pulls the data when a validator from the ones it manages is chosen to propose a block. I think this solution should be enough.

[michaelsproul]

@ricardolyn Based on a conversation with the Teku devs I think they're not re-reading the config file on every block proposal, but just doing it once per epoch.

If we do adopt the proposer config format in Lighthouse we'd likely do it with 2 separate flags --proposer-config and --proposer-config-url. This would allow us to only re-read the file on a IO notification from the OS (using something like https://github.com/notify-rs/notify). Unfortunately for the HTTP API we're out of luck and will just have to poll.

Are you using files or the HTTP API in your Teku setup @ricardolyn?

[ricardolyn]

just doing it once per epoch.

correct. I checked the logs yesterday, and it is once per epoch. I had the wrong assumption.

Are you using files or the HTTP API in your Teku setup @ricardolyn?

HTTP API.

[ricardolyn]

@michaelsproul do you know if this is something you plan to implement soon? as we are near the Goerli+Prater merge, and we need to quickly decide if we swap to the KeyManager API before it (not too long now), which was not our most attractive option. thank you 👍

[michaelsproul]

Hey @ricardolyn, it's probably not something we'll get to before the Goerli merge, we've got a lot of other high prio changes we need to ship. I think it would be good to use the keymanager API, and even the Lighthouse API for gas limits (#3342) until the gas limit API I proposed is accepted (ethereum/keymanager-APIs#39).