Skip to content

feat(network): v2 peer connection, handshake, and handshaker service - #11276

Draft
arya2 wants to merge 1 commit into
p2p-v2-3-quic-transportfrom
p2p-v2-4-peer-connection
Draft

feat(network): v2 peer connection, handshake, and handshaker service#11276
arya2 wants to merge 1 commit into
p2p-v2-3-quic-transportfrom
p2p-v2-4-peer-connection

Conversation

@arya2

@arya2 arya2 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fourth PR of the five-PR stack implementing the draft version 2 Zcash P2P network protocol (zcash/zips#1344). This PR adds the peer connection layer: the application handshake and the connection task that map Zebra's internal request/response protocol onto v2 streams.

Solution

Adds zebra-network/src/peer/v2/:

  • Handshake (handshake.rs): the initiator opens the dedicated handshake stream and the peers exchange init records; version negotiation takes the minimum of the advertised versions, gated on the epoch minimum. Self-connection detection uses the shared HandshakeNonces policy from the first PR of the stack (per-transport nonce set, never evicted on failed handshakes).
  • Connection task (connection.rs): each outbound internal request opens its own request stream, so requests run concurrently and are cancelled and timed out per stream, with the response-to-request echo check single-sourced for blocks and transactions. Inbound request streams are decoded with bounded buffers, served by the inbound service, and answered on the same stream. Announcements use long-lived unidirectional streams, with outbound announcements dropped rather than queued when the transport applies backpressure.
  • Compact block serving: short transaction ID tables are precomputed when a compact block is sent, so SHORTID follow-ups are pure lookups; tables are skipped for peers that requested full transaction IDs.
  • Address handling: address requests are answered through the shared bounded-random-sampling cache policy (Zebra should limit the number of addresses it uses from a single Addrs response, to avoid address book takeover #1869), and get-addr is only answered on inbound connections to impede fingerprinting.
  • Unresponsive peer detection: the transport answers keep-alives locally, so heartbeats cannot detect a peer whose application never answers. The connection disconnects after three request timeouts during which the peer sent no response at all — concurrent timeouts overlapping a response do not count.
  • Handshaker service (service.rs): produces the same peer Client type as the legacy transport, so v2 peers plug into the peer set unchanged.

The modules are #[allow(dead_code)]-scaffolded until the peer set integration PR consumes them; the attribute is removed in the final PR of the stack.

Tests

  • Loopback QUIC pair tests for the handshake (version negotiation, obsolete version rejection, self-connection detection, duplicate nonce handling) and for the full service (request round trips over real QUIC connections, pushed transaction serving, compact block and SHORTID serving, oversized response scoring, unresponsive-peer disconnection).
  • cargo clippy -p zebra-network --all-targets warning-free; full zebra-network suite passes.

Specifications & References

Follow-up Work

  • Final PR of the stack: p2p-v2-5-peer-set-integration.
  • High-bandwidth compact block announcements (announce = 1) and full-ID mode are not requested yet.
  • The stringly PeerError::V2Protocol/V2Internal variants should become a structured error enum in a follow-up.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Claude Code was used to implement the connection layer and tests, run the test/lint verification, and draft the commit messages and this description; the changes were reviewed by the author.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

@v12-auditor

v12-auditor Bot commented Aug 17, 2026

Copy link
Copy Markdown

Note

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

Open the full results here.

FindingSeverityDetails
F-232221 🟡 Medium
Compact block fully decoded before rejection on full-block request

The outbound BlocksByHash path always requests BlockFormat::Full, and correctly rejects a BlockResponseEntry::Compact answer as a protocol violation. However, the rejection happens only after BlockResponseEntry::read has already consumed and materialized the entire attacker-controlled compact block: on seeing the RESULT_COMPACT_BLOCK tag, read immediately calls CompactBlock::read, which loops over up to MAX_COMPACT_BLOCK_TX_COUNT (65,536) prefilled transactions, each read with the per-element 2 MiB bound and each retained in the returned prefilled vector. The compact-block parser bounds element count and individual element size but imposes no aggregate byte limit, so the nominal ceiling is far larger than one block. The dispatch design couples reading the result tag with decoding the selected object, so the caller cannot check that the result kind matches the requested format until decoding has completed.

F-232225 🟡 Medium
Serial untimed handshake stream discovery lets one silent stream deny handshakes

handshake::respond discovers the handshake stream with a serial loop: it calls connection.accept_bi(), then fully awaits record::read_u8(&mut recv) on that stream before it can accept the next one. record::read_u8 delegates to read_exact_or_incomplete, which awaits read_exact with no timeout, so it resolves only when a byte arrives, the stream is reset, or the stream is finished. A remote-opened bidirectional stream that is visible but carries zero readable bytes and no FIN therefore parks the loop, and the responder never reaches params.local_init.write or the remote init read even when the real handshake stream's bytes have already arrived. The only bound is the 3s whole-handshake timeout wrapper in the service. The module's own post-handshake code applies exactly the missing defense: read_stream_type_or_fail wraps the identical read in a timeout precisely so an idle stream cannot pin its reader task, and serves each stream in its own task rather than serially.

F-232230 🔵 Low
Full-ID compact blocks cannot serve their own missing transactions

When a peer advertises full_ids = true, build_compact_block_response returns the compact block early and deliberately retains no transaction lookup table, on the reasoning that such a peer has no nonce to compute short IDs from and will fetch missing transactions by WTXID instead. However, the get-tx serving path can satisfy those WTXIDs only from the per-connection pushed-transaction cache or the internal TransactionsById service, and the production inbound service queries only the mempool, not mined block state. Transactions from an older block, or from a newly accepted block after mempool removal, therefore receive NotFound even though Zebra just sent their IDs as part of its own compact-block representation.

F-232232 🟡 Medium
Handshake stream reset silently disables protocol monitoring

The v2 design states that finishing or resetting the handshake stream signals intent to disconnect. monitor_handshake_stream honors only the finish half: a clean finish is detected as Ok(None) and correctly sets ConnectionClosed and closes with NO_ERROR. A reset of the peer's send half surfaces differently, because read_first_byte uses plain read and quinn's ReadError::Reset is absorbed by the #[from] std::io::Error conversion into WireError::Io, which lands on the Err(WireError::Io(_)) => break arm and exits the monitor silently with no error-slot update, no close, and no misbehavior score. The rest of the connection remains fully operational, so from that point the handshake stream is no longer monitored at all and the duplicate-init protocol check the monitor exists to enforce becomes unreachable. The code already knows how to discriminate this error shape, since stream_reset_code walks WireError::Io sources for quinn::ReadError::Reset, but that helper is used only on the outbound request path.

F-232233 🟡 Medium
Unbounded unknown-kind records with pre-read allocation

Both the handshake read path and the handshake-stream monitor accept an unlimited number of unknown-kind records with no cap, no rate limit, and no misbehavior score. InitRecord::read loops on record::read_record and continues on every HandshakeRecord::Unknown, and monitor_handshake_stream likewise logs and continues. This forward-compatibility rule stands in contrast to every other peer-controlled quantity in the v2 layer, which carries an explicit bound and a misbehavior penalty. Compounding it, read_exact_payload allocates vec![0u8; len] before reading the body, where len is peer-declared and bounded only by MAX_RECORD_PAYLOAD_LEN of 2 MiB, and read_record_timeout starts its clock only once a record has begun.

F-232235 🔵 Low
Evict-before-insert shrinks caches and drops unrelated entries

Two per-connection caches perform capacity eviction before determining whether the insertion will actually add a new key. In the Request::PushTransaction arm, while pushed.len() >= PUSHED_TRANSACTION_CACHE_LIMIT { pushed.shift_remove_index(0) } runs before an unconditional insert, so re-pushing a transaction already present in a full cache evicts the oldest unrelated transaction and then merely replaces the existing key, leaving one fewer entry. build_compact_block_response has the identical pattern for sent_compact_blocks: re-requesting a block already cached but not at index zero removes the oldest different block and then replaces the existing value without increasing length. Because IndexMap::insert replacement also does not move the entry to the most-recent position, neither map consistently represents the most recently used items.

F-232238 🟡 Medium
Handshake discovery swallows errors treated as fatal elsewhere

In the responder's discovery loop, any error from record::read_u8 is discarded by Err(_) => drop((send, recv)), with a comment assuming the stream was reset before its type byte arrived. That arm is not limited to resets: read_exact_or_incomplete maps a stream finished with zero bytes to WireError::Protocol, so a peer that opens a bidirectional stream and immediately finishes it without a type byte lands in the same swallowed arm and the loop simply iterates. The post-handshake path classifies exactly this condition as a peer violation and fails the connection, since read_stream_type_or_fail calls fail_protocol when the error is not a transport-level stream failure.

F-232240 🟡 Medium
Global timeout accounting both masks and overcounts unresponsive peers

The v2 unresponsive-peer control derives all timeout decisions from the connection-global responses_received counter and shared consecutive_timeouts counter, even though requests execute concurrently. Any response received during a request's timeout window causes that request's timeout to be ignored, and record_response resets the entire shared timeout count; OutboundError::NotFound takes this response path as well. Consequently, a peer can prevent the control from accumulating timeouts by producing one qualifying reply in overlapping windows while allowing other requests to time out. Conversely, when several requests share a window with no completed response, each observes the same unchanged snapshot and increments the counter, so MAX_CONSECUTIVE_REQUEST_TIMEOUTS simultaneous timeouts disconnect the peer after one stall. Timeout accounting must instead distinguish response and timeout progress for individual requests or deduplicated timeout windows, so unrelated responses cannot mask unanswered requests and one concurrent stall is counted only once.

And 13 more auto-invalidated findings.

Analyzed six files, diff 8cf4212...73ab5f6.

@mergify

mergify Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 1 of 1 protections blocking · waiting on ⛓️ dependency

Protection Waiting on
🔴 ⛓️ Depends-On Requirements ⛓️ dependency

🔴 ⛓️ Depends-On Requirements

Waiting for

This rule is failing.

Requirement based on the presence of Depends-On in the body of the pull request

Carry the version 2 wire formats over QUIC, and connect them to the rest
of the node.

The transport opens one endpoint on the UDP port of the configured
listen address, so a node serves both protocols on the same address: QUIC
uses UDP where the legacy protocol uses TCP. Peers are identified by an
ephemeral self-signed certificate, the ALPN identifier separates
networks, and 0-RTT is never negotiable, so a replayed early-data packet
cannot reach the application.

The connection service maps the stream layer onto the peer set: each
request stream carries one request and its response, announcement streams
carry blocks, transactions and addresses, and both sides end up producing
an ordinary `peer::Client`. Inbound admission, connection limits, per-IP
limits, bans, and handshake nonces are shared with the legacy transport
rather than duplicated, so a v2 peer is subject to the same policy.

`ConnectionInfo` now records the peer's actual handshake data per
transport, instead of synthesizing a legacy `version` message for v2
peers.

Both options are off by default: `network.v2_listen` serves the listener,
and `network.initial_v2_peers` names peers to dial over QUIC. With them
off, nothing about the node changes.
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.

Tracking: v2 P2P protocol (QUIC) and known-hash sync with spentness hints

1 participant