feat(mirror): the pass runner -- disk and chain observed, reclaims before creates - #414
Conversation
Local evidence, terminal
Revert-proof, run against committed state (file copies, not
|
|
Correctness gate on Confirmed so far, with evidence, so it survives an interruption: 1. The money path is genuinely NOT live — the PR's central claim holds. Grepped the whole crate tree, not the PR body:
So the decision not to construct or schedule a pass in this unit is correct and honestly declared: no coin can be created, reclaimed or broadcast at this head. 2. 3. 4. F6 is fixed. 5. The §25 promotions are narrow and nothing over-promoted. 6. The in-flight defect was real. One gating finding and two non-gating ones are being posted inline now. Verdict follows. |
5ba189c to
80d2d2a
Compare
loop-security — IN PROGRESS, not the verdictAuditing Posting findings as they resolve so nothing is lost to a stall. Four items are settled. 1. The restraint HOLDS — nothing here starts unattended spending (VERIFIED)The load-bearing claim in the brief. Measured, not taken on trust:
So every finding below is LATENT — it becomes live only when a production 2.
|
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED
Head reviewed: 80d2d2aeb9548176531b13a931802cb7a1ce7f50 (resolved from the remote, not from the dispatch brief). Correctness leg, fresh context.
One gating finding. Everything else verified clean, and the central judgement call -- not constructing or scheduling a pass in this unit -- is right and honestly declared.
Ranked findings
| # | severity | where | what |
|---|---|---|---|
| 1 | GATING | mirror/pass.rs:223-225 |
the Reclaiming precedence ignores the epoch, so an ordinary rollover reports a correctly-bonded capsule as money leaving; no fixture varies the epoch term against a bond holding both coins |
| 2 | non-gating | mirror/runner.rs:374-386 |
an unreadable ledger is unknown but resolves toward spending, asymmetrically with the chain's unknown; the third state is not expressible on PassReport |
| 3 | non-gating | SPEC.md 25.8 banner |
still cites dig-node-control-interface 0.25.0 while this PR's vocabulary matches 0.26.0. Outside the diff and pre-existing; sweep it when step 6 adopts 0.26.0. Resolved by me. |
What I verified, and how
The deliberate omission -- verified by grep over crates/, not from the PR body. MirrorEffects has no occurrence outside mirror/runner.rs; dig_mirror_coin::list has zero occurrences anywhere; MirrorSigner::new has zero callers; PassRunner::new is reached only from runner.rs:545, which is #[cfg(test)]. The money path is not live and the PR is not under-declaring what it did. The reasoning is correct on its own terms: constructing a MirrorSigner is the act that starts unattended spending, and 2.6 puts that behind a watched pass, not beside the first test of the runner.
The three section-25 promotions, each checked independently.
- 25.4 steps 4/5/6 --
PassRunner::runatrunner.rs:218,in_flight_createscalled atrunner.rs:230. The order isexecute's: every reclaim attempted first with failures collected, then creates behindif let Some(per_coin), breaking at the first error. Real. - 25.1 Relayed exclusion --
split_by_provenance(runner.rs:324) has one production caller,run(runner.rs:220), which splits its ownobserve_diskresult;PassInputs::heldand::relayedare separate fields thereafter. No caller can substitute a pre-filtered set. TheCapsuleProvenancematch is exhaustive over a non-#[non_exhaustive]enum (dig-node-core/src/lib.rs:4117), so a future third provenance is a compile error rather than a silent promotion into held. Real, and structural rather than a promise. - 25.8 vocabulary --
Disabled,WithheldandReclaimingare emitted from production atpass.rs:216,:233,:241.Withheldhas a genuine producer, the relayed set, which is what discharges the vacuity 412 flagged. Real -- but see finding 1, which is about the precedence, not the vocabulary.
Nothing else was promoted. 25.4's banner still marks the three triggers and the confirmation record pending; 25.1's still marks the biconditional and the observation pending; 25.8's own banner is still PENDING -- not yet implemented. The section list's new read-NARROWLY clause is what makes those consistent rather than contradictory, and it is the right instrument. No born-false claim in this diff.
The head moved mid-review, and I re-ran the affected checks. The brief named ab17b15; the lane rebased and pushed 80d2d2a while I was reading. The delta is CHANGELOG.md plus twelve lines of SPEC.md, and every finding below was re-resolved against 80d2d2a -- pass.rs:223-225 is byte-identical there, and runner.rs is untouched by the rebase.
The self-caught banner correction is accurate, not merely different. It now reads "the deciding and the ORDERING halves exist, and nothing RUNS them -- no pass is constructed, so no observation is ever made and no spend is ever attempted", which is exactly what the grep above shows, and it does not overclaim in the opposite direction: it asserts nothing about observations existing. The added "0.26.0 is NOT adopted at this head and the method is NOT served" is also true of the diff -- crates/dig-node-service/Cargo.toml:92 still declares dig-node-control-interface = "0.24", and control_contract_conformance.rs is not in the diff and cannot have gone red.
The in-flight ledger defect, all four parts.
- Real.
SpendRecordcarried onlystore_id; root and epoch existed solely inside the free-prosepurpose.plan.rs:116-117keys reclaim/create on(store, root, epoch), so store-keyed suppression would withhold a legitimate create for a different root of the same store, and at rollover last epoch's open entry would suppress this epoch's create -- the node stops rolling over. Unimplementable as specified; correctly diagnosed. - The fix supplies the key.
AuditedBond { root, epoch }on bothSpendIntentandSpendRecord, carried throughSpendJournal::begin(spend_audit.rs:787), populated at the one production siteMirrorSpends::intent(spends.rs:136-150), and consumed with all three terms atrunner.rs:390-397. serde(default)does not reintroduce an absent-means-something collapse. Absent yieldsNone, which suppresses nothing, so the pass falls back to the chain observation -- which already excludes any bond holding a current-epoch coin. It fails toward one possible duplicate, never toward a false suppression, and the epoch-overflow path (spends.rs:145) fails the same way. Finding 2 is the one place that direction is argued too broadly.- The wire is untouched.
spend_row(control.rs:3236-3263) maps field by field and does not carrybond; 409's contract-type response is not regressed.
F6. signer.rs:176-191 replaces the bare ? with an explicit arm calling journal.failed(&recorded, FailureStage::Signing, cause) before returning, so Drop's Unresolved is unreachable on a signing failure. The regression test asserts money_may_have_moved() directly and not only the discriminant -- correct, since a future third status with the wrong answer would pass a status-only assertion. The unsignable_for_tests seam (a truncated CLVM cons, cfg(test)) fails inside signature computation, so the test exercises the signing failure and not the ownership refusal in front of it.
BondState blast radius re-derived. Withheld, Disabled and Reclaiming appear only in mirror/pass.rs and mirror/runner.rs across crates/; the enum has no consumer outside the mirror module and reaches no served method. Zero-reference claim confirmed. Reclaiming-outranks-the-chain is the right idea; finding 1 is that it is applied one term too wide.
Ordering and funds-independence. Spot-checked the revert-proof's logic rather than re-running it. reclaims_are_not_gated_on_funds builds its fixture at balance zero with two coins to reclaim, so it is red against a funds-gated implementation and -- correctly -- green under a reversed order, because it pins funds-independence rather than placement. the_reclaim_precedes_the_create_in_the_same_pass uses one recording list, the only shape that can see an interleaving. The two properties do have separate proofs, as claimed.
Fixtures against the nearest wrong implementation. Confirmed as described: the failing reclaim is placed first with an honest coin behind it, so a stop-on-first-failure implementation is red; an_in_flight_create_is_suppressed_across_a_restart builds the runner fresh over the log, which an in-memory ledger would fail while passing a reused-instance test; the three coins at 300/900/1500 admit no partial sum equal to the total; and the 0x prefix and the case are varied independently in the canonicalisation test.
Housekeeping. No unrelated formatting of another lane's files despite cargo fmt --all's workspace reach. dig-app-core is untouched by this diff and does not depend on dig-node-service, so its six failures are pre-existing and not caused here. MIRROR_SPEND_FEE_CEILING_MOJOS did already exist at signer.rs:70 -- not a finding, as the PR says.
Required checks, asserted by name
check-merge-preconditions.sh --repo DIG-Network/dig-node --pr 414 --allow-draft, run unpiped: exit 1, because Test + coverage was IN_PROGRESS. The other four required contexts -- Lint commit messages, Check version increment, Rustfmt, Clippy -- were present and SUCCESS, and unresolvedReviewThreads was 0 before this review. The rebase to 80d2d2a restarts them, so re-run it before any merge; the exit code is the verdict, never the rollup.
Not merging, not undrafting, not editing. Finding 1 goes back to the lane. Findings 2 and 3 are non-gating and I am resolving them myself.
loop-security — IN PROGRESS (2/2), not the verdictRe-resolved to The Rust is byte-identical between the two heads. Only The SPEC delta is a correction in the honest direction and I checked it rather than waving it through: the banner now says the deciding and ordering halves exist with nothing running them, and the 0.26.0 sentence now reads "agreeing with LEAD QUESTION 1 — can the loop double-spend a bond? No permanently. The
|
loop-security VERDICT: CHANGES-REQUIRED (one blocking item)Audited head: Nothing in this PR is a LIVE vulnerability — the restraint holds (see "What is clean"), so no unattended spending exists at this head. I am gating on exactly one item anyway, and I want to be explicit about why, because the brief's rule is "gate on live, ticket the rest". F1 is a defect in this artifact's own headline invariant. F1 — BLOCKING.
|
…ming The predicate matched on (store_id, root) and ignored the epoch, while the Bonded lookup two lines below also required epoch == current_epoch. plan() reclaims every epoch < current coin unconditionally, so on any pass between the new epoch's coin confirming and the old epoch's reclaim confirming, a genuinely collateralised, advertising capsule reported Reclaiming -- the inverse of the false money statement the variant was added to prevent. The precedence is kept, not deleted: it is still correct for the switch-off and NoLongerHeld paths, where the coin that would read Bonded is itself going home. It is now matched by coin_id, so it can only outrank the coin it is about. Also corrects SPEC.md 25.8's banner, which still cited contract 0.25.0; 0.26.0 is published. Co-Authored-By: Claude <noreply@anthropic.com>
… the pass
The wallet balance was read with `?` before the plan, so a wallet that could
be talked to but could not report its $DIG aborted `run()` and made NO reclaim
-- a node unable to advertise AND unable to recover what it had already locked.
That is legacy bug 6 reached through the funds READ rather than the funds gate,
and it inverts the file's own rule 1.
The balance is now `Option<u64>` and reaches only the pricing of creates. `None`
is not zero: an uncovered bond reports the new `FundsUnknown` state rather than
`Unfunded`, which would assert the wallet is short on no evidence and send a
person hunting for $DIG when what is broken is the wallet the node asks.
`BondState::Reclaiming` now carries {coin_id, epoch, amount}, matching `Bonded`,
so the state names the coin it is about instead of asserting a bare fact.
The test double is widened to fail the balance read: every FakeEffects returned
Ok, so `PassError::Wallet` on that call was unreachable from any fixture and
the arm read as covered while never having run.
Co-Authored-By: Claude <noreply@anthropic.com>
loop-security re-audit — IN PROGRESS, not the verdictAudited head: F1 — CLEARED. The invariant moved, not just the code.1. No remaining path where an unreadable balance prevents a reclaim attempt. The independence is
2.
That is the money-lie removal being enforced by the type, not by a convention. 3. The The assertion discriminates: F2 — precedence preserved, and the money figure has ONE source
On the two-amount-sources question — confirmed, there is only one source. Every per-coin amount One consequence worth carrying to step 6, stated so nobody re-derives it: the row payloads are Still to check: the |
loop-security re-audit — PASSAudited head: Scope: the delta Both of my round-1 findings are resolved. No gating security defect in this delta. F1 — RESOLVED. The invariant moved, not just the code.1. No remaining path where an unreadable balance prevents a reclaim attemptThe independence is now structural at three layers, not incidental at one:
Legacy bug 6 is now closed through the funds read as well as through the funds gate. 2.
|
1 participant
DO NOT MERGE — gate round in progress. Custody-adjacent; this takes the full triple gate.
Step 4's impure half of #412 — the mirror-coin pass
runner (
SPEC.md§25.4). The pure planner, presence tracker, spend builders and scoped signer landedin #379 (
fba53ec); this adds the half that observes, orders and executes.Closes #412
What the runner is, and what it deliberately is not
crates/dig-node-service/src/mirror/runner.rs. It owns steps 1, 2, 4, 5 and 6: it observes disk andchain, derives the in-flight set from the audit record, asks
pass::decidewhat to do, then does it —every reclaim first, creates second, stopping cleanly.
Everything that can fail sits behind one trait,
MirrorEffects. What that trait deliberately doesnot hold is the ORDER, the funds gating, or the in-flight suppression — those are ordinary code
here, over a trait a test implements in twenty lines. Each of the three rules is a property of the
SEQUENCE rather than of any single effect, so a test that could only observe effects individually
could not see any of them.
Not in this diff, and stated plainly rather than implied: nothing constructs a pass and nothing
schedules one.
MirrorEffectshas no production implementation, sodig_mirror_coin::list,MirrorSigner::new,build_createandbuild_reclaimstill have zero callers, and no coin iscreated, reclaimed or broadcast at this head. Constructing a
MirrorSignerat bring-up is the actthat starts unattended spending, and doing it in the same diff that first tests the runner would ship
the money path before anyone has watched a single pass run. That is the next child, and #412's step 8
is its acceptance.
Blast radius checked
gitnexus fell back to grep + direct read (§2.0 bound 2, stated as required): this worktree had no
index and a fresh
analyzewas not worth the ~10-minute stop for a radius this small.BondState(renamed + 2 new variants)mirror/pass.rs— the enum has no consumer yetSpendIntent/SpendRecord(+ bond)MirrorSpends::intentMirrorSigner::signsigner.rsare the only callerspass::decide/PassInputs(+ relayed)PassRunner::runspends_list_wire/spend_rowmapSpendRecordinto the contract'sAutomatedSpendfield by field,so the added field does not reach the
control.spends.listwire.#[serde(default)]means anexisting audit line still parses.
The five items #412 carried
1. F6 — the signing-failure status.
signer.rsused a bare?onWalletSigner::sign, sorecordeddropped inside the frame andDropwroteUnresolved— "the node signed; money may wellhave moved". No bundle exists when signing fails. Now
Failed { stage: Signing }, the only statuswhose
money_may_have_moved()is false. Regression test first, and it needed a newunsignable_for_testsseam (a truncated CLVM cons) because an empty spend set always signs.2.
BondState::Withheld→Disabled, plus newWithheldandReclaiming— matchingdig-node-control-interface 0.26.0. Three states differing in scope and decisively in remedy: an
operator told "withheld" about a disabled node goes looking at content when they should be looking
for a switch.
Reclaimingoutranks the chain inbond_states, because the coin is still there — thatis what a reclaim is for — and
Bondedwould say the collateral is advertising a capsule whose moneyis on its way home.
3.
Relayedfiltered AT THE SOURCE.split_by_provenancesplits the observation byCapsuleProvenanceat the point it is made, andPassInputs::held/::relayedare separate fieldsthereafter — so the create path is structurally unable to see a relayed capsule and no caller can
substitute a pre-filtered set. This is the one place an attacker influences what the node spends its
own money on. It also gives
withhelda real producer: aHeld-keyed derivation could never emitit, which was the vacuously-satisfied clause #412 flagged.
4. Two
SPEC.mddoc corrections. §25.2's banner said "two structural halves" and listed three, onebeing a parameter the same commit removed — now three, correctly named. §25's allowlist tail reached
clauses the subsection banners call pending — now read narrowly, entry by entry.
5.
MIRROR_SPEND_FEE_CEILING_MOJOS— already exists. #379 shipped it atsigner.rs:70withboth-sided coverage. The brief listed it as owed; it is not. Nothing to do, recorded so nobody re-does it.
From the coordinator's 0.26.0 note
provenance_unknown. The relayed set travels through toPassInputs::relayedandbond_statesemits aWithheldrow for each, so a page can be complete.PassReport::locked_dig_base_unitsis the WHOLE-SET total, including reclaiming coins —summed here, where the entire chain observation is in hand, precisely so §25.8 cannot compute it by
summing a page. A page sum under-reports locked money, showing unspendable funds as available.
canonical), so this producer's order agreeswith a client's
MirrorBondKeyderive rather than paging out of step with it.0.26.0 is NOT adopted in this PR. Adopting it reddens
control_contract_conformance.rsuntil themethod is served, which is step 6 — a different unit of work. This PR leaves that test green.
Tests — and the proof they are load-bearing
61 mirror tests, 12 of them new in
runner.rs. What each new one pins:reclaims_are_not_gated_on_fundsthe_reclaim_precedes_the_create_in_the_same_passa_failed_reclaim_does_not_stop_the_next_onea_shortfall_creates_a_deterministic_prefix_and_is_not_an_errorstopped_atstaysNonea_failed_create_stops_the_pass_...an_in_flight_create_is_suppressed_across_a_restarta_resolved_record_no_longer_suppresses_the_createan_open_record_for_a_previous_epoch/..._another_root_of_the_same_storea_requirement_unknown_defers_creates_but_not_reclaimsthe_switch_off_reclaims_everything_and_creates_nothinga_relayed_capsule_never_reaches_the_create_pathHeldbond alongside, so silent-drop reports one state instead of twobond_ids_are_canonicalised_before_anything_orders_them0xvaried independently, ordered so raw strings sort differentlythe_locked_total_includes_coins_that_are_being_reclaimedan_unreadable_chain_makes_the_pass_spend_nothingRevert-proof, committed first. Reverting only the F6 fix, and separately reversing only the
reclaim/create order, fails exactly two tests and only those two — 59 passed, 2 failed. Notably
reclaims_are_not_gated_on_fundsstayed green under the reversed order, which is correct: it pinsfunds-independence, not placement. The two properties have separate proofs.
Gates
cargo test -p dig-node-service— 587 lib tests green, mirror suite 61/61cargo clippy -p dig-node-service --all-targets -- -D warnings— cleancargo fmt --all— clean; it touched only the two files in this diff0.169.0→0.170.0(minor: new capability, additive field, no wire change)Six failures seen in an earlier accidental workspace-wide run are in
dig-app-core(
wallet::node::*cache tests and a copy space-run check) — a different crate with no dependency ondig-node-service, untouched by this diff, and pre-existing.What remains for #412
Constructing and scheduling a pass (a
MirrorEffectsover the node's chain transport, operatorwallet and capsule cache; the start-up run, the
MIRROR_ROUND_LENGTH_MStick, the debounced presencetrigger), then step 6 (serve
control.mirror.bondStates, adopt 0.26.0, add thedignverb),step 7 (the DHT pointer) and step 8 (the real-machine proof, which is the epic's acceptance).
Co-Authored-By: Claude noreply@anthropic.com