2 Commits

Author SHA1 Message Date
05b00b158b fix(wallet): surface silent save failures + stale-state cleanups (W5-1, W5-2, W6-1, W6-3)
P2 robustness batch (localized):

- W5-1 (Med, lite): persistAfterBroadcast returned false on a persistent post-send/shield
  save failure, but both callers discarded it and it never logged — completely silent. It
  now liteLogs the failure (the spent note re-derives on the next sync, so it's a
  robustness gap, not fund loss).

- W5-2 (Med, lite): 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,
  so it's 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() cleared entries_ then threw on the first non-object array
  element — discarding EVERY contact. It now guards is_object() + per-entry try/catch,
  skipping and counting malformed entries.

Build-clean; ctest 1/1. Remaining P2: W6-2 (refresh-staleness badge — needs UI, overlaps
the diagnostics Foundation bundle). See docs/wallet-hardening.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-02 15:22:13 -05:00
f9ddab059e fix(wallet): stamp syncedHere only after identity verified + guard the startup wallet file (W1-3)
- W1-3 (Med): updateWalletIndexForActiveWallet stamped syncedHere in the markOpened block
  at bare connect (idHash still empty), letting a freshly-restored wallet skip its needed
  rescan. syncedHere is now stamped only once the wallet's identity is verified (idHash
  non-empty), so it takes effect at the post-address-refresh index update; 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 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.

Completes P1-B. Remaining P1: W3-3 (sweep opid persistence) deferred for careful
adversarially-reviewed work — re-tracking a stale opid could hang the migration if the op
poller doesn't time out; the existing balance/mined gates already prevent fund loss. See
docs/wallet-hardening.md.

Build-clean; ctest 1/1.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-02 15:18:09 -05:00
6 changed files with 73 additions and 20 deletions

View File

@@ -18,9 +18,9 @@ Status legend: ☐ not started · ◐ in progress · ☑ landed & verified
|-------|----------|-------|--------| |-------|----------|-------|--------|
| **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-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) | ☑ | | **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-A** | W3-1, W3-2, W3-4 ✓ · W3-3 | Migrate-to-seed correctness (fund-adjacent) | ◐ 3/4 |
| **P1-B** | W1-1, W1-2, W1-4 ✓ · W1-3 ☐ | Missing/wrong wallet-file safety | ◐ 3/4 | | **P1-B** | W1-1, W1-2, W1-3, W1-4 ✓ + startup guard | Missing/wrong wallet-file safety | |
| **P2** | W6-2, W5-1, W5-2, W6-1, W6-3 | Stale state & lite save-failure surfacing | | | **P2** | W5-1, W5-2, W6-1, W6-3 ✓ · W6-2 ☐ | Stale state & lite save-failure surfacing | ◐ 4/5 |
| **F** | W7-2, W7-3, W7-4, QoL | Diagnostics foundation + QoL bundle | ☐ | | **F** | W7-2, W7-3, W7-4, QoL | Diagnostics foundation + QoL bundle | ☐ |
--- ---
@@ -112,6 +112,17 @@ Land W7-2 first — it unblocks the rest.
## Progress log ## Progress log
- **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: - **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-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". - **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".

View File

@@ -341,6 +341,28 @@ bool App::init()
// Ensure ObsidianDragon config directory and template files exist // Ensure ObsidianDragon config directory and template files exist
util::Platform::ensureObsidianDragonSetup(); util::Platform::ensureObsidianDragonSetup();
// W1-1 (startup): if the recorded active wallet file was moved/deleted between sessions, don't hand a
// missing -wallet=<name> to the daemon — it would auto-create a fresh empty wallet under that name,
// silently "opening" as a zero-balance wallet at launch. Fall back to the always-present default and
// warn. (The default "wallet.dat" is legitimately absent on first run, so it is skipped.) Runs before
// the vault init below so the vault is scoped to the wallet actually opened.
if (settings_) {
const std::string active = settings_->getActiveWalletFile();
if (!active.empty() && active != "wallet.dat") {
std::error_code walEc;
const std::string walPath = util::Platform::getDragonXDataDir() + "/" + active;
if (!std::filesystem::exists(walPath, walEc)) {
DEBUG_LOGF("[App] active wallet '%s' not found at startup — falling back to wallet.dat\n",
active.c_str());
settings_->setActiveWalletFile("wallet.dat");
settings_->save();
ui::Notifications::instance().warning(
"Your last-used wallet file (" + active + ") was not found — opened the default wallet "
"instead. If you moved it, restore it and switch back from the wallet list.", 20.0f);
}
}
}
// Initialize PIN vault, scoped to the active wallet so one wallet's stored passphrase is never // Initialize PIN vault, scoped to the active wallet so one wallet's stored passphrase is never
// offered for another (the default wallet keeps the legacy vault.dat). // offered for another (the default wallet keeps the legacy vault.dat).
vault_ = std::make_unique<util::SecureVault>(settings_ ? settings_->getActiveWalletFile() : ""); vault_ = std::make_unique<util::SecureVault>(settings_ ? settings_->getActiveWalletFile() : "");

View File

@@ -1096,9 +1096,16 @@ void App::updateWalletIndexForActiveWallet(bool markOpened)
if (!ec) e.sizeBytesAtLastOpen = static_cast<long long>(sz); if (!ec) e.sizeBytesAtLastOpen = static_cast<long long>(sz);
} }
// W1-3: record "synced here" only once the wallet's identity is actually verified (its addresses are
// known -> idHash non-empty). Stamping it on the bare connect (before any address readback) would let
// a freshly-restored wallet skip its needed rescan. It's idempotent, so the post-refresh update
// (updateWalletIndexForActiveWallet after addresses load) sets it once; lastOpenedEpoch is still
// recorded at open time here.
if (!idHash.empty()) {
e.syncedHere = true; // loaded + identity-verified in this datadir -> catch-up (no full rescan)
}
if (markOpened) { if (markOpened) {
e.lastOpenedEpoch = static_cast<long long>(std::time(nullptr)); e.lastOpenedEpoch = static_cast<long long>(std::time(nullptr));
e.syncedHere = true; // we've loaded it in this datadir -> catch-up (no full rescan) on switch
} }
if (wallet_index_.upsert(e)) wallet_index_.save(); if (wallet_index_.upsert(e)) wallet_index_.save();

View File

@@ -46,7 +46,12 @@ bool AddressBook::load()
entries_.clear(); entries_.clear();
if (j.contains("entries") && j["entries"].is_array()) { if (j.contains("entries") && j["entries"].is_array()) {
size_t skipped = 0;
for (const auto& entry : j["entries"]) { for (const auto& entry : j["entries"]) {
// W6-3: skip (and count) a malformed element rather than letting one bad entry throw and
// abort the whole load — which would discard EVERY contact (entries_ was already cleared).
if (!entry.is_object()) { ++skipped; continue; }
try {
AddressBookEntry e; AddressBookEntry e;
e.label = entry.value("label", ""); e.label = entry.value("label", "");
e.address = entry.value("address", ""); e.address = entry.value("address", "");
@@ -55,11 +60,11 @@ bool AddressBook::load()
// multi-wallet scoping lands — a contact you already had stays visible everywhere. // multi-wallet scoping lands — a contact you already had stays visible everywhere.
e.scope = entry.value("scope", "global"); e.scope = entry.value("scope", "global");
e.avatar = entry.value("avatar", ""); e.avatar = entry.value("avatar", "");
if (!e.address.empty()) entries_.push_back(e);
if (!e.address.empty()) { } catch (const std::exception&) { ++skipped; }
entries_.push_back(e);
}
} }
if (skipped > 0)
DEBUG_LOGF("Address book: skipped %zu malformed entr%s\n", skipped, skipped == 1 ? "y" : "ies");
} }
DEBUG_LOGF("Address book loaded: %zu entries\n", entries_.size()); DEBUG_LOGF("Address book loaded: %zu entries\n", entries_.size());

View File

@@ -335,6 +335,10 @@ struct WalletState {
transactions.clear(); transactions.clear();
peers.clear(); peers.clear();
bannedPeers.clear(); bannedPeers.clear();
// W6-1: reset node-level mining state too — the daemon restarts on a wallet switch (mining
// stops), so leaving the previous wallet's hashrate/blocks would show stale mining stats.
mining = MiningInfo{};
pool_mining = PoolMiningState{};
} }
// Rebuild combined addresses list from z/t lists // Rebuild combined addresses list from z/t lists

View File

@@ -80,8 +80,10 @@ bool persistAfterBroadcast(LiteClientBridge& bridge)
for (int attempt = 0; attempt < 2; ++attempt) { for (int attempt = 0; attempt < 2; ++attempt) {
if (bridge.execute("save", "").ok) return true; if (bridge.execute("save", "").ok) return true;
} }
// Persistent failure: the spent note will be re-derived from the chain on the next sync, // Persistent failure: the spent note will be re-derived from the chain on the next sync, so this
// so this is a robustness gap, not fund loss. (Retry handles the common transient case.) // is a robustness gap, not fund loss. Log it (W5-1) — both callers discard this return, so the
// failure was previously completely silent.
liteLog("save failed after send/shield — the wallet will re-derive it on the next sync");
return false; return false;
} }
@@ -600,7 +602,8 @@ void LiteWalletController::startSync()
// The backend does NOT auto-save after a sync, so persist the freshly-scanned wallet; // The backend does NOT auto-save after a sync, so persist the freshly-scanned wallet;
// otherwise the next launch re-scans from the checkpoint (~30 min). Set `done` only // otherwise the next launch re-scans from the checkpoint (~30 min). Set `done` only
// after the save so a syncComplete() observer sees a fully-persisted wallet. // after the save so a syncComplete() observer sees a fully-persisted wallet.
bridge->execute("save", ""); if (!bridge->execute("save", "").ok) // W5-2: don't leave a failed post-sync save silent
liteLog("save failed after sync — the next launch will re-scan from the checkpoint");
} }
done->store(true); done->store(true);
}); });
@@ -631,7 +634,8 @@ bool LiteWalletController::startRescan()
// `rescan` clears the wallet's synced block cache and re-downloads/re-scans from the // `rescan` clears the wallet's synced block cache and re-downloads/re-scans from the
// birthday height — a blocking, uninterruptible full scan, same as `sync`. // birthday height — a blocking, uninterruptible full scan, same as `sync`.
bridge->execute("rescan", ""); bridge->execute("rescan", "");
bridge->execute("save", ""); // backend doesn't auto-save after a rescan if (!bridge->execute("save", "").ok) // W5-2: don't leave a failed post-rescan save silent
liteLog("save failed after rescan — the next launch will re-scan from the checkpoint");
} }
done->store(true); done->store(true);
}); });