Skip to content

fix(p2p): process one request per reqresp stream to enforce rate limit#24552

Merged
spalladino merged 1 commit into
merge-train/spartan-v5from
spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the
Jul 7, 2026
Merged

fix(p2p): process one request per reqresp stream to enforce rate limit#24552
spalladino merged 1 commit into
merge-train/spartan-v5from
spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the

Conversation

@spalladino

Copy link
Copy Markdown
Contributor

Context

The reqresp rate limiter is consulted once per inbound stream (streamHandler), but the sub-protocol handler was invoked once per chunk read from that stream (processStream). Req/resp is one-request-one-response and an honest sender writes a single payload before half-closing, but a malicious peer can open one stream (costing a single rate-limit token) and then push many request frames on it — each frame arrives as its own chunk and drives a full handler invocation (mempool / block / tree lookups). Per-peer and global rate limits are bypassed by the fan-out factor.

Approach

Make processStream handle exactly one request per stream: after emitting the response for the first chunk, the pipeline generator returns instead of looping over the rest of the source. Extra frames a peer queued on the same stream are discarded when the stream closes, so work is bounded to one request per token. This does not regress legitimate traffic — no code path pipelines multiple requests on a single stream, and the one-chunk-per-request framing is already in force.

The alternative (per-invocation rate checks inside the loop) was rejected: nothing pipelines, and mid-stream status signalling is broken on the requester side, which parses the first chunk as status and concatenates the rest as data.

Adds a unit test that drives processStream with three frames on one stream and asserts the handler runs once and the sink receives a single SUCCESS + response pair.

Fixes A-1324

@spalladino spalladino force-pushed the spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the branch from 8f434d3 to 55deb1c Compare July 6, 2026 17:33
@spalladino spalladino added wip Work in progress and removed wip Work in progress labels Jul 6, 2026
@spalladino spalladino enabled auto-merge (squash) July 7, 2026 12:08
@spalladino spalladino force-pushed the spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the branch from 55deb1c to 52b44d8 Compare July 7, 2026 12:13
spalladino added a commit that referenced this pull request Jul 7, 2026
…24554)

A reqresp request payload must fit in a single muxer frame: yamux splits
writes larger than 64KiB (minus the 12-byte frame header) into multiple
frames, each of which arrives at the responder as a separate chunk, and
the responder never reassembles a request from multiple chunks. A
request type growing past that limit would surface as a confusing
decoding error on the responder instead of failing at the sender.

- Assert the payload size in `sendRequestToPeer` before dialing,
throwing a descriptive `OversizedReqRespRequestError` locally. The check
runs before the generic error handling so the remote peer is not
penalized for a local bug.
- Pin `maxMessageSize` on the yamux muxer to its library default (64KiB)
so a dependency upgrade cannot silently change the limit the assertion
relies on.
- Tests: an oversized payload is rejected without dialing the peer and
without penalizing it; a payload at exactly the limit round-trips
successfully.

Related to #24552.

## No current subprotocol can exceed the limit

The limit is 65,524 bytes (64KiB yamux frame minus the 12-byte header).
**No reqresp subprotocol today can produce a request anywhere near it**,
so this assertion cannot fire on legitimate traffic:

- `GOODBYE`: 1 byte (the reason code).
- `PING`: a few bytes.
- `STATUS`: a `StatusMessage` (component versions string, block numbers,
block hash) — tens of bytes.
- `AUTH`: an `AuthRequest` (`StatusMessage` plus a 32-byte challenge) —
tens of bytes.
- `TX`: the request type is `TxHashArray` (4 + 32·n bytes), but this
subprotocol currently has no sender — only the server-side handler
remains.
- `BLOCK_TXS`: the only request that scales with anything. Serialized as
`archiveRoot` (32 bytes) + tx-indices `BitVector` (4 + ⌈N/8⌉ bytes, N =
tx count of the proposal) + tx-hashes commitment (32 bytes) + optional
full-hash vector (4 + 32·H bytes). Pinned-peer and smart-peer requests
send indices only (H = 0). Dumb-peer requests include full hashes but
chunk them to `txBatchSize`, which is always the default 8 in production
(`BatchTxRequester` is constructed without opts, and the option is not
wired to any config). N is capped at deserialization by
`MAX_TXS_PER_BLOCK = 2^16`, so even a maliciously large proposal yields
a worst-case request of ~8.5KB — about 13% of the limit. A realistic
32-tx block produces requests of a few hundred bytes.

Breaching the limit would take a proposal with ~460k txs (which
`BitVector.fromBuffer` rejects at 2^16 anyway) or a `txBatchSize` over
~1,800 with full hashes enabled — neither is reachable today. The
assertion exists so that if a future change makes a request type scale
past the limit, it fails loudly at the sender instead of as a decoding
error at the responder.
@spalladino spalladino force-pushed the spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the branch from 52b44d8 to 3385ef7 Compare July 7, 2026 12:34
@spalladino spalladino merged commit 2a713f9 into merge-train/spartan-v5 Jul 7, 2026
12 checks passed
@spalladino spalladino deleted the spl/a-1324-reqresp-multiple-handler-invocations-per-stream-bypass-the branch July 7, 2026 12:52
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.

2 participants