Skip to content

feat(download): forward the availability ask across connected pool peers - #258

Merged
MichaelTaylor3d merged 2 commits into
mainfrom
loop/3128-forwarded-ask
Aug 20, 2026
Merged

feat(download): forward the availability ask across connected pool peers#258
MichaelTaylor3d merged 2 commits into
mainfrom
loop/3128-forwarded-ask

Conversation

@MichaelTaylor3d

@MichaelTaylor3dMichaelTaylor3d commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

DO NOT MERGE — DRAFT, gate round not yet run.

Core of epic https://github.com/DIG-Network/dig_ecosystem/issues/3128 (requirements 2/3/6).
Closes#259.

The whole change, in one sentence

On a miss, before answering, NodeContent also asks its connected pool peers dig.getAvailability and merges their returned providers into the existing enrichment/redirect answer.

Zero new verbs. Zero new address structs. Zero new result types. No dig-peer change.

Blast radius checked

impact was attempted per-worktree; the index build did not complete inside the 10-minute bound, so the radius below is from ripgrep + call-graph reads, per CLAUDE.md §2.0 bound (2) — stated rather than skipped.

SymbolDirect callersHandling
NodeContent::find_providersmiss_outcome (download.rs:1598), availability_answer (lib.rs:3679), the DEBUG locate logUnchanged. The new locate_holders wraps it; both miss legs moved to the wrapper so they cannot drift apart
Node::availability_batchseams/dig_rpc/dispatch.rs:313 (JSON-RPC), peer.rs:1554 (dig-nat mux), 12 testsSignature gained hops_used; both production sites updated, mux passes REDIRECT_HOP_CAP (fail-closed)
Node::availability_answeravailability_batch only, + 4 testsSignature gained hops_used
NodeContent::fetch_resource9 sites across lib.rs/download.rsError path only: the ? became an if let Err so the SPEC 6.8 escape can fire before returning. Same Err value, same message
NodeContent::new / for_dht30+ test sites, the composition rootSignatures unchanged — the two new legs install through setters, as set_capsule_warmer already does
MissRateLimitermiss + proxy limitersAdditive: a third with_relay_defaults(); the existing two untouched

detect_changes() was unavailable for the same reason; git diff --stat is 12 files and every touched symbol is in the table above.

Risk: MEDIUM. No custody, no crypto, no key handling. It is peer-reachable and network-amplifying, which is why each of the three bounds is pinned by a test proven load-bearing below.

The five constraints, each confirmed

  1. handle_rpc_as with the true peer_id — confirmed, and unchanged. The inbound serve path was already correct: peer.rs:1369 passes RequestorId::Peer(conn_key) into handle_rpc_as, and handle_availability (peer.rs:1553) does the same. This PR adds no call to handle_rpc, so nothing can reach RequestorId::from_origin's Peer("").

  2. Download locator only — confirmed by NOT touching either union. The forwarded ask is not a ProviderLocator at all; it is a per-miss call inside locate_holders. PoolProviderLocator stays download-only, the raw discovery locator stays DHT-only, and neither UnionLocator construction in download.rs:855-895 is modified.

  3. Insert position: forwarded records are APPENDED, after this node's own DHT findings. Three reasons, the third load-bearing:

    • the requestor dials in list order, and a claim relayed by one more untrusted hop must not lead a claim the holder announced itself;
    • with no forwarded answers the list is byte-identical to what shipped, so the change is a strict suffix of the old behaviour;
    • the MAX_REDIRECT_PROVIDERS truncation then falls on the forwarded tail, which is what makes the cap non-displacing. Prepending would let one connected peer bury every genuine holder for free.

    best_address() is untouched: it is decided inside the download union, which this PR does not modify.

  4. Relay bucket bound.DEFAULT_RELAY_ASK_BURST = 4, DEFAULT_RELAY_ASK_REFILL_PER_SEC = 1.0, per requestor, separate from both the lookup and the proxy buckets. Global ceiling MAX_CONCURRENT_FORWARDED_ASKS = 32 concurrent outbound asks node-wide (try_acquire, never awaited). Per-miss fan-out FORWARDED_ASK_FANOUT = 4.

    Corrected cost figure — the gate was right and my original was ~5x low. The relay token is charged on inbound admission and one token buys four outbound dials: the charge is 1:4, not 1:1. One 64 KiB frame at redirect_depth: 0 against a full relay burst fans out 16 → 64 → 256 → 1024 = ~1,360 dials and ~1,360 DHT walks (every ask that lands also drives a find_providers at its receiver, and 4 asks per requestor per peer sits under that node's miss burst of 16, so none are refused). Sustained at the 1 token/s refill that is ~340 asks/s per attacker identity. 4^4 = 256 is the leaf count of one question, not the cost of a frame. The node-wide semaphore bounds concurrency, not the aggregate — the work still happens, serialized, and every downstream node has its own independent 32. Both corrections now live in the FORWARDED_ASK_FANOUT and MAX_CONCURRENT_FORWARDED_ASKS doc comments, with the "this is an exponent, not a knob" note kept and sharpened now the true number is visible.

  5. Operator kill switch — DIG_NODE_FORWARD_ON_MISS, default OFF. Truthy on/1/true/yes (case-insensitive) enables it; unset, empty, falsy and unrecognised values all fail CLOSED, so a typo can never become a network-wide amplifier. It mirrors DIG_NODE_INBOUND_DEMAND_CACHE — an existing default-OFF amplification gate in the same file, same resolve_* pure-core shape — rather than inventing a second config idiom, and is resolved once at engine construction like DIG_NODE_ON_MISS, so a node's amplification posture is fixed for its lifetime. Disabled, the leg is never installed, the refusal costs nothing on the miss path, and the answer is byte-identical to what shipped before this PR (already pinned by without_the_leg_the_answer_is_the_shipped_dht_answer).

    Default OFF is the honest call rather than a cautious one: the proxy leg costs one capsule fetch by this node — expensive in bytes, but local and bounded — while the forwarded ask recruits other nodes' bandwidth and DHT budget at ~1,360 dials per admitted frame. A path that amplifies more than an opt-in path cannot honestly be gated less than it.

  6. Every bound reused.redirect_depth/REDIRECT_HOP_CAP for depth, MAX_REDIRECT_PROVIDERS for the cap, allow_miss_lookup for admission. No parallel bound invented.

forget_discovered — the caller is built; the binding is blocked upstream

dig_dht::DhtService::forget_discovered ships in dig-dht 0.12. This crate cannot resolve it: dig-download 0.17.4 and dig-peer-selector 0.9.0 both require dig-dht ^0.11, and tests/dependency_tree.rs correctly forbids two dig-dht copies across the download engine's trust boundary. Bumping this crate alone forks the candidate types on that boundary — a worse defect than the one being fixed.

Worth stating plainly too: the resolved dig-dht here is 0.11.1, which has no discovery cache at all. The 15-minute sticky-poison exposure does not exist in this tree yet.

So what was missing is built and tested: the call site. fetch_resource now calls forget_stale_discovery on the exact edge where "every located candidate was unreachable" becomes true, behind a DiscoveryCache seam. That judgement is the part dig-dht cannot supply for itself. Binding it is one set_discovery_cache call from for_dht the day the cascade lands.

Two blocked release-first cascades found, both pre-existing:

  • dig-rpc-protocol0.8.0 (this epic's own lane A) is unreachable for the same reason: dig-download 0.17.4 pins ^0.6, dig-peer 0.10 pins ^0.7, and the_workspace_carries_exactly_one_module_wire_crate asserts one copy. The stale "0.6" pin at Cargo.toml:135 is therefore correct today, not drift to fix here. This PR does not need the crate type: redirect_depth is read from raw JSON by the shipped download::redirect_depth parser, the same one every other redirect leg uses.
  • dig-dht0.12.0, above.

Gate round 1 — three fixes applied

Gate findingFix
SPEC §10.4.5 was FALSE — it survived the renumber still claiming "No NEW party learns the request", which the feature in the same PR makes untrueRewritten. It now states the disclosure radius (up to 4 pool peers per hop, recursively to the cap, ~1,360 nodes, none of which the requestor chose or can enumerate), and that this is not a disclosure a completed direct read would have made, because it happens on a MISS. Two real limits are stated without being overstated into an anonymity claim: the requestor's identity is not carried past the first hop (a forwarded ask contains only the item + redirect_depth, and each receiver authenticates the FORWARDING node), and nothing is retained — but a peer may log what it was asked, and timing correlation is untouched
Cost figure ~5x low at forwarded_ask.rsCorrected to ~1,360 dials + ~1,360 DHT walks per admitted frame, ~340 asks/s sustained, with the 1:4 inbound-charge-to-outbound-dial asymmetry spelled out and the leaf-count-vs-frame-cost confusion named so the next reader cannot repeat it. Also corrected the semaphore's doc: it bounds concurrency, not the aggregate
No operator kill switchDIG_NODE_FORWARD_ON_MISS, default OFF — see constraint 5 above

A third false claim was found while fixing the first and is also corrected: §10.4.4's own requirements were written as unconditional MUSTs, which contradicted the new opt-in gate. They are now explicitly conditional on it, with "a node with the feature disabled MUST forward nothing" stated outright. Leaving that would have reproduced exactly the defect the gate caught — a normative document asserting something the code does not do.

Version: no further bump.forward_on_miss_enabled() is additive public API on a 0.x crate, which is the minor slot — and dig-node-core is already 0.47.0 → 0.48.0, workspace 0.127.0 → 0.128.0. Both minor, so the additive surface fits the bump already taken.

New guard, proven load-bearing.the_forwarded_ask_is_off_unless_explicitly_enabled covers the truthy vocabulary, every falsy spelling, unset, empty, and an unrecognised value. Mutating resolve_forward_on_miss to the default-ON shape (the !matches!(falsy) form its sibling resolve_backfill_on_miss legitimately uses) fails it — so the default is pinned, not incidental. 926 tests green, clippy -D warnings clean.

Outstanding coherence obligation (not in this PR — different repo, single-writer)

docs.dig.net documents the sibling flag at docs/protocol/peer-network.md:806 ("FETCH-THROUGH (opt-in, DIG_NODE_ON_MISS=fetch)"), plus 13 locale copies under i18n/*/docusaurus-plugin-content-docs/current/protocol/peer-network.md:697. Per §4.3 the forwarded ask and DIG_NODE_FORWARD_ON_MISS belong beside it. I am single-writer for dig-node only and have not touched that repo — flagging it with the exact anchors so it can be dispatched rather than discovered later.

Deliberately out of scope

The hand-rolled json! redirect literals at download.rs:1731-1755 are NOT migrated to the typed RedirectInfo. Real drift, worth fixing, and folding it in here would make this diff unreviewable.

Evidence

  • 925 tests green (cargo test -p dig-node-core), 18 of them new. cargo clippy --all-targets -- -D warnings clean. cargo fmt clean.
  • Five mutations, each caught by exactly the intended test. Production code was mutated, never a test; the tree was committed first and restored from a file copy.
Mutation to PRODUCTION codeCaught by
forwarded records prepended instead of appendeda_hostile_slate_of_forwarded_holders_cannot_displace_our_own
hop cap >= becomes > (off by one)the_hop_budget_is_pinned_from_both_sides
relay leg draws from the cheap lookup bucketthe_relay_allowance_is_per_requestor_and_separate_from_the_lookup_budget
forget_stale_discovery on every fetch (wrong edge)both SPEC-6.8 tests, including the success control
.take(FORWARDED_ASK_FANOUT) removedthe_fan_out_is_capped_regardless_of_pool_size

Fixture notes, since narrowness is where false greens live:

  • The placement test uses a full slate of MAX_REDIRECT_PROVIDERS fabricated holders against one honest DHT holder, so prepending is visible as an eviction. A presence assertion would have passed under that mutation.
  • The requestor-exclusion test keeps a second, innocent peer connected, so "excluded the requestor" is distinguishable from "excluded everyone".
  • The relay-bucket test drives a second, different caller after draining the first, so a per-requestor bound is distinguishable from a global one, and asserts allow_miss_lookup is still willing — proving the separation, not merely the refusal.
  • The hop cap is pinned from both sides: at the cap must not forward, one under must.
  • a_successful_download_forgets_nothing is the control that makes the SPEC-6.8 test a statement about failure rather than about fetching.

Demo — four nodes, and why three cannot show it

A-B-C with C holding is one hop, which is exactly the shipped -32008 redirect plus the shipped getAvailability enrichment: control and treatment are the same code path. Recursion needs A-B-C-D, D holding, and neither A nor B able to locate D.

Setup (four hosts, one isolated --network-id e2e-3128): D holds the capsule and announces into a DHT partition A and B cannot reach. A is connected only to B; B only to A and C; C to B and D.

On host A:

DIG_NODE_FORWARD_ON_MISS=on # required on A, B and C — the leg is opt-in
dign content availability \
--store <STORE_HEX> --root <ROOT_HEX> --retrieval-key <RK_HEX> \
--node dig.local --json

What distinguishes a recursive answer from a direct one:

  • The peer_id in providers[]. A direct answer names only holders A's own DHT walk found — with the partition in place that array is [] today. A recursive answer names D's peer_id, which A has no DHT record for and is not connected to. dign peers list --json on A must not contain D: a named holder that is in neither A's pool nor its DHT is proof the answer travelled.
  • Depth, on the wire.RUST_LOG=dig_node_core=debug on B and C shows one forwarded ask line each, at redirect_depth 1 on B and 2 on C. Two hops is the observation; one hop is the shipped behaviour.
  • The control that makes it a proof: stop D's node and re-run. providers[] must go empty rather than answering from a stale cache — otherwise the first run proved caching, not recursion.
  • The negative bound: re-run from A with --redirect-depth 4. B must forward nothing (no forwarded ask line) and still answer — the_hop_budget_is_pinned_from_both_sides, observed live.

Not run here: no 4-node fleet was provisioned for this lane.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security — audit IN PROGRESS (interim, not a verdict)

Head audited: 3011a34f19c9105bc259c9446b22501864462b93, base 2cfbb55ab4734483b801d2b2f51d7c5e2a6c809d.

Read so far: forwarded_ask.rs (full), the download.rs diff, lib.rs/peer.rs/rate_limit.rs/dispatch.rs diffs, and the surrounding reachability path (peer.rs:1128-1175, handle_rpc_as at :1369, RequestorId::from_origin).

Facts pinned so far (no verdict yet):

  • REDIRECT_HOP_CAP = 4, FORWARDED_ASK_FANOUT = 4, MAX_CONCURRENT_FORWARDED_ASKS = 32, relay bucket burst 4 / refill 1.0/s, miss bucket burst 16 / refill 4.0/s, MAX_AVAILABILITY_ITEMS = 512.
  • Item 5 (handle_rpc_as) looks clean on a first pass: the only handle_rpc_as call outside tests is peer.rs:1369, and it passes RequestorId::Peer(conn_key). from_origin's Peer("") is reachable only via the handle_rpc wrapper, which this PR does not add a caller for.
  • Item 6 (mux fail-closed) reads as implemented: handle_availability passes REDIRECT_HOP_CAP verbatim, so hops_used >= REDIRECT_HOP_CAP refuses on that leg. Still verifying it is the only mux entry.
  • Destination set is NOT attacker-controlled: forwardable_peers reads self.connected_pool only, so there is no SSRF/reflection primitive here. Confirmed.

Open lines I am still working, in priority order:

  1. The amplification arithmetic — specifically that the relay token is charged ONCE per admitted inbound miss and buys FOUR outbound dials, and what the per-hop bucket refill does to the sustained (not just burst) tree.
  2. Whether the pool addresses the ask dials are the gossip endpoint (9445) or the peer-RPC endpoint (9444) — peer.rs:103 says the pool carries gossip addresses.
  3. Reproducing the placement mutation (a_hostile_slate_of_forwarded_holders_cannot_displace_our_own) and the requestor-exclusion test in my own worktree.

Verdict to follow on this PR before I return. PR stays DRAFT.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security — mutation results + one confirmed guard gap (still not the verdict)

Head 3011a34f19c9105bc259c9446b22501864462b93. All probes run in my own detached worktree C:\tmp\worktrees\sec-3128; the lane's dn-3128 was not touched.

Baseline

cargo test -p dig-node-core --lib forwarded_ask_tests11 passed, 0 failed.

Mutation 1 — the PLACEMENT claim. CONFIRMED LOAD-BEARING.

Swapped locate_holders to prepend rather than append:

letmut providers = self.forwarded_holders(content, hops_used, requestor).await;
providers.extend(self.find_providers(content).await);

Result: exactly one test fails, a_hostile_slate_of_forwarded_holders_cannot_displace_our_own, with left: "6464…" (the first fabricated holder, 0x64 = 100) where the honest DHT holder 0101… should lead. Because the slate is MAX_REDIRECT_PROVIDERS = 8 fabricated records, the honest holder lands at index 8 and is truncated away entirely. The stated property holds exactly as claimed: appending is what makes the cap non-displacing, and one connected peer answering with 8 fabricated holders would otherwise evict every genuine holder for free. 10 other tests still pass, so the test is specific, not a blanket.

Mutation 2 — requestor exclusion. CONFIRMED, and the test is not vacuous.

Deleted .filter(|(peer, _)| Some(peer.as_str()) != asker) from forwardable_peers. Exactly one test fails, the_asking_peer_is_never_asked_back, and the failure prints ["0202…", "0101…"] — the innocent peer is still asked alongside the requestor. That is the distinction the second innocent peer was put there to make, and it works: this test cannot pass by an implementation that simply stopped forwarding.

Both mutations reverted; worktree clean before the next probe.

PROBE (mine, not the PR's) — the self-exclusion invariant is NOT carried onto the forwarded leg

download.rs:1245-1247:

letmut providers = self.find_providers(content).await;// self-filtered
providers.extend(self.forwarded_holders(...).await);// NOT self-filtered

find_providers filters self_peer_id with the rationale "a redirect must never point the caller back at the node that just missed", and SPEC.md:5371 states the stronger invariant: the locator is wrapped so "NO source — DHT or pool — can ever offer self". The forwarded ask is deliberately not a ProviderLocator (which is correct for requirement 3), and the cost of that is that it inherits neither guard.

Probe test, run at the audited head, with a control:

CONTROL: engine self=peer5, DHT answers [provider(5)] -> locate_holders == [] (self dropped)
PROBE: engine self=peer5, DHT empty, peer answers [provider(5)] -> locate_holders == ["0505…"]

Output: forwarded answer names: ["0505050505050505050505050505050505050505050505050505050505050505"] — passed, i.e. the node named ITSELF as a holder of content it had just missed.

Scenario: honest node A is connected to malicious pool peer P (P knows A's peer_id — it is the mTLS identity on their link). Reader R asks A for content A does not hold. A forwards to P; P answers providers: [{peer_id: <A>, addresses: […]}]; A serves R a -32008 redirect naming A. R re-requests at A, misses, forwards, gets the same answer, redirects again — the bounce loop the shipped self-filter and the hop cap both exist to prevent.

Severity: defense-in-depth, not gating, and I am not gating on it. Two things bound it. REDIRECT_HOP_CAP still terminates the bounce at 4 round-trips, which is precisely the backstop it was documented as. And the record is APPENDED, so it only occupies a tail slot unless this node's DHT found nothing — and in that case P already controls the whole answer, so naming A adds little beyond generic junk. The forwarded records also never reach this node's own dial path (fetch_resource uses the separate self-excluded download locator), which I verified separately.

Recommended follow-up (one line, a filter in locate_holders mirroring find_providers), plus a test with a control, so SPEC.md:5371's "NO source" stays true.

Verdict comment to follow.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security VERDICT: PASS

Head audited: 3011a34f19c9105bc259c9446b22501864462b93 (resolved from remote via gh pr view 258 --json headRefOid), base 2cfbb55ab4734483b801d2b2f51d7c5e2a6c809d. No GATING security defect. PR left DRAFT.

Worst an attacker gets from one admitted frame: roughly 1,360 mTLS dials and 1,360 DHT walks spread across the network, and nothing else — no read, no forge, no elevation, no custody reach, and every refusal path degrades the answer rather than failing the request.


1. AMPLIFICATION — bounded; every stated bound verified present and effective

The relay token is charged on INBOUND admission, and one token buys FOUR outbound dials.forwarded_holders calls relay_rate_limiter.check(requestor) once per admitted miss, then fans out to FORWARDED_ASK_FANOUT peers. The token-to-work ratio is 1:4, not 1:1.

Measured constants: REDIRECT_HOP_CAP = 4, FORWARDED_ASK_FANOUT = 4, MAX_CONCURRENT_FORWARDED_ASKS = 32, relay burst 4 / refill 1.0/s, miss burst 16 / refill 4.0/s, MAX_AVAILABILITY_ITEMS = 512, MAX_REDIRECT_PROVIDERS = dig_dht::MAX_ADDRESSES_PER_RECORD = 8, read_framed cap 64 KiB.

One dig.getAvailability frame at redirect_depth: 0 carrying 4+ not-held items, from any peer that can complete an mTLS handshake:

legadmittedoutbound
entry nodemiss burst 16 gives 16 DHT walks; relay burst 4 gives 4 fan-outs16 asks at depth 1
depth 1each of 4 peers receives 4 asks from one parent; its relay bucket for that parent is burst 4, which exactly accommodates them64 at depth 2
depth 2same256 at depth 3
depth 3same1024 at depth 4
depth 4at the hop capforwards nothing

About 1,360 dials plus 1,360 DHT walks from one 64 KiB frame. Sustained, the per-hop 1/s refill holds it near 340 asks/s network-wide per attacker identity. Each dial rides full_nat_config, so a failed direct dial escalates onto hole-punch and then relay infrastructure.

The module doc's figure is low.forwarded_ask.rs:47-51 says "the worst case a question can reach is 256 nodes". 256 is the LEAF count of one question; the message count is 340, and one admitted frame buys four questions (relay burst 4) for about 1,360. The number that justifies the exponent should be the one an operator actually pays. Recommend correcting it — the design is unchanged, only the stated cost.

Ordering is correct: both the limiter and the semaphore run BEFORE any peer is selected or dialled, so they pace work not yet done rather than work already built.

The 32-slot ceiling is genuinely node-wide and cannot be circumvented by distinct requestors.forwarded_ask_slots is one shared semaphore built once in NodeContent::new, and NodeContent is the single per-node engine; every forwarded_holders call takes from it regardless of requestor, via try_acquire_owned, never awaited. I specifically checked that the permit binds to a NAMED _slot rather than a bare underscore — the bare form would have dropped the permit immediately and silently voided the whole ceiling. hold_every_forwarded_ask_slot drives the real semaphore, so the test pins the production object rather than a re-derived copy.

Residual, non-gating: because the next hop's bucket is keyed by the FORWARDING node, an attacker's traffic burns an innocent relaying node's allowance at every one of its peers. The effect is degradation only (fewer named holders); no request fails.

Verdict: not gating. The bounds are layered, each one runs, each is pinned by a test I mutation-checked, and the failure mode is work, not compromise.

2. INSERT POSITION — claim VERIFIED by mutation, and it is load-bearing

Reproduced. Swapping locate_holders to prepend fails exactly one test, a_hostile_slate_of_forwarded_holders_cannot_displace_our_own, with left: "6464..." (fabricated holder 100) where honest holder 0101... should lead. With an 8-record hostile slate the honest holder lands at index 8 and MAX_REDIRECT_PROVIDERS truncates it away ENTIRELY. The claim is exact: appending is what makes the cap non-displacing, and one connected peer would otherwise evict every genuine holder for free.

best_address() and both UnionLocators are untouched — the diff does not name them, and for_dht's union is unchanged apart from a dht.clone(). The arbiter-e2e 404 class is not reachable from this diff.

3. HEARSAY vs THE ASSERTION PATH — structurally confirmed

The forwarded ask is not a ProviderLocator. PoolProviderLocator remains download-only (download.rs:929), and the raw discovery locator is unchanged (dht_locator plus two EmptyLocators, wrapped in SelfExcludingLocator and CapsuleFallbackLocator). find_providers is byte-unchanged.

Forwarded records DO reach the -32008 redirect, via locate_holders. That does not violate the rule as stated at pool_locator.rs:23-26 / download.rs:850-853, which forbids naming EVERY CONNECTED PEER; a forwarded record is a specific claimed holder. Critically, forwarded records never become dial targets for THIS node — fetch_resource uses the separate self-excluded download locator — so hearsay stays on the answer, never on our own fetch path.

Address strings are unvalidated Rust Strings, but dig-download's addr.rs:47 rejects anything that is not an IP literal (AddrError::NotAnIpLiteral), so there is no DNS-resolution primitive on the recipient. An arbitrary IP:port in a redirect is inherited from the pre-existing DHT-announce path and is not a regression here.

One real gap, defense-in-depth, not gating — detailed with a probe in my previous comment:locate_holders applies find_providers's self-filter to the DHT half only, so a connected peer can name THIS node as a holder of content it just missed, contradicting SPEC.md:5371's claim that NO source, DHT or pool, can ever offer self. The hop cap bounds the resulting bounce at 4 round-trips, and in the case where it is reachable the malicious peer already controls the whole answer.

4. REQUESTOR EXCLUSION AND LOOPS — verified, and the test is not vacuous

Reproduced. Deleting the asker filter from forwardable_peers fails exactly one test, the_asking_peer_is_never_asked_back, and the failure prints ["0202...", "0101..."] — the innocent second peer is still asked. The fixture genuinely distinguishes "excluded the requestor" from "excluded everyone".

Longer cycles (A to B to C back to A) are not prevented by a visited set and are bounded by the hop cap alone. That is the same property the shipped redirect leg already relies on, and it terminates at 4. saturating_add on the next depth; redirect_depth parses to 0 on absent or garbage input, which is no worse than an attacker simply sending 0.

5. handle_rpc_as — no path reaches the shared empty-peer bucket

Every production caller checked, not just the two named:

  • peer wire: peer.rs:1369 passes RequestorId::Peer(conn_key); handle_availability at :1383 likewise.
  • HTTP JSON-RPC: dig-node-service/src/server.rs:1210 calls handle_rpc_as with requestor_for(peer_addr)Local on loopback, Anonymous(ip) otherwise. It does NOT go through the handle_rpc wrapper.
  • FFI: dig-runtime/src/lib.rs:112, ReadOrigin::Local.
  • control surface: control.rs:845 and :2281, both ReadOrigin::Local.

The shared empty-peer bucket survives only for a caller-less peer session, where it is more restrictive rather than less, and the empty asker string matches no 64-hex pool key. The recursive path does not collapse onto one bucket. The HTTP surface is loopback-only unless DIG_NODE_ALLOW_REMOTE=1, which meaningfully narrows the anonymous entry point.

6. THE MUX LEG FAILS CLOSED — true, not merely intended

NodeResponder (peer.rs:1313) is the ONLY production impl PeerRpcResponder; the other five are tests. Its handle_availability passes crate::download::REDIRECT_HOP_CAP verbatim, so the at-or-over-cap check refuses on that leg unconditionally. classify_request routes the bare items-shaped mux frame there and the method-shaped frame to the JSON-RPC leg, which reads params.redirect_depth. The forwarded request carries method with items nested under params, so it can never be misrouted onto the counter-less shape. Fail-closed direction confirmed on every mux path.


Other areas checked

  • Secrets / credentials: none introduced, logged, or committed. The new log lines carry a peer_id (public) and a count.
  • Dependencies: the Cargo.lock change is two version strings. No new, updated, or loosened dependency.
  • Panics on adversarial input: none. parse_forwarded_providers is question-mark and filter_map throughout, with u16::try_from(...).ok()? and PeerId::from_hex(...)?; NatForwardedAsk::ask is all .ok()? inside a tokio::time::timeout; mutex locks use unwrap_or_else(|p| p.into_inner()). The single .expect is test-only.
  • Allocation bound: a hostile answer is capped by read_framed's 64 KiB, times 4 asks, times 32 slots. Bounded.
  • Content-key binding: records are keyed to what WE asked (content.to_key()), never to what the answer claims — pinned by test.
  • No lock held across an await:forwardable_peers returns owned data; the std mutex guard never crosses an await point.
  • No SSRF: the dialled set comes only from self.connected_pool; attacker input never selects a destination.
  • Merge preconditions (check-merge-preconditions.sh, exit code read unpiped): all 5 required contexts SUCCESS by name, 0 unresolved threads. Exit 1 is the DRAFT flag only — which is correct, and it should stay draft until the gates return.

Recommended follow-up tickets (none gating)

  1. No operator kill switch.for_dht always installs the leg; there is no config or env var to turn forwarding off. The closest existing equivalent, the proxy fetch-through leg, is OFF by default and the operator opts in. An operator whose node is being used as an amplifier currently has no remedy short of downgrading. Highest-value follow-up.
  2. Self-exclusion on the forwarded half of locate_holders, so SPEC.md:5371 stays true (probe plus control in my previous comment).
  3. Correct the forwarded_ask.rs:47-51 cost figure from "256 nodes" to the real per-frame message count.
  4. SPEC.md 10.4.5 is now false. It survived the renumber unchanged and still says "No NEW party learns the request beyond those the direct read would already involve." The forwarded ask discloses the store_id / root / retrieval_key triple to up to 4 connected pool peers — the peers best placed to correlate it with the reader's identity, since they hold a live authenticated link to them. Section 10.4.4 is otherwise excellent and genuinely normative; this is one stale sentence in a privacy claim.

Method / hygiene

Read-only against git objects. All probes ran in my OWN detached worktree C:\tmp\worktrees\sec-3128, created at the audited SHA and removed afterwards, with git worktree prune run. The lane's C:\tmp\worktrees\dn-3128 and the shared primary checkout were NOT touched — the primary is still at b92de34 with no modifications from me. No shared state was mutated.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security — scoped re-gate of 37a2307, interim (1/2)

Auditing 37a2307d6389522c76e429048cc6b38f10664978 (resolved from remote), delta only vs 3011a34. Substance already cleared in the prior PASS is not being re-audited.

Static checks complete, all clear so far:

CRLF flip is genuinely gone.git cat-file blob 37a2307:SPEC.md | tr -cd '\r' | wc -c = 0 CR bytes across 6,171 lines. Full-PR diff stat from merge-base 2cfbb55 is 12 files, +1457/−31 — not the inflated +7543/−6117. The delta vs 3011a34 touches exactly 3 files (SPEC.md, download.rs, forwarded_ask.rs); no scratch file survived the git add -A.

"Disabled MUST forward nothing" is enforced in code, not merely asserted. Traced every path:

  • forwarded_ask is a private OnceLock with exactly one read site (download.rs:1325) and one production write site (download.rs:1155), now behind if forward_on_miss_enabled(). set_forwarded_ask is pub(crate), so nothing outside the crate can install the leg.
  • forwarded_holders is the sole funnel to any outbound ask, and its first gate is let Some(ask) = self.forwarded_ask.get() else { return Vec::new() };.
  • This matters more than the originating case: because the recursion runs through the same miss path, a disabled node also refuses the relay role. The kill switch therefore cuts the amplification chain at every disabled node, rather than only declining to start one. That is the property the SPEC clause needs and it holds.

The corrected figure is arithmetically right. Verified against the constants at head: REDIRECT_HOP_CAP = 4 (download.rs:94), FORWARDED_ASK_FANOUT = 4, DEFAULT_RELAY_ASK_BURST = 4.0 @ DEFAULT_RELAY_ASK_REFILL_PER_SEC = 1.0 (rate_limit.rs:200,205), DEFAULT_MISS_LOOKUP_BURST = 16.0 (rate_limit.rs:165).

  • Depth trace: a frame admitted at hops_used = 0 forwards at depths 1→2→3→4; receivers at hops_used = 4 hit >= REDIRECT_HOP_CAP and stop. Waves are therefore 16, 64, 256, 1024 = 1,360 outbound asks, and since every landed ask drives a find_providers at its receiver, 1,360 DHT walks.
  • The 4-asks-per-requestor-per-peer claim checks out: 4 sits under that node's own miss-lookup burst of 16, so none are refused.
  • Sustained: refill 1 token/s → 4 + 16 + 64 + 256 = 340 asks/s per identity. Consistent.
  • 4^4 = 256 as leaf-count-of-one-question vs 1,360-per-frame is the right distinction, and 1360/256 ≈ 5.3x matches the stated "~5x understatement".

Next: fail-closed mutation probe on resolve_forward_on_miss in an isolated worktree, then the SPEC privacy-clause judgement and the MUST sweep.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

loop-security: PASS (scoped re-gate)

Head audited: 37a2307d6389522c76e429048cc6b38f10664978 — resolved from remote via gh pr view 258 --json headRefOid, not from the dispatch prompt. Delta-only vs 3011a34 (3 files: SPEC.md, download.rs, forwarded_ask.rs). Substance cleared in the prior PASS was not re-audited.


1. SPEC 10.4.5 privacy rewrite — does NOT over-correct. Both limits verified TRUE.

Limit 1, "identity not carried past the first hop" — verified against the wire, not the prose.forwarded_request() (crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs:129-139) builds the entire outbound body as params: { items: [content_id_json(content)], redirect_depth: next_depth }. No requestor id, no originating peer_id, no caller address, no correlation token. Downstream receivers authenticate the forwarding hop over mTLS and see nothing else. The claim is exactly true, and it is correctly fenced: "a property of the message, not a defence against timing or traffic correlation, and it MUST NOT be described as anonymity."

Limit 2, "nothing is retained" matches the hearsay-containment property already cleared (10.4.4 forbids storing, re-serving, publishing), and the hedge "a peer is free to log what it was asked, and nothing here prevents that" is the honest reading. Neither limit is written as a guarantee.

The ~1,360 figure is stated as an upper bound and is one. Distinct nodes reached is necessarily fewer than asks issued, because overlapping pool membership re-asks the same node; "up to ~1,360" is correct and conservative.

One thing the rewrite improved that was not asked for. The first paragraph changed "the requestor wanted that resource" to "someone wanted that resource". That is a real correction, not a softening: on the proxy path the serving holder sees the MIDDLE node, so it learns the resource was wanted without learning by whom. The old wording overstated the proxy leg's disclosure.

No over-correction found. Nothing in the rewrite claims a protection the code does not have.


2. SPEC 10.4.4 conditionalization — the defect class was NOT re-introduced.

Swept every MUST / MUST NOT in 10.4.4-10.4.6 (19 of them). Each falls into exactly one of three sound categories:

  • Scoped to the enabled case — the opening WHEN ENABLED plus the blanket "Every requirement in this clause is conditional on that gate" covers the fan-out, ordering, depth, exclusion, relay-budget and no-retention bullets. When disabled these are vacuously satisfied, never falsified.
  • Unconditional and true — the mux AvailabilityRequest fail-closed rule (no depth field, treat as budget spent, MUST NOT forward) holds in both states; the 10.4.6 policy MUST (a path that amplifies more than an opt-in path MUST NOT be gated less than it) is satisfied by this very change.
  • Authoring constraints — the MUST NOT be described as anonymity and MUST NOT infer the stronger property clauses bind the reader, not the implementation, and are true.

Critically, "a node with the feature disabled MUST forward nothing" is ENFORCED, not asserted. Traced it rather than trusting it:

  • forwarded_ask is a private OnceLock with exactly one production write site (download.rs:1155, now behind the forward_on_miss_enabled() gate) and exactly one read site (download.rs:1325). set_forwarded_ask is pub(crate), so nothing outside the crate can install the leg.
  • forwarded_holders is the sole funnel to any outbound ask, and its first gate returns an empty vec when the leg is absent.
  • This matters more than the originating case, and is the part worth stating plainly: because the recursion runs back through the same miss path, a disabled node also refuses the relay role. The switch therefore cuts the amplification chain at every disabled node rather than merely declining to start one.

Non-gating drafting nit: read hyper-literally, "Every requirement in this clause is conditional on that gate; a node with the feature disabled MUST forward nothing" lets the blanket scope the disabled-MUST itself, making it vacuous. The semicolon structure makes the intent unambiguous and the code enforces the behaviour regardless. Not worth a round.


3. Kill switch — fail-closed verified by mutation, both mutants killed.

Probes run in my own worktree (C:\tmp\worktrees\sec-258-regate, detached at 37a2307); the lane's dn-3128 and the sibling dig-node-260 were not touched.

variantresult
baseline (verbatim from head)PASS — standalone AND in-crate (download::tests::the_forwarded_ask_is_off_unless_explicitly_enabled ... ok, dig-node-core v0.48.0, 863 filtered)
mutant A — the negated-falsy default-ON shapeKILLED, on the unset input: "DEFAULT IS OFF - an unconfigured node must never forward"
mutant B — unrecognised value treated as enabledKILLED by the explicit assertion: "an unrecognised value must fail CLOSED"

Mutant A is not a strawman, and I verified that rather than taking it on faith.resolve_backfill_on_miss at download.rs:171 has that negated-falsy shape verbatim, 68 lines above resolve_forward_on_miss in the same file. A copy-paste of the wrong sibling is the single most likely way this defect would actually be introduced, and the test kills it on the most important input — the unset one.

The new resolver is also shape-identical to resolve_inbound_demand_cache (download.rs:210) — same default-OFF, same truthy vocabulary — so the claim that it mirrors an existing idiom rather than inventing a second one is true.


4. The corrected figure — arithmetically right, and the ceiling correction is material and correct.

Verified against the constants at head, not against the prose: REDIRECT_HOP_CAP = 4 (download.rs:94), FORWARDED_ASK_FANOUT = 4, DEFAULT_RELAY_ASK_BURST = 4.0 at refill 1.0/s (rate_limit.rs:200,205), DEFAULT_MISS_LOOKUP_BURST = 16.0 (rate_limit.rs:165).

  • Depth trace: a frame admitted at depth 0 forwards at depths 1, 2, 3, 4; receivers arriving at depth 4 hit the cap and stop. Waves are 16, 64, 256, 1024 = 1,360, and since every landed ask drives a find_providers at its receiver, 1,360 DHT walks. Correct.
  • The 4-under-16 claim holds: 4 asks per requestor per peer sits under that node's own miss-lookup burst of 16, so none are refused — the chain is not self-limiting.
  • Sustained 340/s: refill 1 token/s, giving 4 + 16 + 64 + 256. Correct.
  • The leaf-count distinction (256 as the leaves of ONE question vs 1,360 per frame) is the right one; 1360/256 is about 5.3x, matching the stated ~5x understatement.

The MAX_CONCURRENT_FORWARDED_ASKS correction is right, and it does change what the ceiling means. The permit is taken once per forwarding miss and held across all 4 sequential dials, so 32 bounds concurrent forwarding misses at THIS node. At the rate the relay bucket permits one identity (4 concurrent), the 32-slot ceiling is never the binding constraint — so it genuinely does not reduce the 1,360 — and every downstream node has its own independent 32. Reading it as an aggregate cap would have been a real understatement.

Non-gating precision nit:"the work still happens, serialized" is loose. The permit is taken with try_acquire_owned and never awaited, so an over-ceiling miss drops the forward rather than queueing it (download.rs:1334). This errs conservative — it makes the ceiling sound weaker than it is — and the field doc at download.rs:713 states the drop behaviour correctly. Safe direction; no change required.


5. Also verified

  • CRLF flip genuinely gone. Counting CR bytes in the committed blob at 37a2307 gives 0, across 6,171 lines. Full-PR stat from merge-base 2cfbb55 is 12 files, +1457/-31, not the inflated +7543/-6117. No scratch file survived the earlier git add -A — the added-file set is exactly forwarded_ask_tests.rs and seams/dig_peer/forwarded_ask.rs.
  • Version reasoning holds.pub mod download (lib.rs:54), so forward_on_miss_enabled() is genuine public API — additive, hence minor. dig-node-core 0.47.0 to 0.48.0, node 0.127.0 to 0.128.0, both already carried pre-delta. Both forwarded-ask files are NEW in this PR, so making the leg default-OFF is not a behaviour regression for any released consumer. No further bump needed.
  • Default-OFF justification: I AGREE, explicitly. The precedent is real and I checked it rather than accepting it — DIG_NODE_ON_MISS falls through to MissMode::Redirect (download.rs:152), so fetch-through IS opt-in, and inbound_demand_cache_enabled defaults OFF on its own amplification reasoning. The asymmetry is the deciding fact: the proxy leg spends this node's bytes on one capsule — costly but local and bounded — while the forwarded ask spends other nodes' bandwidth and DHT budget at a 1:4 inbound-to-outbound charge. A path that recruits third-party resources cannot honestly be gated more loosely than one that spends only your own.

Flaky test — does NOT gate, and should not be closed as harmless

tests::cache_lock_is_exclusive_then_released is outside this PR's blast radius, measured rather than assumed:

  • The full base..head diff touches none of acquire_cache_lock, lockfile_path, config_path, set_cache_cap_bytes, set_wc_project_id, DIG_NODE_CACHE or ENV_GUARD — the grep over the whole diff returns empty.
  • The new 498-line forwarded_ask_tests.rs touches no env var and no cache lock — only two tempfile::tempdir() calls, which cannot contend with the advisory lock or the env guard.

So the PR cannot have introduced it by env contention, and the residual load effect of +498 lines of tests on a pre-existing timing-sensitive test is not a defect in this diff. Green in CI, green in the full 864-test re-run, green in isolation.

The lane was right not to call it harmless, and I am not overriding that. Recommend a follow-up dig-node ticket with one specific instruction that materially changes its severity: record WHICH assertion failed. If it was the first one (a held lock must block a concurrent try_lock), that is a genuine mutual-exclusion failure in acquire_cache_lock and deserves its own audit, since config read-modify-write serialization depends on it. If it was the second (after release the lock is re-acquirable), it is a lingering-handle timing artifact. That distinction is unrecoverable once the run is gone, which is why it belongs in the ticket now. Either way the code is pre-existing on main and gating this PR would not fix it.


Merge preconditions

check-merge-preconditions.sh exit 1, RESULT BLOCKED — solely because draft=true, which is the intended state per the dispatch. All 5 branch-protection-required contexts asserted present and SUCCESS by name (Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage), with 0 unresolved review threads.

Leaving DRAFT as instructed. No security defect in the delta. Two non-gating precision nits named above, neither worth a round.

MichaelTaylor3dand others added 2 commits August 19, 2026 23:25
On a content miss, `NodeContent` now ALSO asks its connected pool peers the
existing `dig.getAvailability` verb and merges their returned `providers` into
the enrichment/redirect answer it was already building. Content discovery
becomes recursive: a holder reachable through connections this node already
holds is named even when no DHT record here can point at it.
Zero new verbs, zero new address structs, zero new result types, no dig-peer
change. The hop budget rides the shipped `redirect_depth`/`REDIRECT_HOP_CAP`,
the answer cap is the shipped `MAX_REDIRECT_PROVIDERS`, and admission is the
shipped `allow_miss_lookup`.
Ordering is a contract, not an implementation detail: our own DHT findings lead
and forwarded records follow, deduplicated keeping the first occurrence. The
requestor dials in list order and the list is truncated at the cap, so appending
is what makes that cap non-displacing -- a peer answering with a full slate of
fabricated holders spends only the tail and can never evict a holder we found
ourselves.
The outbound fan-out is charged to a NEW separate per-requestor relay bucket
(burst 4, refill 1/s), never the cheap-lookup budget, because requestor identity
keys the immediate caller and a shared bucket would let one admitted inbound
frame spend a victim's allowance across every peer this node holds. A node-wide
semaphore of 32 bounds the amplification the per-requestor buckets structurally
cannot see. Fan-out is 4 peers per miss, excluding self and the asking peer.
The dig-nat mux `AvailabilityRequest` shape carries no hop counter, so that leg
declares the budget spent and forwards nothing -- fail-closed, since a request
that cannot count hops cannot bound a recursion.
Also builds the caller dig-dht SPEC 6.8 requires and had none of: a download
that reaches none of its located candidates forgets the cached lookup answer.
The binding to `DhtService::forget_discovered` is behind a seam because that
method ships in dig-dht 0.12, which `dig-download` 0.17.4 and
`dig-peer-selector` 0.9.0 both block at `^0.11`.
The leg is OPT-IN: `DIG_NODE_FORWARD_ON_MISS`, default OFF, mirroring
`DIG_NODE_INBOUND_DEMAND_CACHE`'s shape rather than inventing a second config
idiom. The relay token is charged on INBOUND admission while one token buys
four OUTBOUND dials, so one admitted frame at `redirect_depth: 0` fans to
roughly 1,360 dials and 1,360 DHT walks across the network (16, 64, 256, 1024)
-- about 340 asks/s sustained per requestor identity. The node-wide semaphore
bounds concurrency, not that total. The strictly cheaper, node-local proxy leg
is already opt-in, and a path that amplifies more than an opt-in path cannot
honestly be gated less than it.
SPEC 10.4.5's privacy claim is corrected rather than left to be inherited: the
forwarded ask discloses the requested triple to parties a direct read would
never have involved -- up to ~1,360 nodes the requestor did not choose and
cannot enumerate. 10.4.4's MUSTs are now explicitly conditional on the gate.
Closes#259
Refs: DIG-Network/dig_ecosystem#3128
Co-Authored-By: Claude <noreply@anthropic.com>
Minor, not patch: this PR adds a new capability -- a node now forwards an
availability ask to its connected pool peers when it cannot answer from its own
holdings, so a requestor reaches content held one hop beyond its own pool. That is
additive and compatible; nothing existing changes shape.
The bump is against 0.128.0 rather than 0.127.0 because the stable cut for 0.128.0
(the Sage RPC port move) landed on main while this branch was in its gate round.
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 20, 2026 06:45
@MichaelTaylor3d
MichaelTaylor3d merged commit 1ea757d into mainAug 20, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/3128-forwarded-ask branch August 20, 2026 06:46
MichaelTaylor3d added a commit that referenced this pull request Aug 20, 2026
… ask
`SPEC.md` §19.3 states without qualification that NO source — DHT or pool — can ever
offer self. The DHT leg honoured it twice over (a `SelfExcludingLocator` wrapper AND a
second hand-written filter inside `NodeContent::find_providers`); the FORWARDED leg
added by #258 honoured it not at all, so a peer's answer naming this node reached the
merged provider set and cost the requestor a self-dial.
The rule now has ONE implementation — `retain_excluding_self` — and it is applied at the
MERGE point in `locate_holders`, which is where it covers every source the answer draws
from rather than only the ones a reader thought to wrap. The two prior copies now call it.
Closes#261
Refs DIG-Network/dig_ecosystem#3128
Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d added a commit that referenced this pull request Aug 20, 2026
… ask
`SPEC.md` §19.3 states without qualification that NO source — DHT or pool — can ever
offer self. The DHT leg honoured it twice over (a `SelfExcludingLocator` wrapper AND a
second hand-written filter inside `NodeContent::find_providers`); the FORWARDED leg
added by #258 honoured it not at all, so a peer's answer naming this node reached the
merged provider set and cost the requestor a self-dial.
The rule now has ONE implementation — `retain_excluding_self` — and it is applied at the
MERGE point in `locate_holders`, which is where it covers every source the answer draws
from rather than only the ones a reader thought to wrap. The two prior copies now call it.
Closes#261
Refs DIG-Network/dig_ecosystem#3128
Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for freeto 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.

The forwarded availability ask: merge connected-pool providers into the miss answer

1 participant

@MichaelTaylor3d