diff --git a/packages/beacon-node/src/chain/validation/block.ts b/packages/beacon-node/src/chain/validation/block.ts index af7fa6a57984..988f1e0489d7 100644 --- a/packages/beacon-node/src/chain/validation/block.ts +++ b/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 -- diff --git a/packages/beacon-node/src/network/processor/gossipHandlers.ts b/packages/beacon-node/src/network/processor/gossipHandlers.ts index 65383fda33e4..dcb5f4b72bb8 100644 --- a/packages/beacon-node/src/network/processor/gossipHandlers.ts +++ b/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); diff --git a/packages/beacon-node/test/unit/chain/validation/block.test.ts b/packages/beacon-node/test/unit/chain/validation/block.test.ts index 0960268683fe..5b8e5d0881a6 100644 --- a/packages/beacon-node/test/unit/chain/validation/block.test.ts +++ b/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); diff --git a/packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts b/packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts index a2677855b823..ee2c452212ad 100644 --- a/packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts +++ b/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}> { + 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; + 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; + + 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; 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(),