fix(p2p): de-duplicate and cap tx req/resp requests by rkarabut · Pull Request #201 · aztec-labs-eng/aztec-node

rkarabut · 2026-09-15T09:14:29Z

A peer could send a tx-fetch request for an unbounded number of hashes, making the node read and serialize the potentially very large number of txs and return an oversized response.

Fix: cap the request at 100 hashes (honest requesters use 8), also de-duplicate before serving.

Fixes A-2061

greptile-apps · 2026-09-15T09:22:25Z

Confidence Score: 2/5

The PR is not yet safe to merge because valid configured requesters can exceed the new responder limit, and the remaining response-cost bound still permits substantial availability impact.

Findings

Cap conflicts with configuration ▶
Response workload remains unbounded ▶
Tests use forbidden casts ▶

Summary

This PR de-duplicates transaction hashes before pool access and rejects TX req/resp requests containing more than 100 hashes. It also adds focused tests for duplicate, distinct, and over-limit requests.

Repeated hashes now trigger one pool lookup and one response entry.
Requests over the new fixed count limit fail before pool access.
The fixed responder limit is not coordinated with the configurable requester batch size.
The count limit still permits costly responses containing up to roughly 51.2 MiB of transaction data.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Decode TxHashArray] --> B{More than 100 hashes?}
    B -->|Yes| C[BADLY_FORMED_REQUEST]
    B -->|No| D[De-duplicate by hash]
    D --> E[Load all distinct transactions concurrently]
    E --> F[Serialize complete TxArray]
    F --> G[Synchronously compress response]

_{Reviews (1) · Last reviewed commit: "fix(p2p): de-duplicate and cap tx req/re..."}

greptile-apps · 2026-09-15T09:22:28Z

      throw new ReqRespStatusError(ReqRespStatus.BADLY_FORMED_REQUEST, { cause: err });
    }

+    if (txHashes.length > MAX_TX_HASHES_PER_REQUEST) {


Cap conflicts with configuration

If P2P_BATCH_TX_REQUESTER_TX_BATCH_SIZE is configured above 100, the requester sends a batch that this handler rejects as BADLY_FORMED_REQUEST. The requester does not split and retry that batch; it penalizes the peer and clears its transaction metadata, which can prevent the batch from being fetched. Constrain the configuration and chunking helper to this limit, or derive both sides from a shared constant.

This handler serves the TX subprotocol, whose requests are chunked by chunkTxHashesRequest (fixed default 8), not P2P_BATCH_TX_REQUESTER_TX_BATCH_SIZE — that config drives the batch requester, which only sends BLOCK_TXS (batch_tx_requester.ts sends ReqRespSubProtocol.BLOCK_TXS). So no configurable path reaches this handler above the cap of 100 (12x the fixed TX chunk). The equivalent config-vs-cap coupling is real for BLOCK_TXS and is handled in that finding's fix (A-2026).

You're right — I conflated the TX and BLOCK_TXS request paths. chunkTxHashesRequest uses the fixed default of 8 for this handler, while P2P_BATCH_TX_REQUESTER_TX_BATCH_SIZE applies to BLOCK_TXS, not TX. Therefore no configurable requester can exceed this handler's cap of 100, and the cap does not introduce the failure mode I flagged. The configuration-coupling concern belongs to the separate BLOCK_TXS fix; I'll withdraw this comment.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

greptile-apps · 2026-09-15T09:22:28Z

+// 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;


Response workload remains unbounded

The count limit still allows about 51.2 MiB of uncompressed data because each of the 100 transactions may be 512 KiB. An admitted peer can issue ten such requests per second, and every response is fully materialized and then compressed synchronously without an output-byte or in-flight-work limit. This leaves a practical memory and event-loop exhaustion path; add a responder-side serialized-byte or work ceiling.

How this was verified: A request can reach 100 distinct pool entries of up to 512 KiB each, after which the complete response is serialized and synchronously compressed.

Agreed this is the stronger bound. The filed finding is the repeated-hash amplification, which the de-duplication fully fixes; the count cap additionally bounds a distinct-hash request to 100. A responder-side serialized-byte / work ceiling before the synchronous Snappy compression is the broader hardening (the finding lists it as a separate suggestion) and belongs in the shared reqresp compression path rather than this handler — tracking it as a follow-up so this fix stays focused.

greptile-apps · 2026-09-15T09:22:28Z

+
+describe('reqRespTxHandler', () => {
+  const peerId = {} as any;
+  const makeMempools = (getTxByHash: (h: TxHash) => Promise<unknown>) => ({ txPool: { getTxByHash } }) as any;


Tests use forbidden casts

The new peerId fixture uses as any, and makeMempools repeats the same pattern on the following line. This violates the repository directive to never use as any and hides whether these fixtures satisfy the handler contract. This repository requirement must be satisfied before merging; use typed fixtures or satisfies with the minimal required interfaces.

Context Used: yarn-project/CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fixed in 46d216c — replaced the as-any fixtures with mock() and mockDeep(), matching the block_txs handler test.

spalladino

LGTM, but should we add a check on the request side that we are not requesting more than MAX_TX_HASHES_PER_REQUEST, given txBatchSize is configurable by the operator and could be accidentally set to more than this value?

EDIT: Did I just make the same mistake greptile did above?

spalladino · 2026-09-18T20:37:29Z

+// 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.


Fun fact: chunkTxHashesRequest is dead code that we need to prune

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 <noreply@anthropic.com>

Replace the `as any` peerId/mempools fixtures with mock<PeerId>() and mockDeep<MemPools>(), matching the block_txs handler test style. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…c 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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

This node no longer originates TX hash-list requests (batch tx fetching goes through the BLOCK_TXS protocol), so the tx-hash chunker had no callers. Drop it and the now-unused chunk import; the responder-side MAX_TX_HASHES_PER_REQUEST cap is unchanged.

rkarabut · 2026-09-18T22:13:43Z

Removed the dead chunkTxHashesRequest helper (and its now-unused chunk import) — good catch.

On the request-side cap: txBatchSize drives the BLOCK_TXS protocol (a BitVector of indices into a known block proposal), not the TX hash-list protocol that MAX_TX_HASHES_PER_REQUEST bounds. Nothing originates TX hash-list requests anymore — chunkTxHashesRequest was the only one — so there's no request path that could exceed that cap. Your EDIT was right; no request-side check needed here.

greptile-apps Bot reviewed Sep 15, 2026

View reviewed changes

rkarabut requested a review from spalladino September 16, 2026 23:39

rkarabut force-pushed the rk/fix-a2061-tx-reqresp-dedup branch 2 times, most recently from f5ef3d7 to b5cded1 Compare September 17, 2026 10:33

spalladino approved these changes Sep 18, 2026

View reviewed changes

rkarabut and others added 5 commits September 18, 2026 22:03

test(p2p): use typed mocks in tx handler tests (no as-any)

84faaaf

Replace the `as any` peerId/mempools fixtures with mock<PeerId>() and mockDeep<MemPools>(), matching the block_txs handler test style. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

test(p2p): drop no-await async from tx handler mocks (require-await)

92d2f3e

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 <noreply@anthropic.com>

rkarabut force-pushed the rk/fix-a2061-tx-reqresp-dedup branch from f076258 to 008d93c Compare September 18, 2026 22:13

Navigation Menu

fix(p2p): de-duplicate and cap tx req/resp requests - #201

fix(p2p): de-duplicate and cap tx req/resp requests#201
rkarabut wants to merge 5 commits into
mainfrom
rk/fix-a2061-tx-reqresp-dedup

rkarabut commented Sep 15, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Sep 15, 2026 •

edited

Loading

Diagram

Uh oh!

greptile-apps Bot Sep 15, 2026

Uh oh!

rkarabut Sep 15, 2026

Uh oh!

greptile-apps Bot Sep 15, 2026

Uh oh!

greptile-apps Bot Sep 15, 2026

Uh oh!

rkarabut Sep 15, 2026

Uh oh!

greptile-apps Bot Sep 15, 2026

Uh oh!

rkarabut Sep 15, 2026

Uh oh!

spalladino left a comment •

edited

Loading

Uh oh!

spalladino Sep 18, 2026

Uh oh!

rkarabut commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rkarabut commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 2/5

Findings

Summary

Diagram

Uh oh!

greptile-apps Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

rkarabut Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

rkarabut Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

rkarabut Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

spalladino left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalladino Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

rkarabut commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rkarabut commented Sep 15, 2026 •

edited

Loading

greptile-apps Bot commented Sep 15, 2026 •

edited

Loading

spalladino left a comment •

edited

Loading