test(slash): attest eagerly when skipping checkpoint validation (#23722)

Slashing e2e tests that relied on `skipCheckpointProposalValidation`
were flakey when a previous block in the checkpoint was invalid. Reason
is that a checkpoint proposal usually carries a last block, which was
validated, but required the previous invalid block to be found. Since it
wasn't, the block proposal validation was timing out at the end of the
slot, then yielding to the checkpoint validation, which immediately
produced the attestation. This attestation only made it because of the
500ms grace period we give to them after the slot end.

This PR changes the flow when `skipCheckpointProposalValidation` is set,
so we attest immediately, and then we try processing the last block.

## Motivation

The `e2e_slashing_attested_invalid_proposal` test flakes: a "lazy"
validator (`skipCheckpointProposalValidation`) is supposed to attest to
a bad checkpoint proposal so an honest node records the offense, but it
sometimes attests ~24s late and peers reject the attestation as stale
(`Checkpoint attestation slot N is not current or next slot`), so the
offense is never recorded and the test times out. The root cause is a
real liveness issue, not just a test artifact: a skip-validation node's
attestation was serialized behind processing the last block embedded in
the checkpoint proposal, and that block's re-execution blocks until the
re-execution deadline (~a full slot) when its parent is unavailable —
which is guaranteed here, since the parent is the deliberately-invalid
block. Whether the resulting late attestation lands inside the slot's
acceptance window then comes down to timing jitter.

## Approach

When a node skips checkpoint proposal validation, its attestation needs
nothing from the embedded block, so create and broadcast it before
processing that block instead of after. Nodes that validate checkpoints
keep the original order (block first), because their checkpoint
validation depends on the last block. The decision is gated on the
`skipCheckpointProposalValidation` flag, mirrored into `P2PConfig` next
to its sibling `skipProposalSlotValidation`; the default is `false`, so
all production nodes are byte-for-byte unchanged.

## Changes

- **p2p (`libp2p_service.ts`)**: In `handleGossipedCheckpointProposal`,
process the checkpoint proposal before the embedded block when
`skipCheckpointProposalValidation` is set; otherwise keep the existing
block-then-checkpoint order. Single shared closure, no duplication.
- **p2p (`config.ts`)**: Mirror `skipCheckpointProposalValidation` into
`P2PConfig` (interface + mapping, no env var, default `false`).
- **validator-client (`proposal_handler.ts`)**: Remove a redundant
second `skipCheckpointProposalValidation` early-return in the all-nodes
checkpoint handler (dead code; the first early-return already covers
it).
- **p2p (tests)**: Add a test that a skip-mode node attests without
waiting for stalled block processing, and a complementary test that the
default node still processes the block first.

Author: spalladino

yarn-project/p2p/src/config.ts

@@ -246,6 +246,13 @@ export interface P2PConfig
 
   /** Accept proposal gossip regardless of slot timing (for testing only). */
   skipProposalSlotValidation?: boolean;
+
+  /**
+   * Whether this node skips checkpoint proposal validation and always attests. When set, the checkpoint
+   * attestation is created and broadcast before the embedded last block is processed, so it is not delayed
+   * past the slot's attestation window by that block's re-execution. Mirrors the validator config flag.
+   */
+  skipCheckpointProposalValidation?: boolean;
 }
 
 export const DEFAULT_P2P_PORT = 40400;
@@ -580,6 +587,11 @@ export const p2pConfigMappings: ConfigMappingsType<P2PConfig> = {
     description: 'Accept proposal gossip regardless of slot timing (for testing only)',
     ...booleanConfigHelper(false),
   },
+  skipCheckpointProposalValidation: {
+    description:
+      'Skip checkpoint proposal validation and always attest, broadcasting the attestation before processing the embedded last block',
+    ...booleanConfigHelper(false),
+  },
   minTxPoolAgeMs: {
     env: 'P2P_MIN_TX_POOL_AGE_MS',
     description: 'Minimum age (ms) a transaction must have been in the pool before it is eligible for block building.',

yarn-project/p2p/src/services/libp2p/libp2p_service.test.ts

@@ -955,6 +955,93 @@ describe('LibP2PService', () => {
       expect(storedBlock).toBeDefined();
     });
 
+    it('skipCheckpointProposalValidation: attests before (not gated by) slow last-block processing', async () => {
+      // Recreate the service in skip-validation mode and re-register the checkpoint/block callbacks on it.
+      service = createTestLibP2PServiceWithPools(
+        mockPeerManager,
+        reportMessageValidationResultSpy,
+        attestationPool,
+        mockTxPool,
+        mockEpochCache,
+        { skipCheckpointProposalValidation: true },
+      );
+      service.registerBlockReceivedCallback(blockReceivedCallback as any);
+      service.registerValidatorCheckpointReceivedCallback(validatorCheckpointReceivedCallback as any);
+      service.registerAllNodesCheckpointReceivedCallback(allNodesCheckpointReceivedCallback as any);
+
+      const checkpointHeader = makeCheckpointHeader(1, { slotNumber: targetSlot });
+      const blockHeader = makeBlockHeader(1, { slotNumber: targetSlot });
+      const proposal = await makeCheckpointProposal({ signer, checkpointHeader, lastBlock: { blockHeader } });
+
+      // Block processing hangs until released, simulating waiting for the parent block up to the
+      // re-execution deadline. In skip mode the attestation must not be blocked behind it.
+      let releaseBlock!: () => void;
+      blockReceivedCallback.mockReturnValue(
+        new Promise<boolean>(resolve => {
+          releaseBlock = () => resolve(true);
+        }),
+      );
+
+      // Resolves once the checkpoint attestation callback runs; if it were serialized behind the hung block
+      // processing this would never resolve and the test would time out.
+      let signalCheckpoint!: () => void;
+      const checkpointInvoked = new Promise<void>(resolve => {
+        signalCheckpoint = resolve;
+      });
+      validatorCheckpointReceivedCallback.mockImplementation(() => {
+        signalCheckpoint();
+        return Promise.resolve([]);
+      });
+
+      const handled = service.handleGossipedCheckpointProposal(proposal.toBuffer(), 'msg-1', mockPeerId);
+
+      await checkpointInvoked;
+      expect(validatorCheckpointReceivedCallback).toHaveBeenCalledTimes(1);
+
+      releaseBlock();
+      await handled;
+      expect(blockReceivedCallback).toHaveBeenCalledTimes(1);
+    });
+
+    it('default: processes the last block before the checkpoint proposal', async () => {
+      const checkpointHeader = makeCheckpointHeader(1, { slotNumber: targetSlot });
+      const blockHeader = makeBlockHeader(1, { slotNumber: targetSlot });
+      const proposal = await makeCheckpointProposal({ signer, checkpointHeader, lastBlock: { blockHeader } });
+
+      // Block processing hangs until released and signals when it starts; with validation enabled the
+      // checkpoint callback must wait for the block to finish.
+      let releaseBlock!: () => void;
+      let signalBlockStarted!: () => void;
+      const blockStarted = new Promise<void>(resolve => {
+        signalBlockStarted = resolve;
+      });
+      blockReceivedCallback.mockImplementation(() => {
+        signalBlockStarted();
+        return new Promise<boolean>(resolve => {
+          releaseBlock = () => resolve(true);
+        });
+      });
+
+      let checkpointInvoked = false;
+      validatorCheckpointReceivedCallback.mockImplementation(() => {
+        checkpointInvoked = true;
+        return Promise.resolve([]);
+      });
+
+      const handled = service.handleGossipedCheckpointProposal(proposal.toBuffer(), 'msg-1', mockPeerId);
+
+      // Wait until block processing is in flight (hung), then flush microtasks. The checkpoint callback
+      // must not have run, since it is gated behind the block.
+      await blockStarted;
+      await new Promise(resolve => setImmediate(resolve));
+      expect(checkpointInvoked).toBe(false);
+
+      // Once the block completes, the checkpoint proposal is processed.
+      releaseBlock();
+      await handled;
+      expect(checkpointInvoked).toBe(true);
+    });
+
     it('lastBlock processed even when checkpoint cap exceeded', async () => {
       const checkpointHeader = makeCheckpointHeader(1, { slotNumber: targetSlot });
       const blockHeader = makeBlockHeader(1, { slotNumber: targetSlot });
@@ -1525,6 +1612,7 @@ function createTestLibP2PServiceWithPools(
   attestationPool: AttestationPool,
   mockTxPool: MockProxy<TxPoolV2>,
   mockEpochCache: MockProxy<EpochCacheInterface>,
+  configOverrides?: Partial<P2PConfig>,
 ): TestLibP2PService {
   const mockNode = mock<PubSubLibp2p>();
   mockNode.services = {
@@ -1539,5 +1627,6 @@ function createTestLibP2PServiceWithPools(
     attestationPool,
     txPool: mockTxPool,
     epochCache: mockEpochCache,
+    configOverrides,
   });
 }

yarn-project/p2p/src/services/libp2p/libp2p_service.ts

@@ -1342,17 +1342,33 @@ export class LibP2PService extends WithTracer implements P2PService {
       TopicType.checkpoint_proposal,
     );
 
+    // Process checkpoint proposal if valid and not equivocated.
+    const processCheckpointFn = () =>
+      result === TopicValidatorResult.Accept && checkpoint && !isEquivocated
+        ? this.processValidCheckpointProposal(checkpoint.toCore(), source)
+        : Promise.resolve();
+
     // If the checkpoint contained a valid last block, we process it even if the checkpoint itself is to be rejected
     // TODO(palla/mbps): Is this ok? Should we be considering a block from a checkpoint that was equivocated?
-    if (processBlock && checkpoint?.getBlockProposal()) {
-      await this.processValidBlockProposal(checkpoint.getBlockProposal()!, source);
-    }
-
-    if (result !== TopicValidatorResult.Accept || !checkpoint || isEquivocated) {
+    const processBlockFn = () =>
+      processBlock && checkpoint && checkpoint.getBlockProposal()
+        ? this.processValidBlockProposal(checkpoint.getBlockProposal()!, source)
+        : Promise.resolve();
+
+    // A node that skips checkpoint validation attests without re-executing the embedded last block, so run
+    // the checkpoint callback first: this creates and broadcasts the attestation before the block is
+    // processed. Otherwise the block's re-execution — which can stall until the re-execution deadline
+    // waiting for a parent that may never arrive — would delay the attestation past the slot's attestation
+    // window, after which peers reject it as stale.
+    if (this.config.skipCheckpointProposalValidation) {
+      await processCheckpointFn();
+      await processBlockFn();
       return;
     }
 
-    await this.processValidCheckpointProposal(checkpoint.toCore(), source);
+    // Process the block first, since it's required for the checkpoint proposal validation.
+    await processBlockFn();
+    await processCheckpointFn();
   }
 
   /**

yarn-project/validator-client/src/proposal_handler.ts

@@ -311,14 +311,6 @@ export class ProposalHandler {
           return undefined;
         }
 
-        if (this.config.skipCheckpointProposalValidation) {
-          this.log.warn(
-            `Skipping all-nodes checkpoint proposal validation for slot ${proposal.slotNumber}`,
-            proposalInfo,
-          );
-          return undefined;
-        }
-
         const result = await this.handleCheckpointProposal(proposal, proposalInfo);
         if (!result.isValid) {
           await this.checkpointProposalValidationFailureCallback?.(proposal, result, proposalInfo);