Fix validator activation monitoring with inactive keys #8558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prestonvanloon
requested review from
rkapka and
rauljordan
and removed request for
rkapka
nisdas
reviewed
Mar 5, 2021
| @@ -129,3 +148,24 @@ func incrementRetries(ctx context.Context) context.Context { | |||
| attempts := streamAttempts(ctx) | |||
| return context.WithValue(ctx, waitForActivationAttemptsContextKey, attempts+1) | |||
| } | |||
|
|
|||
| func handleAccountsChanged(ctx context.Context, v Validator, accountsChangedChan chan<- struct{}) { | |||
There was a problem hiding this comment.
This method seems redundant, its basically creates a channel to relay account changes to the accountsChangedChan . How about changing the accountsChangedChan type to a chan<- [][48]byte and then subscribe account changes to it directly ? You could skip this additional routine and channel being created here.
There was a problem hiding this comment.
Excellent feedback! Doing this now.
rkapka
previously approved these changes
Mar 5, 2021
rkapka
dismissed
their stale review
Doesn't make sense to approve and suggest changes.
typo fixes
rkapka
previously approved these changes
Mar 5, 2021
prestonvanloon
added
Bug
rkapka
previously approved these changes
Mar 5, 2021
|
Some feedback from @rkapka in discord is that we could refactor this further to remove the recursive functionality. This feature / refactoring will come in a future PR. Thanks! |
This was referenced Mar 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
When starting a Prysm validator, the client would monitor the beacon chain for activations of its public keys. If none of the keys are active, it will continue to monitor until one of the keys has been activated. One of the features of the validator is that the key manager is monitoring for new key additions. The bug was that the new key additions were not being included in the activation monitoring routine and would require a restart to pick up the changes.
Tested in pyrmont and saw the keys reloaded.
Which issues(s) does this PR fix?
Fixes #8546
Other notes for review
Relevant doc: https://docs.google.com/document/d/1fLdUi6AHQ5-wYOy6DCzW5JlLlq3VuI275-KoyEZ2Pmw/edit#
This PR solves the following issues:
Some unrelated files are included due to goimports changes.
This PR also properly stops the SubscribeAccountChanges goroutine.