Skip to content

fix(wallet): retire the last largest-first rival, and make the unguarded sync setter unreachable - #474

Draft
MichaelTaylor3d wants to merge 6 commits into
mainfrom
loop/mc-drain-wallet
Draft

fix(wallet): retire the last largest-first rival, and make the unguarded sync setter unreachable#474
MichaelTaylor3d wants to merge 6 commits into
mainfrom
loop/mc-drain-wallet

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What this changes

Closes #428 and
#462.

Two loaded guns in dig-wallet/src/sage. Both tickets' premises needed correcting first, and in
each case the correction changed the fix — the details are in the per-ticket sections below.

Blast radius checked

gitnexus indexes for this repo are ~300 commits stale (§2.0: a stale index returns a false-safe
impactedCount: 0), so this was taken by grep + direct read and is stated as such.

  • select_xch: private to offers.rs, two call sites in build_make_offer/build_take_offer plus its
    own tests. No cross-crate exposure.
  • set_initial_sync_complete: 56 references, all inside dig-wallet, none outside the crate — one
    production (sync.rs:952, disarming), 55 in test modules. Renamed by exact identifier, which is
    unique to this crate; verified zero residual references afterwards.
  • dig-node-service builds clean against the changed crate.

Evidence summary

dig-wallet full lib suite: 758 passed; 0 failed; 1 ignored.

The #428 test was red first for the right reason — it panicked at offers.rs:421, the
sum += c.amount line itself — and is revert-proof: with only the accumulation reverted on a committed
tree, 1 passed; 1 failed, panicking at the restored +=.

Status

DRAFT — the gate round has not returned.


Fixed on loop/mc-drain-wallet, head 78c2b51, draft PR #474.

Two corrections to the ticket, both from measuring origin/main before building.

1. There is ONE rival left, not three

The table lists select_cats (#2) and select_cat_rows (#3) as rivals to be deleted. Both had
already migrated
before this ticket was worked:

  • offers.rs:437 select_cats — calls selection::select_largest_first, filter is its own
  • rpc.rs:4917 select_cat_rows — calls selection::select_largest_first

So the scope was one function, select_xch, and it is now the fourth caller of the canonical
selector. The tiebreak was confirmed rather than assumed, as the ticket asks:
b.amount.cmp(&a.amount).then(a.coin_id().cmp(&b.coin_id())) is exactly the canonical key
(amount, coin_id) — descending amount, ascending coin id. The refusal message is preserved verbatim,
so an operator's log line does not change spelling under a refactor.

2. The shipped consequence is the OPPOSITE of the one predicted, and milder

The ticket says a wrapped sum "can satisfy the sum >= need break, returning a coin set whose real
total is far below need". That is not reachable, and the reason is worth recording because it
also constrains what a valid test can look like:

  • the walk breaks the moment sum >= need, and the set is sorted descending, so the largest coin
    is added to a zero total and cannot overflow on its own;
  • to overflow on a later coin, a1 + a2 must exceed u64::MAX while a1 < need, forcing both
    a1 > u64::MAX / 2 and need > u64::MAX / 2;
  • and any set that overflows sums to more than u64::MAX, hence more than any u64 target — so the
    set genuinely does cover the target.

The real shipped failure is therefore: debug builds PANIC on a money path, and release builds wrap
to a small total and return a spurious insufficient XCH to offer for a coin set that could in fact
pay. Safe direction, wrong answer. The disagreement the ticket identifies is real and the fix is
unchanged; only the severity claim needed correcting.

This cost a wrong test first. My initial regression test asserted a refusal on the overflow
fixture and failed against the correct implementation — the fixture could not exhibit the property I
had written down, because the property was not true. The reasoning above is now recorded in the test's
own doc-comment so the next reader does not repeat it.

Evidence

  • Red first, for the right reason: the new test panicked at offers.rs:421 — the sum += c.amount
    line itself — before the fix.
  • Revert-proof: with only the accumulation reverted on a committed tree, 1 passed; 1 failed,
    panicking at the restored +=.
  • Control test: an ordinary target still takes the single 100 rather than 70 + 30, so a selector that
    always refused would not pass.
  • dig-wallet full lib suite: 758 passed; 0 failed; 1 ignored. dig-node-service builds clean
    against it.

(Measured and fixed by the standing dig-node backlog-drain lane, 2026-08-31.)
Fixed on loop/mc-drain-wallet, head 78c2b51, draft PR #474.

Your title is right and your body is wrong, and the difference changed the fix.

The setter DOES have a production caller

The body says "a raw, unguarded setter ... with no production caller" and asks for
#[cfg(test)]. I confirmed the no-caller claim as the ticket instructs — and got it wrong the first
time, from a truncated grep output that showed only the first 20 of 56 references, all of which
happened to be test code.

The full set contains one production call:

crates/dig-wallet/src/sage/sync.rs:952 — the reorg handler, inside apply_update, well above that
file's #[cfg(test)] boundary at line 1318:

if moved_backwards {
    tracing::warn!(... "wallet sync: replica moved backwards; clearing initial-sync-complete ...");
    db.set_initial_sync_complete(false).await?;
}

#[cfg(test)] would not have compiled. That is the ticket's own acceptance bar working — "enforced by
the compiler rather than by convention"
— just earlier than intended.

The title states the correct fix: narrow the TYPE

"can express the unguarded true direction — narrow its type". Exactly. The production call writes
false, and that direction is not the defect:

direction what it does is it the #454 money lie?
true arms initial_sync_complete; over an emptied table this is balance 0, synced true on a funded wallet yes — the whole reason #454 added reset_epoch guards
false disarms, so wallet reads fall back until a fresh catch-up no — it can only make the node claim LESS than it knows

So gating the whole function would have removed a legitimate, conservative production capability in
order to close a hole that only exists in one of its two directions.

What shipped

pub async fn clear_initial_sync_complete(&self) -> sqlx::Result<()>   // no argument: cannot arm
async fn write_initial_sync_complete(&self, complete: bool)            // private
#[cfg(test)] pub async fn force_initial_sync_complete_for_test(&self, complete: bool)

There is no true to pass. The unguarded arm is unrepresentable rather than discouraged — the same
shape as a scalar that cannot represent a set of two. Arming stays reachable only through
complete_catch_up and record_coverage, which carry the reset_epoch guards. All 55 test call sites
were renamed to the forcing hatch, whose name carries its own danger as the ticket asks.

Evidence

The arming half is discharged structurally and cannot be a runtime assertion — which is stated in
the new test's doc-comment rather than left implied, so a reader who finds only the test does not
conclude the arming direction went unconsidered. The ticket rules out a grep-for-callers test; the type
is the enforcement.

clear_initial_sync_complete_disarms_an_armed_flag pins the direction that IS representable, from an
explicitly ARMED fixture so the clear is not asserted against an already-false flag.

dig-wallet full lib suite: 758 passed; 0 failed; 1 ignored. dig-node-service builds clean.

(Measured and fixed by the standing dig-node backlog-drain lane, 2026-08-31.)

…ded arm unrepresentable

#428 -- select_xch was the LAST of the four largest-first selectors still carrying
its own loop; select_cats and select_cat_rows had already migrated, so the ticket's
table of three rivals was stale. Its divergence was asymmetric: a bare += where the
other three saturate. The shipped consequence is milder than the ticket predicted and
worth recording -- a wrapped total reads back as a shortfall, so release builds refuse
a coin set that could pay rather than accepting one that cannot, and debug builds
panic on a money path. Tiebreak confirmed identical before deleting, not assumed.

#462 -- the ticket body says the setter has no production caller. It does:
sync.rs's reorg handler disarms the flag when the replica moves backwards. The
ticket's TITLE is the correct statement of the problem -- the setter can express the
unguarded TRUE direction -- so the fix is to remove the parameter rather than to
gate the function. clear_initial_sync_complete() cannot arm because there is no true
to pass; the raw write is private; tests keep a named forcing hatch. Disarming stays
public and unguarded, deliberately: it can only make the node claim less than it
knows.

Closes #428
Closes #462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dig-wallet: select_xch is a fourth largest-first rival and disagrees with its three siblings on overflow direction

1 participant