fix(zetaclient): keep signing when the keygen record is reset

[kingpinXD]

Summary

• zetacore blanks the keygen record on any observer set change (x/observer/abci.go BeginBlocker). The struct literal it writes zeroes the grantee list and resets the status to pending at block MaxInt64.
• zetaclient treated that record as a gate in three places, so a routine validator unbonding took every mainnet signer down at once and none could start again.
• Prefer the finalized TSS instead: whitelist from TSS.TssParticipantList, skip the ceremony when a key already exists, and stop watching the keygen record for restarts.
• Dry mode (start.go) already derived everything from the TSS object this way.

What happened on mainnet

An observer's validator was jailed and began unbonding. The staking hook dropped it from the observer set, and the next BeginBlocker blanked the keygen. waitForNewKeygen read that as a new keygen and shut every signer down; each then failed to restart because an empty grantee list meant an empty p2p whitelist.

No transaction was involved. Signing itself never reads the keygen record: go-tss takes both the threshold and the participant set from the local key share file, so the existing key stayed perfectly usable throughout.

Behaviour change worth flagging: scheduled keygen rotation is disabled while a finalized key exists

An earlier version of this description said rotation was unaffected. That was wrong, and the reasoning was circular — thanks @julianrubino for catching it.

KeygenCeremony is only called from Setup, and waitForNewKeygen was the only thing that restarted zetaclient when a keygen was scheduled. With the ceremony skipped and the watcher gone, a MsgUpdateKeygen will not start a ceremony, so no new key reaches TSS history, so waitForNewKeyGeneration never fires.

That is deliberate — waiting on the record is exactly what strands the signer once it has been reset — but it must not be discovered by an operator mid-rotation. Rotating on purpose means removing the current TSS first, or reintroducing a deliberate trigger. Both setup.go step 5 and the Listen comment now say so.

Genesis and first keygen are unaffected: with no TSS on chain the keygen record is still authoritative.

Tests

• resolveTSSPeers unit tests cover the blanked record, a scheduled keygen, a successful keygen, and the no-TSS-yet case. Mutation-checked: reverting the guard fails three of the four.
• New tss_listener_test.go (the listener had no tests). Covers no-shutdown on a blanked record, and that both TSS-address and TSS-history changes still shut down. Mutation-checked: re-adding a keygen watcher fails it.
• New isolated e2e, make start-keygen-reset-test. It resets the record, then asserts an outbound still signs under the same TSS key.

The e2e uses MsgAddObserver only as a stand-in — its handler writes byte-for-byte the same state as the BeginBlocker, in one transaction, without needing to drive a validator through jailing and unbonding. No add-observer transaction was involved in the incident. The test comment says so explicitly.

Compatibility with the emergency drain (#4612)

Verified compatible. The drain has no keygen references, and its deterministic leader election comes from localStateItem.ParticipantKeys in the key share, not the peer whitelist.

Worth noting: startDrainIfArmed sits after zetatss.Setup in start.go, so while Setup fails the drain cannot start either. This PR restores that lever.

Hotfix

Targets release/zetaclient/v37 and release/zetaclient/v38 to follow, same commit. Both touched files are byte-identical across all three branches.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Cd1ZRjpN5br7NRYA9sCp7G

---

Note

High Risk
Changes TSS startup, P2P whitelisting, and restart triggers in security-critical signing path; deliberate behavior change skips scheduled keygen at startup while a finalized key exists.

Overview
Fixes a mainnet-class outage where observer set changes blank the on-chain keygen record (empty grantees, pending at MaxInt64) while the finalized TSS key is unchanged.

zetaclient now treats a finalized TSS as authoritative: P2P whitelist comes from TSS.TssParticipantList, startup skips the keygen ceremony when a key already exists, and the TSS listener no longer watches the keygen record for restarts (only TSS pubkey changes and new entries in TSS history still trigger restart). Real key rotation behavior is unchanged.

Adds unit tests for resolveTSSPeers and the listener, plus an isolated e2e (make start-keygen-reset-test / --keygen-reset-test) that reproduces the blanked keygen state and asserts an outbound still signs under the same TSS key.

^{Reviewed by Cursor Bugbot for commit f705c56. Configure here.}

Greptile Summary

The PR keeps zetaclient operational when observer-set changes blank the keygen record by treating the finalized TSS as authoritative.

• Derives the P2P whitelist from finalized TSS participants and skips an unnecessary keygen ceremony.
• Removes keygen-record changes as a zetaclient restart trigger while retaining current-TSS and TSS-history watchers.
• Adds unit and isolated end-to-end coverage for continued signing after a keygen reset.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable defects identified in the changed signing, startup, listener, or test paths.

The finalized TSS remains authoritative after a keygen-record reset, peer selection retains the original participant set, and the remaining listener triggers continue to respond to actual TSS changes; the deliberate effect on scheduled rotations is clearly documented.

Important Files Changed

Filename	Overview
zetaclient/tss/setup.go	Makes the finalized TSS authoritative for peer selection and ceremony decisions, with explicit handling for missing peer data and TSS-query failures.
zetaclient/maintenance/tss_listener.go	Removes keygen-record polling while preserving restart triggers for current-TSS and TSS-history changes.
zetaclient/tss/setup_test.go	Covers peer resolution for initial keygen, blanked and scheduled records, genesis imports, and successful keygen state.
zetaclient/maintenance/tss_listener_test.go	Verifies that keygen resets are ignored while TSS address and history changes still trigger shutdown.
e2e/e2etests/test_keygen_reset_signing.go	Reproduces the reset state through the administrative handler and verifies that an outbound remains signable with the existing TSS key.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[zetaclient Setup] --> B[Read keygen record]
    B --> C[Read finalized TSS with retry]
    C --> D{Finalized TSS exists?}
    D -->|Yes| E[Whitelist TSS participants]
    E --> F[Skip keygen ceremony]
    D -->|No| G[Whitelist keygen grantees]
    G --> H[Run keygen ceremony]
    F --> I[Verify key shares and start signing]
    H --> I
    J[Observer set changes] --> K[Keygen record reset]
    K --> L[TSS listener ignores reset]
    L --> I
    M[TSS address or history changes] --> N[Trigger zetaclient restart]

Loading

_{Reviews (1): Last reviewed commit: "test(zetaclient): stop the listener test..." | Re-trigger Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 77.55102% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
zetaclient/tss/setup.go	80.85%	9 Missing ⚠️
zetaclient/tss/service.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[julianrubino]

Full review — read the diff plus the surrounding TSS setup/listener/keygen paths and the zetacore keygen→TSS flow. Core fix is correct and well-tested. One framing point worth correcting in the description, and two robustness notes. Details below + two inline comments.

Verified sound

• resolveTSSPeers whitelist source is right. TSS.TssParticipantList is populated from keygen.GetGranteePubkeys() at TSS creation (x/observer/keeper/msg_server_vote_tss.go:124), so for any key made via the normal ceremony it's a non-empty snapshot of the exact participants that produced the key — i.e. a correct superset of the local key-share participants. Empty-TSS → keygen-record path preserves genesis/first-keygen.
• Stopping the loop is correct. Removing waitForNewKeygen + skipping the ceremony when a finalized key exists breaks the blanked-record restart storm cleanly; waitForUpdate (TSS addr) and waitForNewKeyGeneration (TSS history) still cover a genuine key change.
• Tests are strong — mutation-checked units, the listener finally has tests, and the isolated e2e reproduces the exact zetacore state. The comment that MsgAddObserver is a stand-in (not the cause) is exactly the right kind of honesty.
• Drain compatibility is not just preserved — this is a prerequisite for it. startDrainIfArmed sits after zetatss.Setup in start.go, so while Setup was failing on mainnet the drain couldn't have started either. This restores that lever. 👍

1. Important (framing, not code): "Rotation is unchanged" is not accurate — this disables scheduled keygen rotation

KeygenCeremony is only ever called from Setup (zetaclient/tss/setup.go), and the removed waitForNewKeygen was the trigger that restarted the client on a newly-scheduled keygen → which is what re-ran Setup→KeygenCeremony for the new key. With both gone, while a finalized key exists: nothing runs a scheduled rotation ceremony, so no new key lands in TSS history, so waitForNewKeyGeneration never fires. Genesis/first keygen still works (empty-TSS path).

For the current wind-down (deliberately avoiding keygen, moving funds via the drain) this is fine and probably the intent — but the body says "Rotation is unchanged … the listener still restarts on it," which reads as if rotation still works. It doesn't, until a finalized key is gone. Suggest rewording to "scheduled keygen rotation is disabled while a finalized key exists" so a future operator doesn't assume otherwise, and note the deliberate re-enable path if a real rotation is ever needed.

2. Robustness: the GetTSS error fallback re-opens the exact failure mode (inline)

See inline on setup.go. On a GetTSS error you fall back to the keygen record; if that record is blanked (the incident state) and GetTSS blips, you get both symptoms back (empty whitelist + ceremony waiting on MaxInt64). Strictly better than before, but since the premise is "the keygen record is unreliable," consider retry/fail-loud instead of silently trusting it.

3. Minor: genesis-imported TSS without a participant list (inline)

See inline on resolveTSSPeers. The whitelist now hinges on TssParticipantList being populated; a TSS imported via x/observer/genesis.go without that field would yield an empty whitelist = same crash. Not the mainnet case, worth a guard or a note.

4. Nit

WithMetrics (zetaclient/tss/service.go:109) reads keygen.GranteePubkeys for blame labels — during the blanked window those labels go empty for a cycle. Harmless, just flagging.

5. codecov/patch is red

The isolated e2e path isn't counted in unit coverage; confirm that's the reason and not a genuinely uncovered branch in resolveTSSPeers/listener.

Net: approve-worthy once the description reflects that rotation-keygen is disabled (#1). #2/#3 are cheap hardening on a security-critical path and worth doing while we're here.

---

Precision notes (independently re-verified)

Two clarifications so the above is airtight — neither changes any verdict:

• #1 mechanism: the listener's trigger is graceful.ShutdownNow() + a supervised restart (the process manager brings zetaclient back up), not an in-process restart. Functionally identical for the argument — the restarted Setup is what would re-run the ceremony — but worth stating precisely: the removed waitForNewKeygen caused a shutdown, and a scheduled rotation now triggers no shutdown at all.
• #2 crash path: an empty whitelist doesn't fail at NewServer on its own — NewServer hard-errors only when the whitelist and the bootstrap peers are both empty. On mainnet bootstrap peers are configured, so Setup passes through the empty whitelist and reaches KeygenCeremony, which then hangs on the MaxInt64 block. That matches the observed incident (Setup crash, not a NewServer bootstrap-peer error). If bootstrap peers were also empty you'd instead get the "whitelisted peers missing" abort at NewServer — still a Setup failure, different line.

[morde08]

Approach looks right to me. Whitelisting from TSS.TssParticipantList is the correct source — it is written once from the grantee list at msg_server_vote_tss.go:124, is never touched by the BeginBlocker, and is exactly the set that produced the key in use (9 entries on mainnet today), so it is a correct superset for p2p admission. This restores full function without a governance action or a ceremony, which is strictly better than the MsgUpdateKeygen route in gov-ops#1184.

Two things before this goes to the release branches — one substantive, one a description fix.

---

1. GetTSS failure falls open into the original outage

currentTSS, tssErr := p.Zetacore.GetTSS(ctx)
if tssErr != nil {
    setupLogger.Info().Err(tssErr).Msg("no finalized TSS available")
    currentTSS = observertypes.TSS{}
}

Any error — including a transient RPC blip — is treated as "no TSS", so resolveTSSPeers falls back to keygen.GranteePubkeys. On mainnet today that is empty, which means an empty whitelist and the exact crash this PR fixes. GetTSSHistory two lines down fails closed and is loud; this one fails open into the bad state, silently, at Info.

Suggest wrapping it in retry.DoTypedWithBackoffAndRetry (as waitForNewKeyGeneration already does for GetTSSHistory) and only treating a genuine not-found as the empty case. This matters most during a coordinated restart against a busy zetacore — which is precisely the rollout for this fix.

Minor, same function: resolveTSSPeers uses TssPubkey != "" as the proxy for "usable participant list". Guarding on len(currentTSS.TssParticipantList) > 0 instead makes it total — otherwise a finalized TSS with an empty participant list yields an empty whitelist and the same crash.

---

2. "Rotation is unchanged" contradicts the code

KeygenCeremony is called from exactly one place (zetaclient/tss/setup.go:122) and Setup from exactly one (cmd/zetaclientd/start.go:146, at startup). With waitForNewKeygen removed, a running client never restarts on a scheduled keygen; and with the ceremony skipped whenever a finalized TSS exists, a manually restarted one will not run it either. The two remaining watchers are both reactive — TssPubkey changing, and TSS history length growing — and both only fire after a ceremony has already succeeded. So MsgUpdateKeygen becomes inert and there is no remaining path to rotate the key.

The tests make clear this is intended — "finalized key wins over a populated keygen record" codifies it, and the reasoning in that comment is sound. But the summary says the opposite, and the summary is what reviewers and the v37/v38 backport approvers will read. Worth rewording to state the removal explicitly.

Then the real question: if a rotation is ever needed while this is deployed, what is the path? As written it appears to be "roll the clients back first". If that is the accepted answer it is worth writing down. If not, an escape hatch — honour a scheduled keygen when the record has grantees and an operator opts in — would keep the option without reintroducing the strand this PR is removing.

---

Neither blocks the fix. Nice catch on waitForNewKeygen being the thing that took every running signer down at once — that explains why operators who never touched their nodes were also reporting crashes.

[kingpinXD]

Thanks both — all three points were real. Pushed in e8f89fbd2.

Rotation framing (@julianrubino #1). You're right and the old wording was circular: nothing would have produced the new key the listener was supposed to react to. The description now leads with "scheduled keygen rotation is disabled while a finalized key exists", and both setup.go step 5 and the Listen comment say the same, including how to rotate on purpose.

GetTSS fallback (@ws4charlie, @julianrubino #2). Retried with backoff; only the not-found code counts as absent; anything else fails loudly instead of falling back to a record we've just declared unreliable.

Genesis TSS without participants (@julianrubino #3). Confirmed real — x/observer/genesis.go:91 imports the TSS verbatim. Falls back to the keygen grantees while staying finalized so an imported key is never regenerated. Also added a guard so an empty whitelist fails with a clear message rather than as missing bootstrap peers.

Nit #4 — WithMetrics. Confirmed: it reads keygen.GranteePubkeys for blame labels, so during a blanked window those labels are empty until the record is repopulated. Metrics-only, no signing impact. Left alone to keep this diff to the outage.

#5 — codecov. The uncovered lines are the new block inside Setup itself, which has no unit test because it needs a real p2p server, key files and pre-params. The decision logic is factored into resolveTSSPeers and tssNotFound, both unit-tested and mutation-checked. Not an uncovered branch in either.

New tests: genesis-TSS-without-participants, and tssNotFound across not-found / transient / nil. Both mutation-checked — pointing tssNotFound at the wrong code fails two subtests, dropping the participant-list fallback fails the genesis one.

Verification on localnet is unchanged and still green: start-keygen-reset-test passes with the record blank at the end, and start-drain-test still moves funds.

[kingpinXD]

Thanks @morde08 — both points were right, and both are addressed as of 1795af8e0. Point 1 landed differently from your suggestion though, so it is worth saying why.

1. GetTSS failing open. Fixed, but without the not-found classification.

I did build that first, matching the status code. It was the wrong tool: zetacore answers an absent record with InvalidArgument rather than NotFound, so the match depended on that being wrong, and this ships to v37/v38 clients talking to a zetacore of a different version — correcting the code upstream would have stopped fresh chains starting. errors.Is is no help either, since the error crosses a gRPC boundary and only a code and a string survive.

Then it turned out nothing needed the distinction:

• Blanked record — nothing to whitelist, so the guard stops startup with a clear message and the supervisor retries. Fails closed and loud, which is what you asked for.
• Intact record — the grantees are the same set, and a keygen that already succeeded makes the ceremony a noop, so a blip heals itself instead of taking the node down.
• Fresh chain — the record is the correct source and the ceremony is the right next step.

GetTSS is retried first, constant 5s x 10, sized to outlast an RPC restart rather than the sub-second exponential I had before. Your coordinated-restart-against-a-busy-zetacore scenario is exactly what moved that number.

Your minor point is fixed as you described: resolveTSSPeers falls back to the keygen grantees when the participant list is empty, still reporting finalized so an imported key is never regenerated. It is real — x/observer/genesis.go:91 imports the TSS verbatim.

2. Rotation. Description corrected before your review landed; it now leads with rotation being disabled while a finalized key exists, and both code comments say the same.

On your real question — the honest answer today is "roll the clients back first", and I would rather write that down than pretend otherwise. Your escape hatch is the better answer, but it adds an opt-in path to an outage hotfix going to two release branches, so I would rather land this and do it deliberately as a follow-up than smuggle it in here. Happy to be overruled if you would rather it went in now.

Verification. Localnet keygen-reset e2e and the drain regression both pass; the record is still blank at the end and both signers stay clean, and a manual restart into the blanked state comes up on the existing key. Re-running both now against the current head, since the last few commits changed the startup path.

[kingpinXD]

@morde08 on the rotation path: we are not planning a TSS keygen at any point in the foreseeable future, so I am leaving the escape hatch out rather than adding an opt in path to a hotfix going to two release branches.

If that ever changes, the route is to roll the clients back first, and the step 5 comment in setup.go spells that out so nobody has to rediscover it under pressure.

[ws4charlie]

lgtm — fail-closed throughout (a blanked keygen record now ends in the "no TSS peers to whitelist" error rather than an empty whitelist), and the migration suites are off with the restore condition documented. Only the freshly-queued CI is outstanding; publish-release-legacy is red on every branch in the repo, not this PR.

[ws4charlie]

LGTM. The GetTSS retry plus the empty-whitelist guard covers my earlier comment nicely — worst case now is a loud startup failure instead of a signer hanging on a block that never comes, and the genesis-TSS-with-no-participants case is a good catch. Rotation staying off is clearly documented in all three places and the migration suites are disabled with a note, so nothing is silently broken.

[morde08]

Second pass, at 4e3fbd935 (the resolveWhitelist extraction). I re-derived the claims rather than reading them, and the core fix holds up.

Verified independently

• TssParticipantList is written once at TSS creation and is untouched by x/observer/abci.go:37's SetKeygen(Keygen{BlockNumber: MaxInt64}), so it survives the blanking. The struct literal does zero GranteePubkeys exactly as described.
• The e2e stand-in claim checks out: MsgAddObserver with AddNodeAccountOnly=true runs DisableInboundOnly + the same SetKeygen (x/observer/keeper/msg_server_add_observer.go:37,51). The comment's insistence that this is a convenience stand-in and not what happened on mainnet is worth keeping exactly as written.
• The "ceremony is a noop" reasoning is sound: zetaclient/tss/keygen.go:104 returns (false, nil) on KeyGenSuccess and line 89 returns zc.GetTSS(ctx), so the GetTSS-failed-but-record-intact fallback really does self-heal.
• I ran the mutation check myself. Re-adding a keygen watcher to Listen fails TestTSSListenerIgnoresKeygen and all three TestTSSListener subtests on the unexpected GetKeyGen. The guard is real, not decorative. All 10 new subtests pass locally.

Nothing below blocks the hotfix. The CI wiring gap is the one I'd want closed before this goes to v37/v38, since it is the only thing that would catch a regression of this exact bug.

[julianrubino]

Update after independent re-verification (two reviewers re-derived the claims against the current head b000039, which has advanced past my earlier review). Two corrections + one addition:

Correction 1 — my earlier codecov/patch note is stale

It's green on the current head, not red. The coverage gap I flagged was closed by the commits added since. Disregard that point.

Correction 2 — the GetTSS fallback is worse than I rated it (elevating: minor → Medium-High, worth closing before merge)

My inline on setup.go treated the GetTSS-error fallback as defense-in-depth. On closer look it re-opens the exact outage this PR fixes, under the same storm:

• GetTSS here is a single-shot call, no retry — contrast the listener (tss_listener.go) which wraps the identical call in retry.DoTypedWithBackoffAndRetry. The grpc handler errors on any transport failure, not just genesis "not found".
• On a healthy node whose keygen record is currently blanked (the incident state), a transient GetTSS hiccup at startup → currentTSS = TSS{} → resolveTSSPeers returns finalized=false → whitelist = empty grantees and KeygenCeremony runs against the blanked record and blocks forever at the zetaHeight < MaxInt64 wait.
• And it hangs rather than crash-loops: NewServer only errors when whitelist and bootstrap peers are both empty; with bootstrap peers configured (mainnet) it passes through to the stuck ceremony, so a k8s restart does not recover it.
• Trigger = mass restart → zetacore RPC pressure → transient GetTSS failure while the record is blanked — precisely the storm this PR targets.

Fix: wrap this GetTSS in the same retry the listener uses, and/or distinguish a genuine not-found (genesis → ceremony) from a transient error (→ return it, let the supervisor retry) instead of silently degrading. The genesis empty-participant-list guard I flagged inline would double as the fast-fail here (if finalized && len(participantList)==0 → error loudly).

Addition — no test restarts a client against a blanked record end-to-end

The e2e reproduces the state and proves keep-signing, but never restarts a zetaclient, so the actual mainnet failure mechanism — cold-start whitelist recovery from TssParticipantList against a blanked record — rests entirely on the resolveWhitelist unit test. Solid coverage, but if you want the e2e to truly "reproduce the incident," it needs a client restart against the blanked record.

Everything else in my original review stands and was confirmed: the whitelist-source change is a tightening not a weakening (TssParticipantList = the actual share holders; admission gate ≠ signing threshold), rotation-keygen is genuinely disabled while a finalized key exists (the "rotation unaffected" wording is circular — nothing produces the new key to land in history), and the tests are mutation-resistant. Net: ship-worthy, but I'd close the GetTSS-retry gap first since it re-opens the same outage.

[kingpinXD]

@julianrubino thanks for re-running this. Taking the three points in turn.

Correction 2 — the premise is stale. GetTSS has been retried since 3f95b978a, which landed before the head you reviewed. On b000039 it reads:

currentTSS, tssErr := retry.DoTypedWithBackoffAndRetry(
    func() (observertypes.TSS, error) { return client.GetTSS(ctx) },
    retry.DefaultConstantBackoff(),
)

Same helper and same backoff as the listener — 11 attempts over roughly 50s. GetKeyGen and GetTSSHistory got the same treatment in that commit, plus a third GetKeyGen hiding inside WithMetrics that also killed startup.

And the scenario terminates earlier than you traced it. Blanked record plus a GetTSS failure gives currentTSS = TSS{}, so resolveTSSPeers returns the empty grantee list with finalized=false — and then hits this, before NewServer and before any ceremony:

if len(peerSource) == 0 {
    return observertypes.TSS{}, nil, false, errors.New("no TSS peers to whitelist: ...")
}

Startup stops with that error. It never reaches KeygenCeremony, so the forever-block at zetaHeight < MaxInt64 is not reachable by this route. TestResolveWhitelist/query_failure_on_a_blanked_record_stops_startup pins exactly that, and mutating the guard fails exactly that test.

On "hangs rather than crash-loops": the NewServer condition you quote is real but it is only a pre-check. The binding gate is downstream in go-tss — resolveBootstrapPeers (go-tss@v0.6.3/tss/tss.go:243-247) opens with if len(net.WhitelistedPeers) == 0 { return nil, errors.New("whitelisted peers missing") }, with no reference to bootstrap peers. That is the mainnet crash string. @morde08 raised the same point and this was the answer there too.

I also built and then removed the not-found-vs-transient classification you suggest, in an earlier round. It could not be done reliably: the status code crosses an RPC boundary, so errors.Is does not survive it. The guard above is what makes the unconditional fallback safe instead.

Correction 1 — agreed, and thanks. Codecov is green at 77.55%.

Addition — you are right, and it is not fixable in the test. The e2e proves keep-signing but never restarts a client, so cold-start recovery rests on the unit tests. I did verify it by hand during development: a docker restart against the blanked state comes up with TSS key already finalized; whitelisting its participants grantees_in_keygen_record=0, then TSS service created.

Automating it is blocked on infrastructure. The e2e runs inside the orchestrator container, which has no docker socket — only promtail gets one, under the monitoring profile — and there is no docker client dependency in the repo. So a test cannot restart a sibling container. Filed as #4625, since it blocks any future test of zetaclient startup, not just this one.