From 04e45f363afba0ce84cbdca96889a4128001a2db Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 20:54:05 -0700 Subject: [PATCH 1/4] chore(wallet): anchor the syncStatus honesty lane (#495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvage anchor for dig-node#495: `control.wallet.syncStatus` can still emit `{phase: "synced", peak_height: null}` — a positive currency claim beside a refusal to say what height it is a claim about. Version bump only; the fix follows in this branch. Refs #495 Co-Authored-By: Claude --- Cargo.toml | 2 +- crates/dig-wallet/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index effdd8e9..d1a42e36 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.236.0" +version = "0.242.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over diff --git a/crates/dig-wallet/Cargo.toml b/crates/dig-wallet/Cargo.toml index 86cca621..a7856032 100644 --- a/crates/dig-wallet/Cargo.toml +++ b/crates/dig-wallet/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "dig-wallet" -version = "0.46.0" +version = "0.47.0" edition = "2021" license = "GPL-2.0-only" description = "DIG Browser built-in Chia wallet sidecar: a local axum server (using digstore-chain + chia-wallet-sdk over coinset.org) that serves a Sage-mirroring wallet UI. Native Rust so BLS signing works; the browser opens it at 127.0.0.1." From 40bcabc0f127e873c244563deeb072cb716368bc Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 21:22:45 -0700 Subject: [PATCH 2/4] test(wallet): red -- a synced phase that cannot name its heights (#495) `control.wallet.syncStatus` can emit `{phase: "synced", peak_height: null}`: a positive claim of currency beside a refusal to say what height it is current at. Both unmeasured arms of `is_following` are production-reachable. Adds three tests and REVERSES one that encoded the defect as correct behaviour. All four fail for the right reason against today's code: 762 passed; 4 failed; 1 ignored; 0 measured; 0 filtered out. Co-Authored-By: Claude --- Cargo.lock | 4 +- .../src/sage/sync_supervisor/tests.rs | 228 +++++++++++++++++- 2 files changed, 219 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fea707be..b2922085 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.236.0" +version = "0.242.0" dependencies = [ "async-trait", "axum", @@ -3337,7 +3337,7 @@ dependencies = [ [[package]] name = "dig-wallet" -version = "0.46.0" +version = "0.47.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs index d79581e5..ff67e9e8 100644 --- a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs +++ b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs @@ -3242,14 +3242,24 @@ async fn the_following_tolerance_holds_at_the_bound_and_fails_one_beyond_it() { ); } -/// **Proves (dig_ecosystem#2851):** an unmeasured height on EITHER side leaves the phase exactly as -/// it was. -/// -/// The Option-honesty guard. `None` is unobservable, never a zero, and an unobservable gap is not -/// an accusation — a node with no chain transport must not be reported as behind a chain it cannot -/// see. +/// **Proves (dig-node#495):** an unmeasured height on EITHER side WITHHOLDS the `Synced` claim. +/// +/// This test previously asserted the opposite, and that expectation was wrong in two ways worth +/// stating rather than quietly deleting. +/// +/// It GENERALISED a justification that only ever applied to ONE side. "An unobservable gap is not +/// an accusation" is sound about the PEER tier: a node whose chain transport was never built has +/// no second opinion, and a missing second opinion must not be spent as evidence against the +/// replica. It says nothing about a missing REPLICA height, which is the subject of the claim +/// having no measurement whatsoever. +/// +/// And it treated a phase as the ABSENCE OF AN ACCUSATION. `Synced` is a positive claim of +/// currency, so it needs evidence FOR itself, not merely the lack of evidence against. +/// [`SyncPhase::Syncing`] already covers both cases in its own words: the replica is "otherwise +/// not both caught up AND currently following the chain" — and with either height missing, +/// "currently following" cannot be established. #[tokio::test] -async fn an_unmeasured_height_leaves_the_phase_unchanged() { +async fn an_unmeasured_height_on_either_side_withholds_the_synced_claim() { let db = WalletDb::open_in_memory().await.unwrap(); db.force_initial_sync_complete_for_test(true).await.unwrap(); @@ -3261,8 +3271,8 @@ async fn an_unmeasured_height_leaves_the_phase_unchanged() { // The replica's own peak is unknown; the peers' is far ahead. assert_eq!( handle.status(&db, tier_at(PEERS_PEAK)).await.unwrap().phase, - SyncPhase::Synced, - "an unknown replica peak was read as evidence of being behind" + SyncPhase::Syncing, + "a replica that cannot name its own height claimed to be current at it" ); // The replica's peak is known; nobody has measured the peers'. @@ -3273,8 +3283,8 @@ async fn an_unmeasured_height_leaves_the_phase_unchanged() { .await .unwrap() .phase, - SyncPhase::Synced, - "an unobservable peer tier was read as evidence against the replica" + SyncPhase::Syncing, + "a replica with no second opinion was reported as current on that basis alone" ); } @@ -4875,3 +4885,199 @@ async fn a_frame_on_a_live_session_attributes_through_the_update_loop() { h.stop().await; } + +// --------------------------------------------------------------------------- +// A synced phase must carry the heights that bound it (dig-node#495) +// --------------------------------------------------------------------------- + +/// **Proves (dig-node#495):** a replica that has never recorded a peak of its own is never +/// reported as `Synced`, however complete its latched catch-up is. +/// +/// This state is production-reachable, not hypothetical. +/// [`crate::sage::db::WalletDb::latch_synced_over_unless_reset`] sets `initial_sync_complete` +/// without ever writing a peak — its own doc says the path "has replayed nothing and has no +/// terminal height to offer". With a may-write session attached, that emitted +/// `{phase: "synced", peak_height: null}`: a positive claim of currency beside a refusal to say +/// what height it is current AT. +/// +/// FIXTURE DESIGN: everything else on the ladder is the honest, synced-reaching case — latched +/// catch-up, a peer attached now, write authority, a non-empty watched set — so the ONLY thing +/// that can withhold the claim is the missing replica peak. The peers are deliberately far ahead +/// so the gap would be damning if it could be computed at all. +#[tokio::test] +async fn a_replica_with_no_peak_of_its_own_is_never_reported_as_synced() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.force_initial_sync_complete_for_test(true).await.unwrap(); + // No `set_peak`: this replica has no terminal height to offer. + + let (handle, _rx) = SyncHandle::new(); + handle.set_connected(1); + handle.set_trust(true); + handle.set_watched(1, true); + + let status = handle.status(&db, tier_at(PEERS_PEAK)).await.unwrap(); + assert_ne!( + status.phase, + SyncPhase::Synced, + "a replica with no peak of its own claimed to be current at a height it cannot name" + ); + assert_eq!(status.phase, SyncPhase::Syncing); + // The measurement stays honest: unknown is still reported as unknown, never fabricated. + assert_eq!( + status.peak_height, None, + "an absent replica peak must not be invented to make the payload look complete" + ); +} + +/// **Proves (dig-node#495):** a replica with no second opinion about the chain tip is never +/// reported as `Synced`. +/// +/// [`crate::sage::fallback::ChainPeerTier::peak_height`] is `None` until one of the node's own +/// peers says something. A replica thousands of blocks behind then reported `synced` purely +/// because nothing was in a position to contradict it — the shape of dig-node#416, whose measured +/// lag was 8,380 blocks. +/// +/// FIXTURE DESIGN: the replica DOES know its own peak here, so the two unmeasured arms stay +/// distinguishable — this half fails only for the missing peer height, and its sibling above only +/// for the missing replica height. The payload assertion is the point: `chia_peer_peak_height` +/// stays `None`, which is what reports this case DISTINCTLY from a measured lag. +#[tokio::test] +async fn a_replica_with_no_second_opinion_is_never_reported_as_synced() { + let db = WalletDb::open_in_memory().await.unwrap(); + db.force_initial_sync_complete_for_test(true).await.unwrap(); + db.set_peak(FROZEN_REPLICA_PEAK, "aa").await.unwrap(); + + let (handle, _rx) = SyncHandle::new(); + handle.set_connected(1); + handle.set_trust(true); + handle.set_watched(1, true); + + let status = handle + .status(&db, ChainPeerTier::UNOBSERVABLE) + .await + .unwrap(); + assert_ne!( + status.phase, + SyncPhase::Synced, + "a replica nothing was able to contradict was reported as current on that basis alone" + ); + assert_eq!(status.phase, SyncPhase::Syncing); + assert_eq!( + status.chia_peer_peak_height, None, + "the unmeasured peer tier is what makes this case distinguishable, and it must be \ + reported as unmeasured rather than as an observed zero" + ); + assert_eq!(status.peak_height, Some(FROZEN_REPLICA_PEAK)); +} + +/// **Proves (dig-node#495):** ACROSS THE WHOLE INPUT SPACE of the phase ladder, `Synced` implies +/// both bounding heights are present. The pairing this ticket removes is unrepresentable, not +/// merely absent from a chosen fixture. +/// +/// FIXTURE DESIGN: the full cross-product of every input [`SyncHandle::status`] reads — the +/// replica peak, the peer peak, the latched flag, the session's history (never attached, attached +/// then dropped, attached now), its write authority, its resolved watched set, and whether a +/// wallet is enrolled. A single hand-picked fixture would prove only that ONE route to `Synced` +/// carries its heights; the grid proves every route does. +/// +/// THE NON-VACUITY ASSERTION IS LOAD-BEARING. An implication over an input space that never +/// reaches `Synced` holds trivially, so this test would pass against an implementation that +/// simply never emitted the phase — the strongest possible false green here. Counting the +/// combinations that DO reach it, and demanding the count be non-zero, is what makes the +/// implication evidence. +/// +/// The handle is driven in the order the supervisor learns its facts (connect, then trust, then +/// the resolved set), because `set_connected(0)` deliberately clears trust and the watched set — +/// a dropped session's measurement is no longer a measurement. +#[tokio::test] +async fn a_synced_phase_always_carries_the_heights_that_bound_it() { + /// How the session reached its current state, which is not recoverable from the peer count + /// alone: never attaching and attaching-then-dropping both report zero peers, and only the + /// first is `NotStarted`. + #[derive(Debug, Clone, Copy)] + enum Session { + NeverConnected, + Dropped, + Connected, + } + + let replica_peaks = [None, Some(FROZEN_REPLICA_PEAK), Some(PEERS_PEAK)]; + let peer_peaks = [None, Some(PEERS_PEAK)]; + let sessions = [ + Session::NeverConnected, + Session::Dropped, + Session::Connected, + ]; + + let mut synced_reached = 0usize; + + for replica_peak in replica_peaks { + for latched in [false, true] { + // One DB per (replica peak, latched) pair: those are the only inputs the DB carries, + // and re-opening one per combination would spend the whole grid on migrations. + let db = WalletDb::open_in_memory().await.unwrap(); + if let Some(peak) = replica_peak { + db.set_peak(peak, "aa").await.unwrap(); + } + db.force_initial_sync_complete_for_test(latched) + .await + .unwrap(); + + for peer_peak in peer_peaks { + for session in sessions { + for may_write in [false, true] { + for watched in [None, Some(0u32), Some(1u32)] { + for wallet_enrolled in [false, true] { + let (handle, _rx) = SyncHandle::new(); + match session { + Session::NeverConnected => {} + Session::Dropped | Session::Connected => { + handle.set_connected(1); + handle.set_trust(may_write); + if let Some(n) = watched { + handle.set_watched(n, wallet_enrolled); + } + if matches!(session, Session::Dropped) { + handle.set_connected(0); + } + } + } + + let tier = match peer_peak { + None => ChainPeerTier::UNOBSERVABLE, + Some(peak) => tier_at(peak), + }; + let status = handle.status(&db, tier).await.unwrap(); + + let combination = format!( + "replica_peak={replica_peak:?} peer_peak={peer_peak:?} \ + latched={latched} session={session:?} \ + may_write={may_write} watched={watched:?} \ + wallet_enrolled={wallet_enrolled}" + ); + + if status.phase == SyncPhase::Synced { + synced_reached += 1; + assert!( + status.peak_height.is_some(), + "synced with no replica height, at {combination}" + ); + assert!( + status.chia_peer_peak_height.is_some(), + "synced with no peer height, at {combination}" + ); + } + } + } + } + } + } + } + } + + assert!( + synced_reached > 0, + "no combination reached Synced, so the implication above held vacuously and proves \ + nothing about an implementation that does emit the phase" + ); +} From a465d83c73f229fc15627b19c2121ada468988c4 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 21:31:30 -0700 Subject: [PATCH 3/4] fix(wallet): make a synced phase carry the heights that bound it (#495) `control.wallet.syncStatus` could emit `{phase: "synced", peak_height: null}` -- a positive claim of currency beside a refusal to say what height it is current AT. Both unmeasured arms of the old `is_following` predicate are production-reachable: the latch-over path sets `initial_sync_complete` without ever writing a peak, and the peer tier reports no height until one of the node's own peers speaks. `is_following` is replaced by `FollowingEvidence`, which carries the two heights whose gap establishes currency and cannot be constructed when either is unmeasured. A private `settled` module then produces the phase and the reported peak TOGETHER: `SettledPhase::synced` demands the evidence and takes the height FROM it, and is the only route to `SyncPhase::Synced` in the crate's non-test code. The pairing is unrepresentable rather than unlikely. `replica_answer_is_current` converges onto the same constructor, dropping its two duplicated guards -- behaviour there is unchanged, and the structural agreement its doc claimed between the two endpoints is now true, having been false when written. No wire change: the phase set, its spellings and the six emitted fields are untouched. Co-Authored-By: Claude --- crates/dig-wallet/src/sage/rpc.rs | 76 ++++---- crates/dig-wallet/src/sage/sync.rs | 8 +- crates/dig-wallet/src/sage/sync_supervisor.rs | 171 +++++++++++++++--- .../src/sage/sync_supervisor/tests.rs | 45 +++-- 4 files changed, 203 insertions(+), 97 deletions(-) diff --git a/crates/dig-wallet/src/sage/rpc.rs b/crates/dig-wallet/src/sage/rpc.rs index e2a74219..81e54f6c 100644 --- a/crates/dig-wallet/src/sage/rpc.rs +++ b/crates/dig-wallet/src/sage/rpc.rs @@ -1035,7 +1035,7 @@ impl WalletBackend { /// `synced: true` on that basis told a client a stale balance was settled, which is the /// money-adjacent falsehood dig_ecosystem#2869 exists to remove. /// - /// It reuses [`super::sync_supervisor::is_following`] — the SAME predicate + /// It reuses [`super::sync_supervisor::FollowingEvidence`] — the SAME evidence /// `control.wallet.syncStatus` reports its phase from — so a client cannot be told `synced` by /// one endpoint and `syncing` by the other about the same moment. /// @@ -1046,12 +1046,16 @@ impl WalletBackend { /// `db.is_synced()` directly and so could, and on a live node did, report `synced: true` /// about the same replica this method was calling stale (dig-node#293). /// - /// It narrows that predicate on BOTH of its unobservable arms, and only here. `is_following` - /// answers `true` whenever EITHER height is missing, because on a status endpoint an absent - /// measurement is not an accusation against the replica. On a money read it is the opposite: - /// currency is a claim, and nothing that was never measured can establish one. Either arm left - /// unnarrowed leaves `synced: true` resting on the latched `initial_sync_complete` this method - /// exists to stop trusting. + /// BOTH unobservable arms withhold the claim, and that is now a property of the evidence type + /// itself rather than a narrowing applied at this call site. `FollowingEvidence::measure` + /// yields `None` whenever EITHER height is missing: currency is a claim, and nothing that was + /// never measured can establish one. Either arm left permissive would leave `synced: true` + /// resting on the latched `initial_sync_complete` this method exists to stop trusting. + /// + /// This method used to hold that narrowing alone, in two `let ... else { return false }` + /// guards duplicating exactly those arms. dig-node#495 moved the rule into the constructor and + /// this call site simply asks for the evidence — one narrowing, in one place, for both + /// endpoints. /// /// - **No PEER height** — there is no second opinion to compare the replica against. /// - **No REPLICA height** — there is no figure to compare, and `synced: true` would then be @@ -1065,35 +1069,24 @@ impl WalletBackend { /// Either way the figure is still SERVED, with whatever `peak_height` is actually known, /// labelled stale — never withheld. /// - /// `is_following` itself is deliberately left ALONE. Its permissive `_ => true` is correct for - /// the sync-phase reporting it was written for, where an unmeasured tier must not be spent as - /// evidence against a replica; narrowing it there would change a phase machine owned by another - /// family. The narrowing is a property of the MONEY read, so it lives at this call site. - /// - /// That placement has a LIMIT, and it is stated here rather than left to be rediscovered: - /// this method is the single gate for every read served by [`WalletBackend`] — each one either - /// passes through here or writes the literal `false` — but it is NOT the only producer of a - /// `synced` claim in the crate. [`super::sync_supervisor::SyncHandle::status`] reaches - /// `SyncPhase::Synced` through its own `is_following` call and pairs it with the replica's raw - /// `peak_height`, so `control.wallet.sync-status` can still emit - /// `{phase: "synced", peak_height: null}` — the exact pairing this gate abolishes on the money - /// reads. That path is deliberately out of scope: it is a status endpoint rather than a - /// currency claim, and the phase machine belongs to another family (dig_ecosystem#2761). - /// - /// The asymmetry worth carrying into that work: `is_following`'s own doc justifies its - /// permissive arm ENTIRELY in terms of an unmeasured PEER tier, and offers no justification at - /// all for the unmeasured-REPLICA arm. Those are different things. An absent peer height is a - /// missing second opinion, which is fairly read as no accusation; an absent replica height is - /// the subject of the claim having no measurement whatsoever, which supports no verdict in - /// either direction. + /// The structural agreement claimed above is now TRUE, and it was FALSE when first written — + /// which is the point of the convergence (dig-node#495). This method is the single gate for + /// every read served by [`WalletBackend`], but it was never the only producer of a `synced` + /// claim in the crate: [`super::sync_supervisor::SyncHandle::status`] reached + /// `SyncPhase::Synced` through a predicate permissive on both unmeasured arms, and paired it + /// with the replica's separately-read `peak_height`, so `control.wallet.syncStatus` could emit + /// `{phase: "synced", peak_height: null}` — the exact pairing this gate abolished on the money + /// reads while the status endpoint kept producing it. + /// + /// Both endpoints now derive `synced` from the same [`super::sync_supervisor::FollowingEvidence`], + /// and on that path the reported height comes FROM the evidence, so the pairing is + /// unrepresentable rather than merely unlikely. async fn replica_answer_is_current(&self, peak_height: Option) -> bool { - let Some(replica_peak) = peak_height else { - return false; - }; - let Some(peer_peak) = self.chain_peer_tier().await.peak_height else { - return false; - }; - super::sync_supervisor::is_following(Some(replica_peak), Some(peer_peak)) + super::sync_supervisor::FollowingEvidence::measure( + peak_height, + self.chain_peer_tier().await.peak_height, + ) + .is_some() } /// The chain-sync supervisor's handle, if one is running. @@ -7514,10 +7507,11 @@ mod tests { /// chain peer that has announced a height serves its figure labelled stale. /// /// This is the state a freshly-started node sits in, and the one a node with no reachable chain - /// peer sits in indefinitely. [`super::sync_supervisor::is_following`] answers `true` there by - /// design (an absent second opinion is not an accusation on a status endpoint), so a money read - /// delegating to it unnarrowed pairs `synced: true` with an arbitrarily old `peak_height` — the - /// stale-presented-as-current claim this PR exists to remove. + /// peer sits in indefinitely. The predicate this gate once delegated to answered `true` there + /// by design (an absent second opinion was read as no accusation), so a money read delegating + /// to it unnarrowed pairs `synced: true` with an arbitrarily old `peak_height`, the + /// stale-presented-as-current claim this PR exists to remove. Since dig-node#495 + /// [`super::sync_supervisor::FollowingEvidence::measure`] withholds the evidence itself. /// /// FIXTURE DESIGN — `peak_height: None` is what makes the tier unobservable, and it is the only /// axis varied from [`a_replica_level_with_its_peers_still_reports_synced`], which stays green @@ -7561,8 +7555,8 @@ mod tests { /// **Proves:** an UNKNOWN REPLICA height is never reported as current either — the other /// `None` arm of the same predicate, on the endpoint where it actually reaches production. /// - /// [`super::sync_supervisor::is_following`] answers `true` when EITHER side is `None`, and - /// [`WalletBackend::replica_answer_is_current`] narrowed only the peer side. `chain_peak` + /// The predicate this gate once delegated to answered `true` when EITHER side was `None`, and + /// an earlier [`WalletBackend::replica_answer_is_current`] narrowed only the peer side. `chain_peak` /// escapes the remaining arm by construction — it calls the gate inside `if let Some(peak)`, /// so it can never hand it a `None` replica. The balance and coin reads do not: they read /// `sync_state().peak_height` as an `Option` and pass it straight through. So the money reads, diff --git a/crates/dig-wallet/src/sage/sync.rs b/crates/dig-wallet/src/sage/sync.rs index 6dd16f13..e5e3f78b 100644 --- a/crates/dig-wallet/src/sage/sync.rs +++ b/crates/dig-wallet/src/sage/sync.rs @@ -1320,7 +1320,7 @@ mod tests { use super::*; use crate::sage::db::WalletDb; use crate::sage::sync_supervisor::{ - is_following, StallVerdict, StallWatch, SESSION_MAX_LIFETIME, STALL_AFTER, + FollowingEvidence, StallVerdict, StallWatch, SESSION_MAX_LIFETIME, STALL_AFTER, }; fn coin(parent: u8, ph: u8, amount: u64) -> Coin { @@ -3341,7 +3341,7 @@ mod tests { /// /// This is the test the whole change exists for. An accepted `u32::MAX` does not merely /// misreport the peak — it permanently DISABLES both of them, and silently: - /// [`is_following`]'s `peers.saturating_sub(replica)` is `0` for ever, so the phase reports + /// [`FollowingEvidence`]'s `peers.saturating_sub(replica)` is `0` for ever, so the phase reports /// `Synced` however far behind the replica really is; and [`StallWatch`]'s /// `behind = peers > replica` is false for ever, so the stall clock never starts. /// @@ -3367,7 +3367,7 @@ mod tests { let peers = Some(anchor + 50); assert!( - !is_following(replica, peers), + FollowingEvidence::measure(replica, peers).is_none(), "the phase must still be able to see a replica {replica:?} behind peers {peers:?}" ); @@ -3611,7 +3611,7 @@ mod tests { let peers = Some(anchor + 50); assert!( - !is_following(replica, peers), + FollowingEvidence::measure(replica, peers).is_none(), "the phase must still see a replica {replica:?} behind peers {peers:?}" ); diff --git a/crates/dig-wallet/src/sage/sync_supervisor.rs b/crates/dig-wallet/src/sage/sync_supervisor.rs index 6ae214f3..ef7f4a4f 100644 --- a/crates/dig-wallet/src/sage/sync_supervisor.rs +++ b/crates/dig-wallet/src/sage/sync_supervisor.rs @@ -295,8 +295,15 @@ declare_sync_phases! { /// /// It is NOT a fourth way of spelling `Synced`: `Synced` licenses /// [`crate::sage::routing::route`] to serve wallet-scoped reads from the local replica, and - /// over an un-queried DB that reads a funded wallet as empty. This says the chain replica is - /// current AND that no wallet-scoped claim is being made at all. + /// over an un-queried DB that reads a funded wallet as empty. This says only that NO + /// wallet-scoped claim is being made at all. + /// + /// It deliberately claims nothing about the replica's currency, and an earlier version of this + /// sentence did (dig-node#495): this arm reads no heights whatsoever -- no + /// `initial_sync_complete`, no gap against the peers -- so it is in no position to. Requiring + /// currency HERE would regress dig_ecosystem#2609 straight back to reporting a default install + /// as forever catching up, which is why the arm's behaviour is right and only its description + /// was wrong. NoWalletEnrolled => "no_wallet_enrolled", /// **A wallet IS enrolled, but no addresses are being watched for it** — so the user's coins /// are not being followed. @@ -422,7 +429,7 @@ impl SyncHandle { /// Compose the live counters with the DB's persisted sync state. /// /// `Synced` requires a completed catch-up, a peer attached now, AND the replica actually - /// following the chain ([`is_following`]): an offline replica is stale, however complete its + /// following the chain ([`FollowingEvidence`]): an offline replica is stale, however complete its /// last catch-up was, and reporting it synced is the shape that makes a client trust a day-old /// balance. Neither of the first two clauses is about the PRESENT — one is a latched flag, the /// other says a socket exists — so a replica frozen behind a half-open peer satisfied both and @@ -439,8 +446,8 @@ impl SyncHandle { ) -> sqlx::Result { let observed = self.observed(); let state = db.sync_state().await?; - let phase = if !observed.ever_connected { - SyncPhase::NotStarted + let settled = if !observed.ever_connected { + settled::SettledPhase::not_started(state.peak_height) } else if observed.peers >= 1 && observed.session_may_write && observed.watched == Some(0) { // Three facts get us to "this session is watching nothing", and each rules out a // different lie: @@ -475,22 +482,24 @@ impl SyncHandle { // indistinguishable from the address set alone, which is exactly how the first version // of this fix came to report a locked wallet as settled. if observed.wallet_enrolled { - SyncPhase::WalletNotUnlocked + settled::SettledPhase::wallet_not_unlocked(state.peak_height) } else { - SyncPhase::NoWalletEnrolled + settled::SettledPhase::no_wallet_enrolled(state.peak_height) } - } else if state.initial_sync_complete - && observed.peers >= 1 - && observed.session_may_write - && is_following(state.peak_height, tier.peak_height) + } else if let Some(evidence) = + FollowingEvidence::measure(state.peak_height, tier.peak_height).filter(|_| { + state.initial_sync_complete && observed.peers >= 1 && observed.session_may_write + }) { - SyncPhase::Synced + // The evidence is what licenses the claim AND what supplies the height reported with + // it, so a `synced` phase can never be paired with an absent peak (dig-node#495). + settled::SettledPhase::synced(evidence) } else { - SyncPhase::Syncing + settled::SettledPhase::syncing(state.peak_height) }; Ok(WalletSyncStatus { - phase, - peak_height: state.peak_height, + phase: settled.phase(), + peak_height: settled.peak_height(), chia_peer_count: tier.peer_count, subscription_peer_count: Some(observed.peers), chia_peer_peak_height: tier.peak_height, @@ -567,20 +576,125 @@ impl SyncHandle { } } -/// Whether the replica is following the chain RIGHT NOW, judged by the only evidence the status -/// payload already carries: the gap between the replica's own peak and what the node's OWN peers -/// announced. +/// Evidence that the replica is following the chain RIGHT NOW: the two heights whose gap +/// establishes it, held together. +/// +/// It exists as a TYPE rather than a predicate so the heights travel with the verdict. A `bool` +/// answer can be paired with any peak a caller happens to read separately, and that is exactly how +/// `{phase: "synced", peak_height: null}` was reachable (dig-node#495) — a positive claim of +/// currency beside a refusal to say what height it is current AT. +/// +/// **An unmeasured height on EITHER side yields no evidence.** The predicate this replaces answered +/// `true` whenever either was `None`, on the reasoning that an unobservable gap is not an +/// accusation. That reasoning is REVERSED here, and it must not be left standing as though it still +/// held: it is sound only about the PEER side, and only for a verdict that is the absence of an +/// accusation. +/// +/// - **No PEER height** — there is no second opinion. Fairly read as no accusation, but equally it +/// is no evidence FOR currency: a replica thousands of blocks behind reports the same thing +/// (dig-node#416, measured at 8,380 blocks behind). +/// - **No REPLICA height** — the subject of the claim has no measurement whatsoever, which supports +/// no verdict in either direction. Production-reachable: [`super::db::WalletDb`]'s latch-over path +/// sets `initial_sync_complete` without ever writing a peak. /// -/// `None` on either side is UNOBSERVABLE, and an unobservable gap is never an accusation — it -/// answers `true` and leaves the phase exactly as it was before this existed. That matters because -/// the peer tier is genuinely unmeasured on a node whose chain transport has not been built, and a -/// missing measurement must not be spent as evidence against the replica. +/// A phase is not the absence of an accusation. [`SyncPhase::Synced`] ASSERTS currency, so it needs +/// evidence for itself; [`SyncPhase::Syncing`] already covers both unmeasured cases in its own +/// words — the replica is otherwise not both caught up AND currently following the chain. /// /// See [`FOLLOWING_TOLERANCE`] for why the slack is small and which way it is allowed to be wrong. -pub(crate) fn is_following(replica: Option, peers: Option) -> bool { - match (replica, peers) { - (Some(replica), Some(peers)) => peers.saturating_sub(replica) <= FOLLOWING_TOLERANCE, - _ => true, +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct FollowingEvidence { + replica_peak: u32, + peer_peak: u32, +} + +impl FollowingEvidence { + /// The ONLY constructor: `None` when either height is unmeasured, or when the measured gap + /// exceeds [`FOLLOWING_TOLERANCE`]. + pub(crate) fn measure(replica: Option, peers: Option) -> Option { + let (replica_peak, peer_peak) = (replica?, peers?); + (peer_peak.saturating_sub(replica_peak) <= FOLLOWING_TOLERANCE).then_some(Self { + replica_peak, + peer_peak, + }) + } + + /// The replica height this evidence was drawn from — the height a `Synced` claim is a claim + /// ABOUT, so it is reported from here rather than re-read from a separate field. + pub(crate) fn replica_peak(self) -> u32 { + self.replica_peak + } +} + +/// The phase and the replica peak, produced TOGETHER so neither can be chosen without the other. +/// +/// The privacy boundary is the whole mechanism, and it is a MODULE rather than a struct because +/// Rust privacy is module-scoped: private fields are visible to the rest of their own module, so a +/// struct declared beside [`SyncHandle::status`] could still be built by a literal there, with any +/// pairing at all. Declared here, [`settled::SettledPhase`]'s fields are unreachable from the +/// parent, so its constructors are the only way to make one — and [`settled::SettledPhase::synced`] +/// is the only route to [`SyncPhase::Synced`] in this crate's non-test code (dig-node#495). +mod settled { + use super::{FollowingEvidence, SyncPhase}; + + /// A phase and the replica peak reported beside it. + pub(super) struct SettledPhase { + phase: SyncPhase, + peak_height: Option, + } + + impl SettledPhase { + /// The ONLY route to [`SyncPhase::Synced`]: it demands the evidence, and takes the reported + /// height FROM that evidence rather than from a separately read field. A caller therefore + /// cannot claim currency without naming the height it is current at. + pub(super) fn synced(evidence: FollowingEvidence) -> Self { + Self { + phase: SyncPhase::Synced, + peak_height: Some(evidence.replica_peak()), + } + } + + /// No peer has ever attached. The DB's peak — known or not — is still reported honestly. + pub(super) fn not_started(peak_height: Option) -> Self { + Self { + phase: SyncPhase::NotStarted, + peak_height, + } + } + + /// Not both caught up AND currently following. Every phase other than `Synced` reports the + /// replica's peak exactly as the DB holds it, `None` included: withholding a claim is not a + /// reason to withhold a measurement. + pub(super) fn syncing(peak_height: Option) -> Self { + Self { + phase: SyncPhase::Syncing, + peak_height, + } + } + + /// No wallet is enrolled, so there is nothing to follow. + pub(super) fn no_wallet_enrolled(peak_height: Option) -> Self { + Self { + phase: SyncPhase::NoWalletEnrolled, + peak_height, + } + } + + /// A wallet is enrolled but its addresses are not being watched. + pub(super) fn wallet_not_unlocked(peak_height: Option) -> Self { + Self { + phase: SyncPhase::WalletNotUnlocked, + peak_height, + } + } + + pub(super) fn phase(&self) -> SyncPhase { + self.phase + } + + pub(super) fn peak_height(&self) -> Option { + self.peak_height + } } } @@ -595,9 +709,10 @@ pub async fn status_without_supervisor( tier: super::fallback::ChainPeerTier, ) -> sqlx::Result { let state = db.sync_state().await?; + let settled = settled::SettledPhase::not_started(state.peak_height); Ok(WalletSyncStatus { - phase: SyncPhase::NotStarted, - peak_height: state.peak_height, + phase: settled.phase(), + peak_height: settled.peak_height(), chia_peer_count: tier.peer_count, // No supervisor is attached, so nobody is holding a subscription session to count. subscription_peer_count: None, diff --git a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs index ff67e9e8..efccedbe 100644 --- a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs +++ b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs @@ -1124,20 +1124,16 @@ async fn phase_is_syncing_when_caught_up_but_no_peer() { // A peer whose trust is unresolved may not write, and a non-writing session cannot be synced // (dig_ecosystem#2666). This fixture is about the PEER COUNT, so give it a corroborated peer. handle.set_trust(true); + // The tier is OBSERVABLE and level with the replica, so the SUBSCRIPTION peer count is the + // only axis varied below. An unobservable tier would withhold the synced claim for its own + // reason (dig-node#495), and this test would stop being about the peer count at all. assert_eq!( - handle - .status(&db, ChainPeerTier::UNOBSERVABLE) - .await - .unwrap() - .phase, + handle.status(&db, tier_at(6_000_000)).await.unwrap().phase, SyncPhase::Synced ); handle.set_connected(0); - let status = handle - .status(&db, ChainPeerTier::UNOBSERVABLE) - .await - .unwrap(); + let status = handle.status(&db, tier_at(6_000_000)).await.unwrap(); assert_eq!( status.phase, SyncPhase::Syncing, @@ -1164,22 +1160,17 @@ async fn phase_ladder_not_started_syncing_synced() { handle.set_connected(1); handle.set_trust(true); + // Level with its peers throughout, so the LATCH is the only axis varied across the rungs + // (dig-node#495: an unmeasured height withholds `Synced` on its own). + db.set_peak(6_000_000, "aa").await.unwrap(); assert_eq!( - handle - .status(&db, ChainPeerTier::UNOBSERVABLE) - .await - .unwrap() - .phase, + handle.status(&db, tier_at(6_000_000)).await.unwrap().phase, SyncPhase::Syncing ); db.force_initial_sync_complete_for_test(true).await.unwrap(); assert_eq!( - handle - .status(&db, ChainPeerTier::UNOBSERVABLE) - .await - .unwrap() - .phase, + handle.status(&db, tier_at(6_000_000)).await.unwrap().phase, SyncPhase::Synced ); } @@ -1421,6 +1412,9 @@ async fn a_previously_synced_wallet_restarted_locked_is_not_reported_as_synced() async fn a_completed_catch_up_still_reports_synced_while_watching_addresses() { let db = WalletDb::open_in_memory().await.unwrap(); db.force_initial_sync_complete_for_test(true).await.unwrap(); + // Both heights measured and level: a control for the arm ordering must not be prevented from + // reaching `Synced` by the unmeasured-height rule (dig-node#495), or it stops being a control. + db.set_peak(FROZEN_REPLICA_PEAK, "aa").await.unwrap(); let (handle, _rx) = SyncHandle::new(); handle.set_connected(1); @@ -1429,7 +1423,7 @@ async fn a_completed_catch_up_still_reports_synced_while_watching_addresses() { assert_eq!( handle - .status(&db, ChainPeerTier::UNOBSERVABLE) + .status(&db, tier_at(FROZEN_REPLICA_PEAK)) .await .unwrap() .phase, @@ -4302,9 +4296,11 @@ async fn one_host_is_one_voice_however_many_ports_it_answers_on() { /// watched addresses put the fixture squarely on the path that actually reaches `Synced`, which is /// the only path this ticket is about. /// -/// The peer tier is `UNOBSERVABLE` so [`is_following`] answers `true` and cannot be the thing that -/// fails the assertion: an unmeasured chain peak already returns the phase unchanged, so a fixture -/// carrying a lagging tier peak would go green against a node that never learned about refusal. +/// The peer tier is OBSERVABLE and LEVEL with the replica so the refusal is the only thing that +/// can fail the assertion. It was `UNOBSERVABLE` until dig-node#495, on the reasoning that an +/// unmeasured chain peak returned the phase unchanged; that is no longer true - an unmeasured +/// height now withholds `Synced` by itself, which would have made this test pass against a node +/// that never learned about refusal at all. A lagging tier peak would be vacuous the same way. /// /// What the user saw before: `{phase: "synced", peak_height: }` on a node whose every /// frame is dropped before any DB write — unbounded, invisible staleness reported as settled. @@ -4313,6 +4309,7 @@ async fn a_refused_writer_is_not_reported_as_synced() { let db = WalletDb::open_in_memory().await.unwrap(); // The catch-up genuinely completed in an earlier run; the flag is persistent. db.force_initial_sync_complete_for_test(true).await.unwrap(); + db.set_peak(FROZEN_REPLICA_PEAK, "aa").await.unwrap(); let (handle, _rx) = SyncHandle::new(); handle.set_connected(1); @@ -4322,7 +4319,7 @@ async fn a_refused_writer_is_not_reported_as_synced() { handle.set_watched(3, true); let status = handle - .status(&db, ChainPeerTier::UNOBSERVABLE) + .status(&db, tier_at(FROZEN_REPLICA_PEAK)) .await .unwrap(); assert_ne!( From 1e6c10ce1f8d5d1666d86ff8c1162176b614a771 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 1 Sep 2026 22:28:13 -0700 Subject: [PATCH 4/4] test(wallet): re-point the mid-catch-up test at its own property `an_enrolled_wallet_mid_catch_up_still_reports_syncing` ran against an UNOBSERVABLE peer tier and a db with no recorded peak. Before this PR the `is_following(None, None)` answer was `true`, so an unfinished catch-up was the only input that could produce `Syncing` and the test discriminated. This PR made both of those inputs independently decisive, and the test became vacuous with respect to #2609: it stayed green with `state.initial_sync_complete` deleted from the `Synced` arm. Give it a MEASURED peer tier LEVEL with a recorded replica peak, as its four siblings already have, so every other route to `Syncing` is closed. Measured both ways: green with the clause removed under the old fixture, red under the new one. No production code changed. Co-Authored-By: Claude --- crates/dig-wallet/src/sage/sync_supervisor/tests.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs index efccedbe..bc26b1e8 100644 --- a/crates/dig-wallet/src/sage/sync_supervisor/tests.rs +++ b/crates/dig-wallet/src/sage/sync_supervisor/tests.rs @@ -1593,16 +1593,26 @@ async fn dropping_a_session_clears_its_subscription_facts() { /// `Syncing`. /// /// The new variant must not swallow the genuine catching-up case it sits next to. +/// +/// FIXTURE DESIGN - the peer tier is OBSERVABLE and LEVEL with a recorded replica peak. It was +/// `UNOBSERVABLE` against a db with no peak until dig-node#495, when an unmeasured chain height and +/// an unmeasured replica height each became independently sufficient to withhold `Synced`. Those +/// two inputs were inert here before and are decisive now, so the old fixture reported `Syncing` +/// even with the `initial_sync_complete` clause deleted from the `Synced` arm. Levelling both +/// heights closes every other route to `Syncing` and re-points the test at the unfinished +/// catch-up, which is the only property it names. #[tokio::test] async fn an_enrolled_wallet_mid_catch_up_still_reports_syncing() { let db = WalletDb::open_in_memory().await.unwrap(); + db.set_peak(FROZEN_REPLICA_PEAK, "aa").await.unwrap(); + let (handle, _rx) = SyncHandle::new(); handle.set_connected(1); handle.set_trust(true); handle.set_watched(3, true); let status = handle - .status(&db, ChainPeerTier::UNOBSERVABLE) + .status(&db, tier_at(FROZEN_REPLICA_PEAK)) .await .unwrap(); assert_eq!(status.phase, SyncPhase::Syncing);