fix(wallet): retire the last largest-first rival, and make the unguarded sync setter unreachable - #474
Draft
MichaelTaylor3d wants to merge 3 commits into
Draft
fix(wallet): retire the last largest-first rival, and make the unguarded sync setter unreachable#474MichaelTaylor3d wants to merge 3 commits into
MichaelTaylor3d wants to merge 3 commits into
Conversation
… unguarded sync setter
…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
This was referenced Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Closes #428 and
#462.
Two loaded guns in
dig-wallet/src/sage. Both tickets' premises needed correcting first, and ineach case the correction changed the fix — the details are in the per-ticket sections below.
select_xchwas the LAST largest-first rival, not one of three; the other two had alreadymigrated. Its shipped failure is a debug-build panic on a money path and a spurious refusal in
release, not the false success the ticket predicted.
sync.rs:952), so#[cfg(test)]would nothave compiled. The ticket's title names the right fix: narrow the type so the unguarded
trueisunrepresentable, while the production
false— the conservative direction — keeps working.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 tooffers.rs, two call sites inbuild_make_offer/build_take_offerplus itsown tests. No cross-crate exposure.
set_initial_sync_complete: 56 references, all insidedig-wallet, none outside the crate — oneproduction (
sync.rs:952, disarming), 55 in test modules. Renamed by exact identifier, which isunique to this crate; verified zero residual references afterwards.
dig-node-servicebuilds clean against the changed crate.Evidence summary
dig-walletfull lib suite: 758 passed; 0 failed; 1 ignored.The #428 test was red first for the right reason — it panicked at
offers.rs:421, thesum += c.amountline itself — and is revert-proof: with only the accumulation reverted on a committedtree,
1 passed; 1 failed, panicking at the restored+=.Status
DRAFT — the gate round has not returned.
Fixed on
loop/mc-drain-wallet, head78c2b51, draft PR #474.Two corrections to the ticket, both from measuring
origin/mainbefore building.1. There is ONE rival left, not three
The table lists
select_cats(#2) andselect_cat_rows(#3) as rivals to be deleted. Both hadalready migrated before this ticket was worked:
offers.rs:437select_cats— callsselection::select_largest_first, filter is its ownrpc.rs:4917select_cat_rows— callsselection::select_largest_firstSo the scope was one function,
select_xch, and it is now the fourth caller of the canonicalselector. 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 thesum >= needbreak, returning a coin set whose realtotal is far below
need". That is not reachable, and the reason is worth recording because italso constrains what a valid test can look like:
sum >= need, and the set is sorted descending, so the largest coinis added to a zero total and cannot overflow on its own;
a1 + a2must exceedu64::MAXwhilea1 < need, forcing botha1 > u64::MAX / 2andneed > u64::MAX / 2;u64::MAX, hence more than anyu64target — so theset 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 offerfor a coin set that could in factpay. 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
offers.rs:421— thesum += c.amountline itself — before the fix.
1 passed; 1 failed,panicking at the restored
+=.always refused would not pass.
dig-walletfull lib suite: 758 passed; 0 failed; 1 ignored.dig-node-servicebuilds cleanagainst it.
(Measured and fixed by the standing dig-node backlog-drain lane, 2026-08-31.)
Fixed on
loop/mc-drain-wallet, head78c2b51, 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 firsttime, from a truncated
grepoutput that showed only the first 20 of 56 references, all of whichhappened to be test code.
The full set contains one production call:
#[cfg(test)]would not have compiled. That is the ticket's own acceptance bar working — "enforced bythe 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:trueinitial_sync_complete; over an emptied table this isbalance 0, synced trueon a funded walletreset_epochguardsfalseSo 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
There is no
trueto pass. The unguarded arm is unrepresentable rather than discouraged — the sameshape as a scalar that cannot represent a set of two. Arming stays reachable only through
complete_catch_upandrecord_coverage, which carry thereset_epochguards. All 55 test call siteswere 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_flagpins the direction that IS representable, from anexplicitly ARMED fixture so the clear is not asserted against an already-false flag.
dig-walletfull lib suite: 758 passed; 0 failed; 1 ignored.dig-node-servicebuilds clean.(Measured and fixed by the standing dig-node backlog-drain lane, 2026-08-31.)