Skip to content

fix(validator): prevent double-sign on stuck-duty cleanup race and handle pg pool errors (A-1313, A-1314)#24556

Merged
spalladino merged 10 commits into
merge-train/spartan-v5from
spl/a-1313-a-1314-ha-signer-hardening
Jul 7, 2026
Merged

fix(validator): prevent double-sign on stuck-duty cleanup race and handle pg pool errors (A-1313, A-1314)#24556
spalladino merged 10 commits into
merge-train/spartan-v5from
spl/a-1313-a-1314-ha-signer-hardening

Conversation

@spalladino

@spalladino spalladino commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Hardens the validator HA signing path against an honest-validator double-sign and a Postgres-induced crash. Both issues live entirely in the HA signer.

Context

A-1313 (stuck-duty cleanup race → slashable double-sign). signWithProtection discarded the boolean from recordSuccess and returned the signature regardless. If the background cleanup loop deleted our SIGNING protection row while the (remote) signer was slow, the signature was broadcast with no protection record in place — so a later proposal for the same slot with different data could sign freely, i.e. slashable equivocation by an honest validator. The race is live today: SequencerClient.start() starts the validator client, which starts the cleanup loop. (The issue text claimed this was masked by #1111 never starting the cleanup loop; that is stale and no longer true.)

A-1314 (pg.Pool without an 'error' listener crashes HA validators). createHASigner built a new Pool(...) with no pool.on('error'). pg re-emits idle-client errors (e.g. a Postgres restart severing an idle connection) on the pool; with no listener Node escalates to uncaughtException and crashes the process — taking down every HA replica sharing the DB at once.

Approach

Four defense-in-depth fixes for A-1313 (part 1 alone closes the slashable broadcast; parts 2-3 remove the ways the race arises) plus the independent A-1314 liveness fix:

  • Treat recordSuccess === false as a signing failure: throw a new SigningLockLostError and never return/broadcast the signature. The duty is not deleted, since we no longer own the row. This is the hard safety backstop: even under pathological timer behavior the worst case is a failed duty, never a broadcast double-sign.
  • Bound each signer call with a hard timeout (executeTimeout), configured via signerCallTimeoutMs (default 30s) and clamped at construction to maxStuckDutiesAgeMs / 2 (default 144s / 2). Since signWithProtection is the only writer of SIGNING rows and every path through it is bounded by this clamped timeout, an in-flight signing always times out and releases its row well before stuck-duty cleanup could reclaim it — so cleanup can never race a live signing. A timeout takes the existing failure path (release the lock, then throw), and the orphaned signer promise is discarded.
  • Add a request timeout (AbortSignal.timeout) to both Web3Signer key-store fetches, surfaced as a clear timeout error instead of hanging. The keystore is only constructed directly in tests today (production uses node-keystore's remote signer), so the timeout is a constructor option defaulting to 30s.
  • Register a pool.on('error', ...) structured-log handler in createHASigner (covering both the created and injected pool), and add the same handler to the e2e HA fixture pools. Only message/code is logged, never the raw error.

API changes

  • New env var VALIDATOR_SIGNER_CALL_TIMEOUT_MS (default 30000) controls the per-call signing timeout (effective value clamped to maxStuckDutiesAgeMs / 2), exposed as config field signerCallTimeoutMs.
  • The confusable existing config field signingTimeoutMs (how long to wait for a peer node's in-progress signing, default 3s) is renamed to peerSigningTimeoutMs so each name says whose wait it bounds. Its env var VALIDATOR_HA_SIGNING_TIMEOUT_MS is already shipped and is kept unchanged; the derived CLI flag follows the field rename (--sequencer.signingTimeoutMs--sequencer.peerSigningTimeoutMs).

Each fix follows red/green with unit tests in validator-ha-signer (errors, signer, slashing-protection service, LMDB + Postgres backends, factory) and validator-client (Web3Signer key store).

Fixes A-1313
Fixes A-1314

@spalladino spalladino force-pushed the spl/a-1313-a-1314-ha-signer-hardening branch from 5028c35 to 8cb1fe1 Compare July 6, 2026 20:52
spalladino pushed a commit that referenced this pull request Jul 6, 2026
…uplicate_proposal test (#24557)

## Problem

`multi-node/slashing/duplicate_proposal › slashes validator who sends
duplicate proposals`
(`yarn-project/end-to-end/src/multi-node/slashing/equivocation_offenses.test.ts`)
flaked in CI on the unrelated PR #24556 with:

```
Target proposer 0x90f79bf6eb2c4f870365e785982e1f101e93b906 not found in any slot after 20 epoch attempts
```

Evidence: http://ci.aztec-labs.com/eb0b4e658e28440c (the sibling
`duplicate_attestation` case in the same file passed in the same run).

## Root cause

Pure statistics — no logic bug. `advanceToEpochBeforeProposer`
(`yarn-project/end-to-end/src/multi-node/slashing/setup.ts` on this
branch) scans upcoming epochs for one where the target validator is the
proposer, giving up after `maxAttempts = 20` epochs. The test calls it
with the default.

This suite uses a 2-slot epoch with `warmupSlots = 1`, so each epoch
attempt inspects exactly **one** candidate slot. The proposer for a slot
is `keccak256(abi.encode(epoch, slot, seed)) % committeeSize`, and with
the suite's 4-member committee (`COMMITTEE_SIZE = NUM_VALIDATORS = 4` in
`setup.ts`) each attempt is an independent 1/4 draw. Miss probability
over 20 attempts: `(3/4)^20 ≈ 0.32%`, i.e. roughly 1 in 315 runs —
matching the observed occasional flake.

## Fix

Raise the default `maxAttempts` from 20 to 50, bounding the miss
probability at `(3/4)^50 ≈ 5.7e-7` (~1 in 1.8M). Each attempt is only a
committee query plus an anvil timestamp warp, so the expected attempt
count stays ~4 and the extra headroom costs essentially nothing. The
raised default also covers this helper's other callers with the same
exposure (`broadcasted_invalid_block_proposal_slash`,
`broadcasted_invalid_checkpoint_proposal_slash`), none of which override
`maxAttempts`.

## Relationship to PR #24548

This is the same failure mode fixed by #24548 for the
`duplicate_attestation` flake, but #24548 does **not** cover this call
site: its patch touches `yarn-project/end-to-end/src/e2e_p2p/shared.ts`,
a file that does not exist on `merge-train/spartan-v5` (on this branch
the helper lives in `multi-node/slashing/setup.ts`, and there is no
`e2e_p2p/` directory). #24548's head branch was cut from a `next`-layout
tree while targeting `merge-train/spartan-v5`, which is why GitHub
reports it as dirty with ~4k changed files — it needs a rebase onto the
branch it targets, at which point its fix would land in this same file.
This PR applies the fix directly to the `merge-train/spartan-v5` copy of
the helper.

No tracked issue exists for this flake.

---
*Created by
[claudebox](https://claudebox.work/v2/sessions/0959caa9f03f59fa) ·
group: `slackbot`*
@spalladino spalladino enabled auto-merge (squash) July 6, 2026 21:08
@spalladino spalladino merged commit c19f81a into merge-train/spartan-v5 Jul 7, 2026
26 checks passed
@spalladino spalladino deleted the spl/a-1313-a-1314-ha-signer-hardening branch July 7, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants