Skip to content

feat(network)!: add bounded service sessions and request resources - #943

Merged
czarcas7ic merged 20 commits into
mainfrom
adam/getblocks-paired-transport
Sep 11, 2026
Merged

feat(network)!: add bounded service sessions and request resources#943
czarcas7ic merged 20 commits into
mainfrom
adam/getblocks-paired-transport

Conversation

@czarcas7ic

@czarcas7ic czarcas7ic commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Motivation

A blocked P2P service must retain its charged resources without stopping other services on the connection. This transport chunk follows #942 and includes the service-session generalization from #956.

Solution

Negotiate complete persistent sessions from service declarations, with independent bounded queues, service-owned write deadlines, and shared admission resources held through cleanup. Record remote closes and write timeouts before a failed request claim can cancel the session, preserving download cooldowns and repeated-stall disconnects. Buffered responses wait for decode capacity and are validated before charging a stall. Remote closure cannot hide malformed frames. Local cancellation and body backpressure stay neutral, and connection shutdown interrupts pending validation. Dropping all application handles drains every member before retiring the session. Retained receivers and sender clones keep it alive even if incoming traffic arrives after an application receiver is dropped. The reader discards those frames after transport checks, preserving real peer-close detection. Reject duplicate offers while a remote session is active or retiring before reserving service capacity. If a retry races an abandoned incomplete session, discard both offers and use the existing cooldown so other services stay connected. Production block sync retains its current protocol until #945 activates the paired layout.

Testing

The malformed-frame backpressure regressions fail before the fix. At 596f91d78, all 371 selected block-sync, session, and transport tests pass without retries. Network all-target Clippy with warnings denied, formatting, Markdown lint, and changelog checks pass.

The duplicate-offer regression fails before the fix at 35769d8bc and passes at 253fa4e27. All 24 service-session tests 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. Prepared with Codex assistance.

Changelog

Updated the transport fragment and service-session guide. Package versions follow the merged Iroh upgrade.

@v12-auditor

v12-auditor Bot commented Sep 9, 2026

Copy link
Copy Markdown

Note

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

Open the full results here.

FindingSeverityDetails
F-271391 🟠 High
Missing block source type breaks builds

ZakuraHeaderSyncDriverStartup::block_range_source is declared as Arc<dyn super::BlockRangeSource> in the changed handler. From zakura::handler, that path must resolve to crate::zakura::BlockRangeSource. No trait, struct, type alias, or re-export with that name exists anywhere in the repository; the field declaration is the only occurrence. Rust must resolve the type while compiling the library even if no runtime path constructs the struct. The zakura-network crate therefore fails to compile.

F-271392 🟠 High
Startup constructor omits required field

The diff makes block_range_source a required field of ZakuraHeaderSyncDriverStartup. The repository's sole production struct literal initializes all pre-existing members but omits this new field. It has neither a struct-update expression nor a Default implementation that could provide the value. Once the missing BlockRangeSource type is added, Rust will independently reject this constructor with a missing-field error. The production startup adapter therefore remains uncompilable even after the first name-resolution error is fixed.

F-271393 🟠 High
Undefined stream constant breaks builds

stream_kind_label now matches on crate::zakura::ZAKURA_STREAM_BLOCK_REQUESTS. No constant with that name is declared or re-exported anywhere in the repository; the changed match arm is its only occurrence. The implemented native block protocol defines ZAKURA_STREAM_BLOCK_SYNC instead. Rust must resolve every constant path used in a match pattern at compile time. This unresolved symbol prevents the zakura-network library from compiling.

F-271394 🟡 Medium
Slow pair setup stalls connection reactor

An authenticated peer can keep one connection's reactor unavailable by repeatedly opening a negotiated ordered-pair stream, supplying the ordinary prelude slowly, and withholding the eight-byte pair identifier. serve_connection awaits admit_bi_stream directly inside the sole connection.accept_bi() select branch, so no other reactor branch is polled until peer-paced setup completes. The ordinary prelude's fixed and frame-cap portions each get a separate prelude_timeout, and the new pair identifier gets a third full timeout; under defaults, one stream can occupy the reactor for almost nine seconds. After reset, a prequeued stream can repeat the sequence. The stream-open bucket does not stop this because it refills while the reactor is blocked and the attacker consumes far fewer than the default 32 tokens per second.

F-271395 🟡 Medium
Incomplete pairs monopolize service capacity

For a finite-capacity ordered-pair service, an authenticated peer can occupy a service slot without supplying a usable stream half. admit_bi_stream calls reserve_or_share, which invokes reserve_ordered_session, before reading the peer-controlled eight-byte pair identifier. Withholding those bytes retains the resource owner for the full timeout; supplying the ID retains it in PendingPair until the companion deadline. Once released, a prequeued offer can immediately reacquire the slot. The default three-second setup timeout is far slower than the 32-per-second open-token refill, so the general open limiter never imposes a meaningful cooldown.

F-271398 🔵 Low
Regression test indexes missing stream

The changed regression test constructs the real BlockSyncService, reads streams()[0], and then unconditionally reads streams()[1]. Production BlockSyncService::streams returns block_sync_streams(), which is backed by a one-element [Stream; 1]. The test therefore panics on an out-of-bounds slice index before reaching its registry assertions or network setup. This remains a deterministic test failure after the separately reported undefined stream constant is corrected.

F-271399 🟡 Medium
Missing request limit permits oversized allocations

The changed test expects the block-request role to declare a nine-byte type-2 payload limit, but production BlockSyncService does not override Service::message_payload_limits. The registry therefore receives the trait default empty slice. read_frame_with_types only tightens the stream cap when a matching entry exists; with no entry it allocates and reads the full peer-declared payload up to the approximately 3 MiB block-sync stream cap. A type-2 frame can thus carry a valid nine-byte GetBlocks prefix plus megabytes of padding, all of which are allocated and read before the codec rejects trailing data.

F-271400 🔵 Low
Production block sync bypasses paired isolation

The changed handler implements generic paired setup and pair-local write policies, and its new production-facing test assumes block sync has data and request roles. Actual BlockSyncService still declares only stream kind 6 and does not override ordered_stream_pair, so registry lookup always returns None. The handler consequently invokes spawn_single, creates no companion role, and runs block sync under OrderedWritePolicy::Standalone. The custom PairService tests exercise only synthetic generic machinery and do not activate it for production block sync.

F-271403 🟠 High
Ping requests amplify memory before validation

The built-in legacy request service inherits empty message_payload_limits and no message_types allowlist while its request/response stream accepts frames up to the 1 MiB control-frame cap. A remote Ping header can therefore declare almost 1 MiB of payload, which read_frame_with_types allocates before LegacyRequestFrame::decode_frame can enforce that Ping payloads are empty. By sending all but the final byte, the peer keeps each allocation live until the long payload timeout. The open limiter still permits enough concurrent streams to repeat this up to the per-connection stream ceiling.

F-271404 🟡 Medium
Frame phases reset the read deadline

read_frame_with_types does not enforce one deadline after the first frame byte, despite its documented contract. It gives the remaining seven header bytes a full read_timeout, then read_frame_payload starts a second full timeout after parsing and allocation. One-shot request streams also receive a separate full timeout while waiting for the first byte. A peer can make progress just before each boundary and multiply the intended per-frame lifetime.

F-271405 🔵 Low
Request workers ignore message allowlists

The new Service::message_types contract promises pre-allocation rejection of message types not accepted on a stream role. Admission stores the service's allowlist in StreamWorkerContext, but request_stream_worker calls the read_frame wrapper, which always passes None to read_frame_with_types. No later transport or registry check restores the allowlist. Custom request/response services therefore receive types they explicitly excluded through the new API.

F-271406 🟡 Medium
Tiny frame caps tear down sibling services

An authenticated peer controls StreamPrelude.max_frame_bytes, and admission copies its minimum directly into outbound_frame_cap without checking that it can carry a frame header or mandatory protocol message. The peer can establish healthy sibling services, then open discovery or a paired role with a cap such as one byte. Discovery immediately queues non-empty hello/query frames; encoding against that cap fails deterministically. The worker treats this peer-induced local encode failure as a generic ordered_write_error and cancels the shared connection token.

F-271407 🟡 Medium
Reopen race misclassifies valid sessions

OrderedSessionState.remote_session_id is cleared only after the old worker fully ends and its asynchronous OrderedSessionExit is selected by the connection reactor. A remote endpoint entitled to reopen an ordered service can close/reset the old stream or pair and immediately open the replacement. QUIC acceptance of the new stream is not ordered after old-worker teardown, and a pair delays its single exit until both role workers end. If the replacement is admitted first, the stale remote ID causes the handler to classify the valid next generation as a duplicate and cancel the entire connection.

F-271408 🟡 Medium
Full pairs enable unmetered stream churn

A peer can negotiate max_open_streams = 2 and establish a two-stream ordered pair, consuming both permits in the connection-wide application semaphore. It can then continuously open additional QUIC bidirectional streams. Each is rejected at the semaphore check before open_limiter.try_take, so none consumes stream-open budget. Because the reactor uses a biased select with accept_bi before the sibling outbound-work branch, a maintained backlog of these uncharged streams repeatedly wins selection. The QUIC transport's remote stream allowance is configured from the larger local hard maximum rather than the lower negotiated application limit.

F-271409 🔵 Low
Destructure block-sync test constructor result before wrapping

The discovery test assigns the complete result of BlockSyncService::new_for_test(...) to block_sync and wraps that value in Arc. The constructor returns (Self, mpsc::Receiver<BlockSyncEvent>), so this creates Arc<(BlockSyncService, mpsc::Receiver<BlockSyncEvent>)> rather than Arc<BlockSyncService>. Consequently, the value cannot satisfy the Arc<dyn Service> expected by set_connection_owners. The tuple-backed Arc also does not expose BlockSyncService methods such as add_peer, causing an additional compilation error. Destructure the constructor result first, retain or explicitly ignore the event receiver as appropriate, and wrap only the returned service in Arc.

And one more auto-invalidated finding.

Analyzed 10 files, diff b9968af...ae949e5.

@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-11T19:40:12.908974Z 2e67320 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.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@chatgpt-codex-connector

This comment was marked as resolved.

@czarcas7ic
czarcas7ic marked this pull request as ready for review September 9, 2026 23:49

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9549ca2c86

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2437 to +2440
let opening_stream_count: usize = ordered_streams
.iter()
.map(|stream| self.registry.ordered_stream_pair(*stream).map_or(1, |_| 2))
.sum();

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 Badge Exclude retired roles from the queue split

When a peer negotiates the two-role pair plus a service whose demand is Retire, and advertises max_inbound_queue_depth = 2, this active-role count is 2 but the following queue guard still compares the limit with negotiated_ordered_streams.len() (3), closing the connection before the usable pair starts. Fresh evidence in this revision is that opening_stream_count now excludes retired sessions for the stream-limit check, while queue validation and queue_split_stream_count still include them; derive those from the non-retired role set as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This also happens on main, so we'll handle excluding retired services from the queue split in a separate follow-up.

Comment thread crates/zakura-network/src/zakura/handler.rs
@chatgpt-codex-connector

This comment was marked as resolved.

@czarcas7ic
czarcas7ic added this pull request to stack #954 September 10, 2026 02:25
@czarcas7ic
czarcas7ic requested a review from a team September 10, 2026 02:26
chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@czarcas7ic czarcas7ic changed the title feat(network): add paired transport and guarded request resources feat(network)!: add paired transport and guarded request resources Sep 10, 2026
@czarcas7ic czarcas7ic changed the title feat(network)!: add paired transport and guarded request resources feat(network)!: add bounded service sessions and request resources Sep 10, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@evan-forbes evan-forbes added msg-reg Peer message regulation project v2 p2p everything that touch the new p2p stack network breaking something that must be included with a version upgrade to a networking component such as a reactor labels Sep 10, 2026
@evan-forbes

Copy link
Copy Markdown
Contributor

forwarding ai audit

 In zakura.audit-pr943/crates/zakura-network/src/zakura/handler.rs:4197, incoming traffic after the application drops its receiver cancels the entire session. That cancellation
  can:

  - Kill a session despite a retained sender.
  - Reset another stream before its queued writes finish.

  Two real QUIC regression tests reproduce these outcomes. All 447 existing selected tests passed, so current coverage misses this case.

  The shared ownership and independent queues look justified. The weakness is lifecycle coordination across reader cancellation and writer draining.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

Base automatically changed from adam/getblocks-owned-storage to main September 11, 2026 18:33
@czarcas7ic
czarcas7ic force-pushed the adam/getblocks-paired-transport branch from 596f91d to a79e041 Compare September 11, 2026 19:25
@czarcas7ic
czarcas7ic merged commit 09b79c9 into main Sep 11, 2026
48 checks passed
@czarcas7ic
czarcas7ic deleted the adam/getblocks-paired-transport branch September 11, 2026 19:49
evan-forbes added a commit that referenced this pull request Sep 12, 2026
Main's #943 added Service::message_payload_limits. The transport checks
those limits from the frame header before it allocates a payload, which
is the job this PR's MessagePayloadCap did. The two mechanisms conflicted
in handler.rs, transport/service.rs, transport/mod.rs, and
block_sync/service.rs.

Take main's mechanism and drop this PR's copy: the MessagePayloadCap
type, the Stream::message_payload_caps field, the read_frame parameter,
the registry cap validation, and their tests. Main's
message_payload_limits_apply_before_payload_reads covers the same
header-time rejection.

Keep this PR's GetBlocks limit by declaring it through the new hook:
BlockSyncService::message_payload_limits returns
(MSG_BS_GET_BLOCKS, GET_BLOCKS.payload_cap) for the block-sync stream.
The declaration test now asserts that declared limit.
evan-forbes added a commit that referenced this pull request Sep 12, 2026
handler.rs conflicted in three places:
- The imports take main's list, add this PR's control-handshake items,
  and drop the inline-handshake items that control_handshake.rs now owns.
- The helper block keeps main's new read_frame_payload and #[cfg(test)]
  write_ordered_frame. It drops read_control_payload and
  write_control_payload, which this PR moved into control_handshake.rs.
  Main did not change either helper.

Main's #943 and #968 changed types that the handshake testkit uses:
- scenario.rs sets the new ZakuraLocalLimits::nat_traversal field to
  false, the production default.
- runner.rs classifies the new SessionFull error as a local resource
  rejection, and InvalidMessageType and InvalidServiceSession as peer
  violations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-feature 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