Skip to content

fix(p2p): bound BLOCK_TXS explicit-hash requests - #202

Open
rkarabut wants to merge 2 commits into
mainfrom
rk/fix-a2026-block-txs-response-cap
Open

rkarabut wants to merge 2 commits into
mainfrom
rk/fix-a2026-block-txs-response-cap

Conversation

@rkarabut

@rkarabut rkarabut commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

A BLOCK_TXS request can name transactions by explicit hash. The response-size estimate counted only the bit-list positions, so a peer could send a small request with an empty bit list and many explicit hashes and force the node to serialize a full tx per hash: a large response from a small request.

Fix: count explicit hashes in the size estimate, cap explicit hashes per request (shared MAX_BLOCK_TXS_PER_REQUEST), and clamp the batch requester to the same bound so honest requests stay under it.

Test: a request naming more explicit hashes than the cap is rejected before the pool lookup. Verified red/green on the built base (block_txs 42/42, batch-tx-requester 37/37); prettier and tsgo clean.

🤖 Generated with Claude Code

Fixes A-2026

@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR appears safe to merge, with a non-blocking opportunity to make the new protocol regression test more precise.

Findings

  1. P2 Protocol status remains untested

Summary

This PR bounds explicit-hash BLOCK_TXS requests and aligns requester batching and response-size estimation with that limit.

  • Rejects requests containing more than 128 explicit transaction hashes before accessing the transaction pool.
  • Clamps requester batches to the responder’s limit.
  • Includes explicit hashes when estimating expected response size.
  • Adds regression coverage for rejecting an over-limit request.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Decode BLOCK_TXS request] --> B{Explicit hashes over 128?}
    B -->|Yes| C[Return BADLY_FORMED_REQUEST]
    B -->|No| D[Seed requested hash set]
    D --> E{Matching block available?}
    E -->|Yes| F[Add hashes selected by index bits]
    E -->|No| G[Keep explicit hashes only]
    F --> H[Fetch transactions from pool]
    G --> H
    H --> I[Serialize BLOCK_TXS response]
Loading

Reviews (1) · Last reviewed commit: "fix(p2p): bound BLOCK_TXS explicit-hash ..."

const tooMany = Array.from({ length: MAX_BLOCK_TXS_PER_REQUEST + 1 }, () => TxHash.random());
const request = makeRequest(Fr.random(), BitVector.init(0, []), [], tooMany);

await expect(callHandler(request)).rejects.toThrow();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Protocol status remains untested

This assertion only checks that an exception is thrown, so the test would still pass if the handler returned the wrong protocol status. Assert that the error is a ReqRespStatusError with BADLY_FORMED_REQUEST to protect the wire-level behavior introduced by this change.

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
rkarabut force-pushed the rk/fix-a2026-block-txs-response-cap branch from 7c784f0 to 59330f7 Compare September 16, 2026 23:33
The BLOCK_TXS handler serves explicit request.txHashes even when it lacks
the block, so a peer could send an empty bit list with a huge explicit-hash
list and force one full tx to be served per hash (~1 GB from a ~64 KB
request). Cap the explicit-hash count at a shared MAX_BLOCK_TXS_PER_REQUEST,
clamp the batch requester's batch size to the same bound so honest requests
never exceed it, and count explicit hashes in calculateBlockTxsResponseSize
so the size estimate reflects them. Adds a handler regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rkarabut
rkarabut force-pushed the rk/fix-a2026-block-txs-response-cap branch from 59330f7 to 3199ff0 Compare September 17, 2026 10:34
…ED_REQUEST

The cap test only checked that something threw, so it would pass on the wrong
protocol status. Assert a ReqRespStatusError with BADLY_FORMED_REQUEST so the
wire-level behavior is pinned.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant