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

[rkarabut]

Problem

reqRespTxHandler looked up and serialized every entry in a tx-fetch request. A legal-size request (~65 KB) fits ~2,047 copies of one 32-byte tx hash, so a peer can repeat a single hash and make the node read and serialize the same (up to 512 KiB) tx once per copy — building ~1 GiB of response and blocking the event loop on synchronous compression. The per-peer rate limit counts requests, not entries, so it doesn't bound this.

Fix

• De-duplicate the requested hashes before pool reads (a Map keyed by hash string), mirroring the sibling BLOCK_TXS handler's Set. A repeated hash is now served once.
• Cap the request at a generous ceiling (MAX_TX_HASHES_PER_REQUEST = 100); honest requesters chunk at 8 (chunkTxHashesRequest / batch-requester default), so this only rejects abusive over-batches, with BADLY_FORMED_REQUEST.

Test

Adds reqRespTxHandler tests: a repeated hash causes exactly one pool read; a normal distinct batch still serves each; an over-cap request is rejected before any pool read. Verified red→green against the built base (@aztec-labs/p2p jest 9/9 green; the dedup + cap tests fail against the original handler), prettier --check clean, tsgo -b --emitDeclarationOnly exit 0.

Addresses the LabsBox audit findings "[aztec-node] TX req/resp serves repeated hashes repeatedly" and its near-duplicate ("repeats one transaction hash into a 1 GiB response"). A responder-side output ceiling before compression (also suggested in the finding) is left as separate hardening.

🤖 Generated with Claude Code

[greptile-apps]

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

1.  Cap conflicts with configuration ▶
2.   Response workload remains unbounded ▶
3.  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]

Loading

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

[greptile-apps]

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.

[rkarabut]

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).

[greptile-apps]

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]

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.

[rkarabut]

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]

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!

[rkarabut]

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