fix(wallet): a key enrolled after catch-up no longer reads an empty replica

[MichaelTaylor3d]

DO NOT MERGE — gate round in progress.

Closes dig_ecosystem#2871.

What changed, and why the shape changed

The previous round fixed the symptom by clearing initial_sync_complete at the enrolment boundary. Two gating findings showed that shape cannot be made correct, because it requires two mutations to land in the right order across a process boundary:

• F1 — a catch-up already in flight over {K1} calls complete_catch_up unconditionally when the peer answers is_finished, re-latching the flag over a set that never contained K2. A first catch-up replays from genesis over many batches, so an enrolment lands squarely inside that window.
• F2 — registry.watch persists BEFORE the invalidation runs. If that second write fails (or the process dies), the key is enrolled with the flag still true, and the client's retry reports added = 0, so an added > 0 guard never fires. Unrecoverable by retry.

So the flag stopped being maintained and the write became self-describing: a completed sync records the puzzle-hash SET it ran over, and routing asks about coverage rather than about a global flag.

The predicate, at both money read sites (balance_for_address, coins_for_address, via the single WalletBackend::replica_is_authoritative):

initial_sync_complete && recorded_covered_set ⊇ currently_followed_set

currently_followed_set comes from sync_supervisor::followed_puzzle_hashes — the SAME union (custody ∪ watch registry) the supervisor subscribes, now one definition consumed by both sides, so the router and the subscriber cannot disagree about which addresses are followed.

Why containment, not the equality-on-a-fingerprint the brief specified

Equality invalidates on NARROWING too: control.wallet.unwatch would stop matching and force a needless full resync, sending every read to the oracle for its duration — a self-inflicted outage on a correct operation. A sync over the wider set genuinely covers what remains, so the question has to be containment. It is stored as the canonical set (CoveredSet, comma-joined lowercase hex) rather than a hash, which is what makes containment expressible. This is the only deviation from the decided shape; everything else follows it.

F1 and F2 are now impossible by construction

• F1 — CatchUpReplay CARRIES the covered set (built from the subscription's own puzzle_hashes vector), and complete_catch_up writes it in the SAME transaction as peak_height / header_hash / initial_sync_complete. A completion cannot describe addresses its own subscription did not contain, whenever it happens to land. Ordering is irrelevant: the late writer records {K1}, the followed set is {K1,K2}, containment fails.
• F2 — there is no second write. watch_keys registers and returns; the clear, the if added > 0 guard and the widening detection are deleted, and it is no longer async or fallible (the WalletReadFailed branch in control.rs is gone with them). There is nothing left to fail between the two mutations because there is one mutation.

Also in this round

• F4 (from the security gate) — WatchRegistry::watch is now pub(crate), so "the single door onto enrolment" is a compiler guarantee rather than a convention. This replaces the control-plane door test the brief asked for: the narrowing makes re-pointing the handler at the registry fail to compile, which is strictly stronger than a test.
• rpc.rs:688 — the doc claiming reads fall back "until the next catch-up completes over the widened set" is gone; F1 disproved it and the restructure makes it obsolete.
• refresh_tracked_coins (variant 1b) — watchlist_is_covered_by and its guard are KEPT unchanged (the gate verified both), and the path now RECORDS the set it fetched alongside latching the flag. Without that it would latch a flag with stale or absent coverage and buy itself nothing, since routing asks about coverage.
• SPEC §18.6f — new invariant: the replica may answer only for addresses a sync actually covered; containment not equality; coverage may not be inferred from a second ordered write.

Schema

Additive: sync_state.covered_puzzle_hashes TEXT, added by CREATE TABLE for fresh DBs and by an idempotent ALTER TABLE … ADD COLUMN for existing ones (§5.1). An existing replica's column arrives NULL, which reads as covers nothing — fail closed: reads fall to the chain oracle, which answers truthfully, until the next sync records a set. initial_sync_complete's own meaning is unchanged; its other consumers (the phase logic, await_puzzle_hashes, the arrival baseline) read exactly what they read before.

The lever this does NOT remove — read this

control.wallet.watch remains an unbounded lever that can hold the node in permanent oracle fallback, and the fingerprint/coverage scheme inherits it: enrolling junk keys widens the followed set, so coverage fails and every read falls back just the same. A holder of a PAIRED token can call it (is_pairing_admin_method excludes it); there is no control-plane rate limit and no bound on key count or registry size. Bounded in the right direction — fallback_rate caps egress and returns RateLimited rather than a fabricated figure, so the worst case is a read DoS, never a wrong number. Filed as dig_ecosystem#2877; deliberately not fixed here.

Tests — what each catches

Test	Wrong implementation it catches
a_key_enrolled_after_the_catch_up_is_not_answered_from_the_replica	routing on the bare flag (the shipped defect). Order is the fixture: catch-up completes first, K2 enrols after.
a_catch_up_in_flight_cannot_vouch_for_a_key_enrolled_while_it_ran (new, F1)	any flag-based fix, including "clear again after the catch-up returns". The completion is the LAST writer; the reverse order passes against that wrong version and proves nothing.
a_repeated_enrolment_that_adds_nothing_still_leaves_the_new_key_uncovered (new, F2)	invalidation gated on added > 0 / on a second write landing. The second watch returns added = 0 and IS the client's retry.
re_announcing_a_known_key_leaves_the_replica_authoritative (kept, now asserts ROUTING)	clearing on every watch — which strands a healthy node in permanent fallback while dig-app re-announces on every unlock.
deregistering_a_key_leaves_the_remaining_ones_covered (new)	equality on a whole-set fingerprint, under which unwatch forces a needless resync.
a_custody_only_refresh_does_not_vouch_for_enrolled_addresses + its control (kept, 1b)	a point-read refresh latching for addresses it never fetched.
coverage::tests ×5	order/spelling sensitivity, a separator-free concatenation ({aa,bb} vs {aabb}), equality-instead-of-containment, and a lossy storage round-trip.

Red proven by mutation, not assumed (committed first, reverted by file copy):

• replica_is_authoritative → self.db.is_synced() (the defect): exactly the three F1/F2/enrolment tests fail; every control passes.
• CoveredSet::covers → == (the equality shape): a_narrowed_followed_set_stays_covered and deregistering_a_key_leaves_the_remaining_ones_covered fail, plus five superset fixtures.

Evidence

• cargo test -p dig-wallet --lib — 532 passed, 0 failed.
• cargo test --workspace — all suites green (dig-node-service 815, dig-node-core 351, …), 0 failed.
• cargo clippy --workspace --all-targets clean; cargo fmt --all applied.

Blast radius checked

gitnexus was NOT used: per CLAUDE.md §2.0 bound (2), blast radius was established by exhaustive rg over every symbol touched plus a full-workspace cargo check --all-targets, which for Rust closes the call graph mechanically — every caller of a changed signature is a compile error, and there were none left. Symbols changed and their full caller sets:

• CatchUpReplay::finished_at (+covered) — 1 production caller (sync::initial_sync_with_authority), 20 test call sites.
• WalletDb::complete_catch_up / sync_state / SyncState — signature unchanged; new field/column.
• WalletBackend::watch_keys (async→sync, Result<Option<_>>→Option<_>) — 1 production caller (control.rs::wallet_watch), 4 test call sites.
• WatchRegistry::watch (pub→pub(crate)) — no out-of-crate callers existed.
• balance_for_address / coins_for_address — bodies only; wire shape unchanged.
• UnionPuzzleHashSource::puzzle_hashes — now delegates to followed_puzzle_hashes; behaviour identical (same BTreeSet union, same order).

No HIGH/CRITICAL-risk symbol was edited blind, and no wire/RPC contract changed: control.wallet.watch returns the same {added, watched} shape.

Version

dig-wallet0.21.1 → 0.22.0 — minor, because WatchRegistry::watch narrowed to pub(crate) and watch_keys changed signature; both are breaking for a 0.x library crate. Workspace (the released dig-node binary) stays 0.117.1: a behaviour fix with no user-visible API change, already ahead of the latest tag.

[MichaelTaylor3d]

Gate-head equivalence, verified by the orchestrator

The correctness and security gates were dispatched against 4b5c3bebb0c28626be9f0e6c0af9622821951fab. Head then moved to a11c13c for the rustfmt fix. I verified the delta myself rather than taking the lane's word, because a push after a gate normally voids its verdict (§2.4a):

crates/dig-wallet/src/sage/rpc.rs | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

The entire change is a three-line rewrap of address_of — a test helper inside mod tests. No production line, no assertion, no fixture value. So the gate verdicts taken against 4b5c3beb's logic apply unchanged to a11c13c, and I will merge on them rather than paying for a second round.

Recorded because "the delta was only formatting" is exactly the claim that should never be accepted from the party that made it.

[MichaelTaylor3d]

CHANGES-REQUIRED - reviewed at 4b5c3bebb0c28626be9f0e6c0af9622821951fab.

The fix is the right shape and closes the sequence the ticket describes. But the invariant as stated - a Source::Db answer may only be produced when the completed catch-up actually covered the queried address - is not held on every path. Two routes still reach the exact money lie #2871 exists to stop, both with no attacker and no operator configuration, and both ending with initial_sync_complete = true over a set the catch-up never covered.

GATING

1. crates/dig-wallet/src/sage/sync.rs:992 - a catch-up already in flight latches unconditionally over the set it was handed at session start, overwriting the clear watch_keys just made. Enrol during a catch-up and the shipped defect is unfixed.
2. crates/dig-wallet/src/sage/rpc.rs:709-712 - the registry write (persisted) is committed BEFORE the invalidation, and watch is idempotent, so a failed or interrupted invalidation can never be retried: the retry adds 0 and never clears.

NON-GATING: nothing pins that control.wallet.watch routes through watch_keys; watchlist() still exposes WatchRegistry::watch publicly; the new watch_keys doc asserts a property finding 1 disproves.

Checked and found CORRECT: watchlist_is_covered_by normalises identically to watchlist_follows (both normalize_ph(hex::encode(puzzle_hash_for(pk)))), with no always-true path beyond the correct empty-registry case, and refresh_tracked_coins passes lowercase hex so the comparison is real; no lock is held across an await in watch_keys (the RwLock guard is scoped inside WatchRegistry::watch); watch is additive-only so a membership change without growth cannot arise, and unwatch correctly needs no invalidation because narrowing is safe; the fixture in a_key_enrolled_after_the_catch_up_is_not_answered_from_the_replica genuinely latches BEFORE enrolling K2 and would be vacuous reversed; the 1b test exercises refresh_tracked_coins itself and carries a real control; registry.watch has exactly one production caller.

Both gating findings are money-path concurrency and write-ordering on the very defect the stopped release is waiting on - not handed to Copilot; they need an implementer holding the threat model.

[MichaelTaylor3d]

Gate returned CHANGES-REQUIRED. Reversing the shape I decided on the ticket.

Both gating findings share one root cause: initial_sync_complete is a bare bool that records nothing about which addresses the completed catch-up covered. Every fix that keeps it a bool has to hold two mutations in the right order across a process boundary — which is exactly why there are two holes and not one:

• F1 the in-flight catch-up re-latches over its stale set, overwriting the clear;
• F2 the widening commits first and watch is idempotent, so a failed clear can never be retried.

Ordering was never going to hold this. So the fix stops maintaining the flag and makes the write self-describing: complete_catch_up records a fingerprint of the puzzle-hash set it actually ran over, inside its existing transaction, and routing asks "does the last completed catch-up cover the current followed set?" rather than "is a global flag true?".

Both findings then become impossible by construction:

• F1 — an in-flight catch-up over {K1} records the fingerprint of {K1}; the current set is {K1,K2}; they differ, so nothing is covered until a catch-up genuinely runs over {K1,K2}. The write can no longer claim a set it did not cover.
• F2 — watch_keys stops clearing anything at all. There is no second mutation to order, no added > 0 guard, no failed-write window. The finding has nothing left to describe.

It also deletes code rather than adding a guard, which is the better sign.

What I kept from the previous round, because the gate verified it independently: watchlist_is_covered_by and its variant-1b guard (normalisation matches watchlist_follows exactly, no always-true path), and the three existing tests including re_announcing_a_known_key_leaves_the_replica_authoritative — dig-app re-announces its whole account on every unlock, so "just clear on every watch" would strand a healthy node in permanent fallback. That test is what forbids the easy wrong answer.

Accepted cost is unchanged from the original decision: after enrolling a key, reads fall to the oracle until the next catch-up. The oracle answers truthfully. Per-address coverage — so enrolling K2 need not also blind K1 — is filed as a follow-up rather than built here.

The release stays stopped until this lands.

[MichaelTaylor3d]

Security gate: PASS — but it found the same two defects, rated differently. Here is why I am not merging on it.

Two independent gates, fresh contexts, reached opposite gating verdicts on the identical two defects. That disagreement is worth recording rather than quietly resolving.

The security gate's argument for non-gating is genuinely strong: this PR is "a strict narrowing of a defect that is on main today with no precondition — no token, no race, permanent, and persisted across restart. Blocking this PR leaves that shipping-blocked and unfixed."

It rests on a premise that no longer holds. It assumes main reaches users, so shipping a partial fix beats shipping none. I stopped the release — nightly-release.yml is disabled_manually and v0.117.0 is untagged — so there is no user exposure to trade against, and the comparison is between a partial fix and a complete one, not between a partial fix and nothing.

Two further reasons the correctness gate's GATING call is the one to follow:

1. F2 is not a race. It is a process death or a failed DB write between two statements — disk full, corruption, a crash. No timing window and no attacker required, and the retry path actively reports success while leaving the money lie in place. Rating it by the improbability of F1's 54 ms window does not cover it.
2. The complete fix is smaller than the partial one. The restructure deletes watch_keys's clear, the added > 0 guard and the widening detection, and makes both findings unexpressible rather than unlikely. There is no schedule argument for shipping the weaker version.

What the security gate cleared, recorded so it is not re-derived

is_open_control_read byte-identical to base and control.wallet.watch never in it; both transports still token-gated after the async refactor; no lock held across an await; added cannot under-count a widening (canonical G1 encoding via PublicKey::from_bytes); watchlist_is_covered_by always-true only when no enrolled address exists to lie about; Cargo.lock delta is exactly the two version lines; new tracing emits no key, address, or balance.

Most useful for the next reader: upsert_derivation has no production caller, so on a shipped node scoped ⟺ watchlist_follows, and WatchRegistry::watch is the only production widening of the followed set.

Two new findings, forwarded to the lane

• F4 (fixing here):WatchRegistry::watch is pub and watchlist() hands out &WatchRegistry, so the single door is a convention, not a guarantee. Narrowing to pub(crate) makes it structural.
• F3 (follow-up, not fixed here):watch_keys is a lever that forces every read onto the third-party oracle, and it survives the restructure — enrolling junk keys changes the followed-set fingerprint, so coverage fails and every address falls back regardless. A paired token can loop it. Bounded in the right direction (fallback_rate returns RateLimited, never a fabricated figure), so it is a read DoS, never a wrong number. The PR body will state that the fingerprint scheme inherits it.

[MichaelTaylor3d]

Both GATING findings are addressed by removing the shape that caused them, not by re-ordering it.

F1 (sync.rs:992, the in-flight catch-up): CatchUpReplay now carries the puzzle-hash set the catch-up subscribed, and complete_catch_up writes it in the same transaction as the flag. A late completion records {K1} while the followed set is {K1,K2}, so containment fails and nothing is treated as covered. Ordering no longer matters, because the write cannot describe a set the subscription did not contain. Pinned by a_catch_up_in_flight_cannot_vouch_for_a_key_enrolled_while_it_ran, whose fixture enrols K2 BEFORE the completion lands.

F2 (rpc.rs:709-712, the second write): there is no second write. watch_keys registers and returns; the clear, the if added > 0 guard, the widening detection, and the async/fallible signature are all gone, along with the WalletReadFailed branch in control.rs. Pinned by a_repeated_enrolment_that_adds_nothing_still_leaves_the_new_key_uncovered, whose second watch call returns added = 0 and is exactly the client retry the old shape could not recover from.

One deviation from the decided shape, stated in the PR body: coverage is asked as CONTAINMENT over a stored set rather than equality over a fingerprint, because equality would also invalidate on unwatch and force a needless full resync.

[MichaelTaylor3d]

VERDICT: CHANGES-REQUIRED (recorded as a comment review - GitHub refuses REQUEST_CHANGES on a self-authored PR, HTTP 422).

CHANGES-REQUIRED — reviewed at e1c9522a613ed71c99509ed744fc6fa5ee456c58.

The restructure is sound where it was asked to be. F1 and F2 are genuinely impossible by construction, not merely unlikely, and I verified that rather than taking it:

• F1 — CatchUpReplay.covered is built inside finished_at from the very puzzle_hashes vector initial_sync_with_authority passes to request_puzzle_state (sync.rs:960 / :1002), it is a required constructor argument, the field is pub(super) with no setter, and complete_catch_up binds it in the same UPDATE as peak_height/header_hash/initial_sync_complete. A completion cannot describe an address its own subscription did not contain, at any ordering.
• F2 — watch_keys performs exactly one mutation (registry.watch) and returns. There is no second write, no added > 0 guard and no fallible/async invalidation left to fail. Verified there is no other door: WatchRegistry::watch is pub(crate), control.rs is a different crate, so re-pointing the handler at the registry is a compile error — that is structural, not conventional, and nothing needed the wider visibility (registered/unwatch/is_empty stay pub, and narrowing is the safe direction).
• Encoding — every constructor funnels through from_hex, which normalises via the router's own normalize_ph, sorts and dedups; the field is private, so covers' binary_search can never see an unsorted receiver. Round-trip is lossless, from_storage("") is the empty set, and the empty set covers only the empty set. Members only ever originate from hex::encode, so the comma separator is unambiguous in practice.
• Containment direction — recorded ⊇ followed, correct way round. An empty recorded set cannot vacuously cover a non-empty followed set; and Some(empty) is unreachable in production (sync.rs:931 refuses an empty subscription; refresh_tracked_coins returns early on empty phs).
• Router/subscriber union — service.rs:233 and :255-260 hand the samecustody and watchlist handles to UnionPuzzleHashSource and to the backend, and UnionPuzzleHashSource::puzzle_hashes now isfollowed_puzzle_hashes. No second copy of the union.
• Migration — additive, ALTER errors swallowed (idempotent), NULL reads as None reads as covers-nothing, and initial_sync_complete's meaning is untouched for is_synced, the phase logic and the arrival baseline.
• Fixture check (the place a weakening would hide) — db_with_owned_derivation records coverage over exactly owned_ph(), not blanket coverage. Those tests still require the flag, and had their followed set been wider they would now FAIL rather than pass. Not a rubber stamp. The three new tests discriminate: mutating the predicate back to is_synced() flips exactly the three that expect Fallback while re_announcing… and deregistering… (which expect Db) stay green — that is structurally forced by the fixtures, so the claim holds.

What blocks it: one gating finding. This PR adds a normative, unconditional SPEC MUST — "a read MUST be served from the local replica only while that recording CONTAINS the set the node currently follows" — and two money reads on the Sage-parity RPC surface do not obey it. They still route on the bare initial_sync_complete, with a client-supplied identity, and produce the same balance 0 / looks synced signature this release is stopped for. Details inline.

Non-gating findings are posted separately and resolved by me so they cannot block the merge.

[MichaelTaylor3d]

Non-gating notes (N1-N3), posted as comment threads and resolved by me so they cannot bar merge under required_conversation_resolution.

[MichaelTaylor3d]

Addressed at 4c0db9b.

Copilot's 5cd6b96 did the SPEC scoping correctly — the invariant now reads as governing the address-scoped money reads (balance_for_address / coins_for_address), and names get_sync_status and wallet_coins as still routing on the bare flag. It did not do the second half of the ask: no follow-up issue was opened, and the sentence said "tracked separately" without saying where.

Completed here:

• DIG-Network/dig_ecosystem#2878 filed (mvp, area:dig-node), citing both call sites, scoped_identity() as the source of the client-scoped set, and the downstream feeds at the coin/balance methods. It carries the reviewer's constraint forward explicitly: re-routing an identity-scoped money read is a behaviour change on a money surface and needs the threat model, so the issue asks for the decision, not a mechanical re-point.
• SPEC §18.6f cites #2878 by number and now states plainly that the invariant does not yet hold on that surface, rather than implying the tracking is a formality. The file:line pair was replaced with file + function names, since line numbers in a normative spec go stale on the next edit.

The containment invariant is unchanged and the routing at both Sage-parity sites is untouched, per the instruction.

Also in this push, unrelated and found in the worktree: fecb67a fixes an enrolment log line that rendered an 18-space run, from a long string literal wrapped across source lines.