fix(mirror): release the funding coins of a spend that never lands - #475
Conversation
…hat never lands Co-Authored-By: Claude <noreply@anthropic.com>
…ases its coins The audit-record reservation withheld the funding coins of every non-terminal record, and the #457 resolver promotes only positively -- so a Submitted or Unresolved spend whose coin never appears was never settled and its coins were never released. A genuinely funded operator wallet then reports Insufficient permanently. Unlike the epoch-keyed create suppression, nothing lapsed. The reservation becomes a TIME BOX rather than a status. The record is not rewritten, not settled and not failed: it stays the Submitted or Unresolved it was and stays chaseable by resolve_landed and reconcile. Only the hold lapses. Window is 2 x MIRROR_ROUND_LENGTH_MS = 20 minutes, derived from dig-wallet's own post-broadcast RESERVATION_TTL_MS (10 minutes, ~12 Chia blocks) plus one further round, because this hold is re-evaluated only once per round and a threshold equal to the poll interval would release a record on the first pass at which its confirmation could even have been observed. Refs #471 Co-Authored-By: Claude <noreply@anthropic.com>
…t one is not Refs #471 Co-Authored-By: Claude <noreply@anthropic.com>
… never rewrites its record Refs #471 Co-Authored-By: Claude <noreply@anthropic.com>
loop-security — IN PROGRESS, not the verdictAuditing 1. Both constants verified on disk — the 20-minute figure is what the PR says it is
2. The
|
loop-security — IN PROGRESS, not the verdict (2 of n)Head 4. The clock is NOT caller-supplied, and it fails CLOSED
The 5. The record stays honest — verified structurally, not just by the test
FINDING 1 (MEDIUM, not gating) — the window's safety rationale cites a guard that does not apply to the state this PR is about
That is false for the dominant production state, and the code that makes it false is in this repo:
Money consequence, stated plainly. The doc's conclusion survives even though its reason does So this is NOT a live vulnerability and I am not gating on it. It is a false safety argument in Still to run: the mutation probe, and the version/lock read on disk. |
loop-security — IN PROGRESS, not the verdict (3 of n): the mutation probe, runHead Baseline — the test COUNT, not the exit status
Mutation M1 — the window REMOVED (
|
loop-security — VERDICT: PASSAudited head: The core question: is 20 minutes safe in the DANGEROUS direction?Yes, and the argument does not rest on the PR's own reasoning.
The added round IS labelled a judgement, as the brief required: "The second round is the part The clock is not caller-supplied
The record stays honest — structurally, not merely by assertion
Interaction with #457's resolver — it cannot confirm against another spend's coinThree independent reasons, all read in code rather than in prose:
No second caller reads the old path — counted, not sampled23 matches for The mutation probe, run — both directions, exactly as claimedBaseline
The important shape holds: a fix that releases everything immediately fails the control. Full FindingsFINDING 1 — MEDIUM, not gating: the window's safety rationale cites a guard that does not apply to the state this PR is about
That is false for the dominant production state, and the code making it false is in this repo:
Money consequence, stated plainly. The doc's CONCLUSION survives even though its REASON does not: Why it still matters: a false safety argument in custody code, and the concrete danger is a future FINDING 2 — LOW, not gating: the unbounded rival survives as a PUBLIC API
FINDING 3 — LOW, informational: "one clock reading for the whole pass" is not what the code does
FINDING 4 — LOW, informational, PRE-EXISTING: the audit log is untrusted at rest, in both directions
Areas checked, and why each is clear
What I could NOT reach — stated plainly
Shared stateNo shared checkout touched. All work ran in this PR's own worktree PASS. Findings 1-4 are defense-in-depth and doc accuracy. Finding 1 is worth the short doc |
The window's safety argument cited §25.4.6's duplicate-create suppression as the asymmetry that makes an early release tolerable. That is wrong, and wrong about precisely the case this window exists for: `runner.rs` filters `Pending | Submitted` and excludes `Unresolved` deliberately, while `RecordedSpend`'s `Drop` writes `Unresolved` -- so a record reaching this window is ALWAYS `Unresolved`, and its bond is NOT suppressed. `mirror/resolve.rs` already stated that, so two modules' docs contradicted. What actually holds is the mempool rule, and it is stronger: a released coin re-drawn by any create collides with a bundle that may still be resident, and Chia's replace-by-fee requires the replacement to spend a SUPERSET of the conflicting coins -- which two independent mirror creates never do. The collision is a REFUSAL, not a double spend. Also records that this hold is strictly MORE conservative than the ecosystem's shipped answer for the same phase: dig-wallet's RESERVATION_TTL_MS already releases the same coin at 10 minutes, and this holds it for 20. Found by the pre-merge security gate. Refs #471 Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
DO NOT MERGE — DRAFT, gate round has not run. Custody code; this takes the full triple gate.
Closes #471.
The defect
funding::committed_funding_coin_idswithheld the funding coins of every!is_terminal()auditrecord. Correct as far as it goes — a coin committed to a bundle that may sit in a mempool must not
be reselected, which is the double-spend window #348 exists to close.
What was missing is anything that ages the record out. The resolver added in #457 promotes only
POSITIVELY, on observing the coin, so a
SubmittedorUnresolvedcreate whose coin never appearsis never settled by anything, and its coins are never released. A genuinely funded operator wallet
then reports
Insufficientpermanently.Unlike §25.4.6's create suppression — keyed on the bond's epoch, so it self-clears at the rollover —
this was keyed on the record's status alone, and nothing changes status. Reachable with no attacker
present: a hard kill between
beginand any outcome, or aSubmittedbundle evicted from a mempoolwithout confirming.
The shape (settled before building, §1.10)
A reservation is a TIME-BOXED HOLD, not a status. The record is not rewritten, not settled, and
not failed. It stays exactly the
SubmittedorUnresolvedit was, and stays chaseable byresolve_landedandreconcileindefinitely —Unresolvedmeans "this node signed and does notknow what happened", and that stays true after the coins are released. Writing a fabricated failure
to tidy the bookkeeping is the money lie
Confirmed's shape — height and coin id inside thevariant — exists to prevent, and the tests assert the record is untouched.
Does
Pendingcollapse the problem? No — but it was never part of itSpendJournal::beginwritesfunding_coin_ids: Vec::new(); the ids are populated only atsubmitted(). So aPendingrecord contributes zero coins to the committed set today, andalways did. The stuck set is exactly the records that genuinely had a signed bundle handed to the
network —
Submitted,Unresolved, andFailed { stage: Broadcast | Confirmation }. There is nofree lunch here; the window is genuinely needed. Recording this because the hypothesis was
reasonable and the measurement refuted it.
The window:
2 x MIRROR_ROUND_LENGTH_MS= 20 minutes, N = 2 passesDerived from two named quantities, neither invented:
The chain-side figure is 10 minutes.
dig_wallet's own post-broadcastRESERVATION_TTL_MS(
sage/rpc.rs:544) holds a pushed bundle's inputs for exactly that, and writes out itsreasoning: Chia blocks are ~52 s apart, so ten minutes is roughly a dozen chances for the spend
to land — past the point where a still-unconfirmed bundle is more likely dropped than pending.
This covers the same phase of the same lifecycle, so it takes the same figure rather than
inventing a second one. Two lifetimes for one phase is the disagreement
CLIENT_RESERVATION_DEFAULT_TTL_MSwas written to resolve, not to repeat.One further round, and this part is judgement, stated as such. The hold is re-evaluated only
once per
MIRROR_ROUND_LENGTH_MS, which is also 10 minutes. A threshold equal to the pollinterval aliases badly: a record could be released by the very first pass at which the resolver
is even ELIGIBLE to have observed its confirmation. The smallest window leaving a full round of
chain observation AFTER the chain-side figure has elapsed is two rounds.
Why the asymmetry is tolerable. Releasing too early re-opens #348's double-select; releasing too
late strands money. But §25.4.6 still suppresses a second create for the SAME
(store, root, epoch)while the record is open, so a released coin can only be re-drawn by a create for a different
bond — and that collision fails CLOSED at the mempool.
This is explicitly not a shortening of
RESERVATION_TTL_MS, which that crate's own docs recordas trading a double-select for a lockout — the strictly worse failure. #471 is a lockout
arriving by another route, so the fix must not create a third; it does not touch that constant.
Blast radius (gitnexus indexes are stale here — measured, see caveat)
list_reposreports the registereddig-nodeindex 301 commits behind HEAD (last indexed2026-07-19, and pointed at the primary checkout, not this worktree);
dig_ecosystemis 1,316behind. I ran
impactanyway and it returned exactly the documented false-safe shape —— a confident zero for a symbol with real callers, because it did not exist 301 commits ago. So the
radius below was measured by
grep+ direct read across the whole repo, and I am saying so ratherthan citing a risk rating I did not earn.
SpendRecordgainsreserves_funding_at— new method, zero existing callers.FUNDING_RESERVATION_WINDOW_MS,CommitmentsUnreadable,spend_audit::committed_funding_coin_ids— all new symbols.server.rs:2815, switched here.lifecycle.rsconsumes the pre-computed set and is unchanged.mirror/funding.rsis untouched — it belongs to live lane #469, which rewrites it+507/-19,with hunks landing directly on
committed_funding_coin_ids. A conflict there is near-certain.The predicate belongs to the spend journal rather than to $DIG selection anyway: the old function
read only a
&SpendLogand returned coin ids, with nothing to do with funding. So it lands inspend_audit.rsand the single production call site switches.Residue:
funding::committed_funding_coin_idsis now production-dead, kept alive only by its ownunit tests. Deleting it (and folding its tests into the
spend_auditones) is the routed hand-offfor #469. Flagging it rather than leaving it silent, because a dead rival is exactly what
CLAUDE.md's centralize-rivals rule exists to catch — I am reporting it, not endorsing it.
Evidence
Both halves of the bound are pinned, and both were proved load-bearing by mutation. A test
proving only the release proves half the property and passes a fix that releases everything.
spend_auditunit tests!is_terminal()alone — the defect restored)a_stuck_spend_releases_its_funding_coins_once_the_hold_lapses..._is_selectable_two_passes_later; the in-flight control still passeda_record_dated_in_the_future_keeps_its_holdandthe_window_never_extends_a_hold...The two acceptance tests are integration-level and assert the observable — does the real selector
hand a coin back — not an internal flag:
a_coin_committed_to_a_spend_still_in_flight_is_not_selectable— atWINDOW - 1ms,select_operator_dig_catsrefuses withInsufficient. One millisecond inside the bound, because abound tested only from well within it can only confirm itself.
a_coin_committed_to_a_spend_that_never_lands_is_selectable_two_passes_later— N = 2,asserted from the constants (
WINDOW / MIRROR_ROUND_LENGTH_MS == 2) rather than implied. The coincomes back, AND the ledger still holds one record, still
Submitted, stillmay_have_reached_the_network().Fixture discipline: both probes use the same chain, wallet and audit record, varying only the
observer's clock — a fixture that varied the coins or the statuses could be satisfied by a fix that
released everything. Fixture time is pinned at an explicit
NOWthroughSpendJournal::with_clock;passing a small literal through a wall-clock API would leave every record expired by ~1.8 billion
seconds and assert the release path while never exercising the hold. The operator's entire
holding is committed, so
Insufficientis the observable rather than a coin quietly found elsewhere.A fixture bug was caught and fixed on the first run: both probes derived one temp log path and, run
on parallel threads, appended to one file — which reads as the code under test having written a
record it never wrote.
Test counts are quoted from the runner (
32 passed ... 679 filtered out,2 passed ... 0 filtered out), not from an exit status: a filter matching nothing exits 0 printingrunning 0 tests.cargo fmt --all -- --checkclean.cargo clippy -p dig-node-service --all-targets -- -D warningsclean. A workspace-wide clippy run crashed compiling dependencies (
dig-gossip,dig-download)with
STATUS_STACK_BUFFER_OVERRUN— a local rustc/Windows failure unrelated to this diff; CI buildson Linux.
Version
0.198.0->0.206.0(minor: new public API on a shipped crate).0.199-0.205are taken by openPRs #467/#469/#470/#472/#468/#474.
Cargo.lockrefreshed in the same commit.Docs
SPEC.md§25.4.6 gains a normative clause: the reservation is a bounded hold, the window and itsderivation, and — separately — that expiry MUST NOT change the record.