Repository navigation
Conversation
A tokenless peer could complete the QUIC/TLS handshake, never send Ready, and reconnect as fast as AUTH_HANDSHAKE_TIMEOUT frees its slot, keeping all 32 global pre-auth slots full and refusing every legitimate miner. Bound in-flight pre-auth handshakes per source IP so exhausting the global budget requires many distinct hosts rather than one. Co-authored-by: Cursor <cursoragent@cursor.com>
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: Opus 5.5
The per-IP cap does not close the reported lockout. It keys on a source address that nobody has validated yet. The fix for that is small.
Blocking
-
node/src/miner_server.rs:655-666: the per-IP slot is keyed on an unvalidated source address.
endpoint.accept()yields a quinnIncomingbuilt from a single client Initial packet. At that pointremote_address_validated()is false. The code never checks it and never callsretry(). Both slots are charged right there, and they stay held for up to 10s (line 686) if the handshake never finishes. So the comment at line 80 ("a peer must receive our packets to finish the handshake, so its source IP is genuine") is false at the point where the slot is taken. Two results:- One host on a network that allows source spoofing sends 32 Initials every 10s from 8 forged IPs, 4 per IP. All 32 global slots stay full at about 4 KB/s. This is the same lockout the PR sets out to fix.
- New with this PR: 4 spoofed Initials every 10s carrying a known miner's IP keep that IP at the cap. That miner is refused every time it reconnects. This also gets through the source-IP firewall allow-list that MINING.md offers as an option. Before this PR the same attack took 32 packets and hit every miner. The warn log then names the victim's IP as the offender.
Fix: before taking either slot, do
if !connecting.remote_address_validated() { connecting.retry()...; continue; }. quinn guaranteesmay_retry()is true in that case, and Retry is stateless. The per-IP key is then a proven address and the comment becomes true. The cost is one extra round trip per connect.
Non-blocking
:83: even with genuine addresses, 8 IPv4 addresses (one /29) hold all 32 global slots. Each slot can also be held for about 20s, not 10s. The handshake timeout (line 686) and the Ready timeout (line 760) run one after the other while both guards are held. So "many distinct hosts" means 8. The PR raises the cost of the DoS but does not close it. Consider one shared deadline for handshake plus Ready, and the loopback-default bind listed as a follow-up.:83: miners that share one source IP (a NAT farm, a Docker port proxy, several GPU processes on 127.0.0.1) now share 4 pre-auth slots. The quantus-miner reconnect backoff has no jitter and only resets after auth (crates/miner-service/src/quic.rs), so refused miners retry together and can collide again. The pre-auth window is short, so this needs a near-simultaneous reconnect. MINING.md still says "Multiple miners can connect simultaneously" and does not mention the new limit.:1087: the new test only coverstry_acquire_ip_sloton a local map. Nothing coversacceptor_task. Ifdrop(ip_guard)moved afterserve_authenticated_miner(...).await, the per-IP slot would be held for the whole session and no test would fail.:661: each refused Initial writes awarn!line, at a rate the sender controls. The global branch at line 672 already does this, so it is not new. But one IP now reaches it after 4 slots instead of 32. A counter with a periodic summary line would bound it.:625:if let Some(n) = counts.get_mut(&self.ip)silently skips a state that cannot happen. Log an error (ordebug_assert!) instead of a silent no-op.:660and:671: the IP guard and the semaphore permit are always taken and released together. That gives two near-identical refusal blocks and paireddrops at 695-696 and 717-718. OnePreAuthSlotguard holding both would remove the copy and make it impossible to release only one. The new field initializer is also copied into three struct literals (152, 997, 1082).:74-82,:111-112,:657-659: the same rationale is written three times, with em-dashes and long sentences. One short doc on the constant is enough.- Nits:
remote_ip(655) andremote(685) read the same address twice, so hoist oneremote. Checking the global semaphore first would skip the map insert and remove when the global budget is full.
Checked and fine
The per-IP slot is released on every path: global-refuse continue, auth success, auth failure, handshake error, timeout, and panic. The count cannot underflow. The map stays at 33 entries or fewer. The std Mutex is never held across an .await. IPv6 /64 keying does not matter today because the endpoint binds 0.0.0.0.
Tests were not run locally, because a full node build exceeds the time limit. CI Format, Clippy and the Linux test matrix pass. The macOS test job was still pending.
Verdict: Request changes. The per-IP cap needs QUIC address validation (Retry) before it charges a slot. Without it, the cap does not stop the lockout it targets.
…th slots Address review: the per-IP cap keyed on the unvalidated address of a single Initial, so spoofed Initials could still fill the budget or pin a real miner's IP at the cap. Send a stateless QUIC Retry first and only charge a slot once the peer has proven it receives at that address. Also: fold the global permit and per-IP count into one PreAuthSlot guard, take the global budget before the per-IP map, bound the whole pre-auth path under a single AUTH_HANDSHAKE_TIMEOUT deadline (one 10s hold instead of two back-to-back), debug_assert on the impossible release path, trim duplicated docs, and document the pre-auth limits and Retry round-trip in MINING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks — the blocking point was right, and the recommended fix is in 1 (blocking) — unvalidated source address. Fixed. The accept loop now checks 2 — 8 IPs / ~20s hold. Partially addressed. Both phases now share one deadline: 3 — shared-IP miners. The limit is now documented in 4 — test coverage. Replaced the map-only test with two that exercise the real 5 — warn-log rate. Spoofed Initials now get a silent Retry and never reach the warn, so the attacker-controlled rate is gone for the unvalidated case. A validated peer that holds its cap still logs once per refused connect, bounded by its round-trip rate; left as 6 — Local: |
n13
left a comment
There was a problem hiding this comment.
Verdict: Approve
Reviewer: GPT-6.1 Sol
Reviewed head e611d8e6b6594a4abffa44fe2add347f2c16986e against base aae53e3693ab244c86cf3f10bb39e39500f5c6c2, including both changed files, the earlier review, and Quinn's locked-version address-validation implementation. No blocking findings.
The accept loop now validates the source address before reserving either budget. The combined guard releases both counts on rejection, handshake failure, timeout, and successful authentication; successful sessions release their slot before entering the long-lived handler. QUIC and Ready authentication share one deadline. The documented NAT limit matches the implementation.
Validation:
scripts/fmt.sh --all -- --check, Taplo formatting, andgit diff --check: passed.- A temporary standalone harness compiled the unchanged
node/src/miner_server.rswith the locked Quinn versions, a Tokio re-export, and a SHA-256 shim for the otherwise unrelated Substrate dependency. All 14 existing module tests passed. Three additional real QUIC tests also passed: eight concurrent authenticated sessions from one IP; four stalled pre-auth sessions rejecting a fifth and releasing slots after timeout; repeated wrong-token rejection followed by successful authentication. Total: 17 passed. timeout 60 env SKIP_WASM_BUILD=1 CARGO_TARGET_DIR=.review-target cargo test --locked --offline -p quantus-node miner_serverstopped during dependency compilation at the requested limit; the full node suite was not completed. GitHub CI remains in progress.
Non-blocking follow-up: add the real QUIC accept-loop cases to the repository suite so Retry ordering and release before long-lived sessions remain covered.
The previous CI run failed during ubuntu apt-get (Temporary failure resolving azure.archive.ubuntu.com) before Format ran. No code change; local fmt --check -p quantus-node is clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Problem
On the external-miner QUIC endpoint (
--miner-listen-port), the pre-auth slot (unauth_slots) is acquired before theReady { token }check, and TLS useswith_no_client_auth(). A tokenless peer can therefore complete the QUIC/TLS handshake, never sendReady, and reconnect the instantAUTH_HANDSHAKE_TIMEOUT(10s) frees its slot. The global cap ofMAX_UNAUTHENTICATED_CONNECTIONS = 32bounds how long a slot is held but not how fast one peer can re-take it: ~32/10s = 3.2 handshakes/s keeps all 32 slots full and refuses every legitimate miner. Already-authenticated miners keep their connection (the permit is dropped on success), but any miner that must (re)connect — e.g. right after a node restart — is locked out for as long as the attacker continues.This was reported as a sustained DoS of a mining node. The companion crash issue (RUSTSEC-2026-0342, empty DATAGRAM frames in
quinn-proto≤ 0.11.16) is already fixed onmainvia the bump toquinn-proto 0.11.19; this PR addresses the remaining lockout.Fix
Add a per-source-IP cap on in-flight pre-auth handshakes (
MAX_UNAUTHENTICATED_CONNECTIONS_PER_IP = 4), enforced before the global permit. A peer must receive our packets to finish the handshake, so its source IP is genuine — bounding slots per IP means exhausting the global budget now requires many distinct hosts rather than one. The per-IP slot is released on the same paths as the global permit (auth success and handshake end), so authenticated miners don't count against it.Notes / possible follow-ups
--miner-listen-addrflag defaulting to loopback, a client certificate pinned like the server's, or a shorter Ready deadline — would move the credential check in front of the slot grab entirely, but each changes deployment or protocol behavior and is left out of this PR.Test
per_ip_preauth_cap_bounds_one_ip_and_releases_on_dropcovers the cap, per-IP isolation, and release-on-drop.SKIP_WASM_BUILD=1 cargo clippy --locked -p quantus-node --testsclean;cargo test -p quantus-node miner_server— 13 passed.Made with Cursor