fix: verify the proposer signature before retaining unknown parent gossip blocks

packages/beacon-node/src/chain/validation/block.ts

@@ -90,6 +90,22 @@ export async function validateGossipBlock(
     throw new BlockGossipError(GossipAction.IGNORE, {code: BlockErrorCode.ALREADY_KNOWN, root: blockRoot});
   }
 
+  // Authenticate the block before the parent lookup so that only blocks signed by a validator are retained for
+  // unknown parent sync, a peer without a validator key is penalized for every block it sends
+  // [REJECT] The proposer index is a valid validator index
+  if (proposerIndex >= chain.pubkeyCache.size) {
+    throw new BlockGossipError(GossipAction.REJECT, {
+      code: BlockErrorCode.UNKNOWN_PROPOSER,
+      slot: blockSlot,
+      root: blockRoot,
+      proposerIndex,
+    });
+  }
+
+  // [REJECT] The proposer signature, signed_beacon_block.signature, is valid with respect to the proposer_index pubkey.
+  await verifyBlockProposerSignature(chain, signedBlock, blockRoot);
+  chain.seenBlockProposers.observeBlockRoot(blockSlot, proposerIndex, blockRoot, signedBlockHeader);
+
   // [REJECT] The current finalized_checkpoint is an ancestor of block -- i.e.
   // get_ancestor(store, block.parent_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)) == store.finalized_checkpoint.root
   const parentRoot = toRootHex(block.parentRoot);
@@ -105,6 +121,8 @@ export async function validateGossipBlock(
     //    descend from the finalized root.
     // (Non-Lighthouse): Since we prune all blocks non-descendant from finalized checking the `db.block` database won't be useful to guard
     // against known bad fork blocks, so we throw PARENT_BLOCK_UNKNOWN for cases (1) and (2)
+    // The block is retained for unknown parent sync, count it as this proposer's proposal for the slot
+    chain.seenBlockProposers.add(blockSlot, proposerIndex, blockRoot);
     throw new BlockGossipError(GossipAction.IGNORE, {code: BlockErrorCode.PARENT_BLOCK_UNKNOWN, parentRoot});
   }
 
@@ -122,6 +140,7 @@ export async function validateGossipBlock(
   if (isGloasBeaconBlock(block)) {
     const parentBlockHashHex = toRootHex(block.body.signedExecutionPayloadBid.message.parentBlockHash);
     if (chain.forkChoice.getBlockHexAndBlockHash(parentRoot, parentBlockHashHex) === null) {
+      chain.seenBlockProposers.add(blockSlot, proposerIndex, blockRoot);
       throw new BlockGossipError(GossipAction.IGNORE, {
         code: BlockErrorCode.PARENT_PAYLOAD_UNKNOWN,
         parentRoot,
@@ -257,20 +276,6 @@ export async function validateGossipBlock(
     }
   }
 
-  // [REJECT] The proposer index is a valid validator index
-  if (proposerIndex >= state.validatorCount) {
-    throw new BlockGossipError(GossipAction.REJECT, {
-      code: BlockErrorCode.UNKNOWN_PROPOSER,
-      slot: blockSlot,
-      root: blockRoot,
-      proposerIndex,
-    });
-  }
-
-  // [REJECT] The proposer signature, signed_beacon_block.signature, is valid with respect to the proposer_index pubkey.
-  await verifyBlockProposerSignature(chain, signedBlock, blockRoot);
-  chain.seenBlockProposers.observeBlockRoot(blockSlot, proposerIndex, blockRoot, signedBlockHeader);
-
   // [REJECT] The block is proposed by the expected proposer_index for the block's slot in the context of the current
   // shuffling (defined by parent_root/slot). If the proposer_index cannot immediately be verified against the expected
   // shuffling, the block MAY be queued for later processing while proposers for the block's branch are calculated --

packages/beacon-node/src/network/processor/gossipHandlers.ts

@@ -244,14 +244,15 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand
 
         // IGNORE means the block is acceptable (e.g. FUTURE_SLOT, ALREADY_KNOWN), just not propagated.
         // Keep the optimistically-added cache entries; they are pruned on finalization. Only REJECT
-        // (provably invalid), unexpected errors and repeat proposals that are not imported prune.
+        // (provably invalid), unexpected errors and repeat proposals that are not imported remove the entry.
         if (e.action === GossipAction.IGNORE) {
-          // Only a signature-verified sibling is imported by the beacon_block handler, any other repeat proposal
-          // is dropped from the caches and re-downloaded by sync if it ever becomes relevant. Its signature was not
-          // verified, so only its own entry is removed, never its claimed ancestors
+          // Only a signature-verified sibling with a known parent is imported by the beacon_block handler, any other
+          // repeat proposal is dropped from the caches and re-downloaded by sync if it ever becomes relevant. Only its
+          // own entry is removed, never its claimed ancestors, which an unverified block must not be able to evict
           if (
             e.type.code === BlockErrorCode.REPEAT_PROPOSAL &&
-            !chain.seenBlockProposers.hasBlockRoot(slot, signedBlock.message.proposerIndex, blockRootHex)
+            (!chain.seenBlockProposers.hasBlockRoot(slot, signedBlock.message.proposerIndex, blockRootHex) ||
+              chain.forkChoice.getBlockHexDefaultStatus(toRootHex(signedBlock.message.parentRoot)) === null)
           ) {
             chain.seenBlockInputCache.remove(blockRootHex);
             if (isForkPostGloas(fork)) {
@@ -752,7 +753,9 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand
           e.type.code === BlockErrorCode.REPEAT_PROPOSAL &&
           // Only a signature-verified sibling recorded in the seen cache is imported. The cache holds at most two
           // roots per proposer and slot, which bounds full imports over gossip to one alternate
-          chain.seenBlockProposers.hasBlockRoot(signedBlock.message.slot, e.type.proposerIndex, e.type.root)
+          chain.seenBlockProposers.hasBlockRoot(signedBlock.message.slot, e.type.proposerIndex, e.type.root) &&
+          // A sibling with an unknown parent cannot be imported, sync fetches it if its branch becomes relevant
+          chain.forkChoice.getBlockHexDefaultStatus(toRootHex(signedBlock.message.parentRoot)) !== null
         ) {
           // blockInput was optimistically seeded in validateBeaconBlock and retained on IGNORE
           const blockInput = chain.seenBlockInputCache.get(e.type.root);

packages/beacon-node/test/unit/chain/validation/block.test.ts

@@ -278,6 +278,53 @@ describe("gossip block validation", () => {
     );
   });
 
+  describe("authentication before the parent lookup", () => {
+    it("rejects an unknown parent block with an invalid proposer signature before looking up the parent", async () => {
+      verifySignature.mockResolvedValue(false);
+
+      await expectRejectedWithLodestarError(
+        validateGossipBlock(config, chain, job, ForkName.phase0),
+        BlockErrorCode.PROPOSAL_SIGNATURE_INVALID
+      );
+      expect(verifySignature).toHaveBeenCalledOnce();
+      expect(chain.seenBlockProposers.isKnown(clockSlot, proposerIndex)).toBe(false);
+      expect(regen.getState).not.toHaveBeenCalled();
+      expect(regen.getPreState).not.toHaveBeenCalled();
+    });
+
+    it("rejects an unknown parent block from a proposer index outside the registry without verifying", async () => {
+      (chain as unknown as {pubkeyCache: {size: number}}).pubkeyCache = {size: proposerIndex};
+
+      await expectRejectedWithLodestarError(
+        validateGossipBlock(config, chain, job, ForkName.phase0),
+        BlockErrorCode.UNKNOWN_PROPOSER
+      );
+      expect(verifySignature).not.toHaveBeenCalled();
+    });
+
+    it("retains a signed unknown parent block as the proposer's proposal and drops a second one for the slot", async () => {
+      const blockRoot = toRootHex(
+        ssz.phase0.BeaconBlockHeader.hashTreeRoot(signedBlockToSignedHeader(config, job).message)
+      );
+
+      await expectRejectedWithLodestarError(
+        validateGossipBlock(config, chain, job, ForkName.phase0),
+        BlockErrorCode.PARENT_BLOCK_UNKNOWN
+      );
+      expect(chain.seenBlockProposers.isKnown(clockSlot, proposerIndex)).toBe(true);
+      expect(chain.seenBlockProposers.hasBlockRoot(clockSlot, proposerIndex, blockRoot)).toBe(true);
+
+      // A second unknown parent block from the same proposer is an equivocation, not another retained block
+      const sibling: SignedBeaconBlock = {signature, message: {...block, stateRoot: Buffer.alloc(32, 1)}};
+      await expectRejectedWithLodestarError(
+        validateGossipBlock(config, chain, sibling, ForkName.phase0),
+        BlockErrorCode.REPEAT_PROPOSAL
+      );
+      expect(verifySignature).toHaveBeenCalledTimes(2);
+      expect(chain.seenBlockProposers.isEquivocating(clockSlot, proposerIndex)).toBe(true);
+    });
+  });
+
   it("NOT_LATER_THAN_PARENT", async () => {
     // Return not known for proposed block
     forkChoice.getBlockHexDefaultStatus.mockReturnValueOnce(null);

packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts

@@ -97,8 +97,101 @@ describe("getGossipHandlers", () => {
     expect(processBlock).not.toHaveBeenCalled();
     expect(threw).toBe(true);
   });
+
+  it("does not import a signature-verified REPEAT_PROPOSAL block whose parent is unknown", async () => {
+    const {processBlock, threw, removeBlockInput, pruneBlockInput} = await runBeaconBlockRepeatProposal(denebConfig, {
+      recorded: true,
+      parentKnown: false,
+    });
+    expect(removeBlockInput).toHaveBeenCalledOnce();
+    expect(pruneBlockInput).not.toHaveBeenCalled();
+
+    expect(processBlock).not.toHaveBeenCalled();
+    expect(threw).toBe(true);
+  });
+
+  it("leaves the peer penalty for a rejected block to the gossip validator", async () => {
+    const {core} = await runBeaconBlockValidationReject(denebConfig, BlockErrorCode.PROPOSAL_SIGNATURE_INVALID);
+
+    expect(core.reportPeer).not.toHaveBeenCalled();
+  });
 });
 
+async function runBeaconBlockValidationReject(
+  config: BeaconConfig,
+  code: BlockErrorCode.PROPOSAL_SIGNATURE_INVALID
+): Promise<{core: Pick<INetworkCore, "reportPeer">}> {
+  const logger = testLogger();
+  const peerIdStr = "16Uiu2HAmTestGossipPeer" as PeerIdStr;
+  const signedBlock = ssz.deneb.SignedBeaconBlock.defaultValue();
+  signedBlock.message.slot = 1;
+  const blockRootHex = toRootHex(ssz.deneb.BeaconBlock.hashTreeRoot(signedBlock.message));
+  const blockInput = BlockInputBlobs.createFromBlock({
+    block: signedBlock,
+    blockRootHex,
+    forkName: ForkName.deneb,
+    daOutOfRange: false,
+    source: BlockInputSource.gossip,
+    seenTimestampSec: 0,
+    peerIdStr,
+  });
+
+  vi.mocked(validateGossipBlock).mockRejectedValue(
+    new BlockGossipError(GossipAction.REJECT, {
+      code,
+      slot: signedBlock.message.slot,
+      root: blockRootHex,
+    })
+  );
+
+  const core = {reportPeer: vi.fn()} as Pick<INetworkCore, "reportPeer">;
+  const chain = {
+    clock: new ClockStopped(1),
+    custodyConfig: {sampledColumns: [], custodyColumns: []} as unknown as CustodyConfig,
+    emitter: new ChainEventEmitter(),
+    logger,
+    persistInvalidSszValue: vi.fn(),
+    processProposerEquivocation: vi.fn(),
+    seenBlockProposers: new SeenBlockProposers(),
+    seenBlockInputCache: {
+      getByBlock: vi.fn().mockReturnValue(blockInput),
+      remove: vi.fn(),
+    } as unknown as SeenBlockInput,
+    seenPayloadEnvelopeInputCache: {
+      add: vi.fn(),
+      remove: vi.fn(),
+    } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"],
+  } as unknown as IBeaconChain;
+
+  const handlers = getGossipHandlers(
+    {
+      aggregatorTracker: {} as AggregatorTracker,
+      chain,
+      config,
+      core: core as INetworkCore,
+      events: new NetworkEventBus(),
+      logger,
+      metrics: null,
+    },
+    {}
+  );
+  const beaconBlockHandler = handlers[GossipType.beacon_block] as SequentialGossipHandler<GossipType.beacon_block>;
+
+  await expect(
+    beaconBlockHandler({
+      gossipData: {serializedData: ssz.deneb.SignedBeaconBlock.serialize(signedBlock)},
+      peerIdStr,
+      seenTimestampSec: 0,
+      topic: {
+        boundary: {fork: ForkName.deneb, epoch: 0},
+        type: GossipType.beacon_block,
+      },
+    })
+  ).rejects.toThrow();
+
+  return {core};
+}
+
 async function runBeaconBlockProcessingError(
   config: BeaconConfig,
   code: BlockErrorCode.EXECUTION_ENGINE_ERROR | BlockErrorCode.EXECUTION_ENGINE_INVALID
@@ -179,7 +272,7 @@ async function runBeaconBlockProcessingError(
 
 async function runBeaconBlockRepeatProposal(
   config: BeaconConfig,
-  {recorded, reject = false}: {recorded: boolean; reject?: boolean}
+  {recorded, reject = false, parentKnown = true}: {recorded: boolean; reject?: boolean; parentKnown?: boolean}
 ): Promise<{
   processBlock: ReturnType<typeof vi.fn>;
   threw: boolean;
@@ -238,6 +331,7 @@ async function runBeaconBlockRepeatProposal(
     clock: new ClockStopped(1),
     custodyConfig: {sampledColumns: [], custodyColumns: []} as unknown as CustodyConfig,
     emitter: new ChainEventEmitter(),
+    forkChoice: {getBlockHexDefaultStatus: vi.fn().mockReturnValue(parentKnown ? {} : null)},
     getBlobsTracker: {triggerGetBlobs: vi.fn()},
     logger,
     persistInvalidSszValue: vi.fn(),