Repository navigation
feat(read): read chunks through a get-or-closer lookup (ADR-0020) - #221
mickvandijke wants to merge 9 commits into
Conversation
dirvine
left a comment
There was a problem hiding this comment.
Changes requested — consume a size-bounded closer-peer decoder
ant-core/src/data/client/kad_get.rs:106–115 passes untrusted peers directly to the new core decoder. The exact core revision pinned at root Cargo.toml:11 lacks the 65,536-byte pre-deserialisation limit; the outer chunk message permits up to 5 MiB. A local exact-head probe accepted 1,050,035 bytes containing 10,000 nodes. No crash or memory exhaustion was demonstrated.
This is the downstream exposure of the same P2 finding in saorsa-core#173, not a separate read-engine bug. Fix the public decoder in core, update this PR's core pin and lockfile (and node pin as the stack advances), then rerun focused and real-transport E2E tests. A retryable client-local size guard can mitigate this caller, but the public decoder still needs its own invariant.
No other introduced blocker was established in the read engine. Legacy fallback, content verification and native/browser separation were reviewed.
Reviewed commit: 7bc58ccb70081b8c5c911cc87a821108d8aadcf4.
Review method: six substantive seats across the linked set, including a completed z-ai/glm-5.2 API review recovered from a worker that timed out after saving its response; coordinator source checks and local execution. Initial functional/security/test seats recommended approval. The coordinator probe, independent adjudicator, API seat and replacement concurrency/release seats corroborate the missing decoder bound. This is not unanimous approval. The API seat recommended client approval with concerns; the coordinator withholds it because the exact client pin still exposes the defect.
Local verification (macOS, rustc 1.96.0): core closer-peers 3 passed and DHT-manager regression 143 passed; protocol all-features 108 passed; node get-or-closer unit tests 3 passed and real-transport E2E 1 passed; client kad_read unit tests 10 passed and real-transport E2E 1 passed. Formatting passed in all four repositories. No production/testnet deployment or merge performed. Draft git pins were verified with cargo metadata --locked; this is not published dependency closure. ADR-0020 remains Proposed.
The native kad read passes each get-or-closer `Closer` payload to saorsa-core's decode_closer_peers. At the pinned revision that decoder parsed payloads of any size the 5 MiB chunk envelope allows. A responder could make the client deserialize and normalize about 1 MB of duplicate nodes per answer. saorsa-core @ fd37775 rejects payloads over the 64 KiB FIND_NODE limit before parsing them. The rejection surfaces as Error::Protocol, which the read treats as retryable: only that peer's step fails, and the lookup goes on without it. ant-node moves to ad88647, which pins the same core. Reported in review of #221. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. This is addressed in two commits. 25d7c5a: bounded decoder.
7d9a248: bad content drops only that peer. Tracing the rest of the answer path turned up a larger exposure in the same request.
Checks: |
a5fa8ae to
7d9a248
Compare
dirvine
left a comment
There was a problem hiding this comment.
Re-review — blockers resolved
Fixes verified
- Core pin:
saorsa-corenowfd377754(the bounded closer-peer decoder) in Cargo.toml and Cargo.lock. The first-round blocker (client passing untrustedpeersto an unbounded decoder) is resolved — the decoder now rejects payloads over 64 KiB before postcard parsing. - ant-node pin: now
ad886472(ADR-0020 branch, carries the fixed core pin — see the coordination note on ant-node#249 about mid-branch vs head). - New: bad-content peers are dropped, read continues.
is_retryable(kad_get.rs:155–160) now includesError::InvalidData;note_bad_contentlogs and passes it through. Inkad_read, a retryable failure drops the peer (Ok(None)); non-retryable local errors (e.g.Config) still abort. Tests: corrupt holder + honest slower holder → chunk found; corrupt-only holder →Ok(None)→ fallback to ordinary read; adapter-rejected error ends the read.
Local verification at head 7d9a248b
- client-engine 12/12, adapter 1/1, e2e 1/1 ok (macOS). CI: E2E ubuntu/macos, Merkle E2E ubuntu/macos, unit tests, builds all green.
- CI red check:
Security Auditfails —cargo auditreports 19 vulnerable dependencies, all inherited from the pinnedsaorsa-coregit rev (e.g.tokio-rustls0.24.1 RUSTSEC-2024-0336/0337,serde1.0.228 RUSTSEC-2025-0049). Not introduced by this PR's code; resolves when the core rev is updated. Flagging for the core maintainer.
Verdict
APPROVE at head 7d9a248b, subject to: (a) the ant-node pin reconciliation (see #249), (b) the core cargo audit failures being tracked in the core rev. Panel: GLM 5.3 seat billing-failed (402) — 2/3 reviewed, provisional.
a7c7c2f to
217de5d
Compare
saorsa-core#173 (c3c4c81), ant-protocol#44 (0c6ae27) and ant-node#249 (6a14b53), each rebased onto its main: saorsa-core 0.29.2, ant-protocol 5.0.1 and ant-node 0.23.1, the rc-2026.10.3 baseline. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ant-node ADR-0020. A native chunk read now runs its own iterative lookup on saorsa-core's lookup engine, with the same alpha, grace and deadline as saorsa-core's lookup. Each peer it queries is asked for the chunk as well as for closer peers, and the read stops at the first verified chunk. - A peer announcing `get-or-closer/1` gets one get-or-closer request. Its closer peers are checked by saorsa-core as a FIND_NODE answer. - Any other peer gets FIND_NODE and a GET at once. Every node in service today takes this route, at the same speed. - Requests belong to the read rather than to the lookup round, so a round's grace period never cancels a chunk still arriving. - A lookup that finds no holder falls back to the existing read path, so the absence rule and the retry round are unchanged. The policy lives in `client_engine::kad_read`, tested on a scripted network; `data::client::kad_get` runs it over saorsa-core and the chunk protocol. In-process test nodes now announce the token, and an e2e test checks that such a network is read with get-or-closer requests only. Against production, a client built from this branch downloaded a 612 MB file in 56 s, against 242 s for the current client. The test nodes' per-address read counts now count get-or-closer requests as reads beside GETs, so the single-flight test still sees every read the client sends. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The two futures were raced in an unbiased select, so when the GET was polled first and finished at once, FIND_NODE was never sent. Polling FIND_NODE first makes both requests go out in the same step, and makes the scripted-network test that checks it deterministic. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every holder a get-or-closer request reaches sends the whole chunk. The lookup round that reaches the close group asked up to three holders at once, so a 50 MiB download received about 2.4 times its size. Peers the lookup knows to be among the closest are now asked one at a time, closest first. One that answers with closer peers hands over at once; one still sending after a second is hedged. Far peers and a cold lookup still run in parallel, so discovery is unchanged. On a 60-node testnet at the default fetch concurrency: bytes received per 50 MiB download fell from 113 to 78 MiB and client CPU by a quarter, with download time unchanged (p50 6.78 s to 6.57 s). Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit's 1 s hedge was not latency-free. Its "download time unchanged" pooled two downloader droplets. Per droplet, against asking every likely holder at once, 1 s cost up to 13% at the default fetch concurrency of 4, and 15-18% at 16. On the same 60-node testnet and 50 MiB files, with 5-6 downloads per arm in each cell: | hedge | syd1, cap 4 | fra1, cap 4 | syd1, cap 16 | fra1, cap 16 | | --- | --- | --- | --- | --- | | 1 s | +13%, -36% | -2%, -33% | +15%, -28% | +18%, -16% | | 500 ms | +9%, -14% | -2%, -22% | +3%, -17% | +8%, -24% | Each cell gives the change in download time p50, then in bytes received. 500 ms keeps most of the byte saving for about half the latency cost. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The kad read asks every peer its lookup queries for the chunk, far peers included. Content that failed verification (a wrong address, or a hash mismatch) was not retryable, as on the ordinary read path. Its `?` in chunk_get_from_closest_peers also skipped that path, and the file download does not retry InvalidData. So any dishonest node that a lookup happened to query could fail the whole download. - A peer whose content fails verification now only drops out of the lookup, as an unreachable peer does. The read goes on to the other holders. - The client logs a warning naming that peer. - If no other peer sends the chunk, the read reports not-found, and the ordinary read path runs. Its rule that bad content ends the read is unchanged. Tests: - New in kad_read: corrupt content from one peer leaves the read to the others; corrupt content from the only holder is not-found. - `corrupt_content_ends_the_read_with_an_error` becomes `an_error_the_adapter_rejects_ends_the_read`, covering the engine's fatal path with a stricter policy. - New in kad_get: the native policy treats InvalidData as one peer's failure. - Both new kad_read tests fail under the former policy. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ADR-0023 made the ordinary read choose its first GET uniformly among a chunk's close group, because every client reading from the nearest holder loaded one node of a popular chunk's close group while the rest idled. The get-or-closer read skipped that: its holder gate released the first likely holder it received, and a lookup round always sends its nearest peer first. On a network where native and browser clients read the same files, that put every native read of a chunk back on one node. The read now takes every step a lookup round has sent before choosing, and picks which likely holder to ask with the ordinary read's `uniform_choice`. A slow holder is still hedged after 500 ms, by another chosen the same way. Tests pass a chooser that returns zero to keep asking the nearest first; a new test checks the chooser sees the whole round, and fails if the round is not drained first. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of the kad read found that it could multiply copies and slow reads: - A slow likely holder was hedged every 500 ms with no limit, so on a link that needs longer than that per chunk, three or four copies shared one connection. The ordinary read never runs more than two GETs. - The holder gate never applied in the first lookup round: a peer only counted as a likely holder once the lookup knew 14 peers. Up to three seeds sent the whole chunk at once wherever the client already knew the close group. - Counting seeds as holders would have made the first round wait for the gate: each queued peer answered the lookup only when its turn came. Later rounds already waited that way, and a peer far from the chunk was asked for it in a get-or-closer that nodes queue behind PUTs. The read now separates routing from chunk requests: - Every queried peer answers the lookup at once. Only the likely holder chosen to go next is asked for the chunk in its lookup step. Other likely holders answer with FIND_NODE and wait for a GET, which is skipped if the lookup has since passed them. Peers far from the chunk get FIND_NODE only, as in saorsa-core's own lookup. - Until the read knows 14 peers, every peer counts as a likely holder. - At most two likely holders are asked at once. The ordinary read behind it changes too: - It runs when the kad read fails, instead of the error ending the read. Every adapter error was retryable, so this was latent. - Its first round starts from the peers the kad lookup ended on. A read of a missing chunk, as manifest builds check, no longer runs two lookups. The two copies of chunk verification become one, `verified_chunk`. Tests: new kad_read tests check the two-request cap, routing that does not wait for the gate, a waiting peer the lookup has passed, far peers asked only for closer peers, and the peers handed back. Each fails with its mechanism removed. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `MiniTestnet::start_without_get_or_closer` starts nodes whose user agent lacks the get-or-closer token, as nodes released before it do. A new e2e test reads a chunk from them through the kad read, with GETs and no get-or-closer request. - The upgraded-network test no longer expects zero plain GETs: a likely holder that waits its turn is asked with one. - The read counters decode only messages small enough to be a read request, so the test nodes no longer decode every PUT's chunk. Both counters share that decoding. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
217de5d to
1061e38
Compare
Native chunk reads now run their own iterative lookup, and ask each peer they query for the chunk as well as for closer peers. Browsers keep today's read. This implements ant-node ADR-0020.
How it works.
get-or-closer/1gets one get-or-closer request; any other peer gets FIND_NODE and a GET at once, in the same round trip. Every node in service today takes that second route.Closeranswer as a FIND_NODE answer, and rejects a payload over the 64 KiB FIND_NODE limit before parsing it. A payload that fails any check fails only that peer's step.retrieve_progressivepath runs, with its absence rule, retry round and rule that bad content ends the read. Its first round starts from the peers the lookup ended on, so reading a chunk that is not stored, as manifest builds check, costs one lookup.Review fixes (2026-10-27). A review found the read could multiply copies and slow down:
Code layout.
client_engine::kad_read: the policy, transport-neutral and tested on a scripted network.data::client::kad_get: the adapter, over saorsa-core and the chunk protocol. It sharesverified_chunkwith the plain GET.MiniTestnet::start_without_get_or_closerstarts nodes that do not, for the legacy route. The read counters no longer decode PUTs.Measurements. See the investigation: sequential reads on production, native.
Those numbers predate the likely-holder gate. Without it, this read fetched 2.5–3.3 copies of a chunk on production, a trade-off the ADR accepted; the gate trades a little latency for fewer copies (see Test evidence).
Stack, merge last:
This PR pins all three by git rev: saorsa-core
c3c4c81, ant-protocol0c6ae27and ant-node6a14b53. It cannot be published as is. Before merging, release saorsa-core#173 and ant-protocol#44, merge and release ant-node#249, and replace the[patch.crates-io]pins with version bumps.Linear issue
Closes V2-1358
Risk tier
Compatibility
GetOrCloserRequestonly to peers whose user agent carriesget-or-closer/1. Every other peer gets FIND_NODE and GET, both existing messages.#[doc(hidden)]Client::chunk_get_kad_only, behindtest-utils.Semver impact
Test evidence
cargo test --lib --all: 892 passed.kad_readhas 19 tests on a scripted network. These are new since the review, and each fails with its mechanism removed:CI's e2e list (
cargo test -p ant-core --features test-utils --test e2e_chunk --test e2e_data --test e2e_file --test e2e_payment --test e2e_native_payment --test e2e_security --test e2e_cost_estimate --test e2e_adr0004 --test e2e_manifest -- --test-threads=1): 69 passed. That includes the newtest_chunk_kad_read_uses_get_on_older_nodes: nodes without the token are read with FIND_NODE and GET, and sent no get-or-closer.cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings, and the browser WASMcargo clippyjob: clean.Dev testnet comparison against main, 2026-10-27, after the review fixes. Two 60-node DigitalOcean fleets with the same layout:
5e01699(before perf(browser): fail dead dials in 3 s and use the whole read budget #222 and ci(release): fall back to generated notes, and add the changelog rotation script #237 landed), differing only by the four PRsThree runs:
ant-testnet/state/comparisons/v2-1358-review-fixes-{vs-main,swapped,swapped-matched}-60m-1027/README.md.Earlier evidence, from before the review fixes:
Live, against today's production nodes, which do not announce the token.
ant file downloadof a 612 MB file took 56 s, against 242 s for the current client. The output was byte-identical, and all 150 chunks came through the new lookup's FIND_NODE-plus-GET route.Likely-holder gate, A/B on the same kad fleet. The client binary was swapped on two downloader droplets (syd1, fra1), and arms alternated over 50 MiB files not read before, with 5–6 downloads per arm in each cell. Each cell gives the change against asking every likely holder at once, as download time p50 and then bytes received:
On these datacenter links bandwidth is not the bottleneck, so the gate shows its latency cost and not its benefit. Client CPU per download fell 10–17%.
Browser variant, tried and dropped (2026-10-12). It ran this read driver over WebRTC Direct, with nodes answering the request inside
chunk_protocol(a missing chunk got the node's browserfind_nodereply). The kad fleet's nodes were swapped in place to that build, keeping their data. Headless Chromium on two downloaders, 50 MiB private files, a fresh client per download. That WASM build was interleaved with this PR's, which reads as today, over 8 runs. Pairs are by round; the intervals resample whole runs:ant-testnet/state/comparisons/v2-1358-kad-vs-main-matched-60m-1009/README.md, sections "Browser get-or-closer reads" and "Why the browser read is not faster here".Browser (WASM) downloads are unchanged by this PR. Headless Chromium on the testnet's downloader droplets, 50 MiB private files, a fresh client per download, with main's and this PR's WASM builds interleaved:
New dependency
none
ADR
ADR-0020: Kademlia-style chunk GET: return the chunk or closer peers (ant-node). Measurements are in the investigation.
Mitigation / rollback
Client-only with no stored-format change: revert this PR. When the new lookup finds nothing, the existing read path still runs.
🤖 Generated with Claude Code