diff --git a/packages/beacon-node/src/chain/blocks/index.ts b/packages/beacon-node/src/chain/blocks/index.ts index f88e1e19473e..4b3a7f270c02 100644 --- a/packages/beacon-node/src/chain/blocks/index.ts +++ b/packages/beacon-node/src/chain/blocks/index.ts @@ -104,7 +104,7 @@ export async function processBlocks( payloadEnvelopesToImport.delete(orphaned.slot); // never validated, drop it from the shared cache so it is not served to peers, by-root sync reloads the // entry from db if the payload is ever needed - this.seenPayloadEnvelopeInputCache.prune(orphaned.payloadEnvelopeInput.blockRootHex); + this.seenPayloadEnvelopeInputCache.remove(orphaned.payloadEnvelopeInput.blockRootHex); } } @@ -278,7 +278,7 @@ async function importPayloadEnvelopesOfKnownBlocks( this.forkChoice.isDescendant(blockRootHex, PayloadStatus.EMPTY, head.blockRoot, head.payloadStatus) ) { // never validated, see processBlocks() - this.seenPayloadEnvelopeInputCache.prune(blockRootHex); + this.seenPayloadEnvelopeInputCache.remove(blockRootHex); orphaned.push({slot, payloadEnvelopeInput: payloadInput}); continue; } diff --git a/packages/beacon-node/src/chain/blocks/payloadEnvelopeInput/types.ts b/packages/beacon-node/src/chain/blocks/payloadEnvelopeInput/types.ts index a2ffa2ed9a79..d2a9594db920 100644 --- a/packages/beacon-node/src/chain/blocks/payloadEnvelopeInput/types.ts +++ b/packages/beacon-node/src/chain/blocks/payloadEnvelopeInput/types.ts @@ -23,7 +23,7 @@ export enum PayloadEnvelopeInputSource { * - prune: explicit prune by root * - cap: insertion-order backstop cap (MAX_PAYLOAD_ENVELOPE_INPUT_CACHE_SIZE) */ -export type PayloadEnvelopeInputPruneReason = "belowParent" | "finalized" | "prune" | "cap"; +export type PayloadEnvelopeInputPruneReason = "belowParent" | "finalized" | "remove" | "cap"; export type SourceMeta = { source: PayloadEnvelopeInputSource; diff --git a/packages/beacon-node/src/chain/seenCache/seenPayloadEnvelopeInput.ts b/packages/beacon-node/src/chain/seenCache/seenPayloadEnvelopeInput.ts index 79f97ea18108..d2577cbd75c0 100644 --- a/packages/beacon-node/src/chain/seenCache/seenPayloadEnvelopeInput.ts +++ b/packages/beacon-node/src/chain/seenCache/seenPayloadEnvelopeInput.ts @@ -256,10 +256,11 @@ export class SeenPayloadEnvelopeInput { return this.payloadInputs.size; } - prune(blockRootHex: RootHex): void { + /** Removes the single PayloadEnvelopeInput from the cache */ + remove(blockRootHex: RootHex): void { const input = this.payloadInputs.get(blockRootHex); if (input) { - this.evictPayloadInput(input, "prune"); + this.evictPayloadInput(input, "remove"); } } diff --git a/packages/beacon-node/src/network/processor/gossipHandlers.ts b/packages/beacon-node/src/network/processor/gossipHandlers.ts index 6dfd3fda6925..65383fda33e4 100644 --- a/packages/beacon-node/src/network/processor/gossipHandlers.ts +++ b/packages/beacon-node/src/network/processor/gossipHandlers.ts @@ -244,8 +244,20 @@ 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) and unexpected errors prune below. + // (provably invalid), unexpected errors and repeat proposals that are not imported prune. 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 + if ( + e.type.code === BlockErrorCode.REPEAT_PROPOSAL && + !chain.seenBlockProposers.hasBlockRoot(slot, signedBlock.message.proposerIndex, blockRootHex) + ) { + chain.seenBlockInputCache.remove(blockRootHex); + if (isForkPostGloas(fork)) { + chain.seenPayloadEnvelopeInputCache.remove(blockRootHex); + } + } throw e; } @@ -258,10 +270,10 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand } // REJECT or unexpected (non-BlockGossipError) error: drop the optimistically-added entries from - // both caches, keeping them consistent. - chain.seenBlockInputCache.prune(blockRootHex); + // both caches, keeping them consistent. The block may carry any parent root, so only its own entry is removed + chain.seenBlockInputCache.remove(blockRootHex); if (isForkPostGloas(fork)) { - chain.seenPayloadEnvelopeInputCache.prune(blockRootHex); + chain.seenPayloadEnvelopeInputCache.remove(blockRootHex); } throw e; } finally { @@ -738,7 +750,8 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand if ( e instanceof BlockGossipError && e.type.code === BlockErrorCode.REPEAT_PROPOSAL && - // this is make sure the block's proposer signature was verified, it should be true anyway + // 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) ) { // blockInput was optimistically seeded in validateBeaconBlock and retained on IGNORE diff --git a/packages/beacon-node/test/mocks/mockedBeaconChain.ts b/packages/beacon-node/test/mocks/mockedBeaconChain.ts index a4384da73fba..4849036de750 100644 --- a/packages/beacon-node/test/mocks/mockedBeaconChain.ts +++ b/packages/beacon-node/test/mocks/mockedBeaconChain.ts @@ -181,7 +181,7 @@ vi.mock("../../src/chain/chain.js", async (importActual) => { seenPayloadEnvelopeInputCache: { get: vi.fn(), getOrReload: vi.fn(), - prune: vi.fn(), + remove: vi.fn(), }, seenPayloadEnvelope: vi.fn(), shufflingCache: new ShufflingCache(), diff --git a/packages/beacon-node/test/unit/chain/blocks/processBlocks.test.ts b/packages/beacon-node/test/unit/chain/blocks/processBlocks.test.ts index 4606f3da31f7..2d33516636fb 100644 --- a/packages/beacon-node/test/unit/chain/blocks/processBlocks.test.ts +++ b/packages/beacon-node/test/unit/chain/blocks/processBlocks.test.ts @@ -295,12 +295,12 @@ describe("chain / blocks / processBlocks", () => { DataAvailabilityStatus.NotRequired, {validSignature: false} ); - expect(chain.seenPayloadEnvelopeInputCache.prune).toHaveBeenCalledExactlyOnceWith( + expect(chain.seenPayloadEnvelopeInputCache.remove).toHaveBeenCalledExactlyOnceWith( parent.payloadInput.blockRootHex ); } else { expect([...(envelopesForDa?.keys() ?? [])]).toEqual([slot - 1, slot]); - expect(chain.seenPayloadEnvelopeInputCache.prune).not.toHaveBeenCalled(); + expect(chain.seenPayloadEnvelopeInputCache.remove).not.toHaveBeenCalled(); } // the batch's own map is left untouched, range sync still owns it expect(payloadEnvelopes.size).toBe(2); @@ -375,7 +375,7 @@ describe("chain / blocks / processBlocks", () => { expect(importBlock).not.toHaveBeenCalled(); if (headOnEmpty && skipAttestations) { expect(importExecutionPayload).not.toHaveBeenCalled(); - expect(chain.seenPayloadEnvelopeInputCache.prune).toHaveBeenCalledExactlyOnceWith(blockRootHex); + expect(chain.seenPayloadEnvelopeInputCache.remove).toHaveBeenCalledExactlyOnceWith(blockRootHex); expect(chain.recomputeForkChoiceHead).not.toHaveBeenCalled(); // the skipped orphaned envelope is returned so range sync can log the serving peer/client expect(result).toEqual({orphaned: [{slot, payloadEnvelopeInput: payloadInput}], skipped: true}); @@ -383,7 +383,7 @@ describe("chain / blocks / processBlocks", () => { expect(importExecutionPayload).toHaveBeenCalledExactlyOnceWith(payloadInput, DataAvailabilityStatus.NotRequired, { validSignature: false, }); - expect(chain.seenPayloadEnvelopeInputCache.prune).not.toHaveBeenCalled(); + expect(chain.seenPayloadEnvelopeInputCache.remove).not.toHaveBeenCalled(); expect(chain.recomputeForkChoiceHead).toHaveBeenCalledOnce(); expect(result).toEqual({orphaned: [], skipped: false}); } diff --git a/packages/beacon-node/test/unit/chain/seenCache/seenPayloadEnvelopeInput.test.ts b/packages/beacon-node/test/unit/chain/seenCache/seenPayloadEnvelopeInput.test.ts index 162d67f0b3bf..17564406237c 100644 --- a/packages/beacon-node/test/unit/chain/seenCache/seenPayloadEnvelopeInput.test.ts +++ b/packages/beacon-node/test/unit/chain/seenCache/seenPayloadEnvelopeInput.test.ts @@ -183,21 +183,21 @@ describe("SeenPayloadEnvelopeInput", () => { expect(cache.size()).toBe(1); }); - it("prune removes a single entry by root and leaves others", () => { + it("remove removes a single entry by root and leaves others", () => { const rootHex1 = addPayloadInput(1); const rootHex2 = addPayloadInput(2); - cache.prune(rootHex1); + cache.remove(rootHex1); expect(cache.get(rootHex1)).toBeUndefined(); expect(cache.get(rootHex2)).toBeDefined(); expect(cache.size()).toBe(1); }); - it("prune is a no-op for an unknown root", () => { + it("remove is a no-op for an unknown root", () => { const rootHex = addPayloadInput(1); - expect(() => cache.prune(`0x${"ab".repeat(32)}`)).not.toThrow(); + expect(() => cache.remove(`0x${"ab".repeat(32)}`)).not.toThrow(); expect(cache.get(rootHex)).toBeDefined(); expect(cache.size()).toBe(1); }); 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 bbec2d65d6db..a2677855b823 100644 --- a/packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts +++ b/packages/beacon-node/test/unit/network/processor/gossipHandlers.test.ts @@ -65,7 +65,8 @@ describe("getGossipHandlers", () => { }); it("imports a signature-verified REPEAT_PROPOSAL (equivocating) block into fork choice but keeps IGNORE", async () => { - const {processBlock, threw} = await runBeaconBlockRepeatProposal(denebConfig, {recorded: true}); + const {processBlock, threw, removeBlockInput} = await runBeaconBlockRepeatProposal(denebConfig, {recorded: true}); + expect(removeBlockInput).not.toHaveBeenCalled(); // imported so LMD-GHOST can weigh it ... expect(processBlock).toHaveBeenCalledOnce(); @@ -73,8 +74,25 @@ describe("getGossipHandlers", () => { expect(threw).toBe(true); }); + it("removes only the rejected block's own cache entry on gossip REJECT", async () => { + const {processBlock, threw, removeBlockInput, pruneBlockInput} = await runBeaconBlockRepeatProposal(denebConfig, { + recorded: false, + reject: true, + }); + expect(threw).toBe(true); + expect(processBlock).not.toHaveBeenCalled(); + expect(removeBlockInput).toHaveBeenCalledOnce(); + expect(pruneBlockInput).not.toHaveBeenCalled(); + }); + it("does not import a REPEAT_PROPOSAL block whose root was not recorded (unverified 3rd+ proposal)", async () => { - const {processBlock, threw} = await runBeaconBlockRepeatProposal(denebConfig, {recorded: false}); + const {processBlock, threw, removeBlockInput, pruneBlockInput} = await runBeaconBlockRepeatProposal(denebConfig, { + recorded: false, + }); + // the block is not kept around, sync re-downloads it if it ever becomes relevant. Only its own entry goes, + // an unverified block must not be able to evict the ancestors it claims + expect(removeBlockInput).toHaveBeenCalledOnce(); + expect(pruneBlockInput).not.toHaveBeenCalled(); expect(processBlock).not.toHaveBeenCalled(); expect(threw).toBe(true); @@ -120,7 +138,7 @@ async function runBeaconBlockProcessingError( seenPayloadEnvelopeInputCache: { add: vi.fn(), get: vi.fn().mockReturnValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], serializedCache: {set: vi.fn()}, } as unknown as IBeaconChain; @@ -161,8 +179,13 @@ async function runBeaconBlockProcessingError( async function runBeaconBlockRepeatProposal( config: BeaconConfig, - {recorded}: {recorded: boolean} -): Promise<{processBlock: ReturnType; threw: boolean}> { + {recorded, reject = false}: {recorded: boolean; reject?: boolean} +): Promise<{ + processBlock: ReturnType; + threw: boolean; + removeBlockInput: ReturnType; + pruneBlockInput: ReturnType; +}> { const logger = testLogger(); const peerIdStr = "16Uiu2HAmTestGossipPeer" as PeerIdStr; const signedBlock = ssz.deneb.SignedBeaconBlock.defaultValue(); @@ -179,13 +202,21 @@ async function runBeaconBlockRepeatProposal( peerIdStr, }); - // gossip validation rejects the 2nd distinct block for this (proposer, slot) with REPEAT_PROPOSAL + // gossip validation rejects the 2nd distinct block for this (proposer, slot) with REPEAT_PROPOSAL, + // or the block outright when `reject` is set vi.mocked(validateGossipBlock).mockRejectedValue( - new BlockGossipError(GossipAction.IGNORE, { - code: BlockErrorCode.REPEAT_PROPOSAL, - proposerIndex: signedBlock.message.proposerIndex, - root: blockRootHex, - }) + reject + ? new BlockGossipError(GossipAction.REJECT, { + code: BlockErrorCode.INCORRECT_PROPOSER, + slot: signedBlock.message.slot, + root: blockRootHex, + proposerIndex: signedBlock.message.proposerIndex, + }) + : new BlockGossipError(GossipAction.IGNORE, { + code: BlockErrorCode.REPEAT_PROPOSAL, + proposerIndex: signedBlock.message.proposerIndex, + root: blockRootHex, + }) ); const seenBlockProposers = new SeenBlockProposers(); @@ -201,24 +232,28 @@ async function runBeaconBlockRepeatProposal( } const processBlock = vi.fn().mockResolvedValue(undefined); + const removeBlockInput = vi.fn(); + const pruneBlockInput = vi.fn(); const chain = { clock: new ClockStopped(1), custodyConfig: {sampledColumns: [], custodyColumns: []} as unknown as CustodyConfig, emitter: new ChainEventEmitter(), getBlobsTracker: {triggerGetBlobs: vi.fn()}, logger, + persistInvalidSszValue: vi.fn(), processBlock, processProposerEquivocation: vi.fn(), seenBlockProposers, seenBlockInputCache: { getByBlock: vi.fn().mockReturnValue(blockInput), get: vi.fn().mockReturnValue(blockInput), - prune: vi.fn(), + remove: removeBlockInput, + prune: pruneBlockInput, } as unknown as SeenBlockInput, seenPayloadEnvelopeInputCache: { add: vi.fn(), get: vi.fn().mockReturnValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], serializedCache: {set: vi.fn()}, } as unknown as IBeaconChain; @@ -257,7 +292,7 @@ async function runBeaconBlockRepeatProposal( await new Promise((resolve) => setTimeout(resolve, 0)); await new Promise((resolve) => setTimeout(resolve, 0)); - return {processBlock, threw}; + return {processBlock, threw, removeBlockInput, pruneBlockInput}; } function getExecutionBlockError( diff --git a/packages/beacon-node/test/unit/sync/unknownBlock.test.ts b/packages/beacon-node/test/unit/sync/unknownBlock.test.ts index ca79ce3cfd21..a0edfbb2ff77 100644 --- a/packages/beacon-node/test/unit/sync/unknownBlock.test.ts +++ b/packages/beacon-node/test/unit/sync/unknownBlock.test.ts @@ -491,7 +491,7 @@ describe("sync by UnknownBlockSync", {timeout: 20_000}, () => { add: vi.fn(), get: vi.fn().mockReturnValue(undefined), getOrReload: vi.fn().mockResolvedValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], }; @@ -740,7 +740,7 @@ describe("UnknownBlockSync", () => { add: vi.fn(), get: vi.fn().mockReturnValue(undefined), getOrReload: vi.fn().mockResolvedValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], } as unknown as IBeaconChain; @@ -827,7 +827,7 @@ describe("UnknownBlockSync", () => { add: vi.fn(), get: vi.fn().mockReturnValue(undefined), getOrReload: vi.fn().mockResolvedValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], seenBlockInputCache: {prune: vi.fn()} as unknown as SeenBlockInput, seenBlockProposers: {isKnown: vi.fn().mockReturnValue(false)} as unknown as SeenBlockProposers, @@ -901,7 +901,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -968,7 +968,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -1042,7 +1042,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -1114,7 +1114,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? cachedPayloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], seenBlockInputCache: { getByBlock: ({ @@ -1221,7 +1221,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], seenBlockInputCache: { getByBlock: ({ @@ -1329,7 +1329,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], seenBlockInputCache: { getByBlock: ({ @@ -1430,7 +1430,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -1491,7 +1491,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === blockRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], }, networkOverrides: {sendExecutionPayloadEnvelopesByRoot}, @@ -1530,7 +1530,7 @@ describe("UnknownBlockSync", () => { add: vi.fn(), get: vi.fn().mockReturnValue(payloadInput), getOrReload: vi.fn().mockResolvedValue(payloadInput), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -1602,7 +1602,7 @@ describe("UnknownBlockSync", () => { getOrReload: vi .fn() .mockImplementation((root: string) => (root === parentRootHex ? payloadInput : undefined)), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi @@ -1657,7 +1657,7 @@ describe("UnknownBlockSync", () => { seenPayloadEnvelopeInputCache: { add: vi.fn(), get: seenGet, - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], forkChoice: { hasPayloadHexUnsafe: vi.fn().mockReturnValue(false), @@ -2140,7 +2140,7 @@ describe("UnknownBlockSync", () => { add: vi.fn(), get: vi.fn(), getOrReload: vi.fn().mockResolvedValue(undefined), - prune: vi.fn(), + remove: vi.fn(), } as unknown as IBeaconChain["seenPayloadEnvelopeInputCache"], seenBlockInputCache: {prune: vi.fn()} as unknown as SeenBlockInput, seenBlockProposers: {