adding doppelganger rechecks mid epoch for key changes - #17285
adding doppelganger rechecks mid epoch for key changes#17285james-prysm wants to merge 23 commits into
Conversation
| if !features.Get().EnableDoppelGanger { | ||
| return | ||
| } | ||
| // Poll late in the epoch (Lighthouse's 3/4 offset) so the beacon node has |
There was a problem hiding this comment.
sorry missed this, it was an artifact checking lighthouse , will make sure this won't happen again
d2e861f broke it out into a helper function anyways
| @@ -0,0 +1,559 @@ | |||
| package client | |||
There was a problem hiding this comment.
Nit, instead of having multiple testing functions by tested function, you can use a single testing function with multiple sub-tests (t.Run).
| } | ||
| for _, pk := range keys { | ||
| d.checked[pk] = true | ||
| delete(d.pending, pk) |
There was a problem hiding this comment.
This deletes pending entries without checking p.blocked.
==> A runner restart reruns CheckDoppelGanger over all keys and silently releases a key that was already confirmed as a live duplicate.
There was a problem hiding this comment.
good catch on this, definitely missed, fixed in d2e861f this introduced more code changes so now i added status and vet startup
| } | ||
| // Keys unknown to the beacon node have no validator index yet, so no | ||
| // duplicate can exist anywhere; start normally instead of refusing. | ||
| if len(resp.Responses) == 0 { |
There was a problem hiding this comment.
Startup treats a non evaluable (empty) doppelganger response as permanently clean via markChecked. However checkReloadedKeys fails the close on the identical answer by keeping the keys quarantined.
There was a problem hiding this comment.
yeah that's a code smell, this refactor was a bit bigger, now i have doppelgangerfrom startup and then doppelganger mid epoch. i think it worked as intended before but definitely a code smell, hopefully this makes it read more straight forward
| @@ -66,7 +77,7 @@ func (v *validator) retryWaitForActivation(ctx context.Context, span octrace.Spa | |||
| // Reconnection attempt backoff, up to 60s. | |||
| time.Sleep(time.Second * time.Duration(min(uint64(attempts), 60))) | |||
| // TODO: refactor this to use the health tracker instead for reattempt | |||
There was a problem hiding this comment.
Do you want to keep this TODO before merging?
There was a problem hiding this comment.
yeah lets handle this separately
| time.Sleep(time.Second * time.Duration(min(uint64(attempts), 60))) | ||
| // TODO: refactor this to use the health tracker instead for reattempt | ||
| return v.WaitForActivation(incrementRetries(ctx)) | ||
| return v.waitForActivation(incrementRetries(ctx), false) |
There was a problem hiding this comment.
retryWaitForActivation is called recursively with a hardcoded accountsChanged=false.
Keys imported during the backoff sleep skip trackReloadedKeysForDoppelGanger.
| if !ok || !isActiveForDuties(st.status, epoch) { | ||
| continue | ||
| } | ||
| // Reloaded keys stay out of duties until their doppelganger check clears. |
There was a problem hiding this comment.
When every key is quarantined the duty store is reset.
RolesAt fails and emits an ERROR every slot for the whole 3-epoch quarantine window.
There was a problem hiding this comment.
another missed case here... i added a usecase, and changed reset to write empty when it's intentionally 0 d2e861f
|
Other fixes found while considering manu's comments
|
| req.ValidatorRequests = append(req.ValidatorRequests, | ||
| ðpb.DoppelGangerRequest_ValidatorRequest{ | ||
| PublicKey: pkey[:], | ||
| Epoch: 0, |
There was a problem hiding this comment.
if dph.validatorEpoch+2 < currentEpoch {
notRecentStringPubKeys = append(notRecentStringPubKeys, spk)
}(in validator/client/beacon-api/doppelganger.go)
Setting Epoch as a zero value makes every keys as notRecentStringPubKeys. We can think of VC1 and VC2 and the operator tries to migrate a key (PK1) from VC1 to VC2. Suppose the operator migrates the keys without waiting 3 epochs. Then AttestationHistoryForPubKey will return empty or nil value, and Epoch for the doppelganger request sets to zero.
It is obvious that a validator bound with PK1 has performed duties so far. Thus, doppelganger checker will mark PK1 as "blocked", and there's no way to recover this state when PK1 falls into the blocked state. Operator will only see a line of error logs.
Although we might blame operators, there might be a better solution, like evaluating the pubkey after three epochs if there's no history found in local.
There was a problem hiding this comment.
nice catch added fix and test here, wdyt 58573b6
# Conflicts: # testing/validator-mock/validator_mock.go # validator/client/iface/validator.go # validator/client/runner_test.go # validator/client/testutil/mock_validator.go
| @@ -0,0 +1,6 @@ | |||
| ### Fixed | |||
|
|
|||
| - Validator client: keys added through a keymanager reload are now held out of duties until a doppelganger check clears them (behind `--enable-doppelganger`). | |||
There was a problem hiding this comment.
Maybe we can make this bullet point to ### Added as it's a new feature?
| watermark := r.Target | ||
| // Cap a pending key's watermark at its quarantine clock: the node's | ||
| // recency band must expire before the wait, or answers stay unevaluated. | ||
| if added, ok := v.doppelGanger.pendingAddedEpoch(pkey); ok && watermark > added { |
There was a problem hiding this comment.
| if added, ok := v.doppelGanger.pendingAddedEpoch(pkey); ok && watermark > added { | |
| if added, ok := v.doppelGanger.pendingAddedEpoch(pkey); ok { |
I think we can just set the recency gate with added epoch, regardless it's greater or lesser than r.Target (== watermark). If r.Target < addedEpoch, say r.Target = 10 and addedEpoch = 20, newly loaded key can be blocked as the doppelganger check returns that it's a duplicate. r.Target is just a local DB view, so it cannot represent whether this key is actually used for signing recently. For pending (but not yet blocked) keys, we can just use addedEpoch. Please correct me if I'm wrong.
There was a problem hiding this comment.
I think you're right on this let me fix it
Co-authored-by: Jun Song <87601811+syjn99@users.noreply.github.com>
3 participants
What type of PR is this?
Feature
What does this PR do? Why is it needed?
with
--enable-doppelganger, only keys present at startup were ever checked — keys added to a running validator client via a keymanager reload became eligible for duties immediately, so a key still attesting elsewhere could be hot-imported and double-sign within the same epoch.With this change (still behind
--enable-doppelganger, off by default):Not changed: a duplicate detected at startup still prevents the client from starting, everything without the flag behaves as before, and no signing/slashing-protection logic is touched — only duty eligibility and check scheduling.
How this was tested
Kurtosis devnet: 4 prysm/ethrex nodes, gloas at epoch 1, 6s slots; vc-1..3 run
--enable-doppelganger, vc-4 is the flag-off control. Every VC's startup log showsPrysm/v7.1.8/1f95dd9a587493383a75cd19644e6182c4c64a49(this branch's commit). All key changes were made against the running VCs viaaccounts import/accounts delete— picked up through the keymanager file watcher, no restarts anywhere in the run.Exact kurtosis config and key-change commands
Key changes (each VC has 128 active keys; wallet at
/validator-keys/prysm, direct keymanager):eligibleEpoch=4); detected and blocked at the epoch-3 poll — one epoch before the earliest clear; 0 attestations all run; the other 128 keys kept attestingCheckDoppelGangerRPC per epoch for 10+ epochs, zero errors; never cleared, never attestedeligibleEpoch=12); cleared exactly at the epoch-12 poll; resumed attesting at slot 422Chain health: continuous head advance across the gloas fork, finality reached epoch 13, no unexpected error-level lines in any VC log.
Which issue(s) does this PR fix?
Fixes # #17328
Other notes for review
Acknowledgements