Files
ObsidianDragon/docs/wallet-hardening.md
DanS c7c3440a7b feat(diagnostics): refresh-staleness badge on the Total Balance card (W6-2)
When the wallet is connected but the balance has quietly stopped refreshing — a busy
daemon can fail z_gettotalbalance without dropping the whole connection (only *both*
core RPCs failing 3x triggers a disconnect) — the old number sits on screen while the
node-status banner stays hidden. The Total Balance card now shows a small pill on its
status line ("Updated 2m ago", amber, escalating to red past 3 min) so the stale value
isn't silently trusted; hovering explains it and points at the node connection.

No refresh-path changes: WalletState::last_balance_update is already stamped only on a
successful fetch (network_refresh_service.cpp), so the badge reads it and computes age
against the same std::time clock via util::formatTimeAgoShort. The decision is a pure,
unit-tested helper (ui/staleness_badge.h::evaluateStalenessBadge, 45s/180s thresholds)
gated on connected so it never contradicts the banner.

Closes P2 (5/5). Build-clean; ctest 1/1 (adds testStalenessBadge).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-02 20:48:56 -05:00

158 lines
22 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Wallet Loading & Management — Hardening Plan
Prioritized, grouped remediation for the wallet loading/management audit (33 verified findings +
diagnosability QoL). Companion to the findings artifact. Line references are against `dev`.
- **Provenance:** 7 parallel subsystem finders, each finding adversarially verified against the
code; the 3 highest-impact confirmed findings re-checked by hand. 32 confirmed, 1 refuted
(W1-5), 1 raised (W5-3 Low→Med).
- **Severity:** 8 High · 12 Medium · 13 Low.
Status legend: ☐ not started · ◐ in progress · ☑ landed & verified
---
## Roadmap (ordered by risk; shared fixes grouped)
| Phase | Findings | Theme | Status |
|-------|----------|-------|--------|
| **P0-A** | W7-1, W2-1, W4-1, W4-3, W2-3, W4-5, W5-3 ✓ | Secret hardening (console redaction + delete-export + memzero + lite encrypt-at-create) | ☑ 7/7 |
| **P0-B** | W2-2/W4-2, W2-4 | Encryption integrity (never silently unencrypted) | ☑ |
| **P1-A** | W3-1, W3-2, W3-4 ✓ · W3-3 ⚑ | Migrate-to-seed correctness (fund-adjacent) | ◐ 3/4 |
| **P1-B** | W1-1, W1-2, W1-3, W1-4 ✓ + startup guard | Missing/wrong wallet-file safety | ☑ |
| **P2** | W5-1, W5-2, W6-1, W6-3, W6-2 ✓ | Stale state & lite save-failure surfacing | ☑ 5/5 |
| **F** | W7-2, W7-3, W7-4 ✓ · QoL: copy-diag + open-log + node-error-banner + staleness-badge ✓, alert-history ☐ | Diagnostics foundation + QoL bundle | ◐ |
---
## P0-A — Secret hardening
Shared fix: a `SecureString` RAII buffer (zeroes on destruction) retrofitted onto the un-scrubbed
key/passphrase paths, plus console redaction and deleting the plaintext export.
- **W7-1 (High)** `console_tab.cpp:1419` — RPC console echoes/stores/clipboards raw secrets. Fix: an
allowlist of secret-bearing first-tokens (`walletpassphrase`, `walletpassphrasechange`,
`encryptwallet`, `importprivkey`, `importwallet`, `z_importkey`, `z_importviewingkey`,
`signrawtransaction`, `magicrecoverkey`, lite equivalents); echo `> walletpassphrase ****` and
keep the raw text out of `command_history_`. Extract a pure `redactConsoleCommand(cmd)` helper for
unit testing. **← implementing first (self-contained + testable).**
- **W2-1 (High)** `wallet_security_workflow.cpp:66` — delete the `obsidiandecryptexport<ts>` plaintext
key dump after `z_importwallet` succeeds (overwrite-then-unlink).
- **W4-3 (High)** `app_network.cpp:4481``sodium_memzero` the concatenated all-keys string in
`exportAllKeys`; write the backup 0600. (Also unify with `ExportAllKeysDialog` — QoL.)
- **W4-1 (High)** `app_network.cpp:3801` — zero the key copies in `importPrivateKey`/`sweepPrivateKey`
(local + worker-lambda copies).
- **W2-3 (Med)** `app_security.cpp:1481` — zero the passphrase threaded through the decrypt lambda chain.
- **W4-5 (Med)** `app.cpp:3577` — the seed-backup `.txt` is a permanent predictable cleartext seed;
at minimum warn + offer to delete, ideally discourage file save in favor of the on-screen phrase.
- **W5-3 (Med)** `lite_wallet_lifecycle_service.cpp:322` — remove the dead `passphrase` field from the
lite create/open/restore requests (unused; a secret copied for nothing).
## P0-B — Encryption integrity
- **W2-2 / W4-2 (High)** `wallet_security_controller.h:89` — the wizard's deferred encryption is
in-memory only and silently lost if the daemon doesn't connect or the app quits/crashes first, so a
wallet the user believes is encrypted stays plaintext. Fix: persist a lightweight
`encryption_requested_but_incomplete` settings flag (NEVER the passphrase) when
`beginDeferredEncryption` is called; surface a persistent warning banner while it's set; clear it
only on confirmed `encryptwallet` success; on next connect, if set, re-prompt for the passphrase to
complete it.
- **W2-4 (Med)** `app_security.cpp:480``lockWallet` only sets `locked` on RPC success; log the
failure and notify (currently a silent no-op that can leave the wallet unlocked).
## P1-A — Migrate-to-seed correctness (fund-adjacent; verify carefully)
- **W3-1 (High)** `app_network.cpp:4327` — adopt hardcodes `datadir + "/wallet.dat"`; use
`settings_->getActiveWalletFile()` so migrating a non-default active wallet swaps the right file.
- **W3-2 (High)** `seed_wallet_creator.cpp:57``remove_all(<config>/seed-migrate)` unconditionally
at Phase-1 start; refuse to wipe if a temp `DRAGONX/wallet.dat` already exists (a prior un-adopted
swept wallet) and surface it, so swept funds in the temp wallet can't be destroyed by re-entry.
- **W3-4 (Med)** `app_network.cpp:1124` — block wallet switching while a migration is *pending*
(`getSeedMigrationPending()`), not only while the dialog is open.
- **W3-3 (Med)** `app_network.cpp:4231` — persist the sweep opid so an app-close mid-Sweeping can
resume/re-poll it instead of silently dropping the txid.
## P1-B — Missing/wrong wallet-file safety
- **W1-1 (High)** `app_network.cpp:1109``fs::exists()`-check the target wallet file in
`switchToWallet()` and before the first daemon launch at startup; if missing, block with an explicit
"Wallet file not found — moved or deleted?" dialog (browse / create-new) instead of letting the
daemon fabricate an empty wallet.
- **W1-3 (Med)** `app_network.cpp:1095` — defer the `syncedHere=true` stamp to the first successful
address/balance readback (idHash non-empty), not bare `onConnected()`.
- **W1-2 (Med)** `app_network.cpp:198` — split `DB_CORRUPT`-specific strings from the generic "Error
loading wallet" fallback; give `DB_TOO_NEW` its own message/action (not a salvage offer).
- **W1-4 (Low)** `wallets_dialog.h:393` — re-`fs::exists()` the in-datadir row before switching (match
the out-of-datadir path).
## P2 — State & lite persistence
- **W6-2 (Med)** `network_refresh_service.cpp:1183` — record a per-field last-success timestamp / a
"refresh failed" flag so the UI can show a staleness badge instead of last-good-as-current.
- **W5-1 / W5-2 (Med)** `lite_wallet_controller.cpp:78,603``liteLog()` the failed save and bubble a
one-shot UI warning (both call sites currently discard the bool).
- **W6-1 (Med)** `wallet_state.h:313` — reset `mining`/`pool_mining` in `clear()` (or comment why not).
- **W6-3 (Low)** `address_book.cpp:46` — per-entry try/catch: skip + count malformed entries instead
of discarding the whole list.
## F — Diagnostics foundation + QoL
Land W7-2 first — it unblocks the rest.
- **W7-2 (Med)** `logger.cpp:31` — call `Logger::instance().init(<config>/dragonx-debug.log)` early in
`main()` on all platforms; add an "Open log folder" action.
- **W7-3 (Med)** `main.cpp:144` — add a `sigaction`-based crash handler writing `dragonx-crash.log` on
POSIX (mirror the Windows SEH path).
- **W7-4 (Low)** `logger.cpp:39` — size-cap/rotate the log on `init()`.
- **QoL** — "Copy diagnostics for support" bundle; persistent alert history; daemon/RPC error banner;
refresh-staleness badge; multi-wallet diagnostic panel; refresh-diagnostics panel; structured
switch/migration audit logging; restore-from-seed entry point (W4-4, effort L).
---
## Progress log
- **W6-2 / Refresh-staleness badge** — ☑ landed. The Total Balance card now shows a small pill on its status line ("Updated 2m ago", amber → red past 3 min) **only when connected but the balance stopped refreshing** — a busy daemon can fail `z_gettotalbalance` without dropping the whole connection (only *both* core RPCs failing 3× triggers a disconnect), leaving stale numbers on screen while the node-status banner stays hidden. No refresh-path changes were needed: `WalletState::last_balance_update` is already stamped only on a successful fetch (`network_refresh_service.cpp:1187`), so the badge just reads it and computes age against the same `std::time` clock (`util::formatTimeAgoShort`). Decision is a pure, unit-tested helper (`ui/staleness_badge.h::evaluateStalenessBadge`, thresholds 45s/180s) gated on `connected` so it never contradicts the banner; hover shows a "may be out of date — check your node connection" tooltip. Build-clean; `ctest` 1/1 (adds `testStalenessBadge`). **This closes P2 (5/5).**
- **Foundation QoL / Persistent node-status banner** — ☑ landed. A persistent horizontal strip now sits at the top of the content column whenever the wallet can't reach its node — distinct from the transient toasts, so an offline wallet is never silently mistaken for a working one. The show/severity/action decision is a pure function (`ui/node_status_banner.h``evaluateNodeStatusBanner`, unit-tested) fed a state snapshot by `App::renderNodeStatusBanner()`. Three cases: **full-node offline** (amber, "Reconnect" → `tryConnect`), **embedded daemon crashed & auto-restart gave up** (red, "Restart node" → `restartDaemon`), **lite wallet failed to open** (red, message-only). Suppressed during the wizard / wallet-switch / daemon-restart / screenshot-sweep / shutdown, and while an expected startup phase (warmup/init/connect-in-progress) already owns the screen. Height in `res/themes/ui.toml` (`banners.node-status`); colours from the material semantic palette; detail text ellipsis-clipped so it can't shove the action button off-screen. Build-clean; `ctest` 1/1 (added `testNodeStatusBanner`). **Remaining QoL:** persistent alert history, and the W6-2 refresh-staleness badge.
- **Foundation QoL / "Copy diagnostics" + "Open log folder"** — ☑ landed: Settings (logging section) now has two actions. **Open log folder** opens the config dir (`Platform::openFolder`) so users can actually find `dragonx-debug.log`/`dragonx-crash.log`. **Copy diagnostics** copies a plaintext support snapshot to the clipboard via the new `App::buildDiagnosticsReport()` — version, build variant, platform, connection status, active wallet path + existence + size, encryption/lock state, sync heights, daemon status/running/crash-count/lastError (full-node), and the log paths. No secrets. Build-clean; `ctest` 1/1. **Remaining QoL:** persistent alert history, a daemon/RPC error banner, and the W6-2 refresh-staleness badge.
- **Foundation / W7-2 · W7-3 · W7-4 (diagnostics infrastructure)** — ☑ landed (answers the original "easier to diagnose" ask — the logging/crash foundation now actually works):
- **W7-2 (Med, keystone):** the app-level `Logger` file sink was never initialized, so `LOG`/`LOGF`/`VERBOSE_LOGF` went nowhere and `dragonx-debug.log` didn't exist on Linux/macOS at all. `main()` now calls `Logger::init(<config>/dragonx-debug.log)` on all platforms. Also fixed a **latent deadlock** this exposed: `init()` wrote its banner via `write()`, which re-locks the non-recursive `mutex_` it already holds — now written directly. On Windows the raw stdout/stderr `freopen` was moved to a separate `dragonx-stdout.log` so the two writers don't contend. New `testLoggerFileSink` (also a deadlock guard — it would hang if that regressed).
- **W7-3 (Med):** no crash handler existed on Linux/macOS. Added an **async-signal-safe** `sigaction` handler (SIGSEGV/ABRT/BUS/FPE/ILL) that writes a signal id + `backtrace_symbols_fd` backtrace to `dragonx-crash.log`, then re-raises the default disposition for a core dump — the POSIX counterpart of the Windows SEH filter.
- **W7-4 (Low):** `Logger::init` now rotates the log to a single `.1` backup when it exceeds 10 MB, so a long/verbose session can't grow it unbounded.
Build-clean; `ctest` 1/1. **Remaining Foundation:** the QoL bundle (mostly UI) — "copy diagnostics for support", an "open log folder" action, persistent alert history, a daemon/RPC error banner, and the W6-2 refresh-staleness badge.
- **P2 / W5-1 · W5-2 · W6-1 · W6-3 (localized batch)** — ☑ landed:
- **W5-1 (Med):** `persistAfterBroadcast` (lite send/shield save) returned false on a persistent save failure but both callers discarded it and it never logged — completely silent. It now `liteLog`s the failure (the note re-derives on next sync, so it's a robustness gap, not fund loss).
- **W5-2 (Med):** the post-**sync** and post-**rescan** `save` results (in the detached scan threads) were ignored; both now `liteLog` on failure (`LiteDiagnostics::log` is mutex-guarded, safe from those threads).
- **W6-1 (Med):** `WalletState::clear()` didn't reset `mining`/`pool_mining`, so a wallet switch could briefly show the previous wallet's hashrate/blocks. Now reset in `clear()` (the daemon restarts on switch, so mining genuinely stops).
- **W6-3 (Low):** `AddressBook::load()` did `entries_.clear()` then threw on the first non-object element — discarding **every** contact. Now it guards `is_object()` + per-entry try/catch, skipping and counting malformed entries.
Build-clean; `ctest` 1/1. **Remaining P2:** W6-2 (surface refresh staleness — the timestamps exist in `WalletState`; this needs the UI "updated Xs ago" badge, which overlaps the diagnostics/QoL Foundation bundle).
- **P1-B / W1-3 + startup wallet-existence guard** — ☑ landed:
- **W1-3 (Med):** `syncedHere` was stamped in the `markOpened` block at bare connect (idHash still empty), letting a freshly-restored wallet skip its needed rescan. It's now stamped only once the identity is verified (idHash non-empty), so it takes effect at the post-address-refresh index update (`updateWalletIndexForActiveWallet` after addresses load), while `lastOpenedEpoch` still records at open.
- **Startup guard (the W1-1 launch counterpart):** `App::init` now `exists()`-checks the recorded active wallet before the daemon is configured; a **non-default** active wallet that was moved/deleted between sessions falls back to the default `wallet.dat` with a warning, instead of the daemon silently auto-creating an empty wallet under the missing name. Runs before the PIN-vault init so the vault is scoped to the wallet actually opened.
Build-clean; `ctest` 1/1.
- **P1-A / W3-3 (sweep opid persistence)** — ⚑ **deferred for careful, adversarially-reviewed work** (not rushed). `trackOperation` only enqueues the opid for a background poller; a resume that re-tracks a persisted opid is only safe if the poller times out a *stale* opid (daemon restarted → op gone) rather than polling forever — otherwise a resume would hang the migration permanently, worse than today's re-sweep. Verifying that (and the double-sweep interactions) is exactly the "two rounds of adversarial review + a live mainnet run" the migration code mandates. The existing safety gates (adopt requires the legacy balance ~0 AND the sweep tx mined) already prevent fund *loss* on a mid-sweep interruption; W3-3 is a stuck-state robustness improvement, so it can wait for a dedicated pass.
- **P1-B / W1-1 (+ W1-4) · W1-2 (wallet-file safety)** — ☑ landed:
- **W1-1 (High):** `switchToWallet` never checked the target wallet file exists, so a moved/deleted file "opened" as a fresh empty wallet (dragonxd auto-creates for a missing `-wallet=`), looking exactly like fund loss. It now `std::filesystem::exists`-checks `datadir + "/" + walletFile` before switching and blocks with a "not found (moved or deleted?)" warning. Placed before the daemon-stop prompt, and — since the check runs no matter how `switchToWallet` is invoked — it also **closes W1-4** (the stale-switcher-row TOCTOU).
- **W1-2 (Med):** `walletOutputLooksCorrupt` matched the generic "Error loading wallet" string, so a `DB_TOO_NEW` (newer-version) wallet was offered a `-salvagewallet` repair that can't fix it. Now the generic match is excluded when the output also contains "newer version".
Build-clean; `ctest` 1/1. **Remaining P1-B:** W1-3 (defer the `syncedHere` stamp to a verified readback) + the startup-path existence check (`app.cpp` hands `getActiveWalletFile()` to the daemon with no `exists()` check — same silent-empty-wallet risk as W1-1 but at launch).
- **P1-A / W3-1 · W3-2 · W3-4 (migrate-to-seed correctness)** — ☑ landed (fund-adjacent — reviewed carefully):
- **W3-1 (High):** `beginAdoptSeedWallet` hardcoded `datadir + "/wallet.dat"` as the file to swap. With a non-default active wallet (e.g. `wallet-2.dat`), that installed the swept seed wallet into an unloaded `wallet.dat` and left the daemon reloading the emptied legacy — funds only recoverable via the seed phrase. Now swaps `datadir + "/" + getActiveWalletFile()` (captured on the main thread; switching is blocked during migration so it can't race).
- **W3-2 (High):** `SeedWalletCreator::create` did `remove_all(<config>/seed-migrate)` unconditionally at the start. A prior migration that swept funds into the temp wallet but was abandoned/crashed before adopting would have that fund-bearing wallet destroyed. It now refuses (with a clear message) when `DRAGONX/wallet.dat` already exists — a completed migration removes the dir on adopt, so a leftover means an unfinished one.
- **W3-4 (Med):** `switchToWallet` only blocked switching while the migration *dialog* was open; closing it via "Later" mid-migration dropped the guard. Now also blocks while `getSeedMigrationPending()`.
Build-clean; `ctest` 1/1. **Remaining P1-A:** W3-3 (persist the sweep opid so an app-close mid-sweep can resume/re-poll instead of silently dropping the txid).
- **P0-B / W2-2 (deferred encryption silently lost) + W2-4 (auto-lock silent-fail)** — ☑ landed:
- **W2-2:** the wizard's deferred encryption was stored only in memory, so a quit/crash or a failed daemon connect before it applied left the wallet unencrypted with **no record it was ever requested** — the user believing it was encrypted. Now a persisted `encryption_pending` settings flag is set the moment encryption is requested (**never the passphrase** — only the fact). `refreshWalletEncryptionState()` reconciles it on every connect: wallet observed **encrypted** → clear the flag; wallet **not** encrypted while the flag is set and no deferred encryption is pending/in-flight → a once-per-session **"your wallet is NOT encrypted — open Settings to finish"** warning (the flag stays set, so it recurs each launch until resolved). We deliberately don't persist the passphrase to auto-complete — surfacing it is the secure choice.
- **W2-4:** `lockWallet()`'s continuation only handled success — a failed `walletlock` silently left the wallet **unlocked** (an unfulfilled auto-lock). It now logs and warns once (reset on the next successful lock), so a failing auto-lock is visible instead of leaving the wallet exposed.
Touches `settings.{h,cpp}`, `app_wizard.cpp`, `app_security.cpp`, `app.h`. Not unit-testable at this layer (RPC/connect-driven state machine); build-clean, `ctest` 1/1.
- **P0-A / W5-3 (lite create-time passphrase)** — ☑ landed (chose option **(b) wire it up**). The lite create/open/restore passphrase was collected but never consumed by the backend — a "passphrase" field that did nothing. It now has a real meaning for all three operations, in `LiteWalletController`: **create/restore**`encryptWallet(passphrase)` (the backend encrypts + locks + saves the brand-new wallet); **open**`unlockWallet(passphrase)`, but only when `encryptionStatus()` reports the existing wallet is actually encrypted+locked (skips a spurious unlock otherwise). Encrypt/unlock take their own copy and wipe it; a post-create encrypt failure is `liteLog`'d (the wallet still exists — the create isn't failed). Six existing lite-controller tests carried an incidental `hunter2` create passphrase from the dead-field era; removed (they test non-encryption flows and want an unencrypted wallet), and added `testLiteWalletControllerCreateEncryptsWithPassphrase` to prove the new behavior. Build-clean; `ctest` 1/1. *(Follow-up UX polish: `settings_page` could show the passphrase field's meaning per operation — "encrypt" for create/restore vs "unlock" for open.)*
- **P0-A / W4-5 (seed-backup file)** — ☑ landed (proportionate): the seed "Save" already wrote 0600 + zeroed the in-memory buffer, but the success message was a bare "Saved to <path>". It now reads "**Saved an UNENCRYPTED seed file — move it to secure offline storage and delete this copy**: <path>", so the plaintext-on-disk risk is called out. `i18n.cpp` (English source; `res/lang` back-fill of this changed key is deferred to the batch i18n pass). A stronger fix (pre-save confirmation, or dropping the file-save in favor of on-screen + Copy) is a follow-up UX decision.
- **P0-A / W4-1 · W4-3 · W2-3 (memzero cluster)** — ☑ landed, using the file's established `sodium_memzero` pattern (matching the existing lambda-capture scrub at app_network.cpp:2885 and JSON scrub at :4025) rather than a new type, since this is fund-moving code:
- **W4-1** `importPrivateKey`/`sweepPrivateKey`: the spending/viewing key is now scrubbed on all paths — the calling-frame copy (after the worker post), the worker-lambda's captured copy (lambda made `mutable`, zeroed after the request is sent), and the JSON request `params` copy.
- **W4-3** `exportAllKeys`/`backupWallet`: the concatenated all-keys buffer is zeroed after the consumer uses it, and the backup file is now written via `Platform::writeFileAtomically(..., restrictPermissions=true)` (atomic + 0600) instead of a umask-default `ofstream`.
- **W2-3** decrypt-wallet passphrase: `std::move`-captured into the worker lambda (so no plaintext copy is left in the calling frame) and `sodium_memzero`'d right after `unlockWallet` (its only use).
Not unit-testable (the scrubbing has no observable RPC effect — the key value sent to the daemon is unchanged; only post-use memory zeroing is added). Build-clean; `ctest` 1/1 (no regression). **Remaining in P0-A:** W5-3 (remove the dead lite `passphrase` field), W4-5 (predictable plaintext seed-backup file).
- **P0-A / W2-1** — ☑ landed: the decrypt-wallet flow now scrubs (best-effort in-place zero-overwrite) and removes the plaintext key export (`obsidiandecryptexport…`) as soon as the `z_importwallet` attempt resolves — success or failure — so a full cleartext dump of every private key is no longer left on disk forever. Recovery remains the encrypted backup (`wallet.dat.encrypted.bak`). `app_security.cpp` (after the import call). Not unit-testable (fs I/O in a deep lambda); build-clean, `ctest` 1/1 (no regression).
- **P0-A / W7-1** — ☑ landed: `RedactConsoleCommand`/`ConsoleCommandCarriesSecret` in `console_tab_helpers` redact secret-bearing commands (an allowlist of 13 first-tokens: `walletpassphrase`, `encryptwallet`, `z_importkey`, …) to `> walletpassphrase ****` before they hit the console echo AND the recall history; the real command still executes unredacted. Wired into `submitConsoleCommand` (`console_tab.cpp`). New `testConsoleSecretRedaction` (11 assertions). Clean build; `ctest` 1/1. (Output-secret commands like `z_exportkey` — result redaction — remain a follow-up.)