Skip to content

fix(download): distinguish not-found from timed-out and cache first-hand holders - #292

Merged
MichaelTaylor3d merged 17 commits into
mainfrom
loop/273-275-ask-outcome
Aug 22, 2026
Merged

fix(download): distinguish not-found from timed-out and cache first-hand holders#292
MichaelTaylor3d merged 17 commits into
mainfrom
loop/273-275-ask-outcome

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

DRAFT -- DO NOT MERGE. Gate round has not run.

Also closes the digstore-chain child of https://github.com/DIG-Network/dig_ecosystem/issues/2610 -- see the measured refusal under Dep bumps.

Closes #273
Closes #275

Children of epic https://github.com/DIG-Network/dig_ecosystem/issues/3128 (requirements 3 and 7).

Why one PR

#273 forces ForwardedAsk::ask off Vec<ProviderRecord> onto an outcome type, which cascades through
forwarded_holderslocate_holdersMissOutcome → the wire. #275 needs a cache wrapped around the
same locate_holders call with each record tagged FirstHand | Hearsay — and that provenance tag is what
#273's outcome type carries. Separately means touching that signature twice and inventing provenance twice.

Blast radius checked

impact was unavailable in this worktree (no .gitnexus index; building one is bounded at 10 minutes per
§2.0 and was not spent here), so the radius was established by call-graph grep + direct read and is stated
in full:

Symbol edited Direct callers found Risk
ForwardedAsk::ask (trait signature) NatForwardedAsk (prod), RecordingAsk, ChainedAsk (tests) — all 3 migrated HIGH — trait signature
HopBudget (tuple → named struct, +2 fields) from_params/fresh/spent/at_depth + 30 call sites across download.rs, lib.rs, peer.rs, forwarded_ask_tests.rs. No call site changed — every constructor kept its signature LOW by construction
locate_holdersLocatedHolders download.rs:answer_miss, lib.rs:3751 (availability responder), plus test sites MEDIUM
MissOutcome (+Inconclusive) 2 envelope methods in download.rs, 1 match in peer.rs:1636 — exhaustive matches, so the compiler enumerated them LOW
dedup_by_peer DELETED — zero callers after the tagged twin landed; it was a rival implementation of one policy

WARNING — HIGH risk on ForwardedAsk::ask and on the dig.getAvailability answer shape. The trait is
pub(crate), so the radius is crate-local, but the wire behaviour changed: a node whose search cannot
establish an absence now answers -32009 where it previously answered a plain not-found. That is the
intended fix and it is stated in SPEC §10.4.4.

detect_changes() was likewise unavailable; the diff was instead confirmed to touch only the expected
symbols by git diff --stat plus a full cargo clippy --workspace --all-targets -D warnings (clean),
which enumerates every affected item.

#273 — the not-found / timed-out distinction

Outcome type, final shape — per-peer, in forwarded_ask.rs:

pub(crate) enum AskOutcome {
    Answered(Vec<ProviderRecord>), // the peer looked; EMPTY IS A REAL ANSWER
    Refused,                       // JSON-RPC error frame: it declined to look
    TimedOut,                      // budget spent
    Unreachable,                   // no connection, or the exchange failed mid-stream
}

and aggregated in download.rs:

pub(crate) struct LocatedHolders {
    records: Vec<(ProviderRecord, dig_sex::discovery::Provenance)>,
    conclusive: bool, // false => an empty `records` is a SILENCE, not an absence
}

MissOutcome gains Inconclusive, answered on the wire as CONTENT_MISS_INCONCLUSIVE = -32009.
dig.getAvailability additionally carries absence_established: bool (additive; an older peer omits it
and a reader falls back to the tolerant reading).

an_error_frame_yields_nobody was replaced, not fixed — it pinned the collapse as correct, which is
why the collapse survived. Its successor is
an_error_frame_is_a_refusal_and_not_an_answer_of_nobody.

A refusal to forward is CONCLUSIVE, deliberately. Recursion ships disabled, so treating "did not ask"
as "could not tell" would make every miss on every default-configured node inconclusive — a worse lie in
the opposite direction. Only an ask that actually failed clears the flag.

The time budget, and which field carries it

FORWARDED_ASK_TIMEOUT (fixed 5s) is split into FORWARDED_ASK_LEAF_TIMEOUT (5s, one leaf ask) plus
ask_budget(hops_remaining, fan_out) = leaf + fan_out × ask_budget(h-1), clamped by
MAX_FORWARDED_ASK_BUDGET = 65s — which is exactly ask_budget(2, 3) at the dig-sex defaults, so
the ceiling is derived rather than chosen.

The budget rides a NEW field, params.budget_ms — not redirect_depth. The two move in opposite
directions (time monotone decreasing, depth monotone increasing), so one integer cannot honestly carry
both, and overloading it would let a hop buy itself hops by claiming time. Clamped at ingress in
HopBudget::from_params; a deadline is fixed once per fan-out and each peer in turn is granted only what
is LEFT, so the budget is carried down and decremented rather than restated.

ChainedAsk now imposes a REAL timeout. It previously had none, which made it structurally unable to
exhibit the defect — the fixture could not express the failure, so the property read as proven.

Request identity — IN SCOPE, not deferred

params.ask_id, an opaque random 16 bytes minted by the originator and echoed by every hop, plus a
bounded TTL'd AskSeenSet (TTL = MAX_FORWARDED_ASK_BUDGET, since a request cannot outlive the largest
budget any hop may grant it; bound 8192). Not derived from content (two independent readers would collide)
nor from the requestor (that would publish who is asking to every hop). An unreadable id is treated as a
NEW question — the honest degradation for an older peer, and the alternative hands anyone a way to
suppress the whole forwarded leg by omitting a field.

#275 — the first-hand holder cache

FIRST-HAND ONLY; hearsay is never cached, so SPEC §10.4.4 survives unamended and the epic's
privacy concern is answered by construction — the cache records only what this node already dialled.

Property Value Why this value
Key ContentId the granularity the requirement is stated at
TTL ADVERTISED_TTL_SECS = 3600s the lifetime this ecosystem already grants a holder's own signed holdings announce, so a cached record expires exactly when the claim behind it would have
Bound 4096 keys misses are stranger-driven, so the footprint must be a constant
Eviction expired first, then oldest-by-insertion every entry is already TTL-bounded, so age dominates recency; the victim is not attacker-chosen
Persistence memory only a record of who holds what is a record of what this node looked for; not written to disk, so no NC-2 at-rest surface

An empty slate is never cached — that would turn one unlucky lookup into an hour of manufactured
absence, the same failure #273 repeals on the wire. Invalidated at the existing dig-dht SPEC §6.8 call
site, so it cannot replay candidates just proven unreachable.

dig_sex::discovery::merge_answers is now ADOPTED (it caps the hearsay portion only and tags
provenance), and dig-node's rival merge plus the bare dedup_by_peer are gone.

Revert-proofs — each fix reverted alone, with the assertion that fired

Two came back GREEN first time and both were real gaps, now closed:

Reverted fix Assertion text that fired
error frame → Answered(vec![]) "a refusal establishes NOTHING about whether the content exists - reading it as 'nobody holds it' is a censorship primitive costing one field"
timeout → Answered(vec![]) "a peer that never answered establishes nothing, so this must not become a not-found"
unproven absence reported as established "and because it never answered, this node has NOT established that nobody holds it - reporting a not-found here is the defect dig-node#273 fixes"
restate the leaf timeout at every hop "this node was given 2s and handed its peer 4.9999957s - a child must never be granted more time than its parent has" + "a peer that may itself ask 3 peers sequentially cannot be given one leaf timeout"
drop the seen-set "the second arrival of the same question must not re-walk the graph"
cache hearsay too "and the DHT was NOT walked again - which is the entire point of remembering it"
drop the wire-budget clamp "a ten-minute claim buys the ceiling and no more"

Gap 1 — the timeout proof was vacuous. Reverting the production timeout mapping came back green,
because the proof drove a ForwardedAsk double that RETURNED TimedOut itself: it asserted the verdict
the code was supposed to reach. The classification is now a delegated awaited_outcome helper driven by a
real future against a paused clock, with a truthful control that finishes inside its budget.

Gap 2 — the clamp was double-applied. The ceiling was enforced at ingress and again on every read,
so removing the ingress clamp was undetectable. A second guard that silently covers for the first is a
guard whose removal nothing can catch; clamping now happens at construction only, making the ceiling an
invariant of the type.

A design defect the PRE-EXISTING tests caught

The first version short-circuited the whole of locate_holders on a cache hit, so a node holding any
first-hand record would stop asking its peers for the rest of the TTL. Two shipped tests —
the_node_wide_ceiling_refuses_a_forward_when_every_slot_is_held and
the_relay_allowance_is_per_requestor_and_separate_from_the_lookup_budget — failed, because both bounds
read as though they LATCHED. The cache now replaces the DHT leg only.

My own cache test had used the forwarded-ask count as a proxy for "discovery ran", and that proxy passed
against the broken version.
It now counts DHT lookups directly via a CountingLocator.

SPEC diff

  • §10.4.4 — the "a peer that does not return providers MUST be read as 'found nobody'" reading is
    REPEALED; four outcomes are now distinguished. Adds the cascade rule (+ -32009), the time-budget
    clause (budget_ms, clamp, decrement), the ask-identity clause, and the merge_answers adoption. The
    hearsay MUST is unchanged.
  • §10.4.5"Nothing is retained" narrows to "No HEARSAY is retained", with the disclosure claim
    preserved rather than re-derived.
  • §10.4.7 (new) — the first-hand holder cache: first-hand-only, key/TTL/bound/eviction, no empty
    slate, discovery-shortcut-not-an-answer, invalidation, memory-only.

Dep bumps -- the cascade, and the ONE that was measured and refused

dig-rpc-protocol  0.6  -> 0.10   (dig-node-core:135, dig-node-service:106)
dig-download      0.18 -> 0.19   (dig-node-core:302 AND :405, the features=["testkit"] entry)
dig-peer          0.9  -> 0.11   (dig-node-core:314)
dig-keystore      0.3.1 -> 0.9.0
dig-constants     0.8.0 -> 0.10.1

Each resolves to exactly ONE Cargo.lock entry -- grep -c 'name = "<crate>"' Cargo.lock is 1 for
dig-rpc-protocol, dig-download, dig-peer and digstore-chain. tests/dependency_tree.rs
asserts the singleton property mechanically and now pins the 0.10. line, so a 0.x caret that
silently failed to reach the next minor fails a test rather than 49 type errors later.

digstore-chain 4c34f0be -> 222f08d: MEASURED, then REVERTED, and the reason is on the money path

The bump itself works -- all 13 digstore.git deps move, the lock resolves digstore-chain 0.26.0.
It then fails with 19 type errors that are all one thing: 222f08d carries the chia-wallet-sdk
0.34 uplift, so digstore-chain speaks chia-protocol 0.36.1 while dig-node speaks chia-protocol 0.26. Both lines were already in the lock before the bump; what the bump does is move
digstore-chain to the far side of that boundary.

Nine errors are dig-node's own ChainReads impl (store_melted.rs:1318-1381), and two of those are
push(SpendBundle) and estimate_fee(SpendBundle). Converting a SpendBundle between two
chia-protocol versions is precisely the byte-drift class dig_ecosystem#2610 exists to remove, and
confirm_melt_via_chain on the other side of the same trait authorizes a store DELETE. A boundary
shim there is worse than not bumping, so this lane did not write one.

Making it one chia line requires moving chia-peer 0.1.3, chia-query 0.5.1 + 0.6.2,
chia-wallet-sdk 0.30, dig-merkle 0.4.5, dig-store, dig-store-cache, and replacing the
chia = "0.26" umbrella in two manifests (dig-node-service:229, dig-wallet:35) with the five
direct facades -- the umbrella has no 0.36 line (0.32 -> 0.42). That is #2610's dig-wallet child
plus dig_ecosystem#2228, not a rev bump, and it is left for that family.

Premise correction: dig-node does NOT carry two digstore revs. All 13 digstore.git deps sit at
one rev, 4c34f0be. 51054a41 is a dig-gossip rev (dig-node-core/Cargo.toml:224,387), a
different repository. There was nothing to reconcile.

Canonical wire adoption -- the local declarations are GONE

dig-rpc-protocol 0.10 owns the four wire additions this PR had hand-declared, so they are adopted
rather than restated:

  • CONTENT_MISS_INCONCLUSIVE -- the local pub const at download.rs:112 is DELETED. Every site
    now reaches ErrorCode::ContentMissInconclusive. The number does not change (-32017 was
    already the value the owner assigned) -- what changes is who may change it. The code also LEAVES the
    local LOCAL_WIRE_CODES collision table, because the owner answers that question for it now; the
    guard asserts the adoption instead of re-declaring the number.
  • budget_ms / ask_id / redirect_depth -- forwarded_request builds
    GetAvailabilityParams and serializes it rather than spelling the field names a second time. A
    local round-trip between two hand-written spellings agrees with itself while disagreeing with every
    other node, and no test in this repo can see that.
  • absence_established -- read through a three-state type (below).

SPEC.md no longer restates the contract: it names dig-rpc-protocol as the definition, records the
origin as Peer, and reproduces the number for readability only with the crate authoritative if
the two ever differ.

Three-state semantics -- adoption changed BEHAVIOUR, it did not just recompile

absence_established was being collapsed with unwrap_or(true), which the taxonomy owner names
in its own docs as the wrong collapse: it turns an unknown into an assertion of absence. That is the
manufactured not-found #273 exists to prevent, arriving through the compatibility door rather than
through an attack. Replaced with SubtreeClaim { Established | NotEstablished | NoClaim }; only
Some(true) lets this node carry a peer's absence forward as proven, and the weakest item in a
batch decides
.

The old rationale (a mixed network would read as inconclusive everywhere) is answered rather than
ignored: the harms are asymmetric -- an over-reported inconclusive costs a retry, an over-reported
absence costs content that exists and cannot be found -- and the cost does not arrive by default,
because the forwarded leg ships DISABLED and a node that asks no peer never reads the field.

budget_ms already kept its three states; what was missing was the behavioural half. Some(0)
means exhausted, do not ask onward, and nothing pinned that a hop granted zero time asks nobody.

A test per state

field state test
absence_established Some(true) / Some(false) / ABSENT, read side absence_established_is_read_as_three_states_and_absent_is_not_true
absence_established weakest-item-in-a-batch one_unproven_item_makes_the_whole_batch_unproven
absence_established emit side: absent when no search ran, present when one did absence_established_is_absent_when_no_search_ran_and_present_when_one_did
budget_ms absent / 0 / granted, all three pinned to DIFFERENT expected values budget_ms_keeps_absent_distinct_from_zero_and_from_a_granted_value
budget_ms Some(0) behaviour: asks nobody AND claims no absence an_exhausted_budget_asks_nobody_and_does_not_claim_the_absence

Revert-proofs for this round

Each fix reverted ALONE, from a COMMITTED tree (file copy, not git checkout), and restored.

Reverted None => NoClaim back to the unwrap_or(true) collapse -- two tests fired:

"a peer that says NOTHING about its search has not said the search succeeded; reading its silence as
an establishment is the unwrap_or(true) the taxonomy owner names as wrong"

"a silent item leaves the batch unproven even beside an established one"

Reverted the budget_ms zero/absent distinction (.filter(|ms| *ms > 0)) -- the assertion that
fired is the side effect, which is deliberately first:

"a hop granted zero time must not ask onward - relaying on time it was never given is the
amplification the budget exists to bound"

That ordering matters: an implementation that asked onward with no time and then reported inconclusive
because everything timed out would satisfy the conclusiveness assertion while doing exactly what an
exhausted budget forbids.

Two fixture corrections found by making the change

  • answer_with (the shared miss fixture) OMITTED absence_established while claiming to model "a
    peer that looked and found nobody". Under correct semantics that fixture models a peer that cannot
    describe its search at all. It now states true explicitly; the absent case has its own test.
  • AskOutcome::into_records had no callers once the outcome arms were matched explicitly -- deleted
    rather than left as dead code.

Also in this round

  • ForwardedAnswers::not_asked split into recursion_disabled() and refused(). Only recursion
    being off is conclusive; a spent budget, a saturated slot pool, or a walk claimed by another path is
    a leg that was supposed to run and did not. Under a burst the saturation cases are the COMMON path,
    so collapsing them turns load into manufactured not-founds exactly when the network is busiest.
  • A failed DHT walk stays a failure. find_providers was unwrap_or_default(), so a node with no
    reachable DHT peer asserted a proven absence for every piece of content in existence.
    absence_established is now the CONJUNCTION of both legs having finished.
  • The ask_id is echoed, not minted per hop -- it travels with the ask, so the diamond dedup fires
    beyond the first hop. AskSeenSet::claim is keyed by (id, content), with a test that the same id
    asking about different content is a different question (otherwise the first item in a batch
    suppresses the walk for every other item in it).

Gate round 3 - the DHT-leg fix was dead code in production (GATING, HIGH)

Round 1 fixed the swallow at walk_for_providers's Err arm. Round 2 re-fixed it. Both were
unreachable, because the swallow is two layers below where anyone looked:

  • union_locator.rs - let Ok(records) = result else { continue; }, only return Ok(merged). So
    UnionLocator::find_providers was infallible.
  • capsule_fallback.rs - two .unwrap_or_default() calls, then Ok(merged).

A failed DHT walk therefore arrived as Ok(vec![]), first_hand_conclusive was set true, and the
conjunction that computes absence_established had no way to clear. A node whose DHT walk fails -
start-up before any DHT peer answers, a partition, an eclipsed routing table, an off-path attacker
degrading its DHT RPCs - answered absence_established: true and MissOutcome::NotFound for content
that exists
, and a hop relays that onward. No forged message required, and worse on a stock node
where recursion ships OFF and this conjunct is the entire search.

The rule now held at every layer: best-effort for FINDING, strict for ABSENCE. A failing leg never
removes what another leg found, so a non-empty answer is unchanged and returns Ok; an EMPTY result
carrying a source failure reports that failure. One failing leg out of three weakens the answer,
and only when it could have been the leg with something to say - it never poisons an answer another
leg supplied. Stated normatively in SPEC.md 10.4 so a reimplementation cannot rebuild the swallow.

Why the suite missed it, and what changed. engine_over hands its double straight to
NodeContent::new, bypassing both swallowing layers - every locator test drove a shape production
never builds. The chain is extracted to NodeContent::provider_locator_chain, the single construction
site for_dht uses, and a_failed_dht_walk_stays_unproven_through_the_production_locator_chain drives
that. It failed on the unfixed code on its first assertion; reverting either guard alone re-fails
it plus that layer's own unit test. Every arm keeps an honest control (all sources completed, found
nobody -> still establishes_absence()), so the fix cannot be satisfied by never concluding anything.

Also in this round:

  • items: [] no longer folds to the Established identity (forwarded_ask.rs) - it handed a
    responder a proven absence for the price of the cheapest message on the wire. NoClaim now.
  • ask_id is pinned onto the request bytes - it was emitted but unasserted, so a regression
    dropping it would leave the diamond dedup inert with nothing red.
  • SPEC 10.4.7's holder-cache TTL corrected from ADVERTISED_TTL_SECS (3600s) to the 300s the code
    enforces. The SPEC carried a rationale holder_cache.rs:71 itself calls "the wrong claim"; a second
    implementation built from it would hold an attacker-seeded slate 12x longer.

Blast radius checked

ProviderLocator::find_providers - every production consumer of the two changed impls:

  • NodeContent::walk_for_providers -> locate_holders (the absence path, the subject of the fix).
  • NodeContent::find_providers (the documented infallible form) -> fetch_resource's candidate count
    and bandwidth.rs - neither claims an absence.
  • the DOWNLOAD locator (download.rs:1132), which unions the pool source with this chain: a failed
    discovery leg beside an empty pool now surfaces as an error rather than a silent zero-provider
    download, and a pool holder still returns Ok. No fetch regression.
  • module_transport.rs's discovered_candidates keeps its Ok-else-empty: it collects dial address
    hints and makes no absence claim, which its own doc states.

UnionLocator::new / CapsuleFallbackLocator::new have no other discovery-path construction site.

Version

0.135.0 -> 0.136.1 (workspace), dig-node-core 0.49.0 -> 0.50.1 (Cargo.lock own-version entries
updated to match). The gate-round-3 fix is a patch on top of the round's minor: no public API changed
(UnionLocator/CapsuleFallbackLocator are pub(crate); NodeContent::find_providers keeps its
signature and stays infallible).

Original round-1 version note: 0.135.0 -> 0.136.0. Rebased onto origin/main e4afb52 (v0.135.0, containing PR#291 8d82aca7) with ZERO conflicts -- main was already an ancestor. Minor: new capability plus additive
wire fields. The one behaviour change a client can observe is an inconclusive miss answering -32009
instead of a not-found — which is the point of #273.

Notes

  • NC-12 holds throughout: every cached or forwarded record is a candidate to DIAL and never a fact; the
    whole-resource merkle bind against the chain-anchored root is what admits bytes. A hop's "not found" may
    be a lie, which is precisely why an unproven absence is no longer laundered into a proven one.
  • The stub commit on this branch had appended an HTML comment into a .rs file, which was a syntax
    error; the branch did not build as pushed. Removed.
  • dig-keystore 0.3.1 -> 0.9.0 is unforced but verified clean against crates.io: kdf.rs is
    byte-identical, DIGOP1 / SCHEME_ID 0x0004 / FORMAT_VERSION_V1 are unchanged, and error.rs is
    additive. Carried rather than reverted, and called out here rather than left silent.
  • The previous lane's uncommitted worktree held substantial real work (the outcome type and the budget
    arithmetic). It was committed before anything else so a second cap could not lose it.

@MichaelTaylor3d
MichaelTaylor3d force-pushed the loop/273-275-ask-outcome branch from 59688dd to 9304dc1 Compare August 21, 2026 09:35
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Resume-ready progress — lane restarted after the previous cap

Branch: loop/273-275-ask-outcome · head: 9304dc1 · rebased onto main = e4afb52 (v0.135.0).

Recovered work the previous lane never committed

The capped lane had left uncommitted work in its worktree — not just the stub the PR showed. It is
now committed as 9304dc1 and pushed, so a second cap cannot lose it:

  • AskOutcome { Answered(Vec<ProviderRecord>) | Refused | TimedOut | Unreachable } with records() and
    is_conclusive().
  • FORWARDED_ASK_TIMEOUT split into FORWARDED_ASK_LEAF_TIMEOUT (5s, one leaf ask) plus
    ask_budget(hops_remaining, fan_out) — the recursive budget — clamped by
    MAX_FORWARDED_ASK_BUDGET = 65s.
  • parse_forwarded_answer: an error frame is Refused, a result frame is Answered even when empty,
    anything else is Unreachable.
  • an_error_frame_yields_nobody replaced by an_error_frame_is_a_refusal_and_not_an_answer_of_nobody.
  • Time budget rides a NEW params.budget_ms field, not redirect_depth — the two move in opposite
    directions (time decreasing, depth increasing), so one integer cannot honestly carry both.

It does not build yet: callers are unmigrated. That is the next step, and it is deliberate — the
salvage was committed before it compiled precisely so it could not be lost again.

Remaining

  1. Migrate the callers: forwarded_holders / locate_holders (download.rs) + every test double, and
    thread budget down with decrement.
  2. MissOutcome: add the inconclusive variants and carry them to the wire.
  3. Provider cache: remember which peers hold what, with a TTL (requirement 7 -- missing from the epic until now) #275: first-hand-only provider cache; adopt dig_sex::discovery::merge_answers + Provenance.
  4. SPEC editorial pass (§10.4.4 unamended; §10.4.5 "nothing is retained" -> "no hearsay is retained").
  5. Dep bumps: dig-download 0.17 -> 0.18, dig-keystore 0.3 -> 0.9 (verify the lock MOVES, not duplicates).

Next command

cargo check -p dig-node-core --all-targets 2>&1 | grep -E "^error" | head -40

@MichaelTaylor3d
MichaelTaylor3d force-pushed the loop/273-275-ask-outcome branch from e682014 to 7a17e6c Compare August 21, 2026 10:44
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Lane complete — ready for the gate round. Still DRAFT; do not merge.

Branch loop/273-275-ask-outcome · head 7a17e6c · one squashed commit on main = e4afb52
(v0.135.0). 12 files, +1835/-137.

CI — all 14 required checks PASS by name

Analyze (actions|javascript-typescript|rust) · Check version increment · Clippy · CodeQL ·
Lint commit messages · Release-script tests · Rustfmt · build .deb (linux-amd64|linux-arm64) ·
build .msi (windows-x64) · build .pkg (macos-universal) · Test + coverage (25m9s).
Attach packages to the release is skipping — release-only, correct on a PR.

  • Coverage 88.40% lines / 87.20% regions / 82.99% functions, against the ≥80% line gate.
  • Zero unresolved review threads.
  • Local cargo test --workspace: 1381 passed, 0 failed. cargo clippy --workspace --all-targets -D warnings and cargo fmt --all --check both clean.

Two false greens found and closed during revert-proofing

Both were MY tests, and both came back green when their fix was reverted:

  1. The timeout proof was vacuous. It drove a ForwardedAsk double that RETURNED TimedOut itself, so
    it asserted the verdict the production code was meant to reach. Reverting the real mapping to
    Answered(vec![]) changed nothing. Fixed by delegating the classification to awaited_outcome and
    driving it with a real future against a paused clock, plus a truthful control that finishes in budget.
  2. The budget ceiling was clamped twice — at ingress and again on every read — so removing the ingress
    clamp was undetectable. Clamping now happens at construction only, which makes the ceiling an invariant
    of the type and the ingress clamp falsifiable.

A design defect the PRE-EXISTING tests caught

My first cache version short-circuited the whole of locate_holders on a hit, so a node holding any
first-hand record would stop asking its peers for an hour. the_node_wide_ceiling_refuses_a_forward_...
and the_relay_allowance_is_per_requestor_... both failed because the two bounds read as though they
LATCHED. The cache now replaces the DHT leg only — and my own cache test had used the forwarded-ask count
as a proxy for "discovery ran", which passed against the broken version; it now counts DHT lookups
directly.

For the gate's attention

  • HIGH risk: ForwardedAsk::ask's signature (crate-local) and the dig.getAvailability answer shape.
    A node that cannot establish an absence now answers -32009 where it previously answered a plain
    not-found. Intended, and stated in SPEC §10.4.4.
  • impact/detect_changes were unavailable (no .gitnexus index in this worktree); the blast radius was
    established by call-graph grep + direct read and is tabulated in the PR body, with a clean
    workspace-wide clippy as the corroborating enumeration.
  • dedup_by_peer was deleted as a rival implementation once its tagged twin landed.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

GATING — orchestrator finding, before the gate round: -32009 is ALREADY TAKEN, and the contract is defined in the wrong repo

Found while reviewing the hand-back, not by a gate. Two problems, one root.

1. Wire error-code COLLISION

This PR introduces CONTENT_MISS_INCONCLUSIVE = -32009. That code is already assigned:

modules/crates/00-foundation/dig-rpc-protocol/src/error.rs:108
    RangeMetadataUnrepresentable = -32009,

and SYSTEM.md records it normatively — "the holder answers it when metadata ALONE cannot fit a frame, and a client MUST treat it as holder-fatal (skip, do not retry that holder for that range, do not count it as a transport failure)".

The two demand OPPOSITE client behaviour, which is what makes this more than a numbering nit:

Code meaning What a correct client does
RangeMetadataUnrepresentable holder-fatal — never ask this holder for this range again
CONTENT_MISS_INCONCLUSIVE absence NOT established — keep looking, ask someone else

A client receiving -32009 cannot tell them apart. It will either permanently blacklist a holder that was merely uncertain, or keep re-asking a holder that can never serve the range. Neither is recoverable by retry logic, because the ambiguity is in the contract.

The Node band's next free code is -32015. Assigned today: -32000, -32003..-32014; -32001/-32002 unused; Onion -32020..-32022; Control -32030..-32032.

Worth noting error.rs already carries a section headed "The -32020..-32022 collision, resolved". This is the second instance of the same class in one file, which is a signal about the process, not about this lane.

2. The contract belongs in dig-rpc-protocol, release-first

SYSTEM.md is explicit: "The RPC contract — method names, request/response types, the error-code taxonomy, tier classification and the generated OpenRPC document — is defined ONCE in the dig-rpc-protocol crate (single source of truth)... a shared-contract change is release-first in dig-rpc-protocol, then adopted by the consumers in the same unit."

So all four of this PR's wire additions are dig-rpc-protocol's to declare, not dig-node's:

  • the new error code,
  • params.budget_ms (the hop time budget),
  • params.ask_id (the request identity),
  • absence_established on the dig.getAvailability answer.

Declaring them locally means rpc.dig.net and every other implementation is built against a contract that does not mention them — and the collision above is exactly what that SSOT exists to prevent, since a crate that owns the whole taxonomy cannot hand out a duplicate.

What happens next

A dig-rpc-protocol change publishes first (additive, minor), then this PR re-pins and consumes the canonical constants. Nothing else in the PR is affected.

Everything else here is strong, and is not being re-litigated

Recorded so the gate does not redo it:

  • The branch as pushed did not compile. The dead lane's push-early stub was an HTML comment inside a .rs file (forwarded_ask.rs:615). It also had substantial uncommitted work in its worktree, committed first before anything else. Both worth knowing: a stub must be something the compiler accepts, and a dead lane's worktree is checked before its branch is trusted.
  • Two of its own tests were false greens, both the same shape — a double that returned the verdict under test, so the test asserted the answer the production code was supposed to reach. And the budget ceiling was clamped twice, making removal of the ingress clamp undetectable. Both fixed; both are the kind of thing that normally survives a gate.
  • A real design defect caught by pre-existing tests: the first cache short-circuited all of locate_holders on a hit, so a node holding any first-hand record would stop asking peers for an hour — making two shipped bounds read as though they LATCHED. The lane's own cache test used forwarded-ask count as a proxy for "discovery ran" and passed against the broken version; it now counts DHT lookups.
  • A refusal to forward stays conclusive — recursion ships disabled by default, so the opposite would make every miss on every default node inconclusive. That is the right call.
  • Cache: first-hand only, so SPEC §10.4.4 stands unamended; TTL 3600s deliberately equal to ADVERTISED_TTL_SECS, so a record expires when its claim would.
  • MAX_FORWARDED_ASK_BUDGET = 65s is derived, being exactly ask_budget(2,3) at the dig-sex defaults, not chosen.
  • Deps moved without duplication: dig-download 0.17.0→0.18.0, dig-keystore 0.3.1→0.9.0, dig-constants 0.8.0→0.10.1, exactly one of each resolving. That also completes epic #3145's dig-wallet step — six minors applied with no code change.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Adoption lane — progress + a BLOCKER on the canonical number

Branch loop/273-275-ask-outcome, head 7a17e6c, worktree C:\tmp\worktrees\dn-273-275 (intact, clean). Staying DRAFT.

BLOCKER: ContentMissInconclusive = -32015 collides with a SHIPPED dig-node code

dig-rpc-protocol 0.9.0 assigns ContentMissInconclusive = -32015 as "the Node band's next free code". Measured against this repo, it is not free:

crates/dig-node-core/src/lib.rs:1363    const METADATA_TOO_LARGE:   i64 = -32015;
crates/dig-node-core/src/seams/capsule/push_capsule.rs:64
                                        const PUSH_PENDING_LIMITED: i64 = -32016;

METADATA_TOO_LARGE is not a draft — its own doc says "Catalogued in docs.dig.net (L7 error catalog)", it is emitted from three live sites (lib.rs:3163, :3179, and asserted at :10880), and openrpc_drift_guard.rs:172 already exists to assert "Two distinct conditions must never collide on one code".

The two meanings are once again opposite: METADATA_TOO_LARGE says this holder can never render this section (stop), ContentMissInconclusive says the question was not answered (keep looking). This is the same defect the gating comment describes, one code to the right — because 0.9.0 picked "next free" by reading only its own taxonomy, and dig-node hand-declares codes that taxonomy never saw.

The true occupancy map (this repo, all -320xx literals)

-32000 -32001 -32003..-32016 -32020..-32022 -32030..-32033
-32040..-32044 -32050..-32052 -32060

Canonical 0.9.0 knows only -32000, -32003..-32015, -32020..-32022, -32030..-32032. So dig-node holds -32001, -32016, -32033, -32040..-32044, -32050..-32052, -32060 entirely undeclared, plus the -32015 collision.

First code free in BOTH: -32017.

Which side moves, and why

METADATA_TOO_LARGE is released and documented; ContentMissInconclusive has never left this draft. The unreleased one moves. So dig-rpc-protocol needs a 0.10.0 that (a) reassigns ContentMissInconclusive to -32017, and (b) absorbs dig-node's shipped -32015/-32016 into the taxonomy so this cannot recur a third time.

What this lane does meanwhile

  1. Pins 0.60.9 (dig-node-core/Cargo.toml:135, dig-node-service/Cargo.toml:106) — the budget_ms / ask_id / absence_established declarations are canonical and do not collide, so adopting them is unblocked.
  2. SPEC.md stops restating the contract and names dig-rpc-protocol as its owner.
  3. Retargets the local const off the -32009 collision onto -32017, marked provisional pending 0.10.0 — shipping the gating comment's own number would be shipping a second collision, so this fixes the live harm now and the const is deleted on adoption.
  4. Adds the guard that would have caught both: a test asserting no dig-node wire code equals a differently-named canonical ErrorCode. It fails at -32009 and at -32015, passes at -32017.

NEXT ACTION: bump both pins, cargo build -p dig-node-core, confirm grep -c 'name = "dig-rpc-protocol"' Cargo.lock stays 1.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Adoption BLOCKED on a 3-crate release-first cascade — what landed instead

Head 93c016c, DRAFT, workspace green (cargo test --workspace exit 0; 894 in dig-node-core).

The pin bump is not possible from this repo alone

Bumping both pins to "0.9" was tried and reverted. It does not fail as a lint — it fails to compile, because the wire TYPES cross an API boundary with a published crate still on 0.6:

error[E0308]: expected `FetchModuleRangeParams`, found a different `FetchModuleRangeParams`
note: there are multiple different versions of crate `dig_rpc_protocol` in the dependency graph

cargo tree -i dig-rpc-protocol@0.6.0 names two published holders:

dig-rpc-protocol v0.6.0
├── dig-download v0.18.0  -> dig-node-core
└── dig-peer     v0.9.0   -> dig-download, dig-node-core

dig-node-service/Cargo.toml:106's own comment anticipated exactly this — "Two majors in one workspace also duplicates the wire TYPES". So the cascade is dig-rpc-protocol -> dig-peer -> dig-download -> dig-node, and dig-node is last. Cargo.lock is back to exactly one dig-rpc-protocol (0.6.0).

And the canonical number cannot be adopted even after the cascade

ContentMissInconclusive = -32015 is not free. -32015 is dig-node's released METADATA_TOO_LARGE — normatively assigned in this repo's own SPEC.md error table (line 2963), doc-commented as "Catalogued in docs.dig.net (L7 error catalog)", emitted from lib.rs:3163/:3179. -32016 is likewise PUSH_PENDING_LIMITED. Neither appears in dig-rpc-protocol's taxonomy, so 0.9.0's "next free code" was free only in its own list.

Occupancy in this repo: -32000, -32001, -32003..-32016, -32020..-32022, -32030..-32033, -32040..-32044, -32050..-32052, -32060. Ecosystem-wide, -32017/-32018/-32019 occur zero times.

Recommended: dig-rpc-protocol 0.10.0 reassigns ContentMissInconclusive to -32017 (the unreleased side moves; METADATA_TOO_LARGE is shipped and documented) and absorbs -32015/-32016 into the taxonomy so this cannot happen a third time.

What this PR does now

  1. CONTENT_MISS_INCONCLUSIVE moved -32009 -> -32017. The gating harm was live and is gone: -32009 is RangeMetadataUnrepresentable, holder-fatal, the opposite instruction. The const is marked PROVISIONAL and names dig-rpc-protocol as the taxonomy's owner; it is deleted in favour of ErrorCode::ContentMissInconclusive once the cascade lands.

  2. The guard that would have caught both, no_local_wire_code_collides_with_a_different_canonical_code: every number this node emits, checked against ErrorCode::ALL by (number, machine_code) and against the other local conditions. Revert-proofed — and the two legs are independently load-bearing:

    • at -32009: "local CONTENT_MISS_INCONCLUSIVE = -32009 is already canonically RANGE_METADATA_UNREPRESENTABLE, and the two do not mean the same thing — a client cannot tell them apart" (canonical leg, lib.rs:4725)
    • at -32015: "local METADATA_TOO_LARGE = -32015 collides with local CONTENT_MISS_INCONCLUSIVE" (local leg, lib.rs:4735)

    A test asserting only != -32009 passes on the second bug, which is why one leg is not enough. Side-effect assertions run first (table size, the code under review present, ErrorCode::ALL non-trivial) so a shrunken table cannot make it vacuous.

  3. SPEC.md records dig-rpc-protocol as the taxonomy's owner, states -32017 is provisional and why, and backfills the missing -32017 error-table row (CONTENT_MISS_INCONCLUSIVE was absent from the table entirely).

  4. A test per three-state field:

    • budget_ms_keeps_absent_distinct_from_zero_and_from_a_granted_value — absent -> derived (non-zero), Some(0) -> Duration::ZERO, Some(4000) -> honoured, plus an explicit assert_ne! on absent-vs-exhausted. Revert-proof: collapsing from_params to .unwrap_or(0) fires the ABSENT leg (left: 0ns, right: 5s).
    • absence_established_is_absent_when_no_search_ran_and_present_when_one_did — the engine-less control makes NO claim (key absent), the engine-attached node claims true. Both miss the item, so available cannot be what distinguishes them. Revert-proof: inserting false unconditionally fires the control leg.
    • ask_id already had all three (the_same_ask_arriving_twice_is_forwarded_once, requests_without_an_identity_do_not_collide_with_each_other).

Version: unchanged at 0.136.0 / dig-node-core 0.50.0. -32009 never left this branch — it was introduced at 7a17e6c in this same unreleased PR — so the number change is not observable to any released client, and the existing minor already covers the feature.

Reported, not fixed here

  • -32050/-32051/-32052 (chat.rs:374-380, module rpc_code): NO_IDENTITY (no persistent identity key, cannot seal as sender), NO_PEER_NETWORK (no gossip pool, a directed send has no transport), SEND_FAILED (the seal or the directed send failed). Chat is not a separate protocol — they are served on the node's ordinary JSON-RPC surface, dispatched from seams::dig_rpc, and the module's own doc calls them "the node's private application range". It is a private band on the shared surface, which is the same undeclared-band condition, so it should be declared canonically. Note the same module correctly reuses the standard -32602 for bad params, so the pattern is already half-adopted.
  • Only FIVE dig_rpc_protocol::ErrorCode references exist in the whole repo (two of them added by this PR), against ~145 raw -3200x literals. The literals that should become ErrorCode::... once the cascade lands: RESOURCE_UNAVAILABLE/RESOURCE_NOT_AVAILABLE -32004 -> ResourceUnavailable (three definitions of one condition across download.rs:110, lib.rs:136, content_serve.rs:37 — the duplication is its own finding), ROOT_NOT_ANCHORED -32005 -> RootNotAnchored, CONTENT_REDIRECT -32008 -> ContentRedirect, CONTENT_MISS_RATE_LIMITED -32003 -> ContentMissRateLimited.
  • download.rs:77 asserts a claim its pin cannot check: "MUST equal dig_rpc_protocol::ErrorCode::ContentMissRateLimited", but that variant does not exist in 0.6.0 (zero matches) — it arrives in a later minor. Nothing currently holds -32003 to canonical. Checkable once the cascade lands.
  • content_serve::SERVE_UNREADABLE -32000 specialises the canonical SERVER_ERROR under a different name. Deliberately excluded from the guard table, and flagged rather than silently omitted: it needs a decision (specialise-and-declare, or fold into SERVER_ERROR), not a swap.

NEXT ACTION: none in this repo — blocked on dig-rpc-protocol 0.10.0, then dig-peer, then dig-download. Handing back to the orchestrator for the cascade.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — INTERIM FINDINGS (1 of 2 batches), audited head 93c016c78731dfec7147b7695973445890b6fc23

Two GATING findings so far, both in the same place: the not-found / timed-out distinction this PR
exists to create is emitted but never consumed, and the leg that actually runs by default is not
covered by it at all.
Posting now rather than at the end.


SEC-1 (GATING, HIGH) — inconclusiveness does NOT cascade back down: absence_established is write-only

crates/dig-node-core/src/lib.rs:3766 is the only occurrence of absence_established in
non-test code. Grep at this head:

crates/dig-node-core/src/lib.rs:3766:   "absence_established".into(),

One emit site. Zero readers. And the function that reads a hop's answer does not look for it:

crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs:262-271

pub(crate) fn parse_forwarded_answer(content: &ContentId, response: &Value) -> AskOutcome {
    if response.get("error").is_some() {
        return AskOutcome::Refused;
    }
    if response.get("result").is_none() {
        return AskOutcome::Unreachable;
    }
    AskOutcome::Answered(parse_forwarded_providers(content, response))
}

A result frame with no providers is Answered(vec![]), which AskOutcome::is_conclusive treats as
a real answer and which download.rs:1647 folds in without clearing answers.conclusive. So a
hop that honestly reports absence_established: false is read by its parent as a conclusive
"I looked and found nobody".

Exploit. Content C exists. Attacker runs one node X, two hops from victim reader R.

  1. R asks node A. A misses, forwards to its pool peer B.
  2. B misses, forwards to its own pool — which includes X.
  3. X accepts the connection and stalls, answering nothing until B's budget expires.
  4. B classifies X as TimedOut, correctly clears its own flag, and answers A with
    absence_established: false and no providers. B is behaving perfectly.
  5. A parses B's frame: it has a result, so Answered(vec![]). answers.conclusive stays true.
  6. A answers R absence_established: true / MissOutcome::NotFound.

R is told, authoritatively, that content which exists does not exist — the exact outcome
MissOutcome::Inconclusive's own doc comment says the type exists to prevent
(download.rs:2117-2126: "one slow peer converts into an authoritative absence — and, since a hop's
answer is itself relayed, propagate that absence downwards"
). Cost to the attacker: one node holding
one TCP connection open. No forgery, no key, no position on the path.

The PR body claims the cascade rule was added to SPEC §10.4.4. The wire field was added; the
reader was not. The distinction is preserved for exactly one hop — the hop this node dials itself
— and is laundered at every hop above it.

What it needs: parse_forwarded_answer must read result.items[].absence_established and map a
false (or a mixed/absent-on-a-recursion-capable-peer reading) to a non-conclusive outcome, and
AskOutcome::Answered needs to carry that bit rather than implying it.


SEC-2 (GATING, HIGH) — conclusive ignores the DHT leg entirely, so a FAILED lookup is reported as an established absence

download.rs:1507-1510:

LocatedHolders {
    records,
    conclusive: forwarded.conclusive,
}

conclusive is derived solely from the forwarded leg. The first-hand leg is
download.rs:1703-1712:

let found = self.locator.find_providers(content).await.unwrap_or_default();

unwrap_or_default() turns a DHT lookup error — no reachable DHT peers, a transport failure, a
timeout inside the walk — into an empty Vec, indistinguishable from "walked the ring, found nobody".
Nothing downstream can tell the two apart, and conclusive never sees either.

Now combine with ForwardedAnswers::not_asked() (download.rs:863-867), which returns
conclusive: true, and with the fact — stated in this PR's own comments at download.rs:1599-1602
— that recursion ships DISABLED by default.

Result on a default-configured node: absence_established is a constant true. Every miss, on
every stock node, asserts a proven absence, whether or not its DHT walk succeeded, whether or not it
has a single DHT peer, whether or not the lookup errored.

Exploit. No forwarded leg is even needed.

  • A node whose DHT connectivity is degraded — cold start after the peer network attaches, a partition,
    an eclipse of its few DHT contacts, or simply an operator behind a filtering NAT — answers
    absence_established: true for all content in the universe.
  • The redirect ladder (§5.3) means a browser-tier reader may be talking to exactly such a node, and
    rpc.dig.net is an ordinary node on that ladder.

Before this PR that node answered a bare not-found, which a client was free to read tolerantly. This
PR converts that ambiguity into a positive assertion the client is explicitly told to act on.
Turning "I have nothing to say" into "I have established there is nothing" is a strictly worse
failure than the one being fixed, and it lands on the default path rather than the opt-in one.

ForwardedAnswers::not_asked()'s justification — "reporting a refusal as inconclusive would make
every miss on every default-configured node inconclusive"
— is a sound argument that a refusal to
forward
is not evidence. It is not an argument that the DHT leg's own failure is evidence, and
the code applies it to both by never modelling the DHT leg's outcome at all.

What it needs: find_providers must surface Ok-empty vs Err (the shape AskOutcome already
uses for the forwarded leg), and LocatedHolders::conclusive must be the AND of both legs. A node
that cannot reach the DHT must emit absence_established: false, or omit the field.


Provider-cache, held-state and error-code findings follow in a second comment.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

ADVERSARIAL VERIFY — REFUTED. This PR does not ship the distinction it claims to ship.

Head 93c016c7. Five probes written against this head, compiled and RUN; all five fired, each on the
assertion named below. Probe source is in a throwaway worktree, not proposed for merge.

running 5 tests
test adv_d_the_forwarded_request_omits_the_ask_id_so_no_hop_can_echo_it ... FAILED
test adv_a_an_inconclusive_child_result_frame_is_read_as_a_conclusive_answer ... FAILED
test adv_a_a_saturated_concurrency_ceiling_is_reported_as_an_established_absence ... FAILED
test adv_a_a_failed_dht_walk_is_reported_as_an_established_absence ... FAILED
test adv_a_a_spent_hop_budget_is_reported_as_an_established_absence ... FAILED
test result: FAILED. 0 passed; 5 failed; 894 filtered out

A — "one slow peer can no longer become an authoritative absence": REFUTED, four independent ways

A1 — the DHT leg's failure is discarded, and it is the leg that runs on EVERY node

download.rs:1507-1509:

LocatedHolders {
    records,
    conclusive: forwarded.conclusive,   // <- the DHT leg is not consulted at all
}

and download.rs:1710-1715:

pub async fn find_providers(&self, content: &ContentId) -> Vec<ProviderRecord> {
    let found = self.locator.find_providers(content).await.unwrap_or_default();

dig-download's own trait doc (locate.rs:20) states the distinction this .unwrap_or_default() throws
away: "A locate failure is a DownloadError; an empty result is Ok(vec![]) (no holders found, not an
error)."
The upstream crate already separates absence from silence; dig-node collapses it — in the PR
whose entire thesis is that collapsing it is the defect.

Consequence, and this is the load-bearing one: RecursionConfig::default().enabled == false
(dig-sex-0.4.0/src/discovery.rs:68). With recursion off, forwarded_holders returns
ForwardedAnswers::not_asked()conclusive: true — on every request. So on a default-configured
node, establishes_absence() is unconditionally true
, the DHT leg is the only leg, and its timeouts
are laundered into proven absences exactly as before. MissOutcome::Inconclusive is unreachable there.
The distinction is not merely leaky in the default posture; it is inert.

Probe: adv_a_a_failed_dht_walk_is_reported_as_an_established_absence — a ProviderLocator returning
Err(DownloadError::transport(..)). Fired on
"REFUTATION: the DHT leg ERRORED and this node still claims to have established an absence".

A2 — the cascade merge rule launders an inconclusive CHILD into a proven absence

This is the crux the brief asked me to check at the intermediary rather than the leaf, and it fails there.

What this node emits when its own search was inconclusive is not an error frame. lib.rs:3766 inserts
absence_established: false into an ordinary result frame. What a parent hop does with that
(forwarded_ask.rs:245-255):

pub(crate) fn parse_forwarded_answer(content: &ContentId, response: &Value) -> AskOutcome {
    if response.get("error").is_some() { return AskOutcome::Refused; }
    if response.get("result").is_none() { return AskOutcome::Unreachable; }
    AskOutcome::Answered(parse_forwarded_providers(content, response))   // <- absence_established never read
}

The parser never reads absence_established. A child that explicitly declared its absence unproven is
classified Answered([]) — conclusive — and the parent may then report NotFound upward. The field is
written and never read anywhere in the tree: grepping it returns one write site, one SPEC line, and three
assertions inside the one test. The same laundering happens when a child skipped its lookup because
allow_miss_lookup was exhausted: result frame, no key, read as a proven Answered([]).

So on the very verb the recursion uses, dig.getAvailability, the distinction has no wire
representation a hop acts on
. -32017 only ever appears on dig.getContent / dig.fetchRange
(download.rs:2554), which the forwarded ask does not call.

Probe: adv_a_an_inconclusive_child_result_frame_is_read_as_a_conclusive_answer. Fired,
left: Answered([]) right: Answered([]).

A3 — every node-local refusal is CONCLUSIVE, and #273 forbids exactly that

ForwardedAnswers::not_asked() (download.rs:861-868) sets conclusive: true, and four distinct paths
reach it:

path file:line reached when
decide_forward refusal download.rs:1604 Disabled, HopBudgetSpent, RelayBudgetSpent, NoEligiblePeers, UnreadableHopBudget
seen-set dedup hit download.rs:1611-1614 a duplicate ask
concurrency ceiling full download.rs:1616-1619 all 32 slots busy
redirect hop cap download.rs:2222-2224 depth >= REDIRECT_HOP_CAP, before any search runs

#273's own constraint list: "A hop that refuses (disabled, budget spent, no eligible peers) must produce
a distinguishable answer from both a genuine not-found and a timeout."
This PR makes them
indistinguishable by design and argues for it. The argument — recursion ships disabled, so treating a
refusal as inconclusive would make every default node inconclusive — is sound for Disabled only. It
does not cover HopBudgetSpent, RelayBudgetSpent, NoEligiblePeers, UnreadableHopBudget, the dedup
hit, or the saturated ceiling, all of which are runtime and peer-influenced. The brief named "a refused
relay budget" and "an exhausted hop budget" specifically; both collapse.

Probes: adv_a_a_spent_hop_budget_is_reported_as_an_established_absence (fired) and
adv_a_a_saturated_concurrency_ceiling_is_reported_as_an_established_absence (fired).

A4 — the control in absence_established_is_absent_when_no_search_ran_and_present_when_one_did proves nothing about a slow peer

The brief's suspicion is correct. The control is a node with no P2P engine, which skips the entire
if let Some(pc) = self.p2p_content() block at lib.rs:3748 — a missing component, not a failing
search
. The positive leg uses MockProviderLocator::fixed(vec![]), which succeeds. So the test
exercises absent and Some(true) and never Some(false); its own docstring concedes this
(lib.rs:9622: "The third state, Some(false) ... is exercised by the forwarded-ask tests"). Given A1
and A3, Some(false) is unreachable on a default node, so no test anywhere exercises it end to end on
this verb.


B — "the held state is bounded": bound EXISTS, but the AT-the-bound behaviour IS the failure mode #273 names — REFUTED

The bound is real and correctly shaped: tokio::sync::Semaphore::new(MAX_CONCURRENT_FORWARDED_ASKS = 32)
(download.rs:782, 1166), taken with try_acquire_owned (download.rs:1616), RAII-released — not a
counter a peer can decrement, and it bounds concurrent held fan-out sessions rather than total work, which
is the distinction from dig-sex's relay budget that #273 asks for. AskSeenSet is bounded at 8192 with a
65s TTL, and the holder cache at 4096. I found no held-state path that bypasses the semaphore.

What happens at the bound is the problem. Slot exhaustion returns not_asked() -> conclusive: true
-> authoritative NotFound. The relay budget is per-requestor but the semaphore is node-wide and shared
across every requestor
, and a slot can be held up to MAX_FORWARDED_ASK_BUDGET = 65s at an upstream
hop's choosing. So ~32 concurrent misses — spread across enough requestor identities to clear the
per-requestor buckets — hold every slot, and for those 65s every other requestor's miss on that node
answers a confident not-found
. That is "one slow peer becomes an authoritative absence" restated at
node scale, produced by the bound that was added to prevent the DoS. #273 asks the ticket to "say what it
does when the bound is reached"; what it does is emit the lie the ticket exists to remove.

Probe: adv_a_a_saturated_concurrency_ceiling_is_reported_as_an_established_absence, driven through the
PR's own hold_every_forwarded_ask_slot(). Fired.


C — the dedup_by_peer deletion: SURVIVED, with one adjacent divergence

I could not find an input where the deleted function and its twin disagree, and I am saying so plainly.

// origin/main download.rs:2213
fn dedup_by_peer(providers: &mut Vec<ProviderRecord>) {
    let mut seen = HashSet::new();
    providers.retain(|p| seen.insert(p.provider_peer_id.clone()));
}
// HEAD download.rs:2602
fn dedup_by_peer_tagged(records: &mut Vec<(ProviderRecord, Provenance)>) {
    let mut seen = HashSet::new();
    records.retain(|(record, _)| seen.insert(record.provider_peer_id.clone()));
}

Same predicate, same key, same first-wins order-preserving semantics. git grep dedup_by_peer origin/main
returns exactly one call site (download.rs:1350), so no second consumer was left behind. First-hand
records precede hearsay in both pipelines, so a duplicated peer keeps the stronger record in both.

The adjacent divergence, reported not as a refutation of C but because it is a real behaviour change:
the ORDER of cap and dedup inverted. main deduped the full merged list; HEAD calls merge_answers,
which takes only max_hearsay_answers = 8 hearsay records, and dedups after. So hearsay entries that
duplicate a first-hand peer now consume hearsay slots before being discarded — with 8 duplicate-peer
records in front, a genuinely new 9th holder that main would have surfaced is dropped. Cheap to make
robust (dedup against the first-hand key set before the cap); worth a decision, not a block on its own.


D — request identity against a DIAMOND: REFUTED. The test name flatters the coverage, and the mechanism is not wired at all

forwarded_request (forwarded_ask.rs:187-201) emits items, redirect_depth, budget_msand no
ask_id.
The identity is parsed on ingress (download.rs:2437-2441) and claimed against the seen-set
(download.rs:1611), but it is never put on the outbound wire, so it cannot survive one hop.

SPEC.md:3137 asserts the opposite as normative: "an opaque 16-byte params.ask_id, minted by the
originator and echoed unchanged by every hop"
. That sentence is false of this implementation.

The consequence is precisely the case the brief separated out. In the diamond A -> {B,C} -> D, B and C each
build their request with forwarded_request, which drops the id; D receives two asks with no ask_id,
and HopBudget::from_params applies .unwrap_or_else(mint_ask_id)two fresh random ids — so both
claim successfully and D re-walks. The PR's own rule that an unreadable id is a NEW question converts the
omission from a refusal into a silent full re-walk. The seen-set can only ever fire on the same id
arriving twice at the same node from the immediate requestor, which is a retry, not a diamond.

the_same_ask_arriving_twice_is_forwarded_once (forwarded_ask_tests.rs:1171) hand-injects the same
ask_id twice into the local locate_holders entry point. Its docstring says "the second call
carries the SAME ask_id, which is what a diamond in the graph produces"
— a diamond in this
implementation produces the opposite, two minted ids. The test pins the seen-set's map semantics (already
covered by an_ask_id_is_claimable_once_and_a_different_id_is_unaffected) and nothing about propagation.
#273's stated evidence bar — "the same request arriving twice by different paths is answered once" — is
not met.

Probe: adv_d_the_forwarded_request_omits_the_ask_id_so_no_hop_can_echo_it. Fired, printing the actual
params: {"budget_ms":5000,"items":[...],"redirect_depth":1}.

Second-order: when the id IS present and claimed, a dedup hit returns conclusive: true (A3), so
wiring the field without also fixing the merge rule would convert the diamond's second arrival into an
authoritative absence — a new instance of the same defect. Fix the two together.


Overall verdict

REFUTED — do not merge.

The types are right and the shape is right: AskOutcome's four variants, LocatedHolders.conclusive
starting true and only ever being cleared, the budget on its own decrementing field, not_asked() forcing
every non-establishing path to be explicit. The lane also found two of its own false greens, which is
better than most gate rounds produce. But the distinction is carried correctly only from
ForwardedAsk::ask up to ForwardedAnswers, and it is dropped at all three boundaries that decide
whether a caller ever sees it:

  1. the other leg (find_providers's .unwrap_or_default()), which is the only leg on a default node;
  2. the wire (absence_established is written and never read; -32017 never appears on the verb the
    recursion uses);
  3. every local refusal, including two the ticket names by name.

And the diamond dedup — the third of #273's three evidence requirements — is not wired at all, while the
SPEC states that it is. A SPEC clause asserting a propagation the code does not perform is worse than the
missing propagation, because the next implementer builds against it.

Smallest set that would flip this to SURVIVED:

  1. find_providers returns its Result; locate_holders clears conclusive on the Err arm.
  2. forwarded_request emits ask_id; parse_forwarded_answer reads absence_established from the
    child's result frame and maps false to a non-Answered outcome.
  3. HopBudgetSpent / RelayBudgetSpent / NoEligiblePeers / UnreadableHopBudget / dedup-hit /
    slots-full clear conclusive; only Disabled stays conclusive, which is the only case the PR's
    argument actually covers.
  4. A test at the getAvailability boundary that reaches absence_established == false — the state no test
    currently reaches.

(Refutation leg only. A correctness reviewer and a security auditor ran in parallel on this head; I did
not read their findings before forming these.)

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES-REQUIRED — correctness + test-vacuity gate

Head reviewed: 93c016c78731dfec7147b7695973445890b6fc23 (resolved from gh pr view --json headRefOid; matches the dispatch). PR stays DRAFT.

Two GATING findings, both on requirement 3 of #273, and both the same shape: a distinction that exists in the type system but not on the path that actually runs. Two non-gating.

# Severity Finding file:line
1 GATING ask_id is parsed but never emitted — no producer anywhere, so the diamond dedup is inert and SPEC.md:3137 states behaviour the code lacks seams/dig_peer/forwarded_ask.rs:191-202
2 GATING A DHT walk that ERRORED is reported as an established absence — #273's own defect surviving on the leg that runs by default. Proven by execution. download.rs:1710-1719, :1507-1510
3 non-gating Three doc blocks detached from their items (§2.5) — handed to @copilot download.rs:2489, :1667, :2590
4 non-gating mint_ask_id entropy fallback is an excludes-everyone path with no diagnostic download.rs:2381-2385

What I verified rather than accepted

Claim 5 — the collision guard — CONFIRMED by running both reverts. The side-effect assertions genuinely run first (lib.rs:4717-4728: table size >= 11, CONTENT_MISS_INCONCLUSIVE present, ErrorCode::ALL >= 20), and each leg fires for its own reason:

  • CONTENT_MISS_INCONCLUSIVE = -32009 → panics at lib.rs:4731, the canonical leg: "local CONTENT_MISS_INCONCLUSIVE = -32009 is already canonically RANGE_METADATA_UNREPRESENTABLE..."
  • = -32015 → panics at lib.rs:4741, the local leg: "local METADATA_TOO_LARGE = -32015 collides with local CONTENT_MISS_INCONCLUSIVE"

Two independent legs, two different assertions, neither masking the other. This is the strongest thing in the diff and the claim about it is exact.

Claim 3 — dedup_by_peer deleted — CONFIRMED. Zero remaining callers. dedup_by_peer_tagged (download.rs:2602) keeps the first occurrence, and because merge_answers orders first-hand ahead of hearsay the retained record is always the stronger claim. The policy is subsumed with no case dropped; only the stale doc survived (finding 3c).

Claim 4 — MissOutcome::Inconclusive — the matches are exhaustive, but see finding 2: on a default-configured node the variant is unreachable in production, because not_asked() is deliberately conclusive and the DHT leg cannot clear the flag.

Claim 1 — ForwardedAsk::ask migration — CONFIRMED: three implementors, all migrated, compiler-enforced. Claim 2 — HopBudget tuple → named struct: confirmed, every constructor kept its signature; no call site changed.

#275 is in good shape and I found no vacuity in it. holder_cache.rs is the strongest-reading file in the diff: first-hand-only by construction, so SPEC §10.4.4 survives unamended and the epic's privacy concern is answered structurally rather than by policy; TTL pinned to ADVERTISED_TTL_SECS with a both-sides boundary test (:310); a real capacity-bound test using strictly increasing timestamps so the TTL cannot silently do the bound's work (:335); empty slate never cached (:283); invalidation wired into forget_stale_discovery. a_second_request_inside_the_ttl_skips_rediscovery counts the locator directly rather than using the forwarded-ask proxy that passed against the broken version.

Provenance is preserved through the merge and is load-bearing for dedup_by_peer_tagged's ordering contract; it is dropped at into_candidates, which is correct for the wire (the answer is a candidate list), so I am not raising it.

Method / disclosure

Read from git objects. All probes ran in my own worktree C:\tmp\worktrees\gate-292 at 93c016c7, each reverted from a file copy (never git checkout <path>), with git status --porcelain empty afterwards. No shared checkout was touched. impact/detect_changes were not run — no index in that worktree — so the body's blast-radius claims were re-checked by grep and by the compiler, consistent with the author's own disclosure.

Neither gating finding is handed to Copilot: both are wire-contract / discrimination fixes whose tests must be genuinely falsifiable. Only finding 3 (docs) went to @copilot.

Comment thread crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs Outdated
Comment thread crates/dig-node-core/src/download.rs Outdated
Comment thread crates/dig-node-core/src/download.rs
Comment thread crates/dig-node-core/src/download.rs
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — FINDINGS (2 of 2), head 93c016c78731dfec7147b7695973445890b6fc23

Continuing from the previous comment (SEC-1, SEC-2). Surfaces 1 (held state), 2 (cache), 4 (error
codes), plus the non-gating notes.


SEC-3 (GATING, HIGH) — one ask_id per REQUEST, but the seen-set is consulted per ITEM: a multi-item batch reports a proven absence for items nobody asked about

HopBudget is Copy (download.rs:2405) and availability_batch passes the same value to every
item (lib.rs:3806-3823):

let capped = &items[..items.len().min(MAX_AVAILABILITY_ITEMS)];   // 512
...
for item in capped {
    answers.push(self.availability_answer(item, &cached, requestor, budget).await);
}

So all 512 items share one ask_id. forwarded_holders claims it per item (download.rs:1611):

if !self.ask_seen.claim(budget.ask_id()) {
    return ForwardedAnswers::not_asked();
}

Item 1 claims it. Items 2..512 all fail the claim, return not_asked() — which is
conclusive: true (download.rs:863-867) — and therefore emit absence_established: true at
lib.rs:3766 having asked zero peers.

This needs no attacker. It is the normal path: the batch verb's own doc says "a batch of
root/resource items (what a downloading peer actually sends)"
. A peer batching 20 not-held items
gets 1 real answer and 19 fabricated authoritative absences, each of which — per SEC-1's contract
and MissOutcome::NotFound's own doc — instructs the reader to stop looking.

That is the "surface lies about whether content exists" class, on a peer-reachable verb, in ordinary
operation. It is also the only place the seen-set actually fires in production (see SEC-4), and it
fires exactly where it must not.

What it needs: key the seen-set by (ask identity, content) rather than by request — or move the
claim so a batch's items are independent. Either way not_asked() must not be reachable from a dedup
refusal while conclusive stays true.


SEC-4 (GATING, MEDIUM) — ask_id has no producer: the dedup guard is vacuous against every real peer, and its test cannot see that

forwarded_request — the only builder of an outbound forwarded ask
(seams/dig_peer/forwarded_ask.rs:190-202):

"params": {
    "items": [crate::download::content_id_json(content)],
    "redirect_depth": next_depth,
    "budget_ms": u64::try_from(budget.as_millis()).unwrap_or(u64::MAX),
},

No ask_id. Grep at this head confirms the only ask_id sites are the parser
(download.rs:2437-2441), the minters, the claim, and test fixtures. Nothing in this repo ever puts
an ask_id on the wire.

HopBudget::from_params therefore takes the absent branch on every real inbound forwarded ask and
mints a fresh random id, so AskSeenSet::claim returns true every time. The diamond dedup documented
at holder_cache.rs:186-192 does not exist in production.

The doc at download.rs:2432-2436 states the opposite as fact —

"Minting one here also means every outbound hop from this node carries an id even when the inbound
leg did not, so the dedup takes effect from this node downwards."

— and that sentence is false at this head.

Why the test does not catch it. the_same_ask_arriving_twice_is_forwarded_once
(forwarded_ask_tests.rs:1171-1200) hand-builds the params:

let diamond = HopBudget::from_params(&serde_json::json!({
    "redirect_depth": 0,
    "ask_id": "0102030405060708090a0b0c0d0e0f10",
}));

It exercises the ingress half against a value no dig-node will ever send. Its sibling
requests_without_an_identity_do_not_collide_with_each_other pins the production behaviour — no
identity, never deduplicated — as correct, which is why the missing egress reads as intended. A test
asserting forwarded_request(..)["params"]["ask_id"] is present, and that
from_params(forwarded_request(..)) round-trips it, is what would fire.

Depth is still capped (hop_cap = 2), so this is duplicated work rather than unbounded recursion —
hence MEDIUM, not HIGH. But it is a bound that is advertised, tested, written into SPEC 10.4.4, and
does not run.


SEC-5 (GATING, MEDIUM) — the 3600 s first-hand TTL imports a SIGNED artifact's trust onto an UNSIGNED one, and its invalidation is wired only on the fetch path

holder_cache.rs:41-42:

pub(crate) const HOLDER_CACHE_TTL: Duration =
    Duration::from_secs(super::holdings::ADVERTISED_TTL_SECS);   // 3600

justified as "that is already how long this ecosystem treats a holder's own signed holdings announce
as live, so a cached record expires exactly when the claim behind it would have."

The claim behind a cached record is not the signed announce. It is one DHT node's unauthenticated
lookup answer, and this PR's own comment says so (download.rs:1675-1677):

"A lookup early-exits on the first on-key answer, so a lying first hop can always return a
fabricated provider set."

Three facts compose:

  1. First-hand is uncapped and goes first. dig_sex::discovery::merge_answers
    (dig-sex-0.4.0/src/discovery.rs:187-203) caps only hearsay by max_hearsay_answers; the
    first_hand slice is copied whole and prepended.
  2. The answer is truncated at 8. MAX_REDIRECT_PROVIDERS = dig_dht::MAX_ADDRESSES_PER_RECORD = 8
    (download.rs:120, dig-dht-0.11.1/src/record.rs:165), applied in providers_json.
  3. forget_stale_discovery has ONE production callerdownload.rs:1785, inside
    fetch_resource. The availability/redirect responder never dials the candidates, so it never
    learns they are unreachable and never invalidates.

Exploit. Attacker runs one DHT node positioned to answer on-key for content C.

  1. Any stranger (or the attacker) asks victim node V about C. V walks the DHT; the attacker wins the
    early-exit and returns fabricated providers.
  2. V caches that slate as first-hand, for 3600 s (download.rs:1472-1478).
  3. For the next hour every requestor asking V about C gets 8 fabricated candidates. V's recursive ask
    still runs and still finds the genuine holders — and every one of them is truncated away,
    because first-hand is prepended and uncapped and the answer is cut at 8.
  4. Nothing invalidates: V never dials on this path, so forget_stale_discovery never fires.

Cost: one DHT answer, renewed hourly. Effect: content C is unreachable through V for an hour at a
time, and the recursive discovery this PR exists to add is systematically defeated for that content.

The reasoning that appending hearsay makes the cap "NON-DISPLACING" and that a peer "can never
evict a holder this node found itself"
(download.rs:1443-1447) is correct in the direction it
examines and inverted in the other: a fabricated first-hand slate displaces every genuine
forwarded holder, deterministically.

Pre-PR the same poisoning was bounded by the DHT discovery cache (the code calls it a 15-minute life,
download.rs:1679). This PR adds a second cache in front of it, 4x longer, whose invalidation reaches
only one of the two call paths.

What it needs (any one): cap the first-hand contribution so genuine hearsay always gets slots; or
key the TTL to the unsigned nature of the claim (minutes, not the signed-announce hour); or wire
invalidation on the availability path.


NON-GATING notes (real, but no exploit that clears the bar — follow-up tickets, do NOT gate)

N1 — the cache bounds KEYS, not memory. HOLDER_CACHE_CAPACITY = 4_096 keys, each an unbounded
Vec<ProviderRecord>. dig-dht caps a lookup response at 64 (dig-dht/src/lookup.rs:45,173), so the
ceiling is roughly 4096 x low-hundreds x ~200 B — order 100 MB, reached by a stranger at the
per-requestor miss-lookup rate (burst 16, refill 4/s, rate_limit.rs:165,169) and multiplied by
Sybils, since RequestorId::Peer keys per peer identity and any stranger can mint one. Sustained
filling also evicts oldest-by-insertion, which are the node's own genuine entries. Bound bytes or
records, not keys.

N2 — Provenance is dropped at the wire. into_candidates()/candidates()
(download.rs:821-830) discard the tag and provider_json (download.rs:2622) emits
{peer_id, addresses} only. A requestor cannot distinguish a holder this node found from one a
stranger named. SPEC 10.4.4's "MUST NOT be re-served as this node's own authoritative claim" is
satisfied only by ordering, not by anything the requestor can observe. The merkle bind limits the harm
to a wasted dial (NC-12 holds), so this is conformance rather than exploitation — but the tag exists,
and carrying it one field further would make the MUST checkable.

N3 — claim() runs BEFORE the slot acquisition. download.rs:1611 then :1616. When the
node-wide ceiling is full the ask id has already been burned, so a later arrival of the same question
is refused for the full 65 s TTL despite nothing ever being walked. Dormant today because of SEC-4;
live the moment SEC-4 is fixed. Acquire the slot first.

N4 — attacker-named host/port candidates (PRE-EXISTING, not this diff).
parse_candidate_addr (forwarded_ask.rs:298-302) accepts any host string, including loopback,
RFC1918 and 169.254.169.254, and those become dial candidates for this node (proxy leg) and for
every requestor. mTLS with a peer_id-bound cert stops it becoming a data-exfil SSRF, leaving a
connect/timing oracle. Unchanged by this PR; worth a filter.


Surfaces that CHECK OUT — recorded so they are not re-audited

  • Wire error code (surface 4): CLEAN. -32009 names nothing but RANGE_METADATA_UNREPRESENTABLE
    at this head (checked across download.rs, lib.rs, peer.rs, SPEC.md); the live holder-fatal
    aliasing is genuinely gone from every emit path. And -32017 is not merely free —
    dig-rpc-protocol main already assigns ContentMissInconclusive = -32017
    (src/error.rs:215), alongside MetadataTooLarge = -32015, PushPendingLimited = -32016 and the
    chat band NoIdentity/NoPeerNetwork/SendFailed = -32050/-32051/-32052. I read the
    taxonomy rather than inheriting the lane's claim. The three "undeclared band" deferrals are
    cleanup, not carried security defects.
  • The node-wide concurrency ceiling is a REAL bound. Semaphore::new(32) on one shared
    NodeContent (download.rs:1166), taken with try_acquire_owned — non-blocking, so exhaustion
    REFUSES rather than queueing, and the permit is held across the whole fan-out loop. Charged
    globally, so Sybils buy nothing; per-peer would have been the defect. hold_every_... is
    #[cfg(test)]. The one thing exhaustion must not do is manufacture an absence — and it does, but
    that is SEC-2, not a defect in the bound.
  • Inbound frames are bounded at 64 KiB (peer.rs:730) and DHT responses at 64
    (dig-dht/src/lookup.rs:45), so parse_forwarded_providers — which is itself uncapped — cannot be
    driven into a large allocation.
  • No provenance FLATTENING in the cache. remember is called with found from the DHT leg only,
    before the merge (download.rs:1472-1478); hearsay never enters FirstHandHolderCache.
    dedup_by_peer_tagged keeps the first occurrence, so a hop echoing a known holder cannot downgrade
    it to hearsay. This half of Provider cache: remember which peers hold what, with a TTL (requirement 7 -- missing from the epic until now) #275 is correct.
  • TTL-expired entries are removed on read (holder_cache.rs:88-96), so no expired-but-present
    entry sends dials to a guaranteed miss. An empty slate is never cached. Capacity eviction is
    age-based and not attacker-steerable.
  • Budget clamp is applied once, at construction (download.rs:2429-2431), so a hop naming a
    ten-minute budget buys the 65 s ceiling and no more. The wire field is separate from
    redirect_depth, so time cannot buy hops.
  • 5.2 IPv6-first is preserved on the new dial path: PeerTarget::with_addrs with the full
    candidate list (forwarded_ask.rs:355-358).
  • NC-1/5.4 untouched: this leg carries provider candidates, not directed payloads.
  • Deps: dig-download 0.17->0.18, dig-keystore 0.3.1->0.9.0, dig-constants 0.8->0.10.1 are
    first-party DIG crates, and the lock moves rather than duplicating. No third-party addition, no
    loosened pin.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security VERDICT: CHANGES-REQUIRED

Audited head: 93c016c78731dfec7147b7695973445890b6fc23 (resolved from gh pr view 292 --json headRefOid at the start AND at the end of the audit; it did not move). Branch
loop/273-275-ask-outcome, base e4afb52. Read-only audit — no shared checkout was mutated, no
worktree was cut, no file was edited.

Ranked findings (detail + exploits in the two comments above)

# Sev Gate Where One line
SEC-1 HIGH GATE forwarded_ask.rs:262-271, lib.rs:3766 absence_established is write-only — one emitter, zero readers; a downstream subtree's inconclusiveness is laundered into a proven absence at every hop above it
SEC-2 HIGH GATE download.rs:1507-1510, :1703-1712, :863-867 conclusive ignores the DHT leg entirely; with recursion off by default absence_established is a constant true, so a node with a failed or empty DHT walk asserts a proven absence for all content
SEC-3 HIGH GATE lib.rs:3806-3823, download.rs:1611 one ask_id per REQUEST but the seen-set is claimed per ITEM, so items 2..512 of any batch report absence_established: true having asked nobody — no attacker required
SEC-4 MED GATE forwarded_ask.rs:190-202 ask_id has no producer: nothing ever puts it on the wire, so the diamond dedup is vacuous against every real peer, and its test hand-builds the field so it cannot see that
SEC-5 MED GATE holder_cache.rs:41-42, download.rs:1443-1447, :1785 a 3600 s TTL justified by a SIGNED announce is applied to an UNSIGNED DHT relay claim; uncapped first-hand + an 8-slot answer cap means one fabricated slate displaces every genuine holder for an hour, with invalidation wired only on the fetch path
N1-N4 LOW/MED note see comment 2 cache bounds keys not bytes; Provenance dropped at the wire; claim() before slot acquisition; pre-existing attacker-named dial candidates

Why these gate

SEC-1, SEC-2 and SEC-3 are the same defect wearing three faces, and it is the inverse of the one
this PR exists to fix. #273's thesis is that collapsing "nobody has it" into "nobody answered" hands
an attacker a censorship primitive. At this head the distinction is created locally and destroyed
at every boundary it has to cross
: it is not read off a hop's answer (SEC-1), it never models the
leg that actually runs (SEC-2), and it is falsified outright for every item after the first in a
batch (SEC-3).

The net effect is that the PR does not merely fail to close the hole — it widens it. Before, a
node emitted a bare not-found that a client was free to read tolerantly. After, it emits an explicit
absence_established: true that SPEC 10.4.4 instructs the client to act on by stopping. Turning
"I have nothing to say" into "I have established there is nothing" is strictly worse than the
ambiguity being replaced, and on a stock node it is the default and only outcome.

SEC-3 in particular needs no adversary and is reachable today by an anonymous peer over the ordinary
dig.getAvailability verb (peer certs are self-signed, peer_id = SHA-256(SPKI), so anyone can mint
an identity and any peer can batch).

What is genuinely good, and should not be re-litigated

The wire error-code surface is clean and was checked independently rather than inherited: -32009
now names only RANGE_METADATA_UNREPRESENTABLE, and -32017 matches the canonical
dig_rpc_protocol::ErrorCode::ContentMissInconclusive already present on that crate's main
(src/error.rs:215) — so the previously-reported -32050/-32051/-32052 and ~145 raw literals
deferrals are cleanup, not carried security defects. The 32-slot concurrency ceiling is a real,
globally-charged, non-blocking bound that Sybils cannot buy around. The budget clamp is applied once,
at construction, on its own wire field. The cache does not flatten provenance, never stores hearsay,
never stores an empty slate, removes expired entries on read, and evicts by an age rule an attacker
cannot steer. NC-12's "every candidate is a dial, never a fact" holds throughout, and the merkle bind
still admits bytes.

Minimum to clear this gate

  1. parse_forwarded_answer reads the peer's absence_established and AskOutcome::Answered carries
    it, so inconclusiveness cascades (SEC-1).
  2. find_providers distinguishes Ok-empty from Err, and LocatedHolders::conclusive is the AND of
    both legs; a node that cannot reach the DHT emits false or omits the field (SEC-2).
  3. The seen-set is keyed per (ask, content) or the claim moves, so a dedup refusal can never coexist
    with conclusive: true (SEC-3).
  4. forwarded_request emits ask_id, with a round-trip test through the real request builder
    (SEC-4).
  5. The first-hand cache's TTL or contribution is bounded so a single fabricated slate cannot displace
    genuine holders for an hour (SEC-5).

Re-gate scope on the fix: this leg (loop-security) in full — every finding above is on the same
outcome/conclusiveness path, so a scoped fix touches all of it. Per the dispatch, none of these
was handed to Copilot
: this is peer-facing attack surface.

PR stays DRAFT. Do not merge.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Progress — resume-ready

Branch loop/273-275-ask-outcome, worktree /c/tmp/worktrees/dn-273-275.

Step 0 — rebase: NO-OP, zero conflicts. origin/main (e4afb52 v0.135.0, which contains PR#291 8d82aca7) is already an ancestor of this branch. mergeStateStatus: BLOCKED is the draft state plus ungated checks, not a conflict.

Recovered work first. The worktree held ~560 lines of uncommitted prior-lane work (recursion-disabled vs refused, failed-DHT-walk inconclusive). Committed as 3e93467 before anything else so a second cap could not lose it. It is mid-refactor and does not yet compile — the ForwardedAsk::ask signature is 7-param at call sites and 6-param in the trait, forwarded_request is referenced and unwritten, ForwardedAnswers::not_asked is gone but still called at download.rs:1595.

Step 1 — three of four bumps DONE (dig-rpc-protocol 0.6→0.10, dig-download 0.18→0.19 both occurrences incl. the features = ["testkit"] one, dig-peer 0.9→0.11). Each resolves to exactly ONE Cargo.lock entry.

Step 3 — digstore-chain rev bump MEASURED AND REVERTED. It is not clean, and the reason is on the money path.

Bumping all 13 digstore.git deps to 222f08d compiles, resolves digstore-chain 0.26.0, and then fails with 19 type errors that are all one thing: rev 222f08d carries the chia-wallet-sdk 0.34 uplift, so digstore-chain now speaks chia-protocol 0.36.1 while dig-node speaks chia-protocol 0.26. Both lines were already in the lock before the bump; what the bump does is move digstore-chain to the far side of that boundary.

Nine of the errors are dig-node's own ChainReads impl (store_melted.rs:1318-1381), and two of those are push(SpendBundle) and estimate_fee(SpendBundle) — converting a SpendBundle between two chia-protocol versions is precisely the byte-drift class dig_ecosystem#2610 exists to remove. confirm_melt_via_chain on the other side of it authorizes a store DELETE. A boundary shim there is worse than not bumping.

Making it one chia line requires moving chia-peer 0.1.3, chia-query 0.5.1 + 0.6.2, chia-wallet-sdk 0.30, dig-merkle 0.4.5, dig-store, dig-store-cache, and replacing the chia = "0.26" umbrella in two manifests (dig-node-service:229, dig-wallet:35) with the five direct facades — the umbrella has no 0.36 line. That is the #2610 dig-wallet child plus dig_ecosystem#2228, not a rev bump.

Premise correction: dig-node does NOT carry two digstore revs. All 13 digstore.git deps sit at one rev, 4c34f0be. 51054a41 is a dig-gossip rev (dig-node-core/Cargo.toml:224,387), a different repository. Nothing to reconcile.

NEXT ACTION: step 2 — finish the prior lane's refactor onto the canonical dig-rpc-protocol 0.10 items across the 32 located sites, delete the local CONTENT_MISS_INCONCLUSIVE const at download.rs:93, and move the wire number off the provisional -32017 onto ErrorCode::ContentMissInconclusive.

@MichaelTaylor3d
MichaelTaylor3d force-pushed the loop/273-275-ask-outcome branch from 4f319e0 to 5e32b5d Compare August 22, 2026 00:15
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Round complete — handing back to the gate. Head 4d2035e, still DRAFT.

All five required checks SUCCESS, asserted BY NAME via check-merge-preconditions.sh (which reads the required set from branch protection, not from the rollup): Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage. Nine further non-required checks also pass, including all four platform builds and CodeQL. RESULT: BLOCKED is draft=true plus the three review threads below — nothing red and nothing absent.

Local full-workspace cargo test --workspace: 2185 passed, 0 failed, exit 0. cargo clippy --workspace --all-targets -- -D warnings clean.

The three GATING threads are answered on the threads themselves, with evidence

  1. ask_id parsed but never emitted — fixed. It is now a parameter of ForwardedAsk::ask and passed as budget.ask_id(), and forwarded_request emits it through GetAvailabilityParams::with_ask_id. the_identity_a_hop_received_is_the_identity_it_emits reparses the request body to prove it survives the wire.
  2. A failed DHT walk laundered into an established absence — fixed at the source (walk_for_providers is fallible) and the flag is now the CONJUNCTION of both legs. That second half mattered more than the first: derived from the forwarded leg alone, absence_established was a constant true on a stock node, where that leg is not installed at all.
  3. Three detached doc blocks — (b) fixed at 5e32b5d (it was also a hard Clippy error, not only readability: a blank line between the block and the item trips empty_line_after_doc_comments under -D warnings, so the branch could not have gone green as pushed), (a) fixed at 4d2035e, (c) already clean. One correction to the instruction, stated on the thread: "and for the outbound request body" is KEPT because finding 1's fix made it true.

I have not resolved any thread — that is the gate's call, and I am the author of the fixes.

What a re-gate should look at, since the diff grew

The adoption of dig-rpc-protocol 0.10 was not a recompile. It changed behaviour in three places, each of which is the same defect class #273 exists to close, and each has a revert-proof recorded in the PR body:

  • absence_established was read with unwrap_or(true) — the collapse the taxonomy owner names in its own docs as turning an unknown into an assertion of absence.
  • A failed DHT walk was unwrap_or_default().
  • Every unasked path was conclusive; only recursion being DISABLED is.

The wire NUMBER did not change (-32017 is what the owner assigned), so no client sees a different code — only a node that is now honest about what it did not establish.

One thing deliberately NOT done

The digstore-chain git-rev bump to 222f08d was performed, measured, and reverted. It compiles and locks cleanly, then fails with 19 type errors that are all one thing: it drags chia-protocol 0.36.1 against dig-node's 0.26, and two of the errors are push(SpendBundle) / estimate_fee(SpendBundle) on the trait whose other half authorizes a store DELETE. Full measurement is in the PR body and on https://github.com/DIG-Network/dig_ecosystem/issues/2610 — it is that epic's own known blocker (#2228) reached from the dig-node side, not a new one.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — interim findings (audit in progress)

Auditing head 4d2035e6e6dbca05af9a0ad8b7fc98956b4811cd, base e4afb522ce6f3ba91b526af2a008a3f630bc7b33 (12 commits). Posting as I form them so nothing is lost if this context dies. Verdict follows in a separate comment.

Clear so far

  1. No unwrap_or(true)-shaped collapse survives on the availability path. There is exactly ONE production reader of the field — subtree_claim at crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs:357 — and it matches all three states explicitly (Some(true) / Some(false) / None). Exactly ONE writer, crates/dig-node-core/src/lib.rs:3766. The other unwrap_or(true) hits in the tree (content_serve.rs:1171, dispatch.rs:904, control_cli.rs:496, sage/rpc.rs, sync_supervisor.rs) are pre-existing files this PR does not touch, and none of them read absence_established. Value::as_bool also returns None for a non-bool ("true", 1), which falls to NoClaim — the safe direction.

  2. The weakest-item rule holds across a batch and is order-independent. The fold at forwarded_ask.rs:365-371 is min over the total order NotEstablished < NoClaim < Established, which is commutative and associative, so no permutation of items changes the result. Established mixed with NoClaim yields NoClaim; mixed with NotEstablished yields NotEstablished.

  3. The aggregate across PEERS is a conjunction, not a disjunction. forwarded_holders (download.rs:1674-1713) starts conclusive: true and only ever CLEARS it; locate_holders (download.rs:1536) then requires first_hand_conclusive && forwarded.conclusive. So a single peer returning Established can never raise this node's claim — it can only fail to lower it. LocatedHolders also derives Default, so the derived value is conclusive: false.

  4. Lock singletons verified against the resolved Cargo.lock at head, not the manifests: dig-rpc-protocol 0.10.1, dig-download 0.19.0, dig-peer 0.11.0, digstore-chain 0.19.2 — exactly one entry each. digstore-chain was correctly NOT bumped and nothing partial was left behind. chia-protocol resolves 0.26.0 + 0.36.1 at head, which is identical to the base — this PR introduces no new duplicate chia surface.

Findings raised so far

A. dig-keystore 0.3.1 -> 0.9.0 is an out-of-scope, unforced bump of a SEED-CUSTODY crate. crates/dig-wallet/Cargo.toml:126. Six 0.x minors, i.e. six breaking lines under SemVer's 0.x rule, in a PR whose scope is content discovery. Measured facts:

  • It was not forced. dig-wallet is the ONLY consumer of dig-keystore in the resolved tree (checked by walking every package block in Cargo.lock at head). Nothing in dig-download 0.19, dig-peer 0.11 or dig-rpc-protocol 0.10 depends on it, and it was a singleton at 0.3.1 before and is a singleton at 0.9.0 now — so no duplicate-instance pressure required this.
  • No code changed with it. The entire crates/dig-wallet/ delta in this PR is the one manifest line.
  • The crate holds the seed at rest. Its own manifest comment describes the AES-256-GCM versioned/magic-tagged/CRC-guarded on-disk layout that replaced digstore_chain::seed for new writes, with seed_store::decrypt_seed still reading old-format files.

A green build proves the API surface dig-wallet calls still exists; it does not prove the ON-DISK format is unchanged. Still measuring that. Flagging now because this is the class where a false PASS is most expensive.

B. subtree_claim reads an EMPTY items array as Establishedforwarded_ask.rs:346-372. The let Some(items) = items else { return NoClaim } guard correctly handles items ABSENT, but items: [] then folds over zero elements and returns the identity, SubtreeClaim::Established. So {"result":{"items":[]}} — a peer that answered about nothing at all — is classified AskOutcome::Answered(vec![]), i.e. conclusive. That is the same shape as the defect being fixed (a claim that was never made, read as an establishment), and it makes items absent and items empty disagree. Assessing exploitability now; a peer that wants a false establishment can already just send absence_established: true, so this may be defense-in-depth rather than a new primitive.

C. A peer named by decide_forward but not found in the pool is skipped WITHOUT clearing conclusivedownload.rs:1676-1678, the bare continue. Every other unasked path in that loop clears the flag. peer_keys is built from the same pool so this should be unreachable today, but it is an unasked leg that claims the absence, which is exactly the invariant the PR establishes. Confirming whether decide_forward can return Forward { peers: [] }.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security — GATING finding: the DHT-leg half of #273 is dead code in production

Head audited: 4d2035e6e6dbca05af9a0ad8b7fc98956b4811cd.

The claim the PR makes

crates/dig-node-core/src/download.rs:1496-1499 is the fix for the DHT leg:

Err(error) => {
    tracing::debug!(%error, "locate: the DHT walk failed; absence unproven");
    (Vec::new(), false)
}

walk_for_providers's own doc (download.rs:1768-1771) states why it matters:

A locate FAILURE stays a failure. [...] collapsing them is how a node with no reachable DHT peer came to assert a proven absence for every piece of content in existence.

and the new SPEC clause (§10.4.4) states it as a MUST:

The DHT leg is subject to the same rule: a provider walk that FAILED (no reachable DHT peer, a transport error) is not a walk that found nobody, and the absence is unproven.

The measurement: that Err arm cannot be reached in production

The locator the engine is built with is a CHAIN (download.rs:1293-1310):

DhtProviderLocator -> UnionLocator -> SelfExcludingLocator -> CapsuleFallbackLocator -> NodeContent

Two layers of that chain swallow the error, and both return Ok:

  1. crates/dig-node-core/src/seams/dig_peer/union_locator.rs:71-74
for result in results {
    let Ok(records) = result else {
        continue; // best-effort: skip a failed source
    };

with the function's ONLY return being Ok(merged) at union_locator.rs:105. UnionLocator::find_providers is therefore infallible — it cannot propagate an Err at any granularity.

  1. crates/dig-node-core/src/seams/dig_peer/capsule_fallback.rs:62-68 (the Resource branch, which is the dominant one)
let by_resource = self.inner.find_providers(content).await.unwrap_or_default();
let by_capsule  = self.inner.find_providers(&capsule).await.unwrap_or_default();
...
Ok(merged)

So a DHT walk that ERRORED arrives at walk_for_providers as Ok(vec![]). locate_holders takes the Ok arm, sets first_hand_conclusive = true (download.rs:1487-1491), and since the forwarded leg ships DISABLED (recursion_disabled() -> conclusive: true), the conjunction at download.rs:1536 yields conclusive == true.

Result: establishes_absence() returns true for a search that never happened.

What that produces on the wire

  • miss_outcome (download.rs:2301-2310) returns MissOutcome::NotFound instead of Inconclusive, and MissOutcome::NotFound's own doc says "This asserts an ABSENCE, and is therefore only reachable when the search actually established one."
  • availability_answer (lib.rs:3766) emits absence_established: true.

Both legs use ContentId::resource (availability_content_id at download.rs:2765, range_content_id at download.rs:2787), so the Resource branch is the normal path, not an edge case.

Concrete scenario

  • State: a dig-node whose DHT walk fails — start-up before any DHT peer answers, a partitioned segment, an eclipsed routing table, or an off-path attacker degrading its DHT RPCs. No forged message and no protocol position is required.
  • Action: a peer asks dig.getAvailability (or hits the range-stream miss) for content that EXISTS and is held elsewhere.
  • Impact: this node answers absence_established: true / plain not-found. Per the taxonomy owner's own contract, Some(true) means "a client MAY stop searching." The reader stops looking for content that exists, and because a hop relays the answer, the false absence propagates downward — the censorship primitive the epic exists to remove, arriving through a failed lookup rather than an attack.

Note this is strictly worse on a stock node than on an opted-in one: with DIG_NODE_FORWARD_ON_MISS off the DHT leg is the ENTIRE search, so absence_established is decided by this broken conjunct alone.

Why the test did not catch it

a_failed_dht_walk_is_not_an_established_absence (forwarded_ask_tests.rs:1512) is a good test pointed at the wrong layer. Its engine_over helper (forwarded_ask_tests.rs:1444-1456) passes the locator straight to NodeContent::new:

let content = NodeContent::new(
    Arc::new(locator),
    ...

so FailingLocator's Err reaches walk_for_providers unmediated. Production interposes the two swallowing layers above, which the fixture never constructs. The double is more capable than production, so the test witnesses a fix on a path production cannot take. The test's own doc anticipates the shape of this — "A double that cannot express the failure cannot witness the fix" — the miss is that expressing it at the wrong layer has the same effect.

This is also the round-1 thread GATING 2, still open on this PR: the fix was applied at locate_holders while the laundering that GATING 2 named lives one and two layers below it.

What would discharge this

Either is fine; the second is the smaller change:

  1. Make the chain error-preserving — have UnionLocator distinguish "every source failed" from "sources returned nothing" and propagate that, and drop capsule_fallback.rs's two .unwrap_or_default() calls in favour of a both-failed -> Err rule.
  2. Keep the chain as-is but derive first_hand_conclusive from something that CAN observe the failure, rather than from the Result the chain has already discarded.

Whichever is chosen, the regression test must construct the production locator chain (or the fix must be asserted at a layer the chain cannot bypass). A revert-proof that installs a bare FailingLocator into NodeContent::new will stay green against the current defect and cannot witness the fix either.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

loop-security VERDICT: CHANGES-REQUIRED

Head audited: 4d2035e6e6dbca05af9a0ad8b7fc98956b4811cd (base e4afb522ce6f3ba91b526af2a008a3f630bc7b33, 12 commits). Read-only; no shared checkout was mutated.

One GATING finding, posted in full above. Three non-gating. The core of the PR — the three-state SubtreeClaim and the weakest-item rule — is correct, and I could not break it.

GATING

S1 (HIGH) — the DHT-leg half of #273 is unreachable in production. UnionLocator::find_providers (union_locator.rs:71-74, whose only return is Ok(merged) at :105) is infallible, and CapsuleFallbackLocator (capsule_fallback.rs:62-68) swallows both inner errors. So download.rs:1496's Err arm never runs, first_hand_conclusive is always true, and a node whose DHT walk FAILED emits absence_established: true and MissOutcome::NotFound for content that exists. On a stock node (recursion ships OFF) this conjunct is the whole search. The regression test passes only because engine_over (forwarded_ask_tests.rs:1444) injects the locator straight into NodeContent::new, bypassing both layers production installs. Detail and remedies in the previous comment.

NON-GATING

S2 (MEDIUM, defense-in-depth) — subtree_claim reads an empty items array as Established. forwarded_ask.rs:346-372: the let Some(items) = items else guard covers items ABSENT, but items: [] folds over zero elements and returns the identity, Established. That contradicts the owner's stated MUST (a client that wants to stop searching MUST require Some(true)) — with zero items there is no Some(true) anywhere. Not gating: a malicious responder gains nothing it does not already have by sending absence_established: true, and the JSON-RPC leg a forwarded ask actually hits returns an error frame on a bad items param (dispatch.rs:317-323), so a conforming dig-node never produces the shape. The MUX leg does have a producer — peer.rs:1380 — so this becomes live if that leg is ever wired to the forwarded path. One-line fix while the file is open.

S3 (MEDIUM) — SPEC 10.4.7 states a TTL the code deliberately repealed, on a security bound. SPEC says the TTL is ADVERTISED_TTL_SECS (= 3600s), "deliberately the same lifetime this ecosystem already grants a holder's own signed holdings announce". The code is HOLDER_CACHE_TTL = Duration::from_secs(300) (holder_cache.rs:71), whose own doc says that exact rationale "borrowed the wrong claim." The quantity is the displacement window during which an attacker-seeded but first-hand slate evicts genuine holders, so a second implementation built from this normative SPEC would hold it 12x longer. remember's doc (holder_cache.rs:206) and the SPEC's "an hour of manufactured absence" carry the same stale number. Code is the safe side; the SPEC is wrong.

S4 (LOW, note) — dig-keystore 0.3.1 -> 0.9.0 is out-of-scope and unforced. crates/dig-wallet/Cargo.toml:126, six 0.x minors on a seed-custody crate, zero accompanying code change, in a discovery PR. Measured and cleared as a live risk: I diffed both crates from crates.io — kdf.rs byte-identical; cipher.rs/format.rs differ only in doc comments; opaque.rs keeps MAGIC = DIGOP1 and SCHEME_ID = 0x0004; the decode path, is_known_magic and FORMAT_VERSION_V1 are unchanged; password.rs differs only in doc comments so zeroization is intact; error.rs is additive under #[non_exhaustive]. The new keyring dep is optional behind the non-default os-keychain feature and never enters the lock. dig-wallet is the ONLY consumer, so nothing forced it. Not a defect — but it should be named in the PR body rather than arriving silently on the custody path.

Answers to the specific questions asked

  • Does any unwrap_or(true)-shaped collapse survive? No. One production reader (forwarded_ask.rs:357) matching all three states explicitly, one writer (lib.rs:3766). No unwrap_or_default() on a bool, no helper hiding one, no None arm folded in with Some(true). Value::as_bool also yields None for a non-bool, which falls to NoClaim — the safe direction. The remaining unwrap_or(true) hits in the tree are in files this PR does not touch and none read absence_established.
  • Does the weakest-item rule hold across a batch, and is it order-independent? Yes to both. The fold (forwarded_ask.rs:365-371) is a min over the total order NotEstablished < NoClaim < Established — commutative and associative, so no permutation changes the result. Established + NoClaim gives NoClaim; Established + NotEstablished gives NotEstablished. The one blemish is the empty-batch identity (S2). The same rule holds ACROSS peers by a different mechanism: ForwardedAnswers.conclusive starts true and is only ever cleared, pinned by one_inconclusive_child_defeats_a_sibling_that_found_nobody.
  • What can a lying peer achieve under NC-12? Less than expected; the composition is sound. locate_holders computes first_hand_conclusive && forwarded.conclusive (download.rs:1536), a CONJUNCTION, so a peer's Established can only fail to LOWER this node's claim, never raise it — one lying peer cannot manufacture a confident absence. merge_answers appends hearsay AFTER first-hand and caps only the hearsay tail, so fabricated holders cannot evict genuine ones. A lying peer can (a) withhold what it knows and assert Established, inherent to hearsay and requiring it to occupy ALL of the fan_out=3 selected slots AND the DHT leg to independently find nothing, and (b) force INCONCLUSIVE at will by returning garbage — the fail-safe direction, costing a retry. The real way to make this node report a confident absence for content that exists is S1, and it needs no lying peer at all.
  • budget_ms three states and the revert ordering: correct. Absent derives its own budget via ask_budget; Some(0) makes time_budget return ZERO so the loop's remaining.is_zero() breaks before any ask AND sets conclusive = false; unreadable is treated as absent and stays under the ceiling. The .filter(|ms| *ms > 0) revert makes the node ask, so ask.asked().len() == 0 — deliberately the FIRST assertion — fires and names the damage ahead of the conclusiveness assertion. Ordering verified correct, and the arm has a truthful control proving the node does forward when granted time.
  • Can the rewritten LOCAL_WIRE_CODES guard fail? Yes, four independent ways: the table shrinking below 10; CONTENT_MISS_INCONCLUSIVE being re-declared locally; content_miss_inconclusive() diverging from the owner's number; ErrorCode::ALL reading near-empty. Honest caveat: the third leg is circular as written, since content_miss_inconclusive() is defined AS the owner's variant at download.rs:106, so it cannot fail today — it becomes load-bearing only once someone replaces the body with a literal, which is exactly the drift it guards. Legitimate for a drift guard, but not proof of anything at the current head.
  • Lock singletons: verified against the resolved Cargo.lock, not the manifests — dig-rpc-protocol 0.10.1, dig-download 0.19.0, dig-peer 0.11.0, digstore-chain 0.19.2, exactly one entry each. The dependency_tree.rs guard asserts starts_with("0.10.") against the real lock and would fail on a downgrade. chia-protocol resolves 0.26.0 + 0.36.1 at head, identical to base, so this PR adds no new duplicate chia surface and the digstore-chain revert left nothing partial.
  • The answer_with fixture: fixed — it now states absence_established: true explicitly (forwarded_ask.rs:519-529), with a doc explaining why omitting it would test the compatibility case while claiming to test a peer that looked. No other fixture depends on the old meaning; the AskOutcome doubles select variants directly and default to Unreachable (forwarded_ask_tests.rs:1496), the safe direction.
  • SPEC: 10.4.4 correctly distinguishes recursion-disabled (conclusive) from a refused leg (not), and names dig-rpc-protocol as the owner with origin Peer rather than restating the contract. But it does not fully match the code — see S3.
  • Requirement 4 of the recursive-discovery epic is absent: no onion-mode streaming back through the hops #276: confirmed untouched and correctly out of scope.
  • Bisect: not a risk, and structurally cannot be. allow_merge_commit=false, allow_rebase_merge=false, allow_squash_merge=true, required_linear_history=true — all 12 commits collapse into ONE commit on main, so git bisect on main never sees an intermediate. Separately, no pre-dep-bump commit references a 0.10-only symbol in code: the single hit in 589139f/273ccdc is a doc comment (download.rs:98), so the dep/code ordering is coherent. As for whether salvaged code reached production paths without review — it did (AskOutcome, holder_cache.rs, the locate_holders rewrite are all salvage-derived), and this audit is that review: S1 is a salvage-derived defect, and the round-1 GATING 2 thread had already pointed at it.

Merge preconditions (FYI, not my gate)

All five required contexts SUCCESS by name on 4d2035e: Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage. mergeStateStatus=BLOCKED — still DRAFT with 3 unresolved review threads, two of them round-1 GATING threads, and GATING 2 is the one S1 shows is not actually discharged.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Non-gating (fix in this PR, cheap): CRLF line-ending flip on two files inflates the diff ~19x and destroys blame.

SPEC.md and crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs were rewritten with CRLF terminators in d4999e4:

git diff --stat 4d2035e..aa30f34            -> SPEC.md 13082 +-, forwarded_ask.rs 1889 +-
git diff --stat --ignore-cr-at-eol 4d2035e..aa30f34 -> SPEC.md 30 +-,    forwarded_ask.rs 47 +-

file confirms both gained with CRLF line terminators between 4d2035e and aa30f34; every other file in the delta is unchanged LF.

Why it matters beyond tidiness: the real edit is 389 lines, the recorded one is 7,836. git blame on forwarded_ask.rs — a security-relevant seam — now attributes every line to this commit, and any reviewer reading the raw diff cannot see the change. It also guarantees a conflict against any sibling branch touching either file.

@copilot fix this: re-normalize SPEC.md and crates/dig-node-core/src/seams/dig_peer/forwarded_ask.rs to LF-only line endings so that git diff --stat 4d2035e..HEAD shows the same counts as git diff --stat --ignore-cr-at-eol 4d2035e..HEAD (SPEC.md ~30 lines, forwarded_ask.rs ~47 lines). Do NOT revert any of the content changes in those files — only the line terminators change. Also add * text=auto eol=lf to .gitattributes at the repo root (create it if absent) so this cannot recur.

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scoped re-gate: PASS (S1 cleared) - head aa30f340c6606d245b1dbb454ddda09a5b08cb72

Scope: 4d2035e..aa30f34. Round-2 findings not re-run.

1. Can a failed DHT walk still reach absence_established: true by any path?

No. I traced every layer of the production chain and each propagates.

layer site behaviour on inner Err
CapsuleFallbackLocator resource fan-out capsule_fallback.rs:67-83 first failure recorded; Err returned iff merged set is empty
CapsuleFallbackLocator non-resource pass-through capsule_fallback.rs:54 returns the inner result unchanged - propagates (never a swallow)
SelfExcludingLocator self_excluding_locator.rs:73 ? - propagates
UnionLocator union_locator.rs:80-133 first failure recorded; Err returned iff merged set is empty
walk_for_providers download.rs:1789 ? - propagates
locate_holders download.rs:1509-1512 Err maps to (vec![], false), so first_hand_conclusive = false
conjunction download.rs:1549 first_hand_conclusive && forwarded.conclusive

Two adjacent doors I checked separately rather than assuming:

  • The cache short-circuit (download.rs:1498, the cached arm returning true) rests on the claim that an empty slate is never remembered. Verified at the source: holder_cache.rs:207-210 returns early when records.is_empty(). That true is earned.
  • The infallible find_providers (download.rs:1800) still discards the distinction by design. Its doc claims one production caller; I grepped rather than trusted it - download.rs:1835, a debug candidate-count in fetch_resource, plus one test. No absence path reaches it.

2. Does the regression test exercise the same locator construction production uses?

Yes, and I re-did the grep myself. Grepping UnionLocator::new, CapsuleFallbackLocator::new and SelfExcludingLocator::new at aa30f34 returns exactly one non-test discovery-chain site: provider_locator_chain (download.rs:1289), called by for_dht at :1323. The second production hit - download.rs:1131 - is the download/fetch locator (PoolProviderLocator unioned with the already-built discovery locator), a different leg that feeds dialing, not absence. The extraction is faithful: the layers, their order and their rationale comments moved verbatim out of for_dht, nothing added or dropped.

Verification I ran myself, not read

  • Clean suite: cargo test -p dig-node-core --lib gave 906 passed, 0 failed (477s), matching the claim.
  • Revert spot-check, union guard only (deleted the if merged.is_empty() block in union_locator.rs; my own worktree, file-copy revert, never a git checkout):
    • forwarded_ask_tests::a_failed_dht_walk_stays_unproven_through_the_production_locator_chain - FAILED
    • union_locator::tests::a_failed_source_makes_an_empty_union_an_error_but_never_hides_a_holder - FAILED
    • Both exactly as the matrix claims, and the right assertion fired - forwarded_ask_tests.rs:1588, "the union and the capsule fallback swallowed the walk failure into Ok(vec![])" - not the control, not an incidental one.
    • A third test, tests::cache_lock_is_exclusive_then_released, also failed in that run and passed in the clean run. It is a filesystem-lock test and I had two cargo runs live against the same worktree; I read it as my own contention, not a defect in this PR. Recorded so nobody re-derives it.

The rule itself - "empty + any source failed means inconclusive"

I judged the rule, not only its implementation, and it is the right line.

The asymmetry is load-bearing and correctly argued: a non-empty result is not an absence claim, so poisoning it would discard genuine holders and undo #1443/#1580 to sharpen a distinction the caller never consults. Only establishes_absence() reads conclusive.

On the partition question - does a node now refuse to ever conclude absence when one leg fails and another legitimately returns zero? Yes, within that partition, and that is correct. absence_established is a positive claim that tells a reader it may stop searching. A node whose DHT leg could not complete has not earned that claim regardless of what its dormant legs report.

The fail-direction is also right, which is the half worth checking rather than assuming: the caller degrades to MissOutcome::Inconclusive (download.rs:2322), carried on the wire as content_miss_inconclusive, which clients retry on (download.rs:2641). Bounded recovery, not denial. This is a fail-closed arm that does not convert one bad input into permanent denial - nothing durable is written, nothing is cached (holder_cache refuses empties), and the next completed walk concludes normally. The two directions are not symmetric: a false true tells the network to stop looking for content that exists and is relayed onward by every hop; a false inconclusive costs a retry.

Do the controls discriminate? Checked individually, not counted. union_locator.rs:308-317 and capsule_fallback.rs:285-294 each build a chain whose sources all complete and find nobody, asserting Ok(empty). The nearest wrong implementation - "error on any empty union" - fails them. forwarded_ask_tests.rs:1592-1601 does the same through the real chain. These are genuine controls: they are what stops this fix trading a false absence for an absence that can never be established at all.

The rest of the delta

  • S2 (forwarded_ask.rs:354-360) - correct, and its control (the one-item established arm inside an_empty_items_array_establishes_nothing) rules out an implementation that never establishes anything. Not guard-not-exercised: dispatch.rs rejects a malformed items param upstream, but a well-formed empty array reaches this guard.
  • SPEC 10.4.7 TTL - verified against code rather than accepted: holder_cache.rs:71 is HOLDER_CACHE_TTL = Duration::from_secs(300), and the displacement-window rationale matches the module doc at :53-55. The new 10.4 layer obligation describes exactly what the code now does; I found no aspirational sentence.
  • ask_id wire pin (forwarded_ask.rs:562-570) - can fail. It asserts on the output of the production builder forwarded_request(...) against the caller-supplied value, so dropping the field or minting a fresh one per hop goes red.
  • The rename empty_and_erroring_source_is_skipped to a_dormant_source_is_skipped did not paper over a gap. The old fixture held EmptyLocator and no erroring source - its name was a lie, which is exactly why the swallow survived it. The erroring case now has real coverage via a purpose-built FailingSource double. Net coverage up, not merely relabelled.

What I attacked and could not break

  • A capsule/store-granularity id bypassing the fan-out fix through the pass-through arm - propagates.
  • SelfExcludingLocator as an unfixed third swallow sitting between the two patched layers - uses ?.
  • An empty cached slate re-entering as conclusive: true - remember refuses empties.
  • The infallible find_providers reaching an absence decision - one debug-only caller.
  • The extracted chain silently diverging from what for_dht used to build - compared line by line, identical.
  • The test passing for an unrelated reason - reverted the guard and read the panic line.

Sequencing note for the orchestrator (not this lane's to fix)

#292 is 0.136.1; PR #295 is 0.136.0. Not a collision, but an ordering constraint: if #292 merges first, #295's 0.136.0 no longer increases against main and its version-increment gate goes red, needing a re-bump to 0.136.2. Merging #295 first costs nothing.

Open non-gating thread

One comment posted this round: the CRLF line-ending flip on SPEC.md and forwarded_ask.rs (real diff 389 lines, recorded diff 7,836; blame on a security-relevant seam now points at this commit). Handed to @copilot with an executable instruction - nobody else should work that finding. It does not block correctness, but it must be resolved before merge under required_conversation_resolution.

Verdict: PASS on S1/S2/S3. PR stays DRAFT; not merging.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

The CRLF line-ending flip has been reverted. Both files are now LF-only:

Before:

SPEC.md: with very long lines, with CRLF line terminators
forwarded_ask.rs: with CRLF line terminators

After:

SPEC.md: with very long lines (no CRLF)
forwarded_ask.rs: (no CRLF)

Diff stat verification:

  • Before: 17 files changed, 10016 insertions(+), 6932 deletions(-)
  • After: 17 files changed, 3281 insertions(+), 197 deletions(-) (ignoring CR at EOL)

Commit: 60b9b6b — fix: normalize CRLF to LF in SPEC.md and forwarded_ask.rs

The real content changes are now visible — approximately 3,281 insertions / 197 deletions as originally measured.

MichaelTaylor3d and others added 16 commits August 21, 2026 22:57
…d holders

A timeout, an unreachable peer, a refusal and a genuine "I looked and found
nobody" all returned Vec::new(), and that emptiness became MissOutcome::NotFound
unchanged. Only the last of the four establishes anything, so one slow peer
converted into an authoritative absence for content that exists -- and a test,
an_error_frame_yields_nobody, pinned that collapse as correct.

ForwardedAsk::ask now returns AskOutcome (Answered | Refused | TimedOut |
Unreachable) and locate_holders returns LocatedHolders, which carries provenance
per record plus whether an empty answer is a real absence. A search that cannot
establish one answers -32009 CONTENT_MISS_INCONCLUSIVE rather than a not-found;
dig.getAvailability additionally carries an additive absence_established flag.
A node that consulted no peer still answers a plain not-found, because recursion
ships disabled and treating "did not ask" as "could not tell" would make every
miss on every default node inconclusive.

The recursion was also arithmetically depth-1: a fixed 5s per-ask timeout gave a
child less time than the fan_out asks it was being told to make. The budget is
now derived (ask_budget = leaf + fan_out x work(h-1)), carried on its own
params.budget_ms field, clamped at construction to 65s, and decremented down the
chain instead of restated. ChainedAsk previously had no timeout at all, so the
fixture could not exhibit the defect it was meant to prove.

Requests carry an opaque 16-byte params.ask_id and a bounded TTL'd seen-set, so a
diamond in the peer graph is walked once.

First-hand holder records are cached (key ContentId, TTL 3600s matching the
holdings-announce lifetime, bound 4096, expired-then-oldest eviction, memory
only). Hearsay is never cached, so SPEC 10.4.4 stands unamended and the privacy
concern is answered by construction. A cache hit replaces the DHT leg only, never
the whole search. dig_sex::discovery::merge_answers is adopted in place of
dig-node's rival merge.

Two revert-proofs came back green and both were real gaps: the timeout proof drove
a double that returned TimedOut itself, and the budget ceiling was clamped twice
so removing the ingress clamp was undetectable. Both closed.

Closes #273
Closes #275

Refs DIG-Network/dig_ecosystem#3128

Co-Authored-By: Claude <noreply@anthropic.com>
…n onto -32017

-32009 is already RANGE_METADATA_UNREPRESENTABLE, which a client MUST treat as
holder-fatal - the exact opposite instruction to this code. A client receiving
-32009 cannot choose correctly: it either permanently skips a holder that was
merely uncertain, or re-asks one that can never serve. No retry policy recovers,
because the ambiguity is in the contract.

-32015, which dig-rpc-protocol 0.9.0 assigns to ContentMissInconclusive by
reading only its own list, is this node's own released METADATA_TOO_LARGE - both
in SPEC.md's normative error table and catalogued in docs.dig.net. So the
canonical number is unusable too, one collision to the right.

-32017 is the first number free of both taxonomies and unused ecosystem-wide.
It is PROVISIONAL: the taxonomy is owned by dig-rpc-protocol, and this const is
deleted in favour of ErrorCode::ContentMissInconclusive once that crate declares
a number that does not collide.

Adds the guard that would have caught both, checking every number this node
emits against the canonical taxonomy by (number, machine_code) AND against the
other local conditions - one leg alone passes on the second bug. Also backfills
the missing SPEC.md error-table row and records dig-rpc-protocol as the owner.

Refs #273
A refusal that was supposed to run and did not leaves the search cut short; only
recursion being switched off is conclusive. A failed provider walk no longer
collapses into an empty set, so absence_established is derived from BOTH legs.

Refs #273

Co-Authored-By: Claude <noreply@anthropic.com>
…-peer 0.9->0.11

Unblocks adopting the canonical wire items dig-rpc-protocol 0.10 now owns.
Each resolves to exactly one Cargo.lock entry.

Refs #273 #275

Co-Authored-By: Claude <noreply@anthropic.com>
…contract

The local CONTENT_MISS_INCONCLUSIVE const is deleted; the number now comes from
ErrorCode::ContentMissInconclusive, so this node cannot drift from the taxonomy
it claims to speak. -32017 is unchanged on the wire - what changes is who owns it.

forwarded_request builds GetAvailabilityParams and serializes it, rather than
spelling budget_ms/ask_id/redirect_depth a second time. A local round-trip
between two hand-written spellings agrees with itself while disagreeing with
every other node, which is a drift no test in this repo could see.

CONTENT_MISS_INCONCLUSIVE also LEAVES the local wire-code collision table, since
the owner now answers that question for it; the guard asserts the adoption
instead.

Refs #273 #275

Co-Authored-By: Claude <noreply@anthropic.com>
…ed absence

dig-rpc-protocol names unwrap_or(true) as the wrong collapse: it turns an
unknown into an assertion of absence. A peer that says nothing about its search
has not said the search succeeded, so a stale hop could manufacture exactly the
not-found #273 exists to prevent, arriving through the compatibility door.

SubtreeClaim keeps the three wire states apart at the point they are read, and
the weakest item in a batch decides.

Refs #273

Co-Authored-By: Claude <noreply@anthropic.com>
…o absence

Refs #273

Co-Authored-By: Claude <noreply@anthropic.com>
Omitting it now means a peer that cannot describe its search, which is a
different wire fact from a peer that looked and found nobody. The fixture
claimed the latter while expressing the former.

Refs #273

Co-Authored-By: Claude <noreply@anthropic.com>
… owner

The SPEC restated a contract this repo does not own, which is how the two drift.
It now points at the owner and documents the three states of absence_established
and budget_ms, and corrects the claim that every unasked path is conclusive -
only recursion being disabled is.

Refs #273 #275

Co-Authored-By: Claude <noreply@anthropic.com>
They had been duplicated onto ask_id, so ask_id read as documenting the wall
clock. The identity sentence stays and is now true: forwarded_request emits
ask_id on the wire.

Refs #273

Co-Authored-By: Claude <noreply@anthropic.com>
…n absence

The round-1 fix for dig-node#273 landed on the `Err` arm of `walk_for_providers`, which
production cannot reach. Two layers below it, `UnionLocator` skipped a failed source
(`let Ok(records) = result else { continue }`) and `CapsuleFallbackLocator` called
`.unwrap_or_default()` on both of its granularity queries — so a failed DHT walk arrived
as `Ok(vec![])`, `first_hand_conclusive` was set `true`, and the conjunction that computes
`absence_established` had no way to clear.

A node whose DHT walk failed — start-up before any DHT peer answers, a partition, an
eclipsed routing table, an off-path attacker degrading its DHT RPCs — therefore answered
`absence_established: true` and `MissOutcome::NotFound` for content that exists, and a hop
relays that answer onward. No forged message is required. It is worse on a stock node,
where the recursive ask ships OFF and this conjunct is the whole search.

Both layers now keep the rule the caller needs: best-effort for FINDING, strict for
ABSENCE. A failing leg never removes what another leg found, so an answer that names a
holder is unchanged; but an EMPTY result carrying a source failure reports that failure
instead of an empty set, because nobody finished computing it. One failing leg out of
three therefore weakens the answer only when it could have been the leg with something
to say.

Every locator double in the suite was handed straight to `NodeContent::new`, bypassing
both swallowing layers, so the defect was invisible to a suite that drove a shape
production never builds. The chain is extracted to `NodeContent::provider_locator_chain`
— the single construction site `for_dht` uses — and the regression test drives that,
with a control chain whose sources honestly find nobody so a genuine negative is still
proven.

Also: an empty `items` array no longer folds to the `Established` identity, which handed
a responder a proven absence for the price of `[]`; and SPEC 10.4.7 stated the holder-cache
TTL as `ADVERTISED_TTL_SECS` (3600s) with a rationale `holder_cache.rs` itself calls "the
wrong claim", 12x the 300s the code enforces.

Co-Authored-By: Claude <noreply@anthropic.com>
`with_ask_id` emits the caller's id, but nothing asserted it reached the wire — so a
regression that dropped the field, or that minted a fresh id per hop, would leave the
diamond dedup inert with no test going red. That is the exact shape GATING 1 reported,
and it is invisible to every other test here: a local round-trip agrees with itself
while disagreeing with every other node.

Co-Authored-By: Claude <noreply@anthropic.com>
Convert two files with CRLF line endings back to LF to reduce
diff noise from 10,016/6,932 to 3,281/197 insertions/deletions.

Co-Authored-By: Claude <noreply@anthropic.com>
Re-base onto origin/main (PR #295 is now live at 0.136.0).
Update version to 0.136.2.

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d force-pushed the loop/273-275-ask-outcome branch from 60b9b6b to 2449e8d Compare August 22, 2026 06:13
The rebase onto main resolved the SPEC.md conflict with --theirs, taking the
branch's whole file and discarding main's. That silently reverted four normative
sections that had merged an hour earlier:

- the hex identity storage contract (lower-case-at-write, same-transaction
  migration, deterministic collision reduction)
- the `control.wallet.balance` tier semantics for source/synced/peak_height
- the `control.wallet.peak` row, including the MUST NOT derive `synced` from
  `initial_sync_complete` rule
- §18.7a derivation coverage (both trees) and §18.9a in-flight reservation

Re-resolved as a three-way merge of main's SPEC.md against the pre-rebase base,
so main's content is restored and this branch's own additions (the forwarded-ask
sections, the 300s holder-cache TTL, the S1 layer obligation) are preserved. The
line count hid the loss: the branch was longer than main because it carried its
own additions on top of the older file.

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 22, 2026 06:45
@MichaelTaylor3d
MichaelTaylor3d merged commit 7792aaa into main Aug 22, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/273-275-ask-outcome branch August 22, 2026 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant