[Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) #2924

pk910 · 2022-01-17T09:04:36Z

Issue Addressed

#2883

Proposed Changes

Added suggested-fee-recipient & suggested-fee-recipient-file flags to validator client (similar to graffiti / graffiti-file implementation).
Added proposer preparation service to VC, which sends the fee-recipient of all known validators to the BN via /eth/v1/validator/prepare_beacon_proposer api once per slot
Added /eth/v1/validator/prepare_beacon_proposer api endpoint and preparation data caching
Added cleanup routine to remove cached proposer preparations when not updated for 2 epochs

Additional Info

Changed the Implementation following the discussion in #2883.

…t-file flags in validator client)

pk910 · 2022-01-17T10:40:18Z

Hmm, it was a nice try, but doesn't work at the moment...

lighthouse[69268]: Jan 17 09:34:48.001 CRIT Error whilst producing block message: All endpoints failed http://localhost:5052/ => RequestFailed("Error from beacon node when producing block: ServerMessage(ErrorMessage { code: 400, message: "BAD_REQUEST: invalid query: Invalid query string", stacktraces: [] })"), service: block

Seems I missed something - Looking into it..

pawanjay176 · 2022-01-17T13:55:21Z

@pk910 This is looking good :) Your issue looks like incorrect parsing of the address. You can try printing out the fee_recipient right before it is passed to the api in block_service.rs and see if it's what you are expecting.

Is it allowed to add an optional parameter to the bn api just like that or is the api specification managed somewhere else and needs a change request there?

You would need to request for change in the api specification as having a different behaviour would break lighthouse vc compatibility with other client beacon nodes. The api specification is maintained here https://github.com/ethereum/beacon-apis/issues

…ice that generates proposer preparations for all known validators at the beginning of each epoch

…file contents

…-validator # Conflicts: # consensus/types/src/lib.rs

pk910 · 2022-01-19T19:57:38Z

I've reimplemented the way how the fee-recipient is transmitted to the BN. There is now a new "proposal preparation" service in VC that sends the fee-recipients of all known validators to the BN at the beginning of each epoch using the new /eth/v1/validator/prepare_beacon_proposer api.

This is a working prototype that has successfully proposed blocks with individual fee-recipients for me in the kintsugi net.

A few things that should be looked into:

VC publishes the proposer preparations at the beginning of each epoch. I'm not sure if that interferes other much more important api calls at the beginning of an epoch or could overload the BN with a big number of VCs?
The proposer preparation is currently stored forever in a simple HashMap.. I'm pretty sure there is a much more elegant solution for that and I have a feeling that this cache should not live in the execution layer code (actually I didn't get it running in the BeaconChain struct, which looks like a better place for such a cache)
Proposer preparations are currently stored in memory forever. There should be something that removes them again when not updated for 2 epochs. As I think the cache implementation needs rework anyway I have not invested work in that yet.
There is no limitation of how many proposer preparations are stored in memory.. As the index is a u64 there can probably be enough entries to fill up BN memory completely - that looks like a possible dos attack surface to me

It can probably be implemented in a much cleaner way, so I'm totally fine if you decide to throw it away and redo yourself ;)
Can also try to rework some parts myself if you give me some advice how to continue :)

paulhauner · 2022-01-20T08:07:13Z

I haven't done a review, but thanks for your contribution @pk910!

Earlier today I created #2936. It seems this PR is along similar lines 🎉

I'll find some time this week or early next to review this 🙏

… too long and looks bad in log file :D)

paulhauner

This is looking really good, we're so close. I've left some minor requests here, I'm keen to merge after they're addressed.

Thanks again for this high-value contribution 🙏

beacon_node/beacon_chain/src/execution_payload.rs

paulhauner · 2022-02-07T05:15:35Z

beacon_node/http_api/src/lib.rs

+                        .epoch()
+                        .map_err(warp_utils::reject::beacon_chain_error)?;
+
+                    info!(


Suggested change

info!(

debug!(

This might get a little noisy when polling once per slot (per validator client).

Agree 👍
Should we change the log entry on VC side to debug, too?

Oh yep, good idea!

validator_client/src/config.rs

validator_client/src/cli.rs

Co-authored-by: Paul Hauner <[email protected]>

pk910 · 2022-02-07T16:17:49Z

@paulhauner
All suggestions applied 👍

paulhauner

This looks great, I just made suggestions for your comment in #2924 (comment).

After those a resolved, it's merge time 🎉

validator_client/src/preparation_service.rs

Co-authored-by: Paul Hauner <[email protected]>

pk910 · 2022-02-08T04:16:26Z

@paulhauner
applied 👍
ready to merge? 🎉

paulhauner · 2022-02-08T04:50:40Z

Let's goooo

bors r+

…t (similar to graffiti / graffiti-file) (#2924) ## Issue Addressed #2883 ## Proposed Changes * Added `suggested-fee-recipient` & `suggested-fee-recipient-file` flags to validator client (similar to graffiti / graffiti-file implementation). * Added proposer preparation service to VC, which sends the fee-recipient of all known validators to the BN via [/eth/v1/validator/prepare_beacon_proposer](ethereum/beacon-APIs#178) api once per slot * Added [/eth/v1/validator/prepare_beacon_proposer](ethereum/beacon-APIs#178) api endpoint and preparation data caching * Added cleanup routine to remove cached proposer preparations when not updated for 2 epochs ## Additional Info Changed the Implementation following the discussion in #2883. Co-authored-by: pk910 <[email protected]> Co-authored-by: Paul Hauner <[email protected]> Co-authored-by: Philipp K <[email protected]>

bors · 2022-02-08T06:55:06Z

Timed out.

paulhauner · 2022-02-08T19:52:04Z

bors retry

…t (similar to graffiti / graffiti-file) (#2924) ## Issue Addressed #2883 ## Proposed Changes * Added `suggested-fee-recipient` & `suggested-fee-recipient-file` flags to validator client (similar to graffiti / graffiti-file implementation). * Added proposer preparation service to VC, which sends the fee-recipient of all known validators to the BN via [/eth/v1/validator/prepare_beacon_proposer](ethereum/beacon-APIs#178) api once per slot * Added [/eth/v1/validator/prepare_beacon_proposer](ethereum/beacon-APIs#178) api endpoint and preparation data caching * Added cleanup routine to remove cached proposer preparations when not updated for 2 epochs ## Additional Info Changed the Implementation following the discussion in #2883. Co-authored-by: pk910 <[email protected]> Co-authored-by: Paul Hauner <[email protected]> Co-authored-by: Philipp K <[email protected]>

bors · 2022-02-08T21:45:49Z

Pull request successfully merged into unstable.

Build succeeded:

pk910 added 3 commits January 17, 2022 09:31

implemented per validator fee-recipient (fee-recipient / fee-recipien…

fa66c7b

…t-file flags in validator client)

run cargo fmt

e30b328

fix clippy issues

d35ded6

pk910 marked this pull request as draft January 17, 2022 12:21

pk910 added 4 commits January 17, 2022 15:29

fixed fee_recipient parameter formatting

51a1af6

reverted fee_recipient api parameter, added proposer preparation serv…

49e27f6

…ice that generates proposer preparations for all known validators at the beginning of each epoch

Added prepare_beacon_proposer api endpoint

2feef90

cache proposer preparations

815b4de

pk910 force-pushed the fee-recipient-per-validator branch from 0b4ad35 to 9384165 Compare January 18, 2022 23:29

Fixed VC panic when fee-recipient-file is unreadable & reload cached …

4330306

…file contents

pk910 force-pushed the fee-recipient-per-validator branch from 9384165 to 4330306 Compare January 19, 2022 13:11

pk910 added 2 commits January 19, 2022 14:16

Merge remote-tracking branch 'origin/unstable' into fee-recipient-per…

fff7c2b

…-validator # Conflicts: # consensus/types/src/lib.rs

cleanup implementation

34d8ada

paulhauner self-requested a review January 20, 2022 08:03

paulhauner self-assigned this Jan 20, 2022

paulhauner added 11 commits January 24, 2022 18:41

Add suggested fee recip. to val defs yaml

62539f9

Use fee recip from val defs

eef70d2

Add incomplete changes

ec1d253

Add incomplete docs

128fb2c

Tidy, publish each slot

1b7ccd9

Add "suggested-" prefix globally

bdade7f

Add comment

9576eff

Tidy docs

15665f6

Clarify docs

381176f

Tidy logs

02847e4

Publish on startup

f112f4f

paulhauner added the bellatrix Required to support the Bellatrix Upgrade label Feb 1, 2022

shorten "Successfully published proposer preparation" log entry (it's…

3072e5a

… too long and looks bad in log file :D)

paulhauner mentioned this pull request Feb 4, 2022

Collecting fee recipients from pre-merge validators #2994

Closed

paulhauner requested changes Feb 7, 2022

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 7, 2022

pk910 and others added 2 commits February 7, 2022 17:03

Apply suggestions from code review

a645948

Co-authored-by: Paul Hauner <[email protected]>

pass proposer_index as value to prepare_execution_payload

fe7ffdc

paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 7, 2022

paulhauner requested changes Feb 7, 2022

View reviewed changes

validator_client/src/preparation_service.rs Outdated Show resolved Hide resolved

validator_client/src/preparation_service.rs Outdated Show resolved Hide resolved

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 7, 2022

paulhauner mentioned this pull request Feb 7, 2022

Addition of prepare_beacon_proposer endpoint. ethereum/beacon-APIs#178

Merged

Apply suggestions from code review

4bc27b2

Co-authored-by: Paul Hauner <[email protected]>

paulhauner added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 8, 2022

bors bot changed the title ~~Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file)~~ [Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) Feb 8, 2022

bors bot closed this Feb 8, 2022

paulhauner mentioned this pull request Feb 17, 2022

Set fee recipient per validator #2883

Closed

This was referenced Mar 2, 2022

Add support for setting fee-recipient per validator ChainSafe/lodestar#3820

Closed

Implementing and calling prepare_beacon_proposer and providing suggestedFeeRecipient to the BN ChainSafe/lodestar#3693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) #2924

[Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) #2924

pk910 commented Jan 17, 2022 •

edited

Loading

pk910 commented Jan 17, 2022

pawanjay176 commented Jan 17, 2022 •

edited

Loading

pk910 commented Jan 19, 2022 •

edited

Loading

paulhauner commented Jan 20, 2022

paulhauner left a comment

paulhauner Feb 7, 2022

pk910 Feb 7, 2022

paulhauner Feb 7, 2022

pk910 commented Feb 7, 2022

paulhauner left a comment

pk910 commented Feb 8, 2022

paulhauner commented Feb 8, 2022

bors bot commented Feb 8, 2022

paulhauner commented Feb 8, 2022

bors bot commented Feb 8, 2022

[Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) #2924

[Merged by Bors] - Allow per validator fee recipient via flag or file in validator client (similar to graffiti / graffiti-file) #2924

Conversation

pk910 commented Jan 17, 2022 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

pk910 commented Jan 17, 2022

pawanjay176 commented Jan 17, 2022 • edited Loading

pk910 commented Jan 19, 2022 • edited Loading

paulhauner commented Jan 20, 2022

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner Feb 7, 2022

Choose a reason for hiding this comment

pk910 Feb 7, 2022

Choose a reason for hiding this comment

paulhauner Feb 7, 2022

Choose a reason for hiding this comment

pk910 commented Feb 7, 2022

paulhauner left a comment

Choose a reason for hiding this comment

pk910 commented Feb 8, 2022

paulhauner commented Feb 8, 2022

bors bot commented Feb 8, 2022

paulhauner commented Feb 8, 2022

bors bot commented Feb 8, 2022

pk910 commented Jan 17, 2022 •

edited

Loading

pawanjay176 commented Jan 17, 2022 •

edited

Loading

pk910 commented Jan 19, 2022 •

edited

Loading