Skip to content

feat(network)!: activate bounded paired GetBlocks serving - #945

Open
czarcas7ic wants to merge 101 commits into
mainfrom
adam/getblocks-paired-activation
Open

feat(network)!: activate bounded paired GetBlocks serving#945
czarcas7ic wants to merge 101 commits into
mainfrom
adam/getblocks-paired-activation

Conversation

@czarcas7ic

@czarcas7ic czarcas7ic commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Motivation

Native GetBlocks serving needs bounded ownership through storage, queued results, and writes while downloads continue on the same connection. This activates the refactor from #892 using the owned storage, service sessions, and request ownership now merged into main.

Solution

Declare the paired data and request streams through the service-session transport now on main. Keep the one-slot request queue, download-owned request deadline, and 32-second data-write deadline. Serve sequentially with one response per authenticated identity and 64 per node, backed by the owned state-read API. Remote closes and write timeouts retain download cooldowns and repeated-stall disconnects after buffered responses are validated, including when closure precedes coordinator startup. Connection shutdown and fatal request errors interrupt pending validation. Local body backpressure stays neutral. Inbound setup preserves an outbound slot, and teardown removes only its exact session generation. The previous native version is no longer selected. Other services and legacy fallback remain available.

Testing

At 49963c0c4, the merge with main preserves paired serving and includes the final batched ownership check. All 510 selected block-sync, transport, and handler tests pass without retries on Rust 1.97. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass. The long transport qualification gates were not rerun for this merge.

At d682a81cf, all 429 selected block-sync, session, and transport tests plus all eight paired-download checks pass without retries. The startup-cancellation regression fails before its fix. Connection shutdown and fatal request errors still interrupt blocked validation. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass.

At d6831f340, all 113 selected handler and block-sync session tests pass without retries, including duplicate rejection, capacity accounting, cleanup, reopening, and paired downloads. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass.

Earlier qualification at 06fdcce37 passed 3,484 tests under the workspace CI profile, excluding three macOS address-binding tests. Workspace Clippy with warnings denied, formatting, and changelog checks pass. All six transport qualification gates passed at that commit, including twenty actual session replacements on one connection with 50 ms RTT and 1% packet loss. Those downloads took 200–205 seconds against the 240-second limit. Twenty saturation, cleanup, and retry cycles also pass. Five-run comparisons on matching Iroh 1.1 dependencies and download policy retain 98.4–99.1% of the original serving path's useful throughput. Peak process memory across qualification is below 233 MiB. Two live-network sync tests time out locally and are excluded by the CI profile. Earlier measurements remain in the archived results. Prepared with Codex assistance.

Changelog

Includes the serving security fragment. The serving design explains behavior, limits, and qualification criteria. The stream specification defines the wire contract.

@v12-auditor

v12-auditor Bot commented Sep 9, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found nine issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-271412 🟠 High
QUIC buffers bypass response memory bounds

FrameGuard retains serving ownership only until SendStream::write_all accepts a frame; the code explicitly states that this is not delivery acknowledgement and that QUIC owns the bytes afterward. Once the terminal frame's local write completes, the serving loop can admit another request from the same peer and resets the per-response byte accounting. A malicious QUIC peer can advertise sufficient flow-control credit, allow several repeated block responses to enter the local send buffer, and then leave the connection just below the point where another write would block and trigger the 32-second timeout. Repeating this across authenticated connections moves cumulative response bytes outside the one-per-peer and 64-node permit accounting while retaining them in QUIC. The configured 32 MiB per-connection send window multiplied by the default connection limits makes the resulting process-memory exposure materially larger than the serving regulator's nominal bound.

F-271413 🟠 High
Write stalls bypass repeated-peer penalties

A malicious peer can stop reading served block data until a paired-data write reaches its timeout while separately advertising a missing height and leaving the victim's outbound download unanswered. The special PairData timeout branch retires the pair without publishing the remote_close cause used by the download routine's stall policy. Shared pair cancellation then makes PeerRoutine::run return through its clean cancellation branch, and remotely_closed() remains false, so no_progress_stall is never invoked. The outstanding download work is requeued by Drop, but no 180-second cooldown or repeated-stall state is recorded. Because the pair is configured to reopen on the same live connection, each replacement is admitted as a fresh generation and the peer can repeat the reset indefinitely.

F-271414 🟠 High
Unsolicited blocks starve peer deadlines

The changed peer loop unconditionally polls inbound frames before its timeout arm in a biased tokio::select!. For every Block frame it reserves capacity from the shared sequencer-input channel and fully deserializes the block before correlating it with an outstanding request. An authenticated peer can advertise a broad servable range and continuously send syntactically valid blocks at unrelated heights inside that range; ignore_servable_range_response then silently drops them without reporting or disconnecting the peer. Keeping the inbound queue continuously ready prevents handle_deadlines from expiring the peer's outstanding above-floor requests or invoking no-progress handling. The default 128-frame queue can retain roughly 256 MB of maximum-size serialized blocks per peer, and multiple identities multiply both the memory and parsing load.

F-271415 🟠 High
Sybil peers monopolize database reads

Serving admission limits only instantaneous ownership: a valid request takes one per-identity slot and one global slot, and the same sequential task immediately accepts another request after the terminal frame releases those owners. The policy explicitly allows a completed range to be requested again and maintains no request history, cooldown, duplicate-range check, or cumulative work budget. Each production request becomes a blocking state lookup and attacker-selected random heights defeat locality. The transport's generic 2,048-message-per-second bucket is per connection and far above the replacement rate needed to keep a storage read continuously active. Consequently, 64 authenticated Sybil identities can continuously refill all default node serving slots while remaining protocol-valid.

F-271416 🟠 High
Half-pairs starve outbound block sync

The first valid inbound role of a paired block-sync session reserves a service-global pending permit and retains it until the companion role arrives or the prelude deadline expires. The pending semaphore is shared between inbound and outbound setup and defaults to only 32 slots, while authenticated node identities are self-generated. An attacker can establish 32 connections, send one valid pair role with a nonzero pair ID on each, and withhold the companion. Expiration is stream-local, leaving the connection available to submit another half, and reservations use immediate try_acquire without a fair outbound share. Maintaining these half-pairs at a low stream-open rate prevents every proactive outbound block-sync pair from acquiring setup capacity despite free outbound session slots.

F-271417 🟠 High
Pre-readiness teardown leaks peer entries

add_peer eagerly inserts a fresh generation into PeerRegistry and publishes the same session in CurrentSessions, but a teardown before reactor readiness removes only the current-session record. Because the watch channel coalesces updates and snapshots only the latest table, an insert followed by removal can be observed only as an empty snapshot. The reactor then has neither a prior installed peer nor a current session carrying the unseen generation, so it cannot call exact-generation remove_session. Connection cleanup clears conn_id but also leaves the registry entry present. Repeating this race with fresh authenticated identities accumulates stale registry entries indefinitely.

F-271418 🟡 Medium
Replacement sessions inherit stale readiness

When admit_session replaces an existing peer generation, it clears outstanding work and changes the generation and connection but preserves the predecessor's received_status, servable range, advertised caps, and slot/RTprop diagnostics. The new PeerRoutine simultaneously starts with no received Status, minimum ranges, and a fresh congestion window. Other routines nevertheless make floor-carrier decisions from the preserved registry facts and can prefer this not-yet-ready replacement. A peer can establish favorable old-generation status and latency state, rapidly replace the pair, and withhold Status on the new generation. Repeating the replacement makes live peers defer floor work to a routine that cannot issue any request.

F-271419 🟡 Medium
Responses can mix canonical chain views

The serving task authorizes a range using one copied BlockSyncStatus, but passes only heights and byte/count limits to storage; the advertised tip_hash is not bound to the read. The state service later clones latest_best_chain() independently and performs each height lookup against that clone with fallback to the concurrently live finalized database. A reorganization between authorization and read can therefore return a different fork than the advertised Status, and a multi-height read can cross from a stale chain clone into a changed database view. The serving loop checks only the tuple height, count, and reported size before encoding each block. It does not verify block hash, coinbase height, parent linkage, or membership in the Status snapshot.

F-271421 🔵 Low
Stale admission deletes successor claim

After inserting a session, add_peer releases the current-session lock and later removes the peer's session-gap claim without checking the owning connection or generation. A superseded add_peer continuation can pause after installing generation A, while generation B replaces it and then tears down. finish_session for B installs a gap claim that keeps B's live connection owned during ordered-stream reopen backoff. When stale continuation A resumes, its unconditional claims.remove(&peer_id) deletes B's claim. The newer connection then appears to have neither an active session nor a reopen-gap owner.

And one more auto-invalidated finding.

Analyzed 37 files, diff 8a42657...b8fa452.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-11T20:39:29.949345Z 49963c0 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@czarcas7ic
czarcas7ic force-pushed the adam/getblocks-paired-activation branch from 36e649b to ee0ae5a Compare September 10, 2026 05:09
@czarcas7ic czarcas7ic changed the title feat(network): activate bounded paired GetBlocks serving feat(network)!: activate bounded paired GetBlocks serving Sep 10, 2026
Base automatically changed from adam/getblocks-request-ownership to main September 11, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocksync anything related to blocksync C-security msg-reg Peer message regulation project network breaking something that must be included with a version upgrade to a networking component such as a reactor v2 p2p everything that touch the new p2p stack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants