fix: masternode/evonode identity load deadlock + identity unload capability (#889)

[Claudius-Maginificent]

TL;DR: Fixes a masternode/evonode identity load that could get stuck forever, adds a working "unload this identity" action so a mixed-up load — or one you just want to forget — can be recovered from without deleting your whole wallet, and hardens that action's data-clearing, wipe-completeness, and disclosure accuracy against edge cases found across multiple review passes.

User story

As a masternode operator, I want loading my masternode/evonode identity to succeed even if that identifier was previously loaded the wrong way, and I want to be able to unload a wrongly-loaded identity from this device and have it stay unloaded, to achieve recovering from a mixed-up load without losing my wallet or being permanently stuck.

Scenario

Base flow

Before the dedicated Masternodes tab existed (or before a user realized an identifier belonged to a masternode), it was possible to load that identifier through the regular "Load Existing Identity" flow. Later, the same identifier is loaded again through the correct flow — the Masternodes tab for a node, or the regular flow for a plain identity.

Actual behavior

A masternode ProTxHash and a regular identity ID are both opaque 32-byte identifiers — the app cannot tell them apart by format. An incomplete ("bare") record left behind by the first, wrong load makes the app believe the identity is already loaded, and it refuses the second, correct load. There was also no way to undo the situation: no action existed to unload a stuck or wrongly-loaded identity from the device, and an unloaded identity could silently reappear on the next wallet-unlock or background scan.

Expected behavior

The duplicate check now recognizes a bare/incomplete existing record and lets the correct load through. Loading also verifies, after fetching the identity from the network, that its actual type matches what was requested — in both directions: a regular identity loaded via the Masternode/Evonode entry point is rejected too, not just the reverse. Identity Hub → Settings has a working "Unload this identity from this device" action that stays unloaded — a background scan or wallet unlock won't bring it back until you explicitly load it again — without affecting any other identity or the wallet itself. Removing an identity from the Identities screen or a masternode's detail view goes through the same unload logic and shows the same accurate confirmation, so it's consistent everywhere. The confirmation names any scheduled DPNS votes that will be cancelled, unambiguously identifies the identity even if its alias is shared with another one, and — for a masternode or evonode — correctly explains it can be reloaded by its ProTxHash while its private keys (if it had any) must be re-entered by hand. "Clear Database" (the full local-data wipe) now keeps every identity reserved until the whole operation is done, so a concurrent load can no longer restore identity data after the wipe has already passed it and still have the wipe report success.

Detailed discussion

What was done

• Fix chore: impl ContextProvider #1 — bare-aware duplicate check (src/backend_task/identity/load_identity.rs): a RejectIfExists load no longer bounces off an empty placeholder record with no keys, alias, or role assignment.

• Fix Fix matrix strategy for ARM and AMD Builds #2 — entry-point type validation, both directions (src/backend_task/identity/load_identity.rs, src/model/qualified_identity/mod.rs): after fetching the identity from the network, its key set is checked for a Purpose::OWNER key to confirm it actually matches the requested load type. A regular identity loaded via the Masternode/Evonode form is now rejected too (TaskError::IdentityIsNotMasternode), not just the reverse case.

• Fix feat: register usernames #3 — identity unload/removal, with a durable "forgotten" marker (src/context/identity_db.rs, src/backend_task/identity/{unload_identity,discover_identities,remove_identity}.rs, src/wallet_backend/dashpay.rs, src/ui/identity/{settings,hub_screen,profile_cache}.rs, src/ui/identities/identities_screen.rs, src/ui/masternodes/detail_screen.rs): IdentityTask::UnloadIdentity clears the identity's vault keys, DashPay overlays/timestamps/address mappings, and its local database record, reusing the same load-mutual-exclusion guard (identity_load_registry) that loads use. A durable, per-identity marker records the unload (storage mechanism corrected in Fix feat: hide document button #7 below) so discovery (background scans and wallet-unlock) leaves it unloaded; only an explicit user-driven load clears the marker. In-memory state (wallet cache, selected/pending identity, cached DashPay profile) is reconciled correctly even when storage deletion committed but a secondary cleanup step failed. The Identities screen's "Remove" action and the Masternode detail screen's removal action both go through this same logic. Wired to a real confirmation dialog in Identity Hub → Settings that captures its target at open time (avoiding a stale-target bug if the selected identity changes while the dialog is open).

• Fix Feat/register usernames2 #4 — review-driven hardening: three review passes (a reviewer comment pass, a full multi-agent security/consistency/QA audit, and a follow-up automated re-review of the resulting changes) surfaced 17 additional gaps, all fixed:

  • A discovery/unload race where a competing claim on the same identity could be released before the caller's wallet-cache insert, letting a stale entry survive.
  • Two load paths that masked cleanup-marker failures behind a bare ? instead of routing through the shared post-persist recovery helper.
  • The masternode detail screen's identity removal now dispatches the same asynchronous RemoveIdentity backend task the rest of the app uses, instead of a separate synchronous DB/vault deletion run on the UI thread; the detail view closes only on a matching successful removal.
  • DashPay profile results are now generation/identity-guarded on the same two screens (dashpay_screen, profile_screen) that display them, closing a late-result gap the original fix only covered on the Identity Hub.
  • clear_network_database ("Clear Database") now retries a per-identity wipe a bounded number of times when it collides with a transient competing claim, instead of failing the whole wipe on the first collision.
  • A faulted vault-key clear no longer lets the identity's local record be purged anyway — the record is retained so the key stays discoverable for a retry, instead of becoming a silently orphaned entry that nothing can find again.
  • Unloading no longer requires the per-identity password even for password-protected identities — documented as intentional (deletion doesn't expose key material the way extraction does).
  • The confirmation dialog, CHANGELOG, and user story now disclose that unloading cancels the identity's queued scheduled DPNS votes, with a live count.
  • Unloading while a competing background load holds the identity's claim now returns an unload-worded message naming the identity, instead of a load-screen message telling the user to "load it again".
  • Several cleanup-failure and cleanup-residue messages were de-duplicated and made independently distinguishable (primary vs. associated cleanup vs. associated removal failure) instead of being conflated into one ambiguous flag.
  • "Clear Database" now also clears the durable per-network "forgotten identity" markers, so re-importing a wallet seed after a full wipe can rediscover identities that were unloaded before the wipe, instead of leaving them silently un-rediscoverable.

• Fix fix: dpns name voting #5 — retained records actually recover now, on every reload path: a masternode/evonode identity whose vault-key clear failed during unload (retained per Fix Feat/register usernames2 #4) previously had no way back — the "load it again" retry the app itself suggests was rejected as "already loaded" by the duplicate check, and the "Clear Database" sweep never reached it either, since it only walked the active index. Reloading such an identity now detects the retained/recoverable state, finishes the deferred cleanup, and proceeds with a fresh load; a genuinely already-loaded identity is still rejected as before. This recovery is wired into every path that can reload an identity — not just the masternode "Load" screen's duplicate-check, but overwriting, merging keys into an existing record, loading from a wallet, loading by DPNS name, and automatic wallet discovery too — so a leftover key from an interrupted unload is cleared before any of them writes a replacement. The full-wipe sweep now reaches and clears these retained records too, including ones whose "forgotten identity" marker had gone stale. Bulk identity removal (Identities screen "Remove") also now invalidates every affected profile-cache entry on the Identity Hub, closing a narrow stale-profile window there.

• Fix fix: active contest vote tallies were not being updated with refresh button #6 — wipe-completeness and disclosure accuracy, from a fresh review pass: a second round of automated review (bot + a full multi-agent security/QA/consistency audit of the resulting changes) found and fixed:

  • clear_network_database now holds every identity's cleanup claim until the entire wipe is done — including the legacy shielded-file cleanup and the in-memory wallet clears — instead of releasing each claim as soon as that one identity was deleted. A concurrent load can no longer restore an identity's data after the wipe's sweep has already passed it while the wipe still reports success; a failure while removing retired shielded files no longer skips resolving those claims either. One narrow, pre-existing exception remains — see Known limitations.
  • A delete that does not remember the unload (the ordinary "Clear Database" path, and the legacy migration path) now writes a safety-net "forgotten" marker if its cleanup tail fails, so the residue stays discoverable by a later retry instead of becoming invisible to every recovery path at once.
  • "Remove" on the Identities list and "Remove masternode" now show the exact same confirmation as Identity Hub → Settings, instead of wording that suggested the identity was merely being untracked. That confirmation: names the identity unambiguously even when its alias is shared with another identity, states what is actually deleted (private keys and the local entry) versus what a "Clear Database" is needed for (contacts, payment history), discloses that the app remembers the unload so automatic discovery does not bring the identity back, states the masternode/evonode-specific reload path (its ProTxHash, with private keys re-entered by hand if it held any) instead of the generic wallet-recovery wording that never applied to nodes, and still names a masternode's voting-identity removal as its own consequence.
  • The cleanup-failure error message now names the recovery that actually exists (reload the identity, then unload/remove it again) instead of suggesting a retry that has nothing left on screen to act on.
  • IDN-017 (this PR's new "unload" story) is renumbered to IDN-020 — it collided with a pre-existing, unrelated IDN-017 ("Top up identity from Platform addresses").

• Fix feat: hide document button #7 — the "forgotten identity" marker was itself broken: SQL against a frozen, read-only database, then a shared-state race: an architecture review caught that Fix Feat/register usernames2 #4's marker (added as a new table in data.db) was silently unwritable in practice — data.db is opened read-only in production once the file exists, and its migration ladder never runs against an existing install. That made unload/removal fail outright past an install's first boot, for every user, not a narrow edge case. The marker is now a DetKv entry — the same k/v mechanism the rest of identity_db.rs already uses — instead of a SQL table (docs/kv-keys.md catalogues the new key). An adversarial re-review of that fix then independently caught a second, self-introduced bug: a single shared key held every network's markers together, so concurrent record/clear operations on different identities could race and lose an update — silently resurrecting a cleared identity, or discarding a fresh unload. (Fix Feat/register usernames2 #4's original per-row SQL table had been immune to this by construction — independent rows don't collide.) Fixed by giving each identity's marker its own key, which removes the shared state the race depended on entirely, and incidentally closed two smaller issues the shared-blob design had: a single damaged marker no longer disables the guard for every identity on the network, and discovery no longer decodes the whole marker set once per identity it checks.

  Note for anyone who built a copy of this branch between Fix Feat/register usernames2 #4 and Fix feat: hide document button #7: your local data.db will have been stamped at schema version 39 by the (now-removed) migration step. Fix feat: hide document button #7 reverts to version 38, so on next launch data.db will report a version newer than the app expects and refuse to start. Delete data.db (or restore its automatic pre-update backup) in your app data directory before running a build from this branch again. This affects local dev/test installs only — nothing in this range was ever released.

• Fix feat: choose mn to vote with #8 — CI review pass (dialog consistency, removal feedback, discovery-marker resurrection) plus a QA follow-through: an automated CI review (github-actions[bot]) surfaced 3 new findings, all fixed:

  • The three unload/remove confirmation dialogs (Identity Hub → Settings, Identities screen, masternode detail) shared their message but not their buttons — the Identities screen used generic "Yes"/"No", and neither Remove dialog blocked input while open. Verb choice and input-blocking are now constructed from one shared, identity-type-keyed builder, so the three call sites cannot drift apart again.
  • Removing an identity from the Masternodes tab showed no feedback at all when cleanup left residue (owner/voter key not fully cleaned up) — identical to a fully successful removal. The removal-outcome banner is now set centrally wherever AppState dispatches RemovedIdentities, so every current and future caller gets it by construction instead of each screen needing its own copy.
  • A bulk "search this wallet up to index N" scan could silently restore ("un-forget") any unloaded identity it happened to re-derive along the way, with no confirmation and no mention in the result. Bulk wallet-index discovery no longer ever restores a forgotten identity, full stop; only a load aimed at that one specific identity (by its wallet index, identity ID, or username) still does, and the result now reports how many identities were left unloaded and how many failed to persist.
  • A follow-up multi-agent QA pass (security, project-consistency, and adversarial-QA reviewers) audited this fix and turned up 20 more findings (3 MEDIUM, 17 LOW, each independently verified against source rather than taken on trust) — two of them reached independently by more than one reviewer from different angles. Fixed: an evonode-specific dialog trigger that still hardcoded the old generic verb; a disclosure message that told a default-mode user to use a control hidden behind "Show Advanced Options"; two cleanup-failure flags that drove a persistent user warning but were never logged anywhere; a discovery-completion log and result that omitted the new skip/failure counts, so an all-failed bulk search still rendered as a plain green "Success"; a code comment claiming a "startup sweep" that does not exist; two docs/user-stories.md sentences that no longer matched the shipped behavior; a stale doc comment arguing for a banner pattern this round replaced; a missing regression test for the actual failure-path banner this round's fix introduces; a message-building function that spliced two independently-localized sentence fragments together, violating this project's own i18n-string rule; and — caught in a second, later pass — the unload confirmation's own disclosure text, which still said "automatic discovery does not bring it back" after this round made every discovery path (not just the automatic one) refuse to restore, at exactly the point a user commits to deleting private keys. Explicitly deferred, with rationale recorded: a stale UX-spec design-doc snapshot (a point-in-time record, not a living doc); a cosmetic module-naming inconsistency; an untested-but-currently-safe interaction between two modal dialog layers (speculative hardening, no defect found); and a test for one specific concurrency interleaving that static analysis and an existing adjacent test already show is safe (the shared per-identity load-claim mutex serializes it).
  • Also bundled: a pre-existing clippy::items_after_test_module failure in src/backend_task/platform_info.rs, inherited from v1.0-dev (introduced by an unrelated, separately-fixed PR) and blocking this branch's own CI — fixed as a pure, no-behavior-change relocation of the test module. A subsequent merge of v1.0-dev (which had by then gained the real, complete fix for the same file — the mechanical relocation plus an actual protocol-version bug fix) superseded this bundled commit's content entirely; the merge conflict was resolved by taking the upstream version, confirmed byte-for-byte.
  • Also fixed: a second, unrelated pre-existing failure — the contest-decision ETA tooltip (approximate_time_until) floored seconds-to-hours instead of rounding, so a target computed from one clock read and checked against a later, independent read could lose a whole display bucket to a few elapsed milliseconds (a 3-hour ETA rendering as "about 2 hours"). Fixed by rounding to the nearest hour/day instead of flooring; a deterministic regression test pins the exact near-boundary case so this doesn't depend on real-clock timing to catch a regression.

Closes #889

Known limitations

• The wipe-completeness guarantee in Fix fix: active contest vote tallies were not being updated with refresh button #6 covers a concurrent load of the same identity specifically. Other identity-mutating operations in flight during a wipe (refreshing an identity, adding a key, sending a transfer/withdrawal, and similar) do not yet take the same exclusion claim and are not guarded against; tracked as follow-up work.
• One narrow exception within the load-race guarantee itself: an identity that is both marked "forgotten" and still present in the index briefly reopens its claim between two internal steps of the wipe. A concurrent load can win that narrow window, but the wipe still correctly reports itself incomplete rather than succeeding silently — this is covered by a dedicated regression test.
• clear_identity_vault_keys still derives which vault entries to delete solely from the identity's stored blob; if that blob is ever missing (a pre-existing, narrower gap in insert_local_qualified_identity, unrelated to this PR's changes), nothing enumerates the vault directly to catch the orphaned key. Tracked as follow-up work on the secret-vault chokepoint.

Testing

• Full cargo test --all-features --lib on the merged tree: 2132 passed, 0 failed, 2 ignored (original PR scope).
• Fix Feat/register usernames2 #4's additions verified with targeted cargo test runs per touched module (identity_db, discover_identities, load_identity, remove_identity, detail_screen, list_screen, profile_screen, dashpay_screen, identity_load_registry, wallet_lifecycle, settings) — all green, including new regression coverage for each fix above.
• Fix fix: dpns name voting #5 verified independently (adversarial pass, not just the implementing pass): targeted cargo test for identity_db, forgotten_identities, wallet_lifecycle (clear_network_database), load_identity (reject-if-exists), and hub_screen, including new coverage for a repaired reload, a still-broken reload, marker-only residue, a full wipe reaching retained records, and profile-cache invalidation on bulk removal — all green. A second, adversarial verification pass then found and confirmed (by an executed test reproduction, not just static review) that the recovery only ran on the masternode Load screen's path; targeted cargo test for load_identity, load_identity_from_wallet, load_identity_by_dpns_name, and discover_identities now cover all five ways an identity can be reloaded, each asserting the old vault key is actually cleared before the replacement is written — all green.
• Fix fix: active contest vote tallies were not being updated with refresh button #6 verified across three independent adversarial passes (not just the implementing pass), each re-verifying the prior passes' work rather than trusting it: a wide targeted cargo test scope (wallet_lifecycle, identity_db, identity/settings, identities_screen, masternode detail_screen, backend_task::error, unload_identity) reached 277 passed, 0 failed on the final commit, confirmed via two independent from-scratch builds to rule out a stale-cache false green. New regression coverage includes the wipe holding claims to the end even when legacy-file cleanup fails, the safety-net marker recovering after an injected vault-key-clear fault, the id-disambiguation and node-restoration wording asserted on the full composed message (not just the appended fragment, which is what let one interim fix's contradiction slip through unnoticed until the next adversarial pass executed it), and the forgotten-and-indexed claim hand-off degrading to a reported failure rather than a silent success.
• Fix feat: hide document button #7 verified in two stages, each with a real negative control (implementation reverted, confirmed the test fails with the exact reported symptom, then restored to confirm green) rather than a static read of the diff: the data.db → DetKv migration, and — after two independent reviewers (security + project-consistency) flagged the same concurrency defect from different angles — the shared-key race fix, proven via a dedicated test running 24 concurrent record/clear operations across distinct identities against the real persister and asserting none were lost either direction. cargo build --all-features, the full scoped test set across identity_db, wallet_lifecycle, backend_task::identity, and database::initialization (206 tests, all passing by name, independently re-run in the merged branch, not just accepted from the implementing pass), cargo fmt --all -- --check, and cargo clippy --all-features --all-targets -- -D warnings all clean.
• Fix feat: choose mn to vote with #8 verified through the same discipline as prior fixes: RED-first regression tests for all 3 CI-reported findings (each confirmed failing against pre-fix code, then passing), independently re-run — not accepted from the implementing pass — including a RED-first negative control on the discovery-resurrection fix specifically. The follow-up QA pass's own coverage gap (the failure-path removal banner had no execution-level test) was closed with a new kittest, and both new/changed kittests were mutation-checked (reverting the fix locally reproduces the exact failure the test exists to catch, not just observed-green). Independent verification, twice: once on the round-9 worktree tip, again on the branch after merging a diverged v1.0-dev — both clean (cargo clippy --all-features --all-targets -- -D warnings, cargo test --lib --all-features — 2221 passed, cargo test --test kittest --all-features full suite — 290 passed, cargo fmt --all -- --check).
• The ETA-tooltip rounding fix verified with a real negative control (implementation reverted, the new deterministic test confirmed failing with the exact reported symptom — "about 2 hours" instead of "about 3 hours" — then restored to confirm green), plus the originally-reported flaky test (ui::components::pill::tests::tooltip_includes_eta_when_decision_time_is_in_the_future) re-run and confirmed passing.
• cargo fmt --all -- --check clean.
• cargo clippy --all-features --tests -- -D warnings clean.

Breaking changes

None.

Checklist

• Tests added/updated
• cargo fmt --all
• cargo clippy --all-features --tests -- -D warnings
• CHANGELOG.md updated
• docs/user-stories.md updated (IDN-020)

Attribution

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds an identity unload flow that removes local identity state and DashPay sidecars, tracks deliberately unloaded identities, reconciles wallet and UI state, prevents stale profile restoration, and validates identity types during loading.

Changes

Identity operations

Layer / File(s)	Summary
Local cleanup and recovery semantics src/context/identity_db.rs, src/wallet_backend/dashpay.rs, src/model/qualified_identity/*, src/database/*	Identity unload clears local keys, metadata, DashPay sidecars, and index entries while preserving sibling identities; cleanup failures are accumulated and forgotten identities are persisted per network.
Backend unload task and removal integration src/backend_task/identity/*, src/backend_task/error.rs, src/backend_task/mod.rs, src/context/wallet_lifecycle/spv.rs	Adds unload dispatch and structured outcomes, reconciles cached identities and selection state, distinguishes cleanup-only failures, and updates shared removal handling.
Identity loading and discovery corrections src/backend_task/identity/*, src/context/wallet_lifecycle/bootstrap.rs	Adds explicit discovery modes, preserves forgotten markers during background discovery, validates identity types, and permits loading into bare placeholders.
Unload confirmation and stale profile invalidation src/ui/identity/settings.rs, src/ui/identity/hub_screen.rs, src/ui/identity/profile_cache.rs, src/ui/dashpay/*	Enables recovery-aware confirmation, dispatches the captured identity target, reports results, and rejects late profile responses.
Related removal UI and documentation src/ui/masternodes/*, src/ui/identities/identities_screen.rs, CHANGELOG.md, docs/user-stories.md	Routes masternode removal through backend tasks, updates removal feedback and navigation, and documents unload behavior and loading fixes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SettingsTab
  participant IdentityTask
  participant IdentityDatabase
  participant WalletCache
  participant ProfileCache
  SettingsTab->>IdentityTask: dispatch unload task
  IdentityTask->>IdentityDatabase: remove local identity state
  IdentityDatabase-->>IdentityTask: return success or cleanup error
  IdentityTask->>WalletCache: evict identity and clear selection
  IdentityTask-->>SettingsTab: return unload result
  SettingsTab->>ProfileCache: invalidate identity profile

Loading

Possibly related PRs

• dashpay/dash-evo-tool#842: Introduced the Identity Hub UI that handles the unload result.
• dashpay/dash-evo-tool#927: Touches the identity removal pipeline, result shape, and removal UI updated by this change.

Suggested reviewers: lklimek

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the two main changes: fixing masternode/evonode identity loading and adding identity unload support.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-889-masternode-identity-lifecycle

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Claudius-Maginificent]

Follow-up push (ea2d0b62): a fresh adversarial pass on the identity-unload path (post-review of e984641b/16fbc1b4) surfaced two more real gaps, both now fixed:

• delete_local_qualified_identity cleared vault keys (irreversible) before removing the identity from the index. A failure in either of the two steps that followed could leave a "zombie" identity — still visible, but with its keys already gone. Reordered so the index entry drops first; added a regression test that forces a real clear_identity_vault_keys failure (a corrupted stored blob) and asserts the identity is already hidden despite it.
• unload_identity() propagated wallet-lock poisoning via ?, inconsistent with this exact lock's documented self-healing convention elsewhere in the codebase. Routed through the existing read_recover/write_recover helpers instead.

66/66 targeted tests green, cargo fmt/clippy --all-features --lib -D warnings clean.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

⛔ Blockers found — Sonnet deferred (commit 06dfbc3)
Canonical validated blockers: 4

[claude]

Claudius reviews the identity-lifecycle fixes 🛠️

I ran the full grumpy trio (security, project, QA) plus a docs pass over the whole diff. Credit where it's due: the core deadlock fix is genuinely good work. validate_loaded_identity_type / identity_carries_owner_key are pure, lock-free, and correctly placed in model/; is_bare_placeholder reopens a bare User record for a Masternode re-load without racing; and — I checked this myself rather than trust the commit messages — both the deadlock repro test and the deletion-fault regression test are real, non-vacuous guards, not tautologies dressed up as coverage. The load registry gives delete a proper exclusive claim, and the confirmation dialog captures its target at open time. Tidy.

The failure paths of the new unload feature are where a few things slipped. Two MEDIUM findings posted inline:

1. Deletion ordering orphans key material (identity_db.rs:987-990) — index-before-key-clear avoids a visible zombie but, on a clear fault, leaves plaintext-recoverable keys on disk with no reachable path to remove them (Settings hides it, the network-clear sweep skips it, the banner tells the user to do the impossible). Wants atomicity, or key-destruction-first ordering.
2. CHANGELOG + stale tooltip overstate recoverability (CHANGELOG.md:11-15, settings.rs:61-62) — "loaded again at any time" contradicts an irreversible key deletion; TIP_UNLOAD still carries the pre-fix optimistic wording the dialog was moved away from, so the screen shows two contradictory risk statements.

9 LOW findings are in the full report (not posted inline to keep the noise down) and worth a glance before merge — a few are quick wins:

• Lock-poisoning fix left a sibling pending_identity_selection.lock()? un-recovered in the same function (unload_identity.rs:32) — reproduces the exact class the commit claims to close.
• WalletBackendNotYetWired arm reports Ok while skipping DashPay/metadata cleanup that resurfaces if the id is reloaded (identity_db.rs:949-980).
• Two removal paths diverge on in-memory cache/selection cleanup; the reused IdentityLoadInProgress message now misdescribes delete/unload collisions; a committed test name embeds an ephemeral code_001_ review ID; stale rustdoc; one bare ? amid three wrapped siblings; and a second CHANGELOG over-claim ("every piece of locally stored data" vs documented residuals).

No CRITICAL or HIGH. Nothing here is a merge-blocker in the catastrophic sense, but the two MEDIUM items touch a destructive, irreversible operation's failure modes and risk communication, so I'd square those away first. Holding the approval until they're addressed.

_{🤖 Reviewed by Claudius the Magnificent — full report (2 MEDIUM, 9 LOW) archived with the run.}
📊 View full HTML review report

[claude]

Claudius returns to the identity-lifecycle fixes 🛠️

Fresh full pass — security, project, QA, and docs specialists over the whole diff. First, credit where it's earned: the two MEDIUMs from the previous round are genuinely settled. The deletion-ordering tradeoff got its maintainer ruling (index-first/keys-last, keep key material persisted longest for disaster recovery — respected, not re-litigated), and the reversibility copy is now consistent across tooltip, dialog, and CHANGELOG. The core issue #889 fix remains tidy: pure lock-free validators in model/, is_bare_placeholder reopening a bare User record without racing, and — verified against the pre-fix code, not the commit messages — non-vacuous deadlock-repro and deletion-fault tests.

One new MEDIUM, posted inline:

• Secret-erasure regression on the full-wipe path (identity_db.rs:949-990) — folding DashPay cleanup into delete_local_qualified_identity with fail-fast ? ahead of clear_identity_vault_keys means a k/v-store fault can now abort before the private-key wipe on the "delete all local data" (F60) path. Pre-PR those were independent best-effort steps and the wipe always ran. Confirmed against the base diff; untested coupling.

9 LOW in the archived report (not posted inline, to keep the noise down) — several are quick wins and a handful are prior-round carry-overs still standing: the poison-propagating ? on pending_identity_selection (sibling of the lock this PR did fix), the WalletBackendNotYetWired branch that reports success while skipping cleanup, a narrow load/unload resurrection race, uneven error-wrapping across the seven cleanup steps, the code_001_ ephemeral review ID still baked into a committed test name, the CHANGELOG's "every piece of locally stored data" over-claim vs. its own documented residuals, an over-broad "you'll need recovery information" warning for wallet-derived identities, the reused "This node is already being loaded" message now reachable for a plain-identity unload, and a duplicated test helper.

No CRITICAL or HIGH. Nothing catastrophic — but the MEDIUM touches a destructive, security-critical secret-deletion guarantee, so I'd square it away (or make an explicit decision on it, as with the last one) before merge. Holding the approval until then.

_{🤖 Reviewed by Claudius the Magnificent — full report (1 MEDIUM, 9 LOW) archived with the run.}
📊 View full HTML review report

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/user-stories.md`:
- Around line 653-655: Update the Identity Hub unloading documentation to make
the permanent private-key deletion warning conditional: wallet-derived
identities should show a restoration message, while recovery information should
be required only for identities with non-wallet-derived keys. Preserve the
existing statements about removing only the selected identity and retaining
other wallet data.

In `@src/backend_task/identity/unload_identity.rs`:
- Around line 21-27: The `?` after `delete_local_qualified_identity` prevents
in-memory reconciliation when cleanup fails after identity storage has already
been deleted. Update `unload_identity` to distinguish load/storage errors from
`IdentityUnloadCleanupFailed`, continue wallet-cache eviction and
selection/pending cleanup for the latter, then return that cleanup error after
reconciliation while preserving short-circuiting for other errors.

In `@src/ui/identity/hub_screen.rs`:
- Around line 539-545: Update the BackendTaskSuccessResult::UnloadedIdentity arm
or record_result flow to remove the unloaded identity from ProfileCache before
displaying the success banner. Add and use a
ProfileCache::remove_identity(&Identifier) helper, or equivalent logic, so
subsequent hub reloads treat that identity as a cache miss and do not reuse
stale DashPay data.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b1b27947-2b98-473b-9f05-47174d2566cc

📥 Commits

Reviewing files that changed from the base of the PR and between e438372 and 6d05025.

📒 Files selected for processing (14)

• CHANGELOG.md
• docs/user-stories.md
• src/backend_task/error.rs
• src/backend_task/identity/load_identity.rs
• src/backend_task/identity/mod.rs
• src/backend_task/identity/unload_identity.rs
• src/backend_task/mod.rs
• src/context/identity_db.rs
• src/context/wallet_lifecycle/spv.rs
• src/model/qualified_identity/encrypted_key_storage.rs
• src/model/qualified_identity/mod.rs
• src/ui/identity/hub_screen.rs
• src/ui/identity/settings.rs
• src/wallet_backend/dashpay.rs

💤 Files with no reviewable changes (1)

• src/context/wallet_lifecycle/spv.rs

[thepastaclaw]

Preliminary review — Codex only

The PR resolves the original bare-record deadlock and adds substantial unload cleanup, but three blocking state-integrity gaps remain. Node loads can still misclassify regular identities, wallet discovery can automatically restore identities the user unloaded, and cleanup-only failures leave persistent and in-memory identity state inconsistent; the documentation and profile cache also need smaller unload-related corrections.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 blocking | 🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/backend_task/identity/load_identity.rs`:
- [BLOCKING] src/backend_task/identity/load_identity.rs:80-90: Reject regular identities loaded through the masternode entry point
  The new post-fetch validation enforces only one direction: a User load rejects an identity with an OWNER key, but a Masternode or Evonode load accepts an ordinary identity without one. The node form permits a read-only load with every private-key field blank, so no later key check catches the mismatch. The fetched identity is then persisted with a node type and omitted from Identity Hub. This is especially relevant to this PR because the new bare-placeholder exception can now let such a node load proceed where the duplicate check previously stopped it. Enforce the reciprocal invariant, return a new actionable TaskError explaining that this is a regular identity and must be loaded from Identity Hub, and add tests for both node types without an OWNER key.

In `src/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:944-948: Prevent discovery from automatically restoring unloaded wallet identities
  The deletion record written here only protects a pending legacy-migration retry and becomes a no-op once migration is Ready or successful. Normal startup and wallet-unlock discovery never consults it, so a wallet-derived identity that the user unloads can be found on Platform again and reinserted by upsert_discovered_identity, contradicting the UI's promise that it can be loaded again later. An already-running discovery can also reinsert the identity immediately because discovery does not participate in the per-identity load registry used by unload. Persist a durable per-network forgotten-identity marker, check it before discovery stores a result, coordinate discovery persistence with the same per-identity exclusion mechanism, and clear the marker only after a successful explicit load. Add coverage for both a later discovery sweep and a discovery/unload race.

In `src/backend_task/identity/unload_identity.rs`:
- [BLOCKING] src/backend_task/identity/unload_identity.rs:21: Finish in-memory eviction when persistent deletion completed with a cleanup error
  IdentityUnloadCleanupFailed is returned only after delete_local_qualified_identity has successfully removed the index entry, wiped the vault keys, and purged the identity blob; it represents a failure in ancillary DashPay or sidecar cleanup. The `?` nevertheless exits before wallet-cache eviction and selected/pending-selection cleanup. The identity is therefore permanently absent from storage but remains active and potentially selected in memory, while the normal identity list can no longer expose it for another unload attempt. Preserve the cleanup error, complete the in-memory reconciliation for this specific committed-deletion outcome, and return the preserved error afterward; continue short-circuiting for failures that occur before deletion commits.

In `src/ui/identity/hub_screen.rs`:
- [SUGGESTION] src/ui/identity/hub_screen.rs:518-545: Treat unload as a cache miss for the unloaded identity
  UnloadedIdentity is not consumed by ProfileCache::record_result and this match arm only displays a banner, leaving the identity's profile in `loaded`. If the identity is explicitly loaded again during the same hub session, the old profile is reused instead of being fetched again, despite the changelog stating that unload removes its cached profile. Invalidation must also cover queued/requested state and an in-flight LoadProfile result. Because DashPayProfile carries no owner ID, simply clearing `in_flight` would allow a late result to be associated with a different request; retain enough generation or tombstone state to discard the unloaded identity's late result safely.

In `docs/user-stories.md`:
- [SUGGESTION] docs/user-stories.md:653: Make the private-key warning conditional
  The implemented dialog now distinguishes wallet-derived identities, whose keys can be restored from the retained wallet, from identities containing keys stored only on this device, which require recovery information. This user story still says all local private keys are permanently deleted, so it no longer describes the exact-head behavior. Update it to state that the confirmation explains which recovery path applies while preserving the statements about identity-scoped cleanup and retained wallet data.

[github-actions]

Review — 25 findings (0 critical, 0 high, 6 medium, 15 low, 4 info)

Four specialists went over this statically — security, project consistency, adversarial QA, and documentation. No build or test was run: this sandbox has no working shell, and this repo's own CI is the full-suite backstop anyway. Everything below rests on reading source at f0a829f; anything I couldn't settle that way is labelled unverified rather than dressed up as certainty.

Let me start with the part that surprised me: most of it holds. IdentityLoadGuard/IdentityLoadToken is genuinely well-built — Drop records Failed on every ? and on panic, and the token check means a superseded load can't publish someone else's outcome. The bare-placeholder check and bidirectional type validation are unit-tested in all four directions. All five reload paths were independently verified to clear the old vault key before writing a replacement — I read every call site rather than taking the PR body's word for it, and the claim survives. Error variants follow the TaskError rules to the letter. The schema revert 39→38 is coherent with no orphaned table, and docs/kv-keys.md matches the implementation exactly. The fault-injection tests install real SQLite triggers instead of mocking, which is how it should be done and rarely is.

Two findings should be settled before merge, both because they contradict claims the PR itself makes:

• SEC-001 — clear_network_database's completeness guarantee is bounded by the single local_identity_ids() snapshot at spv.rs:203. An identity persisted after that snapshot — by an in-flight masternode load, or the discovery sweep — survives the wipe with its keyless-tier owner/voting/payout keys, and the wipe still returns Ok(()). No attacker needed: "this load looks hung, let me wipe local data and start clean" is the natural operator sequence. This is the same defect as residual #1 on the still-open SEC-003 thread, so I've left it there rather than opening a fourth comment about it.
• PROJ-002 — removing a masternode produces no banner at all. The three partial-cleanup failure flags this PR added are consumed on exactly one screen, and it isn't the one the target persona uses. Posted inline.

Four further MEDIUMs are worth attention but a follow-up can carry them: PROJ-001 ("Yes"/"No" on the destructive dialog, against ux-design-patterns.md:97), PROJ-003 (a bulk wallet search silently resurrects unloaded identities, undocumented in both directions), RUST-001 (clear_network_database is still 309 lines — and SEC-001 is exactly the kind of defect that structure hides), and SEC-002 (the blob-derived key inventory, now honestly documented as a Known limitation, which I respect, but a documented hole is still a hole).

On the four prior threads: I re-verified all of them against f0a829f and replied on each. None are fully fixed. The wipe-claim-lifetime and function-sprawl threads are genuinely half-done — real progress, real residue. The marker-privacy thread got its disclosure (tested, verbatim in every dialog variant) but not its data minimisation. The key-inventory thread is unchanged. All four stay open; three of this review's MEDIUMs are those same defects and are deliberately not re-posted as new comments.

Full report with all 25 findings, evidence, and permalinks: review-report/report.html.

This is ambitious, careful work, and the findings below are the residue of that ambition rather than evidence of carelessness. Fix the two blockers and I'll be delighted to stop complaining.

_{🤖 Reviewed by Claudius the Magnificent — Grand Admiral of Code, Lord of All Compilers}

[thepastaclaw]

Preliminary review — Codex only

Carried forward: the persistent DashPay Profile screen still retains an identity unloaded while that screen is hidden; the two prior full-wipe findings are fixed at this head. Newly identified in the latest-delta review: the unload confirmation overstates restoration semantics, the wipe does not coordinate with loads absent from its identity snapshots, and a failed safety-marker write can strand cleanup residue. These four in-scope blockers require changes.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 4 blocking

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/ui/dashpay/profile_screen.rs`:
- [BLOCKING] src/ui/dashpay/profile_screen.rs:192-220: Reconcile the persistent profile screen after a hidden identity unload
  Root screens persist while hidden, but unload and removal results are delivered only to `visible_screen_mut()` in `src/app.rs:2699-2700` and `2714-2715`. Returning to the Profile root calls `refresh_on_arrival()`, yet `refresh()` reads local identities only when `selected_identity` is already `None`; it never checks whether an existing selection still appears in `load_local_user_identities()`. If identity A is unloaded from another screen while identity B remains loaded, Profile therefore retains A's profile, editor, wallet, confirmation, selector string, and request generation. The stale nonempty selector also prevents `IdentitySelector` from applying its fallback, so the screen can continue dispatching work for an identity no longer stored locally. Reconcile the selected ID against storage on every arrival and clear all identity-scoped state and in-flight generations when it has disappeared.

In `src/ui/identity/settings.rs`:
- [BLOCKING] src/ui/identity/settings.rs:1230-1244: Do not claim that every search leaves the identity unloaded
  The destructive confirmation says that "no search brings it back," but the supported targeted wallet-index restoration is presented to users as "Search For Identity" in `src/ui/identities/add_existing_identity_screen.rs:663-711`. That path runs `load_user_identity_from_wallet`, persists the selected identity, and calls `finish_identity_load_after_persist` at `load_identity_from_wallet.rs:272`, which clears its forgotten marker. `docs/user-stories.md:685-687` also states that a load aimed at the identity's own wallet index brings it back. Because this disclosure appears immediately before private-key deletion, distinguish whole-wallet discovery from a targeted search for the specific identity instead of making a false absolute promise.

In `src/context/wallet_lifecycle/spv.rs`:
- [BLOCKING] src/context/wallet_lifecycle/spv.rs:203-255: Block loads that have not reached the identity-index snapshot
  The wipe acquires per-identity claims only for IDs returned by the forgotten-marker and local-index snapshots. A generic identifier load acquires its `IdentityLoadGuard` before its network fetch at `load_identity.rs:161`, while the identity remains absent from the index until persistence at `load_identity.rs:548`. If the wipe takes its owners snapshot during that interval, it never sees or claims the ID; the load can then persist its blob and private keys after the snapshot while the wipe continues, and `clear_network_database` can still return success with that late identity present. Backend tasks run concurrently, and the SystemTask path has no global load/wipe exclusion. Add a global wipe/load barrier, drain all submitted and running identity loads followed by a resweep, or provide equivalent coordination so the PR's stated concurrent-load wipe guarantee also covers identities absent from the initial snapshots.

In `src/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:1271-1286: Do not swallow failure to persist the cleanup recovery marker
  After the identity has been removed from the global index, this failure path relies on `record_forgotten_identity` as the durable enumeration entry for any retained blob and vault-key inventory. If cleanup fails while private keys remain and the marker write also fails, the second error is only logged and the function returns the original cleanup error. Later reload recovery and full wipes enumerate only `local_identity_ids()` and forgotten markers, so they cannot discover this unindexed, unmarked residue and may report success while private keys remain. Establish the recovery entry before removing the index and retire it only after successful cleanup, or otherwise guarantee a durable fallback when the marker write fails; logging the second failure does not preserve the destructive-path invariant.

[thepastaclaw]

🔴 Blocking: Do not claim that every search leaves the identity unloaded

The destructive confirmation says that "no search brings it back," but the supported targeted wallet-index restoration is presented to users as "Search For Identity" in src/ui/identities/add_existing_identity_screen.rs:663-711. That path runs load_user_identity_from_wallet, persists the selected identity, and calls finish_identity_load_after_persist at load_identity_from_wallet.rs:272, which clears its forgotten marker. docs/user-stories.md:685-687 also states that a load aimed at the identity's own wallet index brings it back. Because this disclosure appears immediately before private-key deletion, distinguish whole-wallet discovery from a targeted search for the specific identity instead of making a false absolute promise.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Block loads that have not reached the identity-index snapshot

The wipe acquires per-identity claims only for IDs returned by the forgotten-marker and local-index snapshots. A generic identifier load acquires its IdentityLoadGuard before its network fetch at load_identity.rs:161, while the identity remains absent from the index until persistence at load_identity.rs:548. If the wipe takes its owners snapshot during that interval, it never sees or claims the ID; the load can then persist its blob and private keys after the snapshot while the wipe continues, and clear_network_database can still return success with that late identity present. Backend tasks run concurrently, and the SystemTask path has no global load/wipe exclusion. Add a global wipe/load barrier, drain all submitted and running identity loads followed by a resweep, or provide equivalent coordination so the PR's stated concurrent-load wipe guarantee also covers identities absent from the initial snapshots.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Do not swallow failure to persist the cleanup recovery marker

After the identity has been removed from the global index, this failure path relies on record_forgotten_identity as the durable enumeration entry for any retained blob and vault-key inventory. If cleanup fails while private keys remain and the marker write also fails, the second error is only logged and the function returns the original cleanup error. Later reload recovery and full wipes enumerate only local_identity_ids() and forgotten markers, so they cannot discover this unindexed, unmarked residue and may report success while private keys remain. Establish the recovery entry before removing the index and retire it only after successful cleanup, or otherwise guarantee a durable fallback when the marker write fails; logging the second failure does not preserve the destructive-path invariant.

_{source: ['codex']}

[lklimek]

replaced by #941