Skip to content

fix(wallet): report honest sync state for coin_by_id reads - #227

Merged
MichaelTaylor3d merged 3 commits into
mainfrom
loop/2938-coin-by-id-honest-state
Aug 16, 2026
Merged

fix(wallet): report honest sync state for coin_by_id reads#227
MichaelTaylor3d merged 3 commits into
mainfrom
loop/2938-coin-by-id-honest-state

Conversation

@MichaelTaylor3d

@MichaelTaylor3dMichaelTaylor3d commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

What changed

coin_record-by-id was routed to the chain tier unconditionally and reported source: fallback, synced: false, peak_height: nulleven for a coin the node's own replica held. A freshness warrant no read can ever carry is not strictness — it turns every consumer-side guard built on it into an unconditional refusal. That is how a guarded mint poll came to end in "the chain could not be reached" on a healthy node (dig-account's UNREACHABLE_LOOKS_BEFORE_GIVING_UP = 12 ends the watch as MintOutcome::ConnectionLost).

The tier fields now describe what answered this read:

  • the replica HOLDS the coin and is authoritative → source: db, the replica's own peak_height, and syncedmeasured rather than assumed;
  • anything else → the chain tier answers and says so, unchanged.

Why this shape and not a stricter one

It reuses the instruments the money reads already use, at their own spelling — no new predicate:

instrumentrpc.rsalready used by
replica_is_authoritative (initial_sync_complete && covered.covers(followed_set()), the #2871 coverage machinery):803balance :1173, coins :1320
replica_answer_is_current (measured against the peers' announced peak):902balance :1237, coins :1368

coin_by_id (:1522/:1543) now pairs exactly those two. The eligibility test is the same one the money reads use rather than a second spelling of it, so the replica cannot be trusted here at a moment it is distrusted there.

Followed is not known, and absent is not unknown. A replica MISS still falls through to the chain tier and is never served as an absence — the replica is populated only from this node's own subscriptions, so a miss means "this node does not watch that coin". replica_coin_by_id returns the same None for not authoritative and for holds no such coin, deliberately, so no future caller can come to read either as proven absence.

Failure direction. The wrong answer this ordering can produce is fallback for a coin the replica could have served — a read that is merely more expensive. The opposite mistake would report an unwatched coin as proven absent.

The local arm runs ahead of the liveness check and the rate limiter, matching the balance/coins fast path SPEC §18.7b already documents: neither guards an egress this arm opens, and a replica hit makes no outbound call at all.

How verified

  • 3 new tests in crates/dig-wallet/src/sage/rpc.rs. The two tiers report different amounts for the same coin id, so the assertions prove the read actually MOVED rather than that its labels changed; call_count() == 0 pins that a local answer discloses nothing to the oracle.
  • Control run (non-vacuity proven): with the replica arm disabled, a_coin_the_replica_holds_… and a_behind_replica_serves_… both FAIL (left: Fallback, right: Db). With it, all 3 pass.
  • cargo test -p dig-wallet --lib555 passed, 0 failed.
  • The pinning test the_by_id_read_never_consults_the_local_replica is deliberately replaced — dig_ecosystem#2938 is its named intended cause of death.
  • Wire boundary checked: coin_by_id_wire (control.rs:2227) passes source/synced/peak_height through verbatim, so the fix reaches consumers.

Blast radius

WalletBackend::coin_by_idcontrol.wallet.coinById (control.rs:1618) → dign wallet coin-by-id. coin_spend is deliberately NOT changed: the replica stores coin records, not spends, so it can never produce that answer — SPEC now says so explicitly instead of citing the old always-fallback reason.

SPEC.md updated in the same unit of work.

Closes DIG-Network/dig_ecosystem#2938

MichaelTaylor3dand others added 3 commits August 15, 2026 05:58
Co-Authored-By: Claude <noreply@anthropic.com>
`coin_record`-by-id was routed to the chain tier unconditionally and reported
`source: fallback, synced: false, peak_height: null` even for a coin the node's
own replica held. A warrant no read can ever carry is not strictness — it turns
every consumer-side freshness guard into an unconditional refusal, which is how
a guarded mint poll came to end in "the chain could not be reached" on a healthy
node (dig_ecosystem#2938).
The tier fields now describe what answered THIS read, using the same eligibility
and freshness instruments the balance and coin-list reads already use:
`replica_is_authoritative` (the #2871 coverage machinery) decides whether the
replica may answer, and `replica_answer_is_current` measures whether what it
answered is at the tip. A replica HIT is served locally and says so; a replica
MISS still falls through to the chain tier and is never served as an absence,
because the replica is populated only from this node's own subscriptions.
Closes dig_ecosystem#2938
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Correctness gate — in progress (head 5721bd9f22ab65d9a6b99958c9ab0b662a772619)

Interim note so the audit is durable. Findings so far, all non-blocking:

The primary question — does the new warrant match the established siblings? — answers YES.
replica_coin_by_id (crates/dig-wallet/src/sage/rpc.rs:1516) calls replica_is_authoritative
(:803) and replica_answer_is_current (:902) at their own spelling, which is exactly what
balance_for_address does at :1172 and :1237. No new predicate, no stricter one. The
address-scoped scoped test (:1179) is correctly NOT reproduced here — a by-id read has no
address, and "the replica holds this coin" is the stronger scoping fact.

Failure direction is the survivable one. The worst answer this ordering can produce is
fallback for a coin the replica could have served — more expensive, not untrue. A replica MISS
falls through (:1475) and is never served as absence.

Spelling checked, not assumed.normalize_hex_id (:4492) reduces to bare lowercase; the
coins table is written with hex::encode at sync.rs:441, so the lookup key matches and a held
coin cannot read as a miss.

SPEC swept for the superseded rule. The old "always fallback / always false / always null"
phrasing survives only on control.wallet.coinSpend (SPEC.md:1511), where it is still true and now
carries a correct reason (the replica stores records, not spends). §18.7b (SPEC.md:4365) is generic
and needed no change. §10's WALLET_RATE_LIMITED row (SPEC.md:2856) already said the local fast
path is never gated, which the new arm ordering now makes true for this read too.

Still outstanding: an independent revert probe of the three new tests, and Test + coverage (pending
at this head). Verdict follows.

@MichaelTaylor3dMichaelTaylor3d left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

CORRECTNESS GATE: PASS

Head reviewed: 5721bd9f22ab65d9a6b99958c9ab0b662a772619 (resolved from the remote myself; the head did not move during the review).

The gate this PR most needed to survive is the over-strictness class - three wrong locked shapes in one session were each a stricter contract invented without checking the established one. This change is NOT in that class. It reuses the siblings at their own spelling, and its wrong-answer direction is the survivable one.

Primary question: does the new warrant match replica_is_authoritative / the #2871 coverage machinery?

Yes - verified by reading both sides, not the PR body.

instrumentnew armestablished money readsame?
eligibilityrpc.rs:1521replica_is_authoritative()rpc.rs:1172 (balance), :1320 (coins)identical call, not re-spelled
freshnessrpc.rs:1544replica_answer_is_current(peak_height)rpc.rs:1237identical
peakrpc.rs:1541db.sync_state().peak_heightsameidentical

replica_is_authoritative (:803) is initial_sync_complete && covered.covers(followed_set()) - the #2871 containment test, consumed unchanged. The replica cannot be trusted here at a moment it is distrusted there.

The one deliberate difference is CORRECT. balance_for_address additionally requires scoped (:1179, derivation_exists || watchlist_follows). The by-id read does not reproduce it, and must not: there is no address to scope, and "the replica actually holds this coin id" is a strictly stronger fact than "the replica follows this address" - the row can only be there because a subscription put it there. Looser in form, equal-or-stronger in substance, which is the right direction.

Failure direction interrogated: the worst answer this ordering produces is fallback for a coin the replica could have served - merely more expensive. The inverse mistake (a MISS served as proven absence) is structurally prevented - replica_coin_by_id returns Ok(None) for both "not authoritative" and "holds no such coin", and coin_by_id:1475 falls through on either. Option<WalletCoinByIdResult> cannot express "absent", so no future caller can misread it.

Honesty split, checked against the BalanceReading shape

Three distinct states, none a value dressed as a fact:

  • replica hit, level with peers: db / synced: true / real peak
  • replica hit, behind OR peer tier unobservable: db / synced: false / real peak STILL SERVED (:1544, via the unobservable-answers-false narrowing at :902) - stale, labelled, still usable
  • miss or not authoritative: fallback / false / null, and a genuine outage is still WALLET_NO_CHAIN_SOURCE (:1478), never a null coin

The peak-is-None-while-authoritative case yields db / false / null: honest - unknown height, and source: db still names the tier.

Test-vacuity gate: anchors PROVEN independently, not accepted

I did not take the PR body control run on trust. In my own worktree at this head (C:\tmp\worktrees\gate-2938, since removed; the shared checkout was never written to):

Probe 1 - delete the replica arm (rpc.rs:1475), change nothing else:

a_coin_the_replica_holds_is_answered_from_the_replica_and_says_so FAILED left: 7 right: 100
a_behind_replica_serves_the_coin_it_holds_and_says_it_is_not_current FAILED left: Fallback right: Db

The first fails on the AMOUNT, not on a flag. That is the property, not the outcome: REPLICA_AMOUNT=100 vs ORACLE_AMOUNT=7 for the same coin id means the assertion proves the read moved TIERS, so a fix that set the labels without moving the read cannot pass it. fb.call_count() == 0 additionally pins that no coin id reached the oracle.

Probe 2 - delete only the eligibility check (rpc.rs:1521), arm restored:

a_coin_held_by_a_non_authoritative_replica_is_not_served_from_it FAILED left: 100 right: 7

So the third test is anchored to the #2871 gate specifically and is not a passenger - it discriminates "is the coin in the DB" from "may the DB answer", the exact wrong implementation this shape invites.

a_coin_the_replica_does_not_hold_still_falls_through_to_the_chain correctly survives probe 1 (it asserts the unchanged fallback leg) and is the control that stops the probe-1 tests being satisfied by "if authoritative, answer from the DB".

Deleting the_by_id_read_never_consults_the_local_replica is right rather than a coverage loss: it pinned the behaviour this ticket names as defective, and its replacements cover the trap it existed to prevent (miss-as-absence) more precisely than it did.

Coherence

  • SPEC.md:1510 describes what the code now does, in MUST voice, including both the "MUST NOT report fallback/false for a coin it holds" rule and the miss-falls-through rule.
  • Superseded phrasing SWEPT, not spot-checked. The old "always fallback / always false / always null" survives only at SPEC.md:1511 (coinSpend), where it is still TRUE and now carries a correct reason (the replica stores coin records, not spends) instead of citing the removed always-fallback rule. Section 18.7b (SPEC.md:4365) is written generically over "every wallet read that chooses a source" and needed no edit. Section 10 WALLET_RATE_LIMITED (SPEC.md:2856) already asserted "the cheap local-DB fast path is never gated" - the new arm ordering at :1471 makes that sentence true for this read for the first time.
  • Wire boundary passes the three fields through verbatim (control.rs:2227), so the fix reaches dign wallet coin-by-id.
  • Key spelling checked rather than assumed: normalize_hex_id (:4492) reduces to bare lowercase and sync.rs:441 writes hex::encode, so a held coin cannot read as a miss through a 0x/case mismatch.

dig-constants check (both directions)

Nothing here belongs in dig-constants, and nothing here should be consuming it. The diff introduces no value a second repo must match - REPLICA_AMOUNT/ORACLE_AMOUNT are test fixtures deliberately unequal, and the tier vocabulary (db/fallback) is already owned by Source::as_wire() and the dig-node-control-interface contract, which this PR does not touch.

Beautiful-code

Reads cleanly. replica_coin_by_id is one screen, guard-clause shaped, and its doc-comment explains WHY None is deliberately ambiguous - the non-obvious decision, which is what a WHY-comment is for. The peak is read inside the arm with a comment saying why (:1539). normalize_hex_id as a one-line alias of normalize_ph is defensible: the name carries the intent (an id, not a puzzle hash) and the doc records the coupling to hex::encode.

Non-gating observations - LOGGED, not blocking (CLAUDE.md 2.6, end-to-end-first)

Ticket candidates, deliberately not held against this PR:

  1. A local DB error now fails a read the chain could have served. replica_coin_by_id:1519 maps any sqlx::Error to BalanceError::ReadFailed, which the ? at coin_by_id:1475 makes terminal before the fallback leg is tried. Honest (an error, never a false null), and it matches what balance_for_address:1172 already does, so not a divergence introduced here - but it is the who-failed / what-happens-next shape: a blameless local failure routed to terminal instead of retry-with-another-source.
  2. A mempool-only replica row is now servable by this read (coins_by_ids does not filter on created_height), so a db answer can carry created_height: null. Honest, and consistent with the documented mempool inclusion on control.wallet.coins, but SPEC.md:1510 does not say so for coinById.
  3. Stale spend state on a behind replica: a replica hit that is behind can report spent_height: null for an already-spent coin, where the old always-fallback path would have seen the spend. Labelled synced: false with a real peak, so not a lie - but a consumer that ignores synced reads it as unspent. Worth a line of caller guidance.

Verdict

PASS. No gating findings. Nothing was handed to Copilot - there is nothing to fix.

Merge remains blocked on Test + coverage, still running at this head throughout the review. The PR is correctly still a DRAFT and I have not undrafted it.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

CI reached terminal state after the review above

Test + coverage finished SUCCESS (18m56s, run 31917058710). All five required contexts are now green, asserted BY NAME from branch protection rather than by a rollup glance:

Lint commit messages SUCCESS
Check version increment SUCCESS
Rustfmt SUCCESS
Clippy SUCCESS
Test + coverage SUCCESS

unresolvedReviewThreads=0, mergeStateStatus=CLEAN. check-merge-preconditions.sh still exits non-zero for ONE remaining reason: draft=true, which is correct - the gate round has now returned, so the orchestrator may undraft and squash-merge. I have deliberately not undrafted it myself.

Verdict unchanged: PASS at 5721bd9f22ab65d9a6b99958c9ab0b662a772619.

@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 16, 2026 00:49
@MichaelTaylor3d
MichaelTaylor3d merged commit 7ddef6e into mainAug 16, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/2938-coin-by-id-honest-state branch August 16, 2026 00:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@MichaelTaylor3d