Uh oh!
There was an error while loading. Please reload this page.
fix(reshare): keep a severed capsule pull's partial so the retry resumes - #330
Conversation
A failed capsule warm erased its own staging artifacts, so dig-download's resume machinery was structurally unreachable: the checkpoint survived while the bytes it described did not, and every retry restarted from offset 0. The keep-or-erase choice is now a StagingDisposition decided per failure kind: a verification failure discards (NC-12 — bytes attributable only to a proven-false descriptor must not survive), every transport or local failure preserves. Refs #328 Co-Authored-By: Claude <noreply@anthropic.com>
Bumps dig-node-service 0.144.1 / dig-node-core 0.54.2. Refs #328 Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS — correctness + security gate
Head reviewed: 9942e68135dca55ad9a0d5c63b55048bebb23317 (re-resolved from the remote; matches the dispatched sha). Reviewed in an isolated worktree at C:\tmp\worktrees\gate330; no shared checkout was mutated.
The three questions, answered
1. Can any Preserve arm carry unverified bytes that are not re-verified chunk-wise on resume? — No.
Every staged byte passes attribution twice, and the second pass is against a FRESH descriptor:
- a chunk is written only after matching
info.chunk_hashes[i](dig-download-0.19.1/src/module.rs:487, afterfetch_verified_chunk); - on resume, every checkpointed chunk is re-read and re-hashed against the new attempt's descriptor (
module.rs:459→read_back_verified_chunk,module.rs:745), returningNoneand re-fetching on any mismatch, short read or read error; - a checkpoint whose
chunk_lensdo not match the new plan is never partially reused —load_or_fresh_state(module.rs:715) returns a fresh state andpull_with_descriptorthen callssink.truncate(0)(module.rs:438), so no stale tail can ride into a later plan; - the whole-blob
module_hashgate (module.rs:501) and the chain-anchor gate throughStagedModuleReader(module.rs:527, re-verifying per chunk on every read, bounded tototal_size) both run beforepromote_verified; promote_verified(sink.rs:191) refuses both directions — shorter than the verified length (sink.rs:207) and anything past it (sink.rs:214).
So a hostile hop's preserved bytes survive a resume only if they are byte-identical to what the honest descriptor demands. Preserving can cost a re-fetch; it cannot produce a wrong artifact. The three arms flagged for scrutiny check out on meaning, not name:
State(_)— includesModuleAnchor::Unavailable(module.rs:544), i.e. this node could not reach a chain answer. Blameless-local, and the blob still faces both gates on resume. Preserve is right; discarding here would let a chain-source blip cost a whole capsule.Sink(_)— local disk/read-back faults, including thesupports_read_backrefusal (module.rs:520). A write that failed leaves the chunk uncheckpointed, so resume re-fetches it.TaskEnded— orchestrator channel closed without a verdict. Nothing impeached the bytes; re-attribution decides on resume.
2. Is the exhaustive-match compile-error guarantee real? — Yes.
dig_download::DownloadError is declared #[derive(Error)] pub enum DownloadError at error.rs:75-76 with no#[non_exhaustive] (the attribute appears in that crate only on orchestrator.rs:76, source.rs:43, verify.rs:156). Its variant set is exactly the twelve the match names — Transport, Timeout, Verify, NoProviders, NotFound, MetadataProbeFailed, PagedPrologueUnsupported, Cancelled, State, Sink, NotDownloadable, TaskEnded — and for_failure has no _ arm. A new upstream variant is a build break, not a silent default.
3. Does the verification-failure fixture genuinely stage bytes before failing? — Yes, proven by mutation rather than by reading.
Under the Verify(_) → Preserve mutation, a_verification_failure_still_discards_what_it_stagedfails. It could only fail if the partial actually exists on disk at that point; a fixture that staged nothing would have satisfied !staged.exists() under both dispositions and stayed green. The holder serves honest chunk bytes and honest per-chunk hashes and lies only in module_hash, so chunks pass per-chunk attribution, get written, and die at the whole-blob gate — exactly the shape needed.
Mutations reproduced (in my worktree, at head)
| mutation | result | claimed |
|---|---|---|
E::Verify(_) → Preserve | 19 passed / 2 failed — a_verification_failure_still_discards_what_it_staged + only_a_verification_failure_erases_the_partial fail, resume test stays green | 19/2 ✅ |
restore unconditional discard_staging(&staged) on the Err arm | 20 passed / 1 failed — only a_severed_warm_resumes_from_its_partial_instead_of_refetching_the_capsule fails | 20/1 ✅ |
Both directions are discriminated, by separate tests, in opposite directions. Baseline at head: 21/21 in module_reshare.
The other two discard_staging sites are correct and unchanged
module_reshare.rs:695 (post-promotion cleanup) and :713 (promotion refused — bytes that failed the local re-verify gate). Neither should be disposition-aware.
Checks + SPEC + version
check-merge-preconditions.sh --allow-draft→ EXIT 0, all five required contexts asserted BY NAME asSUCCESS(Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage),unresolvedReviewThreads=0.SPEC.md:6304— the new keep-or-erase clause is honest. Each of its claims is discharged above, including "MUST NOT be promoted, announced, or counted as progress by any path other than a resume of the same generation", whichmodule_download_key+ thechunk_lensequality check enforce structurally.- Versions
0.144.1/ core0.54.2, consistent acrossCargo.toml, the crate manifest andCargo.lock. - §2.4b hold verified as correct.
dig-download0.20.0 declaresdig-nat 0.20,dig-dht 0.12,dig-peer 0.12; this tree resolvesdig-nat 0.18.0,dig-dht 0.11.1,dig-peer 0.11.0,dig-gossip 0.25.0,dig-tls 0.3.1. Taking 0.20.0 alone would deepen an existingchia-protocol 0.26 / 0.36.1split rather than resolve it. Reporting the shape and holding is the right call under §2.4b's "if the delta does not line up, report and stop".
Two non-gating notes are posted inline and resolved by me so they cannot block the merge.
Verdict: PASS. Do not undraft or merge — that is the orchestrator's call.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Closes#328
Parent epic: https://github.com/DIG-Network/dig_ecosystem/issues/3128
The defect
module_reshare.rs:602calleddiscard_staging(&staged)on the pull'sErrarm, removing both thestaging target and
dig_download::staging_path_for(staged). The resume checkpoint survived while thebytes it described did not, so
resumes_stagingcorrectly refused a checkpoint whose staging file hadvanished and every retry restarted from offset 0. dig-download's resume machinery was intact and tested,
and structurally unreachable for capsule warms.
Field measurement on v0.143.1 (a 134,968,945-byte capsule cut at 34,603,008 staged bytes): attempt 2
transferred 182,219,987 bytes against an uninterrupted control of 182,184,850 — 0.02% MORE than a
from-scratch download.
The fix
crates/dig-node-core/src/seams/dig_peer/module_reshare.rs— a newStagingDispositiondecideskeep-or-erase per failure kind, and the
Errarm applies it instead of discarding unconditionally.How transport failure is distinguished from verification failure
StagingDisposition::for_failure(&dig_download::DownloadError)— an exhaustive match with no_arm:DownloadErrorvariantVerify(_)module_hashmismatch, or a blob the chain does not anchor). NC-12: a hop's bytes may be garbage, and garbage that survives is garbage a later honest range completes around.Transport,Timeout,NoProviders,NotFound,MetadataProbeFailed,PagedPrologueUnsupported,CancelledState,Sink,NotDownloadable,TaskEndedStructural, not conditional, in two ways:
dig_download::DownloadErroris not#[non_exhaustive], so the wildcard-free match makes a newupstream failure mode a compile error here. The keep-or-erase question cannot be answered by
default; it has exactly one place it can be answered, once, per error kind.
Errarm except through the disposition — the call sitenames the decision, not the effect.
Chunk-wise verification of a resumed partial — the #3128 shape question, answered
It already exists, which is what makes preserving safe rather than merely cheap.
dig_download::ModuleDownloader::read_back_verified_chunk(module.rs:745) re-reads each checkpointedchunk out of staging and re-hashes it against the descriptor's
chunk_hashes[i]; a mismatch returnsNoneand the chunk is re-fetched. Preserving a partial can therefore cost a re-fetch, never a wrongartifact. Two further layers hold behind it: the whole-blob
module_hashgate, and the chain-anchor gatewhich reads through
StagedModuleReader— a bounded, chunk-re-verifying window — strictly beforepromote_verified. Nothing was needed in this PR; reported per the brief.Also measured while establishing the above (worth knowing, not changed here): dig-download already erases
staging itself in the poisoned cases it can see — it clears state +
truncate(0)on a descriptordemotion, on a plan-shape mismatch, and on a promotion refusal. The one path where suspect bytes reach the
caller is descriptor-attempt exhaustion (
module.rs:397), which returns before those cleanups — and thatis precisely the path this PR's
Discardbranch covers.Tests
Three added, in
crates/dig-node-core/src/seams/dig_peer/module_reshare.rs:a_severed_warm_resumes_from_its_partial_instead_of_refetching_the_capsule— aSeveringHolderserveshalf the chunks, dies, then heals; the second warm is asserted by byte count:
served_in_attempt_2 == capsule_bytes - staged_in_attempt_1, plus< capsule_bytes. Controls, so theassertion cannot pass vacuously: attempt 1 must stage a genuine partial (
0 < staged < capsule), the.download.tmpmust exist and be exactly that many bytes, attempt 2 must fetch something, and thecached capsule must be byte-identical. The SAME holder is reused, healed, so cheapness cannot be
explained by source selection instead of resume; the state store is shared across both warms.
a_verification_failure_still_discards_what_it_staged— a holder with honest chunk bytes and honestper-chunk hashes lying only in the whole-blob
module_hash, so chunks are genuinely staged and there issomething on disk to erase. Asserts both staging paths are gone and nothing reached the cache.
only_a_verification_failure_erases_the_partial— the disposition table directly.Mutation proof (both directions, committed first, restored by file copy — never
git stash)discard_staging(&staged)restored unconditionally in theErrarma_severed_warm_resumes_from_its_partial_instead_of_refetching_the_capsuleFAILED atmodule_reshare.rs:1515(the "partial must SURVIVE" assertion). 20 passed, 1 failed — no over-firing onto the discard tests.E::Verify(_) => Preservea_verification_failure_still_discards_what_it_stagedFAILED andonly_a_verification_failure_erases_the_partialFAILED. 19 passed, 2 failed — the resume test stayed green, so the two branches are independently load-bearing.Evidence
cargo test --no-fail-fast -p dig-node-core --lib→ 961 passed; 0 failed; 0 ignored; 0 filtered out(the targeted filter resolved 21 tests / 940 filtered, so it matched rather than silently selecting
nothing).
cargo fmt --all --checkclean;cargo clippy --workspace --all-targets -- -D warningsclean.Cargo.lockupdated with the bump (required under--locked). CR count of every touched file: 0.Blast radius checked
gitnexus impact discard_staging --direction upstream --repo dignode-328(index built in this worktree):risk LOW, 5 impacted, 1 direct —
CapsuleWarmer::warm_with_config→warm_claiming/warm_capped→warm/warm_relayed, all insidemodule_reshare.rs, one module (Dig_peer), 0 execution flowsaffected. No public API changed;
StagingDispositionis private to the module.Version + deps
dig-node-service0.144.0 → 0.144.1,dig-node-core0.54.1 → 0.54.2 — patch: a bug fix, no APIchange. Read from
origin/mainat bump time (0.144.0, i.e. after #326).§2.4b dep bump deliberately NOT taken, and why:
dig-download0.20.0 is the chia-0.36 transport-stackuplift. dig-node's
dig-dht0.11.1 /dig-peer0.11 /dig-peer-selector0.9 are still on the chia-0.26line, so bumping dig-download alone would ship this crate internally split across two chia lines — the
exact defect §2.4b exists to prevent. It belongs to the chia-0.36 cascade, not to a resume fix.
Spec
SPEC.md— the clause "A failed pull MUST leave no staging artifact behind" was the specified form of thisdefect and is replaced by the keep-or-erase-by-failure-kind rule, including why preserving is safe (the
per-chunk re-attribution) and that a preserved partial is never promoted, announced, or counted as progress
by anything but a resume of the same generation.