Skip to content

Add wallet recovery balance audit - #117

Merged
Bitflash-sh merged 1 commit into
Bitflash-sh:mainfrom
mtg0001:wallet-recovery-audit
Aug 3, 2026
Merged

Add wallet recovery balance audit#117
Bitflash-sh merged 1 commit into
Bitflash-sh:mainfrom
mtg0001:wallet-recovery-audit

Conversation

@mtg0001

@mtg0001 mtg0001 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a wallet recovery audit that separates spendable balance covered by the installed recovery phrase from balance that still depends on wallet.dat-only keys.
  • Show that split in the Wallet Safety dialog, with a warning when any spendable coins are outside the phrase.
  • Add a /recoveryaudit command-line diagnostic and include the same coverage check in the wallet HD self-test.

Why

After recovery phrases landed, the riskiest remaining wallet promise is not just deriving future receive/change keys. It is making sure the user can see whether today's spendable balance would actually come back from the words they wrote down. Pre-seed keys can still exist in wallet.dat, and if they hold coins the phrase alone is not a complete backup.

This PR does not try to finish BIP44 or migrate coins automatically. It makes the current recovery boundary visible and testable before a larger path/interop change.

Testing

  • git diff --check
  • make -f makefile.mingw tests from MSYS2 UCRT64 in src/

@Bitflash-sh

Copy link
Copy Markdown
Owner

Reviewed, built, and tested against real coin. Good work — this is the kind of contribution that lands well: read-only, one clear boundary, and a test that fails if the feature breaks.

What I verified

  • mingw32-make -f makefile.mingw tests in MSYS2 UCRT64: builds clean, all five self-tests pass, including the four new audit asserts.
  • Against a real wallet with real coin — the wallet from the change/restore proof that produced wallet: one quiet batch is not far enough to stop a restore #115. It has a phrase, nHDNext=701, and its coins sit at derived indices 201 (payment) and 302 (change):
Wallet recovery audit
  recovery phrase: present
  derived keys known to this wallet: 701
  total spendable balance:      5.00 BTF
  covered by recovery phrase:   5.00 BTF (1 transaction(s))
  wallet.dat-only balance:      0.00 BTF (0 transaction(s))

Correct on every count: it finds both outputs, recognises them as phrase-backed, and correctly skips the funding transaction that is already spent.

  • Derivation cost is not a problem: 701 derivations are lost in the measurement noise (< 0.3s), so refreshing the audit does not stall the dialog.
  • No lock inversion — DeriveHDKey does not take cs_keyPool, and cs_mapWallet -> cs_mapKeys is the order the rest of the tree already uses.
  • The filter matches GetBalance() exactly (IsFinal && !fSpent, immature coinbase worth zero), so the audit does not invent a second definition of "balance".
  • The self-test's mapWallet.clear() runs inside the temporary data directory, not against a real wallet.
  • The /showderived help correction is right, and worth calling out: the old text said "without installing it", which was simply false — CmdShowDerived uses the installed seed.

One thing I want fixed before this merges

Immature coinbase falls outside both buckets, and an empty bucket turns into a green reassurance.

GetWalletRecoveryAudit skips IsCoinBase() && GetBlocksToMaturity() > 0. Maturity here is 120 confirmations, which at a two-minute block target is about four hours of mining.

Concrete failure: a user mines for two hours with no phrase — every coin immature, all on random pre-seed pool keys — and then creates a recovery phrase. The audit computes zero legacy and zero recoverable, so the dialog shows

All currently spendable balance is covered by the phrase.

in green. They delete their wallet.dat backup on the strength of it. Four hours later those coins mature onto keys the phrase cannot reproduce.

This is the failure shape that has cost this project the most: a broken state that is indistinguishable from a healthy one, phrased as good news. Smallest fix that closes it: a third bucket (nImmatureCredit, split the same way by coverage), or at minimum suppress the green line while any uncovered immature coinbase exists. Miners are most of our users, so this path is not an edge case for us.

Smaller notes, none blocking

  1. if (!DeriveHDKey(i, key, strError)) break; truncates silently. Every coin at a higher index then reports as wallet.dat-only, and the user gets a "keep your backups" warning with no stated cause. Worth carrying the error out in the struct and showing it.
  2. The green line also shows on a zero balance — true but vacuous. Gate it on nRecoverableCredit > 0.
  3. The dialog is NoResize at a fixed 450px. Worst case (never backed up, backup path inside the data directory, a status string, and the legacy warning) overflows it. ImGui scrolls, so this is cosmetic.
  4. CmdRecoveryAudit returns exit code 2 as a signal; that is a reasonable choice but it is not documented in -help.

Fix the immature-coinbase gap and I will merge this.

@mtg0001
mtg0001 force-pushed the wallet-recovery-audit branch from 26da362 to e27d613 Compare August 3, 2026 13:38
@mtg0001

mtg0001 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in the updated branch.

What changed:

  • Immature coinbase outputs are now included in the audit instead of being skipped.
  • Immature rewards are split the same way as spendable balance: phrase-backed vs wallet.dat-only.
  • The Wallet Safety dialog warns when either spendable or immature balance is outside the recovery phrase.
  • The green reassurance is no longer shown for a zero-balance wallet, and it now says "all known wallet balance" only when there is actual covered balance.
  • Derivation truncation is surfaced through the audit result instead of silently making higher-index coins look like ordinary wallet.dat-only coins.
  • /recoveryaudit help now documents exit code 2 when wallet.dat is still needed.
  • The wallet HD self-test now includes the concrete bad case: an immature coinbase paid to a pre-seed key. It verifies that it lands in the wallet.dat-only immature bucket.

Re-tested:

  • git diff --check
  • make -f makefile.mingw tests from MSYS2 UCRT64 in src/

All self-tests pass, including the new immature-reward assertions.

@Bitflash-sh

Copy link
Copy Markdown
Owner

Re-reviewed and merging. The immature gap is closed properly: immature coinbase is now counted and split by coverage rather than dropped, so the totals still reconcile with GetBalance() — mature buckets sum to the balance, immature is reported alongside it instead of hiding inside it.

Verified on my side, not taken on trust:

  • Merged onto current main locally first — it lands clean against gui: fix the ImGui assertion when creating a recovery phrase #118, which also touched gui.cpp. Then built the merged tree, because a clean git merge is not a compiling merge in this codebase (a previous wallet branch merged without conflict and duplicated a function; only the compiler noticed).
  • mingw32-make -f makefile.mingw tests: builds clean, all five self-tests pass, including the two new immature assertions.
  • Your new self-test case is the real thing, not a stand-in: a default CTxIn gives a null prevout, so IsCoinBase() is genuinely true, and with no block attached GetBlocksToMaturity() returns the full 120. It exercises the path it claims to.
  • Re-ran -recoveryaudit against the real wallet from the wallet: one quiet batch is not far enough to stop a restore #115 proof (phrase installed, 701 derived indices, coins at 201 and 302): still 5.00 BTF, all phrase-backed, zero immature, exit 0. The change did not disturb the case that already worked.

Also good calls on the two smaller ones: surfacing the derivation truncation instead of letting it masquerade as legacy coins, and dropping the green line on an empty wallet.

Thanks — this is the third clean one in a row.

@Bitflash-sh
Bitflash-sh merged commit 1d0a4de into Bitflash-sh:main Aug 3, 2026
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.

2 participants