Skip to content

fix(wallet): adopt the published phase contract and split the two empty-set states - #214

Merged
MichaelTaylor3d merged 6 commits into
mainfrom
fix/2609-adopt-dnci-011-phases
Aug 11, 2026
Merged

fix(wallet): adopt the published phase contract and split the two empty-set states#214
MichaelTaylor3d merged 6 commits into
mainfrom
fix/2609-adopt-dnci-011-phases

Conversation

@MichaelTaylor3d

@MichaelTaylor3dMichaelTaylor3d commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Fix-forward for dig_ecosystem#2609. Both gates have returned — see "Gate record" at the bottom.

Scope, stated plainly: this is a PARTIAL fix, and it does not close dig_ecosystem#2666.
It removes the {phase: synced, watched_addresses: 0} lie from the post-restart path, which is
the most commonly reached one. It does not remove it from every path — a REFUSED writer
(session_may_write == false) skips the empty-set arm and still reaches Synced beside a
measured zero watched set. #2666 stays OPEN after this merges, banded MVP. Do not read this PR
as "the node can no longer report synced while watching nothing."

Why this exists

v0.113.0 was merged from 1c40fc9 — the exact commit the pre-merge security gate FAILED — and released. It emits a phase token the published contract never declared.

WalletSyncPhase at dnci 0.10.0 is a closed three-variant Deserialize enum: no serde(other), no non_exhaustive. An unknown token therefore fails the entire WalletSyncStatusResult parse, not one field. A consumer pinned to 0.10 loses its sync read completely against a 0.113.0 node — strictly worse than the bug it replaced. 0.111.0 says something wrong; 0.113.0 says nothing.

The stable feed still serves 0.111.0 (verified against the live manifest), so auto-updating installs are unaffected. Nothing should promote 0.113.0.

What changed

dnci pin0.100.11 (published, with an Unrecognized fallback so this class cannot recur)
no_addresses_to_watchdeleted — it never existed in the contract
new tokensno_wallet_enrolled, wallet_not_unlocked
new factPuzzleHashSource::any_wallet()
version0.114.0

The two states are not one state

The second gating finding. WalletCustody::custodied_public_keys() is empty by design for an ENROLLED wallet in four reachable cases — an adopted legacy seed, a manifest predating the stored-public-keys field, a self-healed manifest, and an entry whose key fails to decode — and nothing back-fills it while locked, which is the state after every restart.

So watched == Some(0) alone cannot tell nothing to do from something to do that is not being done:

  • !any_wallet()no_wallet_enrolled — the honest all-clear; a consumer MAY present it as settled.
  • any_wallet()wallet_not_unlocked — the user's coins are NOT being followed; a consumer MUST NOT render it as settled or present a balance under it as complete.

any_wallet() reads custody's manifest, deliberately not the derivable key set — the key set is exactly the thing that is empty in the case being detected.

The gate that was missing, and now exists

dig-node referenced WalletSyncPhasenowhere. The only coupling to the contract was a hand-typed token list in tests/server.rs, and the 0.113.0 diff widened it from three tokens to four — so CI agreed with the bug and went green.

Two guards replace it:

  1. That assertion now derives from WalletSyncPhase::ALL.
  2. every_phase_the_node_can_emit_is_declared_by_the_published_contract — compares SyncPhase::ALL against the contract's ALL. No server, no fixture, so it holds for phases nothing can reach. Subset, not equality: a consumer may be ahead of a node, but a node MUST NOT emit an undeclared token.
  3. as_wire_matches_the_serialized_token_for_every_phase pins as_wire to serde, so the conformance test cannot be checking a fiction.

Verification

Mutation-verified, not just green:

  • Collapsing the two empty-set states back into one all-clear → 2 tests red (an_enrolled_wallet_with_no_derivable_addresses_is_not_an_all_clear, a_locked_wallet_reaches_wallet_not_unlocked_through_the_supervisor).
  • watched == Some(0)unwrap_or(0) == 0 → red (carried forward from the earlier round).

cargo test -p dig-wallet --lib sync_supervisor: 34 passed, 0 failed. Every #2568 quorum test still green.

Real-machine before/after is posted as a comment.

Not fixed here, deliberately

rpc.rs:3633-3645 still computes SyncLifecycle from initial_sync_complete alone — the same defect on the /ws wire. Filed as #2628 rather than folded in, because two different SyncLifecycle enums exist (dig-wallet/src/sage/events.rs:57 with Syncing/Synced/Disconnected, and the published dig-events-protocol/src/sync.rs:11 with Idle/Syncing/Synced). Extending the wrong closed enum is precisely how this incident happened; that decision deserves its own unit of work. #2609 stays open until both surfaces are honest.

What remains open after this merges

dig_ecosystem#2666 — a refused writer still reports synced while watching nothing. Banded MVP,
stays open. Arm 2 requires session_may_write, so a refused writer falls through to Synced with a
measured zero watched set, through the PeerTrust::Discovered door. Probed directly:

set_trust(false) set_watched(0, enrolled=true) initial_sync_complete=true
=> phase=Synced watched=Some(0) peers=1

This is pre-existing, not introduced here, and this PR strictly shrinks the set of states that can
tell that lie. Reachability is real and needs no attacker: a fresh node was measured sitting
uncorroborated for 400+ seconds with peak_height null throughout, while the installed node
corroborated within the same minutes and a later fresh run corroborated in ~30 seconds. So the
sequence is enroll → unlock → catch up → restart → corroboration lags → the node reports synced
while frozen and falling behind.

dig_ecosystem#2648 chains with it and is also still open.connect_random_peer unconditionally
prepends loopback and chia-query's pool does not dedupe, so a co-resident process can hold the pool
and make corroboration fail deliberately and indefinitely — converting the timing window above
into a controllable one. The two tickets are one attack when chained.

Gate record

  • Correctness gate: PASS — all three probes re-run.
  • Security gate: PASS at 22c294bd, with one required correction, applied in 4092e78c: three
    comments overstated the fix's reach (sync_supervisor.rs claimed a completed catch-up can never
    speak for a session watching nothing; the tests.rs acceptance bar implied more than it pins;
    control.rs credited the measured zero as a discriminator it is not). 22c294bd..4092e78c is
    doc-only — zero non-comment lines — so the code verdict carries forward unchanged.

Draft status alone did not prevent the previous merge — tracked as #2627. This PR was held DRAFT
until both verdicts were in hand, and undrafted immediately before merge.

…ty-set states
v0.113.0 shipped a no_addresses_to_watch token the published WalletSyncPhase never declared, which fails a consumer's ENTIRE WalletSyncStatusResult parse. Adopt dig-node-control-interface 0.11, emit no_wallet_enrolled / wallet_not_unlocked, and add PuzzleHashSource::any_wallet so an enrolled-but-unwatched wallet no longer reports an all-clear. Derive the token assertion from WalletSyncPhase::ALL so node-vs-contract drift fails CI.
@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Real-machine verification — 0.114.0

Both nodes on one box, same chain, same minute. No wallet is enrolled here, so
no_wallet_enrolled is the expected phase and is what it reports.

BEFORE — installed dign 0.111.0:

{"phase":"syncing","peak_height":9132747,"chia_peer_count":1}

AFTER — this branch, dign 0.114.0:

{"phase":"no_wallet_enrolled","peak_height":9132741,"chia_peer_count":1,"watched_addresses":0}

Advancing under the new phase, so this is a live replica and not a latched snapshot — four
consecutive samples: 9132741 → 9132742 → 9132745 → 9132746.

The token is the CONTRACT's

no_wallet_enrolled is a member of WalletSyncPhase::ALL in dig-node-control-interface 0.11. The
v0.113.0 token no_addresses_to_watch appears nowhere in this build.

The invariant, read straight out of the DB

peak=9132747 initial_sync_complete=0

Still 0. The phase settles without latching the flag that would flip §18.7 routing to
Source::Db and read a funded wallet as empty.

What this run does NOT prove

wallet_not_unlocked is not exercised here — this box has no wallet enrolled, and enrolling one
would mean handling seed material, which is out of scope for a verification run. That path is
covered by an_enrolled_wallet_with_no_derivable_addresses_is_not_an_all_clear (handle level, one
bool different from the passing no-wallet case) and
a_locked_wallet_reaches_wallet_not_unlocked_through_the_supervisor (through the real supervisor
loop, from custody's own answer). Both are mutation-verified: collapsing the two states back into a
single all-clear turns both red. Saying so explicitly because "verified on the real machine" would
otherwise imply more than the run actually covered.

Live node untouched

The installed service was never stopped and its binaries were never swapped — the new build ran
standalone on its own port and state dir. dign 0.111.0 still installed and syncing; the test
process has been stopped.

@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: CHANGES-REQUIRED at head 8caa3d41b7ca39a433375ab667419b9954330b26.

The core of this fix is right: the ladder maps correctly, the two empty-set states are genuinely separated (I independently mutation-verified the collapse - an_enrolled_wallet_with_no_derivable_addresses_is_not_an_all_clear and a_locked_wallet_reaches_wallet_not_unlocked_through_the_supervisor both go RED), the doubles are expressive (FixedHashes carries enrolled independently of the set, so WalletNotUnlocked is representable), as_wire is pinned to serde, the dnci 0.11 contract does declare both new tokens plus Unrecognized, the #2568 quorum tests are untouched and green (34/34), SPEC 18.6b is structurally intact (exactly one 18.6b., the two distinct 18.6c. sections at 3806 and 3992, a single contiguous hunk - the restore was clean), and every required check is green with zero unresolved threads.

Three findings block. Two are exactly the #2609 shape - a guard whose PRODUCTION wiring nothing exercises, and a hand-maintained list standing in for a compile-time fact.

  1. MAJOR - the production any_wallet() sourcing is UNVERIFIED. I replaced WalletCustodys impl with the precise wrong implementation the docs warn against and 456/456 dig-wallet lib tests stayed green.
  2. MAJOR - SyncPhase::ALL is hand-maintained; I added a variant to the enum and to as_wire but not to ALL, and both conformance guards stayed green.
  3. MAJOR - Synced outranks WalletNotUnlocked, and initial_sync_complete is persistent, so the state both the code doc and the SPEC call the COMMON state after every restart reports synced on the main path.

Detail inline. Nothing handed to Copilot except the minor doc sweep: (1) and (2) are test/gate-integrity judgement calls and (3) is a state-machine plus SPEC decision.


NON-GATING (minor), posted here because the file is not in the diff so it cannot be anchored inline:

NON-GATING (minor) - stale after the rename: this still names SyncPhase::NoAddressesToWatch, a variant that no longer exists. Same sweep: crates/dig-wallet/src/sage/sync_supervisor/tests.rs:842 is still called phase_is_no_addresses_to_watch_when_custody_is_empty_on_a_writing_peer while asserting NoWalletEnrolled.

@copilot fix this: in DEVELOPMENT_LOG.md line 26 replace SyncPhase::NoAddressesToWatch with SyncPhase::NoWalletEnrolled, and rename the test function at crates/dig-wallet/src/sage/sync_supervisor/tests.rs:842 to phase_is_no_wallet_enrolled_when_custody_is_empty_on_a_writing_peer. Rename only - do NOT change any assertion, fixture, or the set_watched(0, false) argument.

Comment threadcrates/dig-wallet/src/sage/sync_supervisor.rs
Comment threadcrates/dig-wallet/src/sage/sync_supervisor.rs Outdated
Comment threadcrates/dig-wallet/src/sage/sync_supervisor.rs
…etEnrolled
Co-authored-by: MichaelTaylor3d <5665004+MichaelTaylor3d@users.noreply.github.com>

CopilotAI commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

CORRECTNESS GATE: CHANGES-REQUIRED at head 8caa3d41b7ca39a433375ab667419b9954330b26.

The core of this fix is right: the ladder maps correctly, the two empty-set states are genuinely separated (...

Done in the latest commit. DEVELOPMENT_LOG.md:26 now reads SyncPhase::NoWalletEnrolled, and the test at line 842 is renamed to phase_is_no_wallet_enrolled_when_custody_is_empty_on_a_writing_peer — no assertions, fixtures, or set_watched arguments touched. 34/34 sync_supervisor tests pass.

…eak for an unwatched wallet
Three gate findings. (1) initial_sync_complete is persistent, so a wallet caught up then restarted LOCKED reported Synced while watching zero addresses - the empty-set arm now precedes Synced. (2) SyncPhase::ALL was hand-maintained and both gates proved a variant could be emitted while omitted from it; the enum, ALL and as_wire are now generated from one list. (3) the production any_wallet() sourcing was untested; add a real WalletCustody test over a self-healed manifest. Also correct the open-read rationale, which named the wrong field and omitted DIG_NODE_ALLOW_REMOTE.

@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.

PASS (correctness re-gate) — head 22c294b

All three prior MAJOR findings verified addressed by execution, not by claim. Probes run in an isolated worktree (all reverted; tree clean):

  1. Finding 1 — any_wallet() sourcing. Rewrote production WalletCustody::any_wallet (crates/dig-wallet/src/sage/custody.rs:254) to !puzzle_hashes().is_empty(). production_any_wallet_reads_the_manifest_not_the_derivable_keysFAILED at sync_supervisor/tests.rs:958. The test discriminates the property (enrollment source), not an outcome.
  2. Finding 2 / security F1 — ALL untied to the enum. The bypass is now unrepresentable: SyncPhase is emitted only by declare_sync_phases! (sync_supervisor.rs:98-127), which derives the enum, ALL and as_wire from one list, so there is no path to declare a variant outside it and no way to give a variant an as_wire arm while omitting it from ALL. Constructive probe: added Bogus => "bogus" to the invocation → every_phase_the_node_can_emit_is_declared_by_the_published_contractFAILED. My original probe is now structurally foreclosed.
  3. Finding 3 — arm ordering. Moved the Synced arm back above the empty-set arm → a_previously_synced_wallet_restarted_locked_is_not_reported_as_syncedFAILED; its control a_completed_catch_up_still_reports_synced_while_watching_addresses stayed green. Both directions proven.

Reorder blast radius (the risky change). The empty-set arm's predicate is peers >= 1 && session_may_write && watched == Some(0). Its intersection with the old Synced predicate (initial_sync_complete && peers >= 1) is exactly the latched + attached + writable + measured-zero case. So watched == None (unmeasured) and session_may_write == false are untouched — they never entered the new arm — and the only behaviour change is the defect case plus the semantically identical "legitimately became empty after a successful sync", which is the same falsehood and correctly no longer reports Synced. No state lost Synced that should have kept it.

  1. Security F2. The is_open_control_read rationale now attributes the disclosure to the enrollment bit rather than the count, and states the DIG_NODE_ALLOW_REMOTE caveat. Accurate against the code.
  2. Copilot 41845d4.NoAddressesToWatch has zero occurrences repo-wide; the rename survived the rebase.

Readability. The macro is module-private, single-invocation, defined immediately above its use, forwards per-variant doc metas so the full doc surface survives in rustdoc, and its own doc-comment states why it exists. Right scope; nothing a reader needs is obscured.

Gates. All five required contexts asserted BY NAME: SUCCESS (incl. Test + coverage). Unresolved review threads: 0. mergeStateStatus=CLEAN. PR is still draft — correct, per §2.4a; undraft only once the sibling gate verdict is in.

Non-gating observation (no thread opened, not a blocker): with session_may_write == false and a latched flag, a refused writer still reports Synced. Pre-existing, unchanged by this diff, and outside this PR's scope.

MichaelTaylor3dand others added 2 commits August 11, 2026 06:19
…ches
The pre-merge security gate disproved by probe a universal asserted in this
same commit. `sync_supervisor.rs` claimed "a completed catch-up can never
speak for a session that is watching nothing". Arm 2 also requires
`session_may_write`, so a REFUSED writer skips it and falls through to
`Synced` with a measured zero watched set. Measured output:
phase=Synced watched=Some(0) peers=1
set_trust(false) set_watched(0, enrolled=true) initial_sync_complete=true
That is the exact `{synced, watched_addresses: 0}` pair this arm exists to
abolish, reached through the `PeerTrust::Discovered` door. The behaviour is
pre-existing and this delta strictly shrinks the bad set, so it is not a
regression -- but the false sentence would have shipped in the commit that
also says "a gate whose rationale overstates its reach is how a gate becomes
a decoration". A future auditor reading "can never" does not look again.
Three corrections, no behaviour change:
* `sync_supervisor.rs` -- qualify the claim with "and whose peer may write",
and name the residue as #2666 rather than implying it is closed.
* `sync_supervisor/tests.rs` -- narrow the acceptance bar to what the test
actually pins. It sets `set_trust(true)`, so it is silent about a refused
writer; a green here does not mean the node can no longer report synced
while watching nothing.
* `control.rs` -- the measured zero does NOT separate the two empty-set
phases from `syncing`, because a refused writer reports `syncing` with
`watched_addresses: 0` too (the repo's own
`a_refused_writer_is_not_reported_as_nothing_to_watch`). The phase is the
discriminator, not the count.
Refs dig_ecosystem#2666
Co-Authored-By: Claude <noreply@anthropic.com>
…verse
The security gate found SPEC.md normatively specifying the exact defect this
PR removes:
`synced` outranks both: once `initial_sync_complete` is `true` with a
peer attached, the node reports `synced`.
That is a flat contradiction of `sync_supervisor.rs:259-297`, where the
empty-set arm deliberately PRECEDES `Synced`, and of this PR's own test
`a_previously_synced_wallet_restarted_locked_is_not_reported_as_synced`,
which asserts `WalletNotUnlocked` under precisely `initial_sync_complete =
true` plus a peer attached.
This gates the merge rather than being a doc nit. SPEC.md is the contract an
independent reimplementation is built against, and reconciling code TO spec
is the normal repair direction -- so the stale sentence hands a future lane a
normative citation for putting the `Synced` arm back on top and
reintroducing the lie.
The sentence survived from the one-phase text into the rewritten paragraph.
It read as harmless because the same paragraph still claimed
`initial_sync_complete` "can never latch", which is true only for the
NEVER-ENROLLED case -- the latched-then-restarted case is the whole point of
the fix.
Three corrections:
* Scope the never-latches claim to a node that has never enrolled a wallet.
* State the ranking correctly and mark the ORDER as normative, with the
latched-flag reasoning that makes it non-obvious.
* Stop being silent about the refused-writer residue. SPEC previously said
reporting one as nothing-to-watch would present a stalled replica as
healthy, but never said it currently reports `synced`. It does, with a
frozen peak, because §18.6a drops `new_peak_wallet` from a
non-authoritative peer. Recorded as a known gap with a SHOULD, to become a
MUST when #2666 lands.
Refs dig_ecosystem#2666
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 11, 2026 14:05
@MichaelTaylor3d
MichaelTaylor3d merged commit 9a53577 into mainAug 11, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the fix/2609-adopt-dnci-011-phases branch August 11, 2026 14:06
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.

2 participants

@MichaelTaylor3d