fix(wallet): distinguish unknown from zero across the wallet surface - #454
fix(wallet): distinguish unknown from zero across the wallet surface#454MichaelTaylor3d wants to merge 5 commits into
Conversation
Adds the staleness gap to `control.wallet.balance` (`network_peak_height`, `stale_by`, both additive) and makes the human line say what it knows: an unreadable balance renders `unknown` rather than a confident `0`, and a non-current figure is marked NOT CURRENT with its as-of height and its distance from the network. Also adds the missing caller for the funded latch, so a funded auto-created wallet stops being described as disposable. Refs #416 #286
#306: the Sage-parity coin read returned an empty set for ANY CAT while unsynced, so a real $DIG holder read as holding none. Wired to the same asset-scoped hint read the balance already uses. #256: increase_derivation_index returned the shared empty ActionResponse, so a clamped or zero-row write was indistinguishable from success -- and the derivation floor decides which addresses this node scans. It now reports the floors in force, and a zero-row update is an error. #384: control.wallet.resetCoinDb + `dign wallet reset-coin-db --confirm`. Clears the authoritative flag in the SAME transaction as the coins, refuses while a spend is in flight, and touches no key material. Refs #306 #256 #384
…ded latch SPEC gains the control.wallet.resetCoinDb row (the same-transaction clearing of the authoritative flag, the expiry-not-presence refusal, the master tier), the balance row gains network_peak_height and stale_by with the rule that an absent gap and a zero gap are opposite claims, and 16.4's NOT YET SATISFIED block is replaced by the observation-point contract now that a caller exists. Bumps dig-wallet 0.43.0 -> 0.44.0 and the workspace 0.189.0 -> 0.190.0: new capability, every wire change additive. Refs #416 #286 #384
…sumer is The contract-conformance gate refused a served-but-unpublished method, and its sanctioned escape -- KNOWN_UNPUBLISHED -- is deliberately the SAME constant the token gate reads, so tolerating the publish drift and granting a paired token access are one decision. Weighed rather than inherited: #384 exists to put a reset button in the DIG App, and the App holds a paired token, so reserving this to the master token would make the feature unreachable by the only consumer it was built for. What bounds a destructive method here is loopback-only + a token + confirm:true on the wire + a refusal while a spend is in flight + a blast radius holding no key material, not tier alone. This also restores the master-tier drift assertion to equality: the containment relaxation it needed is no longer required. Refs #384
loop-security — IN PROGRESS, not the verdictAudited head: Confirmed clean so far
FINDING 1 — string-continuation damage IS present, in two user-facing messages (MEDIUM)The hazard the brief warned about did not miss this diff. Two distinct corruptions, both in strings a user sees: 1a. This is the line-continuation 1b. This string is not decorative — Two test assertion messages carry the same 14-space runs ( Not a security defect — no money lie, no attacker leverage. Recorded as MEDIUM because 1a/1b are user-facing product text on a destructive operation's only two failure paths, and the brief asked specifically. Continuing: #384 atomicity under concurrency, the open-read amplification question on |
loop-security gate — IN PROGRESS, not the verdictAuditing head Finding 1 — MANGLED LINE-CONTINUATION IN THE DESTRUCTIVE-ACTION WARNING (LOW, cosmetic, confirmed by byte dump)
The Rust string literal contains the two-character escape What a user of This is the confirmation warning for the only destructive method in the diff, and it is rendered Not a security defect and not a lie about money — the text is accurate, only badly wrapped. Rated It is the only instance in the diff — a full scan for Hazard cleared — no
|
IN PROGRESS — not the verdict. CORRECTION to Finding 1.My previous comment said the mangled line-continuation was "the only instance in the diff." That was Finding 1 (REVISED) — nine mangled string literals; four reach a user (LOW, cosmetic, not a lie)
The other five are Not a security defect and not a money lie — every sentence is true, merely wrapped with a gap Severity LOW. Recommend fixing before merge; not gating on its own. |
IN PROGRESS — not the verdict. Finding 2, and it is the significant one.Finding 2 — #384: the reset is atomic, but an IN-FLIGHT CATCH-UP RE-ARMS
|
| t | actor | effect |
|---|---|---|
| T0 | peer session's initial_sync_with_authority |
replaying from genesis; N batches of the user's real coins are now in coins |
| T1 | user presses the #384 reset button (DIG App / dign wallet reset-coin-db --confirm) |
no spend in flight, so it proceeds: coins emptied, initial_sync_complete = 0, coverage ''. Commits. Prints "the replica is no longer authoritative and will re-sync from chain" |
| T2 | the same session receives its final batch, is_finished = true |
complete_catch_up commits initial_sync_complete = 1, covered_puzzle_hashes = <full address set>, peak_height = <peak> |
Resulting state: coins EMPTY, initial_sync_complete = 1, coverage FULL. Wallet-scoped reads
route to the local replica as authoritative and answer balance 0, synced true on a funded wallet —
verbatim the state reset_chain_cache's own doc comment names as "the money-lie class this repo already
documents" and the state this entire PR exists to make unreachable.
Impact. A synced: true zero is not "I don't know", it is "the answer is nothing" — the exact
conflation this PR's through-line is built to remove. It lands at the worst moment: immediately after a
destructive action, while the user watches to see their funds return. stale_by does not rescue it —
the replica genuinely believes it is level with the network, so the #416 NOT CURRENT prefix does not
fire either. Both new honesty surfaces read as reassuring.
It self-heals, which caps this at HIGH rather than CRITICAL: the next peer session's catch-up
replays from genesis and repopulates, because catch-up is not gated on the flag. The window is one
peer-session cycle — minutes to hours, and longest on a node with few peers, which is the node most
likely to have prompted a reset in the first place.
Secondary effect on a wallet whose baseline was never armed: complete_catch_up arms
arrival_baseline_height as COALESCE(existing, MAX(?peak, COALESCE((SELECT MAX(created_height) FROM coins), 0))).
With coins emptied that inner MAX is NULL → 0, so the baseline arms at the replay peak, and every
coin re-synced afterwards at a lower height is classified backfill rather than arrival. Arming is
once-per-wallet, so that is permanent. Lower severity, same root cause.
Cheapest correct fix — a reset epoch. Add a monotonically-increasing reset_epoch to sync_state;
reset_chain_cache bumps it inside its existing transaction; initial_sync_with_authority captures it
before its first batch and passes it into CatchUpReplay; complete_catch_up becomes
... WHERE id = 0 AND reset_epoch = ?captured, and reports zero rows updated as "superseded by a reset,
replica stays non-authoritative". A reset then invalidates any catch-up that began before it, which is
the true intent. A lock is the wrong shape — the reset would block for up to CATCH_UP_DEADLINE.
Adjacent, PRE-EXISTING, not introduced here — recording it because #384 makes it newly reachable on a
funded wallet. refresh_tracked_coins (rpc.rs:3301) is the second production writer of the flag
and its own comment states it "DOES latch over zero rows … declaring an empty table authoritative"
(dig_ecosystem#2514). Before #384 an empty coins table was mostly a fresh-install condition; #384 makes
it a user-triggerable state on a funded wallet, so #2514's window now opens on demand. That one is
defense-in-depth and a follow-up ticket, not a gate on this PR.
Still to finish: the three escalated decisions, the 8 GHAS dismissals, #390/#416 closing keywords.
DRAFT — DO NOT MERGE. The gate round has not returned.
Closes #416
Closes #306
Closes #286
Closes #256
Closes #384
Closes #390
#396 is NOT closed by this PR — see the verdict on that ticket. Its deliverable is a real
mainnet wallet holding a $DIG CAT and an NFT, which is not reachable from a worktree.
The through-line
Every ticket here is a surface that could not distinguish "I do not know" from "the answer is
zero / nothing / fine." Each fix restores that distinction rather than papering over the symptom.
0rendered identically to an empty walletPer ticket
#416 — the stale-replica zero (mvp)
Two halves; one is fixed here and one is not, stated plainly.
Fixed — making staleness legible where a person reads a balance.
control.wallet.balancenowcarries
network_peak_height(the peak this node's own held Chia peers announced) andstale_by(how far behind that peak the figure is). Both additive per §5.1, asserted against a consumer struct
that ignores them.
stale_byisNoneunless BOTH heights are known, because a zero and an absence are oppositeclaims: zero says this figure is level with the network, absence says nothing bounds this
figure. The ticket's measured reading —
balance 0, synced false, peak_height null— is thesecond, and previously had no way to say so.
The human line was the actual defect surface. It read
balance 0 · pending 0 · syncing, built fromresult["balance"].as_u64().unwrap_or(0)— so a missing field printed a confident zero balance —and
syncingreads as reassuring progress rather than as a warning. It now rendersunknownfor anunreadable field and prefixes every non-current figure with
NOT CURRENT, naming the as-of heightand the gap.
NOT fixed — "establish why the replica is not catching up." That is a diagnosis on a specific real
machine's node, which this lane does not have and did not re-measure. Unverified; needs a lane
with the host.
#306 — CATs read as zero while unsynced
wallet_coins's fallback arm didreturn Ok(Vec::new())for any CAT. Its stated blocker —"CAT asset attribution while syncing needs puzzle uncurrying" — does not exist: a CAT coin is
identified by where it sits. Wired to
asset_scoped_fallback_coins, the same helperbalance_for_addressandcoins_for_addressalready use, rather than re-derived — the balance andthe coin list behind it must not be able to scope to different assets.
The ticket's spend-selector warning does not apply as written, and this is worth recording. It
cites
rpc.rs:2933,:3319as spend-input selection reached throughwallet_coins. On today's treewallet_coinshas exactly two callers,get_coinsandget_spendable_coin_count; those linenumbers are now unrelated code. So this widens two READS and does not change what a spend can select.
#286 — the latch nothing called
latch_ever_fundedwas written, persisted and tested, with no production caller. A newwallet_fundedmodule holds the decision as a pure function; the mirror pass is the observationpoint because it already reads the operator wallet's balance on a timer.
Deviation from the ticket's wording, taken deliberately. #286 says "if it is unclear whether
funds were observed, latch." Implemented literally, that latches on
CannotSay— which is the stateevery node is in on its first pass, so every auto wallet would latch immediately and
is_disposablewould be vacuouslyfalsefor ever. That is the exact vacuity the ticket's own bodycites as the pattern to avoid.
The instruction's purpose survives without that cost, because nothing ever records "not funded":
the latch is monotonic, so declining to latch on an unknown defers a decision rather than making the
wrong one, and the next observation that sees money latches. The direction that matters is covered
without a currency gate — a non-zero figure classifies as funded from either tier, so a stale or
fallback answer showing money latches at once.
syncedgates only the zero case. SPEC §16.4'sNOT YET SATISFIEDblock is deleted and replaced with the observation-point contract.#256 — a no-op indistinguishable from success
Enumerated from the dispatch table: 19 sites, not the four the comment implied.
Only
increase_derivation_indexis changed. Its no-op is reachable and money-class: the write isMAX(col, ?), so a request below the floor changes nothing by design, andWHERE id = 0against anabsent settings row updates zero rows while
executereturnsOk— the floor is never raised,the operator is told it was, and funds at higher indices stay invisible with no error and no retry.
It now returns the floors in force, per tree, with
Nonefor a tree not asked about — neverSome(0), which would assert that tree scans nothing. Modelled onChiaPeerRemovalOutcome: numbersa consumer must read, no
boolcompanion. A zero-row update is an error.The other 18 are documented rather than changed, per the ticket's Sage-parity constraint: the
settings/theme/peer/metadata writes have no observable difference between "changed it" and "it
already said that", so there is nothing a richer response could truthfully report.
redownload_nft/update_nfton an unknown id are named as the real remaining candidates, gated onestablishing Sage's own response shape first.
#384 — reset the coin database (mvp, kind:business)
control.wallet.resetCoinDbplusdign wallet reset-coin-db --confirm.initial_sync_completeand the recorded coverage arecleared in the same transaction that empties the coins, so a crash between them cannot leave an
empty replica that is still authoritative — the
balance 0, synced truestate.during a pre-check cannot slip the gap. Liveness is judged by expiry against the node's own
clock, not by row presence: one lapsed unpruned hold must not permanently deny the only recovery
this feature provides. The instant is not caller-supplied — that would be a lapse oracle.
refused: truefieldwould read "your cache was reset" and act on it.
user theme survives while the coins do not.
confirm: truetravels on the WIRE, not asserted in the CLI — a guard only the CLI applies isnot a guard.
#390 — ALREADY SHIPPED, not rebuilt
The §2.0 already-shipped check found this ticket's design merged in
4523894(PR #393) and wiredon both paths —
stage_from_statesatsync.rs:772,promote_staged_catsatsync.rs:833andrpc.rs:3281,cat_admission_pendingatdb.rs:421, 18 tests. Full evidence on the ticket. TheClosesabove is bookkeeping.Blast radius
.gitnexusis registered against the primary checkout and is stale, soimpactwould return afalse-safe zero. Done by grep plus direct read instead, and stated as such:
balance_wire— 4 call sites, all in this file (1 production, 3 tests). All updated.raise_derivation_floor— 2 production callers, both inactions.rs; 2 test callers indb.rs.Return type
()tou32; all updated.increase_derivation_index— 1 production caller (rpc.rs), reachable only via the Sage-paritydispatch. Response widened additively.
wallet_coins— exactly 2 callers,get_coinsandget_spendable_coin_count. Both reads.latch_ever_funded— had zero callers; now one.reset_chain_cache,reset_coin_db,wallet_reset_coin_db,stale_by,FundingObservation,amount,balance_freshness) have no prior callers.A tier decision worth the gate's attention, and a CI finding that forced it.
control.wallet.resetCoinDbis destructive, so the master token looked right, andrequires_master_tokenfails CLOSED for an unpublished name — it landed there by default. CIrefused it:
the_contract_publishes_every_control_method_the_node_servesrejects aserved-but-unpublished method, and its sanctioned escape,
KNOWN_UNPUBLISHED, is deliberately thesame constant the token gate reads — so tolerating the publish drift and granting a paired token
access are ONE decision, by design.
That forced the question rather than allowing a default, and the default was wrong: #384 exists to
put a reset button in the DIG App, and the App holds a paired token. Master-tiering it makes the
feature unreachable by the only consumer it was built for — a guard so tight it removes the
capability is a deletion, not a guard. It is therefore on the paired tier, recorded in
KNOWN_UNPUBLISHED_CONTROL_METHODSwith the reasoning and a removal condition.What bounds the damage is the combination the method does enforce: loopback-only + a token +
confirm: trueon the wire + a refusal while any spend is in flight + a blast radius containing nokey material and nothing a re-sync cannot rebuild. Not tier alone. Flagged explicitly for the
gate — if the reviewer judges a destructive method must be master-tier regardless of reachability,
that is a coherent position and it means #384 cannot ship until the contract publishes.
An interim change is REVERTED and is not in the diff: the master-tier drift assertion was briefly
relaxed from equality to containment, and equality is restored.
Required cross-repo sibling (§1.3b / §4.1):
dig-node-control-interfacemust publishcontrol.wallet.resetCoinDb. Until it does, the method sits inKNOWN_UNPUBLISHED, andthe_unpublished_list_still_describes_real_driftfails the moment it IS published unless the entryis removed — so this cannot rot. Needs an orchestrator-dispatched lane.
Dependencies (§2.4b)
Checked against
index.crates.iowith the requiredUser-Agent. Everydig-*andchia-*declaration in
dig-walletis already at latest —dig-node-control-interface0.27.0,dig-offers0.3.0,dig-clvm0.4.0,dig-keystore0.13.0,chia-query0.20.0. Thechia-*setmoves together and sits uniformly at 0.36.1 / 0.36.0, the stated ceiling pending
chia-wallet-sdkpublishing against 0.48. No bumps were owed; none were made.SemVer
minor —
dig-wallet0.43.0 to 0.44.0, workspace 0.189.0 to 0.190.0. New capability, andevery wire change is additive (§5.1): new fields on the balance result, a new control method and CLI
verb, a widened
increase_derivation_indexresponse.raise_derivation_floorandactions::increase_derivation_indexchanged return type, but both are internal to the crate.SPEC
control.wallet.resetCoinDbrow added with the same-transaction requirement, theexpiry-not-presence refusal rule and the master tier; the balance row extended with
network_peak_height/stale_byand the zero-is-not-absence rule; the CLI verb mapping added;§16.4's
NOT YET SATISFIEDblock replaced with the observation-point contract.