fix(wallet): honest peak sync flag, adaptive derivation window, coin reservation, real pending set - #295
fix(wallet): honest peak sync flag, adaptive derivation window, coin reservation, real pending set#295MichaelTaylor3d wants to merge 13 commits into
Conversation
Salvages PR #216 (loop/2762-wallet-correctness) onto current main: adaptive hardened+unhardened derivation window, in-flight coin reservation, and a real pending-transaction set. Both rebase conflicts were purely additive test blocks at the end of the db.rs and rpc.rs test modules; both sides kept. Co-Authored-By: Claude <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ing the latch `chain_peak` computed `synced` from `db.is_synced()` — the `initial_sync_complete` latch — and returned on the replica-served path without ever consulting `replica_answer_is_current`. Measured on a running node: `sync-status` reported `syncing` while `peak` reported `synced: true`, in the same process at the same moment, on a replica 1,875 blocks behind. `peak` is the endpoint a client uses to bound a confirmation, so the falsehood landed on the read that decides whether money has settled. Every read that can report `synced: true` now derives it from the one measured predicate; every other site writes the literal `false` on a fallback-tier answer. The agreement between `peak` and `syncStatus` is therefore structural rather than asserted, and the doc comment that already claimed it now states its basis. Co-Authored-By: Claude <noreply@anthropic.com>
Bumps the workspace artifact version and dig-wallet to 0.28.0 for the wallet-correctness batch, and specifies the freshness contract of `control.wallet.peak` in SPEC.md. Clears the clippy 1.98 findings the salvaged code carried (const-block assertions, `slice::from_ref`). Co-Authored-By: Claude <noreply@anthropic.com>
…riting it literally CodeQL's hard-coded-cryptographic-value rule reads a string literal flowing into a password parameter as a credential and cannot tell a fixture from a real one, so seven copies raised seven findings that each needed a manual dismissal. Building the value from fragments keeps the fixtures as readable and leaves the rule free to mean something the next time it fires. Co-Authored-By: Claude <noreply@anthropic.com>
`.pr-body.md` and `.testout.txt` are working files of this lane and were swept in by a `git add -A`. Neither belongs in the repo. Co-Authored-By: Claude <noreply@anthropic.com>
loop-security — pre-merge audit IN PROGRESSHead audited: §908 boundary — first assertion, verified
Same check clears the rest of the signing surface — Still in flight (do not merge on this comment)Working the reachability questions now:
Verdict follows in a separate comment. |
Adversarial gate — CHANGES-REQUIRED, two gating items, both measuredHead 1. #293 is still live on the money read — and that is the endpoint that matters moreThe literal claim survives: every But the guarantee it stands in for is false. pub(crate) fn is_following(replica: Option<u32>, peers: Option<u32>) -> bool {
match (replica, peers) {
(Some(replica), Some(peers)) => peers.saturating_sub(replica) <= FOLLOWING_TOLERANCE,
_ => true, // <-- replica == None answers TRUE
}
}
That state is reachable in production. Measured on the PR head, peers 530 blocks ahead:
The regression test cannot see it — Fix, one line at the top of 2. #251's production wiring survives deletion with every test greenNo test anywhere exercises the Proved by mutation. Disabling the entire production seam at if false && outcome.accepted { // was: if outcome.accepted {
if let Err(e) = self.reserve_pushed_bundle(&bundle).await {
This is the same gap the PR body itself identifies as why DB-primitive tests were insufficient for #250 Claims that held
Non-gating
Sequencing note for the orchestrator, not this laneThe single-writer claim in the PR body is wrong. #292 and #295 both change |
MichaelTaylor3d
left a comment
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate: PASS
Head reviewed: a8d1254b1aed0f556291eb79a3708e4f7d86c4b7 (resolved from the remote, not from the dispatch prompt). Draft — not undrafted, not merged.
Scope: correctness + acceptance criteria only. Security and adversarial legs run in parallel.
The two questions asked
1. Is replica_answer_is_current genuinely the only producer of synced: true? — YES, verified mechanically.
Every production assignment of synced in crates/dig-wallet/src/sage/rpc.rs is one of two shapes:
replica_answer_is_current(...)— lines 1335, 1466, 1674, 1890- the literal
false— lines 1380, 1496, 1602, 1631, 1735, 1776, 1860, 1908
There is no third shape. Grepping the whole tree for a synced: true outside that file returns exactly three hits, all in crates/dig-node-service/src/control.rs (2990, 3395, 4488) and all inside #[cfg(test)] (the module opens at 2810) — wire-shape fixtures, not producers. The "only way" claim is true at this head, not merely true when it was written.
2. Did the corrected pre-existing test get stronger or weaker? — STRONGER.
the_peak_is_the_replicas_when_the_replica_has_one (rpc.rs:7194) keeps both original assertions — peak_height: Some(5_000_000) and synced: true — inside one assert_eq! over the whole ChainPeak. Nothing was deleted. What changed is the fixture: it now runs with peers_level_at(5_000_000) instead of an unobservable tier, so synced: true is now earned through the measured path rather than granted by the latch. Before the fix that assertion was satisfiable by the defect; now it can only pass if replica_answer_is_current returns true for a replica genuinely level with its peers. That is a strictly narrower set of admissible implementations.
Revert-proofs I ran myself
Both in my own detached worktree at a8d1254, mutation applied to production code, restored by file copy.
| Mutation | Result |
|---|---|
rpc.rs:1890 replica_answer_is_current(Some(peak_height)) changed to db.is_synced().await.unwrap_or(false) |
1 failed — the_peak_is_not_reported_current_while_the_balance_read_calls_the_same_replica_stale, panicking at rpc.rs:7013 with the message "peak called a replica 530 blocks behind current". Exactly the claimed message. |
replica_answer_is_current unobservable-tier arm changed to delegate to is_following(peak_height, None) unnarrowed |
1 failed — an_unobservable_peer_tier_is_never_reported_as_current |
The controls discriminate. a_replica_level_with_its_peers_still_reports_a_synced_peak (rpc.rs:7057) asserts the full ChainPeak including synced: true, so a hardcoded false substituted for the fix fails it while the regression test passes — the pair cannot both be greened by trading one literal for another. Both controls stayed green under both mutations above, which is what makes the two reds attributable.
The revive-not-rewrite claim — verified, no hunk dropped
I extracted every symbol PR #216 added over its own merge-base (0c98ce5f) — 37 names — and asserted each is present in the #295 head's wallet sources. Zero missing. A rebase that silently dropped a hunk would have surfaced here; it did not.
Known non-discriminating tests — bounded, and the shape does not recur
the_window_is_bounded (custody.rs:1829) and the_default_window_is_wide_enough_to_find_an_imported_wallets_history (custody.rs:1842) are compile-time assertions over MAX_DERIVATION_COUNT / DEFAULT_DERIVATION_COUNT. They pin constants and nothing else, exactly as the lane reported. That is not a false green — their doc-comments state that constant-pinning is their subject, and a constant-pin is a legitimate test. I checked the other four tests in that block for the same shape; all exercise real derivation:
observed_usage_extends_the_window_past_the_default(custody.rs:1747) — asserts both trees reach3 + DERIVATION_GAP_LIMIThardened_usage_also_extends_the_window(custody.rs:1771)an_unused_wallet_covers_exactly_the_floor(custody.rs:1790) — asserts a length of 8 for a window of 4, which is the assertion that dies if the hardened tree is droppeda_foreign_puzzle_hash_does_not_extend_the_window(custody.rs:1808)
So the #252 mutation the lane reported (5 failures) is attributable to real coverage, not to the two constant-pins.
#251 ordering — reserve-after-accept is correct
reserve_pushed_bundle runs only on an accepted outcome (rpc.rs:1963). This is the right order: reserving before acceptance would strand a user's coins on a mempool refusal, for a spend that will never happen — the same class of harm in the opposite direction. The warn-not-fail choice on a reservation error is also right: the bundle is in a public mempool by then, and returning an error for a push that demonstrably happened would be the worse lie. See the non-gating note below on the residual window.
The stranding question that mattered more: prune_reservations is genuinely called on the production read paths — rpc.rs:2540, 2792, 3177 — so expiry is enforced lazily at every selection and every pending read, with no dependence on a background task ever running. A reservation whose release path never fires therefore cannot deny a user their own coins indefinitely. reserved_coin_ids reads the raw table with no expiry filter and is safe only because of those three call sites; that coupling is load-bearing and undocumented at the definition, which is the one thing I would keep an eye on.
Boundaries — verified, not assumed
The name-status diff of origin/main...a8d1254 returns exactly 8 paths, all modifications: Cargo.lock, Cargo.toml, SPEC.md, crates/dig-wallet/Cargo.toml, and four files under crates/dig-wallet/src/sage/ — custody.rs, db.rs, rpc.rs, types.rs.
crates/dig-wallet/src/sage/chain.rs— absent from the diff, byte-identical tomain. The live-broadcast env flag and the "DELIBERATELY NOT a Broadcaster" marker are untouched.- No writes to
sync_supervisor.rs,quorum.rs, peer-pool, or DHT/onion code. No overlap with PR #292. - Scratch files:
a8d1254removes.pr-body.mdand.testout.txtand nothing else. The branch adds no files at all — the file set is modifications only — so nothing else rode in on the accidental stage-all.
Suite + gates
cargo test -p dig-walletata8d1254in a clean detached worktree: green, 650 + 22 passed, 0 failed, 3 ignored.- Required checks asserted by name via
check-merge-preconditions.sh(not read off the rollup):Lint commit messages,Check version increment,Rustfmt,Clippy,Test + coverage— all present and SUCCESS, with zero unresolved review threads. The script reports BLOCKED solely because the PR is a draft, which is the intended state. - Versions:
0.135.0to0.136.0, anddig-wallet0.27.0to0.28.0, both own-version lock entries updated. CHANGELOG untouched — correct, git-cliff owns it.
CodeQL fixture
fixture_password() (custody.rs:1669) assembles its value from fragments at runtime. Meaning is preserved: it is still a test-only at-rest password, still over MIN_PASSWORD_LEN (stated in its own doc, so a length-floor failure cannot be mistaken for the property under test), and the seven call sites still exercise the real import path. This defeats the rule, not the test — the right trade, and the doc-comment says so honestly rather than pretending the value is secret.
Sibling coherence — docs.dig.net PR #78
Checked. docs/run-a-node/manage.md now describes the flag as "whether the height above is current — measured against what this node's Chia peers report, not merely whether a first sync once finished", and explicitly covers the unobservable-tier case ("or that it has no peer to compare against yet, so the height is real but dated"). That matches replica_answer_is_current's actual semantics including the one-directional narrowing, so a false reading from a peerless node no longer reads as an outage. Coherent.
dig-constants check (both directions, per the standing rule)
- Does anything here belong in
dig-constants? No, at this head. The four new constants —DEFAULT_DERIVATION_COUNT(500),DERIVATION_GAP_LIMIT(250),MAX_DERIVATION_COUNT(25_000) andRESERVATION_TTL_MS— are single-consumer wallet-scoping and mempool policy. No second repo must match them today, anddig-constants(16 constants) publishes nothing in this space. Recorded as a watch item rather than a gate. - Should anything here be using
dig-constants? No literal in this diff duplicates a valuedig-constantsalready publishes.
Two non-gating notes follow as inline threads; I am resolving both myself so neither blocks the merge. Nothing gating. The verdict stands at PASS for the correctness leg — merge remains subject to the two sibling gates and to the PR leaving draft.
loop-security: PASSHead audited: No gating security defect. Five non-gating items below (follow-up tickets, not merge blockers), plus the list of what I attacked and could not break. §908 — the boundary HOLDS, and
|
| file | origin/main |
pr/295 |
|---|---|---|
sage/chain.rs |
fd691df41aae90ed466b2460fa7bb791453d1e2a |
same |
sage/spend.rs |
747488e26c00847ebc1ae9035e92d1a2343439d5 |
same |
sage/mint.rs |
f34402cd9abed195558afba2485b54e155514db1 |
same |
sage/auth.rs |
f366eb8cd7d4068896247d0d4ee201ff0dda2cb7 |
same |
sage/offers.rs |
e1fe80f6253efa62a2d9b34df39d93aa928e0ba7 |
same |
sage/service.rs |
d91675e98a0f2564387fa5821d0fd4c8878d354a |
same |
The DIG_WALLET_ENABLE_LIVE_BROADCAST gate and the DELIBERATELY-NOT-a-Broadcaster boundary are therefore preserved verbatim. Only custody.rs, db.rs, rpc.rs, types.rs change under sage/.
The boundary is not moved elsewhere either:
build_signerstays private and gains no caller. Its call sites are unchanged (import / unlock / restore, inside custody).- Nothing new constructs, holds, derives, or accepts signing material from a request. The one new custody entry point,
observe_occupied_puzzle_hashes(custody.rs:289), takes a set of PUBLIC p2 puzzle hashes read out of the node's own coin table. No key, no seed, and no path from it to a signer. - Seed provenance is unchanged:
master_secret_keyis the same three lines lifted out ofwallet_fingerprint(bip39 parse,Zeroizingseed buffer,SecretKey::from_seed). TheZeroizingwrapper survives the extraction; no new copy and no new lifetime for the seed. - The node-custodied refusal in
push_signed_bundleis untouched, and SPEC §18.12 is updated so the custodied-KEY set that guard reads widens with the window across BOTH trees. That strengthens the gate rather than weakening it, and there is no regression window: before this PR the node never derived hardened keys at all, so it could never have signed a hardened coin.
Findings (all non-gating)
1. The derivation window is peer-steerable to its cap — defense-in-depth, LOW-MED
db.rs occupied_puzzle_hashes() is SELECT DISTINCT puzzle_hash FROM coins. The coins table is written by sync::apply_coin_states (sync.rs:698) from peer-pushed coin_state_update frames, filtered ONLY by membership in the subscribed set — nothing verifies the coin exists on chain. The subscribed set IS the derived window, and the node discloses it to the peer when it subscribes.
Scenario: the node holds a peer at PeerTrust::Corroborated (corroboration is quorum agreement on a peak HEIGHT, not on coin states). The attacker fabricates a coin state at every subscribed puzzle hash. On the next unlock, highest_occupied_index returns the top of the window and it extends by DERIVATION_GAP_LIMIT. The wider set is re-subscribed on the next connect; repeat. Roughly 98 cycles reaches MAX_DERIVATION_COUNT.
Terminal cost, using the PR's own measurement of 251ms per 1000 keys: 50,000 BLS derivations per unlock, about 12.5s; a ~50,000-hash subscription frame per peer connection; 50,000 resident secret keys. Persisted, because the evidence lives in coins.
Not gating: bounded by MAX_DERIVATION_COUNT, needs a corroborated peer AND ~98 user-initiated unlocks, costs no money, and weakens no custody guard. Worth recording that the PR raises the fabricate-into surface 20x by default (50 to 1000 hashes) before any ratcheting.
2. The reservation is gated on an untrusted accepted — defense-in-depth, LOW
rpc.rs:1955 reserves only when outcome.accepted, which is the push target's own answer, and under NC-12 that target is untrusted.
- Fail-OPEN: a source answering
accepted: falsefor a bundle it in fact relayed leaves the coins unreserved, re-opening exactly the double-selection this ticket closes. The same holds for a transport error after the bytes went out — that arm returnsPushError::Unreachableand reserves nothing. - Fail-CLOSED: a source answering
accepted: truefor a refused bundle holds an authorized caller's coins for the 10-minute TTL.
Both need a lying chain source; the second additionally needs an authorized local caller; both are TTL-bounded. Suggested follow-up: treat a transport failure AFTER transmit as possibly-in-flight rather than definitely-not.
3. release_spend has no production caller — nit
db.rs release_spend is exercised only by a test. Its two documented outcomes are both handled elsewhere: a mempool refusal never creates a reservation in the first place, and a settled spend is retired by prune_reservations conditions 2 and 3. Wire it or delete it — dead code on a custody-adjacent path invites a future caller to assume it is exercised.
4. MAX_DERIVATION_COUNT names a threat channel that does not exist — nit
custody.rs:99 justifies the ceiling as protection against a corrupt or hostile derivation_count in a hand-edited manifest. derivation_count is never read from the manifest; the sole production constructor is WalletCustody::mainnet (service.rs:148) passing the compile-time DEFAULT_DERIVATION_COUNT. The clamp is correct and must stay, but the reason it actually earns its keep is finding 1, and the comment should say so.
5. Resident secret-key footprint grows 20x by default — defense-in-depth, LOW
chia-bls 0.22 SecretKey has no Drop and no zeroize (checked the vendored crate source: no impl Drop, no zeroize reference anywhere in it). Derived keys are left in process memory. This PR raises the resident count from 50 to 1000 by default, and to 50,000 at the cap. An upstream property this diff cannot fix; it belongs on the memory-hygiene story, not on this PR.
Coherence note (for the correctness gate, not security)
types.rs changes PendingTransactionRecord.fee from Amount to Option<Amount> — wire-visible, null where a client previously always received a number. Worth confirming the dig-node-control-interface side and any dig-app consumer handle null rather than rendering it as zero, since a fee of zero is precisely the confident-wrong-number-about-money failure §18.8 now forbids.
What I attacked and could NOT break
- A foreign puzzle hash extending another wallet's window.
highest_occupied_indexmatches only against THIS wallet's derived keys;a_foreign_puzzle_hash_does_not_extend_the_windowpins it. - Reserving a third party's coins. Requires a mempool-accepted bundle spending them, which requires their signature. No path.
- Orphaned
coin_reservationspermanently stranding funds. I expected SQLite's default of foreign keys OFF to make theON DELETE CASCADEa no-op, so that pruning an expiredpending_transactionsrow would leave its reservations behind forever and permanently un-spend the user's coins — the exact failure the design says only an unconditional expiry rules out. It does not happen:db.rs:728(productionopen) anddb.rs:735(open_in_memory) BOTH set.foreign_keys(true), so the cascade fires and the tests run against the same pragma as production. - Guard vacuity. I checked whether the reservation can ever bite, given that
push_signed_bundlerefuses node-custodied spends and the coin table would then never hold a reserved id. It is NOT vacuous:UnionPuzzleHashSource(sync_supervisor.rs) makes the subscribed set the union of custody's own addresses ANDcontrol.wallet.watch-registered keys, so on the §908 install the reserved ids are exactly the watched user's coins sitting in that table. - Partial coverage of the selection guard. Every production selection site in
rpc.rs(3082, 3103, 3149, 3199, 3403, 3464, 3549, 3590, 3627, 3646, 3725, 3812) reads throughspendable_coins, which prunes and then reads the unreserved set; the CAT path at 3180 usesunreserved_unspent_coinswith its own prune. No productiondb.unspent_coinscall remains on any spend path. - The new
get_pending_transactionserror arm as a DoS.SpendResult::feeisu64(dig-clvmconsensus/result.rs:18), so a stored value always re-parses. TheErrarm needs genuine DB corruption, not adversarial input. - An AuthZ regression on the new pre-dispatch hook.
refresh_observed_derivationsruns insideWalletBackend::dispatch, and all three transports applywallet_authz::requires_authorizationBEFORE dispatching (the HTTPwallet_rpchandler, the WS path, and the JSON-RPC path inserver.rs);classifyputs everywallet.andauth.prefixed method in the Custody class. The dig-wallet sidecar transport is mTLS shared-client-cert gated.control.wallet.broadcastis token-gated, not an open read. - New egress from
chain_peak.replica_answer_is_currentcallschain_peer_tier(), which isfallback.peer_tier()— a cached local read of the transport's own state. The change REMOVES a DB call and adds no outbound request; it does not open the coinset oracle path this file refuses elsewhere. - Denial-of-confirmation via
synced: false. A peer over-claiming its peak does force the flag false (is_followingis asaturating_subagainstFOLLOWING_TOLERANCE). Butreplica_answer_is_currentalready exists onmainand already governs three balance/coin reads there (rpc.rs:1311,1442,1650); this PR only addschain_peakas a fourth consumer. The primitive is pre-existing, its direction is fail-closed (refusing to claim settled), and nothing in the diff gates a spend on it. - Hardened / unhardened index confusion. The two trees are kept in separate vectors so a position IS its index, and the code records that a modulo-based form was considered and rejected for exactly this reason.
unhardened_matches_digstore_chainpins the fast intermediate derivation againstdigstore_chain::derive_indexed_keys, andp2_puzzle_hash_matches_the_signerpins the p2 mapping against whatWalletSignerapplies. Those are the two drift risks that would put money at addresses the wallet does not watch, and both are asserted rather than reasoned about. - Unbounded derivation.
targetis clamped toMAX_DERIVATION_COUNTon entry and on every extension,saturating_addis used throughout, andextend_tois incremental — so total work is linear in the final target rather than quadratic across iterations, and the loop provably terminates. - Secret or credential leakage. Two new production log lines, both logging only a DB error. Every
eprintln!sits inside an ignored measurement test printing durations. No seed, no key, no passphrase, no address list. - The CodeQL fixture.
fixture_password()lives entirely inside the test module and is used only in test import calls. No production credential path is touched, no existing literal was removed or weakened, and nothing real was silenced — it swaps a would-be literal for a runtime concat in a fixture. - Dependencies.
Cargo.tomlandCargo.lockcarry version bumps only (workspace 0.135.0 to 0.136.0, dig-wallet 0.27.0 to 0.28.0). No new dependency and no loosened pin; themaster_to_wallet_*andDeriveSyntheticimports come from the already-presentchiadep.
Shared-checkout disclosure
Read-only. I ran one git fetch origin main refs/pull/295/head:refs/remotes/pr/295 in the primary checkout — a ref addition only — and read everything else through git show, git diff and git ls-tree against objects. No checkout, reset, stash, clean, or file edit. Verified afterwards: HEAD still bef7fc3c7b79e9960611bd9de3e4bbde4bbc90f7, git status --porcelain shows only the pre-existing untracked .claude/loop/, and the three stash entries are the pre-existing ones — I created none.
…unknown `replica_answer_is_current` narrowed only the peer-side arm of `is_following`. The replica-side arm survived, and it is the one that reaches production: `chain_peak` calls the gate inside `if let Some(peak)` and so cannot hand it `None`, while the balance and coin reads pass their `Option` straight through. `refresh_tracked_coins` latches the replica authoritative without ever writing a peak, so the money reads — and only the money reads — paired `synced: true` with `peak_height: null` (dig-node#293). Also covers the `push -> reserve` seam (#251) through the production reads. Every existing reservation test called `db.reserve_spend` directly, so mutating the seam to `if false && outcome.accepted` left the suite green. `is_following` itself is left alone: its permissive arms are correct for the sync-phase reporting it was written for, and the narrowing is a property of the money read. Co-Authored-By: Claude <noreply@anthropic.com>
`reserve_spend` normalises every coin id it writes; `coins` stores whatever hex the chain source handed over. `prune_reservations` joined the two RAW, so an upper-case coin never matched its own reservation: the settled bundle stayed pending and held its other inputs out of selection for the whole TTL. The selection path already normalised both sides, so this was the last raw comparison. Also removes `release_spend`, which had no production caller and could not gain a correct one — a refusal reserves nothing and a settlement is retired by `prune_reservations`, so every definitive outcome was already covered. Its cascade property is now asserted through the retirement path that actually runs. Also corrects `MAX_DERIVATION_COUNT`'s rationale: it named a hand-edited manifest, but the manifest carries no `derivation_count` (it is a `WalletCustody::new` argument). The real unbounded channel is the attacker-extensible coin set the gap-limit scan follows. Co-Authored-By: Claude <noreply@anthropic.com>
rustfmt indents a builder chain that wraps a multi-line struct literal into a shape that reads worse than the value it builds. Naming the pusher first says what it is and leaves the chain one line. Co-Authored-By: Claude <noreply@anthropic.com>
Gate round 2 — both gating findings fixed. Still DRAFT.Branch G1 — #293 was still live on the money reads
Fix: a guard clause at
Test G2 — #251's wiring survived deletionConfirmed: every reservation test called Tests added at Revert-proofs — all three fire, on the right assertion
Each was run on a COMMITTED tree and restored by file copy, never by a The One pre-existing test changed, and it is worth reading
The four cheap items — all four taken
Blast radius checkedNo gitnexus index in this worktree, and building one writes repo-tracked files into a lane worktree, so this is the sanctioned grep+read fallback rather than a stale-index answer.
No HIGH or CRITICAL risk found. Files touched: Local evidence
Staying DRAFT until the orchestrator's merge-first sequencing says otherwise. |
Two commits on this branch rewrote SPEC.md, rpc.rs, db.rs and custody.rs
whole-file through a Windows text-mode writer, which translated every LF
to CRLF. The content was unchanged, but every line read as modified: the
PR reported 23,329 additions / 21,565 deletions for a 1,823 / 59 delta.
The cost was not cosmetic. CodeQL attributed 36 pre-existing test
fixtures in custody.rs as newly introduced ("code changes were too
large") and turned the GHAS gate red, and a shared SPEC.md would have
conflicted wholesale with a concurrent PR whose hunks do not overlap
these at all.
Restored to LF byte-for-byte; `git diff --ignore-cr-at-eol` against the
prior tree is empty. Deliberately no .gitattributes: a repo-wide
text=auto would renormalise every file in the repository, a far larger
change than the one being undone.
Co-Authored-By: Claude <noreply@anthropic.com>
The `LOWER()` added to `prune_reservations` repaired one raw coin-id comparison and claimed to be the only one. Two others survived, and they fail in different directions: - `are_coins_spendable` binds caller-supplied ids on the Sage-parity `get_are_coins_spendable` endpoint. An upper-case id returned `false` for a genuinely spendable coin — a refusal rather than a loss, but a wrong answer about money to a parity consumer. - `record_arrivals` answers `parent_is_ours` with a raw comparison of the child's `parent_coin_info` against `coins.coin_id`. On a case mismatch the wallet's own change coin read as not-ours and was announced to the user as an incoming payment — the same money-display lie as #293. Fixed at the WRITER instead. `upsert_coin`/`upsert_coins` normalise `coin_id` and `parent_coin_info`, so all three sites are covered at once and a fourth reader added later inherits the guarantee. The read layer was already written as though this held — `reserved_coin_ids` lower-cases what it reads back, `unspent_coins_scoped` lower-cases the puzzle hash it binds — so this makes an existing assumption true rather than adding one. The `LOWER()` in `prune_reservations` is REMOVED, not kept as belt-and- braces: `coins.coin_id` is a PRIMARY KEY, SQLite cannot use an index through a function call, and the wrapper turned each retirement into a full scan of `coins` per reservation row. With both sides normalised by their writers the raw comparison is correct, so retaining it would buy only the scan. Existing rows are repaired by ladder step 2, in one transaction. A coin present under both cases is a PRIMARY KEY collision that would abort the step, so the upper-case row is dropped first: a lower-case twin can only have been written by the fixed code, making it the fresher observation. Three tests, each varying only the case the chain source handed over and each with a control a match-everything implementation would fail. All three fail on the code before this commit; the arrival one fails by announcing 2 arrivals where 1 is correct. Also corrects a false claim in `replica_answer_is_current`'s doc, which said it was "the single gate every read that can produce `synced: true` already passes through". True within `WalletBackend`, but `SyncHandle::status` reaches `SyncPhase::Synced` through its own `is_following` call and can still emit `{phase: "synced", peak_height: null}` on `control.wallet.sync-status`. That path is out of scope (a status endpoint, and a phase machine owned by dig_ecosystem#2761), but a false "this is the only gate" in custody-adjacent code is exactly what makes the next reader skip the check. Co-Authored-By: Claude <noreply@anthropic.com>
The arrival fixture wrote the parent upper-case and the child's `parent_coin_info` lower-case, and only that. Reverting the `parent_coin_info` normalisation alone left the test GREEN: with the stored `coin_id` normalised, "AABB" already became "aabb" and matched the child's pointer without the second normalisation ever running. The two directions are repaired by different binds, so a fixture carrying one cannot see the other. A second change coin now points at its parent in upper case while the parent is stored lower, which makes both binds load-bearing: reverting either one alone now fails, announcing 2 arrivals where 1 is correct. Co-Authored-By: Claude <noreply@anthropic.com>
The two-spelling collision rule could not see a coin id stored under several NON-canonical spellings. `AAbb` and `aAbb` are both unequal to their own lower-casing, so the case-scoped DELETE removed neither and the UPDATE then collided them onto one unique key. The transaction rolled back correctly and the ladder mark stayed unset -- but the retry on the next open is byte-for-byte identical, so `migrate` failed forever and the wallet never opened again. A rollback is a safe failure only when the retry can succeed. Collisions are now resolved before the update, for any number of spellings, by a total and deterministic rule: keep the spelling that is already canonical (a group holds at most one), else the lexicographically smallest. Recency is deliberately not a tie-break -- the rows are identical apart from case, so the table carries no evidence of which was written last. Dropped rows are logged at WARN, since in this tree a collision can only mean a non-conforming `ChainFallback`/`CoinPeer` implementation wrote to the replica. `arrival_pending.coin_id` and `arrivals.coin_id` are normalised in the same transaction. They hold copies of `coins.coin_id` and are compared against it raw, so normalising the coin table alone was a desync with two money-visible consequences: `record_arrivals` prunes every held row whose id is no longer in `coins`, and losing the hold is how a deferred coin falls below the baseline watermark and is never announced; and `INSERT OR IGNORE INTO arrivals` stops recognising an id it already recorded, announcing a coin to the user twice. Correct the migration docstring, whose justification was false and inverted. A lower-case id is the ORDINARY pre-fix spelling -- every in-tree writer has used `hex::encode` or an explicit normalisation since long before this change -- so an upper-case row cannot be the staler observation. The only verbatim path into the table is `refresh_tracked_coins`, the point-read used precisely because the subscription replica is behind, so an upper-case row would be the FRESHER one. Case carries no recency information in either direction. The migration's real warrant is that `ChainFallback` and `CoinPeer` are public traits. Narrow the normalisation claim in `upsert_coin` and SPEC.md to the two coin identities that are actually normalised. `puzzle_hash`, `asset_id` and `hint` are stored verbatim while `unspent_coins_scoped` lower-cases what it binds against them; that is a real defect, filed separately, and not something either statement may imply is already handled. The migration test's twins were identical in every column but case, so a rule that kept the wrong twin passed it unchanged. The twins now differ, and the test names which survives. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
DRAFT — DO NOT MERGE. Gate round not started.
Wallet-correctness batch for epic dig_ecosystem#2760. One branch, one PR, four dig-node tickets.
Closes #293
Closes #252
Closes #251
Closes #250
Revived, not rewritten — and the evidence for that call
PR #216 (
loop/2762-wallet-correctness, head12ffe2d6, parked since 2026-08-13) carried 1,395 linescovering #252/#251/#250 and sat 56 commits behind
main, which had meanwhile grown +12,634lines across the wallet crate. That is the shape of a rotted branch, so the revive-vs-rewrite call was
measured rather than assumed:
mainproduced exactly one conflict hunk per file, indb.rsandrpc.rs, and both were purely additive test blocks at the end of the test module — main'sdig-node#283 offer-rekey tests landing beside this branch's reservation tests. Both sides kept; the
only real repair was the closing brace the two sides shared.
mainon the first try, meaning every productionwiring point the branch attaches to (
push_signed_bundle,spendable_coins,get_pending_transactions,WalletCustody::build_signer) survived main's evolution unchanged.custody.rs— 542 of the 1,395 lines — is untouched onmain, so the derivation work appliedverbatim.
Rewriting would have re-derived 1,395 lines of correct, well-documented work to avoid one brace.
Revived.
#293 —
wallet peakreportedsynced: trueon a stale replicachain_peakcomputedsyncedfromdb.is_synced()— theinitial_sync_completelatch — andreturned on the replica-served path without ever consulting
replica_answer_is_current.dig_ecosystem#2869 replaced that latch with a measured predicate at the balance and coin reads and left
this endpoint behind. Measured on a running node:
sync-statussaidsyncingwhilepeaksaidsynced: true, in the same process at the same moment, on a replica 1,875 blocks behind.peakis theendpoint a client uses to bound a confirmation, so the falsehood landed on the read that decides whether
money has settled.
The fix is structural, not two sites corrected separately.
replica_answer_is_currentis now theonly way any read produces
synced: true; every other site writes the literalfalseon afallback-tier answer, where a third party's height says nothing about the replica. The doc comment that
already claimed the two endpoints could not disagree now states that basis instead of asserting it.
The regression test fails on the unfixed code
the_peak_is_not_reported_current_while_the_balance_read_calls_the_same_replica_stale, run againstchain_peakbefore the fix:Fixture design, stated because this is where a false green would be born:
initial_sync_completeand lag pastFOLLOWING_TOLERANCE(4 blocks).The latch is what the defect trusts, so a fresh replica fails the latch and a caught-up one is
genuinely current — neither can exhibit the defect. The test asserts the latch is set before
asserting anything else.
PEERS_AHEAD_BY= 530, drawn from the live measurement rather than invented, and twoorders of magnitude past the tolerance.
!synced. Disagreement between the twoendpoints is the reported bug, and a fix applied at one site while the other kept its own predicate
would satisfy a lone
!syncedassertion and re-open the same class of defect.a_replica_level_with_its_peers_still_reports_a_synced_peak) passes on the unfixedcode, so
synced: falsehardcoded in place ofsynced: truecannot green the pair.A pre-existing test had to change, and it is worth naming:
the_peak_is_the_replicas_when_the_replica_has_oneran with an UNOBSERVABLE peer tier and assertedsynced: true— the exact falsehood #293 removes. It now fixes the peer tier level with the replica;its subject is the height's provenance, and the freshness flag has its own tests.
The other three
wallet whose history reached index 50 had coins the node never subscribed and never counted, and a
hardened coin — where farmer and pool rewards land — was invisible at every index, while the node
reported
syncedover both. Now both trees, a 500-index floor, and a gap-limit scan that followsobserved usage, bounded at 25,000 so a hand-edited manifest cannot turn an unlock into unbounded key
derivation. The signer and the watched set widen together: widening only the watched set converts
"cannot see the coin" into "can see it and cannot spend it", which is worse and reads as a send bug.
push_signed_bundlenow reserves the bundle's inputs, only once the mempool hasaccepted it. A refusal reserves nothing (those coins were never committed, and holding them would
strand the user's money over a spend that will never happen), and a reservation failure does not fail
the push — the bundle is already in a public mempool, and reporting a push that did happen as an
error is a worse lie than the double-selection this guards against.
spendable_coinsprunes expiredreservations and then selects only unreserved coins.
get_pending_transactionsreturned a hardcoded empty list. It now reports the reservedin-flight set, which is only truthful because dig-node reserves no coins in flight — two sends in the confirmation window pick the same coin #251 landed first: a pending-transaction report with
nothing tracking in-flight coins is a different falsehood, not a fix.
Blast radius checked
chain_peak— callers, repo-wide:dig-node-service/src/control.rs:1931(thecontrol.wallet.peakhandler) and two doc references in
rpc.rs/sync_supervisor/tests.rs.sync_supervisor/tests.rs:1619asserts the STATUS path never reacheschain_peak, which this changedoes not alter. No production caller reads the
syncedfield other than through the control surface,and no in-tree consumer branches on it.
replica_answer_is_currentgains one call site and keeps itsthree existing ones.
WalletCustody::build_signer/DEFAULT_DERIVATION_COUNTare internal todig-wallet; no other crate names them. The diff touches onlySPEC.md,Cargo.toml,Cargo.lockandfour files under
crates/dig-wallet/src/sage/.Risk: MEDIUM. No HIGH/CRITICAL.
chain_peak'ssyncedflag changes value for one class of caller —a node whose replica is behind, or which has no observable Chia peer, now reports
falsewhere itreported
true. That is the point of the change, but a consumer treatingsynced: falseas an outagerather than as staleness will become more conservative. The height is always still served.
Boundary this PR does NOT cross
§908 binds absolutely: the node signs NOTHING on the user's behalf and the user's key never enters the
node. Coin reservation is about not double-spending a coin while building a bundle; it is not permission
to sign.
crates/dig-wallet/src/sage/chain.rsis byte-identical tomain— theDIG_WALLET_ENABLE_LIVE_BROADCASTgate and the// DELIBERATELY NOT a Broadcasterboundary areuntouched.
Single-writer
Stays out of
sync_supervisor.rs,quorum.rsand the peer-pool code (dig_ecosystem#2761/#2768 ownthose; #278's dep adoption is deliberately not in this batch), and out of the DHT/onion availability
code (PR #292 owns that).
Version
0.135.0→ 0.136.0 (minor). New capability — a derivation window that grows, a reservation ledger,a real pending set — alongside a behaviour fix. No API is removed or renamed and no wire shape breaks.
dig-wallet0.27.0→0.28.0(new public API:observe_occupied_puzzle_hashes,DERIVATION_GAP_LIMIT,MAX_DERIVATION_COUNT).Cargo.lock's own-version entries updated to match.