fix(validator-client): slash a checkpoint attester only for the payload it signed

[rkarabut]

An invalid block proposal flags the whole slot, so honest checkpoint attesters in that slot became slashable.

Slash a checkpoint attester only when its payload hash is one this node rejected as an invalid checkpoint proposal, matching the evidence AttestedInvalidProposalWatcher uses.

Fixes A-2163

[greptile-apps]

Confidence Score: 4/5

[Critical risk] Changes when checkpoint attesters are slashed for invalid proposals.

The PR appears safe to merge; a same-slot, different-payload regression test would strengthen coverage of its central safeguard.

Findings

1.  Different-payload case lacks coverage ▶

Summary

The PR limits checkpoint-attester slashing to attestations whose signed payload hash matches a checkpoint proposal this node rejected.

• Adds a test showing that an invalid block alone must not make a checkpoint attester slashable.
• Revises a zero-penalty test to record and attest to an invalid checkpoint payload.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Checkpoint attestation received] --> B{Slot has invalid proposal<br/>and no equivocation?}
  B -- No --> C[Do not slash]
  B -- Yes --> D{Payload hash matches a recorded<br/>invalid checkpoint proposal?}
  D -- No --> C
  D -- Yes --> E[Emit attester slash event]

Loading

_{Reviews (1) · Last reviewed commit: "fix(validator-client): dont slash checkp..."}

[greptile-apps]

Different-payload case lacks coverage The new negative test records only an invalid block, so there is no invalid checkpoint hash to compare. The revised positive test uses a matching hash. Please also test an invalid checkpoint proposal followed by an attestation to a different payload in the same slot. Without that case, a future change that slashes whenever any checkpoint hash is recorded could reintroduce the false positive this fix prevents.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[spalladino]

This fixes the case where one bad block made every signer of the slot's valid checkpoint slashable. It also stops slashing an attester who blindly signs a checkpoint built on a block this node rejected: that rejection records no invalid-checkpoint payload hash, and both slashing paths now require one. Requesting changes so the rejection is recorded as evidence that binds it to the attested checkpoint, and so the two copies of this rule become one (details inline).

Separately, the red e2e in attested_invalid_proposal ("slashes a lazy attester for an invalid checkpoint and clears it on delayed equivocation") expects the slash this PR removes, so it needs rewriting under this PR's premise. invalidBlockProposalIndexWithinCheckpoint only randomizes the archive claimed by the gossiped block proposal (validation_service.ts:59-61). The block's header stays real, and the proposer checkpoints the real block (checkpoint_proposal_job.ts:1324-1335). So the rejection concerns an archive the checkpoint never signed, and the attester signed a valid payload. The inline comments are about rejections that invalidate something the signed payload commits to.

written by claude

[spalladino]

An attester who blindly signs a checkpoint built on a block this node rejected is no longer slashed by either path.

Scenario: the proposer gossips block A with an invalid archive, then a checkpoint whose last block is A, and nobody equivocates. An attester that signs everything, for example a node running with skipCheckpointProposalValidation, signs it. On an honest node, A is rejected for a slashable reason and the slot is marked (validator.ts:498), but A is not accepted into the local chain. Checkpoint validation looks for the checkpoint's archive only among local blocks (proposal_handler.ts:1657-1671), retries until the deadline, and returns last_block_not_found (proposal_handler.ts:2129-2141). That reason is not slashable (proposal_handler.ts:416), so markInvalidCheckpointProposal never runs (proposal_handler.ts:753-756).

This check then returns early. AttestedInvalidProposalWatcher already applied the same rule before this PR (attested_invalid_proposal_watcher.ts:123-129), so nothing slashes that attester now. Until this PR, the slot-wide check here was the only path that caught it, and only when the block's rejection finished before the attestation arrived.

Suggestion: record the rejection as evidence, but only when it is bound to the payload the attester signed. That payload is the checkpoint header, archive and fee modifier (checkpoint_proposal.ts:191-193), so the evidence needs two things:

• The rejected block is in the block sequence the payload commits to. Neither the archive the rejected proposal claims nor the checkpoint's embedded last block proves this. The claimed archive is just a value the proposer signed: a proposer can build a valid checkpoint ending at block n with archive R, and gossip an extra, invalid block at index n+1 that also claims R, before the checkpoint or reordered in transit. This node rejects it, and every honest signer of the valid checkpoint would match R. The embedded block is outside the signed payload, so a proposer can attach a different one to the copy a given node receives. The header's blockHeadersHash does commit to the sequence, so inclusion could be checked against it using the slot's retained block proposals. Missing evidence must never count as guilt.
• The rejection invalidates something the payload commits to: the block's header, or the checkpoint's archive when the rejected claim equals it. Today state_mismatch covers both a wrong header and a wrong archive claim (proposal_handler.ts:1817-1827), and a wrong claim for an archive the checkpoint does not sign proves nothing about the checkpoint. So that reason would need splitting. invalid_embedded_txs (proposal_handler.ts:362) says nothing about either.

The existing bookkeeping records only that the slot had an invalid block (validator.ts:497-498), so this needs per-proposal verdicts.

written by claude

[spalladino]

This function and AttestedInvalidProposalWatcher.scanSlot now run the same three checks: the slot is marked, the slot has no equivocation, and the payload hash is recorded. A-2163 came from these two copies drifting apart after #137 fixed only the watcher.

I suggest consolidating on the watcher. This callback decides once, when p2p adds the attestation (libp2p_service.ts:1335). A blind signer attests as soon as it receives the checkpoint. With skipCheckpointProposalValidation it runs the checkpoint callback before the embedded block (libp2p_service.ts:1548-1551). So its attestation can reach an honest node while that node is still re-executing the block, before any evidence exists, and this callback never looks at it again. The watcher reads the same attestation pool later, one slot behind the synced slot. It runs on validators and on non-validator offense collectors alike (aztec-node/src/factory.ts:397-419, 464-476).

Concretely: record the evidence in the proposal handler, then delete handleCheckpointAttestation, slashAttestedToInvalidCheckpointProposal, badAttestationOffenseKeys with MAX_TRACKED_BAD_ATTESTATIONS, and the registration at validator.ts:421-423. That registration is the only production consumer of the p2p checkpoint-attestation callback, so that plumbing can go too, along with its tests in libp2p_service.test.ts. Keep the clear on late equivocation in the duplicate-proposal handler (validator.ts:943), which is separate code. The unit tests for this path move to the watcher's tests.

This is not free. The watcher also scans each slot once, even after a failed pool read, and looks back only four slots on startup. Its clock is the synced L2 slot, so it lags when the archiver lags. It also depends on the attestation pool, which is pruned on finalization (p2p_client.ts:725). An equivocation that lands during its pool read can still slip past its check. The single scan may deserve a retry if the watcher becomes the only path. If you'd rather keep both paths, at least move the predicate into one shared method so the copies cannot drift again.

written by claude

[rkarabut]

I think it would probably be better to make recodding the rejection as evidence a follow-up pr, otherwise fixing

[rkarabut]

@spalladino decided to address it in this one after all