From 5cbf4bdc12d1fd0f41c3aa92b3305d78cc538a7d Mon Sep 17 00:00:00 2001 From: Ratmir Karabut <2141440+rkarabut@users.noreply.github.com> Date: Tue, 15 Sep 2026 09:14:07 +0000 Subject: [PATCH 1/4] fix(p2p): de-duplicate and cap tx req/resp requests The tx-fetch handler looked up and serialized every entry in the request, so a peer could repeat one hash thousands of times within a legal-size request and make the node re-read and re-serialize the same tx per copy, building a huge response and stalling the event loop on compression. De-duplicate the requested hashes before pool reads (matching the BLOCK_TXS handler) and reject requests far above the honest batch size. Adds handler tests for the repeated-hash, normal-batch, and over-cap cases. Co-Authored-By: Claude Opus 4.8 --- .../src/services/reqresp/protocols/tx.test.ts | 49 ++++++++++++++++++- .../p2p/src/services/reqresp/protocols/tx.ts | 21 +++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts index d11b0f1e230..beb4728f2ea 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts @@ -2,7 +2,7 @@ import { MAX_TX_SIZE_KB } from '@aztec-labs/stdlib/p2p'; import { TxHash, TxHashArray } from '@aztec-labs/stdlib/tx'; import { describe, expect, it } from '@jest/globals'; -import { calculateTxResponseSize } from './tx.js'; +import { calculateTxResponseSize, reqRespTxHandler } from './tx.js'; describe('calculateTxResponseSize', () => { it('should return correct size for a single tx hash', () => { @@ -49,3 +49,50 @@ describe('calculateTxResponseSize', () => { expect(calculateTxResponseSize(buffer)).toBe(MAX_TX_SIZE_KB + 1); }); }); + +describe('reqRespTxHandler', () => { + const peerId = {} as any; + const makeMempools = (getTxByHash: (h: TxHash) => Promise) => ({ txPool: { getTxByHash } }) as any; + + it('serves a repeated hash only once (de-duplicates before pool reads)', async () => { + const h = TxHash.random(); + const lookups: string[] = []; + const mempools = makeMempools(async (x: TxHash) => { + lookups.push(x.toString()); + return undefined; + }); + const request = new TxHashArray(h, h, h, h, h).toBuffer(); + + await reqRespTxHandler(mempools)(peerId, request); + + // One repeated hash must cause exactly one pool read, not one per copy. + expect(lookups).toEqual([h.toString()]); + }); + + it('still serves each distinct hash in a normal batch', async () => { + const hashes = Array.from({ length: 3 }, () => TxHash.random()); + const lookups: string[] = []; + const mempools = makeMempools(async (x: TxHash) => { + lookups.push(x.toString()); + return undefined; + }); + const request = new TxHashArray(...hashes).toBuffer(); + + await reqRespTxHandler(mempools)(peerId, request); + + expect(lookups.sort()).toEqual(hashes.map(h => h.toString()).sort()); + }); + + it('rejects a request with too many hashes', async () => { + const lookups: string[] = []; + const mempools = makeMempools(async (x: TxHash) => { + lookups.push(x.toString()); + return undefined; + }); + const request = new TxHashArray(...Array.from({ length: 101 }, () => TxHash.random())).toBuffer(); + + await expect(reqRespTxHandler(mempools)(peerId, request)).rejects.toThrow(); + // Over-cap request is rejected before any pool read. + expect(lookups).toHaveLength(0); + }); +}); diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts index eee62661725..f4ff1398af8 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts @@ -7,6 +7,11 @@ import type { MemPools } from '../../../mem_pools/interface.js'; import type { ReqRespSubProtocolHandler } from '../interface.js'; import { ReqRespStatus, ReqRespStatusError } from '../status.js'; +// Honest requesters chunk tx-fetch requests at 8 hashes (see chunkTxHashesRequest +// and the batch requester default). Reject anything far above that so one peer +// cannot ask for a huge number of txs in a single legal-size request. +const MAX_TX_HASHES_PER_REQUEST = 100; + /** * We want to keep the logic of the req resp handler in this file, but we do not have a reference to the mempools here * so we need to pass it in as a parameter. @@ -30,9 +35,23 @@ export function reqRespTxHandler(mempools: MemPools): ReqRespSubProtocolHandler throw new ReqRespStatusError(ReqRespStatus.BADLY_FORMED_REQUEST, { cause: err }); } + if (txHashes.length > MAX_TX_HASHES_PER_REQUEST) { + throw new ReqRespStatusError(ReqRespStatus.BADLY_FORMED_REQUEST); + } + + // De-duplicate before serving: without this a peer can repeat one hash many + // times and make the node re-read and re-serialize the same tx per copy, + // turning a legal-size request into a huge response. + const uniqueByHash = new Map(); + for (const txHash of txHashes) { + uniqueByHash.set(txHash.toString(), txHash); + } + try { const txs = new TxArray( - ...(await Promise.all(txHashes.map(txHash => mempools.txPool.getTxByHash(txHash)))).filter(t => !!t), + ...(await Promise.all([...uniqueByHash.values()].map(txHash => mempools.txPool.getTxByHash(txHash)))).filter( + t => !!t, + ), ); return txs.toBuffer(); } catch (err: any) { From b5cded170d8aa2dd4ea5ca27f4497ce82cc9c768 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut <2141440+rkarabut@users.noreply.github.com> Date: Tue, 15 Sep 2026 09:31:57 +0000 Subject: [PATCH 2/4] test(p2p): use typed mocks in tx handler tests (no as-any) Replace the `as any` peerId/mempools fixtures with mock() and mockDeep(), matching the block_txs handler test style. Co-Authored-By: Claude Opus 4.8 --- .../p2p/src/services/reqresp/protocols/tx.test.ts | 14 +++++++++++--- .../p2p/src/services/reqresp/protocols/tx.ts | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts index beb4728f2ea..f51956b6bd2 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts @@ -1,7 +1,10 @@ import { MAX_TX_SIZE_KB } from '@aztec-labs/stdlib/p2p'; import { TxHash, TxHashArray } from '@aztec-labs/stdlib/tx'; import { describe, expect, it } from '@jest/globals'; +import type { PeerId } from '@libp2p/interface'; +import { mock, mockDeep } from 'jest-mock-extended'; +import type { MemPools } from '../../../mem_pools/interface.js'; import { calculateTxResponseSize, reqRespTxHandler } from './tx.js'; describe('calculateTxResponseSize', () => { @@ -28,7 +31,7 @@ describe('calculateTxResponseSize', () => { it('should fall back to single tx size for a raw TxHash buffer (not TxHashArray)', () => { // A raw TxHash (32 bytes) is not a valid TxHashArray serialization. - // TxHashArray.fromBuffer silently returns empty array on parse failure. + // TxHashArray.fromBuffer returns an empty array (no error) on a parse failure. const rawHash = TxHash.random().toBuffer(); expect(calculateTxResponseSize(rawHash)).toBe(MAX_TX_SIZE_KB + 1); @@ -51,8 +54,13 @@ describe('calculateTxResponseSize', () => { }); describe('reqRespTxHandler', () => { - const peerId = {} as any; - const makeMempools = (getTxByHash: (h: TxHash) => Promise) => ({ txPool: { getTxByHash } }) as any; + const peerId = mock(); + + const makeMempools = (getTxByHash: (h: TxHash) => Promise): MemPools => { + const mempools = mockDeep(); + mempools.txPool.getTxByHash.mockImplementation(getTxByHash); + return mempools; + }; it('serves a repeated hash only once (de-duplicates before pool reads)', async () => { const h = TxHash.random(); diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts index f4ff1398af8..88ea6e4dc4c 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts @@ -9,7 +9,7 @@ import { ReqRespStatus, ReqRespStatusError } from '../status.js'; // Honest requesters chunk tx-fetch requests at 8 hashes (see chunkTxHashesRequest // and the batch requester default). Reject anything far above that so one peer -// cannot ask for a huge number of txs in a single legal-size request. +// cannot pull far more txs than an honest request needs in a single call. const MAX_TX_HASHES_PER_REQUEST = 100; /** @@ -41,7 +41,7 @@ export function reqRespTxHandler(mempools: MemPools): ReqRespSubProtocolHandler // De-duplicate before serving: without this a peer can repeat one hash many // times and make the node re-read and re-serialize the same tx per copy, - // turning a legal-size request into a huge response. + // turning a small request into a much larger response. const uniqueByHash = new Map(); for (const txHash of txHashes) { uniqueByHash.set(txHash.toString(), txHash); From fa9c698d1a18498278eaef81c6dfd6fa176e6df1 Mon Sep 17 00:00:00 2001 From: Ratmir Karabut <2141440+rkarabut@users.noreply.github.com> Date: Thu, 17 Sep 2026 10:50:36 +0000 Subject: [PATCH 3/4] fix(p2p): bound tx req/resp response to the transport max, not a magic count The handler capped the request at a hard-coded 100 hashes, but 100 x MAX_TX_SIZE_KB is ~51 MiB - larger than the reqresp transport's max response size, which is only enforced on the requester side, so a peer could still make the responder read and serialize an oversized response. Derive the cap from the response budget (DEFAULT_MAX_RESPONSE_SIZE_KB / MAX_TX_SIZE_KB) so the responder never builds more than the transport will carry. Honest requesters chunk at 8, well under it. Assert the over-cap rejection returns BADLY_FORMED_REQUEST, not just any throw. Co-Authored-By: Claude Opus 4.8 --- .../p2p/src/services/reqresp/protocols/tx.test.ts | 8 +++++++- yarn-project/p2p/src/services/reqresp/protocols/tx.ts | 11 +++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts index f51956b6bd2..8fc06a3ddf4 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts @@ -5,6 +5,7 @@ import type { PeerId } from '@libp2p/interface'; import { mock, mockDeep } from 'jest-mock-extended'; import type { MemPools } from '../../../mem_pools/interface.js'; +import { ReqRespStatus, ReqRespStatusError } from '../status.js'; import { calculateTxResponseSize, reqRespTxHandler } from './tx.js'; describe('calculateTxResponseSize', () => { @@ -99,7 +100,12 @@ describe('reqRespTxHandler', () => { }); const request = new TxHashArray(...Array.from({ length: 101 }, () => TxHash.random())).toBuffer(); - await expect(reqRespTxHandler(mempools)(peerId, request)).rejects.toThrow(); + const err = await reqRespTxHandler(mempools)(peerId, request).then( + () => undefined, + e => e, + ); + expect(err).toBeInstanceOf(ReqRespStatusError); + expect((err as ReqRespStatusError).status).toBe(ReqRespStatus.BADLY_FORMED_REQUEST); // Over-cap request is rejected before any pool read. expect(lookups).toHaveLength(0); }); diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts index 88ea6e4dc4c..b74741b05a8 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.ts @@ -4,13 +4,16 @@ import { TxArray, TxHash, TxHashArray } from '@aztec-labs/stdlib/tx'; import type { PeerId } from '@libp2p/interface'; import type { MemPools } from '../../../mem_pools/interface.js'; +import { DEFAULT_MAX_RESPONSE_SIZE_KB } from '../../encoding.js'; import type { ReqRespSubProtocolHandler } from '../interface.js'; import { ReqRespStatus, ReqRespStatusError } from '../status.js'; -// Honest requesters chunk tx-fetch requests at 8 hashes (see chunkTxHashesRequest -// and the batch requester default). Reject anything far above that so one peer -// cannot pull far more txs than an honest request needs in a single call. -const MAX_TX_HASHES_PER_REQUEST = 100; +// Bound the request so the response the responder builds cannot exceed the reqresp +// transport's max response size: each hash yields up to MAX_TX_SIZE_KB, so cap the +// count at that budget. Honest requesters chunk at 8 (see chunkTxHashesRequest), well +// under this; a peer naming more is rejected before the pool lookup instead of forcing +// the node to read and serialize a response larger than the transport will carry. +const MAX_TX_HASHES_PER_REQUEST = Math.floor(DEFAULT_MAX_RESPONSE_SIZE_KB / MAX_TX_SIZE_KB); /** * We want to keep the logic of the req resp handler in this file, but we do not have a reference to the mempools here From f076258c10e16bb13ed5a11b915088e61e86f5dd Mon Sep 17 00:00:00 2001 From: Ratmir Karabut <2141440+rkarabut@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:01:25 +0000 Subject: [PATCH 4/4] test(p2p): drop no-await async from tx handler mocks (require-await) The mempool mock callbacks were async with no await, tripping eslint require-await. Return Promise.resolve(undefined) instead of an async arrow. Co-Authored-By: Claude Opus 4.8 --- .../p2p/src/services/reqresp/protocols/tx.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts index 8fc06a3ddf4..73ae658075b 100644 --- a/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts +++ b/yarn-project/p2p/src/services/reqresp/protocols/tx.test.ts @@ -66,9 +66,9 @@ describe('reqRespTxHandler', () => { it('serves a repeated hash only once (de-duplicates before pool reads)', async () => { const h = TxHash.random(); const lookups: string[] = []; - const mempools = makeMempools(async (x: TxHash) => { + const mempools = makeMempools((x: TxHash) => { lookups.push(x.toString()); - return undefined; + return Promise.resolve(undefined); }); const request = new TxHashArray(h, h, h, h, h).toBuffer(); @@ -81,9 +81,9 @@ describe('reqRespTxHandler', () => { it('still serves each distinct hash in a normal batch', async () => { const hashes = Array.from({ length: 3 }, () => TxHash.random()); const lookups: string[] = []; - const mempools = makeMempools(async (x: TxHash) => { + const mempools = makeMempools((x: TxHash) => { lookups.push(x.toString()); - return undefined; + return Promise.resolve(undefined); }); const request = new TxHashArray(...hashes).toBuffer(); @@ -94,9 +94,9 @@ describe('reqRespTxHandler', () => { it('rejects a request with too many hashes', async () => { const lookups: string[] = []; - const mempools = makeMempools(async (x: TxHash) => { + const mempools = makeMempools((x: TxHash) => { lookups.push(x.toString()); - return undefined; + return Promise.resolve(undefined); }); const request = new TxHashArray(...Array.from({ length: 101 }, () => TxHash.random())).toBuffer();