Improve wait for activation #13448
Improve wait for activation #13448
Conversation
| if len(validatingKeys) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
do we need maybe some small sleep here? we are here in the endless for loop and if channels won't be filled for some longer period I think we can experience some CPU spike here?
validator/client/validator.go
Outdated
| @@ -387,6 +386,12 @@ func (v *validator) checkAndLogValidatorStatus(statuses []*validatorStatus, acti | |||
| } | |||
| case ethpb.ValidatorStatus_ACTIVE, ethpb.ValidatorStatus_EXITING: | |||
| validatorActivated = true | |||
| if status.status.Status == ethpb.ValidatorStatus_ACTIVE { | |||
There was a problem hiding this comment.
Maybe separate the two cases ethpb.ValidatorStatus_ACTIVE and ethpb.ValidatorStatus_EXITING to avoid the
if status.status.Status == ethpb.ValidatorStatus_ACTIVEin the switch/case?
There was a problem hiding this comment.
probably makes more sense just not having the if statement actually. activated should mean that it's using the account
| return v.internalWaitForActivation(ctx, accountsChangedChan) | ||
| default: | ||
| res, err := (*stream).Recv() | ||
| if len(validatingKeys) == 0 { |
There was a problem hiding this comment.
validatingKeys is updated only once, before the for loop.
Should it not be updated inside the for loop as well?
There was a problem hiding this comment.
when the account changes the function is recursive. my understanding is that this loop only really needs to run if you have 0 active keys anyways. once you have 1 active key it exits and proceeds and subsequent key changes are handled in the HandleKeyReload function call which is the loop in runner.go
There was a problem hiding this comment.
You're right, I missed the recursivity.
| case <-accountsChangedChan: | ||
| // Accounts (keys) changed, restart the process. | ||
| // if the accounts changed try it again | ||
| return v.internalWaitForActivation(ctx, accountsChangedChan) | ||
| default: |
There was a problem hiding this comment.
This default case doesn't do anything, we can remove it
There was a problem hiding this comment.
this actually improved the efficiency as I believe golang efficiently handles the loop if this is the case
| // reset the ticker when they are all active | ||
| v.ticker = slots.NewSlotTicker(time.Unix(int64(v.genesisTime), 0), params.BeaconConfig().SecondsPerSlot) |
There was a problem hiding this comment.
What's the point of this code? The ticker is already set here:
prysm/validator/client/validator.go
Line 282 in 2875ce6
There was a problem hiding this comment.
I removed it, i don't think it has a usecase if it's set elsewhere, i wish it was more clear where it was set however
|
I did this test:
Get this response: And this log: The file Then... nothing (no validation). If I restart the validator, eveything is fine. |
|
broke e2e converting to draft as I resolve |
|
Ready again as tests have been fixed |
Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>
What type of PR is this?
Other
What does this PR do? Why is it needed?
Reported by the Figment team - "On startup, there is a check for keys, and if there are no keys it sleeps for 30 seconds. During this time we are using the key manager API to try and load the keys but it will hang for the full 30 seconds till the check happens again."
Upon reviewing the code more deeply there are many inefficiencies in the code with multiple loops and not event-driven via the accountChanged channel when we should be leveraging it. This function only cares if we have 0 validating keys and need at least 1 active one, outside of this function we have a recheck keys function that handles future keystore updates and this architecture should be revisited in the future to find more improvements.
baseline takes 30 seconds on the recheck
new approach go waits for the account to be populated to continue but can happen immediately
Which issues(s) does this PR fix?
Fixes #
Other notes for review