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

Ability to specify different 'fee recipients' for different validator accounts running on a client #4850

Closed
beetrootkid opened this issue Jan 11, 2022 · 9 comments · Fixed by #4894
Assignees

Comments

@beetrootkid
Copy link

Description

As a non-pooling, non-custodial staking provider, we need to be able to specify different coinbase accounts / fee recipients based on the validator account selected to propose a block.

It will be a 1:Many relationship between fee recipients and validator accounts.

Without this, it becomes very difficult for us to distribute the priority fees to the appropriate validator owner.

@ajsutton
Copy link
Contributor

Yep makes sense. We will likely always require a default fee recipient be specified as there are a bunch of ways to add validators and we can't always guarantee a fee recipient are specified as part of them but definitely makes sense to have a way to override that default per validator.

I'm thinking we probably need to support reading the mapping of validator public key to fee recipient address from a YAML file and we'd periodically check that file for changes so you don't need to restart to change the configuration. Mostly I imagine that's useful for when validators are added without restarting so you can also add an override for them.

@tbenr
Copy link
Contributor

tbenr commented Jan 13, 2022

@beetrootkid I'd like to design how things will work, so we can agree on requirements before start implementing it.

So, my first proposal could be a yaml file similar to

# file will be periodically read by teku, no restart will be required

# validator public key -> fee recipient ethereum address
fee_recipients:
  '0x12345...12345': '0x0000000000000000000000000000000000000001'
  '0x12345...54321': '0x0000000000000000000000000000000000000002'

# used for indices not present in fee_recipients
default_fee_recipient: "0x0000000000000000000000000000000000000003"

public keys are long but allows you to prepare the file even before knowing the assigned index for a new validator.

The corresponding CLI argument could be --validators-suggested-fee-recipient-address-file.

I don't if makes sense to you to have the current --validators-suggested-fee-recipient-address as a fallback default value so default_fee_recipient in file can become optional (or even absent). I think it depends if you may need to change default without restarting. If instead we want to reduce potential confusion, the two CLI params could become mutual-exclusive and the file MUST include default_fee_recipient.

@joaocenoura
Copy link

Hi guys, thanks for you input. Dropping some comments inline:

So, my first proposal could be a yaml file similar to <file_content>

The structure makes sense to us. But see last comment.
Would be possible to have a flag to configure the pooling frequency and/or disable pooling?

public keys are long but allows you to prepare the file even before knowing the assigned index for a new validator.

Perfect, it aligns with our backend too.

The corresponding CLI argument could be --validators-suggested-fee-recipient-address-file.

Would it be too much asking to load configuration via URL too? It would simplify a lot more on our side; complementing your proposal, was wondering something like --validators-suggested-fee-recipient-address-url=http://registry:8080/api/fee-recipient which would output same structure you mentioned above but in json.

If instead we want to reduce potential confusion, the two CLI params could become mutual-exclusive and the file MUST include default_fee_recipient.

Agree with this approach, doesn't leave room for any confusion.


Additionally, we wanted to specify the block producer URL per validator. It's similar requirement to the recipient fee and maybe you guys may want to explore some 'per-validator' configuration structure and bundle the fee recipient + block producer url in the same config file/url. Example based of yours:

# file will be periodically read by teku, no restart will be required

# validator public key -> fee recipient ethereum address + block proposer url + future params
'0x12345...12345':
  fee_recipient: '0x0000000000000000000000000000000000000001'
  proposer_url: 'http://flashhumans.net/mev-blocks
'0x12345...54321':
  fee_recipient: '0x0000000000000000000000000000000000000001'
  proposer_url: 'http://stay-poor-but-good-4-eth.net/non-mev-blocks
'0x12345...00001':
  fee_recipient: '0x0000000000000000000000000000000000000003'

# used for indices not present in fee_recipients
default_fee_recipient: "0x0000000000000000000000000000000000000003"
default_proposer_url: "http://local-besu"

I can create a different ticket if you wish. And either way will work for us.

@tbenr
Copy link
Contributor

tbenr commented Jan 16, 2022

As per out of band discussions, the extensibility of the structure is a good suggestion.

For the moment i would leave proposer_url (which could be block_producer_url) once the proposer\builder separation will be implemented.

Wrt URL support, we already have a transparent system for loading file locally or remotely via URL but ATM the file have to be a yaml file in both cases. I'm more lean to have the same param supporting local file or an URL.
So the current --validators-suggested-fee-recipient-address can become --validators-suggested-default-fee-recipient-address and --validators-suggested-fee-recipient-address can become a more general --validators-proposer-information that can hold a local file or an URL.

so to summarise:

  • --validators-proposer-information will support local or remote (URL) data. initially only yaml format with a structure similar to:
# file will be periodically read by teku, no restart will be required

# validator public key -> fee recipient ethereum address + future params
'0x12345...12345':
  suggested_fee_recipient: '0x0000000000000000000000000000000000000001'
'0x12345...00001':
  suggested_fee_recipient: '0x0000000000000000000000000000000000000003'

# default values used for validators not present in previous list
suggested_fee_recipient: "0x0000000000000000000000000000000000000004"
  • for each param in the file we have a corresponding CLI arg that will play the same role of yaml default values. In our case we will have --validators-suggested-fee-recipient-address.
  • --validators-proposer-information and --validators-<param> will be mutually exclusive.
  • --validators-proposer-information-refresh-frequency (optional) will allow specify the polling frequency (0 = disabled)

@joaocenoura WDYT?

cc: @ajsutton

@kanewallmann
Copy link

Adding Rocket Pool's support for this feature. It will be highly desirable for us to specify a fee recipient on a per validator account basis.

The suggested YAML-based solution would meet our requirements.

@joaocenoura
Copy link

Sounds good 👍 that would meet our requirements as well.

Minor note on the yaml requirement: we already have a rest service that teku validators use to pull configuration from. As of now, we only use --validators-external-signer-public-keys which will consume a json list from our rest service. But the new endpoint will require yaml. It would be desirable from our perspective (teku user) to deal with a single format. Still, if you guys need to go for yaml its ok on our end - just set the Accepts HTTP header accordingly. Later if you guys see value in supporting json you can provide a flag like --validators-proposer-information-format=json/yaml and flip the Accepts header accordingly.

@ajsutton
Copy link
Contributor

Given we've wound up using JSON for the remote signer validator keys, it probably makes sense to use JSON here too. I just defaulted to thinking YAML because we use YAML for our config files and this felt like a config file but we use JSON for our REST APIs and it can be argued this is going to most often be used by pointing it to an API.

If it causes confusion we can possibly support both but let's start with JSON and see what happens.

@joaocenoura
Copy link

That's perfect for us 👍 thanks @ajsutton

@tbenr
Copy link
Contributor

tbenr commented Feb 1, 2022

@joaocenoura @beetrootkid @kanewallmann we merged the first iteration on this feature.
please refer to #4894 description for details, there are minor differences from what was discussed here.

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 a pull request may close this issue.

5 participants