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

Keymanager API: fix fee recipient API and add persistence #11540

Merged
merged 4 commits into from
Oct 4, 2022

Conversation

james-prysm
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This PR address 2 issues.

  1. Fee Recipient API did not periodically call the beacon API with the correct data.Keymanager API: Fee Recipient not periodically sending values #11539
  2. Fee Recipient values did not persist correctly after validator restarts. The value will now persist after the start of each epoch if updated through the API. if the validator restarts directly after an update before the beacon API is called then the value will not persist on restart. The value set in the validator client will override values in the beacon node on restart, if one does not wish for the value to overwrite the beacon node's values they should not provide the validator client any on startup.

Which issues(s) does this PR fix?

Fixes #11322,#11539

@james-prysm james-prysm added Bug Something isn't working API Api related tasks Keymanager-API keymanager-api-standards labels Oct 4, 2022
@james-prysm james-prysm self-assigned this Oct 4, 2022
@james-prysm james-prysm marked this pull request as ready for review October 4, 2022 15:25
@james-prysm james-prysm requested a review from a team as a code owner October 4, 2022 15:25
Comment on lines 60 to 61
log.Infof("Validator client started with provided proposer settings. The client will periodically signal updated settings such as fee recipient"+
" in the beacon node and custom builder ( if --%s)", flags.EnableBuilderFlag.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the update the log is a bit ungrammatical/incorrect:

The client will periodically signal updated settings such as fee recipient in the beacon node and custom builder

Comment on lines +256 to +257
v.proposerSettings = settings
v.validator.SetProposerSettings(settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have proposer settings in two places when we can read them from v.validator? This can cause bugs when we update in one place but not the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what caused the original problem, unfortunately...

the runner.go has access to the validator directly but other parts such as the API only has access to the validator service and not the validator inside it... the node service at startup passes the data via config to the validator service into the validator as well ( the validator is only created with the values on validator service start). To try to remediate some of this I made the validator service's proposersettings private and calling the update this way will update both. open to other suggestions on this.

@prylabs-bulldozer prylabs-bulldozer bot merged commit f5efde5 into develop Oct 4, 2022
@delete-merged-branch delete-merged-branch bot deleted the persist-fee-recipient branch October 4, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Bug Something isn't working Keymanager-API keymanager-api-standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fee recipient change not persisted
2 participants