fix(wallet): a synced phase must carry the height that bounds it - #500
Conversation
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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
…onesty # Conflicts: # Cargo.lock # Cargo.toml
|
Independent correctness gate — IN PROGRESS, not the verdict. Head read: Confirmed so far, by grep + direct read (gitnexus index for this repo is ~301 commits stale and returns a false-safe
Still running: the gate-rot revert-proofs on the four amended sibling tests, the sweep for unamended tests now vacuous, the wire-shape check, and the |
loop-security — IN PROGRESS, not the verdictAuditing Resolved so far — no defect in these1. Provenance of the untrusted input (
The security-relevant property, stated in that crate's own doc at
2. Question 3 — the money consumer is not reachable in the dangerous direction. So this PR's effect on that path — making 3. Question 5, first half — 4. Still open
gitnexus is stale for this repo, so all of the above is grep + direct read, as briefed. |
loop-security — IN PROGRESS, not the verdict (2/2 before the verdict)Head Q1 — can a hostile peer MANUFACTURE a
|
|
IN PROGRESS — not the verdict. Gate-rot revert-proofs, executed at Baseline: Method: three single-clause mutations of the
So two of the four amendments are proven to have restored real discrimination — in particular the dig_ecosystem#2666 refusal test does now fail against its own defect, which was the headline risk. The third is a finding and is detailed in an inline comment. Still running: whether ANY test in the 766 catches the dropped peer-count clause, and the RED-first proof of the new tests at |
loop-security: PASSAudited head: gitnexus's index for this repo is ~301 commits stale and returns a false-safe Verdict by area
1. This PR removes an attack primitive (the security value, stated plainly)At
2. Q1 — a hostile peer cannot force the figure DOWNForging 3. Q2 — MEDIUM, pre-existing, NOT gating: an unbounded peer peak is a permanent denial primitiveA single Effect at this head: This PR does not introduce it. At The shape of the gap is worth recording: dig-wallet already bounds its OWN replica peak — Recommended follow-up ticket against No NEW attacker-controllable suppression. The one arm this PR flips ( 4. No amplification — the result I most expected to be wrong aboutI expected a wrongly-
Nothing retries, re-dials or tears down a session on the phase either: 5. Q3 — the money consumer cannot be driven either way
So this PR's effect on that path — 6. Q5 —
|
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
VERDICT: PASS
Head reviewed: 5d3f25ee6fad4c8f7061b052c40f1100f604bc01 (re-resolved from gh pr view 500 --json headRefOid, not from the dispatch brief). Independent correctness gate, fresh context, no code edited. gitnexus was NOT used — its index for this repo is ~301 commits stale and returns a false-safe impactedCount: 0; everything below is grep, direct read, or execution.
All work ran in a private worktree at C:/tmp/worktrees/gate495 cut from this SHA. The PR's own worktree was never mutated; the private one was restored to 5d3f25e with an empty git status --porcelain and has been removed.
Verified by EXECUTION
Baseline. cargo test -p dig-wallet --lib gave 766 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 16.80s.
The new tests are genuine revert-proofs, not decoration. Checked out 40bcabc (the tests-only RED commit, before a465d83 applied the fix) and ran each by exact name. All four FAILED there:
a_replica_with_no_peak_of_its_own_is_never_reported_as_syncedgave0 passed; 1 failed; 766 filtered outa_replica_with_no_second_opinion_is_never_reported_as_syncedgave0 passed; 1 failed; 766 filtered outa_synced_phase_always_carries_the_heights_that_bound_itgave0 passed; 1 failed; 766 filtered outan_unmeasured_height_on_either_side_withholds_the_synced_claimgave0 passed; 1 failed; 766 filtered out
That settles point 2: the cross-product test discriminates against the pre-fix implementation, and its non-vacuity counter (synced_reached > 0) is asserted and does fire — a run in which no combination reached Synced would fail on that assertion, and the test passes at head.
Gate-rot proofs. Three single-clause mutations of the Synced guard in SyncHandle::status (sync_supervisor.rs:490-497), each deleting one conjunct from the filter state.initial_sync_complete && observed.peers >= 1 && observed.session_may_write:
| mutation | test expected to go red | result |
|---|---|---|
drop state.initial_sync_complete |
phase_ladder_not_started_syncing_synced |
FAILED — correct |
drop observed.session_may_write |
a_refused_writer_is_not_reported_as_synced |
FAILED — correct |
drop observed.peers >= 1 |
phase_is_syncing_when_caught_up_but_no_peer |
still passed (see finding 1) |
The headline risk — dig_ecosystem#2666's refusal test going silently vacuous — is genuinely repaired: with session_may_write deleted the test fails, which it would not have done had the fixture kept ChainPeerTier::UNOBSERVABLE.
Whole-suite sensitivity to the un-caught clause. Running the FULL 766 under the drop-peer-count mutation gave 765 passed; 1 failed — sage::sync_supervisor::tests::stall_evidence_survives_the_end_of_a_session catches it. So the clause is covered by the suite; it is only the test named for it that does not discriminate on it. That is why finding 1 is non-gating.
the_following_tolerance_holds_at_the_bound_and_fails_one_beyond_it is untouched by the diff and passes at head and under all three mutations.
Verified by READING
- The synced-with-null-height pairing is unreachable on every emitting path. The struct literal is written at exactly two sites, both in
sync_supervisor.rs(:500,:713), and both take phase and peak from asettled::SettledPhase. In non-test codeSyncPhase::Syncedis written in exactly one place,sync_supervisor.rs:652, insideSettledPhase::synced, which takes its height fromFollowingEvidence::replica_peak()rather than fromstate.peak_height. Nothing indig-node-serviceor elsewhere in the workspace constructs the struct. The module-privacy argument in the doc is correct: fields private tomod settledare unreachable from the parent, so a literal besideSyncHandle::statuscannot bypass the constructors. - Arm order preserved (point 5).
NotStarted, then thewatched == Some(0)branch (WalletNotUnlocked/NoWalletEnrolled), thenSynced, thenSyncing— unchanged. TheSyncedguard moved from an and-chain intomeasure(..).filter(..), which is the same conjunction.StallWatch,SESSION_MAX_LIFETIME,STALL_AFTERand session teardown are untouched by the diff; the onlysync.rschanges are two test call sites and a doc-link rename. replica_answer_is_currentis behaviour-identical on both arms (point 5). Old: absent replica height givesfalse; absent peer height givesfalse; otherwise the tolerance test. New:FollowingEvidence::measure(replica, peer).is_some(), which isNoneon either absent height and otherwise the same tolerance test. Same truth table.- No wire change (point 6). The only line the diff touches inside
declare_sync_phases!is the rustdoc aboveNoWalletEnrolled => "no_wallet_enrolled"; the token, the variant set,ALL,as_wireand the serde attributes are unchanged.control.rsis not in the diff at all, sowallet_sync_statusstill emits the same six fields (control.rs:2566-2572). - The consumer moves conservatively (point 7).
server.rs:2843derives itssyncedflag fromphase == Synced, which this change makes strictly harder to reach, so cases migrate fromObservedEmptytowardCannotSayinwallet_funded.rs:39. TheFundedbranch is decided by the balance beforesyncedis consulted, so a funded wallet cannot become describable as disposable by this change; the latch is monotonic, so the only effect is that a genuinely empty wallet latches later rather than wrongly. - The
NoWalletEnrolleddoc edit is honest (point 8). The arm's behaviour is unchanged, and the new text explicitly withdraws the currency claim the old sentence made and says why requiring currency there would regress dig_ecosystem#2609. It now describes exactly what the arm checks. The rewrittenrpc.rs:1072-1089passage scopes its unrepresentability claim to that path, which is true as written. - Craft and coverage (point 9).
FollowingEvidenceandmod settledread cleanly, names carry intent, and the comments explain why rather than what. Coverage moves up: roughly 199 added test lines against roughly 60 added production lines, and everySettledPhaseconstructor is exercised by the cross-product grid.
Non-gating findings
None of these blocks the merge, and no review thread is opened for them, so required_conversation_resolution stays clear. Ranked.
1. phase_is_syncing_when_caught_up_but_no_peer does not discriminate on the axis it is named for — crates/dig-wallet/src/sage/sync_supervisor/tests.rs:1124-1141.
Deleting observed.peers >= 1 from the Synced guard leaves it green. The reason is that set_connected(0) also clears trust, so the second assertion is carried by observed.session_may_write rather than by the peer count. This is pre-existing rather than introduced here — the same held before the amendment — and the clause is caught elsewhere by stall_evidence_survives_the_end_of_a_session, which is why this is not gating. If it is ever tightened, the cheap fix is to re-assert trust after set_connected(0) so the peer count is the only failing conjunct. The fix must NOT be to relax the assertion.
2. an_enrolled_wallet_mid_catch_up_still_reports_syncing is now vacuous with respect to its own property — crates/dig-wallet/src/sage/sync_supervisor/tests.rs:1596-1608.
Its fixture sets neither a replica peak nor a peer tier, so under the new rule it reaches Syncing for the unmeasured-height reason; deleting state.initial_sync_complete leaves it green (confirmed under the drop-latch mutation). Before this change it would have gone red. This is precisely the class the four amendments were made for, and it was missed by the sweep. It is non-gating only because the property it names is still guarded by the amended phase_ladder_not_started_syncing_synced, which does fail under that mutation. Fix, if taken: give it a measured peak and a level tier_at(..), matching what the four siblings received.
3. The struct's fields are pub and SyncPhase::Synced is a public variant, so the pairing is unrepresentable along the producing path but not in the TYPE — crates/dig-wallet/src/sage/sync_supervisor.rs:330-335. No consumer constructs one today (checked across the workspace), and the doc correctly scopes its claim to this crate's non-test code, so nothing is wrong as merged. Worth considering privatising the two fields behind accessors, or #[non_exhaustive], if the invariant is meant to survive a future consumer.
4. Minor: replica_answer_is_current lost a short-circuit — crates/dig-wallet/src/sage/rpc.rs:1088-1093. The old form returned early without awaiting chain_peer_tier() when the replica peak was absent; the new form always awaits it. peer_tier() is a local read (fallback.rs:147 defaults to UNOBSERVABLE), so this is a cost note, not a correctness one, and the returned value is unaffected.
`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 <noreply@anthropic.com>
Scoped re-gate of
|
VERDICT: PASS — scoped re-gate of
|
| run | cargo test -p dig-wallet --lib an_enrolled_wallet_mid_catch_up_still_reports_syncing |
|---|---|
| mutated (conjunct deleted) | running 1 test → test result: FAILED. 0 passed; 1 failed; 766 filtered out |
| reverted (head as pushed) | running 1 test → test result: ok. 1 passed; 0 failed; 766 filtered out |
Test count is 1 in both directions, so this is not the exit-0/zero-tests-matched trap. The mutation is killed; the test now discriminates the property it names. The lane's claimed mutation proof is real and I reproduced it independently rather than taking it on trust.
3. Housekeeping
The added doc block (tests.rs:1596-1602) states WHY the fixture changed — which inputs were inert before the PR and decisive after — so the next reader cannot re-loosen it by accident. That is the right record for a fix that VACATED a test by relocating enforcement, and it reads cleanly (§2.5).
No findings. No threads opened by this pass. Nothing here bars merge; the orchestrator owns the merge, the version reconciliation and the required-check assertion by name.
…to 0.249.0 Brings origin/main (0.245.0, including #467, #489, #492, #497) onto the branch and sets the workspace version to the pre-assigned 0.249.0. Conflicts and how they were resolved: - `Cargo.toml` — a pure version collision (branch 0.242.0 vs main 0.245.0). Every other main-side hunk was already applied by the auto-merge; the only difference from `origin/main` in this file is the version line, now 0.249.0. - `Cargo.lock` — taken wholesale from `origin/main`, then re-locked with `cargo update -w`, which re-points the two workspace members whose manifests moved (`dig-node-service` 0.245.0 -> 0.249.0, `dig-wallet` 0.47.0 -> 0.48.0). Nothing in the tree still reads 0.242.0. - `crates/dig-wallet/src/sage/rpc.rs` — reported as a conflict by an earlier attempt; on this merge git resolved it textually because the two sides touch disjoint regions of the file. The result was read against BOTH parents rather than accepted on git's word: * MAIN's hunks are intact. `is_definitive_rejection` keeps the #497 narrowing — a refusal frees inputs only when its stated reason is bundle-intrinsic (`super::chain::refusal_is_bundle_intrinsic`), with a HOLD default — and the #492 doc block stating that `synced` is a CURRENCY test computed independently of the routing tier, so `{source: "db", synced: false}` is a reachable state. * THE BRANCH's hunk is intact. `replica_answer_is_current` still delegates to `sync_supervisor::FollowingEvidence::measure`, which withholds the evidence when EITHER the replica or the peer height is unmeasured, so a `synced` phase cannot be emitted without the peak height that bounds it (#495). * No rival implementation survives the merge. The pre-#495 `is_following` predicate is gone from the tree; `FollowingEvidence` is the single producer consumed by both the money reads (`rpc.rs:1085`) and the status endpoint (`sync_supervisor.rs:490`), which is what makes the `{phase: "synced", peak_height: null}` pairing unrepresentable rather than merely unlikely. No behaviour was chosen over the other side: both guards are load-bearing on different questions — one on whether a refusal may free inputs, the other on whether a currency claim may be made at all. dig-wallet: 772 passed, 0 failed, 1 ignored. dig-node-service: 774 passed, 0 failed. Note: `cargo test` on this Windows host needs RUST_MIN_STACK raised (default hits a rustc STATUS_STACK_BUFFER_OVERRUN ICE while encoding dig-node-service metadata) — an environment limit, not a code fault. Co-Authored-By: Claude <noreply@anthropic.com>
Merged
|
…47.1) Co-Authored-By: Claude <noreply@anthropic.com>
|
Orchestrator |
1 participant
DO NOT MERGE — gate round in progress.
Closes #495
What was wrong
control.wallet.syncStatuscomposed its payload from two values chosen independently:phase,from the arm ladder in
SyncHandle::status, andpeak_height, read straight offSyncState.Nothing tied them together, so
{phase: "synced", peak_height: null}— a positive claim that thereplica is current, beside a refusal to say what height it is current AT — was a state the type
could hold and the code could reach.
The predicate behind that arm answered
truewhenever EITHER height was missing:Both
Nonearms are production-reachable:WalletDb::latch_synced_over_unless_reset(db.rs:1475) setsinitial_sync_complete = 1and never writespeak_height— its own doc says "This path hasreplayed nothing and has no terminal height to offer". With a may-write session attached, the
ladder emits the reported pairing.
ChainPeerTier::peak_heightisNone"until one of them says something", soa replica thousands of blocks behind reported
syncedbecause nothing could contradict it. Thatis the shape of The node's replica is ~8,380 blocks behind its own peers, and a stale-replica zero balance is indistinguishable from an empty wallet #416, whose measured lag was 8,380 blocks.
peak_height: nullis not on its own evidence of staleness — it can mean the answer simply carriesno height. What is wrong is the PAIRING with a positive currency claim.
The asymmetry that decided the fix, taken from
is_following's own doc: its permissive arm wasjustified entirely in terms of an unmeasured PEER tier — "a missing measurement must not be spent
as evidence against the replica" — and offered no justification at all for the unmeasured-REPLICA
arm. Those are different things. An absent peer height is a missing second opinion; an absent
replica height is the subject of the claim having no measurement whatsoever. And a phase is not the
absence of an accusation:
syncedASSERTS currency, andSyncing's own doc already covers bothcases — "the replica is otherwise not both caught up AND currently following the chain".
The fix — a shape change, not a value change
The claim now travels with its evidence, so the dishonest pairing has no constructor rather than
merely no fixture.
is_followingis gone. In its place,FollowingEvidence::measure(replica, peers) -> Option<Self>(sync_supervisor.rs:570-620) refuses both unmeasured arms and theout-of-tolerance case. The tolerance itself is unchanged.
mod settled(sync_supervisor.rs:622-700) holdsSettledPhase, whose fields theparent module cannot reach — module-scoped privacy is what actually buys the enforcement here, and
the module doc says so.
SettledPhase::synceddemands aFollowingEvidenceand takes thereported height FROM that evidence rather than from a separately read field.
SyncPhase::Syncednow appears exactly once in non-test construction, at
sync_supervisor.rs:652, inside thatconstructor.
SyncHandle::statusandstatus_without_supervisorbuild aSettledPhase. The arm ORDER isuntouched — it is load-bearing and heavily documented; only the
Syncedpredicate and the sourceof its height changed.
replica_answer_is_current(rpc.rs:1085) adopts the same predicate. Behaviour there isidentical — its two early-return guards were exactly the
Nonearms ofmeasure— but thenarrowing now lives in one place instead of two. Its rustdoc claimed the two endpoints could not
disagree about the same moment; that claim was FALSE when written and is true now.
SyncPhase::NoWalletEnrolled's doc claimed "This says the chain replica is current". Thatarm checks no heights at all. The doc is corrected; the arm's behaviour is deliberately NOT
changed, because requiring currency there would regress dig_ecosystem#2609 back to reporting a
default install as forever catching up.
The peer-tier-unknown case reports
syncingwithchia_peer_peak_height: nullbeside it, which iswhat distinguishes "no second opinion" from "measurably behind".
No wire change. The phase set, its spellings,
ALL,as_wire, and the six fieldscontrol.wallet.syncStatusemits are all untouched, so nodig-node-control-interfacerelease isrequired.
Why this is not cosmetic
server.rs:2843feeds this phase intoFundingObservation::classify(wallet_funded.rs:39). Azero balance under
synced: trueclassifies asObservedEmpty— a positive assertion that thewallet was observed empty — and under
falseasCannotSay. An unboundedsyncedtherefore letthe node assert an observation it was not entitled to. The latch is monotonic and nothing ever
records "not funded", so withholding the claim is strictly the safe direction.
How it was verified
RED first, at the decision, against unmodified production code:
Each of the four failed on its own assertion, and the grid failed at
replica_peak=None peer_peak=None latched=true session=Connected may_write=true watched=None wallet_enrolled=false. That RED run IS the revert-proof; no synthetic revert was needed.0 filtered outis quoted deliberately — a cargo filter that matches nothing prints0 passed; N filtered outand exits 0.GREEN:
test result: ok. 766 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out.Clippy with
-D warningsclean,cargo fmt -p dig-wallet --checkclean, andcargo check -p dig-walletclean again after mergingmain.The acceptance asks for unreachability rather than an absent fixture, so
a_synced_phase_always_carries_the_heights_that_bound_itenumerates the FULL cross-product of theladder's inputs — replica peak, peer peak, latched flag, session state, write trust, watched set,
enrolment — and asserts that a
Syncedphase implies both heights are present, over everycombination. It also counts the combinations that DID reach
Syncedand asserts that count isnon-zero, because an implication over an input space that never reaches
Syncedpasses against animplementation that never emits it.
Live control reading, 2026-09-02 03:56Z, installed node 0.206.0, read-only, service untouched:
Corroborated against an independent source at the same minute —
api.coinset.orgget_blockchain_statereported peak 9233876, delta 0. The second source is cited ratherthan the node's own word because a node reporting
syncedagainst a fabricated chain prints exactlywhat a healthy one prints. This machine is genuinely at the tip, so it is the CONTROL: it does not
exhibit the defect, and it must keep reporting
syncedafter the change. The probe is saved as.claude/scripts/corroborate-wallet-peak.shin the superproject.Tests that encoded the defect, and one that would have gone vacuous
an_unmeasured_height_leaves_the_phase_unchangedassertedSyncPhase::Syncedfor BOTH unmeasuredarms — the reported pairing, asserted as correct behaviour. It is reversed rather than deleted, and
its doc now records why the old expectation was wrong instead of quietly dropping it.
Four sibling tests had chosen
ChainPeerTier::UNOBSERVABLEas a deliberately inert don't-care axis.One matters more than the rest:
a_refused_writer_is_not_reported_as_synced(dig_ecosystem#2666)asserts that the phase is not
Synced, and its doc explicitly picked an unobservable tier so thatthe old permissive predicate "cannot be the thing that fails the assertion". After this change an
unobservable tier withholds
Syncedby itself, so that test would have stayed green against a nodethat never learned about write-refusal at all — a pass with no remaining connection to its ticket.
All four now run on a measured tier level with the replica, so the axis each test varies stays the
only thing that can fail it.
The general lesson, worth more than the four names: after a change that makes a previously-permissive
input newly decisive, the grep to run is not "which tests fail" but "which tests chose this input
precisely because it was inert".
Blast radius
Established by grep and direct read, not by gitnexus — its index for this repo is ~301 commits
stale and returns a false-safe
impactedCount: 0.is_following: two production callers (SyncHandle::status,replica_answer_is_current), two testcallers in
sync.rs, four doc references; zero references remain anywhere.SyncPhase::Synced: oneconstruction, one downstream comparison in
dig-node-service, eight test assertions.StallWatchdoes not use this predicate — session lifetime is untouched.
Boundary against #490
Checked before building.
control.wallet.coinsand its siblings (#490 / PR #492) computesyncedfrom
replica_answer_is_current, which already narrowed both arms; this ticket'sphasecame from adifferent producer, a different type and a different predicate that narrowed neither. Merging #492
would have left the pairing exactly as emittable. The only overlap is textual — both edit rustdoc in
rpc.rs— and this branch mergesmain, not the reverse.Versions
Root
0.239.0 -> 0.242.0;dig-wallet0.47.0 -> 0.48.0. Minor rather than patch: this changeswhat an existing, unchanged consumer is told about states it can already be in.
mainmoved tof1170d0(#489) mid-flight and tookdig-wallet 0.47.0, the version this branch had reserved; themerge commit resolves that, and both versions above were re-read from the files on disk afterwards
rather than from the commit log.