Repository navigation
feat(dht): expose lookup steps for caller-driven lookups - #173
mickvandijke wants to merge 2 commits into
Conversation
dirvine
left a comment
There was a problem hiding this comment.
Changes requested — P2: cap closer-peer payloads before deserialisation
src/dht_network_manager.rs:1681–1682: the new decode_closer_peers calls postcard without checking payload.len() > MAX_MESSAGE_SIZE. The existing DHT ingress checks the 65,536-byte ceiling before decoding; the new entry point bypasses that boundary. The encoder enforces the ceiling, but a malicious responder need not use it. The new native client passes this field through the larger, 5 MiB chunk envelope. Candidate truncation occurs after decoding and normalisation.
Executed exact-head probe: a 1,050,035-byte payload containing 10,000 duplicate nodes was accepted by the public decoder. This establishes excess work/input acceptance, not a demonstrated crash, OOM or fleet-wide DoS.
Required: reject payloads over MAX_MESSAGE_SIZE before postcard parsing; test over-limit rejection and valid exact-boundary handling. Then refresh downstream pins. This preserves honest encoder compatibility.
CI caveat: the Rust-1.91/transport-1.95 mismatch is inherited from the base manifest/lock/workflow; claude-review fails with an app-installation HTTP 401. Neither is introduced by this source diff, and neither is waived by local passing tests.
Reviewed commit: 4deab7562ea48ee9c2152ea8b69d5d644fa92721.
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.
decode_closer_peers parsed its payload with postcard whatever its size. Every other DHT ingress rejects anything over MAX_MESSAGE_SIZE (64 KiB) first. The payload arrives inside an application message that may be much larger: ant-protocol's chunk envelope allows 5 MiB. A responder could therefore make a reader parse and normalize a body of 10,000 duplicate nodes, about 1 MB. The encoder keeps a whole answer, trailer included, within the limit, so the decoder now rejects any larger payload before parsing it. No honest answer is refused. New tests: - a body of 2,000 copies of one peer is rejected; before this change it was accepted - an honest answer padded to exactly the limit is read, and one byte more is rejected Reported in review of #173. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, confirmed and fixed in fd37775.
|
dirvine
left a comment
There was a problem hiding this comment.
Re-review — fixes confirmed
First-round P2 blocker is resolved at fd377754.
Fix verified
decode_closer_peers(src/dht_network_manager.rs:1681) now rejectspayload.len() > MAX_MESSAGE_SIZE(64 KiB) beforesplit_first/postcard::take_from_bytes. The 1,050,035-byte acceptance path from the first round is closed.- Boundary tests: exactly 65,536 bytes accepted; 65,537 rejected; a well-formed 2,000-node payload over the limit rejected before parsing.
- Local run at the current head:
closer_peers5/5 ok;dht_network_manager145/145 ok. Negative control (same tests on the unbounded code) fails as expected — the tests are live, not vacuous.
Non-blocking: encoder/decoder boundary is asymmetric
- Encoder (rs:1641–1646) checks
bytes.len()beforeappend_lookup_extension, so the trailer is not counted there. - Decoder (rs:1681) counts the whole payload, trailer included.
- Practical impact: none today —
append_lookup_extension(rs:5396) caps the final size at 64 KiB, and honest FIND_NODE answers sit far below it; a rejected answer only drops that peer (retryableError::Protocol). - But the invariant is not what the comment at rs:1678 states, and a future encoder change that lets the body approach the limit would make a valid answer decode-fail. Suggest: move the encoder check to after
append_lookup_extension, or correct the comment.
CI note
Rust 1.91 native and WASM fails at 16:32 BST. Cause: claude-review job — claude-code-action@v1 install script returned HTTP 401 (billing/entitlement, not code). Everything else green (tests linux/mac/windows, mutation, wasm, security, lint).
Verdict
APPROVE at head fd377754 — subject to the non-blocking boundary reconciliation above. Panel: GLM 5.3 seat billing-failed (402), so this is 2/3 reviewed — provisional until the GLM seat can run.
decode_closer_peers parsed its payload with postcard whatever its size. Every other DHT ingress rejects anything over MAX_MESSAGE_SIZE (64 KiB) first. The payload arrives inside an application message that may be much larger: ant-protocol's chunk envelope allows 5 MiB. A responder could therefore make a reader parse and normalize a body of 10,000 duplicate nodes, about 1 MB. The encoder keeps a whole answer, trailer included, within the limit, so the decoder now rejects any larger payload before parsing it. No honest answer is refused. New tests: - a body of 2,000 copies of one peer is rejected; before this change it was accepted - an honest answer padded to exactly the limit is read, and one byte more is rejected Reported in review of #173. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fd37775 to
b9d3eb7
Compare
A caller can now run its own iterative lookup with the same peer handling as the native one. ant-node's chunk-or-closer-peers response (ADR-0020) needs this. - `encode_closer_peers` encodes this node's FIND_NODE answer for a key, so an application response can carry closer peers in place of a value. - `decode_closer_peers` checks that payload as a FIND_NODE answer from the responder: signed records, owner views, LAN filter, dial plans. - `find_node_on_peer` runs one FIND_NODE step against one peer. - `connect_lookup_peer` and `is_lookup_candidate_dialable` let the caller connect, and learn the peer's user agent, before choosing a request. The FIND_NODE answer normalization and lookup-trailer encoding move into shared helpers; the wire format and the native lookup are unchanged. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
decode_closer_peers parsed its payload with postcard whatever its size. Every other DHT ingress rejects anything over MAX_MESSAGE_SIZE (64 KiB) first. The payload arrives inside an application message that may be much larger: ant-protocol's chunk envelope allows 5 MiB. A responder could therefore make a reader parse and normalize a body of 10,000 duplicate nodes, about 1 MB. The encoder keeps a whole answer, trailer included, within the limit, so the decoder now rejects any larger payload before parsing it. No honest answer is refused. New tests: - a body of 2,000 copies of one peer is rejected; before this change it was accepted - an honest answer padded to exactly the limit is read, and one byte more is rejected Reported in review of #173. Part of V2-1358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b9d3eb7 to
c3c4c81
Compare
Exposes the steps of saorsa-core's iterative lookup, so a caller can drive a lookup of its own and treat peers exactly as the native lookup does. This is the saorsa-core part of ADR-0020: a chunk-protocol request that returns the chunk if the node holds it, or closer peers otherwise, as Kademlia's FIND_VALUE does.
New public methods on
DhtNetworkManager:encode_closer_peers(key, requester)encodes this node's FIND_NODE answer for a key, so an application response can carry closer peers in place of a value.decode_closer_peers(responder, key, payload, transport_source)checks that payload as a FIND_NODE answer from the responder:find_node_on_peer(node, key)runs one FIND_NODE step against one peer. A client uses it for peers that predate the new request.connect_lookup_peerandis_lookup_candidate_dialablelet the caller connect, and read the peer's user agent, before choosing a request.The FIND_NODE answer normalisation and the lookup-trailer encoding move into shared helpers. The FIND_NODE wire format and the native lookup are unchanged.
Stack, merge and release in this order:
This PR is rebased onto main (0.29.2). ant-node#249 and ant-client#221 pin it by git rev, so it must be released before they merge.
Linear issue
Closes V2-1358
Risk tier
Compatibility
DhtNetworkManager; no existing signature changes.Semver impact
Test evidence
cargo test --lib: 614 passed. Five new tests check that:fd37775)cargo clippy -- -D warnings -D clippy::unwrap_used -D clippy::expect_used: clean.--all-targetsclippy fails on main too, atsrc/network.rs:3090(empty line after a doc comment), which this PR does not touch.End to end, through ant-node's handler and ant-client's read path, as listed in those PRs.
Dev testnet comparison against main, 2026-10-27, after the review fixes. Two 60-node DigitalOcean fleets with the same layout:
Three runs:
ant-testnet/state/comparisons/v2-1358-review-fixes-{vs-main,swapped,swapped-matched}-60m-1027/README.md.New dependency
none
ADR
ADR-0020: Kademlia-style chunk GET: return the chunk or closer peers. Measurements are in the ant-client investigation.
Mitigation / rollback
Purely additive API with no callers outside ADR-0020's read path; revert the commit.
🤖 Generated with Claude Code