fix(download): distinguish not-found from timed-out and cache first-hand holders - #292
Conversation
59688dd to
9304dc1
Compare
Resume-ready progress — lane restarted after the previous capBranch: Recovered work the previous lane never committedThe capped lane had left uncommitted work in its worktree — not just the stub the PR showed. It is
It does not build yet: callers are unmigrated. That is the next step, and it is deliberate — the Remaining
Next command
|
e682014 to
7a17e6c
Compare
Lane complete — ready for the gate round. Still DRAFT; do not merge.Branch CI — all 14 required checks PASS by name
Two false greens found and closed during revert-proofingBoth were MY tests, and both came back green when their fix was reverted:
A design defect the PRE-EXISTING tests caughtMy first cache version short-circuited the whole of For the gate's attention
|
GATING — orchestrator finding, before the gate round:
|
| 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_establishedon thedig.getAvailabilityanswer.
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
.rsfile (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_holderson 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 = 65sis derived, being exactlyask_budget(2,3)at thedig-sexdefaults, 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.
Adoption lane — progress + a BLOCKER on the canonical numberBranch BLOCKER:
|
Adoption BLOCKED on a 3-crate release-first cascade — what landed insteadHead The pin bump is not possible from this repo aloneBumping both pins to
And the canonical number cannot be adopted even after the cascade
Occupancy in this repo: Recommended: dig-rpc-protocol 0.10.0 reassigns What this PR does now
Version: unchanged at Reported, not fixed here
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. |
loop-security — INTERIM FINDINGS (1 of 2 batches), audited head
|
ADVERSARIAL VERIFY — REFUTED. This PR does not ship the distinction it claims to ship.Head A — "one slow peer can no longer become an authoritative absence": REFUTED, four independent waysA1 — the DHT leg's failure is discarded, and it is the leg that runs on EVERY node
LocatedHolders {
records,
conclusive: forwarded.conclusive, // <- the DHT leg is not consulted at all
}and 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 ( Consequence, and this is the load-bearing one: Probe: A2 — the cascade merge rule launders an inconclusive CHILD into a proven absenceThis 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. 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 So on the very verb the recursion uses, Probe: A3 — every node-local refusal is CONCLUSIVE, and #273 forbids exactly that
#273's own constraint list: "A hop that refuses (disabled, budget spent, no eligible peers) must produce Probes: A4 — the control in
|
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
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 atlib.rs:4731, the canonical leg: "local CONTENT_MISS_INCONCLUSIVE = -32009 is already canonically RANGE_METADATA_UNREPRESENTABLE..."= -32015→ panics atlib.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.
loop-security — FINDINGS (2 of 2), head
|
loop-security VERDICT: CHANGES-REQUIREDAudited head: Ranked findings (detail + exploits in the two comments above)
Why these gateSEC-1, SEC-2 and SEC-3 are the same defect wearing three faces, and it is the inverse of the one The net effect is that the PR does not merely fail to close the hole — it widens it. Before, a SEC-3 in particular needs no adversary and is reachable today by an anonymous peer over the ordinary What is genuinely good, and should not be re-litigatedThe wire error-code surface is clean and was checked independently rather than inherited: Minimum to clear this gate
Re-gate scope on the fix: this leg (loop-security) in full — every finding above is on the same PR stays DRAFT. Do not merge. |
Progress — resume-readyBranch Step 0 — rebase: NO-OP, zero conflicts. Recovered work first. The worktree held ~560 lines of uncommitted prior-lane work (recursion-disabled vs refused, failed-DHT-walk inconclusive). Committed as Step 1 — three of four bumps DONE ( Step 3 — Bumping all 13 Nine of the errors are dig-node's own Making it one chia line requires moving Premise correction: dig-node does NOT carry two digstore revs. All 13 NEXT ACTION: step 2 — finish the prior lane's refactor onto the canonical |
4f319e0 to
5e32b5d
Compare
Round complete — handing back to the gate. Head
|
loop-security — interim findings (audit in progress)Auditing head Clear so far
Findings raised so farA.
A green build proves the API surface B. C. A peer named by |
loop-security — GATING finding: the DHT-leg half of #273 is dead code in productionHead audited: The claim the PR makes
Err(error) => {
tracing::debug!(%error, "locate: the DHT walk failed; absence unproven");
(Vec::new(), false)
}
and the new SPEC clause (§10.4.4) states it as a MUST:
The measurement: that
|
loop-security VERDICT: CHANGES-REQUIREDHead audited: One GATING finding, posted in full above. Three non-gating. The core of the PR — the three-state GATINGS1 (HIGH) — the DHT-leg half of #273 is unreachable in production. NON-GATINGS2 (MEDIUM, defense-in-depth) — S3 (MEDIUM) — SPEC 10.4.7 states a TTL the code deliberately repealed, on a security bound. SPEC says the TTL is S4 (LOW, note) — Answers to the specific questions asked
Merge preconditions (FYI, not my gate)All five required contexts SUCCESS by name on |
|
Non-gating (fix in this PR, cheap): CRLF line-ending flip on two files inflates the diff ~19x and destroys blame.
Why it matters beyond tidiness: the real edit is 389 lines, the recorded one is 7,836. @copilot fix this: re-normalize |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
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 returningtrue) rests on the claim that an empty slate is never remembered. Verified at the source:holder_cache.rs:207-210returns early whenrecords.is_empty(). Thattrueis 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 infetch_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 --libgave 906 passed, 0 failed (477s), matching the claim. - Revert spot-check, union guard only (deleted the
if merged.is_empty()block inunion_locator.rs; my own worktree, file-copy revert, never agit checkout):forwarded_ask_tests::a_failed_dht_walk_stays_unproven_through_the_production_locator_chain- FAILEDunion_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 insidean_empty_items_array_establishes_nothing) rules out an implementation that never establishes anything. Not guard-not-exercised:dispatch.rsrejects a malformeditemsparam upstream, but a well-formed empty array reaches this guard. - SPEC 10.4.7 TTL - verified against code rather than accepted:
holder_cache.rs:71isHOLDER_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_idwire pin (forwarded_ask.rs:562-570) - can fail. It asserts on the output of the production builderforwarded_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_skippedtoa_dormant_source_is_skippeddid not paper over a gap. The old fixture heldEmptyLocatorand 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-builtFailingSourcedouble. 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.
SelfExcludingLocatoras an unfixed third swallow sitting between the two patched layers - uses?.- An empty cached slate re-entering as
conclusive: true-rememberrefuses empties. - The infallible
find_providersreaching an absence decision - one debug-only caller. - The extracted chain silently diverging from what
for_dhtused 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.
|
The CRLF line-ending flip has been reverted. Both files are now LF-only: Before: After: Diff stat verification:
Commit: The real content changes are now visible — approximately 3,281 insertions / 197 deletions as originally measured. |
…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>
…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>
60b9b6b to
2449e8d
Compare
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>
DRAFT -- DO NOT MERGE. Gate round has not run.
Also closes the
digstore-chainchild 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::askoffVec<ProviderRecord>onto an outcome type, which cascades throughforwarded_holders→locate_holders→MissOutcome→ the wire. #275 needs a cache wrapped around thesame
locate_holderscall with each record taggedFirstHand | 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
impactwas unavailable in this worktree (no.gitnexusindex; 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:
ForwardedAsk::ask(trait signature)NatForwardedAsk(prod),RecordingAsk,ChainedAsk(tests) — all 3 migratedHopBudget(tuple → named struct, +2 fields)from_params/fresh/spent/at_depth+ 30 call sites acrossdownload.rs,lib.rs,peer.rs,forwarded_ask_tests.rs. No call site changed — every constructor kept its signaturelocate_holders→LocatedHoldersdownload.rs:answer_miss,lib.rs:3751(availability responder), plus test sitesMissOutcome(+Inconclusive)download.rs, 1 match inpeer.rs:1636— exhaustive matches, so the compiler enumerated themdedup_by_peerWARNING — HIGH risk on
ForwardedAsk::askand on thedig.getAvailabilityanswer shape. The trait ispub(crate), so the radius is crate-local, but the wire behaviour changed: a node whose search cannotestablish an absence now answers
-32009where it previously answered a plain not-found. That is theintended fix and it is stated in SPEC §10.4.4.
detect_changes()was likewise unavailable; the diff was instead confirmed to touch only the expectedsymbols by
git diff --statplus a fullcargo 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:and aggregated in
download.rs:MissOutcomegainsInconclusive, answered on the wire asCONTENT_MISS_INCONCLUSIVE = -32009.dig.getAvailabilityadditionally carriesabsence_established: bool(additive; an older peer omits itand a reader falls back to the tolerant reading).
an_error_frame_yields_nobodywas replaced, not fixed — it pinned the collapse as correct, which iswhy 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 intoFORWARDED_ASK_LEAF_TIMEOUT(5s, one leaf ask) plusask_budget(hops_remaining, fan_out) = leaf + fan_out × ask_budget(h-1), clamped byMAX_FORWARDED_ASK_BUDGET = 65s— which is exactlyask_budget(2, 3)at thedig-sexdefaults, sothe ceiling is derived rather than chosen.
The budget rides a NEW field,
params.budget_ms— notredirect_depth. The two move in oppositedirections (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 whatis LEFT, so the budget is carried down and decremented rather than restated.
ChainedAsknow imposes a REAL timeout. It previously had none, which made it structurally unable toexhibit 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 abounded TTL'd
AskSeenSet(TTL =MAX_FORWARDED_ASK_BUDGET, since a request cannot outlive the largestbudget 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.
ContentIdADVERTISED_TTL_SECS= 3600sAn 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_answersis now ADOPTED (it caps the hearsay portion only and tagsprovenance), and dig-node's rival merge plus the bare
dedup_by_peerare 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:
Answered(vec![])Answered(vec![])Gap 1 — the timeout proof was vacuous. Reverting the production timeout mapping came back green,
because the proof drove a
ForwardedAskdouble that RETURNEDTimedOutitself: it asserted the verdictthe code was supposed to reach. The classification is now a delegated
awaited_outcomehelper driven by areal 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_holderson a cache hit, so a node holding anyfirst-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_heldandthe_relay_allowance_is_per_requestor_and_separate_from_the_lookup_budget— failed, because both boundsread 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
providersMUST be read as 'found nobody'" reading isREPEALED; four outcomes are now distinguished. Adds the cascade rule (+
-32009), the time-budgetclause (
budget_ms, clamp, decrement), the ask-identity clause, and themerge_answersadoption. Thehearsay MUST is unchanged.
preserved rather than re-derived.
slate, discovery-shortcut-not-an-answer, invalidation, memory-only.
Dep bumps -- the cascade, and the ONE that was measured and refused
Each resolves to exactly ONE
Cargo.lockentry --grep -c 'name = "<crate>"' Cargo.lockis1fordig-rpc-protocol,dig-download,dig-peeranddigstore-chain.tests/dependency_tree.rsasserts the singleton property mechanically and now pins the
0.10.line, so a 0.x caret thatsilently failed to reach the next minor fails a test rather than 49 type errors later.
digstore-chain4c34f0be->222f08d: MEASURED, then REVERTED, and the reason is on the money pathThe bump itself works -- all 13
digstore.gitdeps move, the lock resolvesdigstore-chain 0.26.0.It then fails with 19 type errors that are all one thing:
222f08dcarries the chia-wallet-sdk0.34 uplift, so digstore-chain speaks
chia-protocol 0.36.1while dig-node speakschia-protocol 0.26. Both lines were already in the lock before the bump; what the bump does is movedigstore-chain to the far side of that boundary.
Nine errors are dig-node's own
ChainReadsimpl (store_melted.rs:1318-1381), and two of those arepush(SpendBundle)andestimate_fee(SpendBundle). Converting aSpendBundlebetween twochia-protocolversions is precisely the byte-drift class dig_ecosystem#2610 exists to remove, andconfirm_melt_via_chainon the other side of the same trait authorizes a store DELETE. A boundaryshim 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 thechia = "0.26"umbrella in two manifests (dig-node-service:229,dig-wallet:35) with the fivedirect facades -- the umbrella has no 0.36 line (0.32 -> 0.42). That is #2610's
dig-walletchildplus dig_ecosystem#2228, not a rev bump, and it is left for that family.
Premise correction: dig-node does NOT carry two
digstorerevs. All 13digstore.gitdeps sit atone rev,
4c34f0be.51054a41is a dig-gossip rev (dig-node-core/Cargo.toml:224,387), adifferent repository. There was nothing to reconcile.
Canonical wire adoption -- the local declarations are GONE
dig-rpc-protocol0.10 owns the four wire additions this PR had hand-declared, so they are adoptedrather than restated:
CONTENT_MISS_INCONCLUSIVE-- the localpub constatdownload.rs:112is DELETED. Every sitenow reaches
ErrorCode::ContentMissInconclusive. The number does not change (-32017wasalready the value the owner assigned) -- what changes is who may change it. The code also LEAVES the
local
LOCAL_WIRE_CODEScollision table, because the owner answers that question for it now; theguard asserts the adoption instead of re-declaring the number.
budget_ms/ask_id/redirect_depth--forwarded_requestbuildsGetAvailabilityParamsand serializes it rather than spelling the field names a second time. Alocal 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.mdno longer restates the contract: it namesdig-rpc-protocolas the definition, records theorigin as
Peer, and reproduces the number for readability only with the crate authoritative ifthe two ever differ.
Three-state semantics -- adoption changed BEHAVIOUR, it did not just recompile
absence_establishedwas being collapsed withunwrap_or(true), which the taxonomy owner namesin 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 }; onlySome(true)lets this node carry a peer's absence forward as proven, and the weakest item in abatch 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_msalready 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
absence_establishedSome(true)/Some(false)/ ABSENT, read sideabsence_established_is_read_as_three_states_and_absent_is_not_trueabsence_establishedone_unproven_item_makes_the_whole_batch_unprovenabsence_establishedabsence_established_is_absent_when_no_search_ran_and_present_when_one_didbudget_ms0/ granted, all three pinned to DIFFERENT expected valuesbudget_ms_keeps_absent_distinct_from_zero_and_from_a_granted_valuebudget_msSome(0)behaviour: asks nobody AND claims no absencean_exhausted_budget_asks_nobody_and_does_not_claim_the_absenceRevert-proofs for this round
Each fix reverted ALONE, from a COMMITTED tree (file copy, not
git checkout), and restored.Reverted
None => NoClaimback to theunwrap_or(true)collapse -- two tests fired:Reverted the
budget_mszero/absent distinction (.filter(|ms| *ms > 0)) -- the assertion thatfired is the side effect, which is deliberately first:
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) OMITTEDabsence_establishedwhile claiming to model "apeer that looked and found nobody". Under correct semantics that fixture models a peer that cannot
describe its search at all. It now states
trueexplicitly; the absent case has its own test.AskOutcome::into_recordshad no callers once the outcome arms were matched explicitly -- deletedrather than left as dead code.
Also in this round
ForwardedAnswers::not_askedsplit intorecursion_disabled()andrefused(). Only recursionbeing 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.
find_providerswasunwrap_or_default(), so a node with noreachable DHT peer asserted a proven absence for every piece of content in existence.
absence_establishedis now the CONJUNCTION of both legs having finished.ask_idis echoed, not minted per hop -- it travels with the ask, so the diamond dedup firesbeyond the first hop.
AskSeenSet::claimis keyed by(id, content), with a test that the same idasking 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'sErrarm. Round 2 re-fixed it. Both wereunreachable, because the swallow is two layers below where anyone looked:
union_locator.rs-let Ok(records) = result else { continue; }, only returnOk(merged). SoUnionLocator::find_providerswas infallible.capsule_fallback.rs- two.unwrap_or_default()calls, thenOk(merged).A failed DHT walk therefore arrived as
Ok(vec![]),first_hand_conclusivewas settrue, and theconjunction that computes
absence_establishedhad 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: trueandMissOutcome::NotFoundfor contentthat 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 resultcarrying 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.md10.4 so a reimplementation cannot rebuild the swallow.Why the suite missed it, and what changed.
engine_overhands its double straight toNodeContent::new, bypassing both swallowing layers - every locator test drove a shape productionnever builds. The chain is extracted to
NodeContent::provider_locator_chain, the single constructionsite
for_dhtuses, anda_failed_dht_walk_stays_unproven_through_the_production_locator_chaindrivesthat. 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 theEstablishedidentity (forwarded_ask.rs) - it handed aresponder a proven absence for the price of the cheapest message on the wire.
NoClaimnow.ask_idis pinned onto the request bytes - it was emitted but unasserted, so a regressiondropping it would leave the diamond dedup inert with nothing red.
ADVERTISED_TTL_SECS(3600s) to the 300s the codeenforces. The SPEC carried a rationale
holder_cache.rs:71itself calls "the wrong claim"; a secondimplementation 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 countand
bandwidth.rs- neither claims an absence.download.rs:1132), which unions the pool source with this chain: a faileddiscovery 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'sdiscovered_candidateskeeps itsOk-else-empty: it collects dial addresshints and makes no absence claim, which its own doc states.
UnionLocator::new/CapsuleFallbackLocator::newhave no other discovery-path construction site.Version
0.135.0 -> 0.136.1(workspace),dig-node-core 0.49.0 -> 0.50.1(Cargo.lockown-version entriesupdated to match). The gate-round-3 fix is a patch on top of the round's minor: no public API changed
(
UnionLocator/CapsuleFallbackLocatorarepub(crate);NodeContent::find_providerskeeps itssignature and stays infallible).
Original round-1 version note:
0.135.0 -> 0.136.0. Rebased ontoorigin/maine4afb52(v0.135.0, containing PR#2918d82aca7) with ZERO conflicts -- main was already an ancestor. Minor: new capability plus additivewire fields. The one behaviour change a client can observe is an inconclusive miss answering
-32009instead of a not-found — which is the point of #273.
Notes
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.
.rsfile, which was a syntaxerror; the branch did not build as pushed. Removed.
dig-keystore 0.3.1 -> 0.9.0is unforced but verified clean against crates.io:kdf.rsisbyte-identical,
DIGOP1/SCHEME_ID 0x0004/FORMAT_VERSION_V1are unchanged, anderror.rsisadditive. Carried rather than reverted, and called out here rather than left silent.
arithmetic). It was committed before anything else so a second cap could not lose it.