[Merged by Bors] - Implement feerecipient API for keymanager by ethDreamer · Pull Request #3213 · sigp/lighthouse

ethDreamer · 2022-05-24T03:25:06Z

Issue Addressed

Implement feerecipient API for keymanager (VC) #3173

Proposed Changes

Moved all fee_recipient_file related logic inside the ValidatorStore as it makes more sense to have this all together there. I tested this with the validators I have on mainnet-shadow-fork-5 and everything appeared to work well. Only technicality is that I can't get the method to return 401 when the authorization header is not specified (it returns 400 instead). Fixing this is probably quite difficult given that none of warp's rejections have code 401.. I don't really think this matters too much though as long as it fails.

ethDreamer · 2022-05-24T15:37:30Z

For convenience, this is how the fee recipient page of the lighthouse book renders:

realbigsean

Nice! Couple of small things. Moving the fee recipient file management to the validator store makes sense to me as well.

common/eth2/src/lighthouse_vc/std_types.rs

validator_client/src/initialized_validators.rs

validator_client/src/http_api/mod.rs

michaelsproul

Looks good

I think it would be good to add a test or two in validator_client/src/http_api/tests/keystores.rs. The rest of the API is quite well tested and I think it would be straight forward to incorporate some feerecipient tests

ethDreamer · 2022-06-03T21:01:53Z

I added some tests which test the API endpoint as well as the fee_recipient_file fallback functionality..
@realbigsean and I were talking and we were wondering if we should just remove the fee_recipient_file functionality entirely now that we have an API endpoint to set the fee_recipient on the fly that will override the file if used.

It seems like it's redundant and less desirable now. On top of that, if we are to remove this, we should do it before the merge when people might start using it in their workflows..

what do you guys think @paulhauner @michaelsproul ?

michaelsproul

I'm on the fence about deleting the fee-recipient file. I think seeing as we already have it we might as well keep it. I think it might also be easier for some users to work with than the HTTP API. E.g. you can can update the default without knowing the pubkey of the validator you want to update the fee recipient for (which could be useful for some kinds of round-robin).

Other than that, there's also the matter of the recently added delete API: ethereum/keymanager-APIs#30. We could add it to this PR before merging, or bring it in separately seeing as this PR is already nicely self-contained and has been sitting in the merge queue so long. What's your preference @ethDreamer?

book/src/suggested-fee-recipient.md

realbigsean · 2022-06-16T18:41:55Z

Keeping the fee recipient file and storing fee recipients in the validator_definitions file seems like it adds too much complexity to be worth it to me. We'd have to decide whether to persist it in one place or both after API requests, and then also determine fallback order in different scenarios. I don't think these would be intuitive to users and we wouldn't want that to lead to misconfigurations that aren't apparent until proposal. To me it makes the most sense to have a single source of truth.

Would it be better to create a default_fee_recipient field in the validator_definitions file and re-use the logic from the fee recipient file around that?

See advisory: https://rustsec.org/advisories/RUSTSEC-2022-0032

michaelsproul

Let's merge this now and come back for future enhancements:

If the fee recipient setting API proves too slow with large number of validators, optimise it (possibly by batching disk writes), or if that isn't possible, add an alternative mechanism for loading fee recipients in bulk (possibly from a URL a la Teku).
Add a default field for the fee recipient in the validator_definitions.yml. For now this can be worked around by using the process-level default (i.e. the value of the --suggested-fee-recipient flag).

michaelsproul · 2022-07-06T02:30:42Z

bors r+

## Issue Addressed * #3173 ## Proposed Changes Moved all `fee_recipient_file` related logic inside the `ValidatorStore` as it makes more sense to have this all together there. I tested this with the validators I have on `mainnet-shadow-fork-5` and everything appeared to work well. Only technicality is that I can't get the method to return `401` when the authorization header is not specified (it returns `400` instead). Fixing this is probably quite difficult given that none of `warp`'s rejections have code `401`.. I don't really think this matters too much though as long as it fails.

bors · 2022-07-06T03:02:17Z

Build failed (retrying...):

beacon-chain-tests

## Issue Addressed * #3173 ## Proposed Changes Moved all `fee_recipient_file` related logic inside the `ValidatorStore` as it makes more sense to have this all together there. I tested this with the validators I have on `mainnet-shadow-fork-5` and everything appeared to work well. Only technicality is that I can't get the method to return `401` when the authorization header is not specified (it returns `400` instead). Fixing this is probably quite difficult given that none of `warp`'s rejections have code `401`.. I don't really think this matters too much though as long as it fails.

michaelsproul · 2022-07-06T03:16:59Z

CI failure looks spurious, I'll restart the batch

bors r-

bors · 2022-07-06T03:17:00Z

Canceled.

michaelsproul · 2022-07-06T03:17:31Z

bors r+

## Issue Addressed * #3173 ## Proposed Changes Moved all `fee_recipient_file` related logic inside the `ValidatorStore` as it makes more sense to have this all together there. I tested this with the validators I have on `mainnet-shadow-fork-5` and everything appeared to work well. Only technicality is that I can't get the method to return `401` when the authorization header is not specified (it returns `400` instead). Fixing this is probably quite difficult given that none of `warp`'s rejections have code `401`.. I don't really think this matters too much though as long as it fails.

bors · 2022-07-06T03:49:27Z

Build failed (retrying...):

dockerfile-ubuntu

## Issue Addressed * #3173 ## Proposed Changes Moved all `fee_recipient_file` related logic inside the `ValidatorStore` as it makes more sense to have this all together there. I tested this with the validators I have on `mainnet-shadow-fork-5` and everything appeared to work well. Only technicality is that I can't get the method to return `401` when the authorization header is not specified (it returns `400` instead). Fixing this is probably quite difficult given that none of `warp`'s rejections have code `401`.. I don't really think this matters too much though as long as it fails.

bors · 2022-07-06T06:15:11Z

Pull request successfully merged into unstable.

Build succeeded:

ethDreamer force-pushed the move_fee_recipient branch from e62a28d to 72a38e1 Compare May 24, 2022 04:51

ethDreamer added ready-for-review The code is ready for review bellatrix Required to support the Bellatrix Upgrade labels May 24, 2022

ethDreamer requested a review from michaelsproul May 24, 2022 05:03

ethDreamer force-pushed the move_fee_recipient branch 2 times, most recently from d6e65e2 to fb0c84a Compare May 24, 2022 15:48

realbigsean requested changes May 24, 2022

View reviewed changes

common/eth2/src/lighthouse_vc/std_types.rs Outdated Show resolved Hide resolved

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

validator_client/src/http_api/mod.rs Outdated Show resolved Hide resolved

realbigsean mentioned this pull request May 24, 2022

[Merged by Bors] - Register validator api #3194

Closed

michaelsproul requested changes May 27, 2022

View reviewed changes

michaelsproul 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 May 27, 2022

ethDreamer 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 Jun 3, 2022

ethDreamer requested a review from michaelsproul June 3, 2022 21:04

michaelsproul reviewed Jun 9, 2022

View reviewed changes

ethDreamer mentioned this pull request Jun 13, 2022

Remove fee_recipient_file functionality #3264

Closed

ethDreamer force-pushed the move_fee_recipient branch 2 times, most recently from 53312f7 to d7857c1 Compare June 15, 2022 18:04

realbigsean reviewed Jun 16, 2022

View reviewed changes

book/src/suggested-fee-recipient.md Show resolved Hide resolved

michaelsproul 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 Jun 19, 2022

ethDreamer added 6 commits July 5, 2022 13:13

Moved fee_recipient_file logic into

683bfed

Implement FeeRecipient API for VC Key Manager

e808bcc

Added Documentation for Fee Recipient API

b8d293e

Fixed HTTP Return Codes

d136e14

Fixed Clippy Warnings & Failing Tests

9e590c4

Fixed Response when Fee Recipient is unset

0ad5a2b

ethDreamer added 6 commits July 5, 2022 13:17

Final Clippy Complaints and Book Touch Up

f01be3a

Addressed @realbigsean's Comments

9a744a0

Added tests for Fee Recipient API

01d106f

Removed Fee Recipient File & Added DELETE Request

6fc01aa

Fixed test for missing auth header

87a4225

Rebase with unstable

b8642ea

ethDreamer force-pushed the move_fee_recipient branch from d1b4fd9 to b8642ea Compare July 5, 2022 18:34

Upgraded openssl-src to 111.22+

5f4d427

See advisory: https://rustsec.org/advisories/RUSTSEC-2022-0032

michaelsproul approved these changes Jul 6, 2022

View reviewed changes

michaelsproul 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 Jul 6, 2022

bors bot changed the title ~~Implement feerecipient API for keymanager~~ [Merged by Bors] - Implement feerecipient API for keymanager Jul 6, 2022

bors bot closed this Jul 6, 2022

paulhauner mentioned this pull request Jul 12, 2022

Implement feerecipient API for keymanager (VC) #3173

Closed

michaelsproul added the backwards-incompat Backwards-incompatible API change label Jul 22, 2022

michaelsproul mentioned this pull request Aug 2, 2022

Implement standard gas limit API #3403

Closed

ethDreamer deleted the move_fee_recipient branch December 19, 2022 21:24

Conversation

ethDreamer commented May 24, 2022

Issue Addressed

Proposed Changes

Uh oh!

ethDreamer commented May 24, 2022

Uh oh!

realbigsean left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

ethDreamer commented Jun 3, 2022

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

realbigsean commented Jun 16, 2022

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Jul 6, 2022

Uh oh!

bors bot commented Jul 6, 2022

Uh oh!

michaelsproul commented Jul 6, 2022

Uh oh!

bors bot commented Jul 6, 2022

Uh oh!

michaelsproul commented Jul 6, 2022

Uh oh!

bors bot commented Jul 6, 2022

Uh oh!

bors bot commented Jul 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants